Rewards may be spread out among the **wrong time period** due to the way the protocol calculates it

[howlbot-integration]

Lines of code

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

Vulnerability details

Bug Description

First, lets reference how the rewards are calculated when a 2nd incentive is introduced while another incentive is still within its rewardsDuration period.

In MultiFeeDistribution.sol's _notifyReward function:

// inside function _notifyReward(address rewardToken, uint256 reward):
if (block.timestamp >= r.periodFinish) {
    .....
} else {
->    uint256 remaining = r.periodFinish - block.timestamp;
->    uint256 leftover = (remaining * r.rewardPerSecond) / 1e12;
->    r.rewardPerSecond = ((reward + leftover) * 1e12) / rewardsDuration;
}

This will cause rewards from the initial incentive to be delayed and spread out across the 2nd incentive's period and initial stakers will have wrong amount of claimable rewards during the timestamps all the way until the 2nd incentive's rewardsDuration ends. Other than delays, the original staker could also possibly permanently lose some of their deserved reward tokens(described further below in "2nd way of exploit" section)

Proof of Code

The symbol diagram below demostrates the senario ran in the foundry test

      [----A----|----B----]           (1st incentive)
                [---------C---------] (2nd incentive)
Day:  0        15         30       45  

• We will use rewardsDuration = 30 days.
• The first [] represents the 1st incentive, which was introduced on day 0, where A represents the first half of the rewards that should be given out during the first half of the first incentive's rewardDuration period. And B represents the second half respectively.
• The second [] represents the 2nd incentive, which was introduced on day 15, where C represents all the rewards from the 2nd incentive to be rewarded.

01: function test_rewardsSpreadAcrossWrongPeriod() 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);//1st incentive introduced on day 0
38:     
39:     skip(15 days); //go to day 15
40:     address[] memory rewardTokens_ = new address[](1);
41:     rewardTokens_[0] = address(loopToken);
42:     vm.prank(Alice);
43:     multiFeeDistribution.getReward(rewardTokens_); //withdraw at day 15
44:     console.log("Alice's balance at day 15| ", loopToken.balanceOf(Alice));
45: 
46:     loopToken.mint(address(this), amount);
47:     loopToken.transfer(address(multiFeeDistribution), amount);
48:     multiFeeDistribution.vestTokens(address(multiFeeDistribution), amount, false);//2nd incentive introduced on day 15
49: 
50:     skip(15 days); //go to day 30
51:     vm.prank(Alice);
52:     multiFeeDistribution.getReward(rewardTokens_); //withdraw at day 30
53:     console.log("Alice's balance at day 30|", loopToken.balanceOf(Alice)); //reminder: Alice's current loopToken balance is inclusive of what she withdrew on day 15
54: }

Console Output:

Ran 1 test for src/test/unit/MultiFeeDistribution.t.sol:MultiFeeDistributionTest
[PASS] test_rewardsSpreadAcrossWrongPeriod() (gas: 813968)
Logs:
  Alice's balance at day 15|  4999999999999999999999
  Alice's balance at day 30| 12499999999999999999998

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

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

Explanation:

1. (Line 37) As mentioned below the symbol diagram, we introduce the first incentive(10000 ether) at day 0.
2. (Line 43) At day 15, Alice withdraws her tokens, receiving part A as seen in the symbol diagram. (4999999999999999999999~=5000 ether)
3. (Line 48) 2nd Incentive(10000 ether) is introduced at day 15, causing remaining rewards leftover from the first incentive to be incorrectly stretched until the end of the 2nd incentive's rewardDuration
4. (Line 52) At day 30, Alice withdraws her tokens, now her total balance is 12499999999999999999998~=12 500 ether.

Let's examine Alice's balance at the end of 30 day: 12 500 ether = 5000 ether + 2500 ether + 5000 ether = A + B/2 + C/2.

However, the rightful amount of rewards her balance should be at day 30 is = A + B + C/2 = 15 000 ether.

Hence the remaining 15 000 ether - 12 500 ether = 2 500 ether that Alice is entitled to claim at day 30, will only be given throughout day 30 to 45.

This is very unfair to Alice who has staked her tokens since the beginning of the first incentive, and now she has to wait longer for the rewards from the first incentive which is a high oppportunity cost incurred for her.

This is made worse if the incentive given at the 2nd wave is significantly smaller than the original amount in wave 1, because it means she will have to wait longer for her significant rewards from the 1st wave, all because of the 2nd wave of small and insignificant incentive.

2nd way of exploit

Referencing the same senario in the Proof of Code section. A malicious staker can choose to stake anytime between day 30 to day 45 and cause Alice to permanently lose some of her rewards.
Example:

• Malicious staker sees the senario in the above section happening, and decides to call stake on day 30.
• When the malicious staker withdraws on day 45, he is able to receive a portion of reward part B, even though he is not supposed to, because we already established in the symbol diagram that part B is supposed to end on day 30. And since the malicious staker only staked on day 30, he should not be getting the rewards as he locked his tokens late.

Hence, Alice permanently loses a portion of reward part B to the malicious staker who is not supposed to receive it.

---

Overall Disclaimer: The example above of the 2nd incentive being introduced at exactly halfway(15 days) of the 1st incentive's duration was just used as an example, this bug still exists as long as the 2nd incentive is introduced at any point of time throughout the 1st incentive's duration, causing the respective portion to be spread across the wrong period.

Recommended Mitigation Steps

+ struct rewardQueue {
+   uint256 periodFinish;
+   uint256 rewardPerSecond;
+ }
// below is the struct from src/reward/interfaces/LockedBalance.sol
struct Reward {
- uint256 periodFinish;
- uint256 rewardPerSecond;
+ rewardQueue[] rewards;   
+ uint256 rewardCounter;
  uint256 lastUpdateTime;
  uint256 rewardPerTokenStored;
  uint256 balance;
}

We can use a queue-like list to store rewards and their respective periodFinish, as well as a counter that we can increment when rewards[rewardCounter].periodFinish < block.timestamp.

And rewards[i].rewardsPerSecond is meant to be distributed between the timeframe of rewards[i-1].periodFinish to rewards[i].periodFinish only.

Tools Used

Manual Review, Foundry, VSCode

Assessed type

Math

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report

[0xAlix2]

@koolexcrypto - MultiFeeDistribution is an exact fork of Radiant's https://github.com/radiant-capital/v2/blob/cd618877151896415705468f1b2a43c4b75b3c5b/contracts/radiant/staking/MultiFeeDistribution.sol, the scenario that the warden pointed out to is intended.

[koolexcrypto]

while this is an exact fork of Radiant, it does not mean it wouldn't have issues. The following both statements could be right:

• Sponsor has intention the same Radiant has on their contract, therefore, the issue above would be a QA.
• Sponsor has intention the same Radiant has on their contract, but couldn't know this issue exists (regardless if it was intended by Radiant) unless subjecting it to an audit which is what happened.

However, since the sponsor confirmed this, a Medium severity is appropriate.

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)