Due to the gas saving mechanism of the uniswap pool’s collectProtocol method, the auctions may often revert griefly.

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/491c7f63e5799d95a181be4a978b2f074dc219a5/src/V3FactoryOwner.sol#L189-L195
https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/UniswapV3Pool.sol#L857
https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/UniswapV3Pool.sol#L862

Vulnerability details

Impact

When the user collects the fees collected by the current protocol, the pool.collectProtocol method will automatically reduce the fee amount by one instead of clearing the fee storage in order to save gas, which causes the verification of V3FactoryOwner.claimFees to fail and triggers the V3FactoryOwner__InsufficientFeesCollected error.
This results in auctions that may often revert griefly, affecting the interests of the auctioneer.

Proof of Concept

This is an interactive combination error involving two points:

1. In order to save gas, UniswapV3Pool.collectProtocol will automatically reduce the fee amount by one instead of clearing the storage.
2. V3FactoryOwner.claimFees will verify the collect fee amount, and the reduced amount will cause the transaction to be reverted.

Why does this error often trigger griefly and affect the interests of the auctioneer? Let‘s take a look at the execution process:

1. The fees in the pool increase from zero. The relevant auctioneer has been monitoring the amount of fees through UniswapV3Pool.protocolFees, but is not aware of the existence of this issue. When the fee value exceeds payoutAmount, the auctioneer passes the amount of fees obtained.
2. At this time, there are two auction parties A and B competing. A and B initiated transactions in block N at the same time. A's tx was executed first with a high gasPrice, but the transaction failed due to the existence of this issue; while B's tx is delayed until block N + 1 due to the lower gasPrice.
3. And there is a swap of the pool between the transactions of A and B, which increases the protocolFees of the pool, so B's tx can be executed normally and wins the auction.

The above process violates the principle of auction. A's tx is executed first but lose, while B's tx is executed after the fee is increased and wins the auction.
This is a specific example that affects the interests of the auctioneer. Due to the timing of transaction execution, the error is implicit and will not be discovered for a period of time, affecting the revenue of the auctioneer.

The following is the specific POC:

diff --git a/test/V3FactoryOwner.t.sol b/test/V3FactoryOwner.t.sol
index 2db6623..2c38861 100644
--- a/test/V3FactoryOwner.t.sol
+++ b/test/V3FactoryOwner.t.sol
@@ -477,4 +477,27 @@ contract ClaimFees is V3FactoryOwnerTest {
     factoryOwner.claimFees(pool, _recipient, _amount0Requested, _amount1Requested);
     vm.stopPrank();
   }
+
+  function testUserClaimAllFeesRevertGriefly() public {
+    uint128 payoutAmount = 1 ether;
+    uint128 amount0Collected = 1 ether;
+    uint128 amount1Collected = 2 ether;
+    _deployFactoryOwnerWithPayoutAmount(payoutAmount);
+    pool.setNextReturn__collectProtocol(amount0Collected, amount1Collected);
+    (uint128 amount0, uint128 amount1) = pool.protocolFees();
+    assertEq(amount0, amount0Collected);
+    assertEq(amount1, amount1Collected);
+
+    address user = makeAddr("User");
+    payoutToken.mint(user, payoutAmount);
+    vm.startPrank(user);
+    payoutToken.approve(address(factoryOwner), payoutAmount);
+    // @audit The user queries the fees collected by the protocol and passes them in as parameters.
+    // @audit As a result, the transaction fails due to uniswap's pool gas saving mechanism.
+    vm.expectRevert(V3FactoryOwner.V3FactoryOwner__InsufficientFeesCollected.selector);
+    factoryOwner.claimFees(pool, user, amount0, amount1);
+    // @audit The user can only reduce the amount by one
+    factoryOwner.claimFees(pool, user, amount0 - 1, amount1 - 1);
+    vm.stopPrank();
+  }
 }
diff --git a/test/mocks/MockUniswapV3Pool.sol b/test/mocks/MockUniswapV3Pool.sol
index 1977a8d..fb78794 100644
--- a/test/mocks/MockUniswapV3Pool.sol
+++ b/test/mocks/MockUniswapV3Pool.sol
@@ -26,14 +26,37 @@ contract MockUniswapV3Pool is IUniswapV3PoolOwnerActions {
     mockFeesAmount1 = amount1;
   }
 
+  function protocolFees() public view returns (uint128 amount0, uint128 amount1) {
+    if (useMockProtocolFeeAmounts) {
+      amount0 = mockFeesAmount0;
+      amount1 = mockFeesAmount1;
+    }
+  }
+
   function collectProtocol(address recipient, uint128 amount0Requested, uint128 amount1Requested)
     external
-    returns (uint128, uint128)
+    returns (uint128 amount0, uint128 amount1)
   {
     lastParam__collectProtocol_recipient = recipient;
     lastParam__collectProtocol_amount0Requested = amount0Requested;
     lastParam__collectProtocol_amount1Requested = amount1Requested;
-    if (useMockProtocolFeeAmounts) return (mockFeesAmount0, mockFeesAmount1);
-    return (amount0Requested, amount1Requested);
+    amount0 = amount0Requested;
+    amount1 = amount1Requested;
+
+    if (useMockProtocolFeeAmounts) {
+      amount0 = amount0Requested > mockFeesAmount0 ? mockFeesAmount0 : amount0Requested;
+      amount1 = amount1Requested > mockFeesAmount1 ? mockFeesAmount1 : amount1Requested;
+
+      if (amount0 > 0) {
+          if (amount0 == mockFeesAmount0) amount0--; // ensure that the slot is not cleared, for gas savings
+          mockFeesAmount0 -= amount0;
+          // TransferHelper.safeTransfer(token0, recipient, amount0);
+      }
+      if (amount1 > 0) {
+          if (amount1 == mockFeesAmount1) amount1--; // ensure that the slot is not cleared, for gas savings
+          mockFeesAmount1 -= amount1;
+          // TransferHelper.safeTransfer(token1, recipient, amount1);
+      }
+    }
   }
 }

Tools Used

Foundry

Recommended Mitigation Steps

The verification of V3FactoryOwner.claimFees should take into account the gas saving mechanism of UniswapV3Pool.collectProtocol and be consistent with it to eliminate interactive combination errors.

Assessed type

Context

[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