`THORChain_Router::transferOut` and `THORChain_Router::_transferOutV5` emits incorrect data in `TransferOut` event breaking one of the Main Invariants of the Protocol

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L182-L238 https://github.com/code-423n4/2024-06-thorchain/blob/main/bifrost/pkg/chainclients/shared/evm/smartcontract_log_parser.go#L213-L243

Vulnerability details

Impact

If the asset being transferred in THORChain_Router::transferOut and THORChain_Router::_transferOutV5 is ETH and for some reason the transfer fails and the ETH was sent back to the vault (i.e. msg.sender), these functions would still emit the TransferOut event with the receiving address of the ETH as to.

Here even though the ETH was returned to the vault, the Bifrost will detect it as ETH transferred to the address to and broadcast these changes to THORChain L1:

  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 {
      _vaultAllowance[msg.sender][asset] -= amount; // Reduce allowance
      (bool success, bytes memory data) = asset.call(
        abi.encodeWithSignature("transfer(address,uint256)", to, amount)
      );
      require(success && (data.length == 0 || abi.decode(data, (bool))));
      safeAmount = amount;
    }
@>  emit TransferOut(msg.sender, to, asset, safeAmount, memo);
  }

  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 {
      _vaultAllowance[msg.sender][
        transferOutPayload.asset
      ] -= transferOutPayload.amount; // Reduce allowance

      (bool success, bytes memory data) = transferOutPayload.asset.call(
        abi.encodeWithSignature(
          "transfer(address,uint256)",
          transferOutPayload.to,
          transferOutPayload.amount
        )
      );

      require(success && (data.length == 0 || abi.decode(data, (bool))));
    }

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

Also as pointed out in the contest README, this issue counts for HIGH impact:

Bifrost Overview

The most important part of Bifrost for this audit contest is the smartcontract_log_parser, and specifically the GetTxInItem function. The GetTxInItem is run for each obvserve transaction on EVM chains. The function iterates through each log emitted by the transaction, and determines if any valid logs were emitted by the THORChain router. If it determines that a log was emitted by the THORChain router, it parses that log and forwards the appropriate details to the THORChain Layer 1 for processing. This is an incredibly sensitive and crucial piece of the codebase, as if it can be tricked, this means malicious contracts can trick Bifrost into informing THORChain of something that didn’t actually happen, leading to loss of funds. In fact, in July of 2021, THORChain’s old router + Bifrost were hacked in this exact way.

This issue also breaks one of the Main Invariants specified by the protocol:

Only valid events emitted from the Router contract itself should result in the txInItem parameter being populated in the GetTxInItem function of the smartcontract_log_parser.

Tools Used

Manual Review

Recommended Mitigation Steps

Return if ETH transfer failed and was sent back to the vault.

    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 {

Assessed type

Other

[c4-judge]

trust1995 marked the issue as satisfactory