Anyone can get swaps for free given certain conditions in `swap`.

[code423n4]

Lines of code

LibSwap.swap GenericSwapFacet.sol

Vulnerability details

Impact

Remaining or unaccounted ERC20 balance could be freely taken through swapTokensGenerics and swap.

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);
        if (!success) {
            string memory reason = LibUtil.getRevertMsg(res);
            revert(reason);
        }

        toAmount = LibAsset.getOwnBalance(_swapData.receivingAssetId) - toAmount;

Given:

• There has been a deposit to LiFi of a non-native ERC20 that makes LibAsset.getOwnBalance(fromAssetId) a desirable amount.

• Attacker calls swapTokensGeneric with a _swapData.fromAmount value just below LibAsset.getOwnBalance(fromAssetId).

• First if statement in swap is skipped (no funds are tranferred to LiFis contract).

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

• Swap happens and increases LibAsset.getOwnBalance(_lifiData.receivingAssetId)
• Difference of LiFis balance of the receiving token before and after swap is calculated using postSwapBalance and transfered to attacker.

Tools Used

Recommended Mitigation Steps

Ensure funds are always subtracted from users account in swap, even if LiFi has enough balance to do the swap.

[H3xept]

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

[H3xept]

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

[gzeoneth]

Keeping this and all duplicates as Med Risk. There can be fund leftover in the contract under normal operation, for example this tx. In fact, ~$300 worth of token is left in the LI.Fi smart contract on ETH mainnet 0x5a9fd7c39a6c488e715437d7b1f3c823d5596ed1 as of block 14597316. I don't think this is High Risk because the max amount lost is no more than allowed slippage, which can be loss to MEV too.

[ezynda3]

This has been fixed in the most recent version of src/Helpers/Swapper.sol which sweeps any latent funds back to the user's wallet.