Panoptic - KupiaSec's results

Lines of code

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

Vulnerability details

Impact

When a token is transferred, it checks if the entire balance is being transferred by comparing liquidity chunk and net liquidity.

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

This causes an issue where invalid token is allowed to be transferred and it makes other token can't be burnt forever. For example, Alice creates an option for selling T amount of call at K, also creates another option(not a leg but separate token) for buying T/2 amount of call at K. In this case, R = T/2, N = T - R = T/2, thus make it possible for long call option to be transferred to another address. Once long call is transferred, liquidity information is reset to zero, making it impossible for the short call being burnt or transferred and the short call's position is stuck on the smart contract forever.

Proof of Concept

Here's a test case written in Foundry that shows above example:

function test_Audit_token_transfer(
    uint256 x,
    uint256 widthSeed,
    int256 strikeSeed
) public {
    uint256 positionSizeSeed = 1e19;
    _initPool(x);

    (int24 width, int24 strike) = PositionUtils.getInRangeSW(
        widthSeed,
        strikeSeed,
        uint24(tickSpacing),
        currentTick
    );

    populatePositionData(width, strike, positionSizeSeed);

    // tokenId1 - short call
    uint256 tokenId1 = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH == 0 ? 1 : 0, 0, 0, 0, strike, width);

    // tokenId2 - long call
    uint256 tokenId2 = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH == 0 ? 1 : 0, 1, 0, 0, strike, width);

    int256 totalCollected;
    int256 totalSwapped;
    int24 newTickk;

    changePrank(Alice);

    positionSize = 1e20;

    // Write long call option - Option1
    (totalCollected, totalSwapped, newTickk) = sfpm.mintTokenizedPosition(
        tokenId1,
        positionSize,
        TickMath.MAX_TICK - 1,
        TickMath.MIN_TICK + 1
    );

    // Write short call option - Option2
    (totalCollected, totalSwapped, newTickk) = sfpm.mintTokenizedPosition(
        tokenId2,
        positionSize / 2,
        TickMath.MAX_TICK - 1,
        TickMath.MIN_TICK + 1
    );

    // int24 tmp = width * tickSpacing / 2;
    // uint256 liq = sfpm.getAccountLiquidity(address(pool), Alice, 0, strike - tmp, strike + tmp);
    // assertEq(liq, 0);
    // return ;

    // Option2 can be transferred because `liquidityChunk = T/2 and N = T- R = T/2 as well`
    sfpm.safeTransferFrom(
        Alice,
        Bob,
        tokenId2,
        positionSize / 2,
        ""
    );

    // Option1 fails to burn, forever fails
    vm.expectRevert(Errors.NotEnoughLiquidity.selector);
    sfpm.burnTokenizedPosition(
        tokenId1,
        positionSize,
        TickMath.MAX_TICK - 1,
        TickMath.MIN_TICK + 1
    );
}

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

When a token is transferred, T and R need to be calculated for all legs and compare it with the account liquidity. Another possible mitigation would be tieng position key to tokenId.

Assessed type

Invalid Validation

L-01: Minting positions might revert when user only has one-side token.

For writing options, call writers only require asset(ETH) to deposit and put writers only require cash(USDC) to deposit, they don't have to hold both asset and cash to write an option. However, by the nature of the minting logic, when the positin is in the money, call writers need to hold both asset and cash even though one side is returned after swap.

Recommendation: Implementation could be upgraded by using flashloan, without requiring option writers to hold both asset and cash.

L-02: Invalid check for upper/lower bound strike price

https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/types/TokenId.sol#L482-L485

Recommendation: Use <= and >= for comparison.