`vestTokens` bug in MultiFeeDistribution.sol causes new incentives to erase previous incentives

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/reward/MultiFeeDistribution.sol#L500-L503

Vulnerability details

Bug Description

In MultiFeeDistribution.sol,

The function _notifyReward(address rewardToken, uint256 reward) does not change r.rewardPerTokenStored and hence relies on _updateReward(address account) to be called before any calls to _notifyReward.
The rest of the functions who call _notifyReward follows this rule except for vestTokens when it is called by a minter to give incentives to the lockers.

function vestTokens(address user, uint256 amount, bool withPenalty) external whenNotPaused {
    if (!minters[msg.sender]) revert InsufficientPermission();
    if (amount == 0) return;

    if (user == address(this)) {
->       // minting to this contract adds the new tokens as incentives for lockers
->       _notifyReward(address(rdntToken), amount);
->       return;
    }
    ......
}

Since _updateReward is an internal function, we also cannot expect minters to call it on their side before calling vestTokens.

Hence, this bug results in rewardData[rewardToken].rewardPerTokenStored being innaccurate as it will not contain the previous results accumulated by the previous rewardData[rewardToken].rewardPerSecond when minters call vestTokens. Resulting in all lockers losing the rewards previously accumulated by the previous rewardPerSecond.

Proof of Concept

01: function test_rewardsGone() public {
02:     assert(rewardsDuration == 30 days); // we will use 30 days as the rewardsDuration for convenience 
03:     address Alice = address(0x123456);
04:     uint256 amount = 1 ether;
05:     uint256[] memory lockDurations = new uint256[](1);
06:     uint256[] memory rewardMultipliers = new uint256[](1);
07:     lockDurations[0] = 700 days;
08:     rewardMultipliers[0] = 1;
09:     multiFeeDistribution.setLockTypeInfo(lockDurations, rewardMultipliers);
10: 
11:     stakeToken.mint(address(this), amount);
12:     multiFeeDistribution.setLPToken(address(stakeToken));
13: 
14:     multiFeeDistribution.setAddresses(IChefIncentivesController(vm.addr(uint256(keccak256("incentivesController")))), vm.addr(uint256(keccak256("treasury"))));
15:     vm.mockCall(
16:         vm.addr(uint256(keccak256("incentivesController"))),
17:         abi.encodeWithSelector(IChefIncentivesController.afterLockUpdate.selector, Alice),
18:         abi.encode(true)
19:     );
20:     stakeToken.approve(address(multiFeeDistribution), amount);
21:     multiFeeDistribution.stake(amount, Alice, 0);    // Alice now has 1 ether staked (with lockTypeIndex=0)
22:     require(multiFeeDistribution.lockedBalance(Alice) == amount);
23: 
24:     address[] memory minters = new address[](1);
25:     minters[0] = address(this);
26:     multiFeeDistribution.setMinters(minters);
27: 
28:     amount = 10000 ether;
29:     loopToken.mint(address(this), amount);
30:     loopToken.transfer(address(multiFeeDistribution), amount);
31: 
32:     vm.mockCall(
33:         mockPriceProvider,
34:         abi.encodeWithSelector(IPriceProvider.getRewardTokenPrice.selector, address(loopToken), amount),
35:         abi.encode(8)
36:     );
37:     multiFeeDistribution.vestTokens(address(multiFeeDistribution), amount, false); //Minter gives first incentives for lockers
38:     
39:     skip(31 days);
40:     IFeeDistribution.RewardData[] memory rewardsData = multiFeeDistribution.claimableRewards(Alice);
41:     require(rewardsData[0].token == address(loopToken));
42:     console.log("After the first incentive given   |", rewardsData[0].amount);
43: 
44: 
45:     amount = 1 ether;
46:     loopToken.mint(address(this), amount);
47:     loopToken.transfer(address(multiFeeDistribution), amount);
48:     vm.mockCall(
49:         mockPriceProvider,
50:         abi.encodeWithSelector(IPriceProvider.getRewardTokenPrice.selector, address(loopToken), amount),
51:         abi.encode(8)
52:     );
53:     multiFeeDistribution.vestTokens(address(multiFeeDistribution), amount, false); //Minter gives second incentives for lockers
54:     rewardsData = multiFeeDistribution.claimableRewards(Alice);
55:     require(rewardsData[0].token == address(loopToken));
56:     console.log("Right after second incentive given|", rewardsData[0].amount);
57: 
58:     skip(31 days);
59:     rewardsData = multiFeeDistribution.claimableRewards(Alice);
60:     require(rewardsData[0].token == address(loopToken));
61:     console.log("After everything ends             |", rewardsData[0].amount);
62: }

Console Output:

Ran 1 test for src/test/unit/MultiFeeDistribution.t.sol:MultiFeeDistributionTest
[PASS] test_rewardsGone() (gas: 738287)
Logs:
  After the first incentive given   | 9999999999999999999999
  Right after second incentive given| 0
  After everything ends             | 999999999999999999

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.35ms (1.38ms CPU time)

Ran 1 test suite in 276.54ms (6.35ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Code walkthrough:

1. (Line 21) Alice stakes 1 ether to become a locker
2. (Line 37) Trusted minter gives out 10000 ether as incentive to lockers by calling vestTokens with parameter user == address(MultiFeeDistribution)
3. (Line 42) 31 days passed, calling claimableRewards now show that Alice has 9999999999999999999999 tokens(10 000 ether) to claim [refer to console output]
4. (Line 53) To further reward lockers, the trusted minter further sends a 2nd incentive of 1 ether (some time after the first incentive was given out), by again calling vestTokens with parameter user == address(MultiFeeDistribution)
5. (Line 56) Now, calling claimableRewards will show that Alice has 0 tokens to claim, proving that the previous rewards accumulated from the 1st incentive for the user has been erased :(
6. (Line 61) After rewardsDuration for the 2nd incentive ends, we call claimableRewards again. We can see from the last line of the console output that only 999999999999999999 tokens(1 ether) are left. Proving that Alice's tokens from the first incentive(10 000 ether) has been erased, and she only retains her share of the rewards coming from the 2nd incentive(1 ether).

Recommended Mitigation Steps

function vestTokens(address user, uint256 amount, bool withPenalty) external whenNotPaused {
    if (!minters[msg.sender]) revert InsufficientPermission();
    if (amount == 0) return;

    if (user == address(this)) {
        // minting to this contract adds the new tokens as incentives for lockers
+        _updateReward(address(this));
        _notifyReward(address(rdntToken), amount);
        return;
    }
    ......
}

Adding _updateReward(address(this)) will ensure rewardData[address(rdntToken)].rewardPerTokenStored is updated accordingly, so that lockers will not lose previously given incentives.

Tools Used

Manual Review, Foundry, VSCode

Assessed type

Other