A portion of the rewards tokens may become permanently locked in the uniStaker

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

During the accumulation period of reward tokens, if there are no staked tokens for a certain period, the amount of reward tokens generated during this period will remain in the contract forever because they cannot be claimed. This implies that the reward tokens are permanently locked in the contract.

Proof of Concept

The notifyRewardAmount will only accumulate the left rewards in the uniStaker, but ignore the rewards that cannot be claimed.

  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;

    if ((scaledRewardRate / SCALE_FACTOR) == 0) revert UniStaker__InvalidRewardRate();

    // This check cannot _guarantee_ sufficient rewards have been transferred to the contract,
    // because it cannot isolate the unclaimed rewards owed to stakers left in the balance. While
    // this check is useful for preventing degenerate cases, it is not sufficient. Therefore, it is
    // critical that only safe reward notifier contracts are approved to call this method by the
    // admin.
    if (
      (scaledRewardRate * REWARD_DURATION) > (REWARD_TOKEN.balanceOf(address(this)) * SCALE_FACTOR)
    ) revert UniStaker__InsufficientRewardBalance();

    emit RewardNotified(_amount, msg.sender);
  }

If there are no staked tokens in the contract from time t1 to t2, then no rewards will be accumulated during this period, and this part of the rewards will remain in the contract and cannot be claimed.

  function rewardPerTokenAccumulated() public view returns (uint256) {
  //@audit rewards will not be accumulated if there are no staked tokens
    if (totalStaked == 0) return rewardPerTokenAccumulatedCheckpoint;

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

POC

Here is a test case: If Alice stakes after 15 days from when the reward is transferred to the uniStaker, she could receive half of the total reward tokens after the rewardEndTime (30 days). Half of the reward tokens remain locked in the uniStaker indefinitely.

diff --git a/test/UniStaker.t.sol b/test/UniStaker.t.sol
index 89124f8..3acafe5 100644
--- a/test/UniStaker.t.sol
+++ b/test/UniStaker.t.sol
@@ -4494,6 +4494,38 @@ contract ClaimReward is UniStakerRewardsTest {
     assertEq(uniStaker.unclaimedReward(_depositor), 0);
   }

+  function test_CliamButLeftTokens(
+    address _depositor,
+    address _delegatee,
+    uint256 _stakeAmount,
+    uint256 _rewardAmount,
+    uint256 _durationPercent
+  ) public {
+
+    address alice = address(2);
+
+    // The contract is notified of a reward
+    _rewardAmount = 200e6;
+    _mintTransferAndNotifyReward(_rewardAmount);
+
+    // after 15 days
+    _jumpAhead(15*24*3600);
+
+    // Alice deposits 0.1e18 staking tokens
+    vm.prank(alice);
+    _boundMintAndStake(alice, 0.1e18, _delegatee);
+
+    // Alice claims after another 1 month
+    //uint256 reward_alice = rewardToken.balanceOf(alice);
+    _jumpAhead(31*24*60*60);
+    vm.prank(alice);
+    uniStaker.claimReward();
+    //uint256 reward_alice_after = rewardToken.balanceOf(alice);
+
+    // There are rewards tokens left in the uniStaker contract that will be locked forever
+    assertGe(rewardToken.balanceOf(address(uniStaker)), 0);
+  }
+
   function testFuzz_EmitsAnEventWhenRewardsAreClaimed(
     address _depositor,
     address _delegatee,

Result:

Running 1 test for test/UniStaker.t.sol:ClaimReward
[PASS] test_CliamButLeftTokens(address,address,uint256,uint256,uint256) (runs: 256, μ: 595973, ~: 595973)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 46.16ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Foundry

Recommended Mitigation Steps

It's recommended to add a recoverERC20 function to withdraw the unused reward tokens.

diff --git a/src/UniStaker.sol b/src/UniStaker.sol
index babdc1a..e99eabb 100644
--- a/src/UniStaker.sol
+++ b/src/UniStaker.sol
@@ -807,4 +807,10 @@ contract UniStaker is INotifiableRewardReceiver, Multicall, EIP712, Nonces {
     bool _isValid = SignatureChecker.isValidSignatureNow(_signer, _hash, _signature);
     if (!_isValid) revert UniStaker__InvalidSignature();
   }
+  function recoverERC20(address tokenAddress, uint256 tokenAmount) external  {
+    _revertIfNotAdmin();
+    require(tokenAddress != address(STAKE_TOKEN), "Cannot withdraw the staking token");
+    IERC20(tokenAddress).safeTransfer(msg.sender, tokenAmount);
+    emit Recovered(tokenAddress, tokenAmount);
+  }
 }

Assessed type

ERC20

[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