When Burning a Tokenized Position validate should be done before flipping the isLong bits in _validateAndForwardToAMM()

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/types/TokenId.sol#L535-L571
https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L673-L702

Vulnerability details

Each leg in a tokenid has a risk partner which is usually its own index but in some cases, it could be another leg (Partner in defined risk position).
In the function validate() if the risk partner of a specific leg is not its own index then some additional checks are done to ensure that they are compatible like

• Ensuring that risk partners are mutual
• Ensures that risk partners have the same asset.
• Ensures that risk partners have the same ratio.
• Plus other checks that depend on the legs isLong value compared to that of its risk partner.

    function validate(TokenId self) internal pure {
        if (self.optionRatio(0) == 0) revert Errors.InvalidTokenIdParameter(1);

// More Code...
                // In the following, we check whether the risk partner of this leg is itself
                // or another leg in this position.
                // Handles case where riskPartner(i) != i ==> leg i has a risk partner that is another leg
                uint256 riskPartnerIndex = self.riskPartner(i);
                if (riskPartnerIndex != i) {
                    // Ensures that risk partners are mutual
                    if (self.riskPartner(riskPartnerIndex) != i)
                        revert Errors.InvalidTokenIdParameter(3);


                    // Ensures that risk partners have 1) the same asset, and 2) the same ratio
                    if (
                        (self.asset(riskPartnerIndex) != self.asset(i)) ||
                        (self.optionRatio(riskPartnerIndex) != self.optionRatio(i))
                    ) revert Errors.InvalidTokenIdParameter(3);


                    // long/short status of associated legs
                    uint256 _isLong = self.isLong(i);
                    uint256 isLongP = self.isLong(riskPartnerIndex);


                    // token type status of associated legs (call/put)
                    uint256 _tokenType = self.tokenType(i);
                    uint256 tokenTypeP = self.tokenType(riskPartnerIndex);


                    // if the position is the same i.e both long calls, short put's etc.
                    // then this is a regular position, not a defined risk position
                    if ((_isLong == isLongP) && (_tokenType == tokenTypeP))
                        revert Errors.InvalidTokenIdParameter(4);


                    // if the two token long-types and the tokenTypes are both different (one is a short call, the other a long put, e.g.), this is a synthetic position
                    // A synthetic long or short is more capital efficient than each leg separated because the long+short premia accumulate proportionally
                    // unlike short stranlges, long strangles also cannot be partnered, because there is no reduction in risk (both legs can earn premia simultaneously)
                    if (((_isLong != isLongP) || _isLong == 1) && (_tokenType != tokenTypeP))
                        revert Errors.InvalidTokenIdParameter(5);
                }
            } // end for loop over legs
        }
    }

In burnTokenizedPosition() the internal function _validateAndForwardToAMM() is called, this function calls Tokenid.flipToBurnToken() which simply flips the isLong bits of all active legs of the tokenid. then validate() is called which validates a position tokenId and its legs.

    /// @param tokenId the option position
    /// @param positionSize the size of the position to create
    /// @param tickLimitLow lower limits on potential slippage
    /// @param tickLimitHigh upper limits on potential slippage
    /// @param isBurn is equal to false for mints and true for burns
    /// @return collectedByLeg An array of LeftRight encoded words containing the amount of token0 and token1 collected as fees for each leg
    /// @return totalMoved the total amount of funds swapped in Uniswap as part of building potential ITM positions
    function _validateAndForwardToAMM(
        TokenId tokenId,
        uint128 positionSize,
        int24 tickLimitLow,
        int24 tickLimitHigh,
        bool isBurn
    ) internal returns (LeftRightUnsigned[4] memory collectedByLeg, LeftRightSigned totalMoved) {
        // Reverts if positionSize is 0 and user did not own the position before minting/burning
        if (positionSize == 0) revert Errors.OptionsBalanceZero();


        /// @dev the flipToBurnToken() function flips the isLong bits
        if (isBurn) {
            tokenId = tokenId.flipToBurnToken();
        }


        // Validate tokenId
        tokenId.validate();


        // Extract univ3pool from the poolId map to Uniswap Pool
        IUniswapV3Pool univ3pool = s_poolContext[tokenId.poolId()].pool;


        // Revert if the pool not been previously initialized
        if (univ3pool == IUniswapV3Pool(address(0))) revert Errors.UniswapPoolNotInitialized();
// More Code...
}

The issue here is that if a leg in the tokenid has its risk partner as another leg (that is, it is not its own risk partner), then flipping the isLong bits may cause one of the checks
in validate() to fail and revert as the isLong bits of its risk partner are not changed as well.

Remember that flipping changes the value of the bit from what it was to an opposite value (from 0 to 1 or from 1 to 0).

For example;
Let's say a leg with a different risk partner has isLong() values that are the same but their tokenType() is different, this would easily pass these checks below from validate()
but after a flip is done to its isLong bits using flipToBurnToken() it will fail and revert in the second check below.

                   // if the position is the same i.e both long calls, short put's etc.
                   // then this is a regular position, not a defined risk position
                   if ((_isLong == isLongP) && (_tokenType == tokenTypeP))
                       revert Errors.InvalidTokenIdParameter(4);


                   // if the two token long-types and the tokenTypes are both different (one is a short call, the other a long put, e.g.), this is a synthetic position
                   // A synthetic long or short is more capital efficient than each leg separated because the long+short premia accumulate proportionally
                   // unlike short stranlges, long strangles also cannot be partnered, because there is no reduction in risk (both legs can earn premia simultaneously)
                   if (((_isLong != isLongP) || _isLong == 1) && (_tokenType != tokenTypeP))
                       revert Errors.InvalidTokenIdParameter(5);

Impact

This will result in a continuous revert of the function leading to an inability to Burn a Tokenized Position.

Tools Used

Manual Review

Recommended Mitigation Steps

This whole issue results from the simple fact the risk partners, if different are not flipped as well, I recommend validating the tokenid before flipping the isLong bits, to ensure any changes caused by flipping will not affect the execution of the function.

    /// @param tokenId the option position
    /// @param positionSize the size of the position to create
    /// @param tickLimitLow lower limits on potential slippage
    /// @param tickLimitHigh upper limits on potential slippage
    /// @param isBurn is equal to false for mints and true for burns
    /// @return collectedByLeg An array of LeftRight encoded words containing the amount of token0 and token1 collected as fees for each leg
    /// @return totalMoved the total amount of funds swapped in Uniswap as part of building potential ITM positions
    function _validateAndForwardToAMM(
        TokenId tokenId,
        uint128 positionSize,
        int24 tickLimitLow,
        int24 tickLimitHigh,
        bool isBurn
    ) internal returns (LeftRightUnsigned[4] memory collectedByLeg, LeftRightSigned totalMoved) {
        // Reverts if positionSize is 0 and user did not own the position before minting/burning
        if (positionSize == 0) revert Errors.OptionsBalanceZero();

    ++  // Validate tokenId
    ++  tokenId.validate();


        /// @dev the flipToBurnToken() function flips the isLong bits
        if (isBurn) {
            tokenId = tokenId.flipToBurnToken();
        }


        // Extract univ3pool from the poolId map to Uniswap Pool
        IUniswapV3Pool univ3pool = s_poolContext[tokenId.poolId()].pool;


        // Revert if the pool not been previously initialized
        if (univ3pool == IUniswapV3Pool(address(0))) revert Errors.UniswapPoolNotInitialized();
// More Code...
}

Assessed type

Other

[c4-judge]

Picodes marked the issue as satisfactory

[Picodes]

Keeping Medium severity under "functionality is broken"

[c4-judge]

Picodes marked the issue as selected for report