Lines of code

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

Vulnerability details

Impact

The protocol has implemented a function batchTransferOutAndCallV5 that can do multiple transfers out. Instead of calling multiple times transferOutAndCallV5, you can call batchTransferOutAndCallV5 function. However when more than one native transfers occur, this tx will fail.

Proof of Concept

As can be seen, the function is payable and uses for loop to call the internal _transferOutAndCallV5 function.

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

The problem is that that internal function uses msg.value instead of aggreagationPayload.fromAmount.

  function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
  ) private {
    if (aggregationPayload.fromAsset == address(0)) {
      // call swapOutV5 with ether
      (bool swapOutSuccess, ) = aggregationPayload.target.call{
        value: msg.value // @audit use of 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
        )
      );
      if (!swapOutSuccess) {
        bool sendSuccess = payable(aggregationPayload.target).send(msg.value);  // @audit use of msg.value
        if (!sendSuccess) {
          payable(address(msg.sender)).transfer(msg.value); // @audit use of msg.value
        }
      }

      emit TransferOutAndCallV5(
        msg.sender,
        aggregationPayload.target,
        msg.value,
        aggregationPayload.toAsset,
        aggregationPayload.recipient,
        aggregationPayload.amountOutMin,
        aggregationPayload.memo,
        aggregationPayload.payload,
        aggregationPayload.originAddress
      );
    } else {
      // ERC20 transfer
    }
  }

If there are more than 1 native transfers, the first one will use all of the msg.value. When it's time to execute the second one, the tx will fail, because there is no native tokens left.

Tools Used

Manual Review

Recommended Mitigation Steps

Use aggreagationPayload.fromAmount instead of msg.value when transferring native.

Assessed type

ETH-Transfer

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

Batch transfers will not work. #51