Skip to content

Validate known packed storage addresses#1452

Open
mds1 wants to merge 10 commits into
mainfrom
codex-packed-storage-address-check
Open

Validate known packed storage addresses#1452
mds1 wants to merge 10 commits into
mainfrom
codex-packed-storage-address-check

Conversation

@mds1

@mds1 mds1 commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

What

PR #1450 added storageCodeExceptions to silence false positives when a storage write contains a packed address. That worked, but it did so by skipping validation entirely for an (account, slot) pair — which also skips checking the address packed into that slot. This PR replaces that escape hatch with a check that actually validates the packed address.

Why

Some L1 upgradeable contracts pack an address alongside smaller fields in the same 32-byte slot (e.g. SystemConfig slot 108 packs superchainConfig with minBaseFee). The state-diff check reads the full 32-byte word, so the packed value is too large to look like an address and the embedded address slips past the normal code/EOA validation. We still want that address checked, not skipped.

How

  1. Revert the config-driven storageCodeExceptions escape hatch.
  2. Hardcode the known packed slots from the checked-in contracts-bedrock storage layout snapshots, which are the source of truth for the offsets.
  3. For those slots, extract the packed 20-byte address and run the existing code/EOA validation on it. The match is keyed on both the slot number and the account's registry name (e.g. SystemConfigProxy), so an unrelated contract that happens to write the same slot number is left alone.
  4. Leave every other storage write on the existing full-word validation path.

Slots covered: AnchorStateRegistryProxy / EthLockboxProxy / SuperchainConfig at slot 0, OptimismPortalProxy.superchainConfig at slot 53, OptimismPortalProxy.ethLockbox at slot 63, and SystemConfigProxy.superchainConfig at slot 108.

@mds1 mds1 marked this pull request as ready for review June 5, 2026 19:36
@mds1 mds1 requested review from a team as code owners June 5, 2026 19:36
@mds1 mds1 requested a review from smartcontracts June 5, 2026 19:36
Comment thread src/tasks/MultisigTask.sol
Comment thread src/tasks/MultisigTask.sol Outdated
Comment thread src/tasks/MultisigTask.sol
Comment thread src/tasks/MultisigTask.sol Outdated
Comment thread test/tasks/MultisigTask.t.sol
@mds1 mds1 marked this pull request as draft June 8, 2026 17:48
@mds1 mds1 force-pushed the codex-packed-storage-address-check branch 2 times, most recently from f957098 to c74c3ed Compare June 11, 2026 13:55
@mds1 mds1 marked this pull request as ready for review June 11, 2026 13:57
Comment thread test/tasks/MultisigTask.t.sol
Comment thread src/tasks/MultisigTask.sol
Comment thread src/tasks/MultisigTask.sol
Comment thread test/tasks/MultisigTask.t.sol Outdated
mds1 and others added 9 commits June 11, 2026 14:39
Inline `Utils.contains(newContracts, account)` and reassign `value` in place
instead of introducing `isNewContract`/`valueToCheck`, so the code/EOA
validation lines stay unchanged from main. Shorten the packed-slot helpers to
`_getPackedStorageAddr`/`_extractPackedAddr`, rewrite the `_getPackedStorageAddr`
doc comment in plain terms, and explain why the `getChains()` try/catch is needed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rename `_registerGlobalStorageAccount` to `_registerSentinelStorageAccount` to
match the registry's vocabulary, and read the sentinel chain from the registry
instead of re-hardcoding its keccak. DRY the two registration helpers through a
shared `_registerStorageAccountOnChain`, drop the single-use 5-arg `createAccess`
overload in favor of the existing mutate-after-build pattern, and document the
storage-write and `_packAddress` helpers.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mds1 mds1 force-pushed the codex-packed-storage-address-check branch from 767ec43 to 48c94ba Compare June 11, 2026 21:40
- Add NatSpec to `_isRegistryAddress` describing the sentinel-then-per-chain
  lookup and the SimpleAddressRegistry fallback.
- Comment the test flag constants tying `0x0101`/spacer/`1 << 160` to the
  real Bedrock Initializable layout.
- Explain why the "not treated as packed" test uses `type(uint256).max` to
  distinguish the full-word and packed paths.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread src/tasks/MultisigTask.sol
@sbvegan sbvegan mentioned this pull request Jun 11, 2026
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant