`batchTransferOutAndCallV5` will be unusable due to msg.value in a loop type situation.

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e5ae503d0dc2394a82242be6860eb538345152a1/ethereum/contracts/THORChain_Router.sol#L401 https://github.com/code-423n4/2024-06-thorchain/blob/e5ae503d0dc2394a82242be6860eb538345152a1/ethereum/contracts/THORChain_Router.sol#L307-L311

Vulnerability details

Impact

Calling batchTransferOutAndCallV5 function and transferring ETH in a batch will be impossible due to the fact that the internally called _transferOutAndCallV5 nakedly handles msg.value, causing that the function will always fail after the first ETH transfer iteration.

Proof of Concept

1. Context

When msg.value is handled in a loop, the entire ETH sent is transferred in the first loop, so upon subsequent loops, this time the second, the amount of ETH sent by the sender left will now be zero while the function attempts to send the msg.value again but this time, the transaction will fail and revert breaking the batchTransferOutAndCallV5 functionality.

2. Bug location

Taking a look at the batchTransferOutAndCallV5 function, the _transferOutAndCallV5 function is called in a for loop while looping through the aggregationPayloads.

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

The _transferOutAndCallV5 function queries if the asset is ETH, upon which msg.value is sent to the aggregationPayload target.

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

3. Final effect

As has been established in the impact section, after the first iteration of sending ETH to the aggregationPayload target, further attempts to send ETH to the next aggregationPayload targets will fail as msg.value will be attempted to be sent again, causing a reversion as balance is not enough. This breaks the batchTransferOutAndCallV5 function, rendering it useless.

If for instance batchTransferOutAndCallV5 is called to transfer 2 ETH to 2 targets at 1 ETH each, the first iteration will send msg.value == which is 2 ETH to the first target in the first loop. And as we have established the msg.value will always remain at 2 ETH throughout the entire execution of the batchTransferOutAndCallV5 function. The msg.value will not automatically be reduced regardless of how many ETH has been transferred. Hence upon the second iteration an attempt is made to transfer another 2 ETH to the second target which will fail as the total remaining ETH sent is now 0, causing the entire function to revert.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider fixing this by using the aggregatorPayload.fromAmount parameter instead as is done in the _transferOutV5 function.

Assessed type

Loop