howlbot-integration[bot] commented 3 months ago

Lines of code

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

Vulnerability details

Impact

The batchTransferOutAndCallV5 function has a vulnerability that causes all the msg.value to be sent to the first target in the loop, leaving subsequent targets without the intended funds. This occurs because the msg.value is not correctly allocated per payload, leading to mismanagement of Ether distribution.

Vulnerability Details

In the batchTransferOutAndCallV5 function, the Ether transfer within the loop is not handled properly. When calling this function with multiple aggregationPayloads, the entire msg.value is sent to the first target, leaving subsequent targets without the intended funds.

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

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

Proof of Concept

A user calls batchTransferOutAndCallV5 with multiple aggregationPayloads, expecting each to receive a portion of the msg.value.
The entire msg.value is sent to the first payload's target.
Subsequent payloads do not receive any Ether, potentially causing failed transactions and unexpected behavior.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, ensure that the correct amount of Ether is allocated for each payload. Instead of sending the entire msg.value, use aggregationPayload.fromAmount for the Ether transfer. This change ensures that each payload receives the appropriate amount of funds, preventing the exhaustion of the contract's Ether on the first target.

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

      (bool swapOutSuccess, ) = aggregationPayload.target.call{
+        aggregationPayload.fromAmount
      }(

-            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.
-                }

+            if (!swapOutSuccess) {
+                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(aggregationPayload.fromAmount); // For failure, bounce back to vault & continue.
+                }

Assessed type

ETH-Transfer

c4-judge commented 3 months ago

trust1995 changed the severity to 2 (Med Risk)