-
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
StraawHaat - Lack of Slippage Protection for Reserve during swaps #97
Comments
The slippage protection for user is implemented(will provide the link later), and we need to fix the base slippage protection for the protocol(may also make sense to cancel the trade from the protocol if's below slippage protection) |
Escalate According to readme ->
|
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. |
I believe if this is considered to be valid, it should be dupped with #66, as it is about slippage protection. |
The readme message says 'which is not an issue right now', but this is an issue right now, so it does not apply. Additionally, it is a dup of #66 because it is the same root cause (no slippage protection) and conceptual mistake. |
The escalation is invalid. This is not a future integration issue; it is an issue now. Regarding the comments, this is not a duplicate of #66. Absolutely everything is different.
One of the most important differences is that this issue has slippage protection for the user but not for reserve. Meanwhile, #66 is about the user's slippage. At least when you think it is a duplicate, provide quotes or show the rules on which points it should be duplicated. Let's look at them: https://docs.sherlock.xyz/audits/judging/judging#ix.-duplication-rules IX. Duplication rules: The duplication rules assume we have a "target issue", and the "potential duplicate" of that issue needs to meet the following requirements to be considered a duplicate.
Only when the "potential duplicate" meets all four requirements will the "potential duplicate" be duplicated with the "target issue", and all duplicates will be awarded the highest severity identified among the duplicates. Otherwise, if the "potential duplicate" doesn't meet all requirements, the "potential duplicate" will not be duplicated but could still be judged any other way (solo, a duplicate of another issue, invalid, or any other severity) Let's start with the first point.
It can be seen that the common is severity. Are we duplicating all the issues by severity. No. Moving on to the last part of the duplication rule: Root cause groupings Here, we should not enter this category at all because the root cause is different. This is not about the user but about the reserve. There is already a user slippage implemented. However, to avoid future disputes, I will also explain why it is not a duplicate under this rule. Let's see the terms:
That means it does not differ on just one point, as noted, but on all points. These are the reasons why this issue is not a duplicate of the others. |
It swaps and sets a slippage of 0, it's the same issue.
Based on this logic, if they had 1 contract for each uniswap, curve and so on pools, with different function names, we would have an infinite amount of issues.
So if we had 5 different roles, it would be 5 different issues.
The fix is comparing to a twap or external oracle, the same for all. |
After working on this and using above scenario, one thing that i realized is that it's impractical to have slippage protection for reserve since :
conclusion :
|
I think when it comes to slippage protection rules are pretty clear: Root cause groupings If the following issues appear in multiple places, even in different contracts. In that case, they may be considered to have the same root cause. Issues with the same logic mistake.
Issues with the same conceptual mistake.
Issues in the category
|
IIUC, this is indeed an issue right now, rather than a threat to external integrations. Hence, I agree that it's valid. Secondly, I agree that this should be a duplicate of #66:
Planning to reject the escalation since it's incorrect, but duplicate this with #66. |
Result: |
Escalations have been resolved successfully! Escalation status:
|
StraawHaat
High
Lack of Slippage Protection for Reserve during swaps
Summary
The
swapRaforDs()
function allows users to swap Redemption Asset (RA) for Depeg Swap (DS) tokens:https://github.com/sherlock-audit/2024-08-cork-protocol/blob/main/Depeg-swap/contracts/core/flash-swaps/FlashSwapRouter.sol#L98-L138
swapRaforDs()
->_swapRaforDs()
:If a user wants to have slippage protection, he can set it as a parameter, and at the end of the function, we have a check.
https://github.com/sherlock-audit/2024-08-cork-protocol/blob/main/Depeg-swap/contracts/core/flash-swaps/FlashSwapRouter.sol#L124
However, when the protocol executes the swap for its reserve, there is no slippage protection in place, exposing the protocol to unfavorable market conditions. Specifically, the reserve sells DS tokens without ensuring a minimum acceptable amount of RA in return, potentially resulting in significant financial losses for the protocol.
In this line, the reserve executes the swap without any slippage protection (the slippage is set to
0
), meaning the reserve will accept any amount of RA in return for the DS tokens it sells. This makes the protocol vulnerable to market volatility or manipulation.There are three variants here:
Root Cause
The lack of slippage protection when the reserve sells its DS tokens in the
_swapRaforDs()
function:Internal pre-conditions
Мalicious or normal users need to set
amountOutMin
to 0. In the Attack Path section, I have explained both variants.swapRaforDs()
with slippage protection, the loss to the protocol is minimal - Low Severity.swapRaforDs()
without slippage protection, the loss to the protocol is significant - High Severity.External pre-conditions
No external pre-conditions
Attack Path
Normal Attack Path
swapRaforDs()
function to swap RA for DS tokens and does not want slippage protection for himself._swapRaforDs()
function._swapRaforDs()
, the protocol attempts to sell DS tokens from the reserve using the following code:Self-sandwich Attack Path
In this scenario, the attacker uses two accounts (let's call them Account A and Account B) to manipulate the protocol’s lack of slippage protection for personal profit.
swapRaforDs()
with zero slippage protection to exploit the price difference caused by Account A.Impact
The protocol suffers losses when the reserve sells DS tokens without any slippage protection. In case of market volatility or manipulation, the protocol will receive less RA in return than the actual value of the DS tokens being sold.
PoC
No response
Mitigation
Introduce Slippage Protection for the Reserve. If a user decides they don't need slippage protection, then you can set a standard base percentage to protect the protocol.
Duplicate of #66
The text was updated successfully, but these errors were encountered: