ChefIncentivesController caches endRewardTime, which is not required, and may cause issues during reward update.

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/main/src/reward/ChefIncentivesController.sol#L874

Vulnerability details

Impact

ChefIncentivesController caches endRewardTime, which is not required, and may cause issues during reward update.

Bug Description

When calculating the endRewardTime, there is a cache mechanism that caches the result for endingTime.updateCadence (in UT it is set to 2 days). However, during this period, if anything changes, the endRewardTime would be incorrect. For example:

1. If rewardsPerSecond increases, then the real endRewardTime would be smaller than the cached endRewardTime.
2. If new rewards (LOOP Tokens) are registered, the real endRewardTime would be larger than the cached endRewardTime.

If the cached endRewardTime is smaller than expected, this will cause the rewards to be not distributed for the time period.

If the cached endRewardTime is larger than expected, the some pools may receive rewards after when they should, causing less rewards for other pools.

    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;
    }

    function endRewardTime() public returns (uint256) {
        if (endingTime.lastUpdatedTime + endingTime.updateCadence > block.timestamp) {
>           return endingTime.estimatedTime;
        }

        uint256 unclaimedRewards = availableRewards();
        uint256 extra = 0;
        uint256 length = poolLength();
        for (uint256 i; i < length; ) {
            VaultInfo storage pool = vaultInfo[registeredTokens[i]];

            if (pool.lastRewardTime > lastAllPoolUpdate) {
                extra +=
                    ((pool.lastRewardTime - lastAllPoolUpdate) * pool.allocPoint * rewardsPerSecond) /
                    totalAllocPoint;
            }
            unchecked {
                i++;
            }
        }
        endingTime.lastUpdatedTime = block.timestamp;

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

Proof of Concept

Presented above.

Tools Used

Manual Review

Recommended Mitigation Steps

Always recalculate for endRewardTime() and remove the cache. This is acceptable, because the _updatePool() function is only called upon user interactions, and not called regularly, so it is not requried to save gas here.

Assessed type

Other

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report