Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A malicious distressed/liquidatable-account holder can DOS the liquidator's TX of the PanopticPool#liquidate() to prevent their account from a liquidation #313

Closed
c4-bot-5 opened this issue Apr 21, 2024 · 5 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-c primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_31_group AI based duplicate group recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-5
Copy link
Contributor

c4-bot-5 commented Apr 21, 2024

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/PanopticPool.sol#L1032
https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/PanopticPool.sol#L1035-L1036

Vulnerability details

Impact

Within the PanopticPool contract, the MAX_TWAP_DELTA_LIQUIDATION would be defined and 513 (approximately 5%) is set to it like this:
https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/PanopticPool.sol#L156

    // The maximum allowed delta between the currentTick and the Uniswap TWAP tick during a liquidation (~5% down, ~5.26% up)
    // Prevents manipulation of the currentTick to liquidate positions at a less favorable price
    int256 internal constant MAX_TWAP_DELTA_LIQUIDATION = 513; ///<----------- @audit

Within the PanopticPool#liquidate(), it would be checked whether or not the price delta between two prices below would exceed the MAX_TWAP_DELTA_LIQUIDATION :

    /// @notice Liquidates a distressed account. Will burn all positions and will issue a bonus to the liquidator.
    /// @dev Will revert if liquidated account is solvent at the TWAP tick or if TWAP tick is too far away from the current tick. ///<----------- @audit
    /// @param positionIdListLiquidator List of positions owned by the liquidator.
    /// @param liquidatee Address of the distressed account.
    /// @param delegations LeftRight amounts of token0 and token1 (token0:token1 right:left) delegated to the liquidatee by the liquidator so the option can be smoothly exercised.
    /// @param positionIdList List of positions owned by the user. Written as [tokenId1, tokenId2, ...].
    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();  ///<----------- @audit

        LeftRightUnsigned tokenData0;
        LeftRightUnsigned tokenData1;
        LeftRightSigned premia;
        {
            (, int24 currentTick, , , , , ) = s_univ3pool.slot0();  ///<----------- @audit

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

Based on the "slot0" in the UniswapV3's documentation, both the current tick (currentTick) and the current price (sqrtPriceX96) would be returned from the UniswapV3Pool#slot0.
The current price (sqrtPriceX96) would be a spot price. And the current tick (currentTick) would usually follow the current price (sqrtPriceX96).

According to the NatSpec of the PanopticPool#liquidate(), the TX of the PanopticPool#liquidate() would be reverted - if TWAP tick (twapTick) is too far away from the current tick (currentTick) like this:

@dev Will revert if liquidated account is solvent at the TWAP tick or if TWAP tick is too far away from the current tick.

Since the current tick (currentTick)-retrieved via the UniswapV3Pool#slot0() would usually follow the current price (sqrtPriceX96)-retrieved via the UniswapV3Pool#slot0(), the currentTick would usually follow a spot price (sqrtPriceX96).
Hence, a malicious actor would easily manipulate the currentTick by sandwiching a victim liquidator's TX of the PanopticPool#liquidate() with directly swapping a large amount of token0/token1 on the target UniswapV3Pool in a single block.
As a result, the victim liquidator's TX of the PanopticPool#liquidate() can be reverted in the validation below:
https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/PanopticPool.sol#L1035-L1036

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

Because, when the victim liquidator's TX of the PanopticPool#liquidate() would be executed right after the malicious actor would dramatically inflate/deflate the current price (sqrtPriceX96) of the target UniswapV3 Pool, the current tick (currentTick) of the target UniswapV3 Pool would also dramatically be moved in the same block, the price delta between two prices (currentTick - twapTick) would exceed the MAX_TWAP_DELTA_LIQUIDATION .

Further more, according to the "Options Traders" part in the documentation, any ERC20 token can be used in the Panoptic Protocol.

Any Token
Users can trade perpetual options on any ERC-20 token, thanks to the permissionless nature of Panoptic and Uniswap.

It means that lower liquidity UniswapV3 Pool would be available on the Panoptic Protocol.
And therefore, lower liquidity UniswapV3 Pool would relatively be easier for a malicious actor to manipulate the currentTick (spot price) by exceeding the MAX_TWAP_DELTA_LIQUIDATION (approximately 5%) at lower cost.

NOTE:
The vulnerabllity that the current price (sqrtPriceX96) of the UniswapV3 Pool is easily manipulated would be proven in the past findings like this:

Proof of Concept

NOTE:
Based on the "slot0" in the UniswapV3's documentation, both the current tick (currentTick) and the current price (sqrtPriceX96) would be returned from the UniswapV3Pool#slot0. And the current tick (currentTick) would usually follow the current price (sqrtPriceX96).

Let's say there are 2 actors:

  • Alice is a liquidator (victim)
  • Bob is a malicious distressed/liquidatable-account holder, who has the large amount of capital.

Attack scenario:

  • 1/ Alice would call the PanopticPool#liquidate() to liquidate Bob's distressed/liquidatable account.

  • 2/ Bob would monitor the TX of the step 1/ and he would front-run it with directly swapping a large amount of token0 for token1 on the UniswapV3Pool, which is the target UniswapV3Pool that Alice attempted to liquidate when the step 1/.

  • 3/ Right after, Bob would submit another TX that back-run the TX of the step 1/ with directly swapping a large amount of token1 for token0 on the UniswapV3Pool, which is as opposite direction of the swap to the step 2/ above.

  • 4/ Bob's TX (step 2/) would be executed first.

    • Now, the current price (sqrtPriceX96) of the target UniswapV3 Pool has been dramatically moved (inflated /or deflated). Hence, the current tick (currentTick) of the target UniswapV3 Pool has also been dramatically moved.
  • 5/ Alice's TX would be executed second.

    • Since the current tick (currentTick) has been dramatically moved when the step 4/, this her TX would be reverted - due to exceeding the MAX_TWAP_DELTA_LIQUIDATION (approximately 5%) inside the validation in the PanopticPool#liquidate() like this:
            // Enforce maximum delta between TWAP and currentTick to prevent extreme price manipulation
            if (Math.abs(currentTick - twapTick) > MAX_TWAP_DELTA_LIQUIDATION)
                revert Errors.StaleTWAP();
  • 6/ Bob's TX (step 3/) would be executed third.
    • Now, the current price (sqrtPriceX96) and the current tick (currentTick) of the target UniswapV3 Pool has been back to the original price and tick when the step 1/.

Tools Used

  • Foundry

Recommended Mitigation Steps

Within the PanopticPool#liquidate(), consider increasing the MAX_TWAP_DELTA_LIQUIDATION from approximately 5% ( 513) to much higher value. (i.e. 30% (3000)).

Assessed type

DoS

@c4-bot-5 c4-bot-5 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Apr 21, 2024
c4-bot-3 added a commit that referenced this issue Apr 21, 2024
@c4-bot-13 c4-bot-13 added the 🤖_31_group AI based duplicate group recommendation label Apr 22, 2024
@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Apr 24, 2024
@c4-judge
Copy link
Contributor

Picodes marked the issue as primary issue

@dyedm1 dyedm1 added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Apr 26, 2024
@dyedm1
Copy link

dyedm1 commented Apr 26, 2024

It is indeed possible to stop a liquidation transaction in this way, but it is expensive, one-time, and the liquidator could just send the transaction through a private mempool if needed (they should be doing this anyway if there is a public mempool, since their liquidation would get stolen by a generalized frontrunner). It is unlikely a liquidatable account holder would be consistently successful in evading liquidation, especially considering the financial incentives involved as the account becomes more distressed.

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels May 6, 2024
@c4-judge
Copy link
Contributor

c4-judge commented May 6, 2024

Picodes changed the severity to QA (Quality Assurance)

@Picodes
Copy link

Picodes commented May 6, 2024

Downgrading to Low with other reports related to being able to front run and revert liquidation transactions as the argument that liquidators should use private mempools to avoid disclosing information seems strong to me.

@c4-judge c4-judge closed this as completed May 6, 2024
@c4-judge
Copy link
Contributor

c4-judge commented May 6, 2024

Picodes marked the issue as grade-c

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-c primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_31_group AI based duplicate group recommendation sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

6 participants