BatchTransferOutAndCallV5 iterate msg value

[howlbot-integration[bot]]

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

Summary

In the current implementation of THORChain_Router::batchTransferOutAndCallV5, the logic is iterating over array of TransferOutAndCallData, but if there are more than one TransferOutAndCallData related to ether transfer, this will not work due to the current implementation.

Proof of concept

If we take a look of the underlying function that is called by THORChain_Router::batchTransferOutAndCallV5 during the iteration, THORChain_Router::_transferOutAndCallV5. We can see that during ether transfer, all of the msg.value will be swapped, but if there are more than one TransferOutAndCallData entries based on ether, the function will be revert all the time. You can take a look at he below code.

function _transferOutAndCallV5(TransferOutAndCallData calldata aggregationPayload) private {

    if (aggregationPayload.fromAsset == address(0)) {

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

    );

Tools Used

Manual review

Impact

The current implementation causes the following issues:

1. Transaction Reversion: If there are multiple TransferOutAndCallData entries involving ether transfers in the batchTransferOutAndCallV5 function, the transaction will revert every time.
2. Inefficiency in Batch Operations: Users attempting to batch multiple ether transfers will encounter failed transactions, rendering the batch processing feature ineffective.
3. Loss of Funds: Although the ether isn't lost permanently due to reversion, it can cause operational delays and confusion, especially in automated or large-scale systems.

Recommended Mitigation Steps

The implementation of the current logic could look like this.

function batchTransferOutAndCallV5(TransferOutAndCallData[] calldata transferData) external payable {
    uint256 totalEtherRequired = 0;

    // Calculate total ether required
    for (uint256 i = 0; i < transferData.length; i++) {
        if (transferData[i].fromAsset == address(0)) {
            totalEtherRequired += transferData[i].fromAmount;
        }
    }

    require(msg.value == totalEtherRequired, "Insufficient ether sent");

    // Execute transfers
    for (uint256 i = 0; i < transferData.length; i++) {
        _transferOutAndCallV5(transferData[i]);
    }
}

function _transferOutAndCallV5(TransferOutAndCallData calldata aggregationPayload
) private {
    if (aggregationPayload.fromAsset == address(0)) {

       require(msg.value == aggregationPayload.fromAmount, "Msg.value is not equal to fromAmount");      
+       (bool swapOutSuccess, ) = aggregationPayload.target.call{ value: aggregationPayload.fromAmount }(abi.encodeWithSignature(
-       (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(aggregationPayload.fromAmount); 
-            bool sendSuccess = payable(aggregationPayload.target).send(msg.value); 
             if (!sendSuccess) {
+             payable(address(msg.sender)).transfer(aggregationPayload.fromAmount); 
-             payable(address(msg.sender)).transfer(aggregationPayload.fromAmount); 
            }
        }

.....

}

Assessed type

Loop

[c4-judge]

trust1995 changed the severity to 2 (Med Risk)

[c4-judge]

trust1995 marked the issue as satisfactory