zarkk01 - ```UniswapV4Implementation::beforeSwap()``` incorrectly the ```beforeSwapDelta_``` comparing ```tokenOut``` with ```amountSpecified``` while the ```amountSpecified``` is in ```WETH``` terms.

[sherlock-admin4]

zarkk01

Medium

UniswapV4Implementation::beforeSwap() incorrectly the beforeSwapDelta_ comparing tokenOut with amountSpecified while the amountSpecified is in WETH terms.

Summary

The UniswapV4Implementation::beforeSwap() compares two variables of different terms when it is about to change the beforeSwapDelta_ leading to unexpected behaviour of the hook.

Root Cause

In UniswapV4Implementation::beforeSwap(), when the user is about to swap WETH for a CollectionToken, there is two options. He can have specified either the exact CollectionTokens he wants to get out or he can have specified the exact WETHs he want to send in the Pool. The variable SwapParams.amountSpecified does this job and if it is negative then it represents the WETH in amount and if it is positive then it represents the CT out amount. Let's see the UniswapV4Implementation::beforeSwap():

function beforeSwap(address sender, PoolKey calldata key, IPoolManager.SwapParams memory params, bytes calldata hookData) public override onlyByPoolManager returns (bytes4 selector_, BeforeSwapDelta beforeSwapDelta_, uint24 swapFee_) {
             // ...

            // Since we have a positive amountSpecified, we can determine the maximum
            // amount that we can transact from our pool fees. We do this by taking the
            // max value of either the pool fees or the amount specified to swap for.
            if (params.amountSpecified >= 0) {
               // ...
            }
            // As we have a negative amountSpecified, this means that we are spending any amount
            // of token to get a specific amount of undesired token.
@>            else {
                (, ethIn, tokenOut, ) = SwapMath.computeSwapStep({
                    sqrtPriceCurrentX96: sqrtPriceX96,
                    sqrtPriceTargetX96: params.sqrtPriceLimitX96,
                    liquidity: poolManager.getLiquidity(poolId),
                    amountRemaining: int(pendingPoolFees.amount1),
                    feePips: 0
                });

                // If we cannot fulfill the full amount of the internal orderbook, then we want
                // to avoid using any of it, as implementing proper support for exact input swaps
                // is significantly difficult when we want to restrict them by the output token
                // we have available.
@>                if (tokenOut <= uint(-params.amountSpecified)) {
                    // Update our hook delta to reduce the upcoming swap amount to show that we have
                    // already spent some of the ETH and received some of the underlying ERC20.
                    // Specified = exact input (ETH)
                    // Unspecified = token1
                    beforeSwapDelta_ = toBeforeSwapDelta(ethIn.toInt128(), -tokenOut.toInt128());
                } else {
                    ethIn = tokenOut = 0;
                }
            }

            // ...
        }

        // Set our return selector
        selector_ = IHooks.beforeSwap.selector;
    }

Link to code

As we can see in the second @>, in the statement we compare tokensOut which is in CollectionToken terms with amountSpecified. However, this is in the case where amountSpecified is negative so it is referring to WETH terms and not CT terms.

Internal pre-conditions

1. Pool to have been initialized and have some liquidity.

External pre-conditions

1. A user to try to swap WETH for CT(collectionToken) with amountSpecified negative meaning in WETH terms.

Attack Path

1. Liquidity be added through Locker::initializeCollection() on the Pool.
2. User tries to swap WETH for ETH and specifies the WETH in.

Impact

The impact of this flaw on UniswapV4Implementation::beforeSwap() can cause unexpected reverts on some swaps with users not knowing what went wrong. As a result, it will be possible, due to this comparing of WETH and CollectionToken which may have different decimals and denomination), lots of swaps from WETH to CT to be reverted or passing while they shouldn't be.

PoC

No PoC needed.

Mitigation

To mitigate successfully this vulnerability, consider making this change on UniswapV4Implementation::beforeSwap() :

-     if (tokenOut <= uint(-params.amountSpecified)) 
+     if (ethIn <= uint(-params.amountSpecified))