y4y - No slippage protection when adding or removing liquidity

[sherlock-admin4]

y4y

high

No slippage protection when adding or removing liquidity

Summary

In the Velodrome strategy, before each withdraw and deposit action, _addLiquidity and _removeLiquidity are called respectively. In those functions, there are no slippage parameters set which makes the entire funds in the strategy face the risk of slippage.

Vulnerability Detail

In general, the strategy first remove all liquidities before actions, and after actions, liquidities are re-added. For example, when an user wants to withdraw his shares from the strategy, he first calls withdraw function in the vault, which vault calls stragtey.beforeAction, and later strategy.withdraw to first withdraw all funds, and transfers user needed amount back to vault, after this, all other left assets are added back to the pool to continue accrue interests.

In _removeLiquidity, for example, we can see when liquidities are removed with parameters:

            decreaseLiquidityParams = INftPositionManager.DecreaseLiquidityParams({
                tokenId: positionMain.nftId,
                liquidity: liquidity,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp
            });

And where amount0Min and amount1Min are set to 0. In most cases, this is alright, as min amount are checked in vault's functions. However, a malicious user can grief this by inputing 1 for min amount because the vault requires min amount needs to be greater than 0. And now, the entire funds in the strategy will no longer have slippage protection. This malicious user can simply deposit 1 wei and withdraw 1 wei at a relatively low cost, when the market is volatile, all liquidity in the strategy will be at risk of encountering slippage and other types of attacks such as sandwich attacks.

Impact

Since the way strategy invests in the pool is by withdrawing all from the pool, and then re-add again, a slippage can potentially cause fund loss for all users who have participated in the strategy. Given that the way of making this work is relatively low, as 1 wei of deposit or withdraw can make this happen, this is a medium or a high severity issue.

Code Snippet

            decreaseLiquidityParams = INftPositionManager.DecreaseLiquidityParams({
                tokenId: positionMain.nftId,
                liquidity: liquidity,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp
            });

Tool used

Manual Review

Recommendation

Add slippage parameters in those liquidity functions, or maybe consider set a fixed range of values as min amount.

Duplicate of #8

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

WildSniper commented:

invalid due to impossibility of such attack with tick lower and upper as params

[MirthFutures]

Escalate As the protocol teams claimed, slippage is surely checked in BeefyVaultConcLiq contract. But the issue here is that, when the strategy handles an action, it withdraws the entire liquidated assets from the pool, does the action, and eventually liquidate with the new amounts again. This mean when a slippage happens, it doesn't only affect one single user's asset, but also all users' assets.

Of course, there is a slippage check in its caller function, either deposit and withdraw has a _minShare or _minAmount0 and _minAmount1 parameter to ensure users get their desired amount. But what if the user intentionally sets a low min amount and let a huge slippage happen?

Suppose a malicious user, who deposit very few amounts in the strategy to make sure the liquidation process goes smoothly, then wait for a moment which can cause some degrees of slippage to assets, then do the withdraw call, but with 1 wei of min amount, to intentionally let the slippage happen. Under the hood, the strategy first withdraws all assets, which shrinks in amount due to slippage, then transfers this malicious user's withdrawal amount, and re-liquidate the rest assets. Normally, this wouldn't be an issue, because it would only harm user's interest, but in this case, since all assets are handled altogether, one user's malicious intention can cause harm to all other user to lose funds.

And for judge's comment that this attack is impossible because of ticks, I believe as long as the assets provided checks out, this malicious user can deposit a minimum amount to carry out this attack. Moreover, even if this attack is not as practical, an user's mistake can also potentially cause fund loss for all users who have enrolled in this strategy, which is obviously an issue.

Lastly, I argue this finding is not a duplicate of #8 as this actually mentions the possibility of a malicious intention can cause harm to all users.

Can you make a POC of this?

[sherlock-admin3]

Escalate As the protocol teams claimed, slippage is surely checked in BeefyVaultConcLiq contract. But the issue here is that, when the strategy handles an action, it withdraws the entire liquidated assets from the pool, does the action, and eventually liquidate with the new amounts again. This mean when a slippage happens, it doesn't only affect one single user's asset, but also all users' assets.

Of course, there is a slippage check in its caller function, either deposit and withdraw has a _minShare or _minAmount0 and _minAmount1 parameter to ensure users get their desired amount. But what if the user intentionally sets a low min amount and let a huge slippage happen?

Suppose a malicious user, who deposit very few amounts in the strategy to make sure the liquidation process goes smoothly, then wait for a moment which can cause some degrees of slippage to assets, then do the withdraw call, but with 1 wei of min amount, to intentionally let the slippage happen. Under the hood, the strategy first withdraws all assets, which shrinks in amount due to slippage, then transfers this malicious user's withdrawal amount, and re-liquidate the rest assets. Normally, this wouldn't be an issue, because it would only harm user's interest, but in this case, since all assets are handled altogether, one user's malicious intention can cause harm to all other user to lose funds.

And for judge's comment that this attack is impossible because of ticks, I believe as long as the assets provided checks out, this malicious user can deposit a minimum amount to carry out this attack. Moreover, even if this attack is not as practical, an user's mistake can also potentially cause fund loss for all users who have enrolled in this strategy, which is obviously an issue.

Lastly, I argue this finding is not a duplicate of https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager-judging/issues/8 as this actually mentions the possibility of a malicious intention can cause harm to all users.

You've deleted an escalation for this issue.

[brandonshiyay]

Okay, it seems I have misread some important lines of code and made a few mistakes, going to withdraw the escalation.