Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/main/ethereum/contracts/THORChain_Router.sol#L310 https://github.com/code-423n4/2024-06-thorchain/blob/main/ethereum/contracts/THORChain_Router.sol#L324 https://github.com/code-423n4/2024-06-thorchain/blob/main/ethereum/contracts/THORChain_Router.sol#L326

Vulnerability details

Impact

The msg.value is indirectly used inside a for loop. This will make the THORChain_Router::batchTransferOutAndCallV5 always revert when multiple transfers of ETH to tokens are made. The whole msg.value is transferred to the target (THORChain_Aggregator) and for the next iteration there is no gas asset available, making the call always revert.

THORChain_Router::batchTransferOutAndCallV5 will always revert if called with multiple TransferOutAndCallData that swaps gas asset for tokens

Proof of Concept

To assert that the call will always revert add this test to the bottom of the 3_Batch.js:

it('Should fail on batch ETH for tokens', async function () {
  /*
    This will always fail because msg.value is used in the loop.
    It sends all the gas asset on the first iteration.
    On the second iteration the function is trying to send the msg.value again,
    but it's already been sent.
  */
  await truffleAssert.reverts(
    ROUTER.batchTransferOutAndCallV5(
      [
        [AGG.address, ETH, _1, TOKEN.address, USER1, '0', '', '0x', 'bc123'],
        [AGG.address, ETH, _1, TOKEN.address, USER2, '0', '', '0x', 'bc123'],
      ],
      { from: ASGARD, value: _2 }
    ),
    'Transaction reverted: function call failed to execute'
  );
});

Tools Used

Manual Review

Recommended Mitigation Steps

Inside THORChain_Router::_transferOutAndCallV5 use the amount from the argument, not msg.value

 function _transferOutAndCallV5(TransferOutAndCallData calldata aggregationPayload) private {
        if (aggregationPayload.fromAsset == address(0)) {
            // call swapOutV5 with ether
-            (bool swapOutSuccess,) = aggregationPayload.target.call{value: msg.value}(
+            (bool swapOutSuccess,) = aggregationPayload.target.call{value: aggregationPayload.fromAmount}(

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,

Assessed type

Loop

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

[M-03] Indirect use of `msg.value` in for loop, causing DoS of `THORChain_Router::batchTransferOutAndCallV5` #54