The Standard

Summary

Users can lose funds with MEV attacks because of wrong implementation of slippage.

Vulnerability Details

After recent vulnerability disclosure, protocol team seems to implemented a "calculateMinimumAmountOut()" function (developer said in live competition kickoff that the old version had a vulnerability in swap, and only change in swap functionality between last version and this version is implementation of this function, hence I believe it was related to this).
The problem is, although this new function protect the user from one scenario: In the case of MEV attacks, users fund can not stolen so much that user become undercollateralised as we can see below:

it does not protect user from MEV completely, and user's fund can still get stolen within a limit such that user stay collateralised enough.
As we can see this function returns "0" if swap amount does not make user undercollateralised even if amountOut is 0. And for the other side (where if 0 amountOut returns, user will be undercollateralised) it only save user so little as to make them collateralised enough.
For example if user have 1000 euros worth of collateral (I won't write decimals for simplicity), and required collateral for user is 600 euros worth of collateral.

Then when user wants to swap 300 euros worth of collateral:
collateralValueMinusSwapValue will be 700 euros hence this function will return 0 and won't protect users from MEV attacks.

When user wants to swap 500 euros worth of collateral:
collateralValueMinusSwapValue will be 500 euros hence this function will return 100 euros worth of collateral token amount and will only protect users 100 euros worth of collateral from MEV, not rest of the 400 euros worth of collateral.

Impact

Users will some of their collateral amount during swap. It is medium likelihood and high impact. I consider this as a medium severity vulnerability.

Tools Used

Manual Review

Recommendations

It is best to incorporate a user specified slippage parameter to swap function. This doesn't mean protocol should remove calculateMinimumAmountOut() function. That function can be used to check if user specified slippage does not protect user from being undercollateralised, in that case second part of the return value will do its job correctly.

Pseudocode:

• Add slippage parameter to swap function.

• Send this slippage parameter to calculateMinimumAmountOut()

• Check inside this function with given slippage percentage, can user be undercollateralised?

• If so use the second part of the return calculator.eurToToken(getToken(_outTokenSymbol), requiredCollateralValue - collateralValueMinusSwapValue);, If not use user specified slippage percentage.