`LibSwap.swap()` will transfer `fromAmount` if there is insufficient balance in the contract allowing an attacker to claim unused funds or forcing a user to transfer excess funds.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L29-L42 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L22-L43

Vulnerability details

Impact

The function will transfer tokens from msg.sender if and only if the current balance of the token is less than the fromAmount for a swap as seen in the lines below.

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

This poses two potential threats. The first is if a genuine user is making a swap with multiple paths and has incorrectly calculated fromAmount for the second swap (e.g. if the price has changed). If LibAsset.getOwnBalance(fromAssetId) < fromAmount where LibAsset.getOwnBalance(fromAssetId) should be the output of the previous swap (though it includes residual balance). Then the user will be charged the entire fromAmount which may revert if the user does not have sufficient funds or has not approved the LiFi contract. The user should not be charged then entire fromAmount since they have already contributed LibAsset.getOwnBalance(fromAssetId) from the previous swap. So even if a user was just one wei short due (possibly due to a price decrease) they will be forced to transfer the entire fromAmount for the next swap step.

The second issue is if there is any balance in the LiFi protocol the user may call LibSwap.swap() with fromAmount = LibAsset.getOwnBalance(fromAssetId) as a result transferFromERC20() will not be called. The function will continue to swap the asset and the user will receive the swapped tokens. Therefore, the user has not paid anything yet swapped the current balance of a ERC20 to in LiFi contract for another ERC20 token to which the attacker will receive.

Proof of Concept

Call GenericSwapToken.swapTokensGeneric() with fromAmount = LibAsset.getOwnBalance(fromAssetId) pick any output token and this will be transferred to the attacker.

    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);
...

    function swapTokensGeneric(LiFiData memory _lifiData, LibSwap.SwapData[] calldata _swapData) public payable {
        uint256 receivingAssetIdBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId);

        // Swap
        _executeSwaps(_lifiData, _swapData);

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

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

        emit LiFiTransferStarted(
            _lifiData.transactionId,
            _lifiData.integrator,
            _lifiData.referrer,
            _lifiData.sendingAssetId,
            _lifiData.receivingAssetId,
            _lifiData.receiver,
            _lifiData.amount,
            _lifiData.destinationChainId,
            block.timestamp
        );
    }

Recommended Mitigation Steps

To mitigate this issue consider passing a boolean variable as a function parameter in swap() this variable will indicate if it is the first step (i.e. first external call) in the swap path. If it is the first step require fromAmount to be transferred from the msg.sender or require fromAmount == msg.value for the native token.

The following steps must use the toAmount from the previous step. To do this modify swap() to return toAmount and have Swapper._executeSwap() pass previousToAmount as a function parameter to LibSwap.swap().

[H3xept]

Duplicate of #66