search

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

6 stars 3 forks source link

The batchTransferOutAndCallV5 function can never handle batch operations with ETH #3

Closed c4-bot-9 closed 3 months ago

c4-bot-9 commented 3 months ago

Lines of code

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

Vulnerability details

Impact

The batchTransferOutAndCallV5 function is used for batch processing of _transferOutAndCallV5 calls. However, due to the implementation of _transferOutAndCallV5 not supporting batch calls for ETH, batch processing of batchTransferOutAndCallV5 with ETH may fail.

Proof of Concept

If the asset is ETH, _transferOutAndCallV5 will pass in the entire msg.value to target.This may result in insufficient ETH for payment in the following ETH processing github:https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/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
      }(
        abi.encodeWithSignature(
          "swapOutV5(address,uint256,address,address,uint256,bytes,string)",
          aggregationPayload.fromAsset,
          aggregationPayload.fromAmount,
          aggregationPayload.toAsset,
          aggregationPayload.recipient,
          aggregationPayload.amountOutMin,
          aggregationPayload.payload,
          aggregationPayload.originAddress
        )
      );
  1. Assuming the following situation, it is necessary to batch process two ETH requests
  2. request1 fromAmount = 1e18 toAsset=USDC recipient = addr1 request2 fromAmount = 2e18 toAsset=DAI recipect = addr2
  3. call batchTransferOutAndCallV5 msg.value = 3e18
  4. When executing request1, all 3e18 ETH will be passed in aggregationPayload.target, swaped to USDC, and request2 will not have enough ETH to pass in.

Tools Used

Manual audit

Recommended Mitigation Steps

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
        )
      );

Assessed type

ETH-Transfer

c4-judge commented 3 months ago

trust1995 marked the issue as satisfactory