ReentrancyGuard: Change value of _ENTERED from 2 to 0

[tzann]

🧐 Motivation This change allows the solidity compiler to optimize the reentrancy check in order to reduce bytecode size and gas costs.

📝 Details Simply change the value of _ENTERED in ReentrancyGuard.sol from 2 to 0.

Essentially, checking for (in)equality with a non-zero number requires a push and eq opcode, whereas checking for (in)equality with zero can make use of the iszero opcode instead. In this case the iszero opcode can even be omitted as the jumpi instruction checks for zero/non-zero already.

Importantly, the cost and refund amounts for the storage operations remain the same. See https://eips.ethereum.org/EIPS/eip-3529 for details, but essentially going 1 -> 2 -> 1 costs the same and gives the same refund as going 1 -> 0 -> 1.

Lastly, the sstore(0, 0) operation is also slightly optimized as it can be done using a push and a dup instead of two push opcodes. Note that this assumes that the _status storage variable is in storage slot 0, which is not guaranteed.

In total, this reduces the bytecode size by 3-4 bytes (depending on storage layout) and gas costs by 6 gas.

Bytecode of _nonReentrantBefore before suggested changes:

      0x02
      0x00
      sload
      sub
      tag_13
      jumpi
      ... <revert stuff here>
  tag_13:
      0x02
      0x00
      sstore
      jump

Bytecode of _nonReentrantBefore after suggested changes:

      0x00
      sload
      tag_13
      jumpi
      ... <revert stuff here>
  tag_13:
      0x00
      dup1
      sstore
      jump

In total, 3-4 bytes of bytecode are removed, and any reentrancy check is cheaper by 6 gas, regardless of the outcome.

[tobiasperel]

Interesting

[frangio]

In theory this makes sense, but there is an edge case that breaks with this change. This edge case has actually come up in conversations in the past as something that people use, and I'm comfortable with giving up on a 6 gas improvement in order to be super sure that we won't be breaking it.

If a proxy contract delegates to a ReentrancyGuard contract, it doesn't need to initialize the _status variable in order for nonReentrant functions to work. The reason is that the require statement is written as _status != _ENTERED:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/d5ca39e9a264ddcadd5742484b6d391ae1647a10/contracts/security/ReentrancyGuard.sol#L58

If we change _ENTERED to 0, this stops working. The variable needs to be initialized or the modifier doesn't work.

Normally proxies invoke an initializer, but some users may be relying that it isn't necessary in this case...

[frangio]

Closing for the reasons explained above.

[tzann]

Thanks for the explanation, makes sense! I wasn't aware of the previous conversations around this topic 😁