LI.FI contest - pmerkleplant's results

Lines of code

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

Vulnerability details

Impact

A token swap via NXTPFacet::swapAndCompleteBridgeTokensViaNXTP in which the receiving amount of tokens is zero does not fail.

Proof of Concept

The function NXTPFacet::swapAndCompleteBridgeTokensViaNXTP does not require that the token balance after the swap is higher than the token balance before the swap (see line 165).

Recommended Mitigation Steps

Add a require statement checking that postSwapBalance - startingBalance > 0 after the _executeSwap call.

Lines of code

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

Vulnerability details

Impact

Any ERC20 token balance held in the contract can be stolen by an attacker.

Proof of Concept

The swap function in the LibSwap.sol library checks if the contract's balance is sufficient to execute the trade (see line 33). If the contract does not hold the number of tokens to swap, the amount is transferred from the caller to the contract.

As long as the swap function is called with a swap amount of a token being less than the balance of the token in the contract, the function does not transfer the tokens from the caller but uses the contract's own tokens for the swap.

Furthermore, as the swapData argument is created by the caller (see for example AnyswapFacet.sol line 76), the contract called could be controlled by the caller and statically only returning 1 arbitrary token.

Recommended Mitigation Steps

Remove the second condition in the if-clause in the LibSwap::swap function to always transfer the tokens from the caller to the contract before executing a swap.

Lines of code

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

Vulnerability details

Impact

A user wanting to bridge ERC20 tokens would lose all ETH accidentally send to the contract.

Proof of Concept

The bridging facets follow the pattern of checking if the token is native or ERC20. In case the token is ERC20, there is no check that msg.value == 0 (except for NXTPFacet::completeBridgeTokensViaNXTP).

This leads to the ETH sent accidentally by a user being held unaccounted for in the contract and being lost for the user.

Recommended Mitigation Steps

Refactor following functions to include a check that msg.value == 0 in case the token address is not zero:

• CBridgeFacet::startBridgeTokensViaCBridge
• CBridgeFacet::swapAndStartBridgeTokenViaCBridge
• AnyswapFacet::startBridgeTokensViaAnyswap
• AnyswapFacet::swapAndStartBridgeTokensViaAnyswap
• NXTPFacet::swapAndStartBridgeTokensViaNXTP
• NXTPFacet::swapAndStartBridgeTokensViaAnyswap
• HopFacet::swapAndStartBridgeTokensViaHop
• HopFacet::startBridgeTokensViaHop

Lines of code

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

Vulnerability details

Impact

A user wanting to bridge ETH via CBridge could lose some amount of ETH.

Proof of Concept

The function startBridgeTokenViaCBridge checks the amount of ETH transfered with msg.value >= _cBridgeData.amount in case the token address is zero.

If a user accidentally sends more ETH than _cBridgeData.amount, that ETH would be held unaccounted for in the contract and be lost for the user.

Recommended Mitigation Steps

Refactor the check to msg.value == _cBridgeData.amount.