sherlock-audit/2023-04-blueberry-judging

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

Opened this issue · 7 comments

sherlock-admin commented

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 commented

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.

  • 👍1
sherlock-admin commented

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 commented

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

  • 👍1
hrishibhat commented

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 commented

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.
IAm0x52 commented

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