Bandit - No Slippage Protection When Adding and Removing Liquidity and Liquidations

[sherlock-admin]

Bandit

high

No Slippage Protection When Adding and Removing Liquidity and Liquidations

Summary

The callbacks which add and remove liquidity on Uniswap do not have slippage parameters.

Vulnerability Detail

When liquidity is added/removed through Aloe's modify function, Uniswap's mint and burn functions are called directly.

The generally correct way to make contract calls is through increaseLiquidity and decreaseLiquidity. amount0Min and amount1Min are passed through as slippage parameters which is used when Uniswap is called this way with:

nonfungiblePositionManager.decreaseLiquidity(params)

With the direct call as implemented in Aloe, attempts to add liquidity can be sandwiched with a transaction which

1. Pushes the pool price to the wrong price
2. Allows the victim to add liquidity
3. Sells tokens into the liquidity for a profit.

(different step order and direction for removing liquidiy)

This applies to adding and removing liquidity during modify, and the liquidity removal during liquidate.

Impact

• Loss of funds to MEV/Sandwich attacks when adding liquidity
• • Loss of funds to MEV/Sandwich attacks when removing liquidity

Code Snippet

https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Borrower.sol#L358-L364

Tool used

Manual Review

Recommendation

Call Uniswap's decreaseLiquidity function and the corresponding increase function and allow customizable slippage parameters as a user input that are passed down in the modify and liquidate functions.

[sherlock-admin2]

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

panprog commented:

low, because uniswapDeposit and withdraw are called via callee contract, which should check for slippage by itself (there is no point in passing some values for slippage since they will be the same in the callee contract). Profiting off liquidity removal (like in liquidate) is impossible - any pool manipulations from current price increases the removed liquidity when calculated at current price, and creates a loss for manipulator.

MohammedRizwan commented:

valid

[haydenshively]

Most of the issues linked here are invalid, as the IManager is responsible for checking slippage inside the callback. I believe I've explained this elsewhere.

113 is distinct in that it references uniswapWithdraw behavior during liquidations. This is a more interesting case, but my understanding is that the value of Uniswap liquidity can only be manipulated upwards so any attack would be a balancing act between [cost of moving the price and increasing liquidity value] and [reward of getting 5% bonus on slightly more assets]. Note that since the 5% bonus is measured at the TWAP, standard sandwich attacks wouldn't increase it much.

If the author of #113 can demonstrate impact, I'd consider that High.