UniStaker Infrastructure - jesjupyter's results

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/V3FactoryOwner.sol#L193-L195 https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/UniswapV3Pool.sol#L856-L865

Vulnerability details

Impact

In V3FactoryOwner::claimFees, a caller calls the function to claim the protocol fees accrued by a given Uniswap v3 pool contract. However, in UniswapV3Pool, the input amount0Requested and amount1Requested are properly handled to reduced by 1 to ensure that the slot is not cleared for gas savings. This case will cause unexpected revert if the caller wants to claim all fees accrued due to the requirement _amount0 < _amount0Requested || _amount1 < _amount1Requested and the lack of document nor comment on the input range of _amount0Requested, _amount1Requested.

Proof of Concept

In V3FactoryOwner::claimFees, a caller calls the function to claim the protocol fees accrued by a given Uniswap v3 pool contract.

    (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) { // @audit will revert, shouldn't be --? if (amount0 == protocolFees.token0) amount0--;
      revert V3FactoryOwner__InsufficientFeesCollected();
    }

There is a check to ensure that the caller won't receive less than requested.

However, in the context of UniswapV3Pool::collectProtocol, when amount0 == protocolFees.token0 or amount1 == protocolFees.token1, they are deducted by 1 so that the slot is not cleared.

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

Since there is no explicit doc nor comment of the range of input _amount0Requested, _amount1Requested, this will cause unexpected revert if the caller wants to claim all fees accrued but ends up with _amount0Requested-1 and _amount1Requested-1 which is an edge case and should not revert anyway.

The PoC is shown below (added in UniStaker.integration.t.sol):

  function test_UnexpectedRevert(
    
  ) public {
    uint128 _amountDai = 1 ether;
    uint128 _amountWeth = 2 ether;

    IERC20 weth = IERC20(payable(WETH_ADDRESS));
    IERC20 dai = IERC20(payable(DAI_ADDRESS));
    IUniswapPool daiWethPool = IUniswapPool(DAI_WETH_3000_POOL);

    // Amount should be high enough to generate fees
    _amountDai = uint128(bound(_amountDai, 1e18, 1_000_000e18));
    _amountWeth = uint128(bound(_amountWeth, 1e18, 1_000_000e18));
    uint256 totalDai = _amountDai;
    uint256 totalWeth = _amountWeth + PAYOUT_AMOUNT;

    vm.deal(address(this), totalWeth);
    deal(address(dai), address(this), totalDai, true);
    deal(address(weth), address(this), totalWeth);

    dai.approve(address(UNISWAP_V3_SWAP_ROUTER), totalDai);
    weth.approve(address(UNISWAP_V3_SWAP_ROUTER), totalWeth);
    weth.approve(address(v3FactoryOwner), PAYOUT_AMOUNT);

    _passQueueAndExecuteProposals();
    _swapTokens(DAI_ADDRESS, WETH_ADDRESS, totalDai);
    _swapTokens(WETH_ADDRESS, DAI_ADDRESS, _amountWeth);

    uint256 originalDaiBalance = IERC20(DAI_ADDRESS).balanceOf(address(this));
    uint256 originalWethBalance = IERC20(WETH_ADDRESS).balanceOf(address(this)) - PAYOUT_AMOUNT;

    (uint128 token0Fees, uint128 token1Fees) = daiWethPool.protocolFees();

    vm.expectRevert();
    v3FactoryOwner.claimFees(
      IUniswapV3PoolOwnerActions(DAI_WETH_3000_POOL), address(this), token0Fees, token1Fees
    ); // <= issue here
  }

The trace shows:

V3FactoryOwner__InsufficientFeesCollected()

Tools Used

Foundry

Recommended Mitigation Steps

Before _pool.collectProtocol, call _pool.protocolFees() to get the current fees. The function should not revert under this case.

    (uint128 token0Fees, uint128 token1Fees) = _pool.protocolFees();
    ...
    if ((_amount0 < _amount0Requested && _amount0Requested != token0Fees) || (_amount1 < _amount1Requested && _amount1Requested != token1Fees)) {
      revert V3FactoryOwner__InsufficientFeesCollected();
    }

Assessed type

DoS