-
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 - Attacker Can Decide The Initialization Ratio Of The AMM Pair #186
Comments
This is valid. will fix |
Actually, this is invalid. The condition for this to happen is for the pool to be empty. But it never happens because the pool is only created when the new DS tokens are issued. |
Escalate I escalate on behalf of @0xNirix |
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. |
On first issuance of DS token for a RA/PA, the protocol creates the pair but does not add liquidity as it returns due to function onNewIssuance(
State storage self,
uint256 prevDsId,
IDsFlashSwapCore flashSwapRouter,
IUniswapV2Router02 ammRouter
) external {
// do nothing at first issuance
if (prevDsId == 0) {
return;
}
if (!self.vault.lpLiquidated.get(prevDsId)) {
_liquidatedLp(self, prevDsId, ammRouter, flashSwapRouter);
}
__provideAmmLiquidityFromPool(self, flashSwapRouter, self.ds[self.globalAssetIdx].ct, ammRouter); //@audit liquidity is only added if this is not the first issuance
}
Therefore the following attack is indeed possible:
The first issuance condition is mentioned in the issue #169 which is duped to this issue. |
This issue is invalid because on the first issuance, the admin may add liquidity itself right after deploying if this is a concern. |
Admin will have to perform multiple steps (get CT, add liquidity with CT/ RA) and can still be front-run by an attacker. |
Yes but according to the admin rule that is irrelevant. |
I thought admin rule says admin is trusted to make the right calls/decisions, but here due to a code deficiency admin cannot guarantee prevention of this attack. |
With the current code implementation, it is clear that the pool is expected to be empty when the first deposit occurs. I do not think that the admin rule is relevant here. |
He can send the 2 transactions atomically with a smart contract or use flashbots via private rpc. Issuing new ds tokens (which create the pools) is admin permissioned, so he can create a new issuance and deploy liquidity without attacker interference using the 2 methods mentioned above. The expected outcome is the initial price to be the one set by the admin. To ensure this happens the admin has to make a small deposit, which he will as he is trusted. |
The expected outcome is mentioned in the code - the source of truth.
Hence this issue is valid. |
That is in the context of that function, but there is more outside of it. |
I disagree the admin rules can be applied here. The admin has to just deploy the contracts and there are no mentions if the admin will use bundled transactions, flashbots or fund the pool themselves. Hence, it's completely viable way to only deploy a pool without any additional transactions and wouldn't be a mistake on the admin's end. But, another question I've got is, is there a way to mitigate this issue in real-time, i.e. can the protocol just set a different pool for the same tokens or anything else? |
No, it would have to be redeployed, as it maps to a certain ra,pa pair. The contract has no funds yet so this would not be a problem anyway. Additionally, if the attacker sets a different uni v2 price, users could arbitrage on this pool by depositing and redeeming in the PSM, which uses the correct exchange rate. So the price would naturally go into the correct one due to arbitrage. Assume attacker sets pool at 2 ra for each 1 ct, when it should be 1:1. Deposit in the psm gives 1 ct and ds for each 1 ra. Redeem takes 1 ct, 1 ds and gives 1 ra. So users would deposit 1 ra in the psm to get 1 ds + 1 ct, and trade this ct in the pool, getting more ra, and repeating, until the pool is balanced. |
The attacker does not need to update the price right after the pool was deployed, it is need to be done just before the first deposit. I have provided the following scenario in #168
As it can be seen redeployment does not mitigates the issue. |
This boils down to the admin allowing users depositing in the vault, which deposits into a uni v2 pool with 0 liquidity, which is vulnerable to such attacks - lack of liquidity. This attack is not a problem of the initial price, but most importantly a low liquidity issue, as it can be performed whenever there is low liquidity. Thus it constitutes a systemic risk of using a uni v2 pool. As such, the admin is responsible for preventing it, as it is a general risk of the protocol. It can never be mitigated unless the admin ensures a minimum liquidity in some way. Even if the pool is enforced to have an initial price of the exchange rate, nothing guarantees that the liquidity reserves stay high. If they decrease, the attack can be performed again. Hence, the only way to guarantee this never happens is not to enforce the initial price, but to ensure the pool has enough liquidity, and this is the job of the admin. |
As @WangSecurity stated in previous comments, admin rule is not applicable here. We have a code which is designed to expect that the pool will be empty when first deposit happens and potential attacker can take advantage of it. |
The user is depositing to the vault without control over the amount of ra and ct it will get, which means it is a slippage issue. By placing a slippage check this issue disappears. Even if you set the initial ratio of the amm, an attacker can always manipulate the price of the pool. |
@0xsimao by saying this is a slippage issue, do you mean that the problem here is users depositing at this skewed rate (10 RA : 1 CT)? |
Yes |
Hmm, but I actually see how the problem here is not just no slippage protection. Even if such a situation happens, and the user deposits 1M Ra and 100k CT with the 10:1 exchange rate, they firstly don't lose anything in value after the deposit, and after withdrawing (before the swap), they don't lose anything. So, it's not about no slippage protection on the deposit cause adding it wouldn't fix anything. About the POC:
It may be that the price wouldn't be 300k RA: 300k CT or that the attacker would've got 700k RA.
Hence, my decision to reject the escalation and leave the issue as it is remains. |
It is a slippage issue because the user should not deposit if the exchange rate is so bad (10:1), unless it is frontrunned and the exchange rate gets worse. If a user deposits with an exchange rate 10:1, most of the funds will be arbitraged away. The fix is the user setting slippage so the price stays near the exchange rate. |
@WangSecurity response to your queries regarding POC:
Yes, like I mentioned these are approximate but roughly correct numbers but omitted some details for brevity. To arrive at above number I used uniswap v2 swap formula. Here is the breakdown:
Arbitrage is only expected if it would profit the arbitrageurs. The attacker can set the price by using a very small amount e.g. .01 RA and 0.001 CT and we should not expect someone to be able to make any profit off that.
The protocol deposits to pool automatically when a user deposits to its liquidity vault (LV). For this LV vault, the user is concerned only about the RA to LV token exchange rate. The price of RA to LV is determined by a separate exchange rate which is refreshed periodically at protocol determined expiration time. So user will not know about this liquidity drain impact till the expiration time (which can be anything e.g. after several days). This LV functionality where user is depositing RA, has pool deposits as an internal mechanism. |
But, the depositor doesn't lose in value just by depositing. Their value before and after the deposit is the same, and they didn't lose anything. Even if there's slippage protection, they deposit without any loss, and a swap to "drain liquidity" is made after the deposit has occurred, and the slippage check wouldn't catch it. About the calculations, thank you very much for elaborating, I appreciate it. But wouldn't this be mitigated in a way expressed here. Even if the RA/CT exchange rate is 10, the users still get 1 CT + 1 DS for 1 RA and can arbitrage the skewed exchange rate themselves, isn't it correct? |
@WangSecurity the slippage catches it because if the depositor specifies a min or max exchange rate, depositing with a bad exchange rate will never go through. Providing liquidity to pools also has slippage checks. |
Correct @WangSecurity, anyone can arbitrage the exchange rate back, however that should not be considered as mitigation because:
|
I think this issue is valid, and escalation can be accepted. I had originally left this issue valid, and the only reason I invalidated it is that the condition for this to happen is for the pool to be empty. But @0xNirix showed early in the discussion that the pool will always be empty. We even got a comment from the developers that nothing would be done after that: // do nothing at first issuance
if (prevDsId == 0) {
return;
} Arguments that the admin can prevent this attack are invalid. A user can front-run his transaction and do the attack before the second function call. And what would be the logic of developers putting Also, the arguments that it is a duplicate of the slippage group are invalid. You can see the duplication rules: https://docs.sherlock.xyz/audits/real-time-judging/judging#ix.-duplication-rules Even if the root-cause is the same, the attack path is different: |
I agree with @cvetanovv, and I believe he explained very well why this issue has to be valid. Planning to accept the escalation and validate the family with medium severity. Plan to apply this decision in a couple of hours. |
@WangSecurity what do you think of this? It is clearly a slippage issue |
As I've said a couple of times earlier, this is not a slippage-related issue, and I stand by my previous arguments about this. The decision remains as in my previous message: accept the escalation and validate with medium severity. Planning to apply this decision in a couple of hours. |
The issue only happens because someone frontruns the user and sets a different exchange rate. If the user deposits with a bad exchange rate, it's their fault. It's exactly like providing liquidity to a unsiwap pool, users also set minimum amounts A and B, so they can control the ratio. If slippage is added then users always deposit with a favorable exchange rate and this is issue is fixed. And slippage is mentioned directly in the rules for groups and the issue can be fixed by adding slippage protection
|
Front-running is not required here.
I believe the point 2 in this comment explains well, why it's not relevant here.
What matters here is the RA to LV exchange rate, and it would be correct. So if we implement slippage protection for this, it won't fix the issue. Moreover, even if considered a slippage-related issue, this family deserves to be separated based on the following:
The decision remains, accept the escalation and validate with medium severity, the decision will be applied tomorrow 10 AM UTC. |
It is for their profit, they will do it.
Again, it was their choice to deposit with a bad exchange rate. Unless they are frontrunned. Obviously users do not deposit blindly, they should set an exchange rate limit, but they do not, as it is a slippage issue. This is like saying depositing in a uniswap pool should not have slippage control, which makes no sense.
It fixes it because users would never deposit with a bad exchange rate, it would be a mistake.
But everything is the same, slippage when interacting with a uniswap pool. The bug is here: (,, uint256 lp) = ammRouter.addLiquidity(
token0, token1, token0Amount, token1Amount, token0Tolerance, token1Tolerance, address(this), block.timestamp
); The tolerances are incorrectly calculated on chain, they should be passed as arguments. Then it would be fixed. |
The exchange rate remains before and after the deposit. Even if you set an exchange rate limit it doesn't matter here because it doesn't change and that slippage check would be satisfied.
The RA:LV exchange would be correct, and it wouldn't be a mistake to deposit at the correct RA:LV exchange rate. The problem is in the RA:CT exchange rate and setting slippage protection to RA:LV exchange rate doesn't fix the problem of the RA:CT exchange rate. My decision to accept the escalation and validate with medium severity remains and it's the final decision. Planning to apply it tomorrow after 10 AM UTC. |
Result: |
Escalations have been resolved successfully! Escalation status:
|
sakshamguruji
Medium
Attacker Can Decide The Initialization Ratio Of The AMM Pair
Summary
When the RA/CT is deposited in the AMM pool it must follow a pre defined ratio , but the attacker can see the pair has been deployed on uniV2 and make the first deposit (adding liquidity) and distrupt the intended ratio of the CT/RA .
Vulnerability Detail
1.) When depositing , the vault decides the ratio of the RA/CT to be deposited in the AMM pool ->
https://github.com/sherlock-audit/2024-08-cork-protocol/blob/main/Depeg-swap/contracts/libraries/VaultLib.sol#L204
If the pool is empty (it's the first liquidity addition) the ratio should follow the default ratio decided at development.
2.) But an attacker can frontrun this first deposit in the vault which adds liquidity to the AMM , and manually call _addLiquidity in the UniV2 pool which calls->
https://github.com/Uniswap/v2-periphery/blob/master/contracts/UniswapV2Router02.sol#L33
and since the pool is empty the attacker can decide the initial ratio of the pool and distrupt the ratio (can be as extreme as possible) and the amount of subsequent deposits of CT/RA in the AMM would follow this initial ratio.
Impact
The attacker can decide the initial ratio of the AMM pool and distrupt subsequent CT/RA deposits in the pool and incorrect DS mints, offload that initialization on the config contract so that only the config contract owner can initialize the vault.
Code Snippet
https://github.com/sherlock-audit/2024-08-cork-protocol/blob/main/Depeg-swap/contracts/libraries/VaultLib.sol#L204
Tool used
Manual Review
Recommendation
offload that initialization on the config contract so that only the config contract owner can initialize the vault.
The text was updated successfully, but these errors were encountered: