search

sherlock-audit / 2023-01-uxd-judging

3 stars 1 forks source link

hansfriese - `PerpDepository._rebalanceNegativePnlWithSwap()` shouldn't use a `sqrtPriceLimitX96` twice. #425

Open github-actions[bot] opened 1 year ago

github-actions[bot] commented 1 year ago

hansfriese

medium

PerpDepository._rebalanceNegativePnlWithSwap() shouldn't use a sqrtPriceLimitX96 twice.

Summary

PerpDepository._rebalanceNegativePnlWithSwap() shouldn't use a sqrtPriceLimitX96 twice.

Vulnerability Detail

Currently, _rebalanceNegativePnlWithSwap() uses a sqrtPriceLimitX96 param twice for placing a perp order and swapping.

    function _rebalanceNegativePnlWithSwap(
        uint256 amount,
        uint256 amountOutMinimum,
        uint160 sqrtPriceLimitX96,
        uint24 swapPoolFee,
        address account
    ) private returns (uint256, uint256) {
        uint256 normalizedAmount = amount.fromDecimalToDecimal(
            ERC20(quoteToken).decimals(),
            18
        );
        _checkNegativePnl(normalizedAmount);
        bool isShort = false;
        bool amountIsInput = true;
        (uint256 baseAmount, uint256 quoteAmount) = _placePerpOrder(
            normalizedAmount,
            isShort,
            amountIsInput,
            sqrtPriceLimitX96
        );
        vault.withdraw(assetToken, baseAmount);
        SwapParams memory params = SwapParams({
            tokenIn: assetToken,
            tokenOut: quoteToken,
            amountIn: baseAmount,
            amountOutMinimum: amountOutMinimum,
            sqrtPriceLimitX96: sqrtPriceLimitX96, //@audit 
            poolFee: swapPoolFee
        });
        uint256 quoteAmountOut = spotSwapper.swapExactInput(params);

In _placePerpOrder(), it uses the uniswap pool inside the perp protocol and uses a spotSwapper for the second swap which is for the uniswap as well.

But as we can see here, Uniswap V3 introduces multiple pools for each token pair and 2 pools might be different and I think it's not good to use the same sqrtPriceLimitX96 for different pools.

Also, I think it's not mandatory to check a sqrtPriceLimitX96 as it checks amountOutMinimum already. (It checks amountOutMinimum only in _openLong() and _openShort().)

Impact

PerpDepository._rebalanceNegativePnlWithSwap() might revert when it should work as it uses the same sqrtPriceLimitX96 for different pools.

Code Snippet

https://github.com/sherlock-audit/2023-01-uxd/blob/main/contracts/integrations/perp/PerpDepository.sol#L478

Tool used

Manual Review

Recommendation

I think we can use the sqrtPriceLimitX96 param for one pool only and it would be enough as there is an amountOutMinimum condition.

WarTech9 commented 1 year ago

Fix: https://github.com/UXDProtocol/uxd-evm/pull/24

IAm0x52 commented 1 year ago

Fix looks good. sqrtPriceLimitX96 is now only applied to the second swap