-
Notifications
You must be signed in to change notification settings - Fork 1
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
sakshamguruji - Exchange Rate Has Been Hardcoded For Assets #120
Comments
Escalate, |
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. |
The above comment is absolutely incorrect , this finding shows how the rebase can change for rebasing tokens and the impact is is correctly explained as users can claim 1 CT and 1 DS for cheaper than expected and redeem them later for PA. , the sponsor had accepted this issue in my Private thread and acknowledged this will be fixed cause this is indeed an issue , I can provide relevant screenshots but I dont think they are necessary , moreover it is mentioned #235 is unrelated and in the original one i.e. #235 the sponsor has mentioned that that's intended . Therefore the escalation is incorrect |
@SakshamGuruji3 did you get this line from Lido docs or Cork docs, cause I'm not sure I understand where you've got this:
Secondly, could you make a more detailed scenario how users could claim CT and DS for cheaper? |
@WangSecurity yeah that portion is from the notion file , they mean that stEth for example is trading currently at 1.17 eth , but what I explained is that the rebase might change and it's natural for the rebase to change. For the second part , the meaning of the exchange rate is -> if exchange rate is 1.17 -> 1.17 eth = 1 cover token + 1 depeg swap (you can see this from the same notion file) Now the rebase has changed , assume its 1.5 (just for example) , since the exchange rate is same , users can still deposit 1.17 eth to receive 1 Ct and 1 Ds while it should be 1.5 eth , Plus I think this issue is different from the dupes as they talk about accounting errors |
It is not natural to rebase, this will happen if the currencies depeg. If we have 1 stETH and 1 ETH and the price of stETH falls to 0.9 ETH, then the protocol should change the rate again. The whole purpose of the protocol is to provide insurance in the case of a depeg event. I am saying again that this issue has presented the main idea of the protocol as a vulnerability. |
The above comment is again incorrect , wstETH is worth 1.17 ETH today, it might be worth 1.5 ETH in the future, depending on how much staking rewards accrue and how the protocol performs , it doesnt need to be a depeg , the issue correctly describes how in such events when exchange rate is way off/above the hardcoded rate the protocol still accounts for 1.17eth for 1 CT and 1DS , what this means is that I as a user got the insurance to depegs at a cheaper rate than what it should be , moreover the sponsor (ik sponsor comments are not relevant but mentioning since it confirms it is not "intended" or "how the protocol operates") confirmed that this hardcoded exchange rate would be replaced subsequently , it was done so to get a feel of the codebase , so the statement "issue has presented the main idea of the protocol as a vulnerability" is incorrect |
Let's go over the information presented in the report.
Rebasing tokens increase the user balance, e.g. if you have 1 stETH which is LIDO rebasing token, then a positive rebase happens you will have for example 1.1 stETH in your account. However, you can still buy 1 stETH for 1 ETH at exchanges considering that no depegs happen. Let's consider there are no fees, users will be able to redeem with CT and DS 1 stETH if they deposited 1 ETH. It is true that shouldn't be the case because if the user held the stETH in his wallet, he would have received the rewards directly and would have 1.1 stETH for example. Why he won't receive that amount if he redeems his CT and DS is because the internal accounting of the protocol doesn't track the rebasing rewards - the PA balance is not increased or decreased. Now let's consider that stETH depegs from ETH the initial exchange rate was 1. Now 1 stETH is worth 0.9 ETH, if we decrease the exchange rate let's say to 0.9 so that users receive more CT and DS tokens when depositing RA, given now ETH(RA) is worth more when users decide to redeem they will receive more tokens than they should, if they call the redeemRaWithCtDs. If the exchange rate is now 0.9 a user that deposited 1 RA for 1 CT and 1 DS when the exchange rate was 1 will receive 0.9 RA back. This shouldn't be the case as the price of the PA(stEHT) dropped, not the price of the RA. If the exchange rate is not changed when a depeg occurs the user can redeem 1 DS and 1 PA(which is now worth 0.9RA) for 1 RA via the redeemWithDs() function, which is the whole idea of the protocol. Given the fact that the DS tokens are used as insurance in case a depeg happens. This is the main idea of the protocol. If the protocol starts to play with the exchange rate for a DS and CT tokens that expire together the whole protocol will become useless. Now let's consider the argument you have mainly introduced in the comments. wstETH is not a rebasing token and users can mint it by depositing stETH token not ETH token. There is no pre-existing peg between these tokens. It is pretty much the same as saying we can use ETH and BTC, but there is no peg between them either. The whole idea of the protocol is to provide insurance in a scenario where tokens depeg, but the tokens that are used first have to have a peg between them. ETH and stETH are a valid pair, USDC and USDT are as well, and so on. For your last argument, comments from sponsors in PT have never been considered as evidence in Sherlock. |
Your first paragraph is just explaining the whole protocol so I wont touch that (dont know why you recited the how the protocol works) , the need for an exchange rate was for the simple reason that Exchange Rate determines how many Redemption Assets you need to deposit to receive 1 Cover Token + 1 Depeg Swap , in the original example it is 1.17 (example of steeth which I incorrectly wrote wsteeth in the reply above , it means it trades at 1.17 eth per wsteth ) , the basic idea is that the rebase is not meant to remain the same . On your second point -> "There is no pre-existing peg between these tokens." , this is what is hardcoded and is problematic since now it might trade 1.5eth per wstEth , this means for insurance provided in the cork protocol you should be depositing 1.5 eth worth of deposit now , me saying I have confirmed this from sponsor is not for proof of this issue but for the fact that the understanding is correct and it is not "intended" which you are implying |
My first paragraph explains why the exchange rate shouldn't be changed because it will lead to other problems. In my second paragraph, I have explained that it is true the readme says that the protocol is expected to work with a lot of tokens but it doesn't mean that pairs will be created randomly. I will refrain from further comments unless additional feedback is requested from the HoJ. |
Agree with this comment. Consider that redemption asset (ra) = eth and pegged asset (pa) = wsteth (documentation example - "Dealing with non-rebasing Pegged Assets"), yielding
If we were to use the actual exchange rate, the protocol would lose its purpose, because the second equation would become |
The claim that this bug presents the core idea of the protocol as a vulnerability is not the case. The bug highlights a specific flaw in handling rebasing tokens, not in the protocol’s entire design , and the above examples have been taken in cases of depegs which is not what this report talks about(it's about rebasing tokens accruing value). Moreover , the statement presented in the comment Rebasing tokens accrue value due to staking rewards or protocol performance, meaning that the exchange rate between a rebasing token and its corresponding asset (e.g., ETH) will change over time. In contrast to the above example which is not what this issue talks about lets take the relevant example , the exchange rate is fixed at the time of deployment, which means that if the rebase increased, for example, from 1.17 ETH to 1.4 ETH, you would still get the Depeg Swap (DS) and Cover Token (CT) for only 1.17 ETH worth of wstETH, meaning you got it for cheaper. This basically acts as a safe hedge for the user because if the peg falls to 0.8 ETH, you will still receive your Redemption Asset (ETH) without any loss. Plus, you paid less for the insurance than what it should have been (1.4 ETH in this case). Users can effectively purchase insurance (CT + DS) cheaper than they should because the exchange rate remains fixed, regardless of the value fluctuations of rebasing tokens. |
Rebasing tokens should not use the exchange rate mechanism. The mechanism is meant for non rebasing tokens, as explained in the example in the docs. The way non rebasing tokens are supposed to be used with exchange rate is this example. It was proven that the exchange rate needs to be fixed, or it does not work.
There is no need to repeat the same wrong example, it works correctly as explained here. If wsteth price goes up instead of down, users do not get anything for cheap, they would deposit 1.17 eth, get 1 ct + 1 ds and then correctly never profit
The mechanism is implemented to profit on depegs, if the price goes up it never is profitable, there is no buying for cheap. The only option is if there was a depeg and then the price comes back up, users can repurchase 1.17 eth for 1 ds + 1 pa (wsteth) until there is no more pa, but this is intended. |
There is some confusion in the discussion and the report (at least for me) around wstETH and stETH, that I'd like to clear:
You can see it in Lido docs. But, the report firstly says wstETH is 1.17 ETH and then claims that stETH is 1.17 ETH which is incorrect and stETH is ~1 ETH. Hence, even if there's a rebasing event on stETH, it's still supposed to hold the ETH peg (as also said in Lido docs). Thus, the scenario in the report won't happen (or the "loss" would be very small in this case). Please correct me if I missed anything, but the decision for now is to accept the escalation, invalidate and de-dup the issue. |
@WangSecurity about steeth and wsteeth I typoed in the replies but I'll try to explain the report better , here is the explanation why I think hardcoding Exchange rate is a bad idea -> Protocol's hardcoded ER: 1 wstETH = 1.17 ETH Some time later: Protocol's hardcoded ER: Still 1 wstETH = 1.17 ETH (unchanged) Bob's Actions: Bob deposits 1.17 ETH into the protocol. Bob's net position: Spent 1.25 ETH, received 1.17 ETH. Net loss: 0.08 ETH. This is not intended imo and Exchange rate should be updatable according to the market conditions |
@SakshamGuruji3 that is exactly what is supposed to happen. It is profitable when wsteth decreases in value. If it increases it is not. This is explained in the docs |
@0xsimao Can you link it up where it says that the above loss is justifiable? |
From the docs it says it is meant for users to profit on depeg events
If it goes up it is not an issue - users are not supposed to redeem ds and pa (wsteth) for eth when the price goes up. Basically the exchange rate acts as the baseline such that users profit whenever the price drops below it. |
That's not a justifiable answer and the discussion would go in circles , the above scenario is a perfectly valid one since user would be redeeming with DS + wsteth , I would leave the discussion to wang now |
If the scenario presented was profitable it would make no sense. Users need to pick a side, it's not possible to profit if the price goes both up or down. If they profit from a price decrease, it means they take a loss if the price increases. Also, there is no reason they would take the loss, as there is nothing forcing them to redeem ds and pa (wsteth) for a loss. |
The whole idea of the protocol is to provide insurance in the scenario a token depegs. If the protocol starts modifying the exchange rate, or implements an oracle in order to do that automatically, the whole purpose of the protocol is lost. When you get insurance on your home, you don't expect to profit if nothing bad happens and your home value increases. You are insured if something bad happens and you will get your money back, but you have to pay the premium. |
"You are insured if something bad happens and you will get your money back, but you have to pay the premium." -> The scenario where Bob 'loses' 0.08 ETH due to wstETH's value increase isn't a premium (premium in this case would be buying DS) , it's an unintended consequence of the protocol's fixed exchange rate failing to account for yield accrual. This 'loss' doesn't go to the protocol as a premium , Bob's 'loss' when wstETH increases in value isn't an intended feature of the protocol . |
No it's not. It's how the protocol works. And the user does not take this loss, only if he redeems but that's on him. |
The fact that a user can't redeem his deposit without incurring a loss when wstETH increases in value is precisely the issue (why would a user redeem is weird question which I don't think needs a detailed answer), and it's certainly not an intended feature of the protocol. To use the house insurance analogy: if your house value increases, you don't expect your insurance to suddenly cover less than what you initially insured. Similarly, users of this protocol shouldn't find themselves in a position where their 'insurance' (the Depeg Swap) effectively decreases in value simply because the underlying asset has accrued yield. This misalignment between the protocol's fixed rate and the true value of wstETH not only makes the user redeem his deposit for a loss but also risks undermining its effectiveness as a hedge against actual depeg events. I think I have already said enough now and would refrain from any other useless discussion , you can go on and on that this is intended (which is not true) and the answer would remain the same |
The user can always redeem with ds + ct at the same rate, so there is no loss. The mechanism to redeem with ds + pa (wsteth) is an alternative to profit on depeg events. As I explained before, the fact that these assets are likely to go up in price will be priced in the price of DS (the insurance is cheaper). As such, the protocol is working according to design and there is no vulnerability here. The docs confirm this, essentially the risk is lower on such assets
|
My question may be very stupid, but I'm still not sure about one part: |
@WangSecurity we are discussing a wsteth price decrease/increase compared to 1.17 ratio to ETH. The protocol ensures users profit if the price goes below this. |
Hm, thank you for that clarification. I agree with @0xsimao and @AtanasDimulski that it's actually a purpose of the protocol to provide insurance to users in case of depeg. If the asset depends on it, then it's known to create opportunities for users to profit. And if the depeg goes up (as in the report), the report doesn't show any loss, only that you can buy cheaper, which is essentially just an arbitrage opportunity. Moreover, I don't think depegs can go up, though it's possible, but the protocol's goal is to provide insurance in case of the depeg going down, which is covered here. Secondly, as was said by the submitter, the report is about rebasing tokens, specifically stETH, which won't cause any problems since its price is pegged to ETH. Planning to accept the escalation and invalidate this report. |
Result: |
Escalations have been resolved successfully! Escalation status:
|
sakshamguruji
Medium
Exchange Rate Has Been Hardcoded For Assets
Summary
Once the exchange rate for an asset has been set it remains same forever ,but in reality a rebasing token would not remain the same value as compared to the underlying asset i.e. wsteEth is trading at 1.17 ETH right now but it might be trading at 1.4 ETH in future.
Vulnerability Detail
1.) The exchange rate for an asset is set here ->
https://github.com/sherlock-audit/2024-08-cork-protocol/blob/main/Depeg-swap/contracts/core/assets/Asset.sol#L20
2.) But this approach is incorrect , taking the example from their own docs ,
therefore for steETH accounting for the exchange rate , you would need to deposit 1.17 steETH to receive 1 CT and 1 DS.
But the rebase might change and steETH might be trading at 1.4 eth and that would mean the exchange rate is incorrect now , users can still deposit 1.17 to receive 1 Ct and 1 Ds while it should be 1.4 . The cover tokens and depeg swaps will be available for cheap in this case.
Impact
Hardcoding exchange rate is incorrect since rebasing tokens will accrue more value with time and due to this users can claim 1 CT and 1 DS for cheaper than expected and redeem them later for PA.
Code Snippet
https://github.com/sherlock-audit/2024-08-cork-protocol/blob/main/Depeg-swap/contracts/core/assets/Asset.sol#L20
Tool used
Manual Review
Recommendation
The exchange rate should be estimated based on the market conditions not hardcoded.
The text was updated successfully, but these errors were encountered: