LI.FI contest - 0xDjango's results

Lines of code

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

Vulnerability details

Impact

In the swapTokensGeneric() function, an arbitrary number of swaps can be performed from and to various tokens. However, the final balance that is sent to the user relies on _lifiData.receivingAssetId which has no use in the swapping functionality. LifiData is claimed to be used purely for analytical reasons per the comments to this function. If this value is input incorrectly, the swapped tokens will simply sit in the contract and be lost to the user.

Proof of Concept

Imagine a call to swapTokensGeneric() with the following parameters (excluding unnecessary parameters for this example):

• LifiData.receivingAssetId = '0xUSDC_ADDRESS'

Single SwapData array:

• LibSwap.SwapData.sendingAssetId = '0xWETH_ADDRESS'
• LibSwap.SwapData.receivingAssetId = '0xDAI_ADDRESS'

Since the receivingAssetId from SwapData does not match the receivingAssetId from LifiData, the final funds will not be sent to the user after the swap is complete, based on the following lines of code:

uint256 receivingAssetIdBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId);

 _executeSwaps(_lifiData, _swapData);

 uint256 postSwapBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId) - receivingAssetIdBalance;

 LibAsset.transferAsset(_lifiData.receivingAssetId, payable(msg.sender), postSwapBalance);

Lines 1, 3, and 4 reference LifiData.receivingAssetId and handle the transfer of funds following the swaps. Line 2 performs the swap, referencing SwapData.receivingAssetId as can be seen in the executeSwaps() function definition:

function _executeSwaps(LiFiData memory _lifiData, LibSwap.SwapData[] calldata _swapData) internal {
        // Swap
        for (uint8 i; i < _swapData.length; i++) {
            require(
                ls.dexWhitelist[_swapData[i].approveTo] == true && ls.dexWhitelist[_swapData[i].callTo] == true,
                "Contract call not allowed!"
            );

            LibSwap.swap(_lifiData.transactionId, _swapData[i]);
        }
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

I recommend adding a check that _lifiData.receivingAssetId equals the receivingAssetId of the last index of the SwapData array, or simply use the receivingAssetId of the last index of the SwapData array for sending the final tokens to the user.

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/GenericSwapFacet.sol#L23-L30 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibSwap.sol#L48

Vulnerability details

Impact

Every function that stems from the GenericSwapFacet lacks checks to ensure that some tokens have been returned via the swaps. In LibSwap.sol in the swap() function, the swap call is sent to the target DEX. A return of success is required, otherwise the operation will revert.

Each "inner" swap via LibSwap.sol lacks output checks and also the "outer" swapTokensGeneric() via GenericSwapFacet.sol lacks a final check as well.

There is a possibility that the calldata is accidently populated with a function in the target router that is not actually performing any swapping functionality, getAmountsOut() for example. The function will return true, but no new returned tokens will be present in the contract. Meanwhile, the contract has already received the user's fromTokens directly.

Tools Used

Manual Review

Recommended Mitigation Steps

This would be a potential use case of using function signature whitelists as opposed to contract address whitelists, as noted as a possibility by the LiFi team.

Otherwise, the following require statement in swapTokensGeneric() would ensure that at least a single token was received:

require(LibAsset.getOwnBalance(_swapData.receivingAssetId) - toAmount) > 0, "No tokens received")

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibSwap.sol#L29-L46

Vulnerability details

Impact

In the swap() function in LibSwap.sol, the SwapData attributes determine many of the initial logical steps, but do not have any bearing on the actual calldata that's passed via the call() function.

Proof of Concept

function swap(bytes32 transactionId, SwapData calldata _swapData) internal {
        uint256 fromAmount = _swapData.fromAmount;
        uint256 toAmount = LibAsset.getOwnBalance(_swapData.receivingAssetId);
        address fromAssetId = _swapData.sendingAssetId;
        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);
        }

        // solhint-disable-next-line avoid-low-level-calls
        (bool success, bytes memory res) = _swapData.callTo.call{ value: msg.value }(_swapData.callData);

The first several lines of this function transfer the _swapData.fromAssetId to the contract in the amount of _swapData.fromAmount, and subsequently approves _swapdata.approveTo for the same amount. The issue is that these values may not be what is being passed to the DEX contract via the _swapData.callData in the call() function.

The most severe issue as a result of a discrepancy that I can foresee is if swapData.fromAmount is larger than what is passed in via the calldata, the full swapData.fromAmount is transferred to the contract, then the swap is performed via the value in the calldata. The leftover amount will sit in the contract, available to whoever next swaps TO the original fromAssetId.

Tools Used

Manual review

Recommended Mitigation Steps

You have mentioned the idea of whitelisting function signatures instead of DEX addresses. If you do so, if it's feasible to obtain the contracts' ABIs to decode the calldata input, you can verifiably ensure that the values used in the internal logic will match what is sent to the DEXs.

Otherwise, you can add a step to "sweep" the remainder balance (if any) following swaps to ensure that the user retains maximum token value.

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/DexManagerFacet.sol#L62-L77

Vulnerability details

Impact

Only one DEX removed while performing batchRemoveDEX() in DexManagerFacet.sol, potentially leading to many whitelisted DEXs previously thought to have been removed.

Proof of Concept

Given the following code to remove multiple DEXs from the whitelist:

function batchRemoveDex(address[] calldata _dexs) external {
	LibDiamond.enforceIsContractOwner();

	for (uint256 i; i < _dexs.length; i++) {
		if (s.dexWhitelist[_dexs[i]] == false) {
			continue;
		}
		s.dexWhitelist[_dexs[i]] = false;
		for (uint256 j; j < s.dexs.length; j++) {
			if (s.dexs[j] == _dexs[i]) {
				_removeDex(j);
				return;
			}
		}
	}
}

The batchRemoveDEX() function contains nested for loops. The inner for loop unfortunately uses return instead of break, causing the execution of the function to stop the moment it removes the first DEX. This is potentially serious as the team will believe they've removed multiple DEXs from being able to be called from the facets, meanwhile the DEXs can still receive said calls.

Tools Used

Manual review.

Recommended Mitigation Steps

Change return to break.

Issue 1 (Low) - Avoid Floating Pragma

Details

Most or all contracts include a floating pragma ^0.8.7. It is recommended that the compiler version be locked to a single version to avoid contracts deployed with different versions that may contain various compiler bugs.

Issue 2 (Low) - No Transfer Ownership Pattern

Link to Code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/OwnershipFacet.sol#L7-L11

Details

It is recommended to employ a transfer/accept ownership transfer pattern. This will avoid accidently transferring ownership to an incorrect address, as the accept function must be called by the pending owner for the transfer to be completed.

Issue 3 (Low) - LifiData can be set arbitrarily, dimenishing useability in analytics

Link to Code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/GenericSwapFacet.sol#L22

Details

The comments specify:

_lifiData data used purely for tracking and analytics

The function uses the _lifiData attributes such as transactionId for logging, but this value can be arbitrary by the msg.sender.

Issue 4 (low) - Unnecessary transfer amount to LibSwap.sol

Link to Code

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

Details

The swap function contains logic that the fromAmount must be transferred to LibSwap.sol if the contract balance is less than the fromAmount. Given the possiblity that the contract already contains some number of tokens, the entire from amount will still be transferred to the contract, and subsequently swapped, leaving the same remainder tokens as before. Given the functionality that checks if the contract balance is equal to or larger than the from amount in the first place, it would make sense to only send the difference between the fromAmount and the contract balance.

Issue 5 (low) - Use of deprecated safeApprove() function

Link to Code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibAsset.sol#L67-L68

Details

OpenZeppelin has marked this function as deprecated and recommends using safeIncreaseAllowance. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38-L43

Repeated if conditions

Same conditions repeated in separate ifs, can be reduced to nested ifs, saving a repeated clause.

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

Update code:

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