Description

When a user performs an external swap, either normally or through the cross-chain router, they can arbitrarily specify the calldata sent to the external swap service:

function externalSwap(
    address approveTarget,
    address swapTarget,
    address fromToken,
    address toToken,
    uint256 fromAmount,
    uint256 minToAmount,
    address payable to,
    bytes calldata data
) external payable override nonReentrant returns (uint256 realToAmount) {
    // [...]
    require(isWhitelisted[swapTarget], "WooRouter: SWAP_TARGET_NOT_ALLOWED");

    // [...]

    _internalFallbackSwap(approveTarget, swapTarget, fromToken, fromAmount, data);

    // [...]
}

function _internalFallbackSwap(
    address approveTarget,
    address swapTarget,
    address fromToken,
    uint256 fromAmount,
    bytes calldata data
) private {
    require(isWhitelisted[approveTarget], "WooRouter: APPROVE_TARGET_NOT_ALLOWED");
    require(isWhitelisted[swapTarget], "WooRouter: SWAP_TARGET_NOT_ALLOWED");

    if (fromToken != ETH_PLACEHOLDER_ADDR) {
        TransferHelper.safeTransferFrom(fromToken, msg.sender, address(this), fromAmount);
        TransferHelper.safeApprove(fromToken, approveTarget, fromAmount);
        (bool success, ) = swapTarget.call{value: 0}(data);
        TransferHelper.safeApprove(fromToken, approveTarget, 0);
        require(success, "WooRouter: FALLBACK_SWAP_FAILED");
    } else {
        require(fromAmount <= msg.value, "WooRouter: fromAmount_INVALID");
        (bool success, ) = swapTarget.call{value: fromAmount}(data);
        require(success, "WooRouter: FALLBACK_SWAP_FAILED");
    }
}

Even though the swap target and the approve target are whitelisted, the calldata is not parsed to ensure that the user is, in fact, calling a swap function.

Impact

If a whitelisted external-swap target contract has any functions that cause unrelated side effects, those side effects can be manipulated in arbitrary ways by users executing false swaps.

For example, if a swap target implements a claimReward function that claims rewards based on a caller's swap volume or a setDelegate function to allow the caller to grant permissions to another address to take some action on behalf of the caller, or if the swap target is also an ERC-20 token and has a transfer function — in all of these cases, a user can profit off of a fake swap.

Even if a swap target does not currently implement such a function, if it is upgradable, a future iteration of it may implement a risky function. For example, consider the case where a swap target is exploited, but the exploit is fixed and funding is secured to reimburse victims, and each affected user can call claimShare on the contract to claim what they lost. If this happens, WOOFI would likely wish to manually call this on behalf of the pool and further divide the share among its users. However, because of this arbitrary calldata issue, they will likely be beaten by someone claiming all the tokens for themselves using a false external swap.

Recommendations

Since the swap target whitelist is already a manually managed whitelist, we recommend including the four-byte signature in the key for this whitelist so that users can only call a permitted (swapTarget, function) pair. This greatly reduces the chance that the whitelist allows for an unexpected call, even if the swap target was upgraded to add additional functionality.

Remediation

Additionally, the WOOFI team has stated that:

Will consider adding a function selector check in later version. But still, the official doc of 1inch integration is not having this check, and we're concerned that it may not have a fixed function to enforce.