averageMagnitude math issue allows early Depositors to decides the max duration against everyone else in a disproportionate way

[c4-bot-7]

Lines of code

https://github.com/Tapioca-DAO/tap-token/blob/050e666142e61018dbfcba64d295f9c458c69700/contracts/governance/twTAP.sol#L323

Vulnerability details

Impact

As I flagged on the Spearbit report averageMagnitude is not a real average

https://github.com/Tapioca-DAO/tap-token/blob/050e666142e61018dbfcba64d295f9c458c69700/contracts/governance/twTAP.sol#L323

pool.averageMagnitude = (pool.averageMagnitude + magnitude) / pool.totalParticipants;

The logic for moving the pool.cumulative is as follows:
https://github.com/Tapioca-DAO/tap-token/blob/050e666142e61018dbfcba64d295f9c458c69700/contracts/governance/twTAP.sol#L328-L337

            if (divergenceForce) {
                pool.cumulative += pool.averageMagnitude;
            } else {
                // TODO: Strongly suspect this is never less. Prove it.
                if (pool.cumulative > pool.averageMagnitude) {
                    pool.cumulative -= pool.averageMagnitude;
                } else {
                    pool.cumulative = 0; /// @audit Needs to be EPOCH_DURATION
                }
            }

Which uses this distorted average

This means that as more lockers lock their tokens, the average will be harder to move.

More specifically, the first 100 or so lockers have a disproportionally high impact on the max duration of locks

To the point where, the first 100 depositors will determine the max and min duration practically forever

That's because the math is not moving the cumulative by the delta against the averageMagnitude, but by the absolute value of averageMagnitude

When we have !divergenceForce that number is extremely small

When we have divergenceForce the number is big, but because we are dividing the Previous Average by the total number of lockers, then the growth of averageMagnitude is tampered

This allows the first few depositors or a malicious deposit to sabotage the lock duration and force it to either be:

• Extremely long, by locking 100 small locks (1k tapioca), which forces all lockers to lock for 4 years
• Extremely short, by spamming short locks, the maximum possible longest lock becomes 3 months

Fundamental issues with the formula

The formula is snappy at first and then becomes very slow to change

Once 100 lockers are there, it's basically impossible to move it

This means that

Logs:
  averageMagnitude 126230400
  cumulative 126230401
  averageMagnitude 1767492
  cumulative 124462909
  averageMagnitude 535058
  cumulative 124997967
  averageMagnitude 516354
  cumulative 125514321
  averageMagnitude 509741
  cumulative 126024062
  averageMagnitude 503395
  cumulative 126527457
  averageMagnitude 497196
  cumulative 126030261
  averageMagnitude 493852
  cumulative 126524113
  averageMagnitude 487910
  cumulative 126036203
  averageMagnitude 4489
  cumulative 126031714
  averageMagnitude 54
  cumulative 126031660
  averageMagnitude 13
  cumulative 126031647
  averageMagnitude 13
  cumulative 126031634
  averageMagnitude 12
  cumulative 126031622
  averageMagnitude 12
  cumulative 126031610
  averageMagnitude 12
  cumulative 126031598
  averageMagnitude 12
  cumulative 126031586
  averageMagnitude 12
  cumulative 126031574

 function test_twapWithHardocodedValues_closeButNotSigar_hasMassiveImpact() public {
        TwTap twtap = new TwTap();

        // Set twTAP to have some participation
        // To force lockers for 4 years for max
        // 100 people
        twtap.setTotalParticipants(100);
        // 4 years
        twtap.setaAverageMagnitude(365.25 days * 4);
        twtap.setCumulative(365.25 days * 4 + 1); /// @audit +1 so we can try to reduce it
        _logTwTap(twtap);

        // Can we write one operation that causes this to go to zero?
        twtap.participate(365.25 days * 4, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(365.25 days * 4, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(365.25 days * 4, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(365.25 days * 4, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(365.25 days * 4, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(365.25 days * 4, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(365.25 days * 4, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(365.25 days * 4, twtap.getMinWeight());
        _logTwTap(twtap);

        // Decreases slowly af
        twtap.participate(twtap.EPOCH_DURATION() + 1, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(twtap.EPOCH_DURATION() + 1, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(twtap.EPOCH_DURATION() + 1, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(twtap.EPOCH_DURATION() + 1, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(twtap.EPOCH_DURATION() + 1, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(twtap.EPOCH_DURATION() + 1, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(twtap.EPOCH_DURATION() + 1, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(twtap.EPOCH_DURATION() + 1, twtap.getMinWeight());
        _logTwTap(twtap);
        twtap.participate(twtap.EPOCH_DURATION() + 1, twtap.getMinWeight());
        _logTwTap(twtap);
    }

When we have 10 lockers we can move it a bit

  averageMagnitude 126230400
  cumulative 126230401
  averageMagnitude 16228794
  cumulative 110001607
  averageMagnitude 6138512
  cumulative 116140119
  averageMagnitude 4732975
  cumulative 120873094
  averageMagnitude 4187821
  cumulative 125060915
  averageMagnitude 3787908
  cumulative 128848823
  averageMagnitude 3457302
  cumulative 125391521
  averageMagnitude 3293549
  cumulative 128685070
  averageMagnitude 3048294
  cumulative 125636776
  averageMagnitude 160513
  cumulative 125476263
  averageMagnitude 8098
  cumulative 125468165
  averageMagnitude 455
  cumulative 125467710
  averageMagnitude 86
  cumulative 125467624
  averageMagnitude 67
  cumulative 125467557
  averageMagnitude 63
  cumulative 125467494
  averageMagnitude 60
  cumulative 125467434
  averageMagnitude 58
  cumulative 125467376
  averageMagnitude 56
  cumulative 125467320

In conclusion

The formula is extremely elastic initially

And it reaches a point where changes become exponentially more expensive too fast

I believe that once a value is set by the first 10 or so people

Everyone else will be forced into that and the formula will no longer be adaptable

Since the average is not changed on releaseTap (dangerous change imo), this means that the first 10 or so depositors can choose for everyone else, in perpetuity as the next avg change is too miniscule to have any drastic impact

Mitigation

It's worth determining whether the logic for shifting max lock duration is necessary, or if it's better to opt for a simpler max and then using the logic simply to determine the multiplier.

Overall, the amount of multiplications makes the math a lot more complicated without a clear benefit

I would recommend:

• Use real weighted average (current one is not average and is not weighted by value, it only considers time)
• Simulate not just the math from the Whitepaper, but the implementation in the code

Assessed type

Math

[c4-sponsor]

0xRektora (sponsor) disputed

[c4-sponsor]

0xRektora (sponsor) acknowledged

[c4-sponsor]

0xRektora marked the issue as disagree with severity

[0xRektora]

I'd see this as informational.
While the scenario you used works in a sandboxed environment, I don't think this wouldn't happen in the real world.
You have to take into account the minimum weight to have a divergent force, which requires a lot of capital, then you have to take into account the incentives for someone to attack and spam lock. Lastly it was intended for the formula to be difficult to move the longer it becomes, in terms of decay, you also have to take into consideration the position expiries, which would drastically decrease the cumulative.

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as grade-a