TransferOutAndCall event is still emitted for failed transferOutAndCall, which will be observed by Bifrost

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e5ae503d0dc2394a82242be6860eb538345152a1/ethereum/contracts/THORChain_Router.sol#L261-L293
https://github.com/code-423n4/2024-06-thorchain/blob/e5ae503d0dc2394a82242be6860eb538345152a1/ethereum/contracts/THORChain_Router.sol#L209-L238
https://github.com/code-423n4/2024-06-thorchain/blob/e5ae503d0dc2394a82242be6860eb538345152a1/ethereum/contracts/THORChain_Router.sol#L185-L207
https://github.com/code-423n4/2024-06-thorchain/blob/e5ae503d0dc2394a82242be6860eb538345152a1/ethereum/contracts/THORChain_Router.sol#L304-L341

Vulnerability details

Impact

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

In the transferOutAndCall function, if the aggregator's swap fails and directly sending ETH to the recipient also fails (i.e. !erc20Success && !ethSuccess == true), the ETH is sent back to the vault.

In this case, the transferOutAndCall should be a no-op transaction, meaning it should have no side effects.

However, it still emits a TransferOutAndCall event, which will be logged by the Bifrost. when Bifrost observes a TransferOutAndCall event, it can not tell whether the funds were transferred to the recipient or sent back to the vault. This could lead to a situation where the THORNChain network believes the recipient has received a fund when they were actually sent back to the vault, resulting in a loss of funds for the recipient.

The same issue also applies to the transferOut and _transferOutV5 functions, which emit the TransferOut event even when ETH is sent back to the vault.
It could also apply to the _transferOutAndCallV5 functions, which emit the TransferOutAndCallV5 event when ETH is sent back to the vault.

Tools Used

Manual Review

Recommended Mitigation Steps

Design a distinct event for the scenario where ETH is sent back to the vault, and handle it separately in Bifrost.

Assessed type

Error

[c4-judge]

trust1995 marked the issue as satisfactory