[WP-H6] Swapper can be used to steal all the funds from the contract

Lines of code

Vulnerability details

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

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

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

The Swapper allow arbitrary _swapData (0x style), this makes it possible for a attacker to steal the funds in the contract.

Based on the context, we beleive it’s possible that the contract can hold funds.

The funds can be the refunds of failed orders, or fee rebates from bridging or dex aggregators, etc.

See also the permissioned WithdrawFacet.

PoC

Given:

• There are 100 USDC tokens in the contract.

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

• 100 USDC to USDT;

As a result, the attacker will receive ~100 USDT with 0 USDC paid.

Recommendation

Given that Swapper is a standlone module that can be and should be deployed as a standalone contract, we suggest spin it off from the diamond so that it can no longer access the funds in the diamond contract.

---

The text was updated successfully, but these errors were encountered:

All reactions