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.
User calls batchTransferOutAndCallV5 with an array of 2 elements both of which have aggregationPayload.fromAsset set as address(0)
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
Since on the very first iteration inside the _transferOutAndCallV5() the call will forward all X + Y ETH to the aggregator (L309-L311)
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
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, sincebatchTransferOutAndCallV5
uses a for-loop andmsg.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.batchTransferOutAndCallV5
with an array of 2 elements both of which haveaggregationPayload.fromAsset
set asaddress(0)
X
for call1 andY
for call2 - which means the caller needs to sendX + Y
amount of ETH alongside the tx_transferOutAndCallV5()
the call will forward allX + Y
ETH to the aggregator (L309-L311)Tools Used
Manual review
Recommended Mitigation Steps
Use
value: aggregationPayload.fromAmount
instead ofvalue: msg.value
Assessed type
Other