0x52 - ShortLongSpell#_withdraw checks slippage limit but never applies it making it useless

[sherlock-admin]

0x52

medium

ShortLongSpell#_withdraw checks slippage limit but never applies it making it useless

Summary

Slippage limits protect the protocol in the event that a malicious user wants to extract value via swaps, this is an important protection in the event that a user finds a way to trick collateral requirements. Currently the sell slippage is checked but never applied so it is useless.

Vulnerability Detail

See summary.

Impact

Slippage limit protections are ineffective for ShortLongSpell

Code Snippet

ShortLongSpell.sol#L160-L20

Tool used

Manual Review

Recommendation

Apply sell slippage after it is checked

[securitygrid]

Escalate for 10 USDC This is not a valid M/H. The toAmont and expectedAmount in the off-chain parameter MegaSwapSellData structure are the real slippage protection parameters. Just like ExactInputParams/ExactOutputParams of uniswapV3 pool.

[sherlock-admin]

Escalate for 10 USDC This is not a valid M/H. The toAmont and expectedAmount in the off-chain parameter MegaSwapSellData structure are the real slippage protection parameters. Just like ExactInputParams/ExactOutputParams of uniswapV3 pool.

You've created a valid escalation for 10 USDC!

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.

[ctf-sec]

I still think this is a valid issue

sellSlippage checked but not applied

https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/spell/ShortLongSpell.sol#L160-L202

   function _withdraw(
        ClosePosParam calldata param,
        Utils.MegaSwapSellData calldata swapData
    ) internal {
        if (param.sellSlippage > bank.config().maxSlippageOfClose())
            revert Errors.RATIO_TOO_HIGH(param.sellSlippage);

it is true that the slippage is checked but not applied

The toAmont and expectedAmount in the off-chain parameter MegaSwapSellData structure are the real slippage protection parameters. Just like ExactInputParams/ExactOutputParams of uniswapV3 pool.

this is true, but the code should still check the slippage based on the received amount instead of off-chain parameter

[hrishibhat]

Escalation rejected

Valid high Agree with the Lead judge comment. Slippage must be checked on code whenever possible instead of an off-chain parameter.

[sherlock-admin]

Escalation rejected

Valid high Agree with the Lead judge comment. Slippage must be checked on code whenever possible instead of an off-chain parameter.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.

[Gornutz]

https://github.com/Blueberryfi/blueberry-core/commit/77aa3248e7ac3d9f5787e9f4f9e0afd2a53b90c5

[IAm0x52]

Seems like slippage validation has been removed completely. This is an appropriate solution as long as intentional removed. Waiting for clarification.