Native token is sent to the wrong address in `_transferOutAndCallV5()` leading to the theft of these assets

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L324

Vulnerability details

Impact

1. Loss of funds

2. Potential profit for a malicious party

Proof of concept

The _transferOutAndCallV5() allows a Vault to swap native ETH to any ERC20 token supported by the protocol through the swapOutV5() function called on an aggregator.

https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L304-L328

function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
) private {
    if (aggregationPayload.fromAsset == address(0)) {
        // call swapOutV5 with ether
        (bool swapOutSuccess, ) = aggregationPayload.target.call{
            value: msg.value
        }(
            abi.encodeWithSignature(
                "swapOutV5(address,uint256,address,address,uint256,bytes,string)",
                aggregationPayload.fromAsset,
                aggregationPayload.fromAmount,
                aggregationPayload.toAsset,
                aggregationPayload.recipient,
                aggregationPayload.amountOutMin,
                aggregationPayload.payload,
                aggregationPayload.originAddress
            )
        );
        if (!swapOutSuccess) {
            bool sendSuccess = payable(aggregationPayload.target).send(msg.value); // If can't swap, just send the recipient the gas asset
            if (!sendSuccess) {
                payable(address(msg.sender)).transfer(msg.value); // For failure, bounce back to vault & continue.
            }
        }

In case the swap fails (out-of-gas or some obscure reasons), the protocol implements a "fallback" mechanism which consists of "just send the recipient the gas asset" as stated by the comment.

In reality, the gas asset is transferred to the aggregator aggregationPayload.target not the recipient, which would be aggregationPayload.recipient.

If we abstract from the developers comment and the actual intention was to send the gas asset to the aggregator, another issue occurs : the gas assets will remain on the aggregator contract.

This allows anyone to call the swapOutV5() function on the aggregator to swap the gas assets to another token resulting in these being effectively stolen from the legitimate user it belonged to.

In both scenarios, the current implementation introduces a vulnerability for the users of the protocol.

Tools used

Manual review

Recommended mitigation steps

Send the gas asset to aggregationPayload.recipient rather than aggregationPayload.target.

Doing so will result in the legitimate user to receive its funds even if the swap failed.

Assessed type

ETH-Transfer

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid

[0xGreed]

I would like to have an explanation about why the issue has been invalidated. From my understanding of this part of the code, it seems that the native assets are sent to the wrong address in case the swap fails. The swap is executed on the contract at address aggregationPayload.target and the fallback mecanism sends the ETH back to this same contract (e.g. aggregationPayload.target) while it was intended to be sent to the recipient of the swap initially.

[c4-judge]

trust1995 marked the issue as unsatisfactory: Out of scope