Lines of code

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

Vulnerability details

Impact

In the function _transferOutAndCallV5, using msg.value when fromAssets is address(0) sends Ether to the aggregationPayload.target. If this function is called multiple times within batchTransferOutAndCallV5, the repeated use of msg.value can cause the transaction to fail due to issues with transferring Ether multiple times.

Proof of Concept

File: https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L310
  function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
  ) private {
    if (aggregationPayload.fromAsset == address(0)) {
      // call swapOutV5 with ether
      (bool swapOutSuccess, ) = aggregationPayload.target.call{
        value: msg.value // @audit 
      }(
        abi.encodeWithSignature(
          "swapOutV5(address,uint256,address,address,uint256,bytes,string)",
          aggregationPayload.fromAsset,
          aggregationPayload.fromAmount,
          aggregationPayload.toAsset,
          aggregationPayload.recipient,
          aggregationPayload.amountOutMin,
          aggregationPayload.payload,
          aggregationPayload.originAddress
        )
      );
      ...
  }
File: https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L397-L403
  function batchTransferOutAndCallV5(
    TransferOutAndCallData[] calldata aggregationPayloads
  ) external payable nonReentrant {
    for (uint i = 0; i < aggregationPayloads.length; ++i) {
      _transferOutAndCallV5(aggregationPayloads[i]); // @audit sends multiple msg.value
    }
  }

Tools Used

Manual review

Recommended Mitigation Steps

Use to determine the amount of Ether to send aggregationPayload.fromAmount instead of msg.value in _transferOutAndCallV5 function. Additionally, after processing all calls, validate if there is any remaining Ether and send it back to the msg.sender in transferOutAndCallV5 & batchTransferOutAndCallV5.

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

Assessed type

ETH-Transfer

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

Passing multiple msg.value in the _transferOutAndCallV5 function can cause batchTransferOutAndCallV5 to fail #78