Swaps can be sandwiched as total receivable can't be user controlled

[code423n4]

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L85-L94 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L98-L107

Vulnerability details

Impact

There is no direct slippage control for the results of the swaps that user can perform before bridging, so they are potential subjects to sandwich attacks. Trades can happen at a manipulated price and end up receiving fewer tokens than current market price dictates.

Placing severity to medium as swapping is a core functionality of the system, while funds value (amount to be swapped and bridged) can be substantial, so sandwich attacks are often enough economically viable and thus probable, while they result in a partial fund loss.

Proof of Concept

All current swap functions invoke _executeSwaps and require net swap result to be positive only, i.e. accept anything by default.

AnyswapFacet.swapAndStartBridgeTokensViaAnyswap:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L85-L94

CBridgeFacet.swapAndStartBridgeTokensViaCBridge:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L98-L107

HopFacet.swapAndStartBridgeTokensViaHop:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L102-L111

NXTPFacet.swapAndStartBridgeTokensViaNXTP:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L91-L100

NXTPFacet.swapAndCompleteBridgeTokensViaNXTP:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L156-L168

GenericSwapFacet.swapTokensGeneric:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L23-L28

_swapData.callTo being called by LibSwap's swap might or might not have enough measures to control for total slippage, depending on the implementation:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L42-L48

Also, this can differ within the set of DEXes white listed, while all the swaps need to be slippage checked.

Recommended Mitigation Steps

As here the universal caller approach is to be implemented, it's prudent to add extra ways to configure the operations, even if they are crucial only in a small enough subset of scenarios.

Consider adding minimum accepted return argument to the mentioned swaps functions and condition execution success on it. The user can always choose to leave the minimum empty, either controlling it via _swapData.callData or choosing not to, but there should be the mechanics to do it at least on the level of the total amounts.

[H3xept]

We do not handle slippage checks directly as they are performed on the swap/bridge side. We limit ourselves to forwarding slippage preferences selected by users to the third-party service that will fulfil the transaction.

[gzeoneth]

Invalid submission.