Incorrect Event Emission due to Ether Transfer Failure in THORChain Router Contract

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L185-L207 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L209-L229 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L261-L293 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L304-L340

Vulnerability details

The THORChain_Router contract has several instances where Ether transfer failures are not correctly handled, resulting in the emission of incorrect events. This can cause downstream systems, such as smartcontract_log_parser.go, to incorrectly process these events.

Impact

The incorrect emission of events due to Ether transfer failures can lead to significant issues in downstream processing. Specifically, the smartcontract_log_parser.go component, which is designed to parse and take appropriate actions based on these events, will process the wrong events. This can result in incorrect accounting within the Thorchain ecosystem.

Proof of Concept

The issue occurs in the following instances within the THORChain_Router contract:

1. transferOut()

    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 {
      // <!-- SKIP -->
      }
  @>> emit TransferOut(msg.sender, to, asset, safeAmount, memo);
    }

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L185C3-L207C4

2. _transferOutV5()

    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 {
        // <!-- SKIP -->
      }

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

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L209C3-L229C6

3. transferOutAndCall()

    function transferOutAndCall(
      address payable aggregator,
      address finalToken,
      address to,
      uint256 amountOutMin,
      string memory memo
    ) public payable nonReentrant {
      uint256 _safeAmount = msg.value;
      // <!-- SKIP -->
      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
      );
    }

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L261C3-L293C4

4. _transferOutAndCallV5()

  function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
  ) private {
    if (aggregationPayload.fromAsset == address(0)) {
      // <!-- SKIP -->
      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
      );

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L304C3-L340C9

Please see the above functions, all are designed to send ETH back to the msg.sender incase if the ETH transfer is failed, but the issue even if its bounce back to the vault they are emitting events which are designed for successful transaction. Subsequently these events are processed by the smartcontract_log_parser.go and cause incorrect accounting or actions.

Tools Used

Manual Review

Recommended Mitigation Steps

Update the event emission logic to ensure that the incorrect events are not emitted even in case of Ether transfer failures. Adding return statement after the bounce back transfer will avoid functions to emit unnesearry events.

1. transferOut()

    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 {
      // <!-- SKIP -->
      }
      emit TransferOut(msg.sender, to, asset, safeAmount, memo);
    }

2. _transferOutV5()

    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.
+         return;
        }
      } else {
        // <!-- SKIP -->
      }

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

3. transferOutAndCall()

    function transferOutAndCall(
      address payable aggregator,
      address finalToken,
      address to,
      uint256 amountOutMin,
      string memory memo
    ) public payable nonReentrant {
      uint256 _safeAmount = msg.value;
      // <!-- SKIP -->
      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.
+         return;
        }
      }

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

4. _transferOutAndCallV5()

  function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
  ) private {
    if (aggregationPayload.fromAsset == address(0)) {
      // <!-- SKIP -->
      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.
+         return;
        }
      }

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

Assessed type

Other

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 changed the severity to 3 (High Risk)