Lines of code

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

Vulnerability details

The batchTransferOutAndCallV5 is implemented to allow users to use the _transferOutAndCallV5() in a batch call. The _transferOutAndCallV5() uses msg.value very irresponsibly which will result in the batch function not working or emitting wrong events. As we can see that the function when calls the target it passes the whole msg.value, so when this function will be called in a batch to transfer ETH to multiple addresses then this will simply revert, as it in the first loop call it will consume all the msg.value, no ethers will be left for the 2nd loop iteration.

Then the function emits the msg.value as an event as well, which is totally wrong as explained above and will be breaking an important protocol's variant: "Only valid events (should be) emitted from the Router contract"

  function _transferOutAndCallV5(TransferOutAndCallData calldata aggregationPayload) private {
    if (aggregationPayload.fromAsset == address(0)) {
      (bool swapOutSuccess, ) = aggregationPayload.target.call{
@>      value: msg.value ///@audit full msgValue used in case of batch!
      }(
        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, ///@audit-issue M wrong event emited! this should be aggpayload.fromAmount not msg.value, in the case of batch functionality this can be provlematic
        aggregationPayload.toAsset,
        aggregationPayload.recipient,
        aggregationPayload.amountOutMin,
        aggregationPayload.memo,
        aggregationPayload.payload,
        aggregationPayload.originAddress
      );
    } else {
      .....
    }
  }

Impact

Broken Functionality
Wrong Events Emissions

Proof of Concept

    it("Should reverts batchTransferOutAndCallV5", async function () {

      await truffleAssert.reverts(ROUTER.batchTransferOutAndCallV5(
        [
          [
            AGG.address,
            ETH,
            _1,
            TOKEN.address,
            USER1,
            "0",
            "OUT:HASH",
            "0x", 
            "bc123",
          ],
          [
            AGG.address,
            ETH,
            _1,
            TOKEN.address,
            USER2,
            "0",
            "OUT:HASH",
            "0x",
            "bc123",
          ],
        ],
        { from: ASGARD, value: _2 },
      ),
      "Transaction reverted: function call failed to execute",
    );;

    });

Tools Used

Shaheen's dk

Recommended Mitigation Steps

Do not use msg.value, use aggregationPayload.fromAmount instead.

  function _transferOutAndCallV5(TransferOutAndCallData calldata aggregationPayload) private {
    if (aggregationPayload.fromAsset == address(0)) {
      (bool swapOutSuccess, ) = aggregationPayload.target.call{
-      value: msg.value
+      aggregationPayload.fromAmount
      }(
        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.fromAmount, 
        aggregationPayload.toAsset,
        aggregationPayload.recipient,
        aggregationPayload.amountOutMin,
        aggregationPayload.memo,
        aggregationPayload.payload,
        aggregationPayload.originAddress
      );
    } else {
      .....
    }
  }

Assessed type

Other

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

`batchTransferOutAndCallV5` function is broken, it will not work in some cases and in some cases it will emit wrong event values. #34