eth cannot be transferred across vaults or routers with `transferAllowance` function

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L165-L178 https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L466-L483 https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L490

Vulnerability details

Impact

The core impact of this issue stems from the incorrect assumption that the safeApprove function can be universally applied across all types of assets, including Ether (ETH), the native ETH asset. This misunderstanding leads to a fundamental flaw in the transferAllowance function. Specifically, when safeApprove attempts to call the approve function on address(0), which is conventionally used to denote ETH in Solidity—the operation fails.

This happens because the zero address (represented by address(0)) is a special address in Ethereum that doesn't correspond to any real account. This will cause a revert and cause DOS on the transfer of eth assets between routers using transferAllowance.

Proof of Concept

In the context of the THORChain_Router.sol smart contract, the safeApprove function is utilized to manage allowances for all asset and would only work for ERC20 tokens.

We see that depositWithExpiry takes care of address(0) as ETH, so we can say for certain that it expects address(0) assets as ETH transfers.

Tools Used

Manual Review

Recommended Mitigation Steps

Develop ETH-Specific scenario Handling: Implement dedicated logic within the smart contract to handle transactions involving ETH separately from ERC20 tokens.

something like

function _routerDeposit(
    address _router,
    address _vault,
    address _asset,
    uint _amount,
    string memory _memo
  ) internal {
    _vaultAllowance[msg.sender][_asset] -= _amount;

if(asset != address(0)){
safeApprove(_asset, _router, _amount);
}

    iROUTER(_router).depositWithExpiry(
      _vault,
      _asset,
      _amount,
      _memo,
      type(uint).max
    ); // Transfer by depositing
  }

Assessed type

ETH-Transfer