search

code-423n4 / 2024-04-panoptic-findings

2 stars 2 forks source link

Median is not updated when burning a position, which can result in an inaccurate solvency check #540

Open c4-bot-9 opened 2 months ago

c4-bot-9 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L569 https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L944

Vulnerability details

Impact

Median price is updated when minting, but not burning. This value is used to calculate the solvency of a user and it might be very inaccurate with pools that have sparse minting but frequent burning.

Proof of Concept

burnOptions doesn't update the median in any of the inner functions:

function burnOptions(
    TokenId tokenId,
    TokenId[] calldata newPositionIdList,
    int24 tickLimitLow,
    int24 tickLimitHigh
) external {
    _burnOptions(COMMIT_LONG_SETTLED, tokenId, msg.sender, tickLimitLow, tickLimitHigh); //@audit-issue M - why no update median?

    _validateSolvency(msg.sender, newPositionIdList, NO_BUFFER);
}

Suppose the following assumptions:

Now suppose that we need to call _validateSolvency:

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

    int24 slowOracleTick;
    if (SLOW_ORACLE_UNISWAP_MODE) { //@audit-issue L dead code?
        slowOracleTick = PanopticMath.computeMedianObservedPrice(
            _univ3pool,
            observationIndex,
            observationCardinality,
            SLOW_ORACLE_CARDINALITY,
            SLOW_ORACLE_PERIOD
        );
    } else { 
        (slowOracleTick, medianData) = PanopticMath.computeInternalMedian(
            observationIndex,
            observationCardinality,
            MEDIAN_PERIOD,
@>          s_miniMedian, //@audit stale median
            _univ3pool
        );
    }

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

A stale s_miniMedian is used to calculate the slowOracleTick if someone has burned their position. The solvency check may pass even if this is no longer the case, as the latter solvency check uses the slowOracleTick.

Tools Used

Manual review

Recommended Mitigation Steps

Consider recalculating the median when a position is burned (and enough time has passed to prevent a price manipulation), similar to the minting logic:

    function burnOptions(
        TokenId tokenId,
        TokenId[] calldata newPositionIdList,
        int24 tickLimitLow,
        int24 tickLimitHigh
    ) external {
        _burnOptions(COMMIT_LONG_SETTLED, tokenId, msg.sender, tickLimitLow, tickLimitHigh); //@audit-issue M - why no update median?

-       _validateSolvency(msg.sender, newPositionIdList, NO_BUFFER);
+       uint256 medianData = _validateSolvency(msg.sender, newPositionIdList, NO_BUFFER);
+       if (medianData != 0) s_miniMedian = medianData;
    }

Assessed type

Invalid Validation

c4-judge commented 2 months ago

Picodes marked the issue as primary issue

dyedm1 commented 2 months ago

This is a subjective design choice -- no specific security issues arise from not updating the s_miniMedian at burn, or for that matter, any other operation (whether the price is stale or not depends on the specific situation and is up to interpretation -- we discouraged issues regarding stale prices in the contest README). There is a pokeMedian function that allows the median tick to be updated every minute if required.

c4-judge commented 2 months ago

Picodes marked the issue as unsatisfactory: Invalid

DadeKuma commented 2 months ago

Minting:

_mintOptions > _mintInSFPMAndUpdateCollateral > mintTokenizedPosition > _validateAndForwardToAMM > liquidity is added, and price has changed > median is updated with the new price

Burning:

_burnOptions > _burnAndHandleExercise > burnTokenizedPosition > _validateAndForwardToAMM > liquidity is removed, and price has changed > median is not updated with the new price

If we call mintOptions we start filling the circular buffer with values. Let's say that by minting we increase the price of TokenA: we start at price 100, and we call mintOptions 30 times, once every hour (the minimum is one minute) so we can fill the buffer, now we are at price 300.

If we now call burnOptions in the same way afterwards with the same values for 30 times, the price drops again to 100, but the median price is way higher than it should be, as it's not updated.

We have two oracle ticks: slow and fast. Usually we check only the fast one, but sometimes we have to check both if one of these is stale.

1) The fast oracle always checks the pool directly by computing the observed price, the median doesn't matter

2) The slow oracle always checks just the internal median, because SLOW_ORACLE_UNISWAP_MODE = false. The computation always return the stale value, as we only use medianTick when checking solvency.

Security issues:

Scenario: User is solvent at fast tick, but the slow tick is stale so we have to check both.

But the solvency check uses the slowOracleTick (i.e. the stale median). Result: the solvency check is very inaccurate, and this is a problem if the user was solvent with the old stale median, but it wasn't with the current one, as the check would pass.

Given the previous assumptions, this would permit users to burn their options when they are insolvent, for example.

There is a pokeMedian function that allows the median tick to be updated every minute if required.

This is true, but users have zero incentives to call this function themselves (especially for each single pool). The protocol would bear the cost of calling this function for each pool/pair of tokens, constantly, on Mainnet, which has a huge transaction cost; so it seems an unrealistic (or at least very expensive) solution to me.

Picodes commented 2 months ago

"users have zero incentives to call this function themselves" -> but liquidators do in theory

"this would permit users to burn their options when they are insolvent" -> so the edge case would be that the user is solvent at the fast tick (so recently), was at the old slow oracle, but isn't under the new median price, in which case it seems that this isn't too much of an issue as the user is solvent under the fast tick, and that the user was insolvent for some time (while the new median was building up) so should have been liquidated? Considering this and all the precautions taken by the sponsor in the readme, QA seems appropriate to me.

c4-judge commented 2 months ago

Picodes changed the severity to QA (Quality Assurance)

c4-judge commented 2 months ago

Picodes marked the issue as grade-a

DadeKuma commented 2 months ago

@Picodes

but liquidators do in theory

They do, but only if they know about this issue. Otherwise they would assume that the protocol works correctly, and it wouldn't make sense to spend money by calling that function.

so the edge case would be that the user is solvent at the fast tick (so recently), was at the old slow oracle, but isn't under the new median price, in which case it seems that this isn't too much of an issue as the user is solvent under the fast tick, and that the user was insolvent for some time (while the new median was building up) so should have been liquidated?

The worst case scenario is where both ticks are stale (i.e. the fast is stale, and the old is wrong as the median is not updated), this would let a user burn even if they are currently insolvent, because we are checking the solvency with a stale fast tick (but right now they are insolvent) and checking the backup solvency with the wrong price with the slow tick, and that would not revert.

/// @notice Falls back to the more conservative tick if the delta between the fast and slow oracle exceeds `MAX_SLOW_FAST_DELTA`.
/// @dev Effectively, this means that the users must be solvent at both the fast and slow oracle ticks if one of them is stale to mint or burn options.

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L881-L882

Right now user is insolvent > fast tick stale, but user is solvent > fallback to conservative slow tick > slow tick is wrong and user is solvent there, but with the correct one they would be insolvent > user can burn, but they shouldn't as they are insolvent

It's also worth noting that there are consequences if we remove the median calculation from mint:

no specific security issues arise from not updating the s_miniMedian at burn, or for that matter, any other operation

Suppose that also the minting never updated the median. The slow tick would always be zero, and all users would be considered insolvent, unless someone constantly calls pokeMedian for each pool.

There are some 'ifs', but this issue still seems Medium to me as there is a leak of value given these assumptions:

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.

Picodes commented 2 months ago

@DadeKuma my understanding is that the fast tick isn't dependant on s_miniMedian so the solvency is in any case equal to the one taken with the fast oracle. Then, the slow oracle can be lagging due to a lack of updates as you're showing here, but it's only a "double confirmation" of the user's solvency so it's not that much of an issue

"Suppose that also the minting never updated the median. The slow tick would always be zero, and all users would be considered insolvent unless someone constantly calls pokeMedian for each pool." -> My understanding is that this is to some extent the required design. You don't need to constantly call pokeMedian, you just need to have a keeper calling it whenever the prices move too much and it would lead to the window checks failing

Picodes commented 2 months ago

Tagging @dyedm1 for visibility