Use of msg.value in _transferOutAndCallV5 loop breaks batchTransferOutAndCallV5 batch functionality.

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

The payable external THORChain_Router.batchTransferOutAndCallV5 function uses the internal _transferOutAndCallV5 function for batch TransferOutAndCallV5 operations, which uses msg.value when calling the aggregator's swapOutV5 function on line 399. All ether sent using msg.value will effectively be used in the first iteration of the _transferOutAndCallV5 loop on line 401, and only 1 call to swapOutV5 takes place:

    //File:ethereum/contracts/THORChain_Router.sol

    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)",
            ...
        }
    }    

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

It is understood that THORChain_Router.batchTransferOutAndCallV5 is intended to be called by Bifrost components. Assuming normal usage of THORChain_Router.batchTransferOutAndCallV5 (nonzero msg.value amounts and an aggregationPayloads argument with more than 1 element), this will cause all subsequent loop iterations to revert, breaking batchTransferOutAndCallV5 functionality which may constitute as DoS. The same will occur if any other contract/EOA ever needs to call THORChain_Router.batchTransferOutAndCallV5 to process batch transactions.

Proof of Concept

The following Forge PoC BatchTransferCallV5.t.sol shows the impact of this finding:

```solidity // SPDX-License-Identifier: NONE pragma solidity >=0.6.0 <0.9.0; import "lib/forge-std/src/Test.sol"; import "lib/forge-std/src/Vm.sol"; import "lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; import "contracts/THORChain_Router.sol"; contract BatchTransferOutAndCallV5Test is Test { THORChain_Router router1; address deployer; address bifrost; function setUp() public { deployer = makeAddr("deployer"); bifrost = makeAddr("bifrost"); vm.startPrank(deployer); router1 = new THORChain_Router(); vm.stopPrank(); } function test_audit_batchTransferOutAndCallV5() public { MockAggregator mag = new MockAggregator(); vm.startPrank(bifrost); // simulate bifrost calling batchTransferOutAndCallV5 // setting TransferOutAndCallData argument THORChain_Router.TransferOutAndCallData[] memory agps = new THORChain_Router.TransferOutAndCallData[](2); THORChain_Router.TransferOutAndCallData memory agp; agp.target = payable(address(mag)); agp.fromAsset = address(0); agp.fromAmount = 1 ether; agp.toAsset = address(123); agp.recipient = address(456); agp.amountOutMin = 0 ether; agp.payload = ""; agp.originAddress = ""; // 2 loop iterations, suppose each loop takes 1 ether agps[0] = agp; agps[1] = agp; // will rever because first loop iteration spends all of msg.value vm.deal(bifrost, 10 ether); assertEq(address(bifrost).balance, 10 ether); vm.expectRevert(); // will revert with EvmError: OutOfFunds router1.batchTransferOutAndCallV5{value: 1 ether}(agps); // any amount of ether per loop will revert. vm.deal(bifrost, 10 ether); assertEq(address(bifrost).balance, 10 ether); vm.expectRevert(); router1.batchTransferOutAndCallV5{value: 10 ether}(agps); vm.stopPrank(); } } contract MockAggregator { constructor() {} function swapOutV5( address fromAsset, uint256 fromAmount, address toAsset, address recipient, uint256 amountOutMin, bytes memory payload, string memory originAddress ) public payable {} receive() external payable {} } ```

1. In the ethereum/test directory, run the following command to install the Foundry Hardat package.

   npm install --save-dev @nomicfoundation/hardhat-foundry

2. Add the following line to the requires in ethereum/hardhat.config.js:

   require("@nomicfoundation/hardhat-foundry");

3. Run The following command to initiate a Forge project.

   npx hardhat init-foundry

4. Add the Forge PoC to the ethereum/test directory.
5. Run the test with the following command:

   
   $ forge test --match-test test_audit_batchTransferOutAndCallV5      
   [⠊] Compiling...
   No files changed, compilation skipped

Ran 1 test for test/BatchTransferCallV5.t.sol:BatchTransferOutAndCallV5Test [PASS] test_audit_batchTransferOutAndCallV5() (gas: 285918) Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 691.13µs (176.38µs CPU time)

Ran 1 test suite in 2.98ms (691.13µs CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

## Tools Used
Foundry, VS Code

## Recommended Mitigation Steps
- Avoid using `msg.value` to send ether in loops.
- If possible, consider using dynamically calculated `msg.value` amounts derived from an outer call's `msg.value` amount in nested calls involving loop logic. Ensure that these dynamic values are correctly implemented to avoid sending incorrect ether amounts per loop iteration.

## Assessed type

Loop

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked issue #44 as primary and marked this issue as a duplicate of 44

[hGq2Wnl]

Hello trust1995, thanks for judging this. May I request for this finding #7 to be reconsidered as the primary issue/selected for report instead of #44 due to the following reasons?

The primary impact and PoC explained in #44 appears to be that Ether can be stolen from the contract after the contract amasses a large Ether balance:

"Consider a scenario where the Router's balance is 100e18 ethers."

However by invalidating findings such as #49, it has been established that the router should not hold Ether.

10 also appears to have been invalidated as users are expected not send invalid Ether amounts to the Router. This is also assumed to be why the [#9] QA report was invalidated, which included one issue similar to #10 and another showing how Ether can accidentally get stuck in the contract due to missing native asset checks.

The mainnet Router also does not currently hold any Ether, so the chances of the Router gaining a substantial Ether balance appears slim (this is also why the issues in QA report #9 were raised as nothing more than low/QA severity).

However #44 does appear to be a duplicate of this issue #7, as the root cause is correctly called out as the use of msg.value in a loop in #44, and #44 does state that the batchTransferOutAndCallV5 function will revert as a secondary impact:

"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 finding #7 however focuses on the primary impact of the batchTransferOutAndCallV5 function reverting, with a PoC, and appropriately does not reference the primary impact as high severity.

Thank you for your consideration.

[trust1995]

Hi, The mitigation section in #44 is far superior, which is why it was chosen in the end.