Assets can be stolen after a swap attempt in `_transferOutAndCallV5()`

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

1. Loss of user funds

2. Profit for a malicious party

Proof of concept

The _transferOutAndCallV5() is an internal function that is used in transferOutAndCallV5() and batchTransferOutAndCallV5() which allows a THOR Vault to transfer an amount of native coin or ERC20 tokens to a recipient by first swapping the corresponding amount using an aggregator.

Here is the code snippet responsible for swapping 1 asset to another before sending the output asset to the recipient :

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

// send ERC20 to aggregator contract so it can do its thing
(bool transferSuccess, bytes memory data) = aggregationPayload
    .fromAsset
    .call(
        abi.encodeWithSignature(
        "transfer(address,uint256)",
        aggregationPayload.target,
        aggregationPayload.fromAmount
        )
    );

require(
    transferSuccess && (data.length == 0 || abi.decode(data, (bool))),
    "Failed to transfer token before dex agg call"
);

// add test case if aggregator fails, it should not revert the whole transaction (transferOutAndCallV5 call succeeds)
// call swapOutV5 with erc20. if the aggregator fails, the transaction should not revert
(bool _dexAggSuccess, ) = aggregationPayload.target.call{value: 0}(
    abi.encodeWithSignature(
        "swapOutV5(address,uint256,address,address,uint256,bytes,string)",
        aggregationPayload.fromAsset,
        aggregationPayload.fromAmount,
        aggregationPayload.toAsset,
        aggregationPayload.recipient,
        aggregationPayload.amountOutMin,
        aggregationPayload.payload,
        aggregationPayload.originAddress
    )
);

Before the swap, the contract transfers the corresponding amount of tokens to the aggregator using a low-level call to transfer().

Now the aggregator holds the tokens.

Once this call is successful, a low-level call to swapOutV5() is made on the aggregator. Because it is a low-level call, in case it fails, the transaction will not revert, which is intended as stated in the comments above.

The transaction can revert for multiple reasons such as a lack of gas, the input and output assets being the same, insufficient amount of tokens due to fee-on-transfer...

The issue here is that the tokens are still being held by the aggregator if the swap failed and a malicious party is now able to call the swapOutV5() and specify its own address as recipient to effectively steal the tokens that were intended by another user.

Even though the aggregator is not in the scope of the audit, the root cause of the vulnerability takes place in the THORChain_Router contract.

Tools used

Manual review

Recommended mitigation steps

As stated by the comments "call swapOutV5 with erc20. if the aggregator fails, the transaction should not revert".

In my opinion, the call should revert in case of failure in order to mitigate the issue.

Assessed type

Token-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, the call to swapOutV5() on the contract at address aggregationPayload.target may fail. If it does, the tokens transferred previously will end-up lost on this contract and potentailly be stolen by another party.