Why not wrap the _balances[to] += amount instruction in an unchecked block?

[PaulRBerg]

This issue is related to #2875, but my question comes from a different angle:

Why not wrap all calculations in the _transfer function in an unchecked block? I'm specifically referring to line 241 in the _transfer function:

openzeppelin-contracts/contracts/token/ERC20/ERC20.sol

Line 241 in e734b42

_balances[to] += amount;

It seems to me that the balance of the to account can never overflow (it's an invariant), due to the fact that the _mint function uses checked arithmetic (and rightly so).

I looked at the tests for the transfer function and couldn't find a test that checks for an overflow on line 241:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/e734b42fc2245b520372bca0099870f40f1e6f38/test/token/ERC20/ERC20.behavior.js

The only argument that I can think of in favor of keeping the current behavior is the precautionary principle: many people inherit ERC20 and modify it according to their project idea, which in some cases might involve changing the _balances mapping outside of _mint.

Thoughts?

[Amxx]

Indeed, the _balances[to] += amount; cannot overflow, because we already ensure that the total supply doesn't overflow.
Also, the balance increase on line 263 could be unchecked.

I think we haven't done that previously because we were not fully comfortable with using unchecked with a low granularity, but this is an obvious gas improvement that we should implement!

Would you like to open a PR for that?

[PaulRBerg]

Also, the balance increase on line 263 could be unchecked.

Good catch!

Would you like to open a PR for that?

Thanks for letting me do it. Here it is!

#3513

[frangio]

So for the record I want to explain why we did not have this optimization before and why we're going to merge it now.

The reasoning until now was that we use unchecked when lack of overflow can be proven through local reasoning. For example:

require(amount <= balances[to]);
unchecked {
    balances[to] -= amount;
}

We know b - a will not overflow because a <= b is asserted immediately before.

The property that balances[to] + amount does not overflow is not a local one in this sense. It can't be shown by reasoning about the function. It can only be proven by reasoning about the entire contract, and observing that: 1) the values in the balances mapping are only increased in _mint, 2) any increase in one account's balance is reflected in an equivalent increase in the _totalSupply and this is capped by 2**256 thanks to overflow checks, 3) all updates to the balances mapping preserve this invariant (the cap).

What is happening now is we're relaxing the requirements to use unchecked to allow it when lack of overflow can be proven through reasoning about one file/contract. Note that this is still "local" in another sense, just "less" local than before.

This makes the case for private variables versus internal ones even stronger. We need to be able to guarantee that the balances mapping is not updated outside of this file.

For future reference, this optimization of ~130 gas, in my opinion, only barely meets the threshold to be worth the extra complexity (complexity to prove the correctness of the contract!).

[PaulRBerg]

"local reasoning" is such an apt way to describe the problem situation, @frangio!

I had the same concern in mind when I said that the only argument I can think of in favor of keeping checked arithmetic is the precautionary principle - if an OpenZeppelin user inherits from ERC20 and implements a way to increase the _balances mapping outside of _mint, the unchecked arithmetic wouldn't be safe anymore. So I agree that making variables private instead of internal would be a sensible default.

[frangio]

Fixed in #3513.