Efficiency of the supply capping mechanism in ERC20Capped

1vd21 · February 15, 2021, 1:54am

I am wondering about the efficiency of the implementation of the supply capping mechanism in ERC20Capped.sol.

It seems each time a transfer is made, the _beforeTokenTransfer function will be called to verify that the cap is not exceeded. Whereas it would seem more natural to inline this verification within the functions that may actually increase totalSupply. Eg in the standard case the _mint function.

Incidentally, the error messages "ERC20: mint to the zero address", "ERC20: burn from the zero address" look inverted to me, one burns (mints) to (from) the zero address.

Skyge · February 15, 2021, 3:06am

I assume that you mean this capped contract: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20Capped.sol.

Whereas it would seem more natural to inline this verification within the functions that may actually increase totalSupply .

And I assume what you mean is that for current code:

_beforeTokenTransfer(address(0), account, amount);

_totalSupply += amount;
_balances[account] += amount;

it should be:

require(totalSupply() + amount <= cap(), "ERC20Capped: cap exceeded");
_totalSupply += amount;
_balances[account] += amount;

I think it is ok for you to write your own capped contract like so, but for the OpenZeppelin team, it seems like not, cause they want to keep a flexible approach, the ERC20.sol is a basic contract and the ERC20Capped.sol is one of the many derivatives of ERC20.sol, in addition to this there are ERC20Burnable.sol, ERC20Pausable.sol and so on, so for this capped contract, you should re-write your own logic only in capped contract to achieve your purpose.

abcoathup · February 15, 2021, 3:14am

Hi @1vd21,

Thanks for raising this.

Using hooks (https://docs.openzeppelin.com/contracts/3.x/extending-contracts#using-hooks) makes for a simpler design compared with overriding _mint though I can see how that would avoid the check on transfers and burns for the from address.

github.com

OpenZeppelin/openzeppelin-contracts/blob/v3.4.0/contracts/token/ERC20/ERC20Capped.sol#L41


    /**
     * @dev See {ERC20-_beforeTokenTransfer}.
     *
     * Requirements:
     *
     * - minted tokens must not cause the total supply to go over the cap.
     */
    function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override {
        super._beforeTokenTransfer(from, to, amount);
        if (from == address(0)) { // When minting tokens
            require(totalSupply().add(amount) <= cap(), "ERC20Capped: cap exceeded");
        }
    }
}

I am not sure what you mean here. The check in _mint is to prevent minting to the zero address and the check in _burn is to prevent burning from the zero address.

github.com

OpenZeppelin/openzeppelin-contracts/blob/v3.4.0/contracts/token/ERC20/ERC20.sol#L229


/** @dev Creates `amount` tokens and assigns them to `account`, increasing
 * the total supply.
 *
 * Emits a {Transfer} event with `from` set to the zero address.
 *
 * Requirements:
 *
 * - `to` cannot be the zero address.
 */
function _mint(address account, uint256 amount) internal virtual {
    require(account != address(0), "ERC20: mint to the zero address");
    _beforeTokenTransfer(address(0), account, amount);
    _totalSupply = _totalSupply.add(amount);
    _balances[account] = _balances[account].add(amount);
    emit Transfer(address(0), account, amount);
}
/**
 * @dev Destroys `amount` tokens from `account`, reducing the

github.com

OpenZeppelin/openzeppelin-contracts/blob/v3.4.0/contracts/token/ERC20/ERC20.sol#L250


 * total supply.
 *
 * Emits a {Transfer} event with `to` set to the zero address.
 *
 * Requirements:
 *
 * - `account` cannot be the zero address.
 * - `account` must have at least `amount` tokens.
 */
function _burn(address account, uint256 amount) internal virtual {
    require(account != address(0), "ERC20: burn from the zero address");
    _beforeTokenTransfer(account, address(0), amount);
    _balances[account] = _balances[account].sub(amount, "ERC20: burn amount exceeds balance");
    _totalSupply = _totalSupply.sub(amount);
    emit Transfer(account, address(0), amount);
}
/**
 * @dev Sets `amount` as the allowance of `spender` over the `owner` s tokens.

Amxx · February 15, 2021, 8:06pm

Hello @1vd21

You are right that fact that the cap protection hooks into _beforeTokenTransfer slightly increasses the cost of all transfers because of the if (from == address(0)) check.

An easy solution if to perform this check in the _mint function, has can be seen in this PR: https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2524

We usually prefer not to overload function and use the hook mechanism whenever possible. This is done because child contracts might have to resolve the overloading, which might be disturbing for some users. However, in this case, overloading clearly is the most gas efficient way, and thus feels like the better way.

Amxx · February 15, 2021, 10:27am

In the future, when you noticed design pattern that could improve gas efficiency, feel free to raise an issue on github, and we will see how we can improve that.