Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L304-L389 https://github.com/code-423n4/2024-06-thorchain/blob/0e02d3c221d7e546a73f25183e29500eaa63f4cf/chain/ethereum/contracts/THORChain_Aggregator.sol#L144-L216 https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L261-L293

Vulnerability details

Impact

When calling the following _transferOutAndCallV5 function with aggregationPayload.fromAsset being address(0), calling the target aggregator's swapOutV5 function below can revert, such as when the swapRouter.swapExactETHForTokens function call cannot satisfy aggregationPayload.amountOutMin. In this case, swapOutSuccess is false so payable(aggregationPayload.target).send(msg.value) is executed. However, such operation sends msg.value to aggregationPayload.target instead of aggregationPayload.recipient. Hence, although calling the target aggregator's swapOutV5 function reverts, the vault, which calls the _transferOutAndCallV5 function, still sends msg.value to the target aggregator while the recipient should receive such msg.value instead. As a result, the recipient loses such msg.value that is entitled to her or him.

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

  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.
        }
      }

      ...
    } else {
      ...
    }
  }

https://github.com/code-423n4/2024-06-thorchain/blob/0e02d3c221d7e546a73f25183e29500eaa63f4cf/chain/ethereum/contracts/THORChain_Aggregator.sol#L144-L216

  function swapOutV5(
    address fromAsset,
    uint256 fromAmount,
    address toAsset,
    address recipient,
    uint256 amountOutMin,
    bytes memory payload,
    string memory originAddress
  ) public payable nonReentrant {
    address[] memory path = new address[](2);
    ...
    if (fromAsset == address(0)) {
      // received ETH, swap to toToken or do as you please
      path[0] = WETH;
      path[1] = toAsset;

      if (payload.length == 0) {
        // no payload, process without wasting gas
        swapRouter.swapExactETHForTokens{value: msg.value}(
          amountOutMin,
          path,
          recipient,
          type(uint).max
        );
      } else {
        // do something with your payload like parse to an aggregator specific structure
        swapRouter.swapExactETHForTokens{value: msg.value}(
          amountOutMin,
          path,
          recipient,
          type(uint).max
        );
      }
    } else {
      ...
    }
  }

In contrast, the following transferOutAndCall function executes payable(to).send(_safeAmount), where _safeAmount equals msg.value, if calling the aggregator's swapOut function reverts. In this case, the vault, which calls the transferOutAndCall function, would correctly send msg.value to the recipient instead of the aggregator, which is unlike what would incorrectly happen with the _transferOutAndCallV5 function.

https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L261-L293

  function transferOutAndCall(
    address payable aggregator,
    address finalToken,
    address to,
    uint256 amountOutMin,
    string memory memo
  ) public payable nonReentrant {
    uint256 _safeAmount = msg.value;
    (bool erc20Success, ) = aggregator.call{value: _safeAmount}(
      abi.encodeWithSignature(
        "swapOut(address,address,uint256)",
        finalToken,
        to,
        amountOutMin
      )
    );
    if (!erc20Success) {
      bool ethSuccess = payable(to).send(_safeAmount); // If can't swap, just send the recipient the ETH
      if (!ethSuccess) {
        payable(address(msg.sender)).transfer(_safeAmount); // For failure, bounce back to vault & continue.
      }
    }
    ...
  }

Proof of Concept

The following steps can occur for the described scenario.

A vault calls the _transferOutAndCallV5 function with address(0) as aggregationPayload.fromAsset.
Calling the target aggregator's swapOutV5 function reverts because the swapRouter.swapExactETHForTokens function call does not satisfy aggregationPayload.amountOutMin.
In the _transferOutAndCallV5 function, executing payable(aggregationPayload.target).send(msg.value) sends msg.value from the vault to the target aggregator instead of the recipient.
The recipient loses such msg.value that is entitled to her or him.

Tools Used

Manual Review

Recommended Mitigation Steps

https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L324 can be updated to the following code.

        bool sendSuccess = payable(aggregationPayload.recipient).send(msg.value);

Assessed type

ETH-Transfer

code-423n4 / 2024-06-thorchain-findings

Calling `_transferOutAndCallV5` function with `aggregationPayload.fromAsset` being `address(0)` causes vault to incorrectly send `msg.value` to target aggregator instead of recipient when calling target aggregator's `swapOutV5` function reverts #42