Claiming all fees results in unexpected revert

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

Attempting to claim all accrued fees from a Uniswap v3 pool triggers a revert due to a gas optimization in the UniswapV3Pool function that collects protocol fees.

This unexpected behavior can result in MEV searchers missing out on potential profits.

Proof of Concept

The issue lies in V3FactoryOwner#claimFees() which uses UniswapV3Pool#collectProtocol() to collect and send the claimed fees:

  function claimFees(
    IUniswapV3PoolOwnerActions _pool,
    address _recipient,
    uint128 _amount0Requested,
    uint128 _amount1Requested
  ) external returns (uint128, uint128) {
    // ...
    (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();
    }
    // ...
  }

Notice how if the fees available for collection are less than requested, this results in a V3FactoryOwner__InsufficientFeesCollected() revert.

Now let's take a look at the implementation of UniswapV3Pool#collectProtocol():

    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) {
            // @audit The gas optimization in question 👇
            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) {
	    // @audit The gas optimization in question 👇
            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);
    }

Notice the gas saving which decrements the amount of fees to be collected by 1 wei if all of the accrued fees in the pool are requested. This is to ensure that the storage slot for protocolFees.token0 and protocolFees.token1 are not set to zero which would be a costly operation from gas perspective.

What this means is that if a MEV searcher tries to claim all accumulated fees in a pool, their transaction will fail due to the above mentioned check check in V3FactoryOwner:

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

The likelihood of this vulnerability is very high because:

• 1. MEV searchers are inherently motivated to maximize their profits, leading them to attempt to claim all available fees from a pool;
• Given that MEV searchers operate through automated scripts, they would logically read and check the total available fees for profitability before the script claims them;

Thus, the impact of this issue is very high and would result in missed profits for many MEV searchers.

Tools Used

Manual review

Recommended Mitigation Steps

While far from ideal, considering that 1 wei is dust, the easiest solution would be to offset the check by 1 wei in V3FactoryOwner#claimFees() to account for the gas optimization in UniswapV3Pool#collectProtocol():

diff --git a/src/V3FactoryOwner.sol b/src/V3FactoryOwner.sol
index 432ffc1..5012197 100644
--- a/src/V3FactoryOwner.sol
+++ b/src/V3FactoryOwner.sol
@@ -190,7 +190,7 @@ contract V3FactoryOwner {
       _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 wei) || _amount1 < (_amount1Requested - 1 wei)) {
       revert V3FactoryOwner__InsufficientFeesCollected();
     }
     emit FeesClaimed(address(_pool), msg.sender, _recipient, _amount0, _amount1);

Alternatively, allowing type(uint128).max as a value for _amount0Requested and _amount1Requested could indicate a request to claim all fees, avoiding the issue entirely.

Assessed type

Error

[c4-judge]

MarioPoneder marked the issue as duplicate of #34

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder marked the issue as grade-b