Lines of code

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

Vulnerability details

The protocol have a whitelist for the tokens, the whitelist includes some Fee-on-Transfer tokens as well like PAXG. The ThorChain_Router contract's functions should be compatible with the Fee-on-transfer tokens but they are not.

The _transferOutAndCallV5 function are implementetd to allow users to make swaps with external liquidity, it uses an aggregator to create the swaps possible: TC_Aggr-Aggregator drawio

As we can see, the _transferOutAndCallV5 function first transfers the tokens to the aggregator and then calls the aggregator's sawpOutV5() to complete the swap.

  function _transferOutAndCallV5(TransferOutAndCallData calldata aggregationPayload) private {
    if (aggregationPayload.fromAsset == address(0)) {
      .....
    } else {
      _vaultAllowance[msg.sender][
        aggregationPayload.fromAsset
      ] -= aggregationPayload.fromAmount; // Reduce allowance
      // send ERC20 to aggregator contract so it can do its thing
      (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,
          aggregationPayload.toAsset,
          aggregationPayload.recipient,
          aggregationPayload.amountOutMin,
          aggregationPayload.payload,
          aggregationPayload.originAddress
        )
      );
    }
  }

The problem is that the protocol tries to swap the same amount of tokens it sent to the aggregator before the swap aggregationPayload.fromAmount. Which means this will revert due to insufficient balance when the token will be a Fee-on-transfer token:

      (bool transferSuccess, bytes memory data) = aggregationPayload
        .fromAsset
        .call(
          abi.encodeWithSignature(
            "transfer(address,uint256)",
            aggregationPayload.target,
@>          aggregationPayload.fromAmount
          )
        );

      (bool _dexAggSuccess, ) = aggregationPayload.target.call{value: 0}(
        abi.encodeWithSignature(
          "swapOutV5(address,uint256,address,address,uint256,bytes,string)",
          aggregationPayload.fromAsset,
          ///@audit-issue M fee-on-transfer tokens will not work bcz they transfer the same amt and then try to swap same amt as well
@>        aggregationPayload.fromAmount,
          aggregationPayload.toAsset,
          aggregationPayload.recipient,
          aggregationPayload.amountOutMin,
          aggregationPayload.payload,
          aggregationPayload.originAddress
        )
      );

Impact

Broken Functionality, doesn't allow users to interact with fee-on-transfer tokens with the protocol

Proof of Concept

Bob deposits 5000 PAXG on Ethereum in order to bridge it to Arbirtum and swap it to the USDT.
Bob's PAXG gets bridged to Arbitrum successfully and the vault calls the transferOutAndCallV5() to perform the swap
TC Router transfers the 5000 PAXG to the aggregator, the aggregator receives 4999 PAXG due to PAXG's 0.02% fee
TC Router calls the swapOutV5 on the aggregator with the 5000 PAXG
The Trx reverts due to the insufficient balance of the aggregator
Bob's Trx fails

Tools Used

Shaheen's YadDasht

Recommended Mitigation Steps

As the aggregators will be whitelisted and probably deployed by the TC, I think instead of transferring the tokens to the aggregator, the aggregators should only get the allowance for the transfer and then they should handle the fee-on-transfer tokens based on the start and end balance math.

Assessed type

ERC20

code-423n4 / 2024-06-thorchain-findings

`_transferOutAndCallV5` function is incompatible for the Fee-on-Transfer tokens #40