Lines of code

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

Vulnerability details

Impact

While calling swapOut function of the aggregator contracts, we are passing the aggregationPayload.fromAmount which is greater than the real balance of the corresponding ERC20 token that we transferred from the Router initially and the swapOut function will fail as the user is expecting the real amount to be retrieved for swaping through that aggregator contract.

Proof of Concept

Before calling the swapOutV5 function in the aggregator we are transferring the required ERC20 tokens from the router contract to the aggregator. The issue is we are not considering is the case of fee On transfer tokens. As a result we are passing the same amount ,that we transferred initially to the aggregator , to the swapOut function without considering that fees are already deducted from the already sent amount.

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"
      );

      // add test case if aggregator fails, it should not revert the whole transaction (transferOutAndCallV5 call succeeds)
      // call swapOutV5 with erc20. if the aggregator fails, the transaction should not revert
      (bool _dexAggSuccess, ) = aggregationPayload.target.call{value: 0}(
        abi.encodeWithSignature(
          "swapOutV5(address,uint256,address,address,uint256,bytes,string)",
          aggregationPayload.fromAsset,
          aggregationPayload.fromAmount, //@audit fee on transfer token will cause this transaction to not happen as expected.FromAmount is greater than aggregator balance at the time of this call.
          aggregationPayload.toAsset,
          aggregationPayload.recipient,
          aggregationPayload.amountOutMin,
          aggregationPayload.payload,
          aggregationPayload.originAddress
        )
      );

      emit TransferOutAndCallV5(
        ....
      );
    }
}

As we can see same amount ( aggregationPayload.fromAmount ) is passed for swapOutV5 which is incorrect in the case of Fee on transfer ERC20 tokens.

Tools Used

Manual review

Recommended Mitigation Steps

Calculate the exact balance of the aggregatorContract before and after the initial transaction and then pass the value to swapOutV5

Assessed type

ERC20

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

SwapOutV5 : Fees are not deducted from the fromAmount on `Fee On Transfer ERC20 Token's` Transaction. #46