howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

The batchTransferOutAndCallV5() function in THORChain_Router is designed to perform a batch of transferAndCall operations on target contracts, which execute swaps and transfers to Layer-1 chains. However, the transferAndCall() function is not designed to process multiple ETH transfers in a single transaction. Due to this issue, batchTransferOutAndCallV5 will always revert if there is more than one ETH transfer.

Impact

This issue results in the inability to process outbound batch transfers that include more than one ETH transfer on Thorchain by the vault.

Proof of Concept

The batchTransferOutAndCallV5 function is designed to perform multiple transferAndCall operations in a single transaction. It calls _transferOutAndCallV5 for each element in the aggregationPayloads array.

  function batchTransferOutAndCallV5(
    TransferOutAndCallData[] calldata aggregationPayloads
  ) external payable nonReentrant {
    for (uint i = 0; i < aggregationPayloads.length; ++i) {
@>>   _transferOutAndCallV5(aggregationPayloads[i]);
    }
  }

Link to the relevant code

In _transferOutAndCallV5, if the fromAsset is ETH (i.e., address(0)), it uses msg.value for the transfer. msg.value represents the total ETH sent with the transaction. In a loop, each iteration will attempt to use the entire msg.value, meaning the first ETH transfer will deplete the total msg.value, leaving no ETH for subsequent transfers.

  function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
  ) private {
    if (aggregationPayload.fromAsset == address(0)) {
      // call swapOutV5 with ether
@>>   (bool swapOutSuccess, ) = aggregationPayload.target.call{
@>>     value: msg.value
      }(

Link to the relevant code

When batchTransferOutAndCallV5 tries to perform more than one ETH transfer, the second and any subsequent transfers will fail because there is no ETH left. This will cause the entire transaction to revert.

Tools Used

Manual Review

Recommended Mitigation Steps

To fix this issue, use aggregationPayload.fromAmount instead of msg.value in the _transferOutAndCallV5 function. This way, each transfer uses the specified amount for that particular transfer, preventing reverts due to insufficient ETH for subsequent transfers.

  function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
  ) private {
    if (aggregationPayload.fromAsset == address(0)) {
      // call swapOutV5 with ether
      (bool swapOutSuccess, ) = aggregationPayload.target.call{
-       value: msg.value
+       value: aggregationPayload.fromAmount
      }(

Assessed type

ETH-Transfer

c4-judge commented 4 months ago

trust1995 changed the severity to 2 (Med Risk)