ReentrancyGuard gas optimization (#1996)

* Improve gas efficiency of reentrancyGuard

* Add changelog entry

* Fix ReentrancyGuard test

CHANGELOG.md

@@ -8,7 +8,7 @@
 
 ### Improvements
  * `ERC777`: `_burn` is now internal, providing more flexibility and making it easier to create tokens that deflate. ([#1908](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1908))
- * `ReentrancyGuard`: greatly improved gas efficiency by using the net gas metering mechanism introduced in the Istanbul hardfork. ([#1992](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1992))
+ * `ReentrancyGuard`: greatly improved gas efficiency by using the net gas metering mechanism introduced in the Istanbul hardfork. ([#1992](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1992), [#1996](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1996))
 
 ### Breaking changes
  * `ERC165Checker` now requires a minimum Solidity compiler version of 0.5.10. ([#1829](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1829))

contracts/utils/ReentrancyGuard.sol

@@ -16,13 +16,16 @@ pragma solidity ^0.5.0;
  * metering changes introduced in the Istanbul hardfork.
  */
 contract ReentrancyGuard {
-    // counter to allow mutex lock with only one SSTORE operation
-    uint256 private _guardCounter;
+    bool private _notEntered;
 
     constructor () internal {
-        // The counter starts at one to prevent changing it from zero to a non-zero
-        // value, which is a more expensive operation.
-        _guardCounter = 1;
+        // Storing an initial non-zero value makes deployment a bit more
+        // expensive, but in exchange the refund on every call to nonReentrant
+        // will be lower in amount. Since refunds are capped to a percetange of
+        // the total transaction's gas, it is best to keep them low in cases
+        // like this one, to increase the likelihood of the full refund coming
+        // into effect.
+        _notEntered = true;
     }
 
     /**
@@ -33,10 +36,16 @@ contract ReentrancyGuard {
      * `private` function that does the actual work.
      */
     modifier nonReentrant() {
-        _guardCounter += 1;
-        uint256 localCounter = _guardCounter;
+        // On the first call to nonReentrant, _notEntered will be true
+        require(_notEntered, "ReentrancyGuard: reentrant call");
+
+        // Any calls to nonReentrant after this point will fail
+        _notEntered = false;
+
         _;
-        require(localCounter == _guardCounter, "ReentrancyGuard: reentrant call");
-        _guardCounter = 1;
+
+        // By storing the original value once again, a refund is triggered (see
+        // https://eips.ethereum.org/EIPS/eip-2200)
+        _notEntered = true;
     }
 }

test/utils/ReentrancyGuard.test.js

@@ -14,7 +14,7 @@ contract('ReentrancyGuard', function () {
   it('should not allow remote callback', async function () {
     const attacker = await ReentrancyAttack.new();
     await expectRevert(
-      this.reentrancyMock.countAndCall(attacker.address), 'ReentrancyGuard: reentrant call');
+      this.reentrancyMock.countAndCall(attacker.address), 'ReentrancyAttack: failed call');
   });
 
   // The following are more side-effects than intended behavior: