Incorrect validation of effectiveLiquidityLimitX32 in _checkLiquiditySpread()

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L1481-L1494

Vulnerability details

Impact

Incorrect validation of effectiveLiquidityLimitX32 in _checkLiquiditySpread(), this will allow minting to succeed even when effectiveLiquidityLimitX32 is exceeded.

Proof of Concept

In SFPM, net = totalLiquidity - removedLiquidity, so totalLiquidity = removedLiquidity + netLiquidity. As seen in the comment below

             net amount    
           received minted  
          ▲ for isLong=0     amount           
          │                 moved out      actual amount 
          │  ┌────┐-T      due isLong=1   in the UniswapV3Pool 
          │  │    │          mints      
          │  │    │                        ┌────┐-(T-R)  
          │  │    │         ┌────┐-R       │    │          
          │  │    │         │    │         │    │     
          └──┴────┴─────────┴────┴─────────┴────┴──────►                     
             total=T       removed=R      net=(T-R)

Also, s_accountLiquidity holds netLiquidty in the right slot, while removedLiquidty is held in the left slot.

When minting a long position, _addUserOption() makes a call to PP._checkLiquiditySpread(), to put a limit on how much new liquidity in one transaction can be deployed into this leg(comment).

 // put a limit on how much new liquidity in one transaction can be deployed into this leg
        // the effective liquidity measures how many times more the newly added liquidity is compared to the existing/base liquidity
        if (effectiveLiquidityFactorX32 > uint256(effectiveLiquidityLimitX32))
            revert Errors.EffectiveLiquidityAboveThreshold();

The core problem lies in the fact that the formula to calculate effectiveLiquidityFactorX32 is totalLiquidity/netLiquidity, but uses the the value from the left slot of s_accountLiquidity[positionKey] as totalLiquidity.

 LeftRightUnsigned accountLiquidities = SFPM.getAccountLiquidity(
            address(s_univ3pool),
            address(this),
            tokenId.tokenType(leg),
            tickLower,
            tickUpper
        );

        uint128 netLiquidity = accountLiquidities.rightSlot();
   @>   uint128 totalLiquidity = accountLiquidities.leftSlot();

Even in the same PP, totalLiquidity was said to be removed + net inside _getTotalLiquidity() function.

 // removed + net
            totalLiquidity = accountLiquidities.rightSlot() + accountLiquidities.leftSlot();

The implication is that the calculation for effectiveLiquidityFactorX32

  uint256 effectiveLiquidityFactorX32;
        unchecked {
            effectiveLiquidityFactorX32 = (uint256(totalLiquidity) * 2 ** 32) / netLiquidity;
        }

Tools Used

Manual review.

Recommended Mitigation Steps

totalLiquidity should include left and right chunk of accountLiquidities

Assessed type

Invalid Validation

[dyedm1]

This comment is confusing (so this can be a QA issue). It actually is defined as total(Removed)liquidity/netLiquidity, and you can see that is consistent with the maximum limit (9x removed/1x net = 90% max).

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Picodes]

Downgrading to QA under "function incorrect as to spec, issues with comments"