UniStaker Infrastructure - osmanozdemir1's results

Lines of code

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

Vulnerability details

This protocol is built to reward UNI token stakers by allowing them to claim some portion of the protocol fees in UNI pools. But there are so many pools and so many different fee tokens and distributing them separately would be a pain. That's why this protocol creates a mechanism and brings MEV searchers to the game.

Basically, MEV searchers race each other, buy the accumulated protocol fees from UNI pools by paying fixed amount of WETH, and get profit if the value of accumulated fees are greater than the paid WETH. This way MEV searcher gain profit, and Uniswap distributes rewards in WETH only.

Logically,

• We can expect these MEV searchers to buy accumulated fees as soon as they are profitable.

• We can also expect these bots to collect all fees in a pool to gain maximum value.

However, collecting all fees in a pool will always revert because of an integration issue between V3FactoryOwner.sol and Uniswap pools.

First, let's check the V3FactoryOwner::claimFees

  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) { //@audit it expects amounts to be equal at least
      revert V3FactoryOwner__InsufficientFeesCollected();
    }
    emit FeesClaimed(address(_pool), msg.sender, _recipient, _amount0, _amount1);
    return (_amount0, _amount1);
  }

This function collects protocol fees from the Uniswap pool and checks the collected amounts. It reverts if the collected amount is less than requested.

Now let's check the Uniswap pool.
https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/UniswapV3Pool.sol#L848C1-L868C6

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

As we can see here, the Uniswap pool decrements 1 wei from the requested amount to save gas if user requests all fees. However, as I mentioned above, the V3FactoryOwner does not allow even 1 wei difference. claimFees function will revert in that case.

The fastest MEV bot will try to collect all fees to gain profit, it will revert. In the meantime, another bot will collect fees before the first bot tries again.

I acknowledge that MEV searchers can try again with different values but the race will be lost already while trying again. Or one can argue that the bots should have requested all fees - 1 in the first place. However, the whole logic behind this reward mechanism is creating a race between MEV searchers, and I think this discrepancy between two parts of the Uniswap creates an unfair race.

Impact

MEV searchers that try to collect all fees in an Uniswap pool will lose their race.

Proof of Concept

Detailed explanation is provided above.

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

https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/UniswapV3Pool.sol#L857

Tools Used

Manual review

Recommended Mitigation Steps

Since the Uniswap itself subtracts 1 wei from maximum claimable amount during fee collection, V3FactoryOwner should allow 1 wei difference and not revert for a fair race and for these two parts of the Uniswap to be perfectly matched.

-    if (_amount0 < _amount0Requested || _amount1 < _amount1Requested) {
+    if (_amount0 < _amount0Requested - 1 || _amount1 < _amount1Requested - 1) {

Assessed type

Invalid Validation