Any UniStaker fees accruing when no one is staking would be lost

[c4-bot-7]

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 UniStaker, notifyRewardAmount() is called.

function notifyRewardAmount(uint256 _amount) external {
  if (!isRewardNotifier[msg.sender]) revert UniStaker__Unauthorized("not notifier", msg.sender);
  // We checkpoint the accumulator without updating the timestamp at which it was updated, because
  // that second operation will be done after updating the reward rate.
  rewardPerTokenAccumulatedCheckpoint = rewardPerTokenAccumulated();
  if (block.timestamp >= rewardEndTime) {
    scaledRewardRate = (_amount * SCALE_FACTOR) / REWARD_DURATION;
  } else {
    uint256 _remainingReward = scaledRewardRate * (rewardEndTime - block.timestamp);
    scaledRewardRate = (_remainingReward + _amount * SCALE_FACTOR) / REWARD_DURATION;
  }
  rewardEndTime = block.timestamp + REWARD_DURATION;
  lastCheckpointTime = block.timestamp;

It sets scaledRewardRate so that the total pending fees would be split uniformally over the next 30 days.
rewardEndTime is set to now + 30 days, while lastCheckpointTime = now

Suppose there's no stakers when the first rewards are sent to the UniStaker, 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:

_checkpointGlobalReward();

Which is:

/// @notice Checkpoints the global reward per token accumulator.
function _checkpointGlobalReward() internal {
  rewardPerTokenAccumulatedCheckpoint = rewardPerTokenAccumulated();
  lastCheckpointTime = lastTimeRewardDistributed();
}

The first line updates the total unlocked reward/token:

function rewardPerTokenAccumulated() public view returns (uint256) {
  if (totalStaked == 0) return rewardPerTokenAccumulatedCheckpoint;
  return rewardPerTokenAccumulatedCheckpoint
    + (scaledRewardRate * (lastTimeRewardDistributed() - lastCheckpointTime)) / totalStaked;
}

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

The second line updates lastCheckpointTime:

function lastTimeRewardDistributed() public view returns (uint256) {
  if (rewardEndTime <= block.timestamp) return rewardEndTime;
  else return block.timestamp;
}

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

At this point, the tokens weren't distributed, but lastCheckpointTime is updated. So in the future, the accumulation of tokens in rewardPerTokenAccumulated() will never include the days 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.

Impact

Permanent freeze of yield which currently belongs to the Uniswap governance.

Proof of Concept

• Uniswap deploys UniStaker + FactoryOwner
• Someone instantly calls claimFees() to get fees and give rewards, before the first stake deposits into UniStaker.
• Rewards from this point and until the first staker, permanently stuck in the contract.

Tools Used

Manual audit

Recommended Mitigation Steps

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

Assessed type

Math

[c4-judge]

MarioPoneder marked the issue as duplicate of #9

[c4-judge]

MarioPoneder marked the issue as duplicate of #369

[c4-judge]

MarioPoneder marked the issue as unsatisfactory:
Invalid