Users solvency validation are being erroneously executed since they are done on the basis of wrong tick data

[howlbot-integration]

Lines of code

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

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/PanopticPool.sol#L852-L896

    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);

        IUniswapV3Pool _univ3pool = s_univ3pool;
        (
            ,
            int24 currentTick,
            uint16 observationIndex,
            uint16 observationCardinality,
            ,
            ,

        ) = _univ3pool.slot0();
        int24 fastOracleTick = PanopticMath.computeMedianObservedPrice(
            _univ3pool,
            observationIndex,
            observationCardinality,
            FAST_ORACLE_CARDINALITY,
            FAST_ORACLE_PERIOD
        );

        int24 slowOracleTick;
        if (SLOW_ORACLE_UNISWAP_MODE) {
            slowOracleTick = PanopticMath.computeMedianObservedPrice(
                _univ3pool,
                observationIndex,
                observationCardinality,
                SLOW_ORACLE_CARDINALITY,
                SLOW_ORACLE_PERIOD
            );
        } else {
            (slowOracleTick, medianData) = PanopticMath.computeInternalMedian(
                observationIndex,
                observationCardinality,
                MEDIAN_PERIOD,
                s_miniMedian,
                _univ3pool
            );
        }

We can see that to get the ticks, the PanopticMath.computeMedianObservedPrice() is queried.

However the PanopticMath.computeMedianObservedPrice() expects the cardinality to be odd to get the right data, see https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/libraries/PanopticMath.sol#L160-L193

    function computeMedianObservedPrice(
        IUniswapV3Pool univ3pool,
        uint256 observationIndex,
        uint256 observationCardinality,
        uint256 cardinality,
        uint256 period
    ) external view returns (int24) {
        //(snip)
        //@audit
            // get the median of the `ticks` array (assuming `cardinality` is odd)
            return int24(Math.sort(ticks)[cardinality / 2]);
        }
    }

Evidently, this function expects the cardinality to be odd, which has also been clearly documented here https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/libraries/PanopticMath.sol#L157

/// @param cardinality The number of `periods` to in the median price array, should be odd

Where as this was ensured previously, the current scope does not do this, this is because as can be seen by the snippet below the SLOW_ORACLE_CARDINALITY has been changed from 7 in the previous scope, to 8 in the current scope.

    /// @notice Amount of Uniswap observations to include in the "slow" oracle price (in Uniswap mode).
-    uint256 internal constant SLOW_ORACLE_CARDINALITY = 7;
+    uint256 internal constant SLOW_ORACLE_CARDINALITY = 8;

This then makes the attempt to get the ticks via to return the wrong tick data since Math.sort() is being queried on false pretence, (assumption that it's odd whereas it's even).

Note that from Math.sol's implementation of sort() & quicksort(), we can see how the cardinality is expected to be odd from which the cardinality / 2 from int24(Math.sort(ticks)[cardinality / 2]) in computeMedianObservedPrice() would return the right median price.

Impact

Pricing integration are done in the wrong pretence which not only goes against the docs but also means that the wrong tick data is used to validate the solvency of a user via validateSolvency() during SLOW_ORACLE_UNISWAP_MODE , this is because the internal median being calculated via PanopticMath.computeMedianObservedPrice() is also going to be inaccurate, considering the Math.sort() getting called expects an odd cardinality, but instead it's being given an even one.

Recommended Mitigation Steps

If the intention is to increase the cardinality of the slow oracle mode, then consider increasing it to another odd value, say 9 rather than 8, i.e apply these fixes:

    /// @notice Amount of Uniswap observations to include in the "slow" oracle price (in Uniswap mode).
-    uint256 internal constant SLOW_ORACLE_CARDINALITY = 8;
+    uint256 internal constant SLOW_ORACLE_CARDINALITY = 9;

Assessed type

Context

[Picodes]

Looks correct although would be low for "function incorrect as to specs"

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b

[Bauchibred]

Hi @Picodes, thanks for judging. However, I'm a bit confused as to how this issue is being finalized. Shouldn't it be instead classified as 2-med risk? I currently believe there are valid reasons to finalize it as such, considering the impact explained in the report and the supporting arguments below:

• From Code4rena’s docs on severity categorization:
  • If the function of a protocol is being affected, it should be considered medium, i.e:

    2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted…

As explained in the report, right from the title, the current implementation means that users might be wrongly proclaimed as solvent or insolvent whereas the direct opposite is the case due to the validation check being done with the use of wrong tick data. Now shouldn't this bug case be then seen as "the function of the protocol or its availability could be impacted" rather than "function incorrect as to specs"? Since the _validateSolvency() is a helper function that gets queried in most core functions in the pool, including but not limited to, minting/burning options, force exercising, checking whether collateral is withdrawable, etc. Indicating the functionalities of all these other methods that query this to get the solvency state of a user would also be impacted/not function as expected, cause their availability/correct functionality is heavily dependent on the correct solvency data being used, i.e if the user is solvent or not.

Thank you for your time and consideration. Also, do clarify if you think I missed anything.

[Picodes]

@Bauchibred see my comment above, this is clearly an instance of "function not following spec" and not of broken functionality

[liveactionllama]

Per direction from the judge, staff have marked as 3rd place.