ge6a - Update price 2 times in the same block

[sherlock-admin2]

ge6a

high

Update price 2 times in the same block

Summary

Pyth allows for unlimited updates/reads of the price within a single block, as long as the publish date of each subsequent price is newer than the previous one. The protocol uses a maxAge of 60 seconds. A user can update the price, open a position, update the price again, and make a profit within a single block without any risk. The profit will be at the expense of the user on the other side of the trade.

Vulnerability Detail

There is no information available about a bot that continuously updates the price, nor about keepers who have an interest in updating the price. From the README file, it is only known that when a transaction is initiated from the frontend, the price is updated. Therefore, the attack is entirely possible and highly likely given the scenario described above. I want to note that it can be executed even in the presence of a bot, but in a slightly different variant. I won't describe it now because there is no data for such a scenario. Also if the attacker doesn't have any other positions they want pay funding or borrowing fee because the time since last update would be 0.

POC

```solidity function testOpenCloseSameBlock() public { vm.prank(taker1); clearingHouse.openPosition( IClearingHouse.OpenPositionParams({ marketId: marketId, maker: address(maker), isBaseToQuote: false, isExactInput: false, amount: 10 ether, oppositeAmountBound: 1000 ether, deadline: block.timestamp, makerData: "" }) ); maker.setBaseToQuotePrice(110e18); maker2.setBaseToQuotePrice(110e18); _mockPythPrice(110, 0); vm.prank(taker1); clearingHouse.closePosition( IClearingHouse.ClosePositionParams({ marketId: marketId, maker: address(maker), oppositeAmountBound: 1000 ether, deadline: block.timestamp, makerData: "" }) ); console.log("Margin of taker1:"); console.logInt(vault.getMargin(marketId, address(taker1))); console.log("Pending margin:"); console.logInt(vault.getPendingMargin(marketId, taker1)); } ```

Impact

Loss of funds for makers, 100% profit for attacker.

Code Snippet

https://github.com/sherlock-audit/2024-02-perpetual/blob/02f17e70a23da5d71364268ccf7ed9ee7cedf428/perp-contract-v3/src/oracle/pythOracleAdapter/PythOracleAdapter.sol#L22

Tool used

Manual Review

Recommendation

Update the source code to use always same price in a single block.

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

ReadMe says "Oracle (Pyth) is expected to accurately report the price of market"

[0xvj]

Escalate

The attack path of the actual issue demonstrates how attacker can use different prices returned by the pyth oracle in the same transaction to extract value from OracleMaker by depositing and withdrawing to get instant arbitrage opportunity.

But the attack path mentioned by warden in the POC uses opening and closing position in the same transaction which wouldn't work due to delay(opening and closing cannot be executed in the same tx as trades are executed by relayer)

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.

As this issue identifies the root cause but doesn't describe the attack path, this should be low as mentioned in the sherlock docs.

[sherlock-admin2]

Escalate

The attack path of the actual issue demonstrates how attacker can use different prices returned by the pyth oracle in the same transaction to extract value from OracleMaker by depositing and withdrawing to get instant arbitrage opportunity.

But the attack path mentioned by warden in the POC uses opening and closing position in the same transaction which wouldn't work due to delay(opening and closing cannot be executed in the same tx as trades are executed by relayer)

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.

As this issue identifies the root cause but doesn't describe the attack path, this should be low as mentioned in the sherlock docs.

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.

[nevillehuang]

@gstoyanovbg Could you verify if the above is true?

Also @0xvj delaying of orders seems to be true but not always executed:

Apart from matching the user's order, no additional losses should be incurred by the relayer. The relayer may have some value extraction strategies available (for example by delaying the execution of the orders), and is trusted not to use them.

[0xvj]

@nevillehuang By delay I mean that attacker cannot execute the opening and closing of a position in the same transaction since orders can only be settled by whitelisted matchers. Due to this the prices at which the trades execute is not in the control of attacker and opening and closing could be executed at any prices.

The original report(#123) is talking about extracting value from the oracle maker by depositing and withdrawing at different Pyth prices where the arbitrage profit can be amplified using a flash loan. But the issues #31 #32 #38 and #131 are talking about making arbitrage profit by opening and closing a trade which I think is not possible.

I would like to hear about your opinion on this @IllIllI000 @rc56

[IllIllI000]

I agree with the escalation, that the submission does not identify a valid attack path, since the whole point of the relayer is to introduce delays in order to prevent vanilla front-running of the OracleMaker. The readme itself says OrderGatewayV2’s relayer may not postpone order enough for mitigating others front-run oracle if the maker is OracleMaker, so anything to do with trying to predict the OracleMaker's trade price is invalid

[gstoyanovbg]

I disagree with the escalation.

1. The relayer may or may not have a delay, nowhere is stated that it will definitely have a delay.

2. Both functions ClearingHouse.openPosition and ClearingHouse.closePosition are permissionless and have nothing to do with the relayer. A user can call them directly from the contract independently, without restrictions. The idea of the relayer is to settle off-chain orders as clearly stated in the README file :

   Relayer is a permissioned address controlled by us, it'll be used to help users settle their off-chain orders(limit order, market order) on chain.

[0xvj]

1. The relayer may or may not have a delay, nowhere is stated that it will definitely have a delay.

@gstoyanovbg By delay I mean that attacker cannot execute the opening and closing of a position in the same transaction as mentioned above.

2. Both functions ClearingHouse.openPosition and ClearingHouse.closePosition are permissionless and have nothing to do with the relayer. A user can call them directly from the contract independently, without restrictions. The idea of the relayer is to settle off-chain orders as clearly stated in the README file.

For opening trades directly through the clearingHouse the opposite side trader must authorise you to open trades on behalf of him. In this case pyth price is not being used and the trade will be executed at the agreed upon price. Only racleMaker/spotHedgeMaker are using the Pyth prices and we cannot open trades with oracleMaker/spotHedgeMaker directly using clearningHouse.

[gstoyanovbg]

@0xvj Can you point to part of the source code that prove your claim ? I want to make sure that we are talking for the same thing.

[0xvj]

@0xvj Can you point to part of the source code that prove your claim ? I want to make sure that we are talking for the same thing.

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

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

The oracle price in the clearingHouse in only being used for making margin requirements checks not for order execution price.

[Simao123133]

The issues mentioning opening and closing positions should remain valid as there is not enough evidence supporting that the attack vector is invalid. It's a fact that open and close position orders may be settled in the same block and that the root cause still applies. The only question is if the attacker is able to pick the 2 prices that will be used when opening and closing the position. This last part depends on how the relayers are set up, including the api. For example, if the api to open and close positions allows sending a signed price, chosen by the user, to update the pyth oracle price, then this issue should be valid. My point is that there is not sufficient evidence to conclude that the attack path is invalid, even if exploiting this issue by depositing/withdrawing is easier.

[gstoyanovbg]

@0xvj Can you point to part of the source code that prove your claim ? I want to make sure that we are talking for the same thing.

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

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

The oracle price in the clearingHouse in only being used for making margin requirements checks not for order execution price.

I still don't see which part of the code you have in mind. I know what the cited 2 functions do but still am not sure that fully understand your point. Can you elaborate further what in them prove your claims ?

[nevillehuang]

@paco0x Can I double check your thoughts on this? Can we execute a same transaction of open and closing position directly via the clearing house contract?

[paco0x]

@paco0x Can I double check your thoughts on this? Can we execute a same transaction of open and closing position directly via the clearing house contract?

I agree with the escalation. The Oracle maker is designed to be executed by our relayer(matcher), so the user can not control the price updated to the contract when opening/closing with oracle maker. Users can only open and close position directly with the spot hedge maker, which only uses the external price for margin ratio calculation. This issue didn't state a clear path to attack by manipulating external prices and open/close position with spot hedge maker.

The maker in the POC code is a test maker that acts like an Oracle Maker but can interact with directly, we only use it in the test for convenience.

[gstoyanovbg]

@paco0x I would like to ask the same question that asked earlier because didn't receive an unambiguous answer - can you point me to a part of the source code that prove the claim from the escalation ? Thanks in advance.

[paco0x]

@paco0x I would like to ask the same question that asked earlier - can you point me to a part of the source code that prove the claim from the escalation ? Thanks in advance.

we have a sender address check here when opening position: https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/clearingHouse/ClearingHouse.sol#L281

it should be set to the OrderGatewayV2 contract in Oracle Maker: https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/clearingHouse/ClearingHouse.sol#L281, so only relayer can sender orders to it

in Spot Hedged Maker, it'll not check the sender: https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/maker/SpotHedgeBaseMaker.sol#L514-L516, anyone can open position directly with it, but it didn't use the external price in the maker itself.

we use a test maker a lot in the test codes for simplicity, by default it allows anyone to interact with it directly, https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/test/helper/TestMaker.sol#L58-L62

[Simao123133]

This was in the readme:

'we're using off-chain oracle (Pyth) to fetch the price of assets, when interacting with the protocol, user first obtains a signed price through API, then updates this price to the protocol and interacts'.

Can't this be used to pick the price? There was no information in the readme clearly stating that the price could not be picked (if valid within 60s) by the user.

As I said before, the validity of this attack path depends on how the backend/frontend works, which should not make this issue invalid (unless it was clearly stated in the docs). Not only because this information was not available to us, but also because the backend/frontend may change.

Thus, I believe the attack path can not be invalidated.

[gstoyanovbg]

@paco0x So you intend to make the only valid sender to be the relayer. From the way the readme file is written and from the fact that in some places in the tests takers are added as valid senders for Oracle maker, I was under the impression that it is possible for users to interact directly with the protocol and be part of the Oracle Maker whitelist. I take it as my mistake, because obviously some of the other Watsons understood it correctly.

@Simao123133 You are technically right and agree with your statement but I don't believe it will change the judges' decision

[nevillehuang]

Based on sponsor comments here and here, I believe this issue to be invalid.

[Simao123133]

Why should this issue be invalid if the reason for the attack path to be invalid is based on the backend? We did not have the chance to look at the backend code and the readme was not clear about how the relayer selects the price (again, users could call an api with a valid price and send it to the relayer, exploiting this issue). The code is clear and as far as it is concerned, this attack path is valid.

[IllIllI000]

because obviously some of the other Watsons understood it correctly.

It took me a while to find this because I thought I had remembered it from somewhere in the docs, but it was actually in a code comment:

// This predictable price can be exploited by a front-runner, so any trade on OracleMaker must be 2-step, aka "delayed",
// and can only be interacted with through OrderGateway and OrderGatewayV2. This is configurable
// via OracleMaker.setValidSender().

https://github.com/sherlock-audit/2024-02-perpetual/blob/02f17e70a23da5d71364268ccf7ed9ee7cedf428/perp-contract-v3/src/maker/OracleMaker.sol#L28-L30

[WangSecurity]

Sorry, made a typo, deleting the previous message to not cause any confusion. Thank you for tagging!

I agree that this issue should indeed be invalid. The execution of transactions to open/close position is not controlled by the attacker. Therefore, the PoC is incorrect and under the duplication rule highlighted by the escalating watson, I'm planning to accept the escalation and invalidate the issue.

[Evert0x]

Result: Unique Invalid

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• 0xvj: accepted