In CurveSpell.closePositionFarm, _removeLiquidity lacks slippage protection if isKilled is True

Summary

When [closePositionFarm](https://github.com/sherlock-audit/2023-07-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L168-L174) is called, isKilled will affect the execution logic of [_removeLiquidity](https://github.com/sherlock-audit/2023-07-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L257-L264). If isKilled is set to false, liquidity will be removed via [ICurvePool(pool).remove_liquidity_one_coin](https://github.com/sherlock-audit/2023-07-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L291-L294), whose second parameter is param.amountOutMin [passed from the caller](https://github.com/sherlock-audit/2023-07-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L169), so this is no problem. If isKilled is set to true, the liquidity will be removed via [ICurvePool(pool).remove_liquidity](https://github.com/sherlock-audit/2023-07-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L280), whose second parameter is [an empty array](https://github.com/sherlock-audit/2023-07-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L279) representing the minOut of each token received. In Solidity, uninitialized values are all 0, so this way is vulnerable to sandwich attacks due to the lack of slippage protection.

Vulnerability Detail

closePositionFarm internally calls _removeLiquidity to remove liquidity. Only cases where isKilled is set to true are discussed here.

File: blueberry-core\contracts\spell\CurveSpell.sol
258:     function _removeLiquidity(
259:         ClosePosParam memory param,
260:         bool isKilled,
261:         address pool,
262:         address[] memory tokens,
263:         IBank.Position memory pos,
264:         uint256 amountPosRemove
265:     ) internal {
......
277:         if (isKilled) {
278:             uint256 len = tokens.length;
279:             if (len == 2) {
280:->               uint256[2] memory minOuts;
281:                 ICurvePool(pool).remove_liquidity(amountPosRemove, minOuts);
282:             } else if (len == 3) {
283:->               uint256[3] memory minOuts;
284:                 ICurvePool(pool).remove_liquidity(amountPosRemove, minOuts);
285:             } else if (len == 4) {
286:->               uint256[4] memory minOuts;
287:                 ICurvePool(pool).remove_liquidity(amountPosRemove, minOuts);
288:             } else {
289:                 revert("Invalid pool length");
290:             }
291:         } else {
......
298:     }

We can see that the minOuts array of L280/L283/L286 are all empty arrays, where each item is 0. minOuts represents the minimum amounts of underlying coins to receive, which is specified by the caller. [This is slippage protection](https://github.com/curvefi/curve-contract/blob/master/contracts/pools/aeth/StableSwapAETH.vy#L498). If minOuts are all 0, then the underlying coins allowed to be received are arbitrary values. If this is noticed by a malicious user, the caller will suffer from a sandwich attack.

Impact

When calling closePositionFarm, if isKilled is set to true, the caller is vulnerable to a sandwich attack and loses funds.

Code Snippet

https://github.com/sherlock-audit/2023-07-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L277-L287

Tool used

Manual Review

Recommendation

If minOuts cannot be calculated off-chain, it is recommended to delete the relevant code when isKilled is true and only use remove_liquidity_one_coin.

Here is an unproved idea for minOuts:

Suppose pool has two tokens, the liquidity to be removed is 10e18 which will be divided into two parts, and maxPer is the slippage protection 95%. We can calculate the minOuts array off-chain:

minOuts[0] = pool.calc_withdraw_one_coin(5e18, 0) * maxPer
minOuts[1] = pool.calc_withdraw_one_coin(5e18, 1) * maxPer

minOuts can be passed as an parameter to closePositionFarm. This is better than an empty array.

sherlock-audit / 2023-07-blueberry-judging

nobody2018 - In CurveSpell.closePositionFarm, _removeLiquidity lacks slippage protection if isKilled is True #61