code423n4 commented 1 year ago

Lines of code

Vulnerability details

Proof of Concept

getUserBoost computes the max amount of boost that a user had for all the gauges.

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Boost.sol#L31

This value will be used on every transfer and burn to check the free gauge boost.

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Boost.sol#L341-L344

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Boost.sol#L312-L314

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Boost.sol#L323-L330

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Boost.sol#L302-L304

Currently getUserBoost only gets incremented/decremented when calling updateUserBoost() and it gets reset when calling decrementAllGaugesAllBoost(). This is the behavior described in this comment.

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Boost.sol#L150-L172

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Boost.sol#L249

However, this will result in the value for getUserBoost getting out-of-sync when calling other functions that decrement gauges, e.g. decrementGaugeBoost(), decrementGaugeAllBoost(), decrementAllGaugesBoost() and decrementGaugesBoostIndexed(). If the removed gauge boost is from the max boost previously computed to the user, the user gauge value will be incorrect when calling the transfer function.

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Boost.sol#L175-L187

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Boost.sol#L190-L195

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Boost.sol#L198-L200

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Boost.sol#L203-L227

Even if only decrementing getUserBoost when calling decrementAlGaugesAllBoost() is expected, there's still a possible edge case when calling decrementAllGaugesBoost() with a value that resets all the boost for the gauge with the max value.

The following POC demonstrates that if decrementAllGaugeBoost() is called with a value that resets the freeGaugeBoost to zero, the transfer function will revert.

function test_decrementAllGaugesToZero_userBoostOutOfSync() public {
    // will attach 100e18 for each gauge
    testAttachTwoGauges();

    // decrement all boost by passing 100e18
    hevm.prank(address(1));
    token.decrementAllGaugesBoost(100 ether);

    (uint128 newUserBoost,) = token.getUserGaugeBoost(address(1), gauge1);
    (uint128 newUserBoost2,) = token.getUserGaugeBoost(address(1), gauge2);

    require(newUserBoost == 0, "userBoost not updated");
    require(newUserBoost2 == 0, "userBoost not updated");

    // getUserBoost should be 0 as the boost is now 0 for all the gauges
    // but it will incorrectly still be marked as 100e18
    require(token.getUserBoost(address(1)) == 100 ether, "total userBoost doesn't match");

    // freeGaugeBoost() computes balanceOf[user] - getUserBoost[user]
    // it should return 100 but it will mistakenly return 0
    // this function will be called on every transfer on the notAttached modifier
    require(token.freeGaugeBoost(address(1)) == 0);

    // as a result, the transfer will fail, but it should work
    hevm.expectRevert(IERC20Boost.AttachedBoost.selector);
    token.transfer(address(2), 100 ether);
}

To demonstrate that the transfer function should work, we can take the existing test testDecrementAllGaugesAllBoost, and add two lines on the bottom to make the transfer.

function test__decrementAllGaugesAllBoost__withTransfer() public {
    testAttachTwoGauges();
    hevm.prank(address(1));
    token.decrementAllGaugesAllBoost();
    (uint128 newUserBoost,) = token.getUserGaugeBoost(address(1), gauge1);
    (uint128 newUserBoost2,) = token.getUserGaugeBoost(address(1), gauge2);
    require(newUserBoost == 0, "userBoost not updated");
    require(newUserBoost2 == 0, "userBoost not updated");
    require(token.getUserBoost(address(1)) == 0 ether, "total userBoost doesn't match");

    // will not revert
    hevm.prank(address(1));
    token.transfer(address(2), 100 ether);
}

Impact

The value for freeGaugeBoost() will be incorrect on every transfer until the function updateUserBoost() gets called. It is possible that this function doesn't get called and freeGaugeBoost() is out-of-sync for days/weeks.

getUserBoost will get a corrupted value, the transfer and burn functions will stop working/work unexpectedly for ERC20Boost, potentially resulting in DOS.

Tools Used

Manual review, foundry/forge.

Recommended Mitigation Steps

My recommendation is to update getUserBoost every time decrementGaugeBoost(), decrementGaugeAllBoost(), decrementAllGaugesBoost() and decrementGaugesBoostIndexed() are called, since the max boost for a gauge can potentially be updated on any of these functions. This could be done by converting updateUserBoost() from external to public and calling it inside decrementGaugeBoost(), decrementGaugeAllBoost(), decrementGaugesBoostIndexed() (since decrementAllGaugesBoost() calls decrementGaugesBoostIndexed()).

However, if updating getUserBoost only when resetting all boost and gauges through decrementAllGaugesAllBoost() is still desired, I would still recommend to prevent decrementAllGaugesBoost() being called with a value that resets all the boost value for a gauge that contains the max boost.