Panoptic - ptsanev's results

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L961-L980

Vulnerability details

Impact

The _createLegInAMM() function is used to create AMM positions using the legs of a specific tokenId. The function uses an unchecked block to save on gas by manually checking for over/underflows, but there's a scenario where the removedLiquidity could underflow.

Proof of Concept

The block of code we are looking at is:

if (_isBurn) {
   removedLiquidity -= chunkLiquidity;
}

Upon burning a position with a isLong=0, this means we are closing a long position, so we should remove short liquidity. Of course, we can only burn liquidity if we have provided it first, which happens when we mint a isLong=1 position where removedLiquidity += chunkLiquidity. The problem is in the value of chunkLiquidity. As stated by the LiquidityChunk library: "A liquidity chunk is an amount of liquidity (an amount of WETH, e.g.) deployed between two ticks: tickLower and tickUpper into a concentrated liquidity AMM". The value of chunkLiquidity between calls can easily fluctuate, since it tracks total liquidity between ticks for a specific leg of a tokenId, but since any user can use any tokenId, some users are bound to use the same tokenId and move the liquidityChunk's liquidity inbetween calls. A simple scenario on the top of my head is a user1 mints, his removedLiquidty = x. Then another user2 mints and his removedLiquidity = y, but the liquidity in the chunk would have increased, meaning if user1 tries to burn his long position removedLiquidity would underflow, breaking user accounting.

Tools Used

Manual Review

Recommended Mitigation Steps

Remove the unchecked field or manually check if the chunkLiquidity does not exceed the removedLiquidity, just like it is done in if (startingLiquidity < chunkLiquidity) revert Errors.NotEnoughLiquidity();

Assessed type

Under/Overflow

[L-01] - usage of slot0 to get spot data of Univ3 pool is prone to manipulation:

        (, newTick, , , , , ) = univ3pool.slot0();

        if ((newTick >= tickLimitHigh) || (newTick <= tickLimitLow)) revert Errors.PriceBoundFail();

Use observe() instead

[L-02] - rebasing tokens would work against the users in events of negative rebases, as stated by the uniswap docs. This detail was probably missed, but due to not being mentioned in the README - Low

[L-03] - the ERC1155Minimal does not implement a balanceOf(), only a balanceOfBatch() function, possibly tampering with potential external integrations that would use it's balanceOf(). Incompliance to ERC1155

[L-04] - tokens with blacklists would not allow users to burn their already opened positions, consider creating a mechanism for removing such users' liquidity, compensating the protocol with their fees.

[L-05] - Slippage tolerance values should be inclusive. Consider rewriting if ((newTick >= tickLimitHigh) || (newTick <= tickLimitLow)) to -> if ((newTick > tickLimitHigh) || (newTick < tickLimitLow)) in order to allow the edge-cases to be accepted.