diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b6f67671..d8bece08f 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/contracts/utils/ReentrancyGuard.sol b/contracts/utils/ReentrancyGuard.sol index 7804893af..c8c2fceb1 100644 --- a/contracts/utils/ReentrancyGuard.sol +++ b/contracts/utils/ReentrancyGuard.sol @@ -12,17 +12,20 @@ pragma solidity ^0.5.0; * those functions `private`, and then adding `external` `nonReentrant` entry * points to them. * - * _Since v2.5.0:_ this module is now much more gas efficient, given net gas + * _Since v2.5.0:_ this module is now much more gas efficient, given net gas * 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; } } diff --git a/test/utils/ReentrancyGuard.test.js b/test/utils/ReentrancyGuard.test.js index c211ceca2..b3120213c 100644 --- a/test/utils/ReentrancyGuard.test.js +++ b/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: