Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L200-L206 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L220-L237 https://github.com/code-423n4/2024-06-thorchain/blob/e3fd3c75ff994dce50d6eb66eb290d467bd494f5/chain/ethereum/contracts/THORChain_Router.sol#L347-L355

Vulnerability details

The router contract is used to deposit funds, which are then transferred out, and the appropriate event should be emitted to be processed by the ThorChain network. However, there is an oversight in some of the functions which emit such an event when the token used is an ERC20 with fee-on-transfer.

The functions which are not compatible with the fee-on transfer are: transferOut(), _transferOutV5(), and _transferOutAndCallV5(). If we look at their implementation, we can see that the actual transferred value is not checked.

  function transferOut(
    address payable to,
    address asset,
    uint amount,
    string memory memo
  ) public payable nonReentrant {
    uint safeAmount;
    if (asset == address(0)) {
      ...
    } else {
      _vaultAllowance[msg.sender][asset] -= amount; // Reduce allowance
      (bool success, bytes memory data) = asset.call(
        abi.encodeWithSignature("transfer(address,uint256)", to, amount)
      );
      require(success && (data.length == 0 || abi.decode(data, (bool))));
      safeAmount = amount;
    }
    emit TransferOut(msg.sender, to, asset, safeAmount, memo);
  }

  function _transferOutV5(TransferOutData memory transferOutPayload) private {
    if (transferOutPayload.asset == address(0)) {
      ...
    } else {
      _vaultAllowance[msg.sender][
        transferOutPayload.asset
      ] -= transferOutPayload.amount; // Reduce allowance

      (bool success, bytes memory data) = transferOutPayload.asset.call(
        abi.encodeWithSignature(
          "transfer(address,uint256)",
          transferOutPayload.to,
          transferOutPayload.amount
        )
      );

      require(success && (data.length == 0 || abi.decode(data, (bool))));
    }

    emit TransferOut(
      msg.sender,
      transferOutPayload.to,
      transferOutPayload.asset,
      transferOutPayload.amount,
      transferOutPayload.memo
    );
  }

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

            ...

      emit TransferOutAndCallV5(
        msg.sender,
        aggregationPayload.target,
        aggregationPayload.fromAmount,
        aggregationPayload.toAsset,
        aggregationPayload.recipient,
        aggregationPayload.amountOutMin,
        aggregationPayload.memo,
        aggregationPayload.payload,
        aggregationPayload.originAddress
      );
    }
  }

The issue lies in the event emitted that uses the incorrect value. As stated in the README, these events are crucial as they are observed by the Bifrost.

TransferOut: if emitted, Bifrost will observe and post a MsgObservedTxOut to THORChain to alert the network of an outbound from a vault.

TransferOutAndCall - if emitted, Bifrost will observe and post a MsgObservedTxOut to THORChain to alert the network of an outbound from a vault.

Impact

The impact of this issue is medium. This is because the events emitted from the Router contract are monitored by the Bifrost and forwarded to the ThorChain network. As mentioned in the README, the event data, particularly the deposited amounts, must be valid.

If it determines that a log was emitted by the THORChain router, it parses that log and forwards the appropriate details to the THORChain Layer 1 for processing. This is an incredibly sensitive and crucial piece of the codebase, as if it can be tricked, this means malicious contracts can trick Bifrost into informing THORChain of something that didn’t actually happen, leading to loss of funds.

Tools Used

Manual Review

Recommended Mitigation Steps

Avoid emitting inaccurate amounts in the event. Instead, calculate the actual value transferred.

Assessed type

Token-Transfer

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