Any Eth in router contract can be stolen by a malicious user

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/733dbe7cd7eef0dffc5e8a2d02e36bf74b196eff/ethereum/contracts/THORChain_Router.sol#L246-L250 https://github.com/code-423n4/2024-06-thorchain/blob/733dbe7cd7eef0dffc5e8a2d02e36bf74b196eff/ethereum/contracts/THORChain_Router.sol#L255-L259

Vulnerability details

Impact

Any ETH contained in the THORChain_Router either sent to the contract mistakenly or any other means can be stolen by a malicious vault. This would be achieved using the transferOutV5 or batchTransferOutV5 function.

Proof of Concept

THORChain_Router::transferOutV5 allows users transfer of ETH to other vaults. This calls _transferOutV5 as can be seen below.

function transferOutV5(
    TransferOutData calldata transferOutPayload
  ) public payable nonReentrant {
    _transferOutV5(transferOutPayload);
  }

Within _transferOutV5, the amount entered in transferOutPayload is transferred to the recipient address.

function _transferOutV5(TransferOutData memory transferOutPayload) private {
    if (transferOutPayload.asset == address(0)) {
      //audit: This allows vault to send eth from this address rather than the calling vault
      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.
      } .............................................

The amount of ETH sent is not dependent on the msg.value but rather on the vault's crafted calldata transferOutPayload. Therefore, by inputting the value of ETH in the THORChain_Router contract, the vault can send the ETH out of the Router contract to any recipient address which can be another contract controlled by them.

Tools Used

Manual Review

Recommended Mitigation Steps

1. For single and batch transferOutV5transactions, If token to transfer is ETH, then msg.value should be cached as a safeAmount and then substracted on every iteration. If safeAmount gets below 0, then msg.value sent with the transaction is exhausted and then transaction should continue or revert if msg.value is insufficient.

function _transferOutV5(TransferOutData memory transferOutPayload) private {
    if (transferOutPayload.asset == address(0)) {
+      int256 safeAmount = msg.value;
     bool success = transferOutPayload.to.send(transferOutPayload.amount); // Send ETH.
+      if(success) { 
+          safeAmount -= transferOutPayload.amount;
+          if(safeAmount < 0) {
+             return; 
          }
       }
      if (!success) {
        payable(address(msg.sender)).transfer(transferOutPayload.amount); // For failure, bounce back to vault & continue.
      }

Assessed type

ETH-Transfer

[c4-judge]

trust1995 marked the issue as not a duplicate

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid

[trust1995]

This one does not uncover the impact of the previously dupped issues. The impact is non-existent because the router should not hold funds anyways.