Transfer invariant can be broken

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

The protocol has an invariant that if someone wants to transfer SFPM-tokens of a certain ID to someone else, they have to send all tokens of that ID that they possess. The checks for this invariant are insufficient.

Proof of Concept

The registerTokenTransfer function performs the following check:

/// @notice update user position data following a token transfer
/// @dev token transfers are only allowed if you transfer your entire balance of a given tokenId and the recipient has none
function registerTokenTransfer(address from, address to, uint256 id, uint256 amount) internal {
    ...
    for (uint256 leg = 0; leg < numLegs; ) {
        // for this leg index: extract the liquidity chunk: a 256bit word containing the liquidity amount and upper/lower tick
        // @dev see `contracts/types/LiquidityChunk.sol`
        uint256 liquidityChunk = PanopticMath.getLiquidityChunk(
            id,
            leg,
            uint128(amount),
            univ3pool.tickSpacing()
        );

        //construct the positionKey for the from and to addresses
        bytes32 positionKey_from = keccak256(
            abi.encodePacked(
                address(univ3pool),
                from,
                id.tokenType(leg),
                liquidityChunk.tickLower(),
                liquidityChunk.tickUpper()
            )
        );

        ...
        // Revert if not all balance is transferred
        uint256 fromLiq = s_accountLiquidity[positionKey_from];
        if (fromLiq.rightSlot() != liquidityChunk.liquidity()) revert Errors.TransferFailed();

The check to validate that all balance is transferred is insufficient. An example:

1. user adds positionSize=200 (optionRatio=1) for tickRange a-b. s_accountLiq[from] will be 0-200 (0 left, 200 right). He gets minted 200 tokens (say tokenId 1 for simplicity)
2. user removes 100 positionSize by minting a long for same tickRange a-b. s_accountLiq[from] will be 100-100. He gets minted 100 tokens with (with tokenId 2)
3. user can now transfer 100 tokens of tokenId 1 as the validation fromLiq.rightSlot() == liquidityChunk.liquidity() passes, even though he owns 200 tokens with tokenId 1

Tools Used

Manual review

Recommended Mitigation Steps

One approach would be to disallow sending tokens where any of its legs has removed liquidity (where leftSlot() != 0). Though this may be more restrictive than the intended design.

Assessed type

Other

[dyedm1]

dup #256

[c4-sponsor]

dyedm1 (sponsor) confirmed

[c4-judge]

Picodes marked the issue as duplicate of #256

[c4-judge]

Picodes marked the issue as partial-50

[Picodes]

Giving partial credit here because the found impact is different from the main one (messing up premia)

[c4-judge]

Picodes changed the severity to 3 (High Risk)