search

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

6 stars 3 forks source link

batchTransferOutAndCallV5 cannot work correctly with ETH transfers #61

Closed howlbot-integration[bot] closed 3 months ago

howlbot-integration[bot] commented 3 months ago

Lines of code

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

Vulnerability details

Impact

batchTransferOutAndCallV5 uses a loop of _transferOutAndCallV5 to batch multiple transactions together.

However, _transferOutAndCallV5 uses msg.value whenever aggregationPayload.fromAsset == address(0) and sends the entire msg.value amount to the target. This is a breaking issue in a batch, since there can be multiple transactions involving ETH, but only one msg.value. The first transaction where ETH is involved will take the entire ETH balance of the batch, breaking the function on the second loop.

Proof of Concept

Assume Batch with transaction A from Alice & B from Bob A: aggregationPayload.fromAsset == address(0), msg.value = 1 ETH B: aggregationPayload.fromAsset == address(0), msg.value = 1 ETH

batchTransferOutAndCallV5 is called with both transactions and a total msg.value of 2 ETH.

Loop 1: The function sends Alice msg.value, which is 2 ETH. Loop 2: The function tries to send Bob the same amount, 2 ETH, but since there is no longer any ETH, the transaction reverts due to insufficient gas/eth.

Tools Used

Manual review

Recommended Mitigation Steps

Use aggregationPayload.fromAmount and not msg.value.

Assessed type

Loop

c4-judge commented 3 months ago

trust1995 marked the issue as satisfactory