Rewards accrued will be permanently locked when there are no stakers

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

When an MEV searcher claims the fees and there are no stakers for the entire REWARD_PERIOD, rewards will be locked forever.

Stakers only have 30 days to stake their tokens before all rewards are permanently locked.

A partial amount of these rewards will be lost anyway if someone stakes before the full 30 days (e.g. first stake after 3 days will result in a 10% total loss).

Proof of Concept

Suppose the following: no one has staked their token in UniStaker yet, so totalStaked = 0.

An MEV searcher calls V3FactoryOwner.claimFees immediately after deployment with a valid pool. Then UniStaker.notifyRewardAmount is also called:

  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;

    ...
  }

Due to totalStaked = 0, then rewardPerTokenAccumulatedCheckpoint is also zero:

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

Let's analyze the previous values step by step, let's say we are at block.timestamp = 1_000_000 for simplicity, REWARD_DURATION = 30 days = 2_592_000, and rewardAmount = 10e18

   rewardPerTokenAccumulatedCheckpoint = 0
   scaledRewardRate = (10e18 * 1e36) / 30 ~= 1e55
   rewardEndTime = 1_000_000 + REWARD_DURATION = 3_592_000
   lastCheckpointTime = 1_000_000

Worst-case scenario: suppose that in these 30 days, no one calls stake. We are now at timestamp = 3_592_001, and someone finally calls stake:

  function _stake(address _depositor, uint256 _amount, address _delegatee, address _beneficiary)
    internal
    returns (DepositIdentifier _depositId)
  {
    _revertIfAddressZero(_delegatee);
    _revertIfAddressZero(_beneficiary);

    _checkpointGlobalReward();
    _checkpointReward(_beneficiary);

    DelegationSurrogate _surrogate = _fetchOrDeploySurrogate(_delegatee);
    _depositId = _useDepositId();

    totalStaked += _amount;
    depositorTotalStaked[_depositor] += _amount;
    earningPower[_beneficiary] += _amount;
    deposits[_depositId] = Deposit({
      balance: _amount,
      owner: _depositor,
      delegatee: _delegatee,
      beneficiary: _beneficiary
    });
    _stakeTokenSafeTransferFrom(_depositor, address(_surrogate), _amount);
    emit StakeDeposited(_depositor, _depositId, _amount, _amount);
    emit BeneficiaryAltered(_depositId, address(0), _beneficiary);
    emit DelegateeAltered(_depositId, address(0), _delegatee);
  }

_checkpointGlobalReward is called:

	rewardPerTokenAccumulatedCheckpoint = rewardPerTokenAccumulated() = 0 (because totalStaked = 0)
	lastCheckpointTime = lastTimeRewardDistributed() = rewardEndTime = 3_592_000

_checkpointReward is called:

	unclaimedRewardCheckpoint[_beneficiary] = unclaimedReward(_beneficiary) = 0
	beneficiaryRewardPerTokenCheckpoint[_beneficiary] = rewardPerTokenAccumulatedCheckpoint = 0

Finally stake data is updated:

    totalStaked += _amount;
    depositorTotalStaked[_depositor] += _amount;
    earningPower[_beneficiary] += _amount;

Claim has no effect, as rewardPerTokenAccumulated() = 0 and beneficiaryRewardPerTokenCheckpoint[_beneficiary] = 0:

	/* @audit
		rewardPerTokenAccumulated() = 0 + (1e55 * (3_592_000 - 3_592_000)) / totalStaked = 0
	*/
	function unclaimedReward(address _beneficiary) public view returns (uint256) {
	    return unclaimedRewardCheckpoint[_beneficiary]
	      + (
	        earningPower[_beneficiary]
	          * (rewardPerTokenAccumulated() - beneficiaryRewardPerTokenCheckpoint[_beneficiary])
	      ) / SCALE_FACTOR;
	  }

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

So, the initial reward will be permanently locked inside UniStaker. Note that this loss of funds can occur even when the entire 30 days are not fully passed. For example, if no one stakes during the first 3 days, 10% of the rewards will be permanently locked anyway.

This can result in a large amount of funds being locked based on how big the rewards are. Suppose that payoutAmount is 10 WETH (this is currently worth $35k), so the total loss for 3 days would be 3.5k.

Coded PoC

Copy-paste the following test in test/UniStaker.t.sol, then run forge test --match-test test_RewardsAreLostWithNoStakers:

contract RewardsPermanentlyLocked is UniStakerRewardsTest {

  function test_RewardsAreLostWithNoStakers(
    address _depositor,
    address _delegatee,
    uint256 _stakeAmount,
    uint256 _rewardAmount
  ) public {
    (_stakeAmount, _rewardAmount) = _boundToRealisticStakeAndReward(_stakeAmount, _rewardAmount);
    // UniStaker has zero reward tokens
    assertEq(rewardToken.balanceOf(address(uniStaker)), 0);

    // The contract is notified of a reward, no stakers yet
    _mintTransferAndNotifyReward(_rewardAmount);

    // UniStaker has _rewardAmount
    assertEq(rewardToken.balanceOf(address(uniStaker)), _rewardAmount);

    // The full duration passes
    _jumpAheadByPercentOfRewardDuration(101);

    // A user deposits staking tokens
    _boundMintAndStake(_depositor, _stakeAmount, _delegatee);

    // UniStaker still has _rewardAmount
    assertEq(rewardToken.balanceOf(address(uniStaker)), _rewardAmount);

    // The user has no rewards
    assertLteWithinOnePercent(uniStaker.unclaimedReward(_depositor), 0);

    // wait some time
    skip(36_000);

    // The user still has no rewards
    assertLteWithinOnePercent(uniStaker.unclaimedReward(_depositor), 0);

    // UniStaker still has _rewardAmount, which is permanently locked
    assertEq(rewardToken.balanceOf(address(uniStaker)), _rewardAmount);
  }
}

Tools Used

Manual review

Recommended Mitigation Steps

I recommend one of these two solutions or a combination of them:

1. Consider adding a timelock to V3FactoryOwner.claimFees so that no one can claim fees for a reasonable amount of time, long enough to let users stake their tokens.

2. Consider reverting the TX when no one has staked their tokens in UniStaker.notifyRewardAmount yet (i.e. when totalStaked = 0).

Assessed type

Timing

[c4-judge]

MarioPoneder marked the issue as duplicate of #9

[c4-judge]

MarioPoneder marked the issue as not a duplicate

[c4-judge]

MarioPoneder marked the issue as primary issue

[c4-sponsor]

apbendi (sponsor) disputed

[apbendi]

We do not consider this a valid report for several reasons:

1. The system is behaving as designed. If there are no stakers, then no one earns rewards. Many of our tests demonstrate this, as it was what was expected.
2. In practice, this is extremely unlikely to occur: the DAO will put the staking contracts in place before turning on rewards fees for any pool.
3. Even if this were to occur, there is a mitigation: the DAO can act as a temporary rewards notifier to "re-notify" the contract of the rewards that are stuck, so they begin to distribute.

[MarioPoneder]

Thought about this for a while since I can agree with both sides, the wardens and the sponsor.
In the end, my logic is the following:

• No user loss occurs when no one is staking (unlikely) and rewards are locked in the contract. It's a problem of the protocol.
• The protocol has a built-in mechanism to re-use such temporarily stuck / undistributed rewards via their rewards notification method. This mechanism can be triggered by the DAO if necessary.

No user loss + protocol can handle this situation with current design / codebase, therefore invalidating this group of issues.
Furthermore, mitigating the unlikely totalStaked = 0 edge-case would only add complexity to the code, since the reward notification mechanism could already handle such a scenario.

[c4-judge]

MarioPoneder marked the issue as unsatisfactory:
Invalid

[EperezOk]

I believe this should be a valid Medium for three reasons:

1. The mitigation in which “the DAO can act as a temporary rewards notifier to "re-notify" the contract of the rewards that are stuck” feels a bit hacky, and goes against the documented and expected usage of UniStaker::notifyRewardAmount. We could also consider this a UX issue, same as Claiming all fees collected will revert because of not clearing slot check #34 which has been judged as a valid Medium.
2. If someone stakes UNI on the first reward cycle, and then unstakes it before the second one, it wouldn’t be fair that rewards that remained stuck for cycle1 are re-distributed on cycle2, where the user has nothing staked.
3. As I mentioned in Rewards may be lost during the first period #42, this issue has been judged as a valid Medium in several previous contests:

• Rewards for initial period can be lost in all of the synthetix derivative contracts 2023-06-lybra-findings#484
• https://github.com/code-423n4/2023-04-rubicon-findings/issues/1295
• Rewards are not rolled over 2022-09-y2k-finance-findings#93

[SovaSlava]

Agree with EperezOk, its should be valid medium, because DAO could not always track the presence of users in the stake contract.

Rewards, distributed in period, when there are no stakers, weth will be permanently locked inside UniStaker. This is a loss of funds.

Could be different situations, when no stakers in contract, but there is fact, that funds will be locked permanently in such situations.

[McCoady]

This issue should be at least a valid QA IMO.

Agree its hard to go higher given:

• Likelihood is low (requires either nobody is staking for a prolonged period or someone 'donates REWARD_TOKEN' by calling V3FactoryOwner::claimFees with 0 requested amounts before anyone has chance to stake.
• Severity is also low (likely would be a few seconds (or one block) of dripped rewards)

However stuck funds are stuck funds and the sponsors mitigation of the DAO giving an address power to call notifyRewardAmount (while having to calculate only the exact amount of undripped rewards to notify or risk introducing the same invariant breaking issue from the ToB report) seems less than ideal.

[SovaSlava]

The argument that the situation when there are no stakers or no one calls the notify function is unlikely, I consider incorrect. Because we cannot know whether the project will be successful or not. Therefore, it is necessary to consider all situations in this case.

[MarioPoneder]

Thanks for raising your concerns!

My take: At no point in time, funds are irrecoverably stuck. They simply cannot be claimed by a staker when no one is staking, which is a very unlikely edge-case. Users are never at a loss because of this. This behaviour is simply a design decision.
In case this unlikely scenario occurs, it would only be a temporary problem of the protocol, since the protocol comes with a built-in reward notification mechanism which allows to account for those unclaimed rewards later when explicitly triggered. According to the contest README, reasonable use of the notification mechanism is expected and from my perspective this can be considered reasonable use.
Considering those points, I don't see any gain for the protocol and especially its users to mitigate this edge-case. Therefore leaving this as it is.

Thank you for your understanding!

[SovaSlava]

@MarioPoneder , look at this case, please.

There is a situation where funds will be irretrievably lost.

January 1 - notifyreward
January 10 - user stake tokens
January 30 - user call unstake
February 5 - notifyreward

As a result, funds from the first notification call, which were intended for stakers in the period from January 1 to January 10 and were not distributed, will not be added on February 5, so they will be permanently lost