Redundant check in ERC20

[grGred]

🧐 Motivation In the ERC20 token contract the validation:

 require(from != address(0), "ERC20: transfer from the zero address");

is redundant. This require may fail only in case, when address(0) calls the transfer function, which is unreal circumstances.

📝 Details The only time, when this require may fail is when transferFrom function is called where owner address is address(0). But before the _transfer call there is a _spendAllowance call, after which _approve is called. So, finally, the transaction will revert in _approve function with error message: "ERC20: approve from the zero address".

I think:

• Validation in _transfer function of zero address is redundant.
• The case, when user tries to make transferFrom from zero address has very strange reason of the revert. I made transfer - but the error is about approve.

[frangio]

Thanks for the suggestions. We need to explore this a bit more. Personally I think relying on the check in _spendAllowance is too indirect.

[balajipachai]

@frangio A few more redundant address zero checks in ERC20 are as follows:

function increaseAllowance(address spender, uint256 addedValue) public virtual returns (bool) {
        address owner = _msgSender(); =====> owner will never be address(0)
        _approve(owner, spender, allowance(owner, spender) + addedValue);
        return true;
}

function decreaseAllowance(address spender, uint256 subtractedValue) public virtual returns (bool) {
        address owner = _msgSender(); =====> owner will never be address(0)
        uint256 currentAllowance = allowance(owner, spender);
        require(currentAllowance >= subtractedValue, "ERC20: decreased allowance below zero");
        unchecked {
            _approve(owner, spender, currentAllowance - subtractedValue);
}

function _approve(address owner, address spender, uint256 amount) internal virtual {
**THE BELOW CHECK owner != address(0) BECOME USELESS WHEN APPROVE IS BEING 
CALLED FROM increaseAllowance & decreaseAllowance**
        require(owner != address(0), "ERC20: approve from the zero address");
        require(spender != address(0), "ERC20: approve to the zero address");

        _allowances[owner][spender] = amount;
        emit Approval(owner, spender, amount);
}

HENCE IN THIS CASE, IN BOTH increaseAllowance & decreaseAllowance the 
require(spender != address(0), "ERC20: approve to the zero address"); statement must be moved from
_approve() to these functions respectively. This ensures only one require statement is executed.

Let us consider transferFrom(address from, address to, uint256 amount)

function transferFrom(address from, address to, uint256 amount) public virtual override returns (bool) {
        address spender = _msgSender(); =====>  spender will never be address(0)
        _spendAllowance(from, spender, amount); **THIS INTERNALLY CALLS _approve()**
        _transfer(from, to, amount);
        return true;
}

Hence, here from != address(0) & to != address(0) check must be performed in transferFrom function itself,
this would remove both redundant checks from `_approve` & `_transfer`.

Let us consider approve(address spender, uint256 amount)

function approve(address spender, uint256 amount) public virtual override returns (bool) {
        address owner = _msgSender(); =====> owner will never be address(0)
        _approve(owner, spender, amount);
        return true;
}

THIS INTERNALLY CALLS _approve(), and again the redundant check in _approve,
thus, here we can check spender != address(0) and the redundant checks from _approve()
can be removed safely

Let us consider transfer(address to, uint256 amount)

function transfer(address to, uint256 amount) public virtual override returns (bool) {
        address owner = _msgSender(); =====> owner will never be address(0)
        _transfer(owner, to, amount);
        return true;
}

THIS INTERNALLY CALLS _transfer() which does redundant address(0) checks for
from & to not equal to address(0), if we add to != address(0) check in this function
itself then the from & to check can be safely removed from the _transfer function

Summary:

• Redundant address(0) checks from _approve can be safely removed.
• Redundant address(0) checks from _transfer can be safely removed.
• This would bring down the transaction cost as well.

Let me know your inputs, if all seems good, I will get started with it and update the code and tests and submit a PR.

[Amxx]

Validation in _transfer function of zero address is redundant.

You may be missing that _transfer is an internal function that may be used by devs to plug into the contracts. We want to make it clear that using 0 as a from address is invalid. Keep in mind that a 0 address could be generated by an unsafe call to ecrecover.

Same goes for _approve.

We are not designing this function just for our own use, but also for devs to plug into. If they were private, then yes, the check would not be strictly necessary ... but they are internal and they could be called in all sorts of way we don't control.

[Amxx]

Also, please note that we are currently working on the next-v5.0 branch in preparation of the next major release, and that the transfer logic is very different on that branch.

[balajipachai]

Perfect, I would love to be part of the development process of next-v5.0, could you help me with those tasks, s.t. I can target their completion?

[balajipachai]

@Amxx Any updates on this?

[RenanSouza2]

This require may fail only in case, when address(0) calls the transfer function, which is unreal circumstances.

@grGred ERC20 uses Context so an overriding of _msgSender() can result in the address from beong zero

    function transfer(address to, uint256 amount) public virtual returns (bool) {
        address owner = _msgSender();
        _transfer(owner, to, amount);
        return true;
    }

[Amxx]

ERC20's transfer mechanism was completelly redesigned in 5.0. Closing