code-423n4 / 2024-04-panoptic-findings

8 stars 3 forks source link

Potential inconsistencies and loss of funds due to the lack of proper error handling and return value checks for the `_validateAndForwardToAMM` function calls. #443

Closed c4-bot-6 closed 4 months ago

c4-bot-6 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L504-L527 https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L471-L495 https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L520-L526 https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L488-L494

Vulnerability details

Impact

mintTokenizedPosition function is responsible for creating a new position with up to 4 legs. Similarly, burnTokenizedPosition function is responsible for burning a position. It follows a similar structure to mintTokenizedPosition but burns the ERC1155 token instead of minting it.

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L504-L527

    function mintTokenizedPosition(
        TokenId tokenId,
        uint128 positionSize,
        int24 slippageTickLimitLow,
        int24 slippageTickLimitHigh
    )
        external
        ReentrancyLock(tokenId.poolId())
        returns (LeftRightUnsigned[4] memory collectedByLeg, LeftRightSigned totalSwapped)
    {
        // create the option position via its ID in this erc1155
        _mint(msg.sender, TokenId.unwrap(tokenId), positionSize);

        emit TokenizedPositionMinted(msg.sender, tokenId, positionSize);

        // validate the incoming option position, then forward to the AMM for minting/burning required liquidity chunks
        (collectedByLeg, totalSwapped) = _validateAndForwardToAMM(
            tokenId,
            positionSize,
            slippageTickLimitLow,
            slippageTickLimitHigh,
            MINT
        );
    }

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L471-L495

    function burnTokenizedPosition(
        TokenId tokenId,
        uint128 positionSize,
        int24 slippageTickLimitLow,
        int24 slippageTickLimitHigh
    )
        external
        ReentrancyLock(tokenId.poolId())
        returns (LeftRightUnsigned[4] memory collectedByLeg, LeftRightSigned totalSwapped)
    {
        // burn this ERC1155 token id
        _burn(msg.sender, TokenId.unwrap(tokenId), positionSize);

        // emit event
        emit TokenizedPositionBurnt(msg.sender, tokenId, positionSize);

        // Call a function that contains other functions to mint/burn position, collect amounts, swap if necessary
        (collectedByLeg, totalSwapped) = _validateAndForwardToAMM(
            tokenId,
            positionSize,
            slippageTickLimitLow,
            slippageTickLimitHigh,
            BURN
        );
    }

Lack of error handling and return value checks for the _validateAndForwardToAMM function calls in both mintTokenizedPosition and burnTokenizedPosition.

If the _validateAndForwardToAMM function fails or returns unexpected values, the calling functions do not handle the failure cases properly. They simply assign the returned values to collectedByLeg and totalSwapped without checking their validity or success status.

mintTokenizedPosition#L520-L525

        (collectedByLeg, totalSwapped) = _validateAndForwardToAMM(
            tokenId,
            positionSize,
            slippageTickLimitLow,
            slippageTickLimitHigh,
            MINT
        );

burnTokenizedPosition#L488-L493

        (collectedByLeg, totalSwapped) = _validateAndForwardToAMM(
            tokenId,
            positionSize,
            slippageTickLimitLow,
            slippageTickLimitHigh,
            BURN
        );

If the _validateAndForwardToAMM function fails or encounters an error, it may revert or return unexpected values. The calling functions (mintTokenizedPosition and burnTokenizedPosition) do not handle the failure cases and blindly assign the returned values to collectedByLeg and totalSwapped.

Tools Used

Vs

Recommended Mitigation Steps

Check the return values of the _validateAndForwardToAMM function for success or failure. Use require statements or if-else conditions to handle different return scenarios.

(bool success, LeftRightUnsigned[4] memory collectedByLeg, LeftRightSigned totalSwapped) = _validateAndForwardToAMM(
    tokenId,
    positionSize,
    slippageTickLimitLow,
    slippageTickLimitHigh,
    MINT
);

require(success, "ValidateAndForwardToAMM failed");

Assessed type

Error

c4-judge commented 4 months ago

Picodes marked the issue as unsatisfactory: Invalid