UniStaker Infrastructure - twicek's results

Lines of code

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

Vulnerability details

Summary

The requested collected fees from a pool can be off-by-one compared to the actual amount received (and returned) when claiming all remaining fees in claimFees.

Description

Anyone can call V3FactoryOwner.claimFees to collect fees from a chosen Uniswap V3 pool by paying the set payoutAmount. Typically a caller will want to maximize the amount _amount0Requested and _amount1Requested that he can get from the pool.

V3FactoryOwner.sol#L181-L204

  function claimFees(
    IUniswapV3PoolOwnerActions _pool,
    address _recipient,
    uint128 _amount0Requested,
    uint128 _amount1Requested
  ) external returns (uint128, uint128) {
    PAYOUT_TOKEN.safeTransferFrom(msg.sender, address(REWARD_RECEIVER), payoutAmount);
    REWARD_RECEIVER.notifyRewardAmount(payoutAmount);
    (uint128 _amount0, uint128 _amount1) =
      _pool.collectProtocol(_recipient, _amount0Requested, _amount1Requested);


    // Protect the caller from receiving less than requested. See `collectProtocol` for context.
    if (_amount0 < _amount0Requested || _amount1 < _amount1Requested) {
      revert V3FactoryOwner__InsufficientFeesCollected();
    }
    emit FeesClaimed(address(_pool), msg.sender, _recipient, _amount0, _amount1);
    return (_amount0, _amount1);
  }


  /// @notice Ensures the msg.sender is the contract admin and reverts otherwise.
  /// @dev Place inside external methods to make them admin-only.
  function _revertIfNotAdmin() internal view {
    if (msg.sender != admin) revert V3FactoryOwner__Unauthorized();
  }

In collectProtocol:

• if amount0Requested == protocolFees.token0, then amount0 == amount0Requested.
• if amount1Requested == protocolFees.token1, then amount1 == amount1Requested.

In this case, amount0and amount1 are subtracted by 1 for gas saving purpose, so the actual amounts returned are:

• amount0 == amount0Requested - 1.
• amount1 == amount1Requested - 1.

UniswapV3Pool.sol#L848-L868

    function collectProtocol(
        address recipient,
        uint128 amount0Requested,
        uint128 amount1Requested
    ) external override lock onlyFactoryOwner returns (uint128 amount0, uint128 amount1) {
        amount0 = amount0Requested > protocolFees.token0 ? protocolFees.token0 : amount0Requested;
        amount1 = amount1Requested > protocolFees.token1 ? protocolFees.token1 : amount1Requested;


        if (amount0 > 0) {
            if (amount0 == protocolFees.token0) amount0--; // ensure that the slot is not cleared, for gas savings
            protocolFees.token0 -= amount0;
            TransferHelper.safeTransfer(token0, recipient, amount0);
        }
        if (amount1 > 0) {
            if (amount1 == protocolFees.token1) amount1--; // ensure that the slot is not cleared, for gas savings
            protocolFees.token1 -= amount1;
            TransferHelper.safeTransfer(token1, recipient, amount1);
        }


        emit CollectProtocol(msg.sender, recipient, amount0, amount1);
    }

Back in claimFees, the amounts returned from collectProtocol are checked against _amount0Requested and _amount1Requested:

V3FactoryOwner.sol#L189-L195

    (uint128 _amount0, uint128 _amount1) =
      _pool.collectProtocol(_recipient, _amount0Requested, _amount1Requested);


    // Protect the caller from receiving less than requested. See `collectProtocol` for context.
    if (_amount0 < _amount0Requested || _amount1 < _amount1Requested) {
      revert V3FactoryOwner__InsufficientFeesCollected();
    }

Since:

• amount0 == amount0Requested - 1.
• amount1 == amount1Requested - 1.

A call to claimFees will always revert if the user tries to collect all the fees available in a given pool.

The gas optimization made in the collectProtocol will make any call to collect the full amount of fee available revert.

Proof of Concept

The following is a foundry test that can be added to V3FactoryOwner.t.sol. You can run it using forge test --match-test "testFuzz_RevertIf_CallerExpectsMoreFeesThanPoolPaysOut_OffByOne". Note: the UniswapV3Pool used is a mock that will simulate the off-by-one scenario I described in my report.

  function testFuzz_RevertIf_CallerExpectsMoreFeesThanPoolPaysOut_OffByOne(
      uint256 _payoutAmount,
      address _caller,
      address _recipient,
      uint128 _amount0Requested,
      uint128 _amount1Requested
  ) public {
      _deployFactoryOwnerWithPayoutAmount(_payoutAmount);
      vm.assume(_caller != address(0) && _recipient != address(0));
      _amount0Requested = uint128(bound(_amount0Requested, 1, type(uint128).max));
      _amount1Requested = uint128(bound(_amount1Requested, 1, type(uint128).max));

      // Adjust the collected amounts to simulate the off-by-one scenario.
      uint128 _amount0Collected = _amount0Requested - 1;
      uint128 _amount1Collected = _amount1Requested - 1;
      pool.setNextReturn__collectProtocol(_amount0Collected, _amount1Collected);

      payoutToken.mint(_caller, _payoutAmount);

      vm.startPrank(_caller);
      payoutToken.approve(address(factoryOwner), _payoutAmount);

      // Expecting revert due to off-by-one error when attempting to collect max fees.
      vm.expectRevert(V3FactoryOwner.V3FactoryOwner__InsufficientFeesCollected.selector);
      factoryOwner.claimFees(pool, _recipient, _amount0Requested, _amount1Requested);
      vm.stopPrank();
  }

Impact

The defensive check put in place in claimFees is overconstrained and leads to a revert using nominal inputs.

Tool used

Manual Review.

Recommended Mitigation Steps

Consider taking into account the optimization made in the UniswapV3Pool contract by modifying the code as follows:

    (uint128 _amount0, uint128 _amount1) =
      _pool.collectProtocol(_recipient, _amount0Requested, _amount1Requested);


    // Protect the caller from receiving less than requested. See `collectProtocol` for context.
-   if (_amount0 < _amount0Requested || _amount1 < _amount1Requested) {
+   if (_amount0 < _amount0Requested - 1 || _amount1 < _amount1Requested - 1) {
      revert V3FactoryOwner__InsufficientFeesCollected();
    }

This modification allows any nominal call to claimFees to be executed without reverting. Note that someone could call claimFees with less than the maximum amount of fee collectable (even if this is against the incentives here). In this case, the modification I suggested could risk 1 token to a user, because the defensive check is less constrained. However, the implementation of collectProtocol would not allow this situation to happen anyway.

Assessed type

Invalid Validation