[WP-H10] `GenericSwapFacet.sol#swapTokensGeneric()` duplicated `.call{ value: msg.value }` makes it possible for the attacker to steal native tokens (ETH) from the contract

[code423n4]

Lines of code

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

Vulnerability details

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

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

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

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]);
    }
}

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

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;
    emit AssetSwapped(
        transactionId,
        _swapData.callTo,
        _swapData.sendingAssetId,
        _swapData.receivingAssetId,
        fromAmount,
        toAmount,
        block.timestamp
    );
}

In the AnyswapFacet.sol, _anyswapData.router is from the caller's calldata, which can really be any contract, including a fake Anyswap router contract, as long as it complies to the interfaces used.

And in _startBridge, it will grant infinite approval for the _anyswapData.token to the _anyswapData.router.

This makes it possible for a attacker to steal all the funds from the contract.

Which we explained in [WP-H6], the diamond contract may be holding some funds for various of reasons.

PoC

Given:

• ETH price is 10,000 USDC per ETH;
• There are 0.1 ETH (native tokens) in the contract.

The attacker can submit a swapTokensGeneric() with 0.1 ETH as receivingAssetId with the following SwapData[]:

1. Swap 0.1 ETH to USDC;
2. Swap 0.1 ETH to USDC.

The attacker will receive ~2,000 USDC. 1,000 of which is the 0.1 ETH stolen from the contract.

Recommendation

1. Make sure to only call with {value: msg.value} when LibAsset.isNativeAsset(fromAssetId):

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

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

    uint256 callValue = msg.value;
    if (!LibAsset.isNativeAsset(fromAssetId)) {
        callValue = 0;
        LibAsset.approveERC20(IERC20(fromAssetId), _swapData.approveTo, fromAmount);
    }

    // solhint-disable-next-line avoid-low-level-calls
    (bool success, bytes memory res) = _swapData.callTo.call{ value: callValue }(_swapData.callData);
    if (!success) {
        string memory reason = LibUtil.getRevertMsg(res);
        revert(reason);
    }

2. Consider spinning off the swapper contract. See also [WP-H6].

[H3xept]

Duplicate of #86