c4-bot-8 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L1231-L1266 https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L650-L734

Vulnerability details

When liquidating, current tick is checked against 10 minutes TWAP:

        int24 twapTick = getUniV3TWAP();
// [...]
        {
            (, int24 currentTick, , , , , ) = s_univ3pool.slot0();

            // Enforce maximum delta between TWAP and currentTick to prevent extreme price manipulation
            if (Math.abs(currentTick - twapTick) > MAX_TWAP_DELTA_LIQUIDATION)
                revert Errors.StaleTWAP();

This is not done for forceExercise. While both have different tasks, force exercise should pay fees, the farther-the-money, the cheaper it should be to force exercise an option position. Additionally, there's a concept of compensating user for loss in value if chunk has lost money between current and median tick, which can be exploited into user whose position is exercised will pay for the exercise.

Impact

Proof of Concept

When position is exercised, current spot price tick is taken together with 10 minutes TWAP:

    function forceExercise(
// [...]
        int24 twapTick = getUniV3TWAP();

        (, int24 currentTick, , , , , ) = s_univ3pool.slot0();
// [...]
        // on forced exercise, the price *must* be outside the position's range for at least 1 leg
        touchedId[0].validateIsExercisable(twapTick);

In order to proceed, token is validated if it's exercisable, i.e. if at least 1 of the long legs is over tickSpacing apart from strike price:

    function validateIsExercisable(TokenId self, int24 currentTick) internal pure {
        //[...]
                if ((currentTick >= _strike + rangeUp) || (currentTick < _strike - rangeDown)) {
                    // if this leg is long and the price beyond the leg's range:
                    // this exercised ID, `self`, appears valid
                    if (self.isLong(i) == 1) return; // validated
                }
            }
        }
        // Fail if position has no legs that is far-out-of-the-money
        revert Errors.NoLegsExercisable();
    }

Following, user whose account is exercised is temporarily delegated virtual shares, to make sure that their position can be settled during burning the position from the user. They will have those shares refunded later:

        // The protocol delegates some virtual shares to ensure the burn can be settled.
        s_collateralToken0.delegate(account, uint128(delegatedAmounts.rightSlot()));
        s_collateralToken1.delegate(account, uint128(delegatedAmounts.leftSlot()));

        // Exercise the option
        // Note: tick limits are not applied here since it is not the exercisor's position being closed
        _burnAllOptionsFrom(account, 0, 0, COMMIT_LONG_SETTLED, touchedId);

Next, what's important here is that position exercise cost is calculated. The problem is that the cost is based on slot0 spot price. This allows to take a flashloan, destabilize position to be highly out of the money and eercise anyone's position for very small amount, because exercise is cheaper, the more OTM the position is:

        // Compute the exerciseFee, this will decrease the further away the price is from the forcedExercised position
        /// @dev use the medianTick to prevent price manipulations based on swaps.
        LeftRightSigned exerciseFees = s_collateralToken0.exerciseCost(
            currentTick,
            twapTick,
            touchedId[0],
            positionBalance,
            longAmounts
        );

However, fees to pay for force exercise in CollateralTracker.exerciseCost() are based on currentTick, which exercisor can manipulate however they wish:

                    maxNumRangesFromStrike = Math.max(
                        maxNumRangesFromStrike,
                        uint256(Math.abs(currentTick - positionId.strike(leg)) / range) // @audit using current tick allows to diminish the exercise cost fees
                    );
// [...]
            int256 fee = (FORCE_EXERCISE_COST >> (maxNumRangesFromStrike - 1)); // exponential decay of fee based on number of half ranges away from the price

            // store the exercise fees in the exerciseFees variable
            exerciseFees = exerciseFees
                .toRightSlot(int128((longAmounts.rightSlot() * fee) / DECIMALS_128))
                .toLeftSlot(int128((longAmounts.leftSlot() * fee) / DECIMALS_128));

However, here the bigger issue pertraits itself:

                    (currentValue0, currentValue1) = Math.getAmountsForLiquidity(
                        currentTick,
                        liquidityChunk
                    );

                    (oracleValue0, oracleValue1) = Math.getAmountsForLiquidity(
                        oracleTick,
                        liquidityChunk
                    );
                }

                uint256 tokenType = positionId.tokenType(leg);
                // compensate user for loss in value if chunk has lost money between current and median tick
                // note: the delta for one token will be positive and the other will be negative. This cancels out any moves in their positions
                if (
                    (tokenType == 0 && currentValue1 < oracleValue1) ||
                    (tokenType == 1 && currentValue0 < oracleValue0)
                )
                    exerciseFees = exerciseFees.sub(
                        LeftRightSigned
                            .wrap(0)
                            .toRightSlot(
                                int128(uint128(oracleValue0)) - int128(uint128(currentValue0))
                            )
                            .toLeftSlot(
                                int128(uint128(oracleValue1)) - int128(uint128(currentValue1))
                            )
                    );

So, if the spot price is manipulated, it's guaranteed that currentValue will be smaller than oracleValue, hence exerciseFees will be substracted by the different between the two.

Finally, forceExercise fees are added to the previously substracted value:

            int256 fee = (FORCE_EXERCISE_COST >> (maxNumRangesFromStrike - 1)); // exponential decay of fee based on number of half ranges away from the price

            // store the exercise fees in the exerciseFees variable
            exerciseFees = exerciseFees
                .toRightSlot(int128((longAmounts.rightSlot() * fee) / DECIMALS_128))
                .toLeftSlot(int128((longAmounts.leftSlot() * fee) / DECIMALS_128));

So now, the fees can be negative, meaning that ideally the exercisor will be paid to exercise an option, which we'll see next. After this, the protocol calculates how much refund should flow from the exercisee (user whose position is exercised) to the exercisor.

        LeftRightSigned refundAmounts = delegatedAmounts.add(exerciseFees);

        // redistribute token composition of refund amounts if user doesn't have enough of one token to pay
        refundAmounts = PanopticMath.getRefundAmounts(
            account,
            refundAmounts,
            twapTick,
            s_collateralToken0,
            s_collateralToken1
        );

What's important to note is that delegatedAmounts.add(exerciseFees) - basically substracts one side if exercise fees, because it was negative. Next, PanopticMath.getRefundAmounts is called. It's not important to understand this issue - it checks what token0/token1 ratio should be refunded, in case that there is not enough to pay from just one token.

Finally, the amound are refunded - exercisor gets difference between refundAmounts and delegatedAmounts, and exercisee receives delegatedAmounts:

            s_collateralToken0.refund(
                account,
                msg.sender,
                refundAmounts.rightSlot() - delegatedAmounts.rightSlot()
            );
            s_collateralToken1.refund(
                account,
                msg.sender,
                refundAmounts.leftSlot() - delegatedAmounts.leftSlot()
            );
        }

        // refund the protocol any virtual shares after settling the difference with the exercisor
        s_collateralToken0.refund(account, uint128(delegatedAmounts.rightSlot()));
        s_collateralToken1.refund(account, uint128(delegatedAmounts.leftSlot()));

Last piece is the CollateralTracker.refund() function, which accepts int and based on the sign, can invert transfer direction:

    function refund(address refunder, address refundee, int256 assets) external onlyPanopticPool {
        if (assets > 0) {
            _transferFrom(refunder, refundee, convertToShares(uint256(assets)));
        } else {
            unchecked {
                _transferFrom(refundee, refunder, convertToShares(uint256(-assets)));
            }
        }
    }

Now, putting it all together, we add exerciseFees to refundAmounts: refundAmounts = delegatedAmounts.add(exerciseFees), and then use it to repay exercisor:

            s_collateralToken0.refund(
                account,
                msg.sender,
                refundAmounts.rightSlot() - delegatedAmounts.rightSlot()
            );

The assumption is that the refundAmounts - delegatedAmounts will be negative, so that the exercisor will pay for the exercise. However, because exerciseFees are negative, it is the exercisee that has to pay for the exercose. So, not only they have their position closed, even though it could be profitable (only 1 long leg has to be significantly Out-Of-Money), they also loose their collateral for the exercisor.

To summarize, the exercisor is paid for exercising an option, if they are able to flashloan some liquidity. And because positions OTM will be common, because will be used in many strategies, malicious actor will have both incentive and a lot of positions to steal from.

Tools Used

Manual Review

Recommended Mitigation Steps

Introduce TWAP check as with liquidation:

    function forceExercise(
        address account,
        TokenId[] calldata touchedId,
        TokenId[] calldata positionIdListExercisee,
        TokenId[] calldata positionIdListExercisor
    ) external {
        // revert if multiple positions are specified
        // the reason why the singular touchedId is an array is so it composes well with the rest of the system
        // '_calculateAccumulatedPremia' expects a list of positions to be touched, and this is the only way to pass a single position
        if (touchedId.length != 1) revert Errors.InputListFail();

        // validate the exercisor's position list (the exercisee's list will be evaluated after their position is force exercised)
        _validatePositionList(msg.sender, positionIdListExercisor, 0);

        uint128 positionBalance = s_positionBalance[account][touchedId[0]].rightSlot();

        // compute the notional value of the short legs (the maximum amount of tokens required to exercise - premia)
        // and the long legs (from which the exercise cost is computed)
        (LeftRightSigned longAmounts, LeftRightSigned delegatedAmounts) = PanopticMath
            .computeExercisedAmounts(touchedId[0], positionBalance);

        int24 twapTick = getUniV3TWAP();

        (, int24 currentTick, , , , , ) = s_univ3pool.slot0();

+            if (Math.abs(currentTick - twapTick) > MAX_TWAP_DELTA_LIQUIDATION)
+                revert Errors.StaleTWAP();

Assessed type

Error

c4-judge commented 4 months ago

Picodes marked the issue as primary issue

dyedm1 commented 4 months ago

RefundAmounts is smaller than delegatedAmounts (in value measured across both tokens) because we subtract the fee from it. This means that an net negative amount of tokens is passed into the respective refund calls -- transferring net value tokens from the force exercisor to the force exercisee.

Picodes commented 4 months ago

Reading the report I think the sponsor is right and values correctly flows from the force exercisor to the exercisee, so I'll invalidate.