Rewards are lost when no one is staking

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5a2761c8277541a24bc551fbd624413b384bea94/src/UniStaker.sol#L755

Vulnerability details

Description

When fees are added to the Rewards contract, notifyRewardAmount() is called.

function notifyRewardAmount(address rewardToken, uint256 amount, uint minRemainingTime) public onlyOperators {
    if (!_rewardData[rewardToken].exists) {
        revert ErrInvalidTokenAddress();
    }
    _updateRewards();
    rewardToken.safeTransferFrom(msg.sender, address(this), amount);
    Reward storage reward = _rewardData[rewardToken];
    uint256 _nextEpoch = nextEpoch();
    uint256 _remainingRewardTime = _nextEpoch - block.timestamp;
    if (_remainingRewardTime < minRemainingTime) {
        revert ErrInsufficientRemainingTime();
    }
    // Take the remainder of the current rewards and add it to the amount for the next period
    if (block.timestamp < reward.periodFinish) {
        amount += _remainingRewardTime * reward.rewardRate;
    }
    // avoid `rewardRate` being 0
    if (amount < _remainingRewardTime) {
        revert ErrNotEnoughReward();
    }
    reward.rewardRate = amount / _remainingRewardTime;
    reward.lastUpdateTime = uint248(block.timestamp);
    reward.periodFinish = _nextEpoch;
    emit LogRewardAdded(amount);
}

It sets rewardRate so that the total pending fees would be split uniformally over the remaining time till end of epoch. periodFinish is set to end of epoch, while lastUpdateTime = now

Suppose there's no stakers when the first rewards are sent to the LockingMultiRewards, for example since yield is zero right now. Some time passes and the first staker joins. All interactions with beneficiaries including stake() run the following line to update global rewards:

_updateRewardsForUser();
->
function _updateRewardsForUser(address user) internal {
    uint256 balance = balanceOf(user);
    uint256 totalSupply_ = totalSupply();
    for (uint256 i; i < rewardTokens.length; ) {
        address token = rewardTokens[i];
        // KEY IS _updateRewardsGlobal() below
        _udpateUserRewards(user, balance, token, _updateRewardsGlobal(token, totalSupply_));
        unchecked {
            ++i;
        }
    }
}

Which is:

function _updateRewardsGlobal(address token_, uint256 totalSupply_) internal returns (uint256 rewardPerToken_) {
    uint256 lastTimeRewardApplicable_ = lastTimeRewardApplicable(token_);
    rewardPerToken_ = _rewardPerToken(token_, lastTimeRewardApplicable_, totalSupply_);
    _rewardData[token_].rewardPerTokenStored = rewardPerToken_;
    _rewardData[token_].lastUpdateTime = uint248(lastTimeRewardApplicable_); // safe to cast as this will never overflow
}

The first line updates the total unlocked reward/token:

function _rewardPerToken(address rewardToken, uint256 lastTimeRewardApplicable_, uint256 totalSupply_) public view returns (uint256) {
    if (totalSupply_ == 0) {
        return _rewardData[rewardToken].rewardPerTokenStored;
    }
    uint256 timeElapsed = lastTimeRewardApplicable_ - _rewardData[rewardToken].lastUpdateTime;
    uint256 pendingRewardsPerToken = (timeElapsed * _rewardData[rewardToken].rewardRate * 1e18) / totalSupply_;
    return _rewardData[rewardToken].rewardPerTokenStored + pendingRewardsPerToken;
}

In case totalSupply_ is zero, it returns the original checkpoint without accounting for the new rewards.

The last line in _updateRewardsGlobal() updates lastUpdateTime:

function lastTimeRewardApplicable(address rewardToken) public view returns (uint256) {
    return MathLib.min(block.timestamp, _rewardData[rewardToken].periodFinish);
}

Note that it would update regardless if totalStaked==0. Therefore, it would store the current timestamp in lastUpdateTime.

At this point, the tokens weren't distributed, but lastCheckpointTime is updated. So in the future, the accumulation of tokens in _rewardPerToken() will never include the time when there weren't any stakers.

This results in loss of fees which are permanently stuck in the contract, whenver there are no stakers. That could be at any part of the contract's lifetime, not just when the contract is booted.

Impact

Permanent freeze of yield which currently belongs to the LP stakers

Proof of Concept

• The Rewards contract is deployed at boot
• Rewards are dispatched to the stakers
• Rewards from this point and until the first staker joins, are permanently stuck in the contract.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider not advancing lastUpdateTime in case totalStaked == 0 in _checkpointGlobalReward().

Assessed type

Math

[0xm3rlin]

irrelevant in practice

[c4-pre-sort]

141345 marked the issue as duplicate of #37

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)

[c4-judge]

thereksfour marked the issue as grade-b