Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/Swapper.sol#L12 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibSwap.sol#L42

Vulnerability details

Impact

_executeSwaps is always wrapped in a loop, however, it uses msg.value to decide the amount of native tokens to use without any additional checks. Attackers can easily drain the existing funds by repeatedly swapping native asset.

Proof of Concept

The following exploit is demonstrated using GenericSwapFacet. But the vulnerability can be triggered from every faucet that uses _executeSwaps.

Assume that the native asset balance of the contract is 100eth
Alice calls GenericSwapFacet.swapTokensGeneric with msg.value = 100eth

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

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

        ...
    }

The _swapData array sent by Alice has two SwapData. So it calls LibSwap.swap twice.

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

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

Alice uses native asset as sendingAsset in both swap data. So LibSwap.swap won't transfer any more native asset from Alice.
_swapData.callTo.call{ value: msg.value }(_swapData.callData) is executed twice. The contract actually send 200 eth to DEX. But only receive 100 eth from Alice.

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

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

        …
    }

Tools Used

Manual code review.

Recommended Mitigation Steps

When sending native asset to DEX in LibSwap.swap, the contract should do bookkeeping on the actual used native asset and received native asset. A common way to do this is to extract msg.value into a local variable, and subtract from the said variable every time when native asset is used. Checks should also be enforced against this local variable instead of the raw msg.value.

code-423n4 / 2022-03-lifinance-findings