The Standard

Summary

The swap() functionality of SmartVaultV3 lacks slippage protection that will result in unfavorable swap prices in sandwich targeted attacks on vault swaps performed by the user in scenarios where they're over-collateralized. Specifically, in the case where their vault balance is more than enough to run the swap as well as pay the associated fee.

Vulnerability Details

swap is a function that the vault manager calls to run a swap via Uniswap. The vulnerable lines of code areamountOutMinimum: minimumAmountOut & sqrtPriceLimitX96: 0 which lack slippage protection. The minimumAmountOut handles just 1 of 2 cases (it handles the case where the vault's balance is insufficient to run a liquidation - minus fee then pre-computes the right amount to balance it out but not the case where the vault's balance is more than enough to run the swap + pay the fee - in the latter case it just returns 0 which is unprotected minimumAmountOut.

Sandwich attacks can be launched to then target user vault swap transactions from the protocol to extract the max value from each swap resulting in adverse prices of the swap of asset tokens in the vault.

An attacker can:

1. Take a flash loan of ETH and buy WBTC from the Uniswap pool related to the swap txn of the vault.

2. User's swap() txn is then executed at the current price manipulated from the first swap of the attacker.

3. The user's swap() txn will spend as much as needed ETH for buying WBTC at an overly-inflated price.

4. Attacker proceeds to swap WBTC for ETH and repays the flash loan.

5. Attacker keeps the rest of the swap profit extracted from vault swap txn.

This can be easily repeated/automated.

If minimumAmountOut is not 0 int he other case, the oracle can still be manipulated in a lesser extent. So the issue also stands in the other case.

Impact

This attack extracts as much from legitimate vault swaps that can lead to much reduced output tokens the smart vault receives from each swap which ultimately translates to huge amount of funds lost via swaps.

Tools Used

Manual review

Recommendations

Handle the latter case as well as specified when the vault is over-collateralised just as you did with the former case. Also, set the sqrtPriceLimitX96. 0 is not a protection of any sort for this parameter and just makes it inactive.

In development, swap impact is not important. In production it is. See this excerpt from the Uniswap v3 core documentation https://docs.uniswap.org/protocol/guides/swaps/single-swaps:

sqrtPriceLimitX96: We set this to zero - which makes this parameter inactive. In production, this value can be used to set the limit for the price the swap will push the pool to, which can help protect against price impact or for setting up logic in a variety of price-relevant mechanisms.

• MinAmount should be either determined off-chain or allow user to chose it in the parameters of the function.