Lines of code

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

Vulnerability details

Impact

If the ETH swap to finalToken in THORChain_Router::transferOutAndCall fails then there will be TransferOutAndCall event emitted with incorrect data.

Case 1: ETH is sent to the recipient to, but the event emitted still tracks the finalToken as passed in the function parameter, even though recipient received ETH and not finalToken.

Case 2: ETH transfer to recipient to fails and the ETH is sent back to the vault, but the event emitted tracks the to and finalToken as passed in the function parameter, even though recipient never received any tokens.

The Bifrost will detect these events with incorrect data and broadcast these changes to THORChain L1:

  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
    );
  }

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

Update the finalToken if swap fails and return if ETH is transferred back to the vault.

    if (!erc20Success) {
      bool ethSuccess = payable(to).send(_safeAmount); // If can't swap, just send the recipient the ETH
+     finalToken = address(0); // address(0) for ETH
      if (!ethSuccess) {
        payable(address(msg.sender)).transfer(_safeAmount); // For failure, bounce back to vault & continue.
+       return;
      }
    }

Assessed type

Other

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