The Standard

Summary

The swap function in SmartVaultV3 has some issues that need to be looked at before the project goes live. They can result in a loss of funds for the vault owner if they decide to use the function.

Vulnerability Details

1. Some pools have even lower fees than 0.3%. For example, the WETH/USDC pool on Optimism has pools with fees equal to 0.05%, lower than 0.3% fees. In such cases, hardcoding the fees to 3000 in the ExactInputSingleParams struct results in users paying more fees. Also, chances are pools with lower fees might have more liquidity and hence provide more efficient swaps, especially for common pairs (WETH/USDC pool with 0.05% fees on Optimism, has more liquidity than 0.3% fees). Hence the swap is not efficient when fees are hardcoded in this manner.

2. The deadline is set as block.timestamp in the ExactInputSingleParams struct. Without a proper deadline, the transaction might be left hanging in the mempool and be executed way later than the user wanted. That could lead to users getting a worse price because a validator can just hold onto the transaction. And when it does get around to putting the transaction in a block, it'll be block.timestamp, so they’ve got no protection there. If there's no deadline, the transaction could be held back indefinitely leading to swaps that are not optimal.

   More on this - https://blog.bytes032.xyz/p/why-you-should-stop-using-block-timestamp-as-deadline-in-swaps

3. calculateMinimumAmountOut can return 0 as amountOutMinimum value. When the code has set 0 as amountOutMinimum, it tells the swap function that the user will accept a minimum amount of 0 output tokens from the swap, opening up the user to a catastrophic loss of funds via MEV bot sandwich attacks. So, when these transactions are seen in the mempool, the user's transaction will be front-run and the user would receive fewer tokens than intended because of 0 slippage. Also, an attacker can exploit the function to always return 0. In the function calculateMinimumAmountOut , if collateralValueMinusSwapValue ≥ requiredCollateralValue, 0 is returned and collateralValueMinusSwapValue = euroCollateral() - calculator.tokenToEur(getToken(_inTokenSymbol), _amount); . So, if euroCollateral increases, then collateralValueMinusSwapValue also increases. euroCollateral is dependent on getAssetBalance which gets the balance of the asset in the contract. An attacker can donate the asset amount directly to the contract, which inflates the value of euroCollateral, hence increasing the value of collateralValueMinusSwapValue. This would result in calculateMinimumAmountOut to return 0 and hence subjected to slippage. The owner of the vault will be extremely prone to this attack when the amount being swapped is quite large.

4. On-Chain slippage calculation can be manipulated. calculateMinimumAmountOut can also return the value calculator.eurToToken(getToken(_outTokenSymbol), requiredCollateralValue - collateralValueMinusSwapValue); This is problematic, as slippage should not be calculated in this manner on-chain. It should be done off-chain.
   While this block of code is trying to set a slippage value, during times of high volatility slippage is unavoidable. Users should always be able to override this slippage with their own slippage to ensure whether they want a transaction to go through or revert. Developers should let users specify their own slippage value.

   More on this - https://dacian.me/defi-slippage-attacks#heading-hard-coded-slippage-may-freeze-user-funds

Impact

The impact of the issues has already been detailed above in the Vulnerability Section.

Tools Used

Manual review

Recommendations

1. It is recommended that the function takes in input that allows users to set their own fees.

2. Allow for a deadline to be set by the user, such that after the deadline the transaction never takes place.

3. It is recommended that the function allow the user to set their own amountOutMinimum to avoid slippage.

4. Avoid calculating slippage on-chain. Do it off-chain.