sherlock-audit / 2024-02-perpetual-judging

2 stars 2 forks source link

ge6a - The protocol uses prices in usd instead of usdt/usdc #149

Closed sherlock-admin3 closed 4 months ago

sherlock-admin3 commented 4 months ago

ge6a

medium

The protocol uses prices in usd instead of usdt/usdc

Summary

The Pyth network does not have Token/USDT or Token/USDC pairs. All prices are returned in USD, for example wBTC/USD. USDT/USDC are pegged to USD, but in case of deviation in the price, this can be used for arbitrage and making profits at the expense of the users of the protocol.

Vulnerability Detail

For example, in the event of a 20% depeg of USDT/USDC, a user can deposit a certain amount of USDT/USDC and open a position that is 20% larger than they could if the real price were used. Also from historical perspective it is not unlikely to have temporary (or not) depegs of stablecoins.

Impact

Loss of funds for the traders.

Code Snippet

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

Tool used

Manual Review

Recommendation

Pull USDT/USD or USDC/USD and use the real price of the collateral.

sherlock-admin3 commented 4 months ago

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

santipu_ commented:

Medium

Oot2k commented 3 months ago

Escalate

I don't think this is a dupe, but rather invalid.

For a perpetual contract (and in perpetual trading in general), the price of the collateral does not matter, for it is only used in the accounting of the PnL when a position is settled. This is also why the following issue in another perpetual trading protocol contest was rejected.

This is only an issue in this particular protocol because of the existence of SpotHedgeBasedMaker, which hedges its position based on Uniswap V3. In other words it's a problem because said Maker is exposed to external prices. It's not a problem in a standard perpetual trading protocol. To make this an issue, you must be able to identify an attack path/a scenario where the difference between external prices matter.

Also by the following Sherlock duping rule:

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.

The impact is vague and there's no attack path here, so I think this is a low.

sherlock-admin2 commented 3 months ago

Escalate

I don't think this is a dupe, but rather invalid.

For a perpetual contract (and in perpetual trading in general), the price of the collateral does not matter, for it is only used in the accounting of the PnL when a position is settled. This is also why the following issue in another perpetual trading protocol contest was rejected.

This is only an issue in this particular protocol because of the existence of SpotHedgeBasedMaker, which hedges its position based on Uniswap V3. In other words it's a problem because said Maker is exposed to external prices. It's not a problem in a standard perpetual trading protocol. To make this an issue, you must be able to identify an attack path/a scenario where the difference between external prices matter.

Also by the following Sherlock duping rule:

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.

The impact is vague and there's no attack path here, so I think this is a low.

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 commented 3 months ago

@Oot2k I agree with your escalation, I believe this issue does not have sufficient information that highlights the possible attack vector

gstoyanovbg commented 3 months ago

I disagree with the escalation. The assertion from the escalation that the price of the used stablecoin does not matter is not true.

Let's consider the following example.

Price of BTC: 10 USD Price of USDC: 1 USD

  1. User A opens a long position with 200 USDC with a total value of $200. OpenNotional = $200, positionSize = 20 BTC
  2. USDC depegs by 50%.
  3. User B opens a long position with 200 USDC with a total value of $100. OpenNotional = $200, positionSize = 20 BTC
  4. USDC restores its peg. The price of BTC measured in USD increases by 20%. The profit for the positions of User A and User B is 20*12-200 = $40. In percentage terms relative to the investment made, the profit for User A is 20%, but for User B it is 40% because the price of USDC measured in USD is not taken into account. This additional profit of $20 (20%) will be paid by the other traders. And similarly, the difference in the risk taken will be paid by the other traders as well. So it is clear that this scenario shows loss of funds for the traders.

In the original report, I described this trivial scenario with a few sentences and did not think that additional explanations were necessary. If such were needed, a POC could have been requested. The other reports may describe other scenarios, but that does not invalidate this report. In Sherlock's rules, it is not stated anywhere that it is mandatory to describe all possible attack scenarios arising from a vulnerability.

Finally, I want to remind that historic decisions are not a source of truth, which is why I have not reviewed the linked report from another contest.

nevillehuang commented 3 months ago

@gstoyanovbg I believe this issue is invalid/low severity due to the lack of sufficient identification of the attack vector specific to perpetual code logic resulting from the depeg.

gstoyanovbg commented 3 months ago

Attack path:

For example, in the event of a 20% depeg of USDT/USDC, a user can deposit a certain amount of USDT/USDC and open a position that is 20% larger than they could if the real price were used.

Impact:

Loss of funds for the traders.

IMO to invalidate a report based on this rule an impact or an attack path should be missing or incorrect. I already proved that the claim from the escalation is not true and that both correct impact and correct attack path are provided in the original report. Their misunderstanding by some Watsons is not a reason for invalidation; if it were, 80% of valid reports in Sherlock would have to be invalid.

detectiveking123 commented 3 months ago

This issue and its duplicates are all invalid. There's no way "this functionality fails in the event of a stablecoin depeg" is a valid medium.

WangSecurity commented 3 months ago

I believe this issue should indeed remain a duplicate. The watson identifies the core issue and briefly mentions the attack path. Therefore, planning to reject the escalation and leave the issue as it is.

detectiveking123 commented 3 months ago

@WangSecurity Why was my comment marked as off-topic? Sherlock has historically not awarded issues that rely on stable-coin de-pegs.

cc @Evert0x

IllIllI000 commented 3 months ago

@WangSecurity the submission does not actually mention the actual attack path. Even in the subsequent explanation the submitter misses what the attack is. Two traders opening/closing trades will have the same PnL, not different amounts. The attack must involve the SpotHedgeBaseMaker's uniswap trades being priced against the pyth oracle - that is the only place that can be attacked (see my submission for a full explanation).

Oot2k commented 3 months ago

Agree with @IllIllI000 here.

WangSecurity commented 3 months ago

After reading all three reports again, to me it still seems that the attack path here indeed valid:

For example, in the event of a 20% depeg of USDT/USDC, a user can deposit a certain amount of USDT/USDC and open a position that is 20% larger than they could if the real price were used

But as I understand from @IllIllI000 comment, it's incorrect. I see how it's different from your, but still seems valid. I understand that it's not as detailed as in your submission or in PUSH0's submission. Can you please explain it in a bit more detail, how this one is incorrect, despite the fact it doesn't mention "SpotHedgeBaseMaker's uniswap trades being priced against the pyth oracle"? Thank you in advance!

IllIllI000 commented 3 months ago

If USDC de-pegs by 30%, the pyth price of Eth will still be $2000/1 Eth. If the user deposits 2000 USDC, they'll be able to go long 1 Eth. When USDC regains its peg and the user sells and exits the platform, they'll still only get back their original 2000 USDC, and won't have made any money, so there's no attack. I agree that it's not good that they got to spend only $1400 to get $2000 in buying power, but the finding did not show a way to exploit this. The rules say 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.

Oot2k commented 3 months ago

I furthermore think there is actually no issue with scenario @IllIllI000 mentions. In case the spends 1400 USD to get 2000 USDC buying power, he is subject to same risks as just buying the USDC and holding till the peg restores.

The important fact on this issue as I mentioned before is the price difference between DEX and Pyth. Which this report does not mention.

gstoyanovbg commented 3 months ago

Disagree with the above comments. The problem arises when the price of ETH changes.

Example: User buys 2000 USDC with 1400 USD, opens a long position of 1 ETH worth 2000 USD, the price of ETH increases by 10%, the peg is restored, and the user withdraws 2100 USD. If the user had held USDC without opening a position, it would have been worth 2000 USD. If the user had opened a position with the real value of 1400 USD, they would have withdrawn 1540 USD. Similarly, the risk of losses is lower because the user can use the difference as a buffer for real losses.

WangSecurity commented 3 months ago

@detectiveking123 I don't know why your comment was marked as off-topic, it was like that before me. Previous decisions doesn't act as a source of truth, if you can forward me to where in the rules it should be invalid, please do so, since I don't see it.

I'm trying to better understand your point of view @IllIllI000 @Oot2k so can you please verify if the attack path is valid or not (just my arbitrary thought to better understand the issue):

  1. Normal Conditions: ETH is $2000 and USDC is $1.

  2. USDC depegs: ETH is still $2000 and USDC is $0.9. Hence, ETH is 1800 USDC. The user opened 1 ETH long and spents 1800 USDC.

  3. USDC back to peg: ETH is $2000 and USDC is $1. Hence, ETH is 2000 USDC. The user now closes the 1 ETH long and profits 200 USDC.

If I'm wrong, please tell me. I see that both of you say the issue is specifically to SpotHedgeMarket and Uniswap, but to better understand, please tell me if the attack path above can be valid?

IllIllI000 commented 3 months ago

If the user trades against the OracleMaker, the OracleMaker quotes prices based on the pyth oracle of Eth/USD. When the user deposits 2000 USDC, they will get $2000 (USD) worth of buying power, so your steps 2 and 3 should be:

  1. USDC depegs: ETH is still $2000 and USDC is $0.9. But, ETH is still $2000. The user opened 1 ETH long and spends 2000 USDC, since the protocol treats USDC as USD.

  2. USDC back to peg: ETH is $2000 and USDC is $1. And, ETH is still $2000. The user now closes the 1 ETH long and profits 0 USDC when they withdraw the USD as USDC.

gstoyanovbg commented 3 months ago

@WangSecurity I agree that in the scenario that you mentioned there is no additional profit for the user because they deposit 2000 USDC and withdraw 2000 USDC. So the comment of @IllIllI000 is technically correct. But this is not the scenario i meant. The problem for the protocol arises when ETH/USD changes its price in the meantime.

IllIllI000 commented 3 months ago

The submission is vague. The code block provided is just all of the lines of the pyth oracle file (the guidelines say not to do this), and it says that a 20% price difference is at the expense of other traders, without providing any details on how they're affected. There is zero evidence specific to any of the logic of the Perpetual protocol. With Perpetual, traders don't get the loss - the LPs potentially get a loss in the form of extending more credit than they should be, based on the borrowing rate configuration. Traders trading against eachother are trading the Eth/USD price, which has nothing to do with the USDC peg. I can write a bot that does the exact same submission any time it sees a file with pyth in the name. The rules say that vague submissions should not be rewarded, for this very reason.

WangSecurity commented 3 months ago

At this point I agree that this report is indeed insufficient to be considered a valid duplicate cause the issue is, in fact, quite complex. If it was something at least similar to what I described above, then I would judge it as a valid duplicate.

Hence, planning to accept the escalation and invalidate the issue.

IllIllI000 commented 3 months ago

@WangSecurity do you mean accept and invalidate?

gstoyanovbg commented 3 months ago

@WangSecurity I don't see where the complexity comes from, this is pretty straightforward. But ok make them happy.

Regardless of the decision for this report, there is a second valid attack path contrary to the claims of @IllIllI000 and @Oot2k .

P.s IllIllI000 feel free to add this template to your bot, doesn't need to credit me.

WangSecurity commented 3 months ago

@IllIllI000 thank you for noticing

@gstoyanovbg I don't disagree that there are different attack paths, but what you say in the report is invalid based on the comment here.

You say:

For example, in the event of a 20% depeg of USDT/USDC, a user can deposit a certain amount of USDT/USDC and open a position that is 20% larger than they could if the real price were used

That is quite similar to what I've wrote above, but as shown in the linked comment, it's incorrect. Even if the depeg is 20% and USDC is $0.8, they still need to deposit 2200 USDC to long 1 ETH (at price of $2000) and when the depeg is back, they withdraw the very same 2200 USDC.

Evert0x commented 3 months ago

Result: Invalid Unique

sherlock-admin4 commented 3 months ago

Escalations have been resolved successfully!

Escalation status: