Expected interaction with claimFees will revert

[c4-bot-1]

Lines of code

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

Vulnerability details

MED: Expected interaction with claimFees will revert
LOC: https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5a2761c8277541a24bc551fbd624413b384bea94/src/V3FactoryOwner.sol#L193

Description

Users are expected to call claimFees() to get fees in exchange for supplied WETH. The inputs (amount0Requested, amount1Requested) are passed to the Pool.

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

If less than the supplied amounts is actually pulled from the Pool, the contract reverts (to protect from executing a losing operation after frontrunning, for example).

The issue is an edge case of collectProtocol() is not accounted for.

If amount0Requested is the entire token0 fees, amount0 is reduced by 1 before returning (to prevent clearing the slot). But since the fee amount exists in the Pool, it is expected that users will pass amount0Requested = rotocolFees.token0 and similarly for amount1. As a result, the protocol is likely to revert when used in a completely reasonable way by integrators.

Note that since the protocol is deployed in Mainnet, the revert represents a non-negligible value loss of gas by the sender.

Impact

Users of claimFees() are likely to experience reverts and hence a loss of gas.

Tools Used

Manual audit

Recommended Mitigation Steps

Relax the check below:

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

If the amount is 1 wei less than the requested amount, claimFees() execution should be considered valid.

Assessed type

Context

[c4-judge]

MarioPoneder marked the issue as duplicate of #34

[c4-judge]

MarioPoneder marked the issue as satisfactory