Discrepency b/w the lastRewadTime and the lastAllPoolUpdate can allow for incorrect reward distribution to pools if registerRewardDeposit deposits less assets

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

Incorrect reward distribution causing some pools to gain more while others to gain less

Proof of Concept

The _massUpdatePools function always sets lastAllPoolUpdate = block.timestamp

    function _massUpdatePools() internal {
        uint256 totalAP = totalAllocPoint;
        uint256 length = poolLength();
        for (uint256 i; i < length; ) {
            _updatePool(vaultInfo[registeredTokens[i]], totalAP);
            unchecked {
                i++;
            }
        }
        lastAllPoolUpdate = block.timestamp;
    }

But the individualPool update timestamp can become lower than block.timestamp if it surpasses the endRewardTime ie. the time in which the entire assets deposited is supposed to be depleted

    function _updatePool(VaultInfo storage pool, uint256 _totalAllocPoint) internal {
        uint256 timestamp = block.timestamp;
        uint256 endReward = endRewardTime();
        if (endReward <= timestamp) {
            timestamp = endReward;
        }
        if (timestamp <= pool.lastRewardTime) {
            return;
        }


        (uint256 reward, uint256 newAccRewardPerShare) = _newRewards(pool, _totalAllocPoint);
        accountedRewards = accountedRewards + reward;
        pool.accRewardPerShare = pool.accRewardPerShare + newAccRewardPerShare;
        pool.lastRewardTime = timestamp;

Further more, the endRewardTime is calculated as newEndTime = (unclaimedRewards + extra) / rewardsPerSecond + lastAllPoolUpdate whenever the rewardsPerSecond is non-zero

    function endRewardTime() public returns (uint256) {
        
        .....

        if (rewardsPerSecond == 0) {
            endingTime.estimatedTime = type(uint256).max;
            return type(uint256).max;
        } else {
            uint256 newEndTime = (unclaimedRewards + extra) / rewardsPerSecond + lastAllPoolUpdate;
            endingTime.estimatedTime = newEndTime;
            return newEndTime;
        }

Hence if a _massUpdatePools call occurs when block.timestamp is greater than the endRewardTime it will set the lastRewardTime of pools to be less than lastAllPoolUpdate, following which even if there are no more rewards, the new endRewardTime would be the timestamp of lastAllPoolUpdate. This will allow a pool to claim rewards worth (lastAllPoolUpdate - initialEndTime) * rewardPerSecond which is not an expected behaviour and not handled with the endRewardTime constaint
The above scenario can occur if the registerRewardDeposit function is invoked with a low amount of deposits (ie. the deposited amount shouldn't cause the new end time to be >= block.timestamp) when the endRewardTime has been surpassed

Eg:

pool A and B, 1:1 rewardRatio
rewardsPerSecond = 1
endRewardTime = 100
block.timestamp = 110
registerRewardDeposit is invoked such that the new endRewardTime (ie. even after including the newly deposited assets) is 105

now the first massUpdatePools call will result in:
lastRewardTime = 105 (endRewardTime)
lastAllPoolUpdate = 110

now the entire rewards of the contract are used up

but when massUpdatePools gets invoked again (eg. via claim -> _updateEmissions -> _massUpdatePools), the new endTime will be 110 (ie. lastPoolUpdate + 0)

claim is called for pool A in the same block,

A's lastRewardTime gets updated to 110 while B's remain 105

at 120 registerRewardDeposit is invoked with a lot of assets,
B will accrue a reward of (120 - 105)/2 while A will only accrue (120 - 110)/2

Tools Used

Manual review

Recommended Mitigation Steps

In case endTime > block.timestamp, can set the lastPoolUpdate to endTime or always ensure that the registerRewardDeposit function will only be called with amounts such that the above issue doesn't occur

Assessed type

Context

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report