Panoptic - fnanni's results

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L979 https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L586-L630 https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/tokens/ERC1155Minimal.sol#L222-L229

Vulnerability details

Impact

By minting, burning and partial transferring positions that share position keys but not token ids, it's possible to reach states in which account's liquidities and minted positions are mismatched.

For example, non-existent positions could have positive removedLiquidities and positions could have huge, corrupted removedLiquidities due to underflowing. In both cases, the calculation of the premium gets corrupted, which could freeze some positions if the calculation reverts or could assign absurd premiums and fees that might lead to economic losses depending on how other smart contracts (not part of this audit) in Panoptic use the SemiFungiblePositionManager contract.

Proof of Concept

Here's a simple test example that puts the contract into the state mentioned above: https://gist.github.com/fnanni-0/e966047e79c87755575de88344661de9. Executing forge test -vv --match-test test_Success_RemovedLiquidityManipulation results in the following output which explains step by step what is happening.

Alice mints 1010 of token C (short put, i.e. adds liquidity to AMM).

  Minted positionSize: 1010
  Alice balance token C: 1010
  Added liquidity: 50
  Removed liquidity: 0
  

Alice mints 810 of token A (long put, i.e. removes liquidity from AMM). 
Token A and Token C have the same position key.

  Minted positionSize: 810
  Alice balance token A: 810
  Added liquidity: 10
  Removed liquidity: 40
  

Partial transfer 210 of token C to Bob. 
Alice is left with active positions (800 of token C and 810 of token A) but liquidity is now empty.

  Alice balance token C: 800
  Alice balance token A: 810
  Added liquidity: 0
  Removed liquidity: 0
  

Alice burns token A --> removed liquidity underflows.

  Alice balance token A: 0
  Alice balance token C: 800
  Added liquidity: 40
  Removed liquidity: 340282366920938463463374607431768211416
  

Bob burns token C --> removed liquidity is still non zero, but Bob doesn't have active positions.

  Bob balance token A: 0
  Bob balance token C: 0
  Bob's added liquidity: 0
  Bob's removed liquidity: 40

Alternatively, a similar scenario can be reached by reentering SemiFungiblePositionManager's transfer function. During the mint of short positions, ERC1155's onERC1155Received() can be used to safeTransferFrom() the tokens that are being minted before the liquidity is actually minted. If the token being minted has the same position key as previously minted short and long positions, safeTransferFrom() can be called with an amount that doesn't revert when registerTokenTransfer() is invoked.

Tools Used

Manual inspection.

Recommended Mitigation Steps

Taking into account that transfers are already limited, the simplest solution would be to remove the transfer feature completely.

If removing transfers is not an option, consider requiring the removedLiquidity to be zero or splitting the transfer of added liquidities from the removed liquidities. Remember that this bug exists because legs with different token ids might write to the same position keys.

Fixing the reentrancy risk might not be necessary depending on how this issue is resolved. The problem seems to happen because there's a mishandling of removedLiquidities. However, if the reentrancy needed to be blocked, checking ReentrancyLock(tokenId.univ3pool()) inside registerTokenTransfer() will do.

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/tokens/ERC1155Minimal.sol#L90-L118 https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L586-L630

Vulnerability details

Impact

Currently, ERC1155Minimal.sol allows 0 transfers, which means that tokens that were not minted can be "transferred". Normally this wouldn't be a problem, but in registerTokenTransfer a 0 transfer can actually transfer non-zero liquidity due to the possibility of different token ids sharing the same liquidity position key.

This could lead to unexpected scenarios in which liquidity gets trapped or removedLiquidity underflows, affecting the calculation of premiums and fees.

Proof of Concept

Here is a simple test example that exploits the mentioned behavior to reach an unexpected state https://gist.github.com/fnanni-0/8ebb660b1d4ef518a409e2b82b293113. Running forge test -vv --match-test test_Success_ZeroTransfers outputs a step by step explanation of what is happening:

Alice mints 1010 of token C (short put, i.e. adds liquidity to AMM).

  Minted positionSize: 1010
  Alice balance token C: 1010
  Added liquidity: 50
  Removed liquidity: 0
  

Alice mints 1010 of token A (long put, i.e. removes all liquidity from AMM). Token A and Token C have the same position key.

  Minted positionSize: 1010
  Alice balance token A: 1010
  Added liquidity: 0
  Removed liquidity: 50
  

Zero transfer of non-existent token B to Bob. Alice is left with active positions (1010 of token C and 1010 of token A) but liquidity is now empty.

  Alice balance token C: 1010
  Alice balance token B: 0
  Alice balance token A: 1010
  Bob balance token C: 0
  Bob balance token B: 0
  Bob balance token A: 0
  Added liquidity: 0
  Removed liquidity: 0
  Bob's added liquidity: 0
  Bob's removed liquidity: 50
  

Alice burns 210 of token A --> removed liquidity underflows.

  Alice balance token A: 800
  Alice balance token C: 1010
  Added liquidity: 10
  Removed liquidity: 340282366920938463463374607431768211446

Tools Used

Code inspection.

Recommended Mitigation Steps

Block transfers of non-existent tokens (i.e. block transfers with zero amounts).

Assessed type

Token-Transfer

1. Simplify ifs in TokenId.sol validate() (~70 gas per leg)

The code here https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/types/TokenId.sol#L507-L518 is equivalent to

if ((isLong ^ isLongP) == (tokenType ^ tokenTypeP))

2. Avoid redundant riskPartner checks in TokenId.sol validate()

The code here https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/types/TokenId.sol#L490-L520 is executed twice per pair of riskPartnerIndexes, but only once is needed. Replacing L491 with if (riskPartnerIndex > 1) removes this inefficiency.

3. Optimize TokenId strike checks in validate()

Instead of parsing token id's strike parameter twice https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/types/TokenId.sol#L482-L485, use a local variable to store the parsed strike variable.

4. Remove unnecessary checks

Checking the positionSize https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L670-L671 doesn't seem needed, because Uniswap V3 already reverts on minting 0 and nothing out of the ordinary seems to happen when burning 0.

5. Remove address(0) check

Checking that the univ3pool is not null is redundant, because calling a null address as it were a Uniswap pool will revert anyways. https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L683

6. Don't use liquidity chunks inside PanopticMath and Math to reduce the back and forth conversion of tickLower,tickUpper,liquidity <--> liquidityChunk

Example 1: https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L586-L621

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/libraries/PanopticMath.sol#L133 PanopticMath.getLiquidityChunk() returns a liquidity chunk but it would be better if it returned the 3 parameters individually. 7 shifts and several casting can be avoided.

Example 2: https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/libraries/Math.sol#L101-L128

3 shifts and casts can be saved by avoiding liquidityChunks in Math.getAmount1ForLiquidity(). Same for Math.getAmount0ForLiquidity(). Here https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L882-L902 it might be reasonable to pass liquidityChunk to _createLegInAMM(), but createChunk should be invoked outside PanopticMath.getLiquidityChunk() only for that purpose.