A Portion of Reward Could Get Stuck or Frozen indefinitely in the UniStaker contract

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

The UniStaker contract initiates reward distribution upon an authorized notifier's call to notifyRewardAmount. This function sets the scaledRewardRate and marks the beginning and end of the reward period (rewardEndTime). A notable flaw arises when rewards accrual begins before any Staker has staked. This gap between rewards notification and the first stake action can lead to rewards not being allocated for the initial period. Consequently, a portion of the rewards may remain unclaimed within the contract indefinitely.

Proof of Concept

The notifyRewardAmount function is triggered by an authorized notifier to signify the transfer of new rewards into the contract, thereby enabling Stakers to start earning rewards. It does this by calculating the new reward rate(scaledRewardRate) based on the notified amount, the scale factor, and the reward duration (i.e. 30 days), subsequently setting rewardEndTime to the current timestamp plus the reward duration.

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);
  }

Consider the scenario where the contract has a reward duration of 2592000 seconds (one month):

• Block N: An authorized notifier calls notifyRewardAmount with a reward intended for distribution for one month. Assume the scaledRewardRate is set to distribute 1 reward token per second for Stakers.

Therefore; scaledRewardRate = 1, rewardEndTime = Timestamp X + 2592000

• Block M (Timestamp = X + Y): After Y seconds, the first Staker stakes, initiating their reward accumulation from Timestamp X + Y. Please note, though, that the rewardEndTime is X + rewardsDuration, not X + Y + rewardsDuration. This delay (Y) leads to a discrepancy where rewards meant for the initial Y seconds remain undistributed, as the contract's reward distribution window does not start from X + Y + rewardsDuration.

For example, a 30-minute delay (Y = 1800 seconds) results in only 2590200 tokens being distributed, leaving 1800 tokens unused in the contract. These tokens stay dormant. If a certain amount remains unused (like the above example inside the contract), and a new reward cycle is not started, that amount remains dormant inside the contract.

Even if the protocol decides to start a new reward cycle to cover this unused amount, it is still a redundant execution as there will likely be a delay between the first stake and the reward cycle initiation, hence consider resolving this issue.

Test Case

Copy & run with forge test --match-test testForStuckedRewards -vvvv (you can name test file as UniStakerTest.t.sol) or check the output below.

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

import {Test, console} from "forge-std/Test.sol";
import {UniStaker, DelegationSurrogate, IERC20, IERC20Delegates} from "src/UniStaker.sol";
import {ERC20VotesMock} from "test/mocks/MockERC20Votes.sol";
import {ERC20Fake} from "test/fakes/ERC20Fake.sol";

contract UniStakerTest is Test {
  UniStaker public Unistaker;  
  ERC20Fake public rewardToken;
  ERC20VotesMock public govToken;

  //Unistaker address set as the admin
  address admin = 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f;
  address alice = vm.addr(1);
  address bob = vm.addr(2);

    struct Deposit {
    uint256 balance;
    address owner;
    address delegatee;
    address beneficiary;
  }

    mapping(address depositor => uint256 amount) public depositorTotalStaked;
    mapping(address beneficiary => uint256 amount) public earningPower;
    mapping(address delegatee => DelegationSurrogate surrogate) public surrogates;
    mapping(address account => uint256) public beneficiaryRewardPerTokenCheckpoint;
    mapping(address account => uint256 amount) public unclaimedRewardCheckpoint;

    uint256 public constant SCALE_FACTOR = 1e36;
    uint256 public rewardEndTime = block.timestamp + REWARD_DURATION;
    uint256 public constant REWARD_DURATION = 30 days;

    address public immutable REWARD_TOKEN  = 0x104fBc016F4bb334D775a19E8A6510109AC63E00; //rewardToken address
    address public immutable STAKE_TOKEN = 0x037eDa3aDB1198021A9b2e88C22B464fD38db3f3; //govToken address

    uint256 public constant INITIAL_SUPPLY1 = 1000 ether; //As stake tokens for alice to stake 
    uint256 public constant INITIAL_SUPPLY2 = 10000000 ether; //As rewards token to be earned

    function setUp() public {
        Unistaker = new UniStaker(
            IERC20(REWARD_TOKEN),
            IERC20Delegates(STAKE_TOKEN), 
            admin 
        );

        vm.startPrank(address(Unistaker));
        rewardToken = new ERC20Fake();
        vm.label(address(rewardToken), "Reward Token");
        //Mint 10000000 WETH to the contract as rewards to be earned
        rewardToken.mint(address(Unistaker), INITIAL_SUPPLY2 / 1e18);
        //Assume Unistaker = RewardNotifier
        Unistaker.setRewardNotifier(address(Unistaker), true);
        //Note: rewards starts streaming immediately
        Unistaker.notifyRewardAmount(10000000);

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

        //Mint 1000 UNI token for alice to stake
        govToken.mint(alice, INITIAL_SUPPLY1 / 1e18);
    }

    function testForStuckedRewards() public {

        //Assume no staker for atleast 1 day
        vm.warp(1 days);

        //alice stakes 1000 UNI tokens
        vm.startPrank(alice);
        IERC20(govToken).approve(address(Unistaker), 1000);
        Unistaker.stake(1000, alice);

        //advance to rewardEndTime + 60 seconds (to ensure rewardEndTime fully ended)
        vm.warp(rewardEndTime + 60 seconds);

        //claim the rewards alice has earned over the period
        Unistaker.claimReward();
        vm.stopPrank();

        //check for the stucked rewards in the contract
        console.log("stucked rewards:", rewardToken.balanceOf(address(Unistaker)));
    }

}

Test Output

Running 1 test for test/UniStakerTest.t.sol:UniStakerTest
[PASS] testForStuckedRewards() (gas: 455773)
Logs:
  stucked rewards: 333330

Traces:
  [455773] UniStakerTest::testForStuckedRewards() 
    ├─ [0] VM::warp(86400 [8.64e4])
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
    │   └─ ← ()
    ├─ [24735] Governance Token::approve(UniStaker: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1000)
    │   ├─ emit Approval(owner: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, spender: UniStaker: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 1000)
    │   └─ ← true
    ├─ [323982] UniStaker::stake(1000, 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
    │   ├─ [61035] → new DelegationSurrogate@0xDDc10602782af652bB913f7bdE1fD82981Db7dd9
    │   │   ├─ [22515] Governance Token::delegate(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
    │   │   │   └─ ← ()
    │   │   ├─ [24735] Governance Token::approve(UniStaker: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
    │   │   │   ├─ emit Approval(owner: DelegationSurrogate: [0xDDc10602782af652bB913f7bdE1fD82981Db7dd9], spender: UniStaker: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
    │   │   │   └─ ← true
    │   │   └─ ← 63 bytes of code
    │   ├─ emit SurrogateDeployed(delegatee: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, surrogate: DelegationSurrogate: [0xDDc10602782af652bB913f7bdE1fD82981Db7dd9])
    │   ├─ [24651] Governance Token::transferFrom(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, DelegationSurrogate: [0xDDc10602782af652bB913f7bdE1fD82981Db7dd9], 1000)
    │   │   ├─ emit Transfer(from: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, to: DelegationSurrogate: [0xDDc10602782af652bB913f7bdE1fD82981Db7dd9], value: 1000)
    │   │   └─ ← true
    │   ├─ emit StakeDeposited(owner: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, depositId: 0, amount: 1000, depositBalance: 1000)
    │   ├─ emit BeneficiaryAltered(depositId: 0, oldBeneficiary: 0x0000000000000000000000000000000000000000, newBeneficiary: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)        
    │   ├─ emit DelegateeAltered(depositId: 0, oldDelegatee: 0x0000000000000000000000000000000000000000, newDelegatee: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
    │   └─ ← 0
    ├─ [0] VM::warp(2592061 [2.592e6])
    │   └─ ← ()
    ├─ [81702] UniStaker::claimReward()
    │   ├─ emit RewardClaimed(beneficiary: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, amount: 9666670 [9.666e6])
    │   ├─ [29846] Reward Token::transfer(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 9666670 [9.666e6])
    │   │   ├─ emit Transfer(from: UniStaker: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], to: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, value: 9666670 [9.666e6])
    │   │   └─ ← true
    │   └─ ← ()
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    ├─ [561] Reward Token::balanceOf(UniStaker: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [staticcall]
    │   └─ ← 333330 [3.333e5]
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000516120000000000000000000000000000000000000000000000000000000000000010737475636b656420726577617264733a00000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.20s
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Summary of the Test

• A reward of 10000000 WETH token gets sent into the Unistaker contract
• notifyRewardAmount() function is called i.e rewards start streaming immediately (and sets the rewardEndTime = block.timestamp + REWARD_DURATION).
• However, no user staked until a day later (86400 seconds)
• Alice stakes 1000 UNI tokens for the remaining reward duration (29 days)
• Since Alice is the only Staker for that reward duration, then the expectation is Alice will accrue the entire rewards (i.e 10000000 WETH)
• However, rewardEndTime ends and alice's accrued rewards = 9666670 WETH. This is solely because the rewards started streaming immediately the notifyRewardAmount() function was called not when the contract received its first stake (i.e when alice staked)
• The remaining rewards of 333330 WETH remain stuck in the contract until the next reward cycle which will likely encounter the same issue and a certain rewards token will always be stuck indefinitely in the contract.

Tools Used

Manual Review, Foundry, Solodit, Research

Recommended Mitigation Steps

The usual mitigation for this issue would be to set the start and end time (i.e. rewardEndTime) for the current reward cycle when the first Staker stakes instead of starting the process in the notifyRewardAmount. In short, defining rewardEndTime in the first stake when total deposits are zero.

Check out how the Sommelier team tackled this issue with _startProgram() https://github.com/PeggyJV/cellar-contracts/blob/afd970c36e9a520326afc888a11d40cdde75c6a7/src/CellarStaking.sol?ref=0xmacro.com#L219

Reference

• https://solodit.xyz/issues/m-12-rewards-for-initial-period-can-be-lost-in-all-of-the-synthetix-derivative-contracts-code4rena-lybra-finance-lybra-finance-git
• https://0xmacro.com/blog/synthetix-staking-rewards-issue-inefficient-reward-distribution/

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