sakshamguruji - Attacker Can Decide The Initialization Ratio Of The AMM Pair

[sherlock-admin4]

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

 function __provideLiquidityWithRatio(
        State storage self,
        uint256 amount,
        IDsFlashSwapCore flashSwapRouter,
        address ctAddress,
        IUniswapV2Router02 ammRouter
    ) internal returns (uint256 ra, uint256 ct) {
        uint256 dsId = self.globalAssetIdx;

        uint256 ctRatio = __getAmmCtPriceRatio(self, flashSwapRouter, dsId);

        (ra, ct) = MathHelper.calculateProvideLiquidityAmountBasedOnCtPrice(amount, ctRatio);

        __provideLiquidity(self, ra, ct, flashSwapRouter, ctAddress, ammRouter, dsId);
    }

function __getAmmCtPriceRatio(State storage self, IDsFlashSwapCore flashSwapRouter, uint256 dsId)
        internal
        view
        returns (uint256 ratio)
    {
        // This basically means that if the reserve is empty, then we use the default ratio supplied at deployment
        ratio = self.ds[dsId].exchangeRate() - self.vault.initialDsPrice;

        // will always fail for the first deposit
        try flashSwapRouter.getCurrentPriceRatio(self.info.toId(), dsId) returns (uint256, uint256 _ctRatio) {
            ratio = _ctRatio;
        } catch {}
    }

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.

[ziankork]

This is valid. will fix

[cvetanovv]

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.

[cvetanovv]

Escalate

I escalate on behalf of @0xNirix

[sherlock-admin3]

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.

[0xNirix]

https://github.com/sherlock-audit/2024-08-cork-protocol/blob/db23bf67e45781b00ee6de5f6f23e621af16bd7e/Depeg-swap/contracts/libraries/VaultLib.sol#L92-L109

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 if (prevDsId == 0) condition. This means the pool is empty and open for the initialization attack by a third party.

    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:

1. Immediately after first DS issuance of a new RA/PA, Attacker calls deposit function in the PSM, providing RA.
2. Attacker receives equivalent amounts of CT and DS based on the correct exchangeRate.
3. Attacker immediately calls the AMM's addLiquidity function, providing CT and RA at an arbitrary ratio, setting an incorrect initial price.
4. Attacker deposits RA into the Liquidity Vault (LV) and/or waits for other users to deposit RA into the LV.
5. The protocol adds liquidity to the AMM using the LV deposit, but at the incorrect price set by the attacker.
6. Attacker arbitrages between the PSM (at the correct rate) and the AMM (at the incorrect rate), extracting value from the system.

The first issuance condition is mentioned in the issue https://github.com/sherlock-audit/2024-08-cork-protocol-judging/issues/169 which is duped to this issue.

[0xsimao]

This issue is invalid because on the first issuance, the admin may add liquidity itself right after deploying if this is a concern.

[0xNirix]

Admin will have to perform multiple steps (get CT, add liquidity with CT/ RA) and can still be front-run by an attacker.

[0xsimao]

Yes but according to the admin rule that is irrelevant.

[0xNirix]

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.

[spdimov]

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.

[0xsimao]

can still be front-run by an attacker.

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.

[0xNirix]

The expected outcome is mentioned in the code - the source of truth.

    // do nothing at first issuance
    if (prevDsId == 0) {
        return;
    }

Hence this issue is valid.

[0xsimao]

That is in the context of that function, but there is more outside of it.

[WangSecurity]

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?

[0xsimao]

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. If the attacker sets 2 ct for each 1 ra, users can get ct cheap, pair with ds in the market and redeem for ra at a better ratio.

[spdimov]

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

1. New issuance has occured with exchange rate between RA:CT+DS = 1.
2. Attacker mints 10 CT + 10 DS by providing 10 RA
3. Another user decide to deposit 50 RA to the LV
4. It is expected that liquidty sent to the AMM would be ~50% RA ~ 50% CT
5. Attacker frontrun the deposit transaction and deposits 1 RA and 0.1 CT
6. Now the price ration between RA:CT would be much higher than expected.
7. Only small amount of the user's 50 RA would be used to mint CT and almost all of it(RA) are going to be provided to the pair
8. New pair reserves are going to be ~45 RA and 4.5 CT
9. Now attacker can provide his CT tokens in order to get more RA than initially invested.

As it can be seen redeployment does not mitigates the issue.

[0xsimao]

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.

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.

[spdimov]

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.

[0xsimao]

Except when it is a systemic risk, which is lack of liquidity, as explained here. That is, this is not really fixable without admin intervention. In fact, the root cause of this attack path is not setting slippage protection. And as explained here, the initial price being different is not a problem.

[siddhpurakaran]

Can you provide more details about mitigation of this issue

[WangSecurity]

What I don't understand for now is who loses here. I agree with @0xsimao that this path is about slippage protection and not the issue we discuss here. This issue indeed opens up an arbitrage opportunity, but that's it. The report doesn't show who would lose in this scenario or how the protocol is rendered useless (the price can be arbitraged to the correct one). Hence, this has to remain invalid, planning to reject the escalation.

[0xNirix]

@WangSecurity, you are absolutely correct that the main issue in this finding is not about slippage protection. The protocol's reliance on the AMM price of RA/CT, which it expects to be fair due to market dynamics, is generally acceptable.

However, this vulnerability allows the initial liquidity provider to set an incorrect price deliberately and maliciously. The protocol suffers losses because it will rely on this incorrect price to provide liquidity, effectively selling a token (RA) at less than fair market price, which the attacker can exploit.

Let's examine the evidence that the protocol fails to add liquidity at the intended price in the code:

1. It is on the first issuance that the protocol fails to provide the initial liquidity (that we have already agreed in this thread but just relisting)

// MUST be called on every new DS issuance
    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 in all other cases the new issuance adds the liquidity
    }

3. Whenever protocol does provide liquidity, the protocol either relies on the real-time ratio or uses a fixed ratio:

   function __provideAmmLiquidityFromPool(
       State storage self,
       IDsFlashSwapCore flashSwapRouter,
       address ctAddress,
       IUniswapV2Router02 ammRouter
   ) internal {
       uint256 dsId = self.globalAssetIdx;
   
       uint256 ctRatio = __getAmmCtPriceRatio(self, flashSwapRouter, dsId);
   
       (uint256 ra, uint256 ct) = self.vault.pool.rationedToAmm(ctRatio);
   
       __provideLiquidity(self, ra, ct, flashSwapRouter, ctAddress, ammRouter, dsId);
   
       self.vault.pool.resetAmmPool();
   }

   In the __getAmmCtPriceRatio method

   function __getAmmCtPriceRatio(State storage self, IDsFlashSwapCore flashSwapRouter, uint256 dsId)
       internal
       view
       returns (uint256 ratio)
   {
       // This basically means that if the reserve is empty, then we use the default ratio supplied at deployment
       ratio = self.ds[dsId].exchangeRate() - self.vault.initialDsPrice;
   
       // will always fail for the first deposit
       try flashSwapRouter.getCurrentPriceRatio(self.info.toId(), dsId) returns (uint256, uint256 _ctRatio) {
           ratio = _ctRatio;
       } catch {}
   }

   In the code, we can clearly see that the intention is to use an initial default ratio if the AMM cannot be used to determine the ratio. However, there is a failure to enforce this for the first issuance.

The consequences of this vulnerability are:

1. An attacker can set an artificially low price for RA.
2. The protocol will then use this incorrect price to provide liquidity, effectively selling RA at below market value.
3. The attacker can exploit this mispricing for immediate profit.

While it's true that arbitrage will eventually correct the price, the protocol will have already suffered losses by selling assets at an unfair price. This is not a normal market dynamic but a direct result of the protocol's failure to secure the initial liquidity provision at the intended price.

[0xsimao]

The protocol will then use this incorrect price to provide liquidity, effectively selling RA at below market value.

This is a slippage protection issue, same attack path as described here.

[SakshamGuruji3]

This should not be under the same slippage issue since the root cause is different , see here for reference how this issue was not duped with other https://github.com/sherlock-audit/2024-08-cork-protocol-judging/issues/166 specially the reasoning by HoJ

[0xsimao]

The root cause is the provide liquidity function not having slippage. This is just an attack path that allows the ratio of the pool to be more easily manipulated, but not the root cause.

[0xNirix]

Will try to reiterate - not a slippage protection issue.

As detailed in the comment https://github.com/sherlock-audit/2024-08-cork-protocol-judging/issues/186#issuecomment-2401372760 with code evidence: The protocol intended to a) set a fixed ratio initially OR b) use AMM ratio for providing liquidity at any point of time after that. Whether it provides these liquidity with or without slippage protection is irrelevant.

The protocol fails to achieve point a) causing loss to the protocol. The initial price can be set by an attacker and protocol will just use that incorrect AMM ratio as expected price. Again whether it uses a slippage against that expected price is irrelevant.

[0xsimao]

The protocol intended to a) set a fixed ratio initially OR b) use AMM ratio for providing liquidity at any point of time after that.

No, the protocol uses the fixed ratio if the pool has no liquidity. And as explained, having a different initial price does not lead to medium impact by itself alone.

[WangSecurity]

Yeah, that's what I think as well, I see how it allows the attacker to profit, but I don't see how it causes losses to other users. Hence, I need a POC (either coded or written) to prove the loss of funds. If no POC is provided, planning reject the escalation and leave the issue as it is.

[0xNirix]

umm...if someone is profiting, that should inherently mean there is an equivalent loss.

Here is the written POC, highlighting one of the ways this loss will manifest. Please note it is a complex protocol and loss can manifest in few different ways. Also some of the numbers are approximate but should be close enough

1. Attacker's actions:

   • Provides initial liquidity to AMM at manipulated price: 1,000 RA : 100 CT (10 RA = 1 CT)

2. Protocol's actions:

   • A large deposit comes to LV for 1,000,000 RA.
   • Adds liquidity based on manipulated price: 1,000,000 RA : 100,000 CT

3. AMM state after protocol liquidity addition:

   • Pool: 1,000,000 RA : 100,000 CT

4. Attacker's arbitrage:

   • Swaps 200,000 CT for 700,000 RA in AMM (price impact considered)

5. Final AMM state (back to normal price):

   • Pool: 300,000 RA : 300,000 CT

Impact

1. Massive Liquidity Drain:

   • Initial liquidity: 1,000,000 RA : 100,000 CT
   • Final liquidity: 300,000 RA : 300,000 CT
   • Loss: ~70% of RA liquidity

2. LP Token Devaluation:

   • LP tokens now represent a share of a much smaller pool
   • Value per LP token decreased by approximately 70%

Impact translated to LV Holders (other users):

1. Liquidity Liquidation: When DS expires, the _liquidatedLp function is called:

   • AMM liquidation yields only 300,000 RA (instead of 1,000,000 RA)

2. Reserve Calculation: The reserve function in VaultPool is called with drastically reduced amounts:

   • Total LV issued: ~1,000,000 LV (unchanged)
   • Available RA: ~300,000 (from AMM)

3. Rate Calculation per LV:

   • Before attack: ~1 RA per LV (1,000,000 RA for 1,000,000 LV)
   • After attack: ~0.3 RA per LV (300,000 RA for 1,000,000 LV)

4. Impact on Redemptions: When users redeem their LV tokens:

   • For every 1,000 LV redeemed, users receive approximately:
     • 300 RA (instead of 1,000 RA)
   • This represents a 70% loss in value compared to the original deposit

This vulnerability allows an attacker to manipulate the AMM price, forcing the protocol to provide liquidity at an unfavorable rate. The subsequent arbitrage drains significant RA from the AMM pool.

The impact is severe with:

1. Direct value extraction by the attacker
2. Signifcant devaluation of LV tokens, causing loss for all LV holders.

[0xsimao]

This is the slippage issue.

[0xNirix]

Please refer to this comment https://github.com/sherlock-audit/2024-08-cork-protocol-judging/issues/186#issuecomment-2404534568 to understand why this is not a slippage issue.

[0xsimao]

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.

[WangSecurity]

@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)?

[0xsimao]

Yes

[WangSecurity]

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:

1. The following numbers look arbitrary (i.e. there's no explanation how you've received those numbers so it looks like just randomly picked numbers. I'm sorry if that's not the case, but it's hard to say, since there's no explanation how we received them):

   Attacker's arbitrage: Swaps 200,000 CT for 700,000 RA in AMM (price impact considered) Final AMM state (back to normal price): Pool: 300,000 RA : 300,000 CT

It may be that the price wouldn't be 300k RA: 300k CT or that the attacker would've got 700k RA.

2. This situation could've been arbitraged easily before anyone deposited into the pool. The attacker deposited 1000 RA and 100 CT, so anyone seeing they can get 10 RA for 1 CT would instantly do that.
3. The depositor can refrain from depositing into the pool if they see an exchange rate like that and wait for the price to stabilise.

Hence, my decision to reject the escalation and leave the issue as it is remains.

[0xsimao]

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.

[0xNirix]

@WangSecurity response to your queries regarding POC:

The following numbers look arbitrary (i.e. there's no explanation how you've received those numbers so it looks like just randomly picked numbers. I'm sorry if that's not the case, but it's hard to say, since there's no explanation how we received them): Attacker's arbitrage: Swaps 200,000 CT for 700,000 RA in AMM (price impact considered) Final AMM state (back to normal price): Pool: 300,000 RA : 300,000 CT It may be that the price wouldn't be 300k RA: 300k CT or that the attacker would've got 700k RA.

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:

1. Initial state:

   • RA (Reserve A) = 1,000,000
   • CT (Reserve B) = 100,000

2. Constant product (k): k = RA CT = 1,000,000 100,000 = 100,000,000,000

3. The swap: User wants to swap 200,000 CT for RA

4. New CT balance after swap: CT_new = 100,000 + 200,000 = 300,000

5. Calculate new RA balance using the constant product formula: k = RA_new CT_new 100,000,000,000 = RA_new 300,000 RA_new = 100,000,000,000 / 300,000 = 333,333.33 (rounded to 2 decimal places)

6. Amount of RA given to the user: RA_out = 1,000,000 - 333,333.33 = 666,666.67

7. Final pool state: RA remaining = 333,333.33 CT remaining = 300,000

This situation could've been arbitraged easily before anyone deposited into the pool. The attacker deposited 1000 RA and 100 CT, so anyone seeing they can get 10 RA for 1 CT would instantly do that.

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 depositor can refrain from depositing into the pool if they see an exchange rate like that and wait for the price to stabilise.

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.

[WangSecurity]

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

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?

[0xsimao]

@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.

[0xNirix]

Correct @WangSecurity, anyone can arbitrage the exchange rate back, however that should not be considered as mitigation because:

1. As mentioned in the previous comment, it should not be expected that users will arbitrage the rate back unless it is to their profit. 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.
2. The users at loss specifically here are LV depositors, they are looking to earn yield on their RA tokens, they should not be expected to know about an internal RA:CT pool that LV internally deposits into as the pool's current rate does not matter to them in anyway during the deposit.

[cvetanovv]

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 return once they want to call the function again to add liquidity? There is no logic in this, and it confirms that the pool will be empty, and vulnerability is possible.

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: Only when the "potential duplicate" meets all four requirements will the "potential duplicate" be duplicated with the "target issue"

[WangSecurity]

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.

[0xsimao]

@WangSecurity what do you think of this? It is clearly a slippage issue

[WangSecurity]

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.

[0xsimao]

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/sandwich/slippage protection:

Can be fixed by slippage protection;

[WangSecurity]

The issue only happens because someone frontruns the user and sets a different exchange rate

Front-running is not required here.

If the user deposits with a bad exchange rate, it's their fault.

I believe the point 2 in this comment explains well, why it's not relevant here.

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

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 exception to this would be if underlying code implementations OR impact OR the fixes are different, then they may be treated separately.

The decision remains, accept the escalation and validate with medium severity, the decision will be applied tomorrow 10 AM UTC.

[0xsimao]

As mentioned in the previous comment, it should not be expected that users will arbitrage the rate back unless it is to their profit. 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.

It is for their profit, they will do it.

The users at loss specifically here are LV depositors, they are looking to earn yield on their RA tokens, they should not be expected to know about an internal RA:CT pool that LV internally deposits into as the pool's current rate does not matter to them in anyway during the deposit.

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.

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.

It fixes it because users would never deposit with a bad exchange rate, it would be a mistake.

The exception to this would be if underlying code implementations OR impact OR the fixes are different, then they may be treated separately.

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.

[WangSecurity]

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

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.

It fixes it because users would never deposit with a bad exchange rate, it would be a mistake

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.

[WangSecurity]

Result: Medium Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• cvetanovv: rejected