Do not reduce approval on transferFrom if current allowance is type(uint256).max

[0xclaudeshannon]

Fixes #3084

The Wrapped Ether (WETH) ERC-20 contract has a gas optimization that does not update the allowance if it is the max uint. I'd like to incorporate this into OpenZeppelin to save 5000 gas per transferFrom call when a max approve is used.

The transferFrom code for WETH is:

    function transferFrom(address src, address dst, uint wad)
        public
        returns (bool)
    {
        require(balanceOf[src] >= wad);

        if (src != msg.sender && allowance[src][msg.sender] != uint(-1)) {
            require(allowance[src][msg.sender] >= wad);
            allowance[src][msg.sender] -= wad;
        }

        balanceOf[src] -= wad;
        balanceOf[dst] += wad;

        Transfer(src, dst, wad);

        return true;
    }

The key line is if (src != msg.sender && allowance[src][msg.sender] != uint(-1)) {. If the allowance is the max uint (0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff), then the WETH contract does not update the allowance mapping. This saves 5000 gas for the SSTORE. The only downside is that this changes the abstraction for approvals; if you set the approval to the maximum amount, then the contract can spend more than the max uint of your token. However, this doesn't seem like a risk, as users wanting to actually specify that amount can simply do the max uint minus 1.

My proposed change is to incorporate this into OpenZeppelin. This could save 5000 gas (or 10-20%) per ERC-20 transferFrom, which is used all across Unsiwap, Compound, and other big protocols.

I need feedback from the team for how to add this to the documentation. It passes all the tests.

The idea for this PR came up in the GAS DAO community discord.

PR Checklist

• Tests
• Documentation
• Changelog entry

[earlit]

This could save users Millions on Gas

[Amxx]

Hello @0xclaudeshannon and thank you for submitting this PR

This was already identified when looking at possible gas optimizations.

AFAIK, ERC20 is not really strict about the approval mechanism. There are changes that technically be breaking, but that would still match the ERC requirement:

• not emitting an Approval event when a transferFrom consumes some/all of the approval.
• not updating the approval value if it's set to type(uint256).max

I'm uncomfortable with the first one, but the second one, that you are proposing, looks reasonable to me.

[frangio]

Yes I think this is reasonable mainly because this pattern is already presented to users as "infinite approval" which is literaly what this PR implements.

[frangio]

Missing changelog entry and documentation before merging.

[0xclaudeshannon]

Missing changelog entry and documentation before merging.

Changelog in b34920b

What should I add for documentation? And which file is that in

[0xclaudeshannon]

Thanks for changing it to type(uint256).max that's much more aesthetically pleasing.

[frangio]

For documentation I think we should add a mention of the behavior in the NatSpec for transferFrom.

[0xclaudeshannon]

For documentation I think we should add a mention of the behavior in the NatSpec for transferFrom.

This good? 04d88dd

[Amxx]

I think this documentation should go in ERC20.sol ... because it is a particularity of the implementation that is not part of the interface/ERC design.

Also, I'm always personally prefer using uint256 over uint, particularly when talking about the maximum value.

[Amxx]

• added tests.
• added equivalent mechanism to ERC777 transferFrom for consistency

[0xclaudeshannon]

I think this documentation should go in ERC20.sol ... because it is a particularity of the implementation that is not part of the interface/ERC design.

Also, I'm always personally prefer using uint256 over uint, particularly when talking about the maximum value.

fixed

[frangio]

LGTM!