`lastRPS` can be set to 0, causing `rewardsPerSecond` to lose its original value

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/reward/ChefIncentivesController.sol#L440-L444

Vulnerability details

Bug Description

In ChefIncentiveController.sol, there is a way to bypass the checks to set lastRPS to 0.

lastRPS is used to store the previous rewardsPerSecond, before the protocol sets rewardsPerSecond to 0.

Essentially lastRPS serves as a backup for the original value, when rewardsPerSecond gets set to 0 due to lack of rewards deposit.

Allowing rewardsPerSecond to regain its original value after reward deposits are replenished through registerRewardDeposit.

In function _updateEmissions():

function _updateEmissions() internal {
->    if (block.timestamp > endRewardTime()) {
->      _massUpdatePools();
->      lastRPS = rewardsPerSecond;
        rewardsPerSecond = 0;
        return;
    }
    setScheduledRewardsPerSecond();
}

At first glance, it might seems that if _updateEmissions() is called twice when rewardsPerSecond is supposed to be set to 0, lastRPS will also be set to 0 and rewardsPerSecond will not be able to regain its original value after a reward deposit.

However the protocol attempts to prevent that by including these lines in endRewardTime():

if (rewardsPerSecond == 0) {
  endingTime.estimatedTime = type(uint256).max;
  return type(uint256).max;
}

So, for example when rewardsPerSecond is already 0 in the second time _updateEmissions() is called, then the if statement will return type(uint256).max and the if statement in _updateEmissions: if (block.timestamp > endRewardTime()) will be false and lastRPS will not be set to 0.

However, there is still a way to bypass this. In lines 873-875 of endRewardTime():

if (endingTime.lastUpdatedTime + endingTime.updateCadence > block.timestamp) {
  return endingTime.estimatedTime;
}

Meaning the function will not run if it has been calculated in the recent updateCadence time. Hence this give rise to the sequence of operations below that could cause lastRPS to be set to 0.

Sequence of Operations

1. Contract runs out of deposited rewards.
2. _updateEmission() is called, since rewardsPerSecond is not 0 yet, endRewardTime() calculates endingTime.estimatedTime properly and returns it.
3. Resulting in if (block.timestamp > endRewardTime()) to be true in _updateEmissions(), setting lastRPS = rewardsPerSecond and rewardsPerSecond = 0
4. Some time later (where time <updateCadence), _updateEmission() is called for a 2nd time.
5. Since, updateCadence prevents re-calculation and simply causes endRewardTime() to return the same endingTime.estimatedTime instead of type(uint256).max, once again if (block.timestamp > endRewardTime()) will be true again just like last time in _updateEmissions()
6. Now that rewardsPerSecond is already 0, lastRPS = rewardsPerSecond will set lastRPS to 0. Resulting in rewardsPerSecond losing its original backup value.

Note that even though _updateEmission() is an internal function which cannot be simply called by external malicious users, they can still call functions like claim which calls _updateEmissionsLine 524, which will then allow the sequence above to happen. Hence the sequence above can still be executed by a malicious external user.

Also updateCadence can be up to 1 week as seen in setEndingTimeUpdateCadenceLine 910. Giving it a long runway for the sequence of attack to happen.

Recommended Mitigation Steps

function _updateEmissions() internal {
    if (block.timestamp > endRewardTime()) {
        _massUpdatePools();
-       lastRPS = rewardsPerSecond;
+       if(rewardsPerSecond!=0) lastRPS = rewardsPerSecond;
        rewardsPerSecond = 0;
        return;
    }
    setScheduledRewardsPerSecond();
}

Tools Used

Manual Review

Assessed type

Other