From 6ad1f01f5fa35a64b12ed1dc718d71ff324751d9 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 16 Jul 2025 16:26:23 +0200 Subject: [PATCH 1/5] Add nonReentrantView modifier --- contracts/mocks/ReentrancyAttack.sol | 5 +++++ contracts/mocks/ReentrancyMock.sol | 9 +++++++++ contracts/mocks/ReentrancyTransientMock.sol | 7 +++++++ contracts/utils/ReentrancyGuard.sol | 18 ++++++++++++++++++ contracts/utils/ReentrancyGuardTransient.sol | 18 ++++++++++++++++++ test/utils/ReentrancyGuard.test.js | 16 ++++++++++++---- 6 files changed, 69 insertions(+), 4 deletions(-) diff --git a/contracts/mocks/ReentrancyAttack.sol b/contracts/mocks/ReentrancyAttack.sol index 3df2d1c2b23..fec1478153e 100644 --- a/contracts/mocks/ReentrancyAttack.sol +++ b/contracts/mocks/ReentrancyAttack.sol @@ -9,4 +9,9 @@ contract ReentrancyAttack is Context { (bool success, ) = _msgSender().call(data); require(success, "ReentrancyAttack: failed call"); } + + function staticcallSender(bytes calldata data) public view { + (bool success, ) = _msgSender().staticcall(data); + require(success, "ReentrancyAttack: failed call"); + } } diff --git a/contracts/mocks/ReentrancyMock.sol b/contracts/mocks/ReentrancyMock.sol index 39e2d5ed850..34971812920 100644 --- a/contracts/mocks/ReentrancyMock.sol +++ b/contracts/mocks/ReentrancyMock.sol @@ -16,6 +16,10 @@ contract ReentrancyMock is ReentrancyGuard { _count(); } + function viewCallback() external view nonReentrantView returns (uint256) { + return counter; + } + function countLocalRecursive(uint256 n) public nonReentrant { if (n > 0) { _count(); @@ -36,6 +40,11 @@ contract ReentrancyMock is ReentrancyGuard { attacker.callSender(abi.encodeCall(this.callback, ())); } + function countAndCallView(ReentrancyAttack attacker) public nonReentrant { + _count(); + attacker.staticcallSender(abi.encodeCall(this.viewCallback, ())); + } + function _count() private { counter += 1; } diff --git a/contracts/mocks/ReentrancyTransientMock.sol b/contracts/mocks/ReentrancyTransientMock.sol index f0e61ea8caa..a624232facb 100644 --- a/contracts/mocks/ReentrancyTransientMock.sol +++ b/contracts/mocks/ReentrancyTransientMock.sol @@ -16,6 +16,8 @@ contract ReentrancyTransientMock is ReentrancyGuardTransient { _count(); } + function viewCallback() external view nonReentrantView {} + function countLocalRecursive(uint256 n) public nonReentrant { if (n > 0) { _count(); @@ -36,6 +38,11 @@ contract ReentrancyTransientMock is ReentrancyGuardTransient { attacker.callSender(abi.encodeCall(this.callback, ())); } + function countAndCallView(ReentrancyAttack attacker) public nonReentrant { + _count(); + attacker.staticcallSender(abi.encodeCall(this.viewCallback, ())); + } + function _count() private { counter += 1; } diff --git a/contracts/utils/ReentrancyGuard.sol b/contracts/utils/ReentrancyGuard.sol index a95fb512f31..6e320f3ce3f 100644 --- a/contracts/utils/ReentrancyGuard.sol +++ b/contracts/utils/ReentrancyGuard.sol @@ -61,6 +61,24 @@ abstract contract ReentrancyGuard { _nonReentrantAfter(); } + /** + * @dev View variant of the `nonReentrant` modifier. Can be used to prevent view functions from being called + * while the internal state of the contract is inconsistent, and invariants do not hold. + * + * This being a "view" version of the modifier, it will not set the reentrancy status. This modifier should only + * be used in view functions. Payable and non-payable function should use the standard "nonReentrant" modifier. + */ + modifier nonReentrantView() { + _nonReentrantBeforeView(); + _; + } + + function _nonReentrantBeforeView() private view { + if (_status == ENTERED) { + revert ReentrancyGuardReentrantCall(); + } + } + function _nonReentrantBefore() private { // On the first call to nonReentrant, _status will be NOT_ENTERED if (_status == ENTERED) { diff --git a/contracts/utils/ReentrancyGuardTransient.sol b/contracts/utils/ReentrancyGuardTransient.sol index a1318c86f3c..c827d6377f0 100644 --- a/contracts/utils/ReentrancyGuardTransient.sol +++ b/contracts/utils/ReentrancyGuardTransient.sol @@ -37,6 +37,24 @@ abstract contract ReentrancyGuardTransient { _nonReentrantAfter(); } + /** + * @dev View variant of the `nonReentrant` modifier. Can be used to prevent view functions from being called + * while the internal state of the contract is inconsistent, and invariants do not hold. + * + * This being a "view" version of the modifier, it will not set the reentrancy status. This modifier should only + * be used in view functions. Payable and non-payable function should use the standard "nonReentrant" modifier. + */ + modifier nonReentrantView() { + _nonReentrantBeforeView(); + _; + } + + function _nonReentrantBeforeView() private view { + if (_reentrancyGuardEntered()) { + revert ReentrancyGuardReentrantCall(); + } + } + function _nonReentrantBefore() private { // On the first call to nonReentrant, REENTRANCY_GUARD_STORAGE.asBoolean().tload() will be false if (_reentrancyGuardEntered()) { diff --git a/test/utils/ReentrancyGuard.test.js b/test/utils/ReentrancyGuard.test.js index c4418563eb5..4a157864998 100644 --- a/test/utils/ReentrancyGuard.test.js +++ b/test/utils/ReentrancyGuard.test.js @@ -7,7 +7,8 @@ for (const variant of ['', 'Transient']) { async function fixture() { const name = `Reentrancy${variant}Mock`; const mock = await ethers.deployContract(name); - return { name, mock }; + const attacker = await ethers.deployContract('ReentrancyAttack'); + return { name, mock, attacker }; } beforeEach(async function () { @@ -20,9 +21,16 @@ for (const variant of ['', 'Transient']) { expect(await this.mock.counter()).to.equal(1n); }); - it('does not allow remote callback', async function () { - const attacker = await ethers.deployContract('ReentrancyAttack'); - await expect(this.mock.countAndCall(attacker)).to.be.revertedWith('ReentrancyAttack: failed call'); + it('nonReentrantView function can be called', async function () { + await this.mock.viewCallback(); + }); + + it('does not allow remote callback to nonReentrant function', async function () { + await expect(this.mock.countAndCall(this.attacker)).to.be.revertedWith('ReentrancyAttack: failed call'); + }); + + it('does not allow remote callback to nonReentrantView function', async function () { + await expect(this.mock.countAndCallView(this.attacker)).to.be.revertedWith('ReentrancyAttack: failed call'); }); it('_reentrancyGuardEntered should be true when guarded', async function () { From 5fa55f3edca69f8b2d3caa7880294ad267241ba6 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 16 Jul 2025 16:28:15 +0200 Subject: [PATCH 2/5] add changeset --- .changeset/quick-pianos-press.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/quick-pianos-press.md diff --git a/.changeset/quick-pianos-press.md b/.changeset/quick-pianos-press.md new file mode 100644 index 00000000000..e9eae71946c --- /dev/null +++ b/.changeset/quick-pianos-press.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`ReentrancyGuard` and `ReentrancyGuardTransient`: Add `nonReentrantView`, a read-only version of the `nonReentrant` modifier. From 6f911614d412dfaf64a13a753b797d5a18d4481f Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 16 Jul 2025 16:31:28 +0200 Subject: [PATCH 3/5] consistency --- contracts/mocks/ReentrancyTransientMock.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contracts/mocks/ReentrancyTransientMock.sol b/contracts/mocks/ReentrancyTransientMock.sol index a624232facb..436b245109d 100644 --- a/contracts/mocks/ReentrancyTransientMock.sol +++ b/contracts/mocks/ReentrancyTransientMock.sol @@ -16,7 +16,9 @@ contract ReentrancyTransientMock is ReentrancyGuardTransient { _count(); } - function viewCallback() external view nonReentrantView {} + function viewCallback() external view nonReentrantView returns (uint256) { + return counter; + } function countLocalRecursive(uint256 n) public nonReentrant { if (n > 0) { From 7732a385c413a3238224f823e4711aaddb949fd0 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Tue, 29 Jul 2025 16:25:49 -0400 Subject: [PATCH 4/5] Apply suggestions from code review --- contracts/utils/ReentrancyGuard.sol | 2 +- contracts/utils/ReentrancyGuardTransient.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/utils/ReentrancyGuard.sol b/contracts/utils/ReentrancyGuard.sol index 6e320f3ce3f..34eb72bb06c 100644 --- a/contracts/utils/ReentrancyGuard.sol +++ b/contracts/utils/ReentrancyGuard.sol @@ -63,7 +63,7 @@ abstract contract ReentrancyGuard { /** * @dev View variant of the `nonReentrant` modifier. Can be used to prevent view functions from being called - * while the internal state of the contract is inconsistent, and invariants do not hold. + * while the internal state of the contract is inconsistent and invariants do not hold. * * This being a "view" version of the modifier, it will not set the reentrancy status. This modifier should only * be used in view functions. Payable and non-payable function should use the standard "nonReentrant" modifier. diff --git a/contracts/utils/ReentrancyGuardTransient.sol b/contracts/utils/ReentrancyGuardTransient.sol index c827d6377f0..e291c654520 100644 --- a/contracts/utils/ReentrancyGuardTransient.sol +++ b/contracts/utils/ReentrancyGuardTransient.sol @@ -39,7 +39,7 @@ abstract contract ReentrancyGuardTransient { /** * @dev View variant of the `nonReentrant` modifier. Can be used to prevent view functions from being called - * while the internal state of the contract is inconsistent, and invariants do not hold. + * while the internal state of the contract is inconsistent and invariants do not hold. * * This being a "view" version of the modifier, it will not set the reentrancy status. This modifier should only * be used in view functions. Payable and non-payable function should use the standard "nonReentrant" modifier. From 9312ffa7554a33d01c1e7b3af6ff294b814763dc Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 5 Aug 2025 08:48:08 -0600 Subject: [PATCH 5/5] NatSpec --- contracts/utils/ReentrancyGuard.sol | 9 +++++---- contracts/utils/ReentrancyGuardTransient.sol | 9 +++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/contracts/utils/ReentrancyGuard.sol b/contracts/utils/ReentrancyGuard.sol index 34eb72bb06c..854c48bb944 100644 --- a/contracts/utils/ReentrancyGuard.sol +++ b/contracts/utils/ReentrancyGuard.sol @@ -62,11 +62,12 @@ abstract contract ReentrancyGuard { } /** - * @dev View variant of the `nonReentrant` modifier. Can be used to prevent view functions from being called - * while the internal state of the contract is inconsistent and invariants do not hold. + * @dev A `view` only version of {nonReentrant}. Use to block view functions + * from being called, preventing reading from inconsistent contract state. * - * This being a "view" version of the modifier, it will not set the reentrancy status. This modifier should only - * be used in view functions. Payable and non-payable function should use the standard "nonReentrant" modifier. + * CAUTION: This is a "view" modifier and does not change the reentrancy + * status. Use it only on view functions. For payable or non-payable functions, + * use the standard {nonReentrant} modifier instead. */ modifier nonReentrantView() { _nonReentrantBeforeView(); diff --git a/contracts/utils/ReentrancyGuardTransient.sol b/contracts/utils/ReentrancyGuardTransient.sol index e291c654520..701d587f9b8 100644 --- a/contracts/utils/ReentrancyGuardTransient.sol +++ b/contracts/utils/ReentrancyGuardTransient.sol @@ -38,11 +38,12 @@ abstract contract ReentrancyGuardTransient { } /** - * @dev View variant of the `nonReentrant` modifier. Can be used to prevent view functions from being called - * while the internal state of the contract is inconsistent and invariants do not hold. + * @dev A `view` only version of {nonReentrant}. Use to block view functions + * from being called, preventing reading from inconsistent contract state. * - * This being a "view" version of the modifier, it will not set the reentrancy status. This modifier should only - * be used in view functions. Payable and non-payable function should use the standard "nonReentrant" modifier. + * CAUTION: This is a "view" modifier and does not change the reentrancy + * status. Use it only on view functions. For payable or non-payable functions, + * use the standard {nonReentrant} modifier instead. */ modifier nonReentrantView() { _nonReentrantBeforeView();