Loss of reward tokens when totalStaked remains 0

[c4-bot-5]

Lines of code

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

Vulnerability details

Impact

The UniStaker contract uses an accumulator to represent the overall reward accrued to all stakers. This accumulator value is stored in rewardPerTokenAccumulatedCheckpoint state variable.

This is how this variable gets updated:

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

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

It can be seen that when totalStaked is 0 then rewardPerTokenAccumulatedCheckpoint value is not updated, i.e, rewards don't accrue. For the entire duration when totalStaked remains 0 the rewardPerTokenAccumulatedCheckpoint isn't updated.

Hence the to-be-distributed reward token amount (scaledRewardRate * duration) for that duration gets locked inside the UniStaker contract forever. This amount can neither be claimed by any future stakers nor can it be recovered by the protocol owners. Hence resulting in permanent loss of reward funds.

Consider this scenario in which:

• 300 reward tokens are intended to be distributed in 30 days.
• For the days 11th to 20th noone remains staked in the UniStaker.
• Then only 200 tokens get distributed after 30 days.
• The reward for 11th to 20th days (100 tokens) remains stuck in the UniStaker contract forever.

In worst case if noone stakes in UniStaker for entire 30 day duration then all the reward tokens gets stuck in the contract.

A more likely scenario would be if the reward yield of UniStaker is low and no user wants to stake for those low rewards. These rewards should ideally be utilized in future to further boost the reward yield when users get interested in staking again.

Proof of Concept

// 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 {ERC20} from "openzeppelin/token/ERC20/ERC20.sol";
import {ERC20Permit} from "openzeppelin/token/ERC20/extensions/ERC20Permit.sol";

contract TokenFake is ERC20, ERC20Permit {
    constructor(string memory _name, string memory _symbol) ERC20(_name, _symbol) ERC20Permit(_name) {}
    function mint(address _account, uint256 _value) public {
        _mint(_account, _value);
    }
    function delegate(address) public {}
}

contract AuditTest is Test {
    UniStaker uniStaker;
    TokenFake uni;
    TokenFake weth;
    address admin;
    address notifier;
    address alice;
    address bob;
    address charlie;
    uint256 alicePK;
    uint256 bobPK;

    function setUp() public {
        admin = makeAddr("admin");
        notifier = makeAddr("notifier");
        (alice, alicePK) = makeAddrAndKey("alice");
        (bob, bobPK) = makeAddrAndKey("bob");
        charlie = makeAddr("charlie");

        uni = new TokenFake("Uniswap", "UNI");
        weth = new TokenFake("Wrapped Ether", "WETH");
        uniStaker = new UniStaker(IERC20(address(weth)), IERC20Delegates(address(uni)), admin);

        vm.prank(admin);
        uniStaker.setRewardNotifier(notifier, true);
    }

    function test_bug_whenTotalStakedIsZero() public {
        weth.mint(address(uniStaker), 1000e18);
        vm.prank(notifier);
        uniStaker.notifyRewardAmount(1000e18);

        uni.mint(alice, 100e18);
        vm.startPrank(alice);

        // total stakes remains 0
        UniStaker.DepositIdentifier depositId1 = uniStaker.stake(0, alice);
        vm.warp(block.timestamp + 30 days);

        assertEq(uniStaker.rewardPerTokenAccumulatedCheckpoint(), 0);
        assertEq(uniStaker.rewardPerTokenAccumulated(), 0);
        assertEq(uniStaker.totalStaked(), 0);

        uni.approve(address(uniStaker), 100e18);
        UniStaker.DepositIdentifier depositId2 = uniStaker.stake(100e18, alice);

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

        uniStaker.withdraw(depositId1, 0);
        uniStaker.claimReward();
        uniStaker.withdraw(depositId2, 100e18);
        uniStaker.claimReward();

        assertEq(weth.balanceOf(alice), 0);
        assertEq(uni.balanceOf(alice), 100e18);
        // The rewards funds remains stuck in contract forever
        assertEq(weth.balanceOf(address(uniStaker)), 1000e18);
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

Consider allocating the unaccrued yield in a separate state variable and utilize it to boost rewards when totalStaked becomes non-zero.

Assessed type

Context

[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