M-02 from past audit not completely fixed. Users can still bypass solvency checks when settling long premium

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/PanopticPool.sol#L1552-L1624

Vulnerability details

Impact

A full breakdown of issue M-02 from the previous audit can be found here, but the vulnerability involves because check for duplicate token ids is not implemented, causing that users can settle long premiums for other users when they're insolvent. The affected functions are functions in which the _validateSolvency function is used including settleLongPremium, forceExercise, burnOptions, liquidate and mintOptions. The vulnerability was however mitigated in the liquidate, forceExercise, burnOptions, and mintOptions causing that it still exists in the settleLongPremium function. So solvency checks can be bypassed by users when settling long premium.

Proof of Concept

The mitigation involves the check for hash not being more than MAX_POSITIONS, which can be found in the _updatePositionsHash function.

    if ((newHash >> 248) > MAX_POSITIONS) revert Errors.TooManyPositionsOpen();

The _updatePositionsHash is used in two places, the _addUserOption which handles hash in the _mintOptions function and in the _updatePositionDataBurn function which is used when burning options. Options are burned upon when force exercising and when liquidating, so that handles the validation.
The hash validation is however not done when settling long premium as can be seen by going through the function causing that the issue still exists and not fully mitigated.

 function settleLongPremium(
        TokenId[] calldata positionIdList,
        address owner,
        uint256 legIndex
    ) external {
        _validatePositionList(owner, positionIdList, 0);

        TokenId tokenId = positionIdList[positionIdList.length - 1];

        if (tokenId.isLong(legIndex) == 0 || legIndex > 3) revert Errors.NotALongLeg();

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

        LeftRightUnsigned accumulatedPremium;
        {
            (int24 tickLower, int24 tickUpper) = tokenId.asTicks(legIndex);

            uint256 tokenType = tokenId.tokenType(legIndex);
            (uint128 premiumAccumulator0, uint128 premiumAccumulator1) = SFPM.getAccountPremium(
                address(s_univ3pool),
                address(this),
                tokenType,
                tickLower,
                tickUpper,
                currentTick,
                1
            );
            accumulatedPremium = LeftRightUnsigned
                .wrap(0)
                .toRightSlot(premiumAccumulator0)
                .toLeftSlot(premiumAccumulator1);

            // update the premium accumulator for the long position to the latest value
            // (the entire premia delta will be settled)
            LeftRightUnsigned premiumAccumulatorsLast = s_options[owner][tokenId][legIndex];
            s_options[owner][tokenId][legIndex] = accumulatedPremium;

            accumulatedPremium = accumulatedPremium.sub(premiumAccumulatorsLast);
        }

        uint256 liquidity = PanopticMath
            .getLiquidityChunk(tokenId, legIndex, s_positionBalance[owner][tokenId].rightSlot())
            .liquidity();

        unchecked {
            // update the realized premia
            LeftRightSigned realizedPremia = LeftRightSigned
                .wrap(0)
                .toRightSlot(int128(int256((accumulatedPremium.rightSlot() * liquidity) / 2 ** 64)))
                .toLeftSlot(int128(int256((accumulatedPremium.leftSlot() * liquidity) / 2 ** 64)));

            // deduct the paid premium tokens from the owner's balance and add them to the cumulative settled token delta
            s_collateralToken0.exercise(owner, 0, 0, 0, -realizedPremia.rightSlot());
            s_collateralToken1.exercise(owner, 0, 0, 0, -realizedPremia.leftSlot());

            bytes32 chunkKey = keccak256(
                abi.encodePacked(
                    tokenId.strike(legIndex),
                    tokenId.width(legIndex),
                    tokenId.tokenType(legIndex)
                )
            );
            // commit the delta in settled tokens (all of the premium paid by long chunks in the tokenIds list) to storage
            s_settledTokens[chunkKey] = s_settledTokens[chunkKey].add(
                LeftRightUnsigned.wrap(uint256(LeftRightSigned.unwrap(realizedPremia)))
            );

            emit PremiumSettled(owner, tokenId, realizedPremia);
        }

        // ensure the owner is solvent (insolvent accounts are not permitted to pay premium unless they are being liquidated)
        _validateSolvency(owner, positionIdList, NO_BUFFER);
    }

Tools Used

Manual code review

Recommended Mitigation Steps

Consider introducing the check in the settleLongPremium function.

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #19

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b

[sammy-tm]

@Picodes

How is this valid? Isn't this handled in the function

[sammy-tm]

@ZanyBonzy Your argument is invalid as to pass _validatePositionList in settleLongPremium the token list must have been minted at some point and gone through the "mint options" flow.

Even apart from this, I think you have misinterpreted the issue here. The issue existed because of the number of positions overflowing 256, which can cause forgery, and not it being above MAX_POSITIONS. The overflow has already been fixed in the updated PanopticMath.updatePositionHash() function.

[c4-judge]

Picodes marked the issue as grade-c