search

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

6 stars 3 forks source link

`batchTransferOutAndCallV5` might revert if there are more than 2 calls, which transfer ETH #39

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/main/chain/ethereum/contracts/THORChain_Router.sol#L309-L311

Vulnerability details

Impact

If batchTransferOutAndCallV5 is called and 2 or more of those are transfers of native ETH, the tx will revert, since batchTransferOutAndCallV5 uses a for-loop and msg.value is preserved through all iterations of the loop.

Proof of Concept

A user wants to make multiple swap via the router and batchTransferOutAndCallV5 function.

  1. User calls batchTransferOutAndCallV5 with an array of 2 elements both of which have aggregationPayload.fromAsset set as address(0)
  2. The amount of ETH can be X for call1 and Y for call2 - which means the caller needs to send X + Y amount of ETH alongside the tx
  3. Since on the very first iteration inside the _transferOutAndCallV5() the call will forward all X + Y ETH to the aggregator (L309-L311)
  4. On the next iteration it tries to do the same, but the ETH amount inside the router contract is already 0

Tools Used

Manual review

Recommended Mitigation Steps

Use value: aggregationPayload.fromAmount instead of value: msg.value

Assessed type

Other

c4-judge commented 3 months ago

trust1995 marked the issue as partial-75