Lines of code

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

Vulnerability details

Impact

The Router contract should not hold ETH, which will be sent to the vault. However, due to incomplete function verification, the Router contract may hold ETH

Proof of Concept

When calling transferOutV5 function, there is no check to see if the sum of the amount field in the TransferOutData[] array is equal to msg.vaule. If msg. vaule is greater than sum (amount), there will be some ETH stuck in the contract github:[1]

  function batchTransferOutV5(
    TransferOutData[] calldata transferOutPayload
  ) external payable nonReentrant {
    for (uint i = 0; i < transferOutPayload.length; ++i) {
      _transferOutV5(transferOutPayload[i]);
    }
  }

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

  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{
        ...
    }

Assuming the following scenario:

there is a transferOutV5 call with an assign of ETH and an amount of 1e18.
If an msg.vaule of 2e18 is passed in during the call

there will be 1e18 of ETH stuck in the contract

Tools Used

Manual review

Recommended Mitigation Steps

Check if msg.vaule is equal to amount before calling _transferOutV5

function transferOutV5(
TransferOutData calldata transferOutPayload
) public payable nonReentrant {
+    if(transferOutPayload.assert == address(0)){
+        require(transferOutPayload.amount == msg.vaule);
+    }
_transferOutV5(transferOutPayload);
}

function batchTransferOutV5(
TransferOutData[] calldata transferOutPayload
) external payable nonReentrant {
+    uint256 totalETHAmount = 0;
+    for (uint i = 0; i < transferOutPayload.length; ++i) {
+       if(transferOutPayload[i].assert == address(0)){
+          totalETHAmount += transferOutPayload[i].amount;
+       }
+    }
+    require(totalETHAmount == msg.sender);
for (uint i = 0; i < transferOutPayload.length; ++i) {
  _transferOutV5(transferOutPayload[i]);
}
}

Assessed type

Access Control

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

Due to incomplete check, in some cases, Router contracts may hold some ETH #5