santipu_ - Attacker can sandwich its own position settlement on `SpotHedgeBaseMaker` to get a better price and have instant profits

[sherlock-admin4]

santipu_

high

Attacker can sandwich its own position settlement on SpotHedgeBaseMaker to get a better price and have instant profits

Summary

SpotHedgeBaseMaker is a maker that hedges its positions, meaning every time it fills an order for a trader, it swaps the filled amount on a DEX to cover that position. An attacker can sandwich its own position settlement with SpotHedgeBaserMaker to inflate/deflate the DEX price and get a better price to settle the position, having instant profits.

For executing this attack, there's no need to FRONTRUN or BACKRUN, the attack can be executed in the same transaction.

Vulnerability Detail

Every time a trader wants to settle a position with SpotHedgeBaseMaker, the maker will swap the filled amount in a DEX to hedge the swap. The price from the swap will be the opening price for that position. This mechanism is executed to protect the LPs at SpotHedgeBaseMaker from price changes when the maker is too short/long skewed.

An attacker can sandwich its own position settlement with SpotHedgeBaseMaker to get a better price for the opened position. This can be executed following these steps:

For this scenario, we'll assume that the base token is ETH and the quote token is USDC. The current ETH price will be 4,000 USD.

1. The attacker gets a huge free flash loan of USDC (e.g., from Morpho Blue).
2. Swap a huge amount of USDC to ETH on the pool used by SpotHedgeBaseMaker, inflating the price from $4,000 to $5,000.
3. Open a short position using SpotHedgeBaseMaker.
4. The maker will swap ETH for USDC at the price of $5,000.
5. The attacker will have the short position opened with a price of $5,000 instead of the market price of $4,000.
6. Swap back the ETH to USDC on the pool to return the pool price to market.
7. Return the flash loan
8. Close the short position using the market price by OracleMaker.
9. PROFIT!

The only active mitigation to this attack is the price band check that is done on ClearingHouse._openPosition which checks that the settlement price doesn't differ too much from the market price. The check called from here and executed here:

https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/clearingHouse/ClearingHouse.sol#L461-L476

/// @notice trade price must be within the price band, which is the oracle price +/- priceBandRatio
function _checkPriceBand(uint256 marketId, uint256 tradePrice) internal view {
    IAddressManager addressManager = getAddressManager();
    Config config = addressManager.getConfig();
    uint256 priceBandRatio = config.getPriceBandRatio(marketId);
    if (priceBandRatio == 0) {
        return;
    }
    bytes32 priceFeedId = config.getPriceFeedId(marketId);
    (uint256 oraclePrice, ) = addressManager.getPythOracleAdapter().getPrice(priceFeedId);
    uint256 upperPrice = oraclePrice.mulWad(WAD + priceBandRatio);
    uint256 lowerPrice = oraclePrice.mulWad(WAD - priceBandRatio);
    if (upperPrice < tradePrice || tradePrice < lowerPrice) {
        revert LibError.PriceOutOfBound(tradePrice, lowerPrice, upperPrice);
    }
}

Even though this mitigation would make this attack less profitable, the attack can still be executed by only moving the DEX price to the percentage allowed by ClearingHouse._checkPriceBand. Moreover, the attack can still empty the PnL pool by executing the same steps lots of times with different positions.

Impact

An attacker can sandwich his own position settlement to get a better price for the position and acquire instant profits by closing the position at the market price. By repeating the attack sequence many times, the attacker could empty the PnL pool.

Code Snippet

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

Tool used

Manual Review

Recommendation

To mitigate this issue, it is recommended to set a slippage value on all the swaps on SpotHedgeBaseMaker to check that the received tokens don't differ too much from the market value. That slippage value should be set with the oracle price from PYTH, this should prevent attackers from sandwiching their own settlements to obtain a better price for the position.

Duplicate of #119

[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

[paco0x]

There're some nuances between this issue and #119, in #119 the attack is performed under the assumption of price difference between pyth & external. And the attacker can intentionally pick pyth prices to update and cause a bad debt and systemic risk. The Spot Hedge Maker is still hedged under both cases. The profit of #119 is coming from the baddebt attacker made.

Without a price difference as the prerequisite, we don't think the attacker can be profitable unless providing a valid POC script.

[IllIllI000]

Escalate

Between #128 and #119, #128 is the high whereas #119 is a medium. I submitted them this way because #119 is limited by protections in the code, whereas #128 shows how those limitations can be bypassed via a front-run. Since a front-run is involved, and they're of different severities, according to the duplication rules this falls into the category of Scenario B where #128 is root issue A, and #119 and #77 are issues in category C, and are therefore Low, leaving #128 as a solo-high.

I believe the purpose of the duplication rule is to reward work that is done on top of the work other submissions have provided, and #128 does this

[sherlock-admin2]

Escalate

Between #128 and #119, #128 is the high whereas #119 is a medium. I submitted them this way because #119 is limited by protections in the code, whereas #128 shows how those limitations can be bypassed via a front-run. Since a front-run is involved, and they're of different severities, according to the duplication rules this falls into the category of Scenario B where #128 is root issue A, and #119 and #77 are issues in category C, and are therefore Low, leaving #128 as a solo-high.

I believe the purpose of the duplication rule is to reward work that is done on top of the work other submissions have provided, and #128 does this

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.

[santipu03]

@IllIllI000 Feel free to correct me but I'm not sure if the kind of front-running mentioned in issue #128 can be considered a "generic vulnerability" as it's stated in the Sherlock docs. I think the docs were referring to the classic front-running issue, where a bot includes a malicious txn before a victim's txn, but this attack isn't possible on Optimism/Blast.

Maybe @Czar102 can clarify things here but by reading the mentioned duplication rules I don't think this case falls into Scenario B because the underlying root cause cannot be considered a "generic vulnerability".

[IllIllI000]

I confirmed that sandwiching counts as front-running, which this submission as well as the duplicate involve, and am assuming that oracle updates such as are being described in #128 do too, though the rule doesn't say anything about what's required for the add-on attack path submission

[santipu03]

The root cause of these three issues is the same: there is no delay in settling positions with SpotHedgeBaseMaker. The mitigation is also the same: use a relayer to settle orders with SpotHedgeBaseMaker.

Now, all of the three issues couldn't have been possible without the root cause. Therefore, even if they're on different severities (medium or high) they should all be marked as duplicates as described in Scenario A. From my point of view, Scenario B is describing the cases when the root cause is a general vulnerability, but I don't think this can be applied here.

I'd love to hear some feedback from @Czar102 about this situation here.

[IllIllI000]

there is no delay is a suggested fix for the root cause of a sandwich/front-run. All sandwiches/front-runs are solved by either introducing delays or having slippage

[nevillehuang]

Agree with @santipu03 comments, I believe this issues should remain as duplicates since the root cause and fix is the same applied to all three issues.

[WangSecurity]

Agree with @santipu03. Scenario A should be applied here since this sandwhich attack is not a regular front-running, i.e. observing the mempool and setting your transactions before/after them. All the issues have the same root cause and fix, therefore, planning to reject the escalation and leave the issue as it is.

[IllIllI000]

IllIllI — 
hi @Czar102 for Scenario B, is sandwiching considered a form of front-running?
Czar102 — 
It requires frontrunning (and backrunning), so any restrictions related to frontrunning should apply, right?

https://discord.com/channels/812037309376495636/1219302941165355059/1225402389574451310

[WangSecurity]

@IllIllI000 by sandwich attack you mean that the attacker will front-run and then back-run another transaction by putting them in this specific order in the block? Right now, the issue 119 seems to be just making two calls right one after another. Or am I missing something?

[IllIllI000]

@WangSecurity the attacker is sandwiching their own trade by doing it in a single transaction in a block, rather than three separate ones in a block

[WangSecurity]

@IllIllI000 as I understand it still doesn't require the classic front-running and back-running, but can be executed in one transaction just from a contract?

[IllIllI000]

It is classic front-running and back-running, but executed in one transaction by a single contract call that does all three parts

[WangSecurity]

I still believe that Scenario B cannot be applied here and we should judge this issue according to Scenario A. I agree with the comment here, hence, planning to reject the escalation and leave the issue as it is.

[IllIllI000]

@WangSecurity it's not clear to me what part of the message you're linking to has convinced you. Can you elaborate? It says From my point of view, Scenario B is describing the cases when the root cause is a general vulnerability, but I don't think this can be applied here, but this is an example from the list in scenario B: Front-running, and Czarek explicitly said it counted as front-running. And "I don't think this can be applied here" is an opinion without actual logic being provided. Are you saying that it's not front-running? If not, what is the distinguishing characteristic that's missing?

[WangSecurity]

The problem is that I don't see what you're trying to front-run in #119

As I understand the workflow:

1. The attacker opening a short of 1Eth
2. means that the SpotHedgeBaseMaker ends up going long 1Eth, and hedges that long by swapping 1WEth for $1
3. After the attacker unwinds the skew, they've gained 1WEth ($4k) from the rebalance

Therefore, as I understand the hedge by SpotHedgeBaseMaker doesn't happen by itself, it happens only cause the attacker firstly opened the short. Therefore, I believe it's not a front-running here.

But if the issue the following, then the escalation can be accepted:

1. The attacker see that the SpotHedgeBaseMaker is going long 1Eth, and hedges that long by swapping 1WEth for $1
2. Attacker front-runs this transaction by his short of 1Eth.
3. SpotHedgeBaseMaker transaction goes through after the attacker
4. After the attacker unwinds the skew, they've gained 1WEth ($4k) from the rebalance

I believe what happens here is in fact the first scenario I addressed above, therefore it's not a front-running per se, cause there would be no hedge by SpotHedgeBaseMaker if the attacker doesn't initiate a short firstly

[IllIllI000]

But if the issue the following, then the escalation can be accepted:

I'm not seeing how that's not what I've outlined. The attacker 'sees' the SHBM is going to go long, because the attacker is going to make it go long. The sandwich in #119 in my POC happens at the comment lines // first stage: skew in favor of collateral token and // third stage: swap back to fix the skew, and in between those is the SHBM's trade via the call to openPosition(), with the comment // second stage: open short position on SHBM. Are you saying those three operations don't constitute a sandwich?

[santipu03]

Given that now #119 will be considered a medium, they now have the same severity and they should be considered duplicates regardless of this discussion right?

[WangSecurity]

@IllIllI000 I believe it's not a classic front-running cause the transaction by SHBM will not execute if the attacker doesn't open a short firstly. Therefore, I think we're not front-running the transaction but exploit the workflow of the protocol.

[IllIllI000]

If I tell my friend to make the trade, and we coordinate such that our transactions reach the sequencer in the right order, then isn't it the same thing? The protocol loses money in both cases, just with an extra person in the case that you want.

[IllIllI000]

@santipu03 I submitted #119 and #128 with the belief that they were both high, but that #119 would be reduced to Med, given the price band limitation. Even though #119 was downgraded due to having constraints that I did not originally see, #128 does not have that limitation (I'll work on a POC to confirm) and should remain high, and the escalation outcome is still relevant.

[santipu03]

Oh, because #128 is marked as a duplicate of #119, I thought that if #119 is downgraded, all its duplicates were also downgraded to medium.

[nevillehuang]

@WangSecurity If you agree #128 is a valid high, than issues duplicated to it will also be a valid high as per sherlock rule Scenario A, since the impact highlighted in #119 and #77 are valid mediums as well.

Scenario A: There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attack paths:

• B -> high severity path
• C -> medium severity attack path/just identifying the vulnerability. Both B & C would not have been possible if error A did not exist in the first place. In this case, both B & C should be put together as duplicates.
• In addition to this, there is a submission D which identifies the core issue but does not clearly describe the impact or an attack path. Then D is considered low.

[WangSecurity]

Can you please tell me where it's incorrect? After reading both #119 and #77 in detail again, they look extremely similar to me and I believe this report is of sufficient quality. If it's not, can you please tell me which part of this report ia incorrect exactly?

[IllIllI000]

Sorry, you're right - I was misremembering my own submission. While mine uses bad debt, it doesn't fully describe where that comes from, so the two are similar enough that one can't really be accepted without the other one being too. Deleted

[WangSecurity]

Planning to reject the escalation and leave the issue as it is.

[Evert0x]

Result: High Duplicate of #119

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• IllIllI000: rejected