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

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

Closed
sherlock-admin3 opened this issue Mar 18, 2024 · 26 comments
Closed

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

sherlock-admin3 opened this issue Mar 18, 2024 · 26 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Non-Reward This issue will not receive a payout

Comments

@sherlock-admin3
Copy link

sherlock-admin3 commented Mar 18, 2024

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.

@github-actions github-actions bot added Medium A valid Medium severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Mar 24, 2024
@sherlock-admin3
Copy link
Author

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

santipu_ commented:

Medium

@sherlock-admin3 sherlock-admin3 changed the title Generous Smoke Donkey - The protocol uses prices in usd instead of usdt/usdc ge6a - The protocol uses prices in usd instead of usdt/usdc Apr 4, 2024
@sherlock-admin3 sherlock-admin3 added the Reward A payout will be made for this issue label Apr 4, 2024
@Oot2k
Copy link

Oot2k commented Apr 6, 2024

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
Copy link
Contributor

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.

@sherlock-admin3 sherlock-admin3 added the Escalated This issue contains a pending escalation label Apr 6, 2024
@nevillehuang
Copy link
Collaborator

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

@gstoyanovbg
Copy link

gstoyanovbg commented Apr 9, 2024

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
Copy link
Collaborator

@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
Copy link

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

This comment was marked as off-topic.

@WangSecurity
Copy link
Collaborator

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
Copy link

detectiveking123 commented Apr 21, 2024

@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
Copy link
Collaborator

@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
Copy link

Oot2k commented Apr 22, 2024

Agree with @IllIllI000 here.

@WangSecurity
Copy link
Collaborator

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
Copy link
Collaborator

IllIllI000 commented Apr 22, 2024

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
Copy link

Oot2k commented Apr 23, 2024

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
Copy link

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
Copy link
Collaborator

@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
Copy link
Collaborator

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
Copy link

gstoyanovbg commented Apr 23, 2024

@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
Copy link
Collaborator

IllIllI000 commented Apr 23, 2024

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
Copy link
Collaborator

WangSecurity commented Apr 23, 2024

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
Copy link
Collaborator

@WangSecurity do you mean accept and invalidate?

@gstoyanovbg
Copy link

@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
Copy link
Collaborator

@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 Evert0x removed the Medium A valid Medium severity issue label Apr 25, 2024
@sherlock-admin4 sherlock-admin4 added Non-Reward This issue will not receive a payout and removed Reward A payout will be made for this issue labels Apr 25, 2024
@Evert0x
Copy link

Evert0x commented Apr 25, 2024

Result:
Invalid
Unique

@sherlock-admin4
Copy link
Contributor

sherlock-admin4 commented Apr 25, 2024

Escalations have been resolved successfully!

Escalation status:

@sherlock-admin2 sherlock-admin2 removed the Escalated This issue contains a pending escalation label Apr 25, 2024
@sherlock-admin3 sherlock-admin3 added the Escalation Resolved This issue's escalations have been approved/rejected label Apr 25, 2024
@sherlock-admin2 sherlock-admin2 removed the Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label label May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Escalation Resolved This issue's escalations have been approved/rejected Non-Reward This issue will not receive a payout
Projects
None yet
Development

No branches or pull requests

10 participants