_validatePositionList() does not check for duplicate tokenIds, allowing attackers to bypass solvency checks

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/PanopticPool.sol#L1367-L1391
https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/PanopticPool.sol#L887-L893

Vulnerability details

Impact

The underlying issue is that _validatePositionList() does not check for duplicate tokenIds. Attackers can use this issue to bypass solvency checks, which leads to several impacts:

1. Users can mint/burn/liquidate/forceExercise when they are insolvent.
2. Users can settleLongPremium/forceExercise another user when the other user is insolvent.

This also conflicts a main invariant stated in contest readme: Users should not be allowed to mint/burn options or pay premium if their end state is insolvent.

Bug Description

First, let's see why _validatePositionList does not check for duplicate tokenIds. For a user position hash, the first 8 bits is the length of tokenIds which overflows, last 248 bits is the xor hash. However, we can easily add 256 tokenIds of the same kind to create a duplicate positionHash.

For example: Hash(key0, key1, key2) == Hash(key0, key1, key2, key0, key0, ..., 256 more key0). This way, we can add any tokenId we want while still arriving the same position hash.

PanopticPool.sol

    function _validatePositionList(
        address account,
        TokenId[] calldata positionIdList,
        uint256 offset
    ) internal view {
        uint256 pLength;
        uint256 currentHash = s_positionsHash[account];

        unchecked {
            pLength = positionIdList.length - offset;
        }
        // note that if pLength == 0 even if a user has existing position(s) the below will fail b/c the fingerprints will mismatch
        // Check that position hash (the fingerprint of option positions) matches the one stored for the '_account'
        uint256 fingerprintIncomingList;

        for (uint256 i = 0; i < pLength; ) {
>           fingerprintIncomingList = PanopticMath.updatePositionsHash(
                fingerprintIncomingList,
                positionIdList[i],
                ADD
            );
            unchecked {
                ++i;
            }
        }

        // revert if fingerprint for provided '_positionIdList' does not match the one stored for the '_account'
        if (fingerprintIncomingList != currentHash) revert Errors.InputListFail();
    }

PanopticMath.sol

    function updatePositionsHash(
        uint256 existingHash,
        TokenId tokenId,
        bool addFlag
    ) internal pure returns (uint256) {
        // add the XOR`ed hash of the single option position `tokenId` to the `existingHash`
        // @dev 0 ^ x = x

        unchecked {
            // update hash by taking the XOR of the new tokenId
            uint248 updatedHash = uint248(existingHash) ^
                (uint248(uint256(keccak256(abi.encode(tokenId)))));
            // increment the top 8 bit if addflag=true, decrement otherwise
            return
                addFlag
                    ? uint256(updatedHash) + (((existingHash >> 248) + 1) << 248)
                    : uint256(updatedHash) + (((existingHash >> 248) - 1) << 248);
        }
    }

Then, let's see how duplicate ids can bypass solvency check. The solvency check is in _validateSolvency(), which is called by all the user interaction functions, such as mint/burn/liquidate/forceExercise/settleLongPremium. This function first checks for a user passed in positionIdList (which we already proved can include duplicates), then calls _checkSolvencyAtTick() to calculate the balanceCross (collateral balance) and thresholdCross (required collateral) for all tokens.

The key is the collateral balance includes the premium that is collected for each of the positions. For most of the positions, the collected premium should be less than required collateral to keep this position open. However, if a position has been open for a long enough time, the fees it accumulated may be larger than required collateral.

For this kind of position, we can duplicate it 256 times (or multiple of 256 times, as long as gas fee is enough), and make our collateral balance grow faster than required collateral. This can make a insolvent account "solvent", by duplicating the key tokenId multiple times.

    function _validateSolvency(
        address user,
        TokenId[] calldata positionIdList,
        uint256 buffer
    ) internal view returns (uint256 medianData) {
        // check that the provided positionIdList matches the positions in memory
>       _validatePositionList(user, positionIdList, 0);
        ...
        // Check the user's solvency at the fast tick; revert if not solvent
        bool solventAtFast = _checkSolvencyAtTick(
            user,
            positionIdList,
            currentTick,
            fastOracleTick,
            buffer
        );
        if (!solventAtFast) revert Errors.NotEnoughCollateral();

        // If one of the ticks is too stale, we fall back to the more conservative tick, i.e, the user must be solvent at both the fast and slow oracle ticks.
        if (Math.abs(int256(fastOracleTick) - slowOracleTick) > MAX_SLOW_FAST_DELTA)
            if (!_checkSolvencyAtTick(user, positionIdList, currentTick, slowOracleTick, buffer))
                revert Errors.NotEnoughCollateral();
    }

    function _checkSolvencyAtTick(
        address account,
        TokenId[] calldata positionIdList,
        int24 currentTick,
        int24 atTick,
        uint256 buffer
    ) internal view returns (bool) {
        (
            LeftRightSigned portfolioPremium,
            uint256[2][] memory positionBalanceArray
        ) = _calculateAccumulatedPremia(
                account,
                positionIdList,
                COMPUTE_ALL_PREMIA,
                ONLY_AVAILABLE_PREMIUM,
                currentTick
            );

        LeftRightUnsigned tokenData0 = s_collateralToken0.getAccountMarginDetails(
            account,
            atTick,
            positionBalanceArray,
            portfolioPremium.rightSlot()
        );
        LeftRightUnsigned tokenData1 = s_collateralToken1.getAccountMarginDetails(
            account,
            atTick,
            positionBalanceArray,
            portfolioPremium.leftSlot()
        );

        (uint256 balanceCross, uint256 thresholdCross) = _getSolvencyBalances(
            tokenData0,
            tokenData1,
            Math.getSqrtRatioAtTick(atTick)
        );

        // compare balance and required tokens, can use unsafe div because denominator is always nonzero
        unchecked {
            return balanceCross >= Math.unsafeDivRoundingUp(thresholdCross * buffer, 10_000);
        }
    }

CollateralTracker.sol

    function getAccountMarginDetails(
        address user,
        int24 currentTick,
        uint256[2][] memory positionBalanceArray,
        int128 premiumAllPositions
    ) public view returns (LeftRightUnsigned tokenData) {
        tokenData = _getAccountMargin(user, currentTick, positionBalanceArray, premiumAllPositions);
    }

    function _getAccountMargin(
        address user,
        int24 atTick,
        uint256[2][] memory positionBalanceArray,
        int128 premiumAllPositions
    ) internal view returns (LeftRightUnsigned tokenData) {
        uint256 tokenRequired;

        // if the account has active options, compute the required collateral to keep account in good health
        if (positionBalanceArray.length > 0) {
            // get all collateral required for the incoming list of positions
            tokenRequired = _getTotalRequiredCollateral(atTick, positionBalanceArray);

            // If premium is negative (ie. user has to pay for their purchased options), add this long premium to the token requirement
            if (premiumAllPositions < 0) {
                unchecked {
                    tokenRequired += uint128(-premiumAllPositions);
                }
            }
        }

        // if premium is positive (ie. user will receive funds due to selling options), add this premum to the user's balance
        uint256 netBalance = convertToAssets(balanceOf[user]);
        if (premiumAllPositions > 0) {
            unchecked {
                netBalance += uint256(uint128(premiumAllPositions));
            }
        }

        // store assetBalance and tokens required in tokenData variable
        tokenData = tokenData.toRightSlot(netBalance.toUint128()).toLeftSlot(
            tokenRequired.toUint128()
        );
        return tokenData;
    }

Now we have shown how to bypass the _validateSolvency(), we can bypass all related checks. Listing them here:

1. Burn options #1, #2
2. Mint options #1
3. Force exercise #1, #2
4. SettleLongPremium #1

Proof of Concept

In this report, we will only prove the core issue: duplicate tokenIds are allowed, and won't craft complicated scenarios for relevant impacts.

Add the following test code in PanopticPool.t.sol, we can see that passing 257 of the same token ids can still work for mintOptions().

    function test_duplicatePositionHash(
        uint256 x,
        uint256[2] memory widthSeeds,
        int256[2] memory strikeSeeds,
        uint256[2] memory positionSizeSeeds,
        uint256 swapSizeSeed
    ) public {
        _initPool(x);

        (int24 width, int24 strike) = PositionUtils.getOTMSW(
            widthSeeds[0],
            strikeSeeds[0],
            uint24(tickSpacing),
            currentTick,
            0
        );

        (int24 width2, int24 strike2) = PositionUtils.getOTMSW(
            widthSeeds[1],
            strikeSeeds[1],
            uint24(tickSpacing),
            currentTick,
            0
        );
        vm.assume(width2 != width || strike2 != strike);

        populatePositionData([width, width2], [strike, strike2], positionSizeSeeds);

        // leg 1
        TokenId tokenId = TokenId.wrap(0).addPoolId(poolId).addLeg(
            0, 1, isWETH, 0, 0, 0, strike, width
        );

        TokenId[] memory posIdList = new TokenId[](257);
        for (uint i = 0; i < 257; ++i) {
            posIdList[i] = tokenId;
        }
        pp.mintOptions(posIdList, positionSizes[0], 0, 0, 0);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Add a check in _validatePositionList that the length is shorter than MAX_POSITIONS (32).

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[stalinMacias]

Hello judge, I'd like to dispute the severity of this report based on the following reasons:

1. When passing duplicates to mint a position on an insolvent account, first of all, the position's owner is playing a race against a liquidator (because the account is insolvent). If the position's owner opts to use this flaw to mint a new position, the liquidator will anyways liquidate all the user's positions, seizing the position's owner collateral.

• Assuming the account is still solvent and the position's owner decides to "abuse" the contracts and mint a new position by passing many duplicates of one of his positions that has accrued a considerable amount of premium, assuming that the duplicates are enough to bypass the solvency check, right after the new position is minted, the whole account will be liquidable, so, liquidators will go ahead and liquidate all the user's position, as a result, the position's owner will lose his collateral. Since the assets never leaves the Panoptic ecosystem, the position's owner practically hijacked himself by forcing his account to mint a new position while his account would end up being insolvent.

2. When liquidating a position, it is true that is possible to pass duplicate positions on the positionList of the account to be liquidated, and indeed this will cause the accountMarging to be bigger than what it really is, thus, this check is bypassed, but, when the actual positions of the provided positionList are burnt as part of the liquidation process this is what will happen:

• The first time one of the duplicates positions is burnt, the SFPM will correctly burn the position, but, when the second duplicate position is attempted to be burnt on the SFPM, the tx will revert because the positionSize of that tokenId is now 0, when the first duplicate was burnt, the positionSize was reset to 0, thus, the whole tx will be reverted, even though the marginAccount could have indeed computed bigger than what it really is, the liquidation can not actually be executed because the positions from the positionList can only be burnt once, after the first match of a duplicate position is burnt, the positionSize will be 0, thus, when the second match of the same duplicate position is attempted to be burnt, the positionSize is 0, thus, on the SFPM the tx will be reverted because the positionSize is 0, thus, users are not at risk.

3. When force exercising, only 1 position can be forced exercise at a time, so, it is not like by passing duplicates on the positionList could be possible to charge multiple times the OptionBuyer on the position being exercised.

4. When settling long premium, only the last position of the positionList is the one that will settle premium on, the rest of the positions only matter to validate if the positionList is indeed the user's position and to validate if the user is solvent after settling the long premium. If it is not solvent, the tx reverts. The only harm that this can cause is to force an account that is insolvent to pay the already owed premium to the option seller, but, anyway, if the account is already insolvent, the account anyways would be liquidated, premium would be settled and user's collateral would be seized.

Because of the above points, I'd like to ask for a reconsideration of the severity assessed in this report, since the most impactful consequence is that users could mint 1 position above their limit, but as a result of doing it, their own accounts will fall into liquidation, thus, they will end up losing their collateral and gaining nothing in return.

[pkqs90]

Hi @stalinMacias.

For point 1, allowing to mint/burn a position for a insolvent account contradicts one of the main invariant in contest readme: Users should not be allowed to mint/burn options or pay premium if their end state is insolvent (at the fast oracle tick, or both ticks if the fast and slow oracle ticks are further away than the threshold)

For point 2, the report never stated that an attacker can liquidate a solvent account. I am aware of what you are saying.

For point 3, the point is the user can pass a forged positionIdListExercisee and force exercise the position even though the exercisee is insolvent.

For point 4, allowing a non solvent account to be settleLongPremium-ed contradicts the readme as well settleLongPremium - Force a solvent option buyer to pay any premium owed to sellers. Options sellers will want to call this if a large portion of the total premium owed in their chunk is pending/has not been settled..

[stalinMacias]

Hey @pkqs90

Not because an invariant is broken, a report should automatically be awarded as a high severity, it should be assessed what is the impact of breaking such an invariant that determines the final severity. The fact that an invariant is broken makes the report a valid concern, but that does not warrant a high sev automatically.

• For point 1, a user who is trying to abuse this problem would cause that his account falls into liquidation mode. Since assets never leave the Panoptic ecosystem (they are just moved between pools), the end result of forcing the mint of this extra position would be that the user losses his collateral because of the account getting liquidated and the user not getting anything in return. Because of this the loss falls on the "attacker" side when his collateral is seized, not on the protocol,

• For point 3, the purpose of force exercising a position is that an OptionSeller can forcefully close a Buyer's position so that the OptionSeller can close his position. If the Buyer's account is insolvent, there is no need to even force the exercise of a single position since the whole account can be liquidated. The OptionBuyer won't need to pay the forceExerciseFees and instead will get an extra commission for liquidating the position.

• For point 4, contradicting the readme is breaking an invariant, for this reason, the problem is indeed valid, but as for the severity, similar to point 3, if the account is already insolvent, there are more incentives to liquidate the position, seize more collateral and get an extra percentage than rather just settling the long premium of a single position.

[pkqs90]

Hey @stalinMacias,

For point 1, I think something to consider here is that the liquidation does not happen automatically, as does in a CEX. Since the protocol is onchain and relies on liquidation bots, the liquidation may not happen in some period of time.

The user can use this time period to open another position and increase the leverage, and as the price moves, the position will be healthy again, and the user "saves" himself from liquidation.

To make a comparison in CEX, this is like when a user is supposed to be liquidated, he instead creates more long/short positions to increase leverage, and when the price moves in the correct direction and account is healthy again, he sells his position to avoid being liquidated.

The difference is this cannot actually happen in a CEX because liquidation is done automatically, but can happen here.

[stalinMacias]

For point 1, I think something to consider here is that the liquidation does not happen automatically, as does in a CEX. Since the protocol is onchain and relies on liquidation bots, the liquidation may not happen in some period of time.
The user can use this time period to open another position and increase the leverage, and as the price moves, the position will be healthy again, and the user "saves" himself from liquidation.

You've just listed a couple of external requirements for this problem to actually have an impact.

• Liquidation bots not liquidating the insolvent position during a period of time
• Price moving in the right direction for the position to become solvent before getting liquidated.

This seems to match more the description of a medium severity rather than a high sev.

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

I've made my points and will let the judge take the final say in this.

[Picodes]

@stalinMacias thanks for your points.

I don't think here "liquidation bots not liquidating the insolvent position during a period of time" is an external requirement considering how critical liquidations are to Panoptic. My reasoning was that even if the loss of funds is not "direct", being able to properly control when positions can be opened is a key feature, and the fact that you can "increase your leverage" while being insolvent prevents proper risk control.

However, it's true that this is not strictly speaking a scenario where "assets can be stolen/lost/compromised directly" so I'll downgrade to Med.

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[udsene]

Hi @Picodes you have made the following comment:

I don't think here "liquidation bots not liquidating the insolvent position during a period of time" is an external requirement considering how critical liquidations are to Panoptic.

But as per the Panoptic documentation the Liquidation Bot is expected to monitor all positions opened in a pool across all accounts at regular time intervals ensuring real-time tracking of the collateral status of each position and to trigger an alert of the underwater accounts and not to liquidate the position directly.

The following is stated in the Panoptic documentation :

The bot is programmed to identify accounts that become liquidatable, flagging any account that falls below the required collateral thresholds.

Upon identifying a liquidatable account, the bot triggers an alert. Liquidators can then engage by interacting directly with the smart contracts governing the pool.

Since the liquidation bots are not programmed to liquidate the positions but to alert the liquidators at regular time intervals, of the underwater accounts for liquidations, there can be a time period between account going underwater and being liquidated. Hence this seems to be a probable scenario to occur where the attacker can use this time period to open another position and increase the leverage, and as the price moves, the position will be healthy again, and the user "saves" himself from liquidation as stated by @pkqs90 above.

I would kindly like to know your opinion on the above.

Thank you

[Picodes]

@udsene I am not sure to understand what additional facts you are bringing. As I said above I agree with you that it's not an external requirement as it's meant to happen, but the loss of funds isn't direct

[udsene]

@Picodes sorry it seems I had misunderstood your comment. Thanks for the clarification.