howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

Impact

An attacker could potentially exploit the vulnerability by repeatedly calling the function that contains the loop with a small amount of Ether each time. This could lead to draining a significant amount of Ether from the contract, as the loop would transfer the Ether to the attacker's address each time it is executed. This could result in a loss of funds for the contract owner or users interacting with the contract.

Proof of Concept

function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
) private {
    if (aggregationPayload.fromAsset == address(0)) {
        // call swapOutV5 with ether
        (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(msg.value); // If can't swap, just send the recipient the gas asset
            if (!sendSuccess) {
                payable(address(msg.sender)).transfer(msg.value); // For failure, bounce back to vault &amp; continue.
            }
        }

        // ... (rest of the code)
    } else {
        // ... (rest of the code)
    }
}

The vulnerability lies in the line

payable(address(msg.sender)).transfer(msg.value);

. This line transfers the entire

msg.value

back to the sender, which can lead to a reentrancy attack if the recipient contract is malicious.

To rectify this vulnerability, you should avoid transferring the entire

msg.value

in a single transaction. Instead, you can implement a pull-based payment system where the recipient contract must explicitly request the funds, and the contract can enforce limits on the amount that can be withdrawn.

Tools Used

SET IN STONE : https://lab.setinstone.io

Recommended Mitigation Steps

Here's an example of how you can modify the code:

mapping(address =&gt; uint256) private pendingWithdrawals;

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

        if (!swapOutSuccess) {
            pendingWithdrawals[msg.sender] += msg.value; // Store the pending withdrawal amount
        }

        // ... (rest of the code)
    } else {
        // ... (rest of the code)
    }
}

function withdrawPendingFunds() external {
    uint256 amount = pendingWithdrawals[msg.sender];
    require(amount &gt; 0, "No pending withdrawals");

    pendingWithdrawals[msg.sender] = 0;
    payable(msg.sender).transfer(amount);
}

In this modified code, instead of transferring the entire msg.value

back to the sender, we store the pending withdrawal amount in a mapping

pendingWithdrawals. The sender can then explicitly call thewithdrawPendingFunds

function to withdraw their pending funds. This way, the contract can enforce limits on the amount that can be withdrawn, and the reentrancy attack is mitigated.

Assessed type

Loop

c4-judge commented 4 months ago

trust1995 changed the severity to 2 (Med Risk)