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

[c4-bot-7]

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