[H-02] Incorrect recipient inside `THORChain_Router::_transferOutAndCallV5`, leading to sending gas asset to the payload target, not the recipient

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

Inside THORChain_Router::_transferOutAndCallV5 when transferring the gas asset and the call to the aggregationPayload.target fails, the gas asset according to the comment next to the incorrect line, documentation and THORChain_Router::_transferOutV5, should be sent to the recipient, not to the aggregationPayload.target

The recipient will never receive gas asset and the funds of the vault will be lost

Proof of Concept

Please add a test to an existing file, so add a new test file called for example 6_Incorrect_Recipient.js and paste the code from below. The code asserts that USER1 tries to swap ETH for tokens and send them to USER2 but the THORChain_Aggregator::swapOutV5 fails and the Aggregator receives the refund, not USER2 as expected.

const Router = artifacts.require('THORChain_Router');
const FailingAggregator = artifacts.require('THORChain_Failing_Aggregator');

const SushiRouter = artifacts.require('SushiRouterSmol');
const Token = artifacts.require('ERC20Token');
const Weth = artifacts.require('WETH');
const BigNumber = require('bignumber.js');
const { expect } = require('chai');
function BN2Str(BN) {
  return new BigNumber(BN).toFixed();
}
function getBN(BN) {
  return new BigNumber(BN);
}

var ROUTER, ASGARD, FAIL_AGG, WETH, SUSHIROUTER, TOKEN, WETH, USER1, USER2;

const ETH = '0x0000000000000000000000000000000000000000';
const _1 = '1000000000000000000';
const _10 = '10000000000000000000';

describe('Aggregator griefing', function () {
  let accounts;

  before(async function () {
    accounts = await web3.eth.getAccounts();
    ROUTER = await Router.new();
    TOKEN = await Token.new(); // User gets 1m TOKENS during construction
    USER1 = accounts[0];
    USER2 = accounts[1];
    ASGARD = accounts[3];

    WETH = await Weth.new();
    SUSHIROUTER = await SushiRouter.new(WETH.address);
    FAIL_AGG = await FailingAggregator.new(WETH.address, SUSHIROUTER.address);
  });

  it('Should Deposit Assets to Router', async function () {
    await web3.eth.sendTransaction({
      to: SUSHIROUTER.address,
      from: USER1,
      value: _10,
    });
    await web3.eth.sendTransaction({
      to: WETH.address,
      from: USER1,
      value: _10,
    });
    await WETH.transfer(SUSHIROUTER.address, _10);

    expect(BN2Str(await web3.eth.getBalance(SUSHIROUTER.address))).to.equal(
      _10
    );
    expect(BN2Str(await WETH.balanceOf(SUSHIROUTER.address))).to.equal(_10);
  });

  it('Should send funds to the incorrect address', async function () {
    /* Get starting balances of the TOKEN */
    const startingTokenBalanceOfUser1 = await web3.eth.getBalance(USER1);
    const startingTokenBalanceOfUser2 = await web3.eth.getBalance(USER2);
    const startingTokenBalanceOfAggregator = await web3.eth.getBalance(
      FAIL_AGG.address
    );

    /* Log starting balances */
    console.log(
      'Starting TOKEN Balance USER1:',
      BN2Str(startingTokenBalanceOfUser1)
    );
    console.log(
      'Starting TOKEN Balance USER2:',
      BN2Str(startingTokenBalanceOfUser2)
    );
    console.log(
      'Starting TOKEN Balance Aggregator:',
      BN2Str(startingTokenBalanceOfAggregator)
    );

    /* Make a transfer call in which the aggregator call fails */
    await ROUTER.transferOutAndCallV5(
      [
        FAIL_AGG.address,
        ETH,
        _1,
        TOKEN.address,
        USER2,
        '0',
        'MEMO',
        '0x', // empty payload
        'bc123', // dummy address
      ],
      { from: ASGARD, value: _1 }
    );

    /* Get ending balances of the TOKEN */
    const endingTokenBalanceOfUser1 = await web3.eth.getBalance(USER1);
    const endingTokenBalanceOfUser2 = await web3.eth.getBalance(USER2);
    const endingTokenBalanceOfAggregator = await web3.eth.getBalance(
      FAIL_AGG.address
    );

    /* Log ending balances */
    console.log(
      '\nEnding TOKEN Balance USER1:',
      BN2Str(endingTokenBalanceOfUser1)
    );
    console.log(
      'Ending TOKEN Balance USER2:',
      BN2Str(endingTokenBalanceOfUser2)
    );
    console.log(
      'Ending TOKEN Balance Aggregator:',
      BN2Str(endingTokenBalanceOfAggregator)
    );

    /* Assert that USER1 has lost his funds and the AGGREGATOR has gained the funds, not USER2 */
    expect(
      getBN(endingTokenBalanceOfAggregator).isGreaterThan(
        startingTokenBalanceOfAggregator
      )
    ).to.equal(true);
    expect(startingTokenBalanceOfUser2).to.equal(endingTokenBalanceOfUser2);
  });
});

Tools Used

Manual Review

Recommended Mitigation Steps

Change the recipient to the correct one

Inside THORChain_Router::_transferOutAndCallV5 change:

if (!swapOutSuccess) {
- bool sendSuccess = payable(aggregationPayload.target).send(msg.value); // If can't swap, just send the recipient the gas asset
+ bool sendSuccess = payable(aggregationPayload.recipient).send(msg.value); // If can't swap, just send the recipient the gas asset

Assessed type

ETH-Transfer

[the-eridanus]

@olegpetroveth - what do you think here? Seems like the gas asset should just be sent to the user right?

[the-eridanus]

Caught up with @olegpetroveth - this is an intentional design decision, so adding "sponsor disputed"

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid

[trust1995]

The docs should be improved, but it does not qualify as a security risk.

[0xGreed]

I believe the issue should be valid.

It is safer to send the ETH directly to the user because sending them to the aggregator represents a risk for them to be stolen by another party.

As soon as ETH are on the aggregator, anyone can call the swapOutV5() function with an arbitrary recipient as long as the fromAsset is ETH to swap them for another asset. Even though there is a rescue function, the following hurdles are encountered:

• the aggregator owner might take some time to be aware that funds need to be rescued
• another party has the ability to frontrun the rescue transaction to execute swapOutV5() before the rescue occurs

The issue should not be overlooked and should be considered as a risk leading to a loss of funds for the protocol users

[trust1995]

@the-eridanus can you confirm if a failed TX will end up storing the funds in the aggregator, where they can be potentially taken by another party?

[ShaheenRehman]

As soon as ETH are on the aggregator, anyone can call the swapOutV5() function with an arbitrary recipient as long as the fromAsset is ETH to swap them for another asset.

I feel like that this assumption is wrong, the given aggregator contract is just an example implementation. And tbh a very loosely implementation, if we make any assumption based on that contract, there will be ton of other issues

[trust1995]

Because the aggregator is OOS, and this finding depends on a badly implemented aggregator, the finding is OOS indeed.

[c4-judge]

trust1995 marked the issue as unsatisfactory: Out of scope

[0xGreed]

I mean, Uniswap V2 pairs also require tokens to be sent into the contract before swapping the tokens and has no rescue function or mecanism to send back the tokens in case something goes wrong but this does not make it a badly implemented contract. It is how users interact with it that can turn it into a risk.

I agree the aggregator is OOS but the root cause of the identified issue resides in the THORChain_Router contract which is definitely in-scope. Applying the recommended mitigation step would get rid of potential risks related to the interaction with any ("loosely implemented") aggregator and would align with the initial intention of the developers which was to "send the recipient the gas asset".

Doing so, the protocol won't be making "any assumption based on that contract" and would simply take safety measures to discard any unexpected behavior.

[the-eridanus]

I mean, Uniswap V2 pairs also require tokens to be sent into the contract before swapping the tokens and has no rescue function or mecanism to send back the tokens in case something goes wrong but this does not make it a badly implemented contract. It is how users interact with it that can turn it into a risk.

I agree the aggregator is OOS but the root cause of the identified issue resides in the THORChain_Router contract which is definitely in-scope. Applying the recommended mitigation step would get rid of potential risks related to the interaction with any ("loosely implemented") aggregator and would align with the initial intention of the developers which was to "send the recipient the gas asset".

Doing so, the protocol won't be making "any assumption based on that contract" and would simply take safety measures to discard any unexpected behavior.

The reason the router doesn't sent the recipient the gas asset is the protocol cannot know if the recipient can even receive ETH. For example, the recipient could be a smart contract wallet that consumes more gas than expected, which would mean the transfer would fail and outbound dropped. Instead, the pattern is decided that the aggregator will receive the ETH and prepared to do so with the proper rescueFunds function. This is a conscious design decision.

[0xGreed]

I understand, thank you for your feedback.

The aggregator being OOS for the audit, I can't debate about the risks the design decision may represent. Just as I explained before, the way the aggregator is currently implemented opens the door for a malicious actor to frontrun the rescueFunds function by calling swapOutV5() and receive the funds waiting to be rescued.

Hope this note will be taken into account in the mitigation process anyway.