Any ERC20 token from the Diamond balance can be stolen

[code423n4]

Lines of code

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

Vulnerability details

Impact

All ERC20 tokens can be stolen from the Diamond contract balance via AnyswapFacet, CBridgeFacet, HopFacet, NXTPFacet and GenericSwapFacet (all Facets that inherit from Swapper and call _executeSwaps) as LibSwap's swap utilize the available contract balance instead of user's funds.

This way a malicious user can construct a call so extract any non-native token balance from the Diamond contract. The only requirement is to pass along the crafted _swapData.fromAmount, which is then passed on to LibSwap's swap (after approveTo and callTo verification). I.e. an attacker can set the amount to the observed contract balance of a token and obtain full swap proceeds (setting approveTo and callTo to a valid DEX).

For example, if Bob the attacker observes the Diamond contract to have 3000 DAI on the balance, and let's say ETH is currently costs 3000 DAI. He can send a call with 3000 DAI to ETH swap in _swapData. As the contract balance covers the swap requirement Bob will not be requested for any additional funds, 3000 DAI from the balance be used, and he will obtain 1 ETH minus cumulative transaction cost for free, effectively emptying the DAI balance of the contract.

If the balance usage mechanics is meant to be for mistakenly sent funds utilization, such a design will lead to deterministic loss of those funds, as said Bob will just setup a bot that claims all balance funds as soon as it is economically viable (balance - expected cost > required profit threshold), while user who made a mistake will always be slower to react than said bot.

If 'first send tokens then call' approach is assumed here it has the similar issue as any front running bot will be free to steal the funds sent by invoking the swap before initial user.

Placing the severity to be high as those are direct fund loss scenarios solely enabled by the described issue.

Proof of Concept

LibSwap's swap function use the contract balance instead of user's funds if it is possible:

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

As LibSwap is a library and the code executes in the contract context, the Diamond contract balance will be used and it can be emptied this way.

The attack can be done via several entry points that invoke _executeSwaps and then return back or bridge the swap results to a user:

AnyswapFacet.swapAndStartBridgeTokensViaAnyswap:

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

CBridgeFacet.swapAndStartBridgeTokensViaCBridge:

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

HopFacet.swapAndStartBridgeTokensViaHop:

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

NXTPFacet.swapAndStartBridgeTokensViaNXTP:

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

NXTPFacet.swapAndCompleteBridgeTokensViaNXTP:

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

GenericSwapFacet.swapTokensGeneric:

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

_executeSwaps run all the swaps from _swapData in the cycle via LibSwap.swap(_lifiData.transactionId, _swapData[i]):

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L14-L21

This way the task for an attacker is to craft _swapData[i] to consist of a sole swap using exactly what the contract holds, obtaining any desired asset to be delivered to attacker's account on another chain.

Recommended Mitigation Steps

As for mistakenly sent funds there is a withdrawal mechanics with the help of WithdrawFacet, consider removing direct usage of own balance for user swap operations.

In the call sequence for the swap-and-bridge case described above the only place where user funds are requested is LibSwap.swap, so there is no workflow that brings in current user's funds to contract balance before that, so any usage of the balance is a transfer from one user to another. As malicious users will use such mechanics in the first place, it is recommended to be disabled.

Now:

if (!LibAsset.isNativeAsset(fromAssetId) && LibAsset.getOwnBalance(fromAssetId) < fromAmount) {
    LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
}

if (!LibAsset.isNativeAsset(fromAssetId)) {
    LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);
}

To be:

if (!LibAsset.isNativeAsset(fromAssetId)) {
    LibAsset.transferFromERC20(fromAssetId, msg.sender, address(this), fromAmount);
    LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);
}

[H3xept]

Duplicate of #66

We are aware that the contract allows users to use latent funds, although we disagree on it being an issue as no funds (ERC20 or native) should ever lay in the contract. To make sure that no value is ever kept by the diamond, we now provide refunds for outstanding user value (after bridges/swaps).