Calling transferOutAndCallV5 function with fee on transfer token will fail

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/ethereum/contracts/THORChain_Router.sol#L391 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/ethereum/contracts/THORChain_Router.sol#L364 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/ethereum/contracts/THORChain_Aggregator.sol#L184 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/ethereum/contracts/THORChain_Aggregator.sol#L203

Vulnerability details

Any time transferOutAndCallV5 function was called with a fee on transfer token like PAXG (Whitelisted) and the aggregator contract have 0 balance of the token, the transaction will fail because a value that is greater than the value received by the aggregator contract was used in calling swapOutV5 function look at the scenario below.

1. Assume a user deposit 1 PAXG token with an intention of swapping to another token.
2. Bifrost will listen to deposit event and call transferOutAndCallV5 function with a TransferOutAndCallData.fromAmount of 1 PAXG - fee because depositWithExpiry function pass (fromAmount - fee1) to deposit event.
3. The transferOutAndCallV5 function will transfer the token again to aggregator contract. Another transfer fee will be deducted now we have (fromAmount - fee1 - fee2) in aggregator contract.
4. But (fromAmount - fee1) was used to call swapOutV5 function of the aggregator contract, Its greater than what the contract recieved.
5. The swapOutV5 function approve (fromAmount - fee1) to swapRouter. At the end the transaction will fail because the aggregator contract will not have enough token (it recieved fromAmount - fee1 - fee2 but approve fromAmount - fee1).

Check below codes also consider reading comments.

function _transferOutAndCallV5(TransferOutAndCallData calldata aggregationPayload) private {

  // ..

  // transfer aggregationPayload.fromAmount token to aggregator but it will receive aggregationPayload.fromAmount - fee
  (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");

  (bool _dexAggSuccess, ) = aggregationPayload.target.call{value: 0}(abi.encodeWithSignature("swapOutV5(address,uint256,address,address,uint256,bytes,string)",
  aggregationPayload.fromAsset,
  aggregationPayload.fromAmount, // original aggregationPayload.fromAmount was used in the swapOutV5 instead of aggregationPayload.fromAmount - fee
  aggregationPayload.toAsset,
  aggregationPayload.recipient,
  aggregationPayload.amountOutMin,
  aggregationPayload.payload,
  aggregationPayload.originAddress
  ));

  // ..

}

function swapOutV5(address fromAsset, uint256 fromAmount, address toAsset, address recipient, uint256 amountOutMin, bytes memory payload, string memory originAddress) public payable nonReentrant {

  // ..

  safeApprove(fromAsset, address(swapRouter), 0);
  safeApprove(fromAsset, address(swapRouter), fromAmount); // Its (fromAmount - fee1)

  // ..

  // (fromAmount - fee1) was also used to call swapExactTokensForEth
  (bool aggSuccess, ) = address(swapRouter).call(abi.encodeWithSignature("swapExactTokensForETH(uint256,uint256,address[],address,uint256)", fromAmount, amountOutMin, path, recipient, type(uint).max));

  // ..

}

Impact

swapOutV5 operation will fail and leads to users lost of transfer fee

Proof of Concept

Below test proof that what the aggregator contract recieved is less than what its try to approve.

1. Add a new file name FeeOnTransferToken in contracts folder and paste below code.

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

// STA, PAXG
contract FeeOnTransfer {
    uint256 public immutable fee;
    uint8 public immutable decimals;
    uint256 public immutable totalSupply;

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

    event Transfer(address indexed _from, address indexed _to, uint256 _value);
    event Approval(address indexed _owner, address indexed _spender, uint256 _value);

    constructor(uint256 _totalSupply, uint8 _decimals,  uint256 _fee) {
        totalSupply = _totalSupply * (10 ** _decimals);
        balanceOf[msg.sender] = totalSupply;
        fee = _fee * (10 ** _decimals);
        decimals = _decimals;

        emit Transfer(address(0), msg.sender, totalSupply);
    }

    function name() public pure returns (string memory) {
        return "My ERC20 Token";
    }

    function symbol() public pure returns (string memory) {
        return "MET";
    }

    function transfer(address _to, uint256 _value) external returns (bool) {
        balanceOf[msg.sender] -= _value;
        balanceOf[_to] += (_value - fee);
        balanceOf[address(0)] += fee;

        emit Transfer(msg.sender, _to, _value);
        return true;
    }

    function transferFrom(address _from, address _to, uint256 _value) public returns (bool) {
        require(balanceOf[_from] >= _value, "Insufficient balance");
        if (_from != msg.sender && allowance[_from][msg.sender] != type(uint).max) {
            require(allowance[_from][msg.sender] >= _value, "Insufficient allowance");
            allowance[_from][msg.sender] -= _value;
        }

        balanceOf[_from] -= _value;
        balanceOf[_to] += (_value - fee);
        balanceOf[address(0)] += fee;

        emit Transfer(_from, _to, (_value - fee));
        emit Transfer(_from, address(0), fee);
        return true;
    }

    function approve(address _spender, uint256 _value) external returns (bool) {
        allowance[msg.sender][_spender] = _value;

        emit Approval(msg.sender, _spender, _value);
        return true;
    }
}

2. Add new test file and paste below code.

const Aggregator = artifacts.require("THORChain_Aggregator");
const Sushiswap = artifacts.require("SushiRouterSmol");
const Router = artifacts.require("THORChain_Router");
const Token = artifacts.require("FeeOnTransfer");
const { ethers } = require("hardhat");
const { expect } = require("chai");

var WETH;
var TOKEN;
var ROUTER1;
var ROUTER2;
var SUSHISWAP;
var AGGREGATOR;
var USER1, USER2;
var ASGARD1, ASGARD2;

const _zero = "0";
const _1000 = "1000000000000000000000";
const _2000 = "2000000000000000000000";
const _3000 = "3000000000000000000000";
const _4000 = "4000000000000000000000";
const _5000 = "5000000000000000000000";

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

describe("THORChain_Router Contract Test", function () {
  let accounts;

  before(async function () {
    WETH = await ethers.getContractFactory("WETH");
    accounts = await web3.eth.getAccounts();
    WETH = await WETH.deploy();

    USER1 = accounts[0];
    USER2 = accounts[1];
    ASGARD1 = accounts[2];
    ASGARD2 = accounts[3];
    YGGDRASIL1 = accounts[4];
    YGGDRASIL2 = accounts[5];

    ROUTER1 = await Router.new();
    ROUTER2 = await Router.new();
    SUSHISWAP = await Sushiswap.new(WETH.address);
    AGGREGATOR = await Aggregator.new(WETH.address, SUSHISWAP.address);

    TOKEN = await Token.new(5000, 18, 1);
  });

  describe("User Deposit Assets", function () {
    it("Should pass allowance by ASGARD1", async function () {
      expect(BN2Str(await TOKEN.totalSupply())).to.equal(_5000);
      expect(BN2Str(await TOKEN.balanceOf(USER1))).to.equal(_5000);

      // Approve ROUTER1 to transfer 2000 tokens
      await TOKEN.approve(ROUTER1.address, _2000, { from: USER1 });

      // ROUTER1 now has 2000 tokens
      await ROUTER1.depositWithExpiry(ASGARD1, TOKEN.address, _2000, "DEPOSIT:THOR.ERC20", currentTime);

      // ASGARD1 transfer 1000 allowance to ASGARD2
      await ROUTER1.transferOutAndCallV5({target: AGGREGATOR.address, fromAsset: TOKEN.address, fromAmount: _1000, toAsset: WETH.address, recipient: USER2, amountOutMin: _2000, memo: "SWAP:PAXG/USDT:ADDRESS:THOR_ADDRESS:2000", payload: 0x55, originAddress: "USER1"}, { from: ASGARD1 });

      // Expect aggregator contract balance is less than what its trying to approve
      expect(Number(await TOKEN.balanceOf(AGGREGATOR.address))).to.lessThan(Number(_1000));
    });
  });
});

Tools Used

Manual review

Recommended Mitigation Steps

Use the (fromAmount - fee1 - fee2) for calling swapOutV5 or make sure the aggregator contract does not have zero value of the calling token anytime.

Assessed type

ERC20

[c4-judge]

trust1995 marked the issue as satisfactory