code-423n4 / 2022-06-nested-findings

0 stars 1 forks source link

Vaults are vulnerable to sandwich attacks due to missing slippage checks #88

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/StakingLPVaultHelpers.sol#L36-L42 https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/StakingLPVaultHelpers.sol#L68-L74 https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L238-L255

Vulnerability details

Impact

Swaps in the new Beefy Vault can have almost all funds taken via MEV sandwich attacks because there is no slippage control

Proof of Concept

The last argument to add_liquidity() is the minimum amount to mint, which is zero here:

File: contracts/libraries/StakingLPVaultHelpers.sol   #1

36           if (poolCoinAmount == 2) {
37               pool.add_liquidity{ value: amount }(CurveHelpers.getAmounts2Coins(pool, eth, amount), 0);
38           } else if (poolCoinAmount == 3) {
39               pool.add_liquidity{ value: amount }(CurveHelpers.getAmounts3Coins(pool, eth, amount), 0);
40           } else {
41               pool.add_liquidity{ value: amount }(CurveHelpers.getAmounts4Coins(pool, eth, amount), 0);
42           }

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/StakingLPVaultHelpers.sol#L36-L42

and here:

File: contracts/libraries/StakingLPVaultHelpers.sol   #2

68           if (poolCoinAmount == 2) {
69               pool.add_liquidity(CurveHelpers.getAmounts2Coins(pool, token, amount), 0);
70           } else if (poolCoinAmount == 3) {
71               pool.add_liquidity(CurveHelpers.getAmounts3Coins(pool, token, amount), 0);
72           } else {
73               pool.add_liquidity(CurveHelpers.getAmounts4Coins(pool, token, amount), 0);
74           }

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/StakingLPVaultHelpers.sol#L68-L74

The second argument in swapExactTokensForTokens() is the minimum amount out, which is only one here, and the call to addLiquidity() afterwards has the same issue as the code above:

File: contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol   #3

238           uint256[] memory swappedAmounts = uniswapRouter.swapExactTokensForTokens(
239               swapAmountIn,
240               1,
241               path,
242               address(this),
243               block.timestamp
244           );
245   
246           (, , mintedLpAmount) = uniswapRouter.addLiquidity(
247               path[0],
248               path[1],
249               amount - swappedAmounts[0],
250               swappedAmounts[1],
251               1,
252               1,
253               address(this),
254               block.timestamp
255           );

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L238-L255

Tools Used

Code inspection

Recommended Mitigation Steps

Use a TWAP price oracle to determine the appropriate amount of assets to expect

obatirou commented 2 years ago

Vaults are vulnerable to sandwich attacks due to missing slippage checks (disputed)

We are checking the slippage at the end of the order. And not every operations. See deposit function