Redundant Check in ERC20 Smart Contracts’ approve()

We saw many smart contract issues during past months: BEC, SMT, HXG plus the most recent FXE, and they shared a common vulnerability: integer overflow. Are we becoming too jittery about overflow problem with imaginary fear? Is it a good practice to add as many safety checks as possible to avoid attacks?

Here comes a snippet:

function approve(address _spender, uint _amount) returns (bool success) {
    // approval amount cannot exceed the balance
    require ( balances[msg.sender] >= _amount );
    // update allowed amount
    allowed[msg.sender][_spender] = _amount;
    // log event
    Approval(msg.sender, _spender, _amount);
    return true;
  }

This is one implementation of approve() in ERC20. The code seems perfect at first glance. But there is one strange line of code at the beginning, if you inspect carefully:

require ( balances[msg.sender] >= _amount );

It checks the current balance by requiring the approved _amout not exceeding the current balance when it is time to approve a transaction. Does this sentence really make sense?

First we would determine if this is essential to the function:

Suppose the program satisfies the condition above when executing approve(), it is safe to say that such check is meaningless given that the following code could possibly lead to a scenario which contradicts with this condition.

In fact, there are certain circumstances where approved allowance would exceed the account balance:

1. Token holders call transfer() to move tokens after approval, thus the final balance becomes less than the allowance.
2. Given multiple users approval first, and one of them, after calling transferFrom(), might get a smaller balance than the value approved before.

Now we can come to a conclusion that adding such a requirement is pointless since we cannot assure the approved allowance exceeding the balance with operations afterwards.

Someone would disagree with this opinion, saying that no severe problems would emerge aside from consuming more gas. Is that really the truth?

We are cooperating with DDEX(a decentralized exchange) on assessing ERC20 contracts on its platform lately and we uncovered a critical problem. Many decentralized exchange has already employed contracts like 0x protocol, and it would call fillOrder() in Exchange Contract to transfer tokens when delivering.

Code for transferring in fillOrder() is shown below:

require(transferViaTokenTransferProxy(
    order.makerToken,
    order.maker,
    msg.sender,
    filledMakerTokenAmount
));
require(transferViaTokenTransferProxy(
    order.takerToken,
    msg.sender,
    order.maker,
    filledTakerTokenAmount
));

An intermediate account is necessary for the decentralized exchange and it functions as the msg.sender above initiating a trade. Here is the detailed process:

1. Transfer tokens from the order form to an intermediate account of a exchange
2. The intermediate account transfers WETH to the seller
3. The buyer transfers WETH to the intermediate account
4. The intermediate account transfers tokens to the buyer

The contract TokenTransferProxy finishes all 4 steps alone and the intermediate account needs to approve abundant allowance for it, while the 0x contract does not offer approve(), thus the intermediate account of the exchange should prepare for this approving process in advance.

Without tokens to be transferred holding ahead by the intermediate account, due to the check,

require ( balances[msg.sender] >= _amount );

the account would not be able to get approving done in advance.

We have observed many similar situations in decentralized and even centralized exchanges, so SECBIT team strongly request leaving out this check when developing ERC20 contracts.

What if someone has already done adding it and deployed the contract? Is there a way out?

When it comes to exchanges, they could only ensure a successful approve by letting the project developing team send a great number of tokens to the intermediate account and restrict the transaction volume within the given range.

By using an internal tool, SECBIT team scanned through all 22681 ERC20 Token smart contracts and found that 21 of them had this trouble, and 3 contracts had already conducted many transactions. Here is a list of Token contracts involved:

IDH

GZR

ADE

SAINT

BULB

ZZZ

KTM

ETI-P

ZORRO01

ZORRO02

With a growing number of decentralized exchanges, we suggest that all token contracts should inspect this issue carefully and fix as early as possible to guarantee the future trading liquility of your project tokens.

SECBIT Labs provides statistics and data above. If interested, please send an email to info@secbit.io.

SECBIT Labs was founded by a group of cryptocurrency-enthusiasts. We are doing research on smart contract security, smart contract formal verification, crypto-protocols, compilation, contract analysis, game theory and crypto-economics.

SECBIT Labs aims to build a trustworthy and thriving blockchain economy.