Reward distributed when totalStaked==0 is lost

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/main/src/UniStaker.sol#L230
https://github.com/code-423n4/2024-02-uniswap-foundation/blob/main/src/UniStaker.sol#L577-L582

Vulnerability details

Impact

When the totalStaked is reduced to 0 before the reward distribution period ends, the remaining reward may become locked indefinitely within the balance of the UniStaker contract.

In another scenario, if users are inactive and do not stake in UniStaker while the reward distributions continue regularly, any rewards distributed during that period where users did not stake will remain locked and unrecoverable.

When totalStaked == 0, the rewardPerTokenAccumulatedCheckpoint value does not get updated.

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

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

In the case of new rewards come after REWARD_DURATION period, the remaining reward (from period where totalStaked == 0) has no effect on the scaledRewardRate value.

if (block.timestamp >= rewardEndTime) {
    scaledRewardRate = (_amount * SCALE_FACTOR) / REWARD_DURATION;
} else {
    uint256 _remainingReward = scaledRewardRate * (rewardEndTime - block.timestamp);
    scaledRewardRate = (_remainingReward + _amount * SCALE_FACTOR) / REWARD_DURATION;
}

Moreover, in described scenarios where totalStaked is 0, UniStaker lacks the mechanism to retrieve or rescue these locked rewards. Consequently, any such rewards will remain inaccessible indefinitely.

Proof of Concept

Poc 1

Consider the following scenario:

1. Alice is the sole staker, with a stake of 1000 UNI.
2. The reward of 10 WETH comes.
3. After waiting 21 days, Alice decides to withdraw her 1000 UNI, resulting in totalStaked becoming 0.
4. At this point, the UniStaker contract still holds 3 WETH from the reward distribution.
5. Suppose no new stakers join for the next 7 days.
6. Further Alice stakes 1000 UNI and a new reward of 10 WETH comes. After another 30 days (REWARD_DURATION), Alice claims the reward of 10 WETH.
7. However, the 3 WETH from the previous distribution remains stuck in the contract, unable to be claimed or rescued.

Poc 2

Consider the following scenario:

1. Despite no one using UniStaker, the reward distribution occurs regularly with 10 WETH initially and another 10 WETH after 7 days.
2. After 40 days, Alice decides to stake 1000 UNI, but no new rewards come.
3. Subsequently, Alice patiently waits for 30 days (REWARD_DURATION), withdraws her UNI, and proceeds to claim the reward. However, to her dismay, she receives 0 WETH as a reward.
4. The reward of 20 WETH remains trapped within UniStaker.

To launch the PoCs place the PoC contract presented below into the test/UniStaker.t.sol file and issue the following commands.

# To run Poc1:
forge test --mp test/UniStaker.t.sol --mt test_stuck_reward_0_total_staked_1

# To run Poc2:
forge test --mp test/UniStaker.t.sol --mt test_stuck_reward_0_total_staked_2

contract PoC is UniStakerRewardsTest {
  function test_stuck_reward_0_total_staked_1() public {
    address alice = address(101);

    require(uniStaker.totalStaked() == 0);

    // 1. Give Alice 1000 UNI tokens.
    _mintGovToken(alice, 1000 ether);

    // 2. Alice stakes 1000 UNI.
    UniStaker.DepositIdentifier _depositId = _stake(alice, 1000 ether, alice);

    // 3. Transfer reward of 10 WETH.
    _mintTransferAndNotifyReward(10 ether);

    // 4. Alice withdraws her UNI after 70% of REWARD_DURATION and claims the reward.
    _jumpAheadByPercentOfRewardDuration(70);

    vm.prank(alice);
    uniStaker.withdraw(_depositId, 1000 ether);

    vm.prank(alice);
    uniStaker.claimReward();

    // 5. TotalStaked == 0, and 3 WETH won't be distributed and locked in uniStaker.
    // There are no new stakes until rewardEndTime.
    require(uniStaker.totalStaked() == 0);
    _jumpAheadByPercentOfRewardDuration(30);

    // 6. Another round of reward distributions. Now Alice stakes and waits 100% of REWARD_DURATION.
    _mintTransferAndNotifyReward(10 ether);

    _depositId = _stake(alice, 100 ether, alice);

    _jumpAheadByPercentOfRewardDuration(100);

    vm.prank(alice);
    uniStaker.withdraw(_depositId, 100 ether);

    vm.prank(alice);
    uniStaker.claimReward();

    // 7. There is a stuck reward of 3 WETH that will remain on UniStaker balance forever.
    require(rewardToken.balanceOf(address(uniStaker)) > 3 ether);
  }

  function test_stuck_reward_0_total_staked_2() public {
    address alice = address(101);

    require(uniStaker.totalStaked() == 0);

    // 1. Give Alice 1000 UNI tokens.
    _mintGovToken(alice, 1000 ether);

    // 2. 10 WETH reward.
    _mintTransferAndNotifyReward(10 ether);

    vm.warp(block.timestamp + 7 days);

    // 3. 10 WETH reward.
    _mintTransferAndNotifyReward(10 ether);

    vm.warp(block.timestamp + 30 days);

    // 4. Alice stakes 1000 UNI, waits 30 days, claims reward.
    UniStaker.DepositIdentifier _depositId = _stake(alice, 1000 ether, alice);

    _jumpAheadByPercentOfRewardDuration(100);

     vm.prank(alice);
    uniStaker.withdraw(_depositId, 100 ether);

    vm.prank(alice);
    uniStaker.claimReward();

    // 5. 20 WETH reward is lost.
    require(rewardToken.balanceOf(address(uniStaker)) >= 20 ether);
  }
}

Tools Used

Manual review.

Recommended Mitigation Steps

Possible mitigation is to add rescue() function to UniStaker.

function rescue() external {
  _revertIfNotAdmin();
  if (totalStaked == 0) {
      SafeERC20.safeTransfer(REWARD_TOKEN, msg.sender, REWARD_TOKEN.balanceOf(address(this)));
  }
}

Additionally, to check totalStaked > 0 inside notifyRewardAmount().

require(totalStaked > 0);

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