Add _reentrancyGuardStorageSlot() #5892
Conversation
🦋 Changeset detectedLatest commit: bf835e9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Reviewer's GuideImplements a storage-slot-based reentrancy guard by replacing the internal status variable with StorageSlot-based reads and writes via a new overrideable accessor, updates the transient guard accordingly, and adds a changeset entry for slot customization. Class diagram for updated ReentrancyGuard and ReentrancyGuardTransientclassDiagram
class ReentrancyGuard {
<<abstract>>
+constructor()
+_reentrancyGuardEntered() bool
+_reentrancyGuardStorageSlot() bytes32
-REENTRANCY_GUARD_STORAGE bytes32
-NOT_ENTERED uint256
-ENTERED uint256
-_nonReentrantBefore() void
-_nonReentrantAfter() void
error ReentrancyGuardReentrantCall
}
class ReentrancyGuardTransient {
<<abstract>>
+_reentrancyGuardEntered() bool
+_reentrancyGuardStorageSlot() bytes32
-REENTRANCY_GUARD_STORAGE bytes32
-_nonReentrantBefore() void
-_nonReentrantAfter() void
}
ReentrancyGuard <|-- ReentrancyGuardTransient
ReentrancyGuard o-- StorageSlot
ReentrancyGuardTransient o-- StorageSlot
Class diagram for StorageSlot usage in ReentrancyGuardclassDiagram
class StorageSlot {
+getUint256Slot() StorageSlot.Uint256Slot
+asBoolean() StorageSlot.BooleanSlot
}
class StorageSlot.Uint256Slot {
+value uint256
}
class StorageSlot.BooleanSlot {
+tstore(bool)
+tload() bool
}
ReentrancyGuard o-- StorageSlot.Uint256Slot
ReentrancyGuardTransient o-- StorageSlot.BooleanSlot
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughAdds an internal hook Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Contract as Contract (inherits ReentrancyGuard)
participant Slot as ReentrancyGuard Storage Slot
Caller->>Contract: call nonReentrant function
Contract->>Contract: _nonReentrantBefore()
Contract->>Contract: _reentrancyGuardStorageSlot()
Contract->>Slot: read slot (entered?)
alt not entered
Contract->>Slot: write ENTERED
Contract->>Contract: execute function body
Contract->>Slot: write NOT_ENTERED (_nonReentrantAfter)
Contract-->>Caller: return
else already entered
Contract-->>Caller: revert ReentrancyGuardReentrantCall
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Add NatSpec documentation on
_reentrancyGuardStorageSlot()to explain the override mechanism and how consumers should pick a unique storage slot. - Consider extracting the repeated
getUint256Slot().value = ...patterns into private setter/getter helpers to reduce boilerplate and improve readability. - Double-check that the chosen
REENTRANCY_GUARD_STORAGEconstant is guaranteed to not collide with other storage slots and document how it was derived for audit clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add NatSpec documentation on `_reentrancyGuardStorageSlot()` to explain the override mechanism and how consumers should pick a unique storage slot.
- Consider extracting the repeated `getUint256Slot().value = ...` patterns into private setter/getter helpers to reduce boilerplate and improve readability.
- Double-check that the chosen `REENTRANCY_GUARD_STORAGE` constant is guaranteed to not collide with other storage slots and document how it was derived for audit clarity.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
contracts/utils/ReentrancyGuard.sol (1)
26-30: Clarify the deprecation note to avoid mixed signals.Current wording reads as both “this is deprecated” and “Transient will be removed,” which can confuse readers. Tighten the phrasing.
- * As of v5.5, this storage-based version of the reentrancy guard is DEPRECATED. In the - * next major release (v6.0), {ReentrancyGuardTransient} will be removed, and its logic - * moved to this contract, making transient storage the standard storage option for - * reentrancy guards. + * As of v5.5, this storage-backed implementation is DEPRECATED. + * In v6.0, this contract will adopt the transient-storage implementation and + * {ReentrancyGuardTransient} will be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.changeset/three-parents-argue.md(1 hunks)contracts/utils/ReentrancyGuard.sol(4 hunks)contracts/utils/ReentrancyGuardTransient.sol(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/three-parents-argue.md
[grammar] ~5-~5: Ensure spelling is correct
Context: ... a pure virtual function, that can be overriden to allow slot customization.
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~5-~5: There might be a mistake here.
Context: ...e overriden to allow slot customization.
(QB_NEW_EN)
🔇 Additional comments (4)
contracts/utils/ReentrancyGuardTransient.sol (2)
47-52: Good refactor: writes go through the slot hook.Switching to
_reentrancyGuardStorageSlot()centralizes slot selection and enables customization in descendants.
59-60: Good refactor: read goes through the slot hook.Keeps read/write symmetry and makes overrides effective everywhere.
contracts/utils/ReentrancyGuard.sol (2)
75-83: Good migration to StorageSlot-based guard.Read/Write via
getUint256Slot()is consistent and keeps semantics intact.
33-38: Confirmed: REENTRANCY_GUARD_STORAGE is identical in both variantsAll occurrences of the
REENTRANCY_GUARD_STORAGEconstant share the same 32-byte value (0x9b779b17422d0df92223018b32b4d1fa46e071723d6817e2486d003becc55f00):
- contracts/utils/ReentrancyGuard.sol (lines 36–37)
- contracts/utils/ReentrancyGuardTransient.sol (lines 19–20)
No divergent defaults—no further changes needed.
.changeset/three-parents-argue.md
Outdated
| 'openzeppelin-solidity': patch | ||
| --- | ||
|
|
||
| `ReentrancyGuard` and `ReentrancyGuardTransient`: Add `_reentrancyGuardStorageSlot()`, a `pure virtual` function, that can be overriden to allow slot customization. |
There was a problem hiding this comment.
Make the hook view-virtual (to enable address-derived slots) and fix wording.
Declaring the hook as pure prevents overrides from using address(this) (required for per-module, diamond-safe slots). Also fix typo and comma splice.
-`ReentrancyGuard` and `ReentrancyGuardTransient`: Add `_reentrancyGuardStorageSlot()`, a `pure virtual` function, that can be overriden to allow slot customization.
+`ReentrancyGuard` and `ReentrancyGuardTransient`: Add `_reentrancyGuardStorageSlot()`, an `internal view virtual` function that can be overridden to allow slot customization.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `ReentrancyGuard` and `ReentrancyGuardTransient`: Add `_reentrancyGuardStorageSlot()`, a `pure virtual` function, that can be overriden to allow slot customization. | |
| `ReentrancyGuard` and `ReentrancyGuardTransient`: Add `_reentrancyGuardStorageSlot()`, an `internal view virtual` function that can be overridden to allow slot customization. |
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Ensure spelling is correct
Context: ... a pure virtual function, that can be overriden to allow slot customization.
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~5-~5: There might be a mistake here.
Context: ...e overriden to allow slot customization.
(QB_NEW_EN)
🤖 Prompt for AI Agents
.changeset/three-parents-argue.md around line 5: the release note currently
states the hook as a `pure virtual` function which prevents overrides from
referencing `address(this)` (needed for per-module, diamond-safe,
address-derived storage slots) and contains a wording/typo/comma-splice; change
the description to say the hook is a `view virtual` function (so overrides can
use `address(this)`), and rewrite the sentence to fix the typo and comma splice
and clarify that the function can be overridden to customize the storage slot
(e.g., for per-module, diamond-safe slots derived from addresses).
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
…zeppelin-contracts into transient-reentry-slot
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/upgradeable/upgradeable.patch (1)
117-141: peerDependencies version placeholder will break installs."@openzeppelin/contracts": "" is not valid semver and will cause npm/yarn/pnpm to error. Use a concrete range aligned with this package’s major.
"peerDependencies": { - "@openzeppelin/contracts": "<package-version>" + "@openzeppelin/contracts": "^5.4.0" }If you need tighter coupling within a monorepo, consider "workspace:^" for pnpm/yarn workspaces.
🧹 Nitpick comments (7)
scripts/upgradeable/upgradeable.patch (7)
101-109: Usage example reads well for upgradeable flow.initialize + __ERC721_init is the right pattern. Consider bumping the pragma in the snippet to ^0.8.24 to match current min version elsewhere.
-pragma solidity ^0.8.20; +pragma solidity ^0.8.24;
185-188: Minor gas/readability: precompute empty-string hash.keccak256("") appears multiple times; define a file-level constant to avoid recomputation and clarify intent.
bytes32 private constant TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); + bytes32 private constant EMPTY_STRING_HASH = keccak256("");Then use EMPTY_STRING_HASH below.
195-198: Immutables kept but set to zero — clarify intent or remove.Keeping _hashedName/_hashedVersion as immutables and assigning zero in the constructor is unusual. If they only serve as a “legacy sentinel”, prefer constants and update comments to avoid implying these live in storage.
- /// @custom:oz-renamed-from _HASHED_NAME - bytes32 private immutable _hashedName; - /// @custom:oz-renamed-from _HASHED_VERSION - bytes32 private immutable _hashedVersion; + // Legacy sentinels kept for upgrade-path detection (non-storage, code-embedded). + /// @custom:oz-renamed-from _HASHED_NAME + bytes32 private constant _LEGACY_HASHED_NAME = bytes32(0); + /// @custom:oz-renamed-from _HASHED_VERSION + bytes32 private constant _LEGACY_HASHED_VERSION = bytes32(0);And adjust references accordingly (see lines 252–255, 302–305, 323–326). If you prefer to keep immutables, at least update the comments below to say “immutable” instead of “storage”.
214-228: Constructor assignments suggest “reset storage” but target immutables.The comment says “Reset prior values in storage”, but the variables are immutables (code, not storage). This is misleading; fix the wording or switch to true storage fields if reset semantics are intended.
- // Reset prior values in storage if upgrading + // Note: these are immutables; there is no storage to reset here.Alternatively, replace immutables with constants as suggested above.
252-255: Condition/message mismatch: “Uninitialized” check reads as legacy-sentinel check.The require currently reverts when legacy hashes are non-zero, which is not an “uninitialized” condition. Either drop this gate or reword to reflect what it guards.
- // If the hashed name and version in storage are non-zero, the contract hasn't been properly initialized - // and the EIP712 domain is not reliable, as it will be missing name and version. - require(_hashedName == 0 && _hashedVersion == 0, "EIP712: Uninitialized"); + // If legacy cached hashes are non-zero, this is a legacy/incorrect implementation for upgradeable usage. + require(_hashedName == 0 && _hashedVersion == 0, "EIP712: legacy-cached-values");If the intent was to ensure initialization, consider checking bytes(_EIP712Name()).length and bytes(_EIP712Version()).length instead (but tests were adjusted to allow empty values).
295-309: Use EMPTY_STRING_HASH in fallback branch.Slight gas/readability improvement; also avoids recomputing keccak256("").
- return keccak256(""); + return EMPTY_STRING_HASH;Apply similarly in _EIP712VersionHash().
317-330: Same as above for version hash; also minor micro-optimization.Reuse the EMPTY_STRING_HASH constant for the version empty case.
- return keccak256(""); + return EMPTY_STRING_HASH;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
scripts/upgradeable/upgradeable.patch(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T13:16:08.640Z
Learnt from: Amxx
PR: OpenZeppelin/openzeppelin-contracts#5904
File: contracts/mocks/crosschain/ERC7786RecipientMock.sol:12-14
Timestamp: 2025-08-29T13:16:08.640Z
Learning: In OpenZeppelin contracts, mock contracts (like ERC7786RecipientMock) don't require input validation such as zero-address checks in constructors, as they are only used for testing purposes in controlled environments.
Applied to files:
scripts/upgradeable/upgradeable.patch
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: coverage
- GitHub Check: slither
- GitHub Check: tests
- GitHub Check: tests-foundry
- GitHub Check: tests-upgradeable
- GitHub Check: halmos
🔇 Additional comments (10)
scripts/upgradeable/upgradeable.patch (10)
69-71: Good addition: clear note about the upgradeable variant.Helps steer users to the right docs.
80-81: npm install path updated correctly.Switch to @openzeppelin/contracts-upgradeable is appropriate.
88-94: Foundry install and remapping look correct.The remapping points to the upgradeable repo as expected.
243-245: LGTM: always rebuilding domain separator.This matches the upgradeable-friendly approach and avoids stale caches.
333-342: Repository URL switch is correct for the upgradeable package.
349-353: remappings.txt aligns both upgradeable and non-upgradeable imports.Looks good.
369-383: Same test adjustment for modules mock looks fine.Mirror the verification above to cover overridden values too.
384-415: Removal of clone/proxy adjustment test is reasonable under the new no-cache model.Given the separator is rebuilt each time, the legacy cache invalidation test is no longer applicable.
147-154: No direct constant usage observed: all reads and writes use_reentrancyGuardStorageSlot(), withREENTRANCY_GUARD_STORAGEconfined to its declaration and the hook’s return.
357-367: (run-scripts block issued)
Fixes #5681
Replaces #5688
PR Checklist
npx changeset add)Summary by Sourcery
Refactor ReentrancyGuard and its transient variant to store state in a configurable StorageSlot and provide a virtual hook for customizing the storage slot
Enhancements:
Documentation:
Summary by CodeRabbit
New Features
Refactor
Documentation