Token balances can be made to not correctly reflect the underlying position

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

Token balances won't reflect underlying position

Proof of Concept

When transferring, the entire liquidity position is transferred to the other user. The checks done for the sender are:

1. the user has enough of the token (if transferring x amount of token, the user is supposed to have x amount)
2. the liquidity amount corresponding to the token amount is equal to the current netLiquidity of the position
   https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L621-L630

    function registerTokenTransfer(address from, address to, uint256 id, uint256 amount) internal {
        
            ........

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


            int256 fromBase = s_accountFeesBase[positionKey_from];


            //update+store liquidity and fee values between accounts
            s_accountLiquidity[positionKey_to] = fromLiq;
            s_accountLiquidity[positionKey_from] = 0;


            s_accountFeesBase[positionKey_to] = fromBase;
            s_accountFeesBase[positionKey_from] = 0;

This makes many paths possible where the token balances of an address doesn't represent the underlying position
Eg:

1. User mints short with posSize = 100 getting liquidity 100
2. User mints long with posSize = 50 making netLiquidity = 50 and removedLiquidity = 50
3. User transfers the short token with amount 50 amount.
   Now the user has 50 short token and 100 long token although the user's liquidity position is 0

It is also not checked whether the token being transferred represents a short even though the netLiquidity is moved

Tools Used

Manual review

Recommended Mitigation Steps

If this is not intended behaviour, seperate the transfer of removedLiquidity and netLiquidity based on long/short nature of the token leg and decrease the feeBase only for short transfers.

Assessed type

Token-Transfer

[c4-judge]

Picodes marked the issue as primary issue

[dyedm1]

another dup #256

[c4-sponsor]

dyedm1 (sponsor) confirmed

[c4-judge]

Picodes marked the issue as duplicate of #256

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as partial-50

[Picodes]

The described impact is not of high severity so giving partial credit