Using msg.value inside a loop breaks function functionality

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

When batchTransferOutAndCallV5() is called with 2 native currency transfers either contract native assets will get stolen (if there are any inside the contract) or the function will always revert.

Proof of Concept

The root of the problem is that msg.value is used inside a loop. When this is done the contract receives for example 1 ETH but on each iteration of the loop msg.value is 1 ETH.

This will cause the contract to try sending on each iteration 1 ETH by spending its own funds and not only the funds it had received. Since the contract is not expected to hold ETH the function will revert on the second loop iteration since there won't be enough ETH to be sent.

Example:

1. We call batchTransferOutAndCallV5() with [{fromAsset: address(0), fromAmount: 5e17...}, {fromAsset: address(0), fromAmount: 5e17...}] and we send 1e18 ETH with the function call.
2. On the first iteration the function will make a swap with the whole msg.value amount - thus using for the first recipient the whole 1 ETH despite the defined fromAmount value: Snippet: link

  function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
  ) private {
    if (aggregationPayload.fromAsset == address(0)) {
      // call swapOutV5 with ether
      (bool swapOutSuccess, ) = aggregationPayload.target.call{
@>      value: msg.value // @audit the value here is 1 ETH
      }(
        abi.encodeWithSignature(
          "swapOutV5(address,uint256,address,address,uint256,bytes,string)",
          aggregationPayload.fromAsset,
          aggregationPayload.fromAmount, // @audit this value gets ignored when fromAsset == address(0)
          aggregationPayload.toAsset,
          aggregationPayload.recipient,
          aggregationPayload.amountOutMin,
          aggregationPayload.payload,
          aggregationPayload.originAddress
        )
      );

      if (!swapOutSuccess) {
        bool sendSuccess = payable(aggregationPayload.target).send(msg.value); // If can't swap, just send the recipient the gas asset
        if (!sendSuccess) {
          payable(address(msg.sender)).transfer(msg.value); // For failure, bounce back to vault & continue.
        }
      }

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

We can see the swapOutV5 is using only msg.value and ignores fromAmount value:

  function swapOutV5(
    address fromAsset,
    uint256 fromAmount,
    address toAsset,
    address recipient,
    uint256 amountOutMin,
    bytes memory payload,
    string memory originAddress
  ) public payable nonReentrant {
    address[] memory path = new address[](2);

    // you can do something with originAddress: https://gitlab.com/thorchain/thornode/-/merge_requests/3339#note_1884273837

    if (fromAsset == address(0)) {
      // received ETH, swap to toToken or do as you please
      path[0] = WETH;
      path[1] = toAsset;

      if (payload.length == 0) {
        // no payload, process without wasting gas
@>      swapRouter.swapExactETHForTokens{value: msg.value}(
@>        amountOutMin,
          path,
          recipient,
          type(uint).max
        );
      } else {
        // do something with your payload like parse to an aggregator specific structure
        swapRouter.swapExactETHForTokens{value: msg.value}(
          amountOutMin,
          path,
          recipient,
          type(uint).max
        );
      }
    } else {
      ...
    }
  }

4. The second loop iteration will revert if there isn't 1 ETH in the contract or if there is it will make the swap by using contract's funds.

Tools Used

Manual Review

Recommended Mitigation Steps

Use fromAmount instead of msg.value and inside batchTransferOutAndCallV5() place a check that verifies that the sum of all native fromAmount values in the function parameters is equal to msg.value. By using fromAmount each loop will use the correct amount for the swap and we will make sure that _transferOutAndCallV5() will only use the native funds that were sent with the same transaction.

Assessed type

ETH-Transfer

[c4-judge]

trust1995 changed the severity to 2 (Med Risk)

[c4-judge]

trust1995 marked the issue as satisfactory