`intrinsicValue` during minting OTM short options is not strictly 0 due to rounding issues

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/CollateralTracker.sol#L1105 https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/libraries/PanopticMath.sol#L390-L410 https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/libraries/PanopticMath.sol#L574-L599

Vulnerability details

Impact

An extra 1 wei amount of token is taken from user each time minting an OTM short option.

Bug Description

intrinsicValue should be 0 when minting OTM short options, however, due to rounding issues, it may be 1 wei if assetType != tokenType.

    function _getExchangedAmount(
        int128 longAmount,
        int128 shortAmount,
        int128 swappedAmount
    ) internal view returns (int256 exchangedAmount) {
        // If amount swapped is positive, the amount of tokens to pay is the ITM amount

        unchecked {
            // intrinsic value is the amount that need to be exchanged due to minting in-the-money
>           int256 intrinsicValue = swappedAmount - (shortAmount - longAmount);

            if (intrinsicValue != 0) {
                // the swap commission is paid on the intrinsic value, and it is always positive
                uint256 swapCommission = Math.unsafeDivRoundingUp(
                    s_ITMSpreadFee * uint256(Math.abs(intrinsicValue)),
                    DECIMALS
                );

                // set the exchanged amount to the sum of the intrinsic value and swapCommission
                exchangedAmount = intrinsicValue + int256(swapCommission);
            }

            //compute total commission amount = commission rate + spread fee
            exchangedAmount += int256(
                Math.unsafeDivRoundingUp(
                    uint256(uint128(shortAmount + longAmount)) * COMMISSION_FEE,
                    DECIMALS
                )
            );
        }
    }

The issue here is that swappedAmount (actual amount that is moved to uniswap) may be larger than shortAmount, which is calculated by PanopticMath.computeExercisedAmounts().

        (LeftRightSigned longAmounts, LeftRightSigned shortAmounts) = PanopticMath
            .computeExercisedAmounts(tokenId, positionSize);

        int256 utilization0 = s_collateralToken0.takeCommissionAddData(
            msg.sender,
            longAmounts.rightSlot(),
            shortAmounts.rightSlot(),
            totalSwapped.rightSlot()
        );

Let's first see how swappedAmount is calculated in (SqrtPriceMath.sol)[https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/SqrtPriceMath.sol]. We can see if liquidity is positive (minting), it uses roundUp == true.

    function getAmount0Delta(
        uint160 sqrtRatioAX96,
        uint160 sqrtRatioBX96,
        int128 liquidity
    ) internal pure returns (int256 amount0) {
        return
            liquidity < 0
                ? -getAmount0Delta(sqrtRatioAX96, sqrtRatioBX96, uint128(-liquidity), false).toInt256()
>               : getAmount0Delta(sqrtRatioAX96, sqrtRatioBX96, uint128(liquidity), true).toInt256();
    }

    function getAmount0Delta(
        uint160 sqrtRatioAX96,
        uint160 sqrtRatioBX96,
        uint128 liquidity,
        bool roundUp
    ) internal pure returns (uint256 amount0) {
        unchecked {
            if (sqrtRatioAX96 > sqrtRatioBX96) (sqrtRatioAX96, sqrtRatioBX96) = (sqrtRatioBX96, sqrtRatioAX96);

            uint256 numerator1 = uint256(liquidity) << FixedPoint96.RESOLUTION;
            uint256 numerator2 = sqrtRatioBX96 - sqrtRatioAX96;

            require(sqrtRatioAX96 > 0);

            return
                roundUp
                    ? UnsafeMath.divRoundingUp(
                        FullMath.mulDivRoundingUp(numerator1, numerator2, sqrtRatioBX96),
                        sqrtRatioAX96
                    )
                    : FullMath.mulDiv(numerator1, numerator2, sqrtRatioBX96) / sqrtRatioAX96;
        }
    }

Then, for PanopticMath.computeExercisedAmounts(), the call stack leads to PanopticMath.getAmountsMoved(), which by default uses round down for liquidity calculation.

    function getAmountsMoved(
        TokenId tokenId,
        uint128 positionSize,
        uint256 legIndex
    ) internal pure returns (LeftRightUnsigned) {
        // get the tick range for this leg in order to get the strike price (the underlying price)
        (int24 tickLower, int24 tickUpper) = tokenId.asTicks(legIndex);

        uint128 amount0;
        uint128 amount1;
        if (tokenId.asset(legIndex) == 0) {
            amount0 = positionSize * uint128(tokenId.optionRatio(legIndex));

            amount1 = Math
                .getAmount1ForLiquidity(Math.getLiquidityForAmount0(tickLower, tickUpper, amount0))
                .toUint128();
        } else {
            amount1 = positionSize * uint128(tokenId.optionRatio(legIndex));

            amount0 = Math
                .getAmount0ForLiquidity(Math.getLiquidityForAmount1(tickLower, tickUpper, amount1))
                .toUint128();
        }

        return LeftRightUnsigned.wrap(0).toRightSlot(amount0).toLeftSlot(amount1);
    }

This rounding difference can cause non-zero intrinsic value even when minting short options OTM, which is unexpected.

Proof of Concept

Run test_Fail_mintOptions_OTMShortCall_NotEnoughCollateral unit test.

Add the following console.log in CollateralTracker.sol, and every time it will log out the swappedAmount is 1 wei larger than shortAmount.

     function _getExchangedAmount(
         int128 longAmount,
         int128 shortAmount,
         int128 swappedAmount
     ) internal view returns (int256 exchangedAmount) {
         // If amount swapped is positive, the amount of tokens to pay is the ITM amount

         unchecked {
             // intrinsic value is the amount that need to be exchanged due to minting in-the-money
             int256 intrinsicValue = swappedAmount - (shortAmount - longAmount);

+            console.log("CollaterTracker._getExchangedAmount():");
+            console.logInt(swappedAmount);
+            console.logInt(shortAmount);
+            console.logInt(longAmount);
+
             if (intrinsicValue != 0) {
                 // the swap commission is paid on the intrinsic value, and it is always positive
                 uint256 swapCommission = Math.unsafeDivRoundingUp(
                     s_ITMSpreadFee * uint256(Math.abs(intrinsicValue)),
                     DECIMALS
                 );

                 // set the exchanged amount to the sum of the intrinsic value and swapCommission
                 exchangedAmount = intrinsicValue + int256(swapCommission);
             }

An example output is:

CollaterTracker._getExchangedAmount():
86780095036757312244
86780095036757312243
0

Tools Used

Foundry

Recommended Mitigation Steps

Handle roundup issues when calculating PanopticMath.computeExercisedAmounts(), just like what uniswap did.

Assessed type

Uniswap

[dyedm1]

True, but don't see a failure case here. The protocol is designed to handle the swaps/token deltas occuring partially or not at all, and makes the user compensate for any discrepancies in the token amounts. That 1 wei taken from the user is important, because it means that they borrowed 1 wei less than what was required to mint their option.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Picodes]

Invalidating in the absence of impact