Lack of the deadline check in claimFees may cause loss of funds

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

The user who attempts to buy protocol fees in V3FactoryOwner.sol may lose his funds in a bad deal if claimFees transaction is executed later in time.

Proof of Concept

Users can pay a fixed amount of tokens to purchase protocol fees accumulated in 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);
  }

For this deal to be profitable PAYOUT_AMOUNT should be less than received fees value. Let's take example from the comment below
https://github.com/code-423n4/2024-02-uniswap-foundation/blob/main/src/V3FactoryOwner.sol#L157-L166

USDC/USDT pool has accumulated fees which value is 26K USD, Alice calls claimFees(usdc_usdt, alice, 13000 ether, 13000 ether) in order to receive 13K USDT and 13K USDC tokens in exchange for PAYOUT_AMOUNT of 10 WETH that is 10 * 2500 = $25K, the profit is 26K - 25K = $1000.

Now imagine that her transaction was executed with a significant time delay due to network conditions or a low gas amount, and WETH/USD price increased to 2700, this would result in a loss of funds as Alice receives 26K in USDC and USDT tokens in exchange for 10 * 2700 = $27K.

Tools Used

Manual review

Recommended Mitigation Steps

Add the deadline check in claimFees

Assessed type

Timing

[MarioPoneder]

Deadline consideration seems to be valid cause only minimum token amounts are enforced but not their actual price value which might change over time.
On the contrary, the user has the ability to revoke the approval.

[c4-judge]

MarioPoneder marked the issue as primary issue

[c4-sponsor]

apbendi (sponsor) disputed

[apbendi]

The scenario described is both highly unlikely (the transaction would not remain in the pool long enough for price changes to matter, someone else would arb it) and completely within the searchers control to mitigate (they can wrap their claims in any contract logic they prefer). This is simply a property of MEV, we do not consider it an issue.

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder marked issue #331 as primary and marked this issue as a duplicate of 331

[MarioPoneder]

Deadline checks as well as slippage checks can be considered state-of-the art and a protocol bears a responsibility to introduce such measures in order to protect its users. Furthermore, the pending transaction doesn't necessarily need to fail even when frontrun by someone else and could still lead to losses (better explained in #331).

However, also the user / MEV searcher bears a responsibility to care about their transactions as they can be cancelled easily. Having a pending transaction in the mempool for e.g. a day (see also PoC of #331) must be considered a careless user error.
See also: https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#verdict-severity-standardization-conditional-on-user-mistake

[c4-judge]

MarioPoneder removed the grade

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-b

[c4-judge]

This previously downgraded issue has been upgraded by MarioPoneder

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder marked the issue as grade-a