core/vm: check SSTORE access affordability before reading committed state

[pratikspatil024]

Summary

The PIP-88 SSTORE gas function (makeGasSStoreFuncPIP88, core/vm/operations_acl.go)
performed the committed-state read before it knew the slot's access cost. Because
the cold access cost (ColdSstoreCostPIP88 = 2940) exceeds the reentrancy sentry
(SstoreSentryGasEIP2200 = 2300), clearing the sentry no longer guarantees the caller
can pay for a cold access — so the read could be performed for a call that then runs out
of gas, i.e. work the gas schedule does not charge for. On bor this path is live behind
Chicago (mainnet, Amoy).

This PR resolves the access cost and warms the slot first, verifies the caller can afford
the access, and only then reads — matching the ordering go-ethereum adopted for its
EIP-8037/8038 SSTORE handler in ethereum/go-ethereum#35261.

Outcome-neutral. The guard only rejects callers that already fail the subsequent
charge — the minimum cold total (3040 = 2940 + 100) exceeds the 2940 guard — so no
transaction changes success/failure or gas used; both routes surface ErrOutOfGas. Every
returned gas value is byte-identical. Not a hardfork.

Verified on a devnet built from this branch (Chicago active), against the same devnet
built from develop:

	develop	this branch
attack tx gasUsed	454349	454349 (identical)
block witness under attack	21,788 B	4,243 B (back to baseline)

The identical gasUsed is the outcome-neutrality check on a live chain — same gas, same
OOG outcome; only the block witness (non-consensus) shrinks, because the reverted read no
longer populates the trie-fetch path that feeds witness collection.

Executed tests

• go test ./core/vm/ — new TestPIP88SStoreAccessCheck passes (5 boundary cases). It
  uses a read-counting StateDB to assert the committed-state read does not happen
  once gas is above the sentry but below the access cost — the boundary an error-string
  check alone would miss.
• Full core/vm suite unchanged (no gas-value test flips). The one intermittent failure,
  TestAbortDuringJump/JUMP, is a pre-existing timing-race flake — it fails identically
  on develop with this change stashed out and passes in isolation.
• go build ./..., gofmt, go vet ./core/vm/ clean.
• Devnet (Kurtosis, bor from this branch): opcode-level debug_traceCall sweep on a
  cold slot flips only in the intended window — at 2600 gas the SSTORE now rejects with
  not enough gas for slot access before the read, while ≤2300 and ≥2940 are unchanged.
  Baseline-vs-attack witness windows show the witness back at baseline (table above); the
  chain produced blocks normally throughout.

Rollout notes

• Not consensus-affecting; no coordinated upgrade required. Gas charged, transaction
  success/failure, receipts, and state roots are identical before and after — a full-sync
  node produces the same blocks. The only change is a smaller block witness for the
  affected calls.
• No hardfork. The fix is a bug fix within the already-active Chicago/PIP-88 behavior,
  not a schedule change.
• Witness / stateless-sync note for reviewers: because the fix removes a read, a fixed
  producer emits a slightly smaller witness. Full-sync nodes are unaffected; witness
  producers and stateless-sync consumers should move together during rollout. The witness
  is not committed in the block header on the current (pre-Amsterdam) chain, so there is no
  consensus impact — worth a reviewer confirming against the target release.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[pratikspatil024]

codegenie review

[pratikspatil024]

@claude review

[github-actions]

🧞 Codegenie Review

✅ No credible findings.

Coverage

Reviewed 2/2 hunks.
Coverage levels: deep 1, normal 1, light 0, skip 0.

🙋 Needs Human Attention

• Does the 'cold, at access, reads' case remain stable if ColdSstoreCostPIP88 or SstoreSentryGasEIP2200 change, i.e. are gas values pinned rather than derived only from params constants?
  • Files: core/vm/gas_table_test.go
  • Symbols: ColdSstoreCostPIP88, SstoreSentryGasEIP2200
  • Reason: Packet reviewer could not resolve this question from the reviewed context.

Stats

• 🤖 Model: anthropic claude-opus-5 high
• 🧞 Codegenie: v0.5.5 (58f82a9b2c)
• Elapsed time: 3m 7s
• Git: 0xPolygon/bor from develop to ppatil/pip88-sstore-access-check (021b907496)
• Review completeness: complete.
• Usage: model calls 17, tokens 328782, cost $1.4327.
• Effective caps: tokens 8000000.

✅ No Findings

No credible findings were found. Everything looks good.

— View Workflow Job

[Copilot]

Pull request overview

This PR adjusts the PIP-88 SSTORE dynamic gas function to determine whether a storage slot access is cold/warm and ensure the caller can afford the slot access cost before performing the committed-state read, preventing unnecessary state reads in calls that will inevitably run out of gas (notably relevant for witness generation paths).

Changes:

• Reorders PIP-88 SSTORE gas logic to resolve access-list status, compute the slot access cost, and guard affordability before calling GetStateAndCommittedState.
• Introduces a targeted unit test that counts committed-state reads to pin the ordering invariant and prevent regressions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
core/vm/operations_acl.go	Reorders PIP-88 SSTORE dynamic gas path to check access affordability before committed-state reads.
core/vm/gas_table_test.go	Adds TestPIP88SStoreAccessCheck to assert no committed-state read occurs in the sentry-cleared-but-cold-unaffordable window.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[claude]

I reviewed this PR and did not find any bugs. The fix reorders the PIP-88 SSTORE gas check so the caller's ability to pay the cold access cost is verified before the committed-state read happens, avoiding an unnecessary trie read on a call that would OOG anyway. Because this touches consensus-critical SSTORE gas logic on a path that is live under Chicago (mainnet, Amoy), a human look is still worthwhile before merging.

Extended reasoning...

Overview

The PR modifies makeGasSStoreFuncPIP88 in core/vm/operations_acl.go to resolve and warm the slot's access cost and verify the caller can afford it before calling GetStateAndCommittedState, instead of reading first and only failing later in the interpreter's post-hoc gas check. A new test (TestPIP88SStoreAccessCheck) uses a read-counting StateDB wrapper to pin the exact gas boundary at which the read is skipped.

Security risk assessment

I traced the interpreter's dynamic-gas handling (core/vm/interpreter.go) and the Call error path (core/vm/evm.go): any error returned from a gasFunc is wrapped with ErrOutOfGas and, since it isn't ErrExecutionReverted, zeroes the caller's remaining gas identically to the old path where the interpreter's own contract.Gas < dynamicCost check would have caught the same shortfall after the (now-removed) read. For the warm-slot case the new guard (100 gas) is strictly weaker than the existing sentry check (2300 gas) and can never fire, matching the added test. For the cold case, the guard threshold (2940) matches ColdSstoreCostPIP88 exactly, so the newly-rejected window (2301–2939 gas) is precisely the window that previously would read state and then fail the interpreter's gas check anyway. I did not find a case where this changes success/failure, gas used, or refund behavior — this matches the PR's own devnet verification (identical gasUsed).

Level of scrutiny

This lives in core/vm/, which the repo's own security rules (evm-security.md, hardfork-rollout.md) flag as consensus-critical, and the affected code path is active under the Chicago/PIP-88 hardfork on mainnet and Amoy. Even though my read of the diff and the interpreter's error handling supports the outcome-neutrality claim, gas-accounting changes on a live hardfork path are exactly the category where a second human reviewer's sign-off is warranted, independent of whether a bug was found.

Other factors

The PR includes a boundary-focused unit test that specifically distinguishes "rejected before read" from "rejected after read," which is good coverage for this kind of ordering fix. No CODEOWNERS or other outstanding review comments are present in the timeline yet.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.20%. Comparing base (bea1a20) to head (021b907).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2356      +/-   ##
===========================================
- Coverage    54.23%   54.20%   -0.03%     
===========================================
  Files          908      908              
  Lines       162243   162252       +9     
===========================================
- Hits         87987    87956      -31     
- Misses       68825    68855      +30     
- Partials      5431     5441      +10     

Files with missing lines	Coverage Δ
core/vm/operations_acl.go	55.59% <100.00%> (+2.79%)	⬆️

... and 24 files with indirect coverage changes

Files with missing lines	Coverage Δ
core/vm/operations_acl.go	55.59% <100.00%> (+2.79%)	⬆️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.