When the ETH interaction fails, the event will also be triggered normally

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L195
https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L212
https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L277
https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L323

Vulnerability details

Impact

For some failed interaction operations, the contract will be returned to ETH, but related events will be triggered and captured normally, which may lead to abnormal status.

Proof of Concept

Here are specific examples of the problem.

What needs to be declared is:
In these instances, those related to the transferOut event can be processed normally because processing has been implemented

if asset.IsEmpty() {
				 
   return false, nil
			
}

But the transferOutAndCall related event does not, as it does not have relevant rules to handle situations where the transfer fails but an unexpected event is sent out.

1. The transferOut function returns ETH to msg.sender after ETH sending fails, but triggers the TransferOut event normally
   github:[1]

  function transferOut(
    address payable to,
    address asset,
    uint amount,
    string memory memo
  ) public payable nonReentrant {
      uint safeAmount;
      if (asset == address(0)) {
      safeAmount = msg.value;
      bool success = to.send(safeAmount); // Send ETH.
      if (!success) {
        payable(address(msg.sender)).transfer(safeAmount); // For failure, bounce back to vault & continue.
      }
    } else {
      ...
    }
    emit TransferOut(msg.sender, to, asset, safeAmount, memo);

2. _The transferOutV5 function returns ETH to msg.sender after ETH sending fails, but triggers the TransferOut event normally.
   github:[2]

  function _transferOutV5(TransferOutData memory transferOutPayload) private {
    if (transferOutPayload.asset == address(0)) {
      bool success = transferOutPayload.to.send(transferOutPayload.amount); // Send ETH.
      if (!success) {
        payable(address(msg.sender)).transfer(transferOutPayload.amount); // For failure, bounce back to vault & continue.
      }
    } else {
       ...
    }

    emit TransferOut(
      msg.sender,
      transferOutPayload.to,
      transferOutPayload.asset,
      transferOutPayload.amount,
      transferOutPayload.memo
    );
  }

3. The transferOutAndCall function swapOut exchange failed, returning ETH, but the TransferOutAndCall event was triggered normally
   github:[3]

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

    emit TransferOutAndCall(
      msg.sender,
      aggregator,
      _safeAmount,
      finalToken,
      to,
      amountOutMin,
      memo
    );
  }

4. _The ETH interaction of transferOutAndCallV5 failed and returned ETH, but the TransferOutAndCallV5 event was triggered normally
   github:[4]

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

      emit TransferOutAndCallV5(
        msg.sender,
        aggregationPayload.target,
        msg.value,
        aggregationPayload.toAsset,
        aggregationPayload.recipient,
        aggregationPayload.amountOutMin,
        aggregationPayload.memo,
        aggregationPayload.payload,
        aggregationPayload.originAddress
      );
    } else {
       ...
    }
  }

Tools Used

Manual review

Recommended Mitigation Steps

After the interaction fails and returns ETH or ERC20, directly return the function instead of continuing to trigger the event.
Here is an example.

function transferOut(
    address payable to,
    address asset,
    uint amount,
    string memory memo
  ) public payable nonReentrant {
      uint safeAmount;
      if (asset == address(0)) {
      safeAmount = msg.value;
      bool success = to.send(safeAmount); // Send ETH.
      if (!success) {
        payable(address(msg.sender)).transfer(safeAmount); // For failure, bounce back to vault & continue.
+        return;
      }
    } else {
      ...
    }
    emit TransferOut(msg.sender, to, asset, safeAmount, memo);

Assessed type

Error

[c4-judge]

trust1995 marked the issue as satisfactory