c4-bot-10 commented 6 months ago

Lines of code

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

Vulnerability details

Proof of Concept

First take a look at https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L156-L158


    int256 internal constant MAX_TWAP_DELTA_LIQUIDATION = 513;

We can see that there is a hardcoded implementation of the maximum amount of allowed difference between the currentTick and the Uniswap's TWAP tick, would be important to note that the only instance where this is used is whenever liquidations are to occur, i.e https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L156-L160

    function liquidate(
        TokenId[] calldata positionIdListLiquidator,
        address liquidatee,
        LeftRightUnsigned delegations,
        TokenId[] calldata positionIdList
    ) external {
        _validatePositionList(liquidatee, positionIdList, 0);

        // Assert the account we are liquidating is actually insolvent
        int24 twapTick = getUniV3TWAP();

        LeftRightUnsigned tokenData0;
        LeftRightUnsigned tokenData1;
        LeftRightSigned premia;
        {
            (, int24 currentTick, , , , , ) = s_univ3pool.slot0();

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

        emit AccountLiquidated(msg.sender, liquidatee, bonusAmounts);
    }

Issue with the current implementation is that the idea of hardcoding MAX_TWAP_DELTA_LIQUIDATION is heavily flawed as to different instances different volatility, as such this could cause heavy reversions in case where they shouldn't be a revert, that's to say on attempts to liquidations of distressed accounts, even if the position is heavily underwater the STALE TWAP price check would cause the attempt on liquidating to revert, cause the protocol would assume there is a price manipulation going on, which then allows the distressed account to hold on to the bad position for even longer, allowing accumulation of bad debt as the position continues to drop in value (which is not uncommon in the crypto world).

Keep in mind that the absolute difference between currentTick and the twapTick is being taken, i.e even if the current price from slot0 is very low indicating position is heavily underwater the check still reverts, which completely flaws protocol's assumption from this comment "// Enforce maximum delta between TWAP and currentTick to prevent extreme price manipulation", cause in this case the prices were not manipulated but rather the asset dropped in value, by just navigating to any reliable oracle/price provider we can see multiple instances where assets that have massive TVL are dropping/incrementing by 5% within a short window, and this is not cause the price was manipulated but because of how volatile these assets are which is due to the fact that the crypto (Defi), is still a a baby compared to tradFi, so this needs to be set differently in a case by case instance, that's to say for cases where prices are easily incremented then the MAX_TWAP_DELTA_LIQUIDATION should be set higher, however for instances that are quite strengthy in their base, then one would even suggest dropping the value of MAX_TWAP_DELTA_LIQUIDATION, but it shouldn't be too low where it now disrupts normal executions or causes continuous failures.

Impact

Often reverts on liquidation attempts for tokens that are quite volatile, thereby causing protocol to allow distressed/liquidatable accounts to hold on to bad debts longer and risk the solvency state since a liquidator would not want to delegate the amount of assets needed to get the liquidation to pass, keep in mind that if the token value for this asset is relatively dropping then this reversions makes the position no longer enticing for the liquidator as the asset value is dropping, and (asset value) might be too small by the time the if (Math.abs(currentTick - twapTick) > MAX_TWAP_DELTA_LIQUIDATION) revert Errors.StaleTWAP(); check passes.

Another subtle impact would be for cases that are not as volatile, them having over a 5% acceptable spread is quite much and would lead to protocol to actually ingest stale data in some cases for such tokens.

Recommended Mitigation Steps

This is a problem for multiple DEFI protocols on different chains, and the best fix I've come across so far is to adopt different ranges of tolerances for different behaviours, this way a more specific case by case price manipulation bound is being set. Additionally introduce a functionality protected by an admin to be able to update the value for `MAX_TWAP_DELTA_LIQUIDATION, this way, post deployment any change can be made.

Assessed type

Context

dyedm1 commented 6 months ago

Dup #466

c4-judge commented 5 months ago

Picodes marked the issue as duplicate of #466