fix: incorrect comment about EIP-2200 refund in ReentrancyGuard#6361
fix: incorrect comment about EIP-2200 refund in ReentrancyGuard#6361realMelTuc wants to merge 2 commits intoOpenZeppelin:masterfrom
Conversation
|
WalkthroughA comment in the 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/utils/ReentrancyGuard.sol`:
- Around line 103-104: The edited comment in ReentrancyGuard.sol is inaccurate —
restore the original wording (or expand it) to explicitly reference EIP-2200
case (3) and use the precise term "original value" so readers understand the
pre-transaction storage value is 1 (NOT_ENTERED), the current value after
_nonReentrantBefore is 2 (ENTERED), and the new value being set back to 1
triggers the SSTORE reset refund; update the comment near _nonReentrantAfter to
either revert to the previous text or clearly state "EIP-2200 case (3): original
nonzero → different nonzero → new equals original => SSTORE_RESET_GAS -
SLOAD_GAS refund" to remove ambiguity introduced by "prior value."
| // By storing the prior value once again, a refund is triggered (see | ||
| // https://eips.ethereum.org/EIPS/eip-2200) |
There was a problem hiding this comment.
The fix is incorrect — the old wording was technically accurate; this change downgrades precision without fixing any real problem.
The stated premise of this PR (and issue #6305) is that 2 → 1 is a non-zero→non-zero change that "does not qualify for an EIP-2200 refund." That premise is wrong.
EIP-2200 (unmodified by EIP-3529) explicitly defines a third refund case: "if the original value is nonzero, the current value is a different nonzero value, and the new value equals the original value, add SSTORE_RESET_GAS - SLOAD_GAS (currently 4,900) gas to the refund counter."
Applying that to _nonReentrantAfter():
| EIP-2200 term | Slot value |
|---|---|
| original value (pre-transaction) | NOT_ENTERED = 1 (nonzero ✓) |
current value (after _nonReentrantBefore) |
ENTERED = 2 (different nonzero ✓) |
| new value | NOT_ENTERED = 1 = original value ✓ |
All three conditions of case (3) are met, so a SSTORE_RESET_GAS - SLOAD_GAS gas refund is triggered. The old comment was correct on substance.
Furthermore, "original value" is the precise EIP-2200 term of art for the pre-transaction storage value. Replacing it with "prior value" introduces ambiguity: "prior" can reasonably be read as "the value immediately before this particular SSTORE" — i.e., ENTERED = 2 — which is wrong.
The comment at lines 45–49 also corroborates the existence of a refund: it explicitly states the refund is "lower in amount" when using non-zero sentinel values (case-3 refund ≈ 4,900 gas) versus if 0/1 were used (case-2 refund ≈ 19,900 gas). These two comments are mutually consistent and both correct.
The correct resolution is to either:
- Revert this change — the original comment was accurate.
- Expand it (if more clarity is desired) to explicitly name EIP-2200 case (3) and distinguish it from the larger zero-clearing refund.
✏️ Option 2 — Expand for clarity
- // By storing the prior value once again, a refund is triggered (see
- // https://eips.ethereum.org/EIPS/eip-2200)
+ // By storing the original value (1) once again after a dirty nonzero write (2),
+ // a partial gas refund is triggered under EIP-2200 case 3
+ // (nonzero original → different nonzero current → restore to original):
+ // SSTORE_RESET_GAS - SLOAD_GAS ≈ 4,900 gas. This is intentionally smaller
+ // than the zero-clearing refund (case 2, ≈19,900 gas) that would arise from
+ // using 0/1 sentinel values, keeping total refunds low and predictable.
+ // See https://eips.ethereum.org/EIPS/eip-2200🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/utils/ReentrancyGuard.sol` around lines 103 - 104, The edited
comment in ReentrancyGuard.sol is inaccurate — restore the original wording (or
expand it) to explicitly reference EIP-2200 case (3) and use the precise term
"original value" so readers understand the pre-transaction storage value is 1
(NOT_ENTERED), the current value after _nonReentrantBefore is 2 (ENTERED), and
the new value being set back to 1 triggers the SSTORE reset refund; update the
comment near _nonReentrantAfter to either revert to the previous text or clearly
state "EIP-2200 case (3): original nonzero → different nonzero → new equals
original => SSTORE_RESET_GAS - SLOAD_GAS refund" to remove ambiguity introduced
by "prior value."
1 participant
Summary\n\nFixed incorrect comment about EIP-2200 refund in ReentrancyGuard.sol. The comment incorrectly stated that restoring the original value triggers a refund according to EIP-2200.\n\n## Changes\n\n- Updated comment in ReentrancyGuard.sol to accurately reflect the gas behavior\n\n## Testing\n\n- Existing tests pass\n\nCloses #6305