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

Abhan1041 - Lack of slippage protection leads to loss of protocol funds #66

Open
sherlock-admin4 opened this issue Sep 10, 2024 · 3 comments
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A High severity issue. Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin4
Copy link
Contributor

sherlock-admin4 commented Sep 10, 2024

Abhan1041

High

Lack of slippage protection leads to loss of protocol funds

Summary

There is no slippage protection while removing liquidity and swap tokens from AMM.

Vulnerability Detail

There are 2 intances where slippage protection is missing which are as below:

  1. When LV token holder redeem before expiry vaultLib::redeemEarly function is called in which _liquidateLpPartial function and in that _redeemCtDsAndSellExcessCt is called. In _redeemCtDsAndSellExcessCt function CT tokens are swapped for RA tokens in AMM as below:
...
ra += ammRouter.swapExactTokensForTokens(ctSellAmount, 0, path, address(this), block.timestamp)[1];
...

As stated above, swapExactTokensForTokens function's 2nd parameter is 0 which shows that there is no slippage protection for this
swap and also deadline is block.timestamp.

  1. In vaultLib::_liquidateLpPartial function __liquidateUnchecked is called in which liquidity is removed from AMM of RA-CT token pair by burning LP tokens of protocol as below:
...
(raReceived, ctReceived) =
            ammRouter.removeLiquidity(raAddress, ctAddress, lp, 0, 0, address(this), block.timestamp);
...

As stated above, removeLiquidity function's 4th & 5th parameter is 0 which shows that there is no slippage protection for this swap and also deadline is block.timestamp.

In such cases, an attacker can frontrun the transaction by seeing it in the mempool and manipulate the price such that protocol transaction have to bear heavy slippage which will leads to loss of protocol funds.

Also, there is block.timestamp as deadline so malicious node can prevent transaction to execute temporary and execute the transaction when there is high slippage which will also leads to loss of protocol funds.

Impact

Loss of protocol funds which will reduce the yield of users.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

Protocol should implement slippage protection and set deadline while removing liquidity and also swap from AMM.

@github-actions github-actions bot added Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A High severity issue. labels Sep 14, 2024
This was referenced Sep 14, 2024
@ziankork
Copy link
Collaborator

Yes this is a valid issue, we've already fixed this prior to our trading competition except for the deadline vulnerability.

@sherlock-admin3 sherlock-admin3 added Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed labels Sep 23, 2024
@sherlock-admin3 sherlock-admin3 changed the title Acidic Flaxen Donkey - Lack of slippage protection leads to loss of protocol funds Abhan1041 - Lack of slippage protection leads to loss of protocol funds Sep 25, 2024
@sherlock-admin3 sherlock-admin3 added the Reward A payout will be made for this issue label Sep 25, 2024
@v-kirilov
Copy link

#75 is a duplicate of this one!

@sherlock-admin2
Copy link
Contributor

The protocol team fixed this issue in the following PRs/commits:
Cork-Technology/Depeg-swap#76

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A High severity issue. Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

5 participants