ERC20._approve remove check

[vsmelov]

🧐 Motivation Save gas for a condition requirement that could never be false

📝 Details in ERC20.sol you have requirement

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

here

    function _approve(
        address owner,
        address spender,
        uint256 amount
    ) internal virtual {
        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);
    }

it's used here (but owner=_msgSender)

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

and here (but owner=_msgSender)

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

and here (but owner=_msgSender)

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

        return true;
    }

and here (but if you try to do transferFrom(address(0), msg.sender, 100) it will raise not enough allowance error)

    function _spendAllowance(
        address owner,
        address spender,
        uint256 amount
    ) internal virtual {
        uint256 currentAllowance = allowance(owner, spender);
        if (currentAllowance != type(uint256).max) {
            require(currentAllowance >= amount, "ERC20: insufficient allowance");
            unchecked {
                _approve(owner, spender, currentAllowance - amount);
            }
        }
    }

how you can see it's not possible for this condition to be false, so you can safely remove it, or at least give motivation why this condition is in ERC20.sol

[angusjoshi]

Are there potential issues with contracts inheriting from ERC20 calling _approve?

[vsmelov]

@angusjoshi1

I don't know in which case someone may call _approve(address(0), smth, smth)

I try use my fantasy to build up such case, but cannot for now

[Amxx]

Rather than just discussing if that is a legitimate thing to do, what we should all discuss "what would the consequence be if someone does that". People do mistakes, and we need to thing if/how we can limit the impact of these mistakes.

[Amxx]

Also, the main motivation of this change is to save gas. We would need to measure that, but IMO the savings would be anecdotal

[frangio]

We could remove the check for the public approve function while keeping it for the internal _approve function but it will make the code more complex. How much of an improvement does it bring?

[Amxx]

Having both a public and an internal is confusing to some. Having an internal and a private is to much IMO.

A if (x == address(0)) check doesn't include any sload. Its really cheap.

[balajipachai]

@frangio does this needs to be worked upon?

[Amxx]

We decided against doing that in 5.0, for reason stated above.