Dripping does not stop when nothing is staked

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/UniStaker.sol#L229-L234

Vulnerability details

Impact

Rewards can be dripped when nothing is staked, during which time rewards are permanently lost.

Proof of Concept

function rewardPerTokenAccumulated() public view returns (uint256) {
  if (totalStaked == 0) return rewardPerTokenAccumulatedCheckpoint;

  return rewardPerTokenAccumulatedCheckpoint
    + (scaledRewardRate * (lastTimeRewardDistributed() - lastCheckpointTime)) / totalStaked;
}

rewardPerTokenAccumulated() returns the cumulative distributed rewards per token. That is, if this is stationary/constant no rewards are being distributed. This happens either when totalStaked == 0 or when lastTimeRewardDistributed() == lastCheckpointTime, i.e. when the rewards distribution has ended.

A newly notified amount of rewards are distributed evenly over the next 30 days, and any currently remaining rewards are added to this amount. The end of this period is set in rewardEndTime. rewardEndTime can only be changed by again notifying a reward, upon which remaining rewards are added to the new 30 day distribution period. That is, the rewards are always distributed at a rate such that the entire amount is distributed in exactly 30 days. This means that any interruption to the distribution before rewardEndTime leads to the rewards not fully distributing, i.e. they are lost.

This happens when totalStaked == 0 during a distribution, i.e. if notifyRewardAmount() is called before anything has been staked, or if everything is unstaked before rewardEndTime.

As a more explicit example illustration, suppose nothing is staked (or never has been) and that notifyRewardAmount() is called. Suppose then 30 days pass and someone stakes. _stake() calls _checkpointGlobalReward() which sets lastCheckpointTime = rewardEndTime. Now consider the rewardPerTokenAccumulated() which is called on this stake and also some time later when rewards are claimed. In both cases it returns the same rewardPerTokenAccumulatedCheckpoint, first (on _stake()) since totalStaked == 0 and later (on _claimReward()) since lastTimeRewardDistributed() - lastCheckpointTime == 0. That is, rewardPerTokenAccumulated() - beneficiaryRewardPerTokenCheckpoint[_beneficiary] == 0 so unclaimedReward() returns 0 and no rewards could be claimed. The rewards which were notified were thus lost.

Recommended Mitigation Steps

A simple solution is to renotify Unistaker of its own balance, by adding in _stake()

if (totalStaked == 0) {
    uint256 lostRewards = REWARD_TOKEN.balanceOf(address(this));
    scaledRewardRate = (lostRewards * SCALE_FACTOR) / REWARD_DURATION;
    rewardEndTime = block.timestamp + REWARD_DURATION;
    lastCheckpointTime = block.timestamp;
}

The slight downside to this is that it restarts the distribution rather than pauses it.

Otherwise the distribution should be paused whenever totalStaked == 0. This may be achieved by keeping a variable startPause for the last time stamp totalStaked == 0 happened, set in _withdraw(). Then in _stake(), if totalStaked == 0 && _amount > 0, we extend rewardEndTime += block.timestamp - max(startPause, rewardEndTime - REWARD_DURATION).

Assessed type

Other

[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