Correct checkCodeHash's stated justification in LibMigrationRegistry - #104
Conversation
All three entry points justified checkCodeHash() with a mechanism solc does not exhibit. The comments said an unguarded call into an empty account returns nothing, decodes as zero, and silently sends a caller down its pre-migration branch (or silently swallows a write). It does not: compiled under this repo's own settings (solc 0.8.25, optimizer, 100000 runs, cancun), all three call shapes REVERT against an address with no code. For the returning calls solc skips the extcodesize check because return data is expected and the ABI decoder reverts on 0 bytes of returndata; for the void call solc emits the existence check and reverts. So an absent registry was never the case that answers a lie — it reverts either way, just anonymously, and what the guard buys there is a NAMED, diagnosable error. The case that genuinely returns a wrong answer, and that the comments never stated as the reason, is the one the suite already covers: unrelated code or an EIP-7702 delegation occupying the deterministic address, free to answer any timestamp, any head, or a silent success for a write. Rewrites the three rationales to state the real mechanism, and the three NoRegistry test rationales that restated the same wrong one. Comments only: no assertion, no behaviour and no bytecode changes, and the tests already assert the named UnexpectedMigrationRegistryCodeHash that the corrected wording claims. Closes #52 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 22 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
|
Reviewed 4a30b34: ready — Closes #52. Comments only, and the correction is a real sharpening rather than a reword. The old text implied the codehash check is what turns an absent registry into a revert; it is not — solc already reverts a high-level call whose returndata is too short to decode, so an empty address reverts either way, just anonymously. What the check actually forbids is the case that does NOT revert: code at the address that is not this registry, an EIP-7702 delegation included, free to answer zero to every migration and send every caller down its pre-migration branch. Naming that as the hazard, and naming the empty case as the one the check merely gives a name to, is accurate. CI green, 0 unresolved threads — vacuous, CodeRabbit reports |
1 participant
Closes #52 (audit finding
MIGREG-1, dimension 5, severity LOW).What was wrong
All three
LibMigrationRegistryentry points justifiedcheckCodeHash()with a mechanism solc does not exhibit. The comments claimed an unguarded call into an empty account returns nothing, decodes as zero, and so silently sends a caller down its pre-migration branch — or, for the write, silently swallows the record.Verified, not assumed
I did not take the issue's word for the mechanism. I compiled a probe under this repo's own settings (
solc = "0.8.25", optimizer,optimizer_runs = 100000,evm_version = "cancun") driving the three call shapes the library uses as high-level typed interface calls, exactly as the library makes them:applied-shaped (uint256)head-shaped (bytes32)applyMigration-shaped (void)ok=false, rdlen 0ok=false, rdlen 0ok=false, rdlen 0ok=true->0x00..ok=true->0x00..ok=trueok=true->0x00..An absent registry reverts in all three directions. Nothing decodes as zero and no write silently succeeds. For the returning calls solc skips the
extcodesizecheck because return data is expected, and the ABI decoder reverts on 0 bytes of returndata; for the void call solc emits the existence check and reverts.So the empty-address case the comments led with was already safe without the guard — what the guard buys there is a named, diagnosable error instead of a bare revert. The case that genuinely answers a lie, and that the comments never stated as the reason, is the one the suite already covers: unrelated code or an EIP-7702 delegation occupying the deterministic address, free to answer any timestamp, any head, or a silent success for a write.
The probe was removed before committing; it is not part of this diff.
The change
Comments only, in two files:
src/lib/LibMigrationRegistry.sol— the three rationales onapplied,headandapplyMigrationnow state the real mechanism: the absent registry reverts anonymously either way, and the check is what names it and what forbids the occupying-code case that does not revert.test/src/lib/LibMigrationRegistry.t.sol— the three*NoRegistrytest rationales restated the same wrong mechanism, so they propagated it; corrected the same way, each now pointing at the*WrongCode/*DelegatedCodetests that cover the case which actually returns a wrong answer.No assertion, no behaviour and no bytecode changes. Every changed line starts with
///, so the deterministic address and codehash pins are untouched. The tests already assert the namedUnexpectedMigrationRegistryCodeHash, which is exactly the behaviour the corrected wording claims.The
checkCodeHash()natspec itself and the siblingLibAddressRegistry.solalready stated an accurate rationale and are unchanged.QA
n/a— the diff changes only///comment prose. There is no code path whose behaviour a new test could discriminate, and every existing assertion is byte-for-byte unchanged. What the change asserts is a claim ABOUT the compiler, and that claim was verified empirically (see Oracle) rather than by a suite test. Existing coverage of the corrected mechanism is already present and passing:testAppliedWrongCode,testHeadWrongCode,testApplyMigrationWrongCode,testAppliedDelegatedCode,testHeadDelegatedCode,testApplyMigrationDelegatedCodecover the occupying-code case the corrected prose names, and the three*NoRegistrytests cover the absent case.n/a— a docs-only diff has no executable lines to mutate. Confirmed structurally: every added and removed line ingit diffbegins with///, and the compiled artifacts are unchanged, which the repo's own codehash/snapshot pin tests independently attest (the snapshot and registry-deploy suites pass, and those fail on any bytecode movement).foundry.tomlsettings that makes the same three high-level typed calls against (a) an address with no code, (b) an address etched with0x60206000f3(returns 32 zero bytes to any call), and (c) an EIP-7702 designator0xef0100 || delegate, and recordedsuccessand returndata length for each. Results are the table above. The empty-address row falsifies the old prose; the occupying-code and delegated rows establish the new prose. This is independent ofLibMigrationRegistry— the probe does not import it.src/lib/LibMigrationRegistry.sol:104-110, 131-135, 153-157, and (B) the same correction applied to the three test rationales attest/src/lib/LibMigrationRegistry.t.sol:264-269, 283-286, 300-303. Covered A and B, both in full. The issue's exact proposed replacement text was used verbatim for A; for B the issue specified the correction without giving text, so the wording is mine and states the same verified mechanism. Deliberately NOT covered, as outside the filed scope: pinning the compiler behaviour as a permanent regression test so the prose cannot drift back, and the pre-existing "Both entry points check" in thecheckCodeHash()natspec (there are three). Say the word and I will file or fold in either.Checks
forge fmt --check— exit 0.forge test— 168 passed, 47 failed, and all 47 failures arevm.createSelectFork: environment variable *_RPC_URL not found, the local.envabsence CLAUDE.md documents. Zero non-RPC failures.LibMigrationRegistryTest— 23 passed, 0 failed.🤖 Generated with Claude Code