LI.FI contest - shw's results

Lines of code

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

Vulnerability details

Impact

There is a WithdrawFacet such that only the owner/admin can recover the lost funds in the contract. However, any user can retrieve the funds by using the swapTokensGeneric function, which might be unexpected behavior.

Proof of Concept

1. Suppose that 1000 USDC is left in the contract. Now, some random user wants to withdraw it.
2. They call the swapTokensGeneric function with corresponding parameters to swap 1000 USDC to DAI via the Uniswap V2 router.
3. The swapTokensGeneric function calls _executeSwaps, which calls LibSwap.swap. In this function, the contract checks whether LibAsset.getOwnBalance(fromAssetId) < fromAmount or not. If it has enough balance, it does not call LibAsset.transferFromERC20 to request the tokens from the caller.
4. Therefore, the user doesn't have to send more USDC to the contract because the funds are enough. As a result, they get the swapped DAI without providing more USDC.

GenericSwapFacet.sol#L22-L44 Swapper.sol#L12-L23 LibSwap.sol#L33-L35

Recommended Mitigation Steps

If recovering funds by anyone is not the desired behavior, consider always transferring the funds from users before the first swap.

Lines of code

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

Vulnerability details

Impact

The _startBridge function in CBridgeFacet is to bridge the tokens to CBridge by calling the sendNative or send function on the bridge contract. However, when calling the sendNative function, no native token is sent to the bridge. The sendNative call always fails because the CBridge side checks whether the exact _cBridgeData.amount of native tokens are provided. If not, it reverts the transaction.

Proof of Concept

Take the Ethereum CBridge V2 as an example. The sendNative function ensures the exact amount of native tokens are provided at line 64.

function sendNative(
 address _receiver,
 uint256 _amount,
 uint64 _dstChainId,
 uint64 _nonce,
 uint32 _maxSlippage
) external payable nonReentrant whenNotPaused {
 require(msg.value == _amount, "Amount mismatch");
 require(nativeWrap != address(0), "Native wrap not set");
 bytes32 transferId = _send(_receiver, nativeWrap, _amount, _dstChainId, _nonce, _maxSlippage);
 IWETH(nativeWrap).deposit{value: _amount}();
 emit Send(transferId, msg.sender, _receiver, nativeWrap, _amount, _dstChainId, _nonce, _maxSlippage);
}

CBridgeFacet.sol#L150-L156

Recommended Mitigation Steps

Consider sending _cBridgeData.amount amount of native tokens when calling the sendNative function, for example:

ICBridge(bridge).sendNative{ value: _cBridgeData.amount }(
 _cBridgeData.receiver,
 _cBridgeData.amount,
 _cBridgeData.dstChainId,
 _cBridgeData.nonce,
 _cBridgeData.maxSlippage
);

Also, add a payable keyword to the sendNative function in the ICBridge interface.

Lines of code

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

Vulnerability details

Impact

The batchRemoveDex function does not work as expected. It should remove all the given DEX addresses from the dexWhitelist. However, it only removes the first successfully found DEX address and then stops removing the rest. The functionality is broken, and therefore the admin can only remove one DEX address at a time.

Proof of Concept

There is a return statement at line 73, so after the first DEX address is found in the array and is removed, the function directly returns.

DexManagerFacet.sol#L73

Recommended Mitigation Steps

Consider removing line 73 (the return in the function).

Lines of code

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

Vulnerability details

Impact

When a user bridges a native token via the startBridgeTokensViaCBridge function of CBridgeFacet, the contract checks whether msg.value >= _cBridgeData.amount holds. In other words, if a user accidentally sends more native tokens than he has to, the contract accepts it but only bridges the _cBridgeData.amount amount of tokens. The rest of the tokens are left in the contract and can be recovered by anyone (see another submission for details).

Notice that in the similar functions of other facets (e.g., AnyswapFacet, HopFacet), the provided native token is ensured to be the exact bridged amount, which effectively prevents the above scenario of loss of funds.

Proof of Concept

CBridgeFacet.sol#L68

Recommended Mitigation Steps

Consider changing >= to == at line 68.

Lines of code

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

Vulnerability details

Impact

The withdraw function in WithdrawFacet uses the native transfer keyword to send ETH, which is considered unsafe because of the fixed gas budget, and its functionality could be broken in some circumstances:

1. The receiver consumes more than 2300 amounts of gas when receiving the ETH.
2. Even if the receiver consumes less than 2300 amount of gas, the consumed gas amount could change in the future when hard forks happen and therefore could exceed the limit.

Proof of Concept

WithdrawFacet.sol#L31

Recommended Mitigation Steps

Consider using a low-level call to send ETH, for example, the LibAsset.transferNativeAsset function.

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L54-L59 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L46 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L64 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L69

Vulnerability details

Impact

The startBridgeTokensVia... functions do not handle fee-on-transfer tokens (e.g, USDT) correctly. If some fee is deducted during the token transfer (LibAsset.transferFromERC20), the transaction will revert because of the following require check. As a result, users cannot bridge fee-on-transfer tokens via the startBridgeTokensVia... functions.

Proof of Concept

Take NXTPFacet as an example. At line 55, _nxtpData.amount is provided as the parameter of LibAsset.transferFromERC20. In cases where a fee-on-transfer token is transferred, the received amount could be less than _nxtpData.amount.

Then, at line 56, the contract ensures whether its balance is increased by exactly _nxtpData.amount, which is false in this case, causing the tx to be reverted.

NXTPFacet.sol#L54-L59

Recommended Mitigation Steps

Consider calculating the received amount of tokens after the transfer, and modify _nxtpData.amount accordingly, for example:

LibAsset.transferFromERC20(sendingAssetId, msg.sender, address(this), _nxtpData.amount);
_nxtpData.amount = LibAsset.getOwnBalance(sendingAssetId) - _sendingAssetIdBalance;