c4-bot-2 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

Effect

Valid input that results in a reverting transaction

Degraded functionality of the protocol

Probability

High (high likelihood of inputting the exact amount of claimable protocol fees)

Summary

The V3FactoryOwner.claimFees() function, can be called by any caller to claim the protocol fees by providing the _amount0Requested and _amount1Requested values to the function as input parameters.

Above values are the protocol fees caller expects to receive from the pool. But if the user inputs the total available protocol fee per each token (Which the user is likely to do to get the full amount of protocol fees) the transaction will revert due to the following condition failing:

    if (_amount0 < _amount0Requested || _amount1 < _amount1Requested) {
      revert V3FactoryOwner__InsufficientFeesCollected();
    }

Proof of Concept

Let's consider the following Scenario:

The UniswapV3 pool contract under the UniswapV3Pool.collectProtocol function implements the following logic before the fees are transferred:

        if (amount0 > 0) {
            if (amount0 == protocolFees.token0) amount0--; 
            ...
        }
        if (amount1 > 0) {
            if (amount1 == protocolFees.token1) amount1--; 
            ...
        }

As per the above logic, if amount0 == protocolFees.token0 (i.e the total available amount is to be transferred) then amount0 is deducted by one (This is to save gas by ensuring that the protocolFees.token0 slot is not empty).
However, when the transaction execution returns to the V3FactoryOwner.claimFees function, the _amount0 < _amount0Requested condition will be true and the transaction will revert since the returned _amount0 value will be less than 1 compared to the _amount0Requested value.
The probability of this transaction reverting is high, since users are highly likely to pass in the highest claimable fee amount to the claimFees() function. Indeed, why pass any amount less than what they can claim? (even by 1 wei).

As this pertains to a degraded functionality of the protocol and that the bug Should be fixed (otherwise, the User Experience will highly likely suffer), the appropriate estimated severity here is Medium.

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

The fix could exist at the smart contract level by updating the conditional check in the V3FactoryOwner.claimFees() function to withstand the off by one difference:

    if (_amount0 < (_amount0Requested - 1) || _amount1 < (_amount1Requested - 1)) {
      revert V3FactoryOwner__InsufficientFeesCollected();
    }

By doing this it is assumed that the user is willing to accept 1 wei less in terms of the received protocol fees, when claiming the maximum available protocol fees.

However, given that the highest impact is on the User Experience, we recommended handling this vulnerability at the frontend UI level by sending 1 wei less to the V3FactoryOwner.claimFees() if the user inputs the maximum (or more) claimable protocol fee amount of the Uniswap pool. This will bypass the amount0 == protocolFees.token0 check in the UniswapV3Pool.collectProtocol, thus the V3FactoryOwner.claimFees() transaction will not revert.

Assessed type

DoS

c4-judge commented 8 months ago

MarioPoneder marked the issue as duplicate of #34