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.

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

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

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

_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

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

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