Liquidity can become permanently stuck in Uniswap when tokens are transferred.

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L1004-L1006
https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L620-L621
https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L627
https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L614-L617
https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L985-L991

Vulnerability details

Impact

It is possible for users to transfer tokens between each other within SFPM.
But we only check the net liquidity when transferring tokens in SFPM.
This can lead to a permanent lock of liquidity that has already been deposited into Uniswap.

Proof of Concept

Users or protocols, such as Panoptic, will interact with SFPM.
Individual users can mint tokens directly using SFPM or through protocols.
For the purpose, we consider the protocols as normal users.

Let me provide an example.
Consider two tickBounds [L1, U1], [L2, U2].
User A mints a short call for [L1, U1] and deposits X liquidity.
The position key for this is calculated as follows:

positionKey = keccak256(abi.encodePacked(address(univ3pool), userA, 1, L1, U1));

The account liquidity is then updated: s_accountLiquidity[positionKey] = [0, X]
https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L1004-L1006

s_accountLiquidity[positionKey] = uint256(0).toLeftSlot(removedLiquidity).toRightSlot(
    updatedLiquidity
);

User A mints 2 short calls in one token.
One call is for [L1, U1] with liquidity X, and the other is for [L2, U2] with liquidity Y.
The account liquidity is updated accordingly: s_accountLiquidity[positionKey] = [0, 2X]
After some time, User A mints a long call position for [L1, U1] and moves X liquidity from Uniswap.
At this point, the account liquidity becomes s_accountLiquidity[positionKey] = [X, X].
User A decides to transfer token related to the long position to User B.
It is possible because the net liquidity (X) of this position [L1, U1] is the same as the liquidity of the long call.
https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L620-L621

uint256 fromLiq = s_accountLiquidity[positionKey_from];
if (fromLiq.rightSlot() != liquidityChunk.liquidity()) revert Errors.TransferFailed();

But the issue arises because the account liquidity for this position of User A is set to 0.
https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L627

s_accountLiquidity[positionKey_to] = fromLiq;
s_accountLiquidity[positionKey_from] = 0;

Despite transferring all account liquidity to User B, User A still holds the first 2 tokens, and these tokens cannot be transferred to User B because User B's account liquidity for this position [L1, U1] is already not 0.
https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L614-L617

if (
    (s_accountLiquidity[positionKey_to] != 0) ||
    (s_accountFeesBase[positionKey_to] != 0)
) revert Errors.TransferFailed();

User A cannot burn these tokens because they lack net liquidity for this position [L1, U1].
https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L985-L991

if (startingLiquidity < chunkLiquidity) {
    revert Errors.NotEnoughLiquidity();
} 

User B cannot use this liquidity because he does not have the tokens.
The only option for User B is to move X liquidity back to Uniswap again.

As we can see, the transfer results in a potential permanent lock of X liquidity in Uniswap.
And the other X liquidity may be stuck in Uniswap or remain within the protocol.
However, no user can utilize that liquidity because there are no tokens available to redeem it.

The PoC test is as below:

function test_RegisterTokenTransfer() public {
        _initPool(0);

        int24 strike_1 = 0;
        int24 width_1 = 10;
        int24 strike_2 = 0;
        int24 width_2 = 20;

        // Alice deposited X liquidity into the position with parameters [tokenType: 1, tickLower: L1, tickUpper: U1].
        uint256 tokenId_1 = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH, 0, 1, 0, strike_1, width_1);
        sfpm.mintTokenizedPosition(tokenId_1, uint128(1), TickMath.MIN_TICK, TickMath.MAX_TICK);

        // Alice deposited X liquidity into the position [tokenType: 1, tickLower: L1, tickUpper: U1] and Y liquidity into the position [tokenType: 1, tickLower: L2, tickUpper: U2] under a single token ID.
        uint256 tmpTokenId = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH, 0, 1, 0, strike_1, width_1);
        uint256 tokenId_2 = tmpTokenId.addLeg(1, 1, isWETH, 0, 1, 1, strike_2, width_2);
        sfpm.mintTokenizedPosition(tokenId_2, uint128(1), TickMath.MIN_TICK, TickMath.MAX_TICK);

        // Alice removed X liquidity from the position [tokenType: 1, tickLower: L1, tickUpper: U1] by initiating a long position.
        uint256 tokenId_3 = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH, 1, 1, 0, strike_1, width_1);
        sfpm.mintTokenizedPosition(tokenId_3, uint128(1), TickMath.MIN_TICK, TickMath.MAX_TICK);

        // It is possible for Alice to transfer the token associated with the aforementioned long position to Bob.
        sfpm.safeTransferFrom(Alice, Bob, tokenId_3, 1, "");

        // Due to Alice's liquidity being reduced to 0, it is not possible for her to remove liquidity from the first and second positions.
        vm.expectRevert(Errors.NotEnoughLiquidity.selector);
        sfpm.burnTokenizedPosition(tokenId_1, 1, TickMath.MIN_TICK, TickMath.MAX_TICK);

        vm.expectRevert(Errors.NotEnoughLiquidity.selector);
        sfpm.burnTokenizedPosition(tokenId_2, 1, TickMath.MIN_TICK, TickMath.MAX_TICK);

        // Bob can burn the long position and shift liquidity from SFPM to Uni V3, but he cannot withdraw that liquidity from Uni V3 as he lacks the tokens associated with the corresponding short positions.
        changePrank(Bob);
        sfpm.burnTokenizedPosition(tokenId_3, 1, TickMath.MIN_TICK, TickMath.MAX_TICK);

        // And, of course, the tokens related to the short positions cannot be transferred from Alice to Bob.
        changePrank(Alice);
        vm.expectRevert(Errors.TransferFailed.selector);
        sfpm.safeTransferFrom(Alice, Bob, tokenId_1, 1, "");
}

Tools Used

Manual

Recommended Mitigation Steps

Do not proceed with the transfer when there is removed liquidity.

function registerTokenTransfer(address from, address to, uint256 id, uint256 amount) internal {
    - if (fromLiq.rightSlot() != liquidityChunk.liquidity()) revert Errors.TransferFailed();
    + if (fromLiq.rightSlot() != liquidityChunk.liquidity() || fromLiq.leftSlot() != 0) revert Errors.TransferFailed();
}

Assessed type

Invalid Validation

[c4-sponsor]

dyedm1 (sponsor) confirmed

[dyedm1]

dup #256

[c4-judge]

Picodes marked the issue as duplicate of #256

[c4-judge]

Picodes marked the issue as satisfactory