TOB-UNIFEE-1: notifyRewardAmount() can be called without transferring tokens

[devtooligan]

Severity: Medium
Difficulty: High
Type: Data Validation
Found by: Echidna
Finding ID: TOB-UNIFEE-1
Target: UniStaker.sol#L574-L576

Description

notifyRewardAmount() can be called without transferring the reward tokens first. This would result in insufficient balance by the contract to cover all reward claims.

The impact of this is that users can claim more rewards than they are entitled to due to the rewardRate being artificially high. Eventually, there won't be enough reward tokens in the contract for all beneficiaries to withdraw, and the claimRewards transactions will revert. When this artificially inflated rewards amount gets high enough, the rewards feature is effectively disabled.

This issues is made possible by a bug in the check that is meant to ensure the reward amount was transferred in with the call:

https://github.com/uniswapfoundation/scopelift/blob/d745dd2a393f4b6a35bca2fd72f4cd198840c081/src/UniStaker.sol#L574-L576

As shown, the check does not consider any unclaimed reward amounts. When the amount of unclaimed rewards is greater than the new total reward amount, then this check will always pass.

According to the development team, this function is only intended to be called from the claimFees() function in V3FactoryOwner and within that function, the transfer is made prior to this call and no other account will be set as the rewardNotifier. In this case, if the protocol is deployed as intended, there would be no way to even call notifyRewardAmount() directly if the only rewardNotifier is the V3FactoryOwner.

However, there is no guarantee the contracts will be deployed in this manner or that no other account will be set as a rewardNotifier. While the likelihood of this issue being exploited is extremely low, the consequences if occurred could be significant.

Scenario

See PoC below showing an example of a staker, Eve, who receives double her fair reward due to this.

Mitigation

Short term, there is no clear cut mitigation for this. Here are some options:

• Change the UniStaker.notifyReward() function so that it uses transferFrom to ensure tokens are received.
• Introduce a way to track unclaimed rewards. The total rewards notified could be tracked in one state variable, and the rewards distributed in another. The net of the two is the unclaimed amount.
• Consider tighter restrictions when enabling a new rewardNotifier.
  • Does there need to be more than one active rewardIdentifier at any time?
  • Does the rewardIdentifier need to be a contract? (i.e. should we check the address has code at time of setting).

Either way, it is important to document this issue in both the UniStaker and V3FactoryOwner contracts as well as any related external documentation.

Long term, stateful invariant tests can help identify issues such as this.

Proof Of Concept

  function test_notifyRewardBug_eveStealsRewards() public {
    // An alternate reward notifier is set by admin
    address alternateRewardNotifier = address(0x123);
    vm.prank(admin);
    uniStaker.setRewardNotifier(alternateRewardNotifier, true);

    // Alice and Eve stake 100 gov tokens each
    uint stakeAmount = 100e18;

    // Alice stakes
    govToken.mint(alice, stakeAmount);
    vm.startPrank(alice);
    govToken.approve(address(uniStaker), stakeAmount);
    uniStaker.stake(stakeAmount, alice);
    vm.stopPrank();

    // Eve stakes
    govToken.mint(eve, stakeAmount);
    vm.startPrank(eve);
    govToken.approve(address(uniStaker), stakeAmount);
    UniStaker.DepositIdentifier eveDepositId = uniStaker.stake(stakeAmount, eve);
    vm.stopPrank();
    vm.warp(block.timestamp + 1 days);

    // Reward notifier calls notifyReward and transfers 10 reward tokens
    rewardToken.mint(address(rewardNotifier), 10e18);
    vm.startPrank(rewardNotifier);
    rewardToken.transfer(address(uniStaker), 10e18);
    uniStaker.notifyRewardAmount(10e18);

    // Since Alice and Eve have both staked the same amount, they should both be streamed
    // 5 reward tokens over the next 7 days
    uint expectedRewardPerStaker = 5e18;
    vm.warp(block.timestamp + 7 days);

    // alternateRewardNotifier calls notifyReward(10) WITHOUT transferring any reward tokens
    vm.startPrank(alternateRewardNotifier);
    uniStaker.notifyRewardAmount(10e18);
    vm.warp(block.timestamp + 7 days);

    // Eve claims double sized reward
    uint startingBalance = rewardToken.balanceOf(eve);
    vm.startPrank(eve);
    uniStaker.claimReward();
    uint endingBalance = rewardToken.balanceOf(eve);
    // Assert that Eve's reward is double the expected reward:
    assertEq(endingBalance - startingBalance, 2 * expectedRewardPerStaker - 1); // 1 wei is lost to rounding

    // Eve withdraws stake
    uniStaker.withdraw(eveDepositId, stakeAmount);
    assertEq(govToken.balanceOf(eve), stakeAmount);
    vm.stopPrank();

    // Eve has gotten away with her stake and a double reward.
    // There are no reward tokens left in the contract for Alice
    assertEq(rewardToken.balanceOf(address(uniStaker)), 0 + 1); // 1 wei is due to rounding

    // The contract's unclaimedRewards is permanently inflated by 10 reward tokens.
    // If the contract is to continue normal operations, 10 reward tokens would always be missing.
    // If this amount were higher, for example 500 WETH, then this contract would be effectively
    // bricked.
  }

Echidna

This issue was also identified by Echidna during our invariant testing campaign with the following call sequence:

Using this invariant check and associated handler:

  function echidna_rewardTokenBalanceGTERemainingRewards() external returns (bool) {
    uint256 remainingTime; // = uniStaker.REWARD_DURATION() - (block.timestamp -
      // uniStaker.rewardLastUpdateTime());
    if (uniStaker.rewardEndTime() <= block.timestamp) return true; // no remaining rewards to stream

    else remainingTime = uniStaker.rewardEndTime() - block.timestamp;
    return (
      (uniStaker.scaledRewardRate() * remainingTime)
        <= (uniStaker.REWARD_TOKEN().balanceOf(address(uniStaker)) * uniStaker.SCALE_FACTOR())
    );
  }

  function adminNotifyRewardsNOTRANSFER(uint256 amount) public {
    hevm.prank(rewardNotifier);
    uniStaker.notifyRewardAmount(amount);
    rewardsNotified += amount;
    rewardsSkipped += amount;
  }

[devtooligan]

At Trail of Bits, we assess findings on two distinct axes, Difficulty and Severity. This is in contrast to many other security firms that provide a singular rating such as low, medium or high.

Difficulty gauges how difficult it would be to trigger or exploit the reported issue. A 'low' rating indicates easy exploitability whilst a 'high' denotes stringent conditions for exploitation, necessitating privileged system access, intricate technical understanding, or the presence of other exploitable weaknesses.

In this case, the difficulty was assigned a 'high' rating meaning it's very difficult and unlikely it can be triggered. If Uniswap deploys these contracts as planned and the proper governance measures are taken to configure the contracts, then the likelihood of exploiting this issue is virtually eliminated. This is because, in the proposed system,notifyReward() is only called from within the V3FactoryOwner contract along with the transfer of funds. In order to exploit this issue, changes would have to be made to the V3FactoryOwner contract or else a different account would have to be inadvertently set as a rewardNotifier.

Severity assesses the actual impact should the issue be exploited. A 'medium' severity was assigned to this finding, owing to the potential for misuse where specific users could receive unmerited rewards however unlikely that may be.

The combination of a high difficulty and medium severity may align with what other entities might label as a 'low' risk finding. Trail of Bits utilizes this bi-dimensional evaluation system to provide comprehensive insights into an issue, reducing the risk of overlooking valid but unlikely issues due to an lower overall rating.

Excerpted from the Trail of Bits Vulnerabilities Category Appendix