Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L196 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L213 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L280 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L326

Vulnerability details

Impact

In cases where the recipient fails to receives native funds being transferred to him via any of the transfer out functions, the system does not have a way of knowing that the transfer have indeed failed. This causes the funds to stay in the vault and the recipient loses the funds.

Proof of Concept

Whenever a vault carries out a transfer out transaction that sends native asset to an address and this operation fails the native asset is returned to the caller but the event is emitted like the transaction is successful.

Example:

User calls depositWithExpiry() on Ethereum to transfer funds.
The system picks up the deposit event and processes the tx.
The user must receive native funds on another chain but the native currency transfer that uses internal send call fails due to OOG error. Then the code sends the funds to the msg.sender which is the vault.

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

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 TransferOut event is emitted which will indicate to the system that the transaction is completed but in fact the user never received the funds.
This results in a loss for the user.

Tools Used

Manual review

Recommended Mitigation Steps

Instead of refunding the funds to the vault and emitting an event either revert when the native assets cannot be transferred to the recipient or change the event parameters to indicate for this failure and occurred refund.

Assessed type

Other

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

Refund functionality breaks protocol logic #70