Unallocated tokens remain in the contract forever

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

There may be undistributed tokens left on the contract balance that will not be taken into account the next time the notifyRewardAmount function is called. They will remain in contract forever.

This can happen in 2 cases:

1. The notifyRewardAmount function was called at a time when there were no stakers, and the first one appeared only a few days later. It will not receive rewards that started being distributed from the moment notifyRewardAmount was called. But the contract might not start distributing rewards (counting) until the first staker appears. Or the next call to notifyRewardAmount should take into account the remaining tokens in the calculation.

   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;

2. If the stakers called for withdraw() before the end of the reward distribution period. And the next call to notifyRewardAmount() occurred when there were no more stakers left. Tokens that should have been awarded as a reward at a time when there are no stakers will not be taken into account the next time the notifyRewardAmount function is called

Proof of Concept

Add this file in test folder

// SPDX-License-Identifier: AGPL-3.0-only
pragma solidity 0.8.23;

import {Vm, Test, stdStorage, StdStorage, console2} from "forge-std/Test.sol";
import {UniStaker, DelegationSurrogate, IERC20, IERC20Delegates} from "src/UniStaker.sol";
import {UniStakerHarness} from "test/harnesses/UniStakerHarness.sol";
import {ERC20VotesMock, ERC20Permit} from "test/mocks/MockERC20Votes.sol";
import {ERC20Fake} from "test/fakes/ERC20Fake.sol";
import {PercentAssertions} from "test/helpers/PercentAssertions.sol";



contract StakeTest is Test{
  ERC20Fake rewardToken;
  ERC20VotesMock uniToken;
  address admin;
  address rewardNotifier;
  address user1 = makeAddr("user1");
  UniStakerHarness uniStaker;

  function setUp() public {
    // Set the block timestamp to an arbitrary value to avoid introducing assumptions into tests
    // based on a starting timestamp of 0, which is the default.
    vm.warp(block.timestamp + 1234);

    rewardToken = new ERC20Fake();
    vm.label(address(rewardToken), "Reward Token");

    uniToken = new ERC20VotesMock();
    vm.label(address(uniToken), "Governance Token");

    rewardNotifier = address(0xaffab1ebeef);
    vm.label(rewardNotifier, "Reward Notifier");

    admin = makeAddr("admin");

    uniStaker = new UniStakerHarness(rewardToken, uniToken, admin);
    vm.label(address(uniStaker), "UniStaker");

    vm.prank(admin);
    uniStaker.setRewardNotifier(rewardNotifier, true);

 
    uniToken.mint(user1, 1000e18);
    rewardToken.mint(rewardNotifier, 200e18);
    vm.prank(user1);
    uniToken.approve(address(uniStaker), 1000e18);
    
  }


    function test() external {
        // NotifyReward
        vm.startPrank(rewardNotifier);
        rewardToken.transfer(address(uniStaker), 100e18);
        uniStaker.notifyRewardAmount(100e18);
        vm.stopPrank();

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

        // Stake
        vm.startPrank(user1);
        uniStaker.stake(10e18, user1); 
        vm.stopPrank();
      
        vm.warp(block.timestamp + 40 days);

        // NotifyReward
        vm.startPrank(rewardNotifier);
        rewardToken.transfer(address(uniStaker), 100e18);
        uniStaker.notifyRewardAmount(100e18);
        vm.stopPrank();

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

        // Claim and withdraw
        vm.startPrank(user1);
        uniStaker.claimReward();
        uniStaker.withdraw(UniStaker.DepositIdentifier.wrap(0),10e18);
        vm.stopPrank();

        // Check balance
        console2.log('remained balance -', rewardToken.balanceOf(address(uniStaker)));
      
       
        
    }


}

Result:

[PASS] test() (gas: 470815)
Logs:
  remained balance - 33333333333333333334

Tools Used

Manual review

Recommended Mitigation Steps

1. Calculate scaledRewardRate based not only on the new amount, but also on the contract balance

function notifyRewardAmount(uint256 _amount) external {
+    uint rewardBalance = REWARD_TOKEN.balanceOf(address(this));
+    _amount =  rewardBalance >= _amount ? rewardBalance : amount;

2. When calling notifyRewardAmount, do not calculate rewardEndTime if there are no stickers

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