Skip to content

fix(llm): derive impl diffs from ABIs and compiler storage layouts - #368

Merged
spalen0 merged 10 commits into
mainfrom
llm-imp
Sep 22, 2026
Merged

spalen0 merged 10 commits into
mainfrom
llm-imp

Conversation

@spalen0

@spalen0 spalen0 commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator

Closes #367.

The problem

The implementation-diff enrichment ran regexes over the concatenated Etherscan bundle, so it could not tell the deployed contract from its bases, libraries and imported interfaces. On the 3Jane USD3/sUSD3 upgrade it produced the same eight "additions" for both proxies — six were USD3 functions, two were IMorpho interface declarations — described clearMarketWindDown as an unpermissioned function on USD3/sUSD3 (it lives on MorphoCredit and is onlyOwner), reported a commented-out restartStrategy() as removed, missed sUSD3's only real change, and called storage safe because an imported library uses namespaced storage.

What changed

Every fact is now target-scoped and carries provenance, in four separated categories.

Category Source of truth
External ABI changes each implementation's own verified ABI, keyed by canonical signature
Changed function bodies the deployed contract's own definition in its own file, with a unified diff
Storage compatibility compiler storageLayout from Sourcify, for both sides
Unvalidated items stated explicitly, so silence is never read as safety
  • Structured record (utils/verified_contract.py) replaces the flattened cache tuple: per-file sources, compiler settings, ABI, and a resolved compilation target (settings.compilationTarget, else the unique declaring file). The disk-cache namespace carries the schema version, since positive source entries never expire. Concatenation survives only as an explicitly named search helper for natspec.
  • Storage (utils/sourcify_layout.py, utils/storage_layout.py) is now COMPATIBLE / INCOMPATIBLE / UNKNOWN. Slot, byte offset and recursive type shape are compared; compiler-internal type/AST ids and variable names are not, so a rename at an unchanged slot is context rather than a conflict. One-sided Sourcify coverage, a match: null response, a malformed payload or an ERC-7201 target all yield UNKNOWN — never a soft COMPATIBLE. No compiler is downloaded or executed in the monitoring path, per the follow-up in the issue.
  • Bodies (utils/solidity_text.py) are compared inside the deployed contract's braces only. Ambiguous target resolution or ambiguous overloads report the analysis unavailable instead of searching the bundle.
  • Risk anchor: upgradeToAndCall / upgradeAndCall now read their bytes argument — an empty payload replaces the implementation and nothing else, rather than always claiming an initializer ran.
  • Prompt guardrails: the system prompt scopes each category, and says that UNKNOWN storage is neither safe nor unsafe, that internal/private functions are not a governance control surface, and that a source-level modifier is not an authorization proof.
  • Consistency gate: every claim is re-derived from its own ABI before the prompt sees it; a section that fails is withheld and logged. If two contracts in one process get the same additions, the diff says so inline and logs a warning but keeps the result — each set was proven against its own ABI, so two siblings genuinely gaining the same function is a real finding.

Verification

Every failure mode from #367 is covered by synthetic, protocol-neutral fixtures (no frozen contract bundles in the repo), including: duplicate signatures across interface/base/target, comment-borne function text, commented-out code, overloaded tuple/array signatures, body-only and moved-into-helper changes, packing and offsets, structs spanning slots, safe renames, correct and over-consumed gaps, same-width retypes, custom value type resolution, ERC-7201 roots (checked against OpenZeppelin's and EIP-1967's published constants), and missing/unmatched/malformed/one-sided Sourcify responses.

The behavior was also checked by hand against the real 3Jane USD3/sUSD3 upgrade that motivated the issue: USD3 gets exactly its 7 ABI additions and 10 removals with a compatible positional layout, and sUSD3 gets no surface change plus its availableDepositLimit body change. Those bundles were not committed — the tooling is general, not tied to one protocol.

Note for review

Sourcify coverage is narrower than Etherscan's, so some upgrades will report UNKNOWN storage where the old code printed a (sometimes wrong) verdict. That is the intended trade: the issue's acceptance criterion is that storage is never called compatible without matched compiler layouts for both implementations.

Not included, flagged in the issue as P2/P3: an independent ERC-7201 namespace validator (namespaced targets are UNKNOWN until it exists), and letting the detail stage downgrade a summary — that fights the deliberate two-stage design where the TLDR is authoritative, so it seemed worth deciding separately.

🤖 Generated with Claude Code

spalen0 and others added 5 commits September 19, 2026 16:21
The implementation-diff enrichment ran regexes over the concatenated
Etherscan bundle, so it could not tell the deployed contract from its
bases, libraries and imported interfaces. In the 3Jane USD3/sUSD3 upgrade
it reported the same eight additions for both proxies — six of them USD3
functions, two of them IMorpho interface declarations — described an
interface declaration as an unpermissioned function, treated commented-out
Solidity as deployed, missed sUSD3's only real change, and called storage
safe because an imported library uses namespaced storage.

Replace the guesswork with target-scoped evidence:

- Cache a structured VerifiedContract (per-file sources, settings, ABI,
  resolved compilation target) instead of a flattened string. The disk
  cache namespace carries the schema version, since positive entries
  never expire. Concatenation survives only as a named search helper.
- Derive the external surface from each implementation's own ABI, keyed
  by canonical signature. Interfaces, comments and internal functions
  cannot appear; generated getters do.
- Compare function bodies within the deployed contract's own definition,
  with a short unified diff. Ambiguous target or overloads report the
  analysis unavailable rather than falling back to the bundle.
- Replace the storage boolean with COMPATIBLE/INCOMPATIBLE/UNKNOWN, from
  the compiler storageLayout Sourcify publishes for both implementations.
  Slot, offset and recursive type shape are compared; compiler-internal
  ids and variable names are not, so a rename at an unchanged slot is
  context, not a conflict. One-sided coverage, a malformed payload or an
  ERC-7201 target is UNKNOWN — never a soft COMPATIBLE. Nothing is
  compiled in the monitoring path.
- State unvalidated areas explicitly (inherited bodies, libraries, free
  functions, namespaces) so silence is not read as safety.
- Make the upgrade*AndCall risk anchor conditional on its bytes argument:
  an empty payload replaces the implementation and nothing else.

A consistency gate re-derives every claim from its own ABI before the
prompt sees it, and flags the same additions being attributed to a second
contract. The 3Jane upgrade is pinned as a regression fixture: USD3 gets
its seven real additions and ten removals with a COMPATIBLE layout, sUSD3
gets no surface change, its availableDepositLimit body change, and UNKNOWN
storage.

Closes #367

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The CI audit job started failing on two anyio advisories published after
main last ran: GHSA-5p39-cfhj-2xmp (process-pool workers block
indefinitely on undrained stderr) and GHSA-82r6-8w77-94w6 (TLSStream
IDNA 2003 host name encoding enables TLS certificate spoofing). Both are
fixed in 4.14.2.

anyio is transitive — httpx, anthropic and openai pull it — so this is a
lockfile-only change: 4.14.2 below Python 3.15, 4.15.1 above. No other
package moved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`strip_noise` blanks comments *and* string literals so that a brace or the
word `function` inside a string can't break brace matching. But the body
text captured for `FunctionDef.fingerprint` came from that same masked
text, so two functions differing only inside a string compared equal:
a changed revert message, role identifier, token name or any other
behavioral string produced no body change at all.

Mask twice instead. `strip_noise` still drives structural scanning; the
new `strip_comments` blanks only comments, and header/body text is read
from it at the same offsets. Whitespace is collapsed for formatting
insensitivity, which would still hide a change inside a string, so
`FunctionDef.literals` pins the literals byte-for-byte and joins the
fingerprint.

Callers now go through `contract_functions()`, which scopes to the
contract, pairs the two maskings and returns spans indexing back into the
original source — removing the two-text handling (and the body-offset
bookkeeping) from `impl_diff`.

Comment-only and reformat-only edits are still not changes; the 3Jane
fixtures report the same bodies as before.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The body comparator walked `set(old) & set(new)`, so it only ever saw
functions both implementations define. Anything added or removed was
dropped — including the case that matters most, where an upgrade moves
logic out of one function and into a new internal helper. On the 3Jane
USD3 upgrade the report showed `_deployFunds` shrinking from 29 lines to
1 and stopped there, silently omitting six functions: `_postDepositHook`
(which implements the headline ring-fence behavior), `_deployDepositedFunds`
(which now holds the deployment logic), `_pendingLoss`, `_wrapUSDC`, and
the deleted `_postWithdrawHook` and `_preTransferHook` hooks.

Diff the union instead. Additions and removals are reported for members
no ABI can carry — internal, private, modifiers — so external ones stay
the ABI section's job and nothing is listed twice. Visibility comes from
the target's own source rather than from matching ABI signatures, since
source types don't always spell the canonical ones
(`initialize(address,Id,…)` vs `initialize(address,bytes32,…)`).

Added functions carry their source, capped at three: a signature alone
doesn't explain behavior that moved into a new helper, which is the whole
point of reporting it. The section is renamed to "Target-defined function
changes" — it is no longer only about bodies — and the prompt now asks the
model to say what the shown code does rather than just name it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two ways storage could still read COMPATIBLE when it wasn't.

Namespaces were looked for in the wrong place. The detector checked the
natspec on the contract declaration, but ERC-7201 annotates the *struct*
inside the contract — and that struct is usually declared by an inherited
base. Real namespaced storage was never seen, so a changed namespace could
pass on a clean positional match. Both 3Jane implementations inherit OZ v5
`Initializable`, whose namespace this missed.

Now the deployed contract and its transitive bases (walked through `is`
clauses across the bundle) are scanned for annotated structs. Merely
imported libraries are still excluded — an imported helper's namespace is
what suppressed the check in #367. Flipping to UNKNOWN on any inherited
namespace would have made COMPATIBLE unreachable for nearly every OZ v5
upgradeable contract, since they all inherit `Initializable`; instead a
namespace is accepted only when provably unchanged: same id, identical
normalized struct text, elementary member types only (a user-defined type
could change beneath identical text). Anything else drops the verdict to
UNKNOWN with the positional detail kept. On 3Jane, `Initializable` is
byte-identical, so USD3 stays COMPATIBLE and the report now says it checked.

Shapes compared only encoding and byte width, so `uint256` -> `bytes32`,
`address` -> `uint160`, `bool` -> `uint8` or a mapping value retype all
passed although the new code reads the old bytes as a different type. Each
level of a shape now carries a normalized kind: elementary types exactly,
enums and UDVTs by name with AST ids stripped, containers structurally.
Contract types and `address` stay one kind, since that retype is
intentional. Same-width retypes are INCOMPATIBLE and described as
"reinterpreted as", distinct from a moved or resized slot.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@spalen0

spalen0 commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

Verified against PR head 11d98884b10ae2813060777f1eac1d08daca1bbd. Both stated limitations are present. I recommend fixing the verdict/coverage handling first, then adding bounded type and storage-access resolution while preserving #367's requirement that the monitoring runtime never downloads or executes a compiler.

Verification

The focused suite passed: 95 tests, 14 subtests across test_storage_layout.py, test_namespaced_storage.py, test_impl_diff.py, and test_impl_diff_3jane.py.

Additional synthetic reproductions exercised the actual comparison functions, mocking only the fetched compiler-layout boundary:

Case Current result Required behavior
type Id is bytes32 replaced with bytes32, same placement INCOMPATIBLE Compatible storage representation once the underlying declaration is resolved; otherwise UNKNOWN
Same name Id, underlying declaration changes bytes32uint256, same width COMPATIBLE Detect the reinterpretation once resolved; otherwise UNKNOWN
Target calls an internal library accessor; library storage member changes uint256bytes32 COMPATIBLE, no collected namespaces Include the used library storage in coverage; unresolved comparison means UNKNOWN
Unannotated storage accessor changes its root COMPATIBLE, no collected namespaces Include the accessor in coverage and detect a proven root change
Annotated namespace keeps identical struct text but its accessor changes root COMPATIBLE, namespace reported unchanged Verify the actual storage root as well as the struct

The relevant paths are _type_kind, collect_namespaces / _unvalidated_reason, and _compare_storage. The generic “Unvalidated items” text currently does not affect the storage verdict.

Artifact availability changes the proposed approach

I checked the live Sourcify v2 response for new USD3. It returns a verified match and 35 source entries in stdJsonOutput, zero containing an AST. The separate sources field does include type Id is bytes32; in src/interfaces/IMorpho.sol. The old USD3 layout describes Id only by label, encoding, and width.

Consequently, “fetch the AST from Sourcify” is not a sufficient implementation plan for this regression. Prefer a matching compiler AST when available, but support a deliberately bounded parser/resolver over verified source. Do not silently start compiling in the cron.

Proposed implementation plan

  1. Separate evidence from coverage. Extend comparison results to retain both proven conflicts and unresolved storage evidence. Aggregate as: any proven conflict → INCOMPATIBLE; otherwise incomplete storage coverage → UNKNOWN; otherwise → COMPATIBLE. Keep the positional result visible separately. Do not downgrade storage for unrelated omissions such as modifier-body analysis. Until storage reachability can be established, explicitly report that coverage as unresolved rather than treating an empty namespace collection as proof of absence.

  2. Resolve UDVT declarations on each side. Use source-qualified declaration identities, import aliases/remappings, and lexical scope to resolve type X is T; never use a bundle-global name match. Use compiler AST references when supplied with the same build, or a token/parser-based resolver for supported source forms. Normalize to the underlying elementary kind recursively through mapping keys/values, arrays, and struct members, while retaining the original type for display. Preserve compiler-reported slot, offset, encoding, and width checks, and the existing contract/address equivalence. Unresolved UDVTs must not compare equal merely because their names match. Report wrapper removal/renaming as context, not proof of behavioral safety.

  3. Discover actual storage access from the target. Extend beyond inheritance to resolved library calls and storage references passed into helpers, including using ... for. Find annotated structs and unannotated storage-pointer assignments such as s.slot := ROOT; track raw sload/sstore and unresolved delegatecalls as uncovered storage access. Merely importing a library must not add its storage to the target. Unsupported calls or ambiguous resolution must retain an explicit coverage gap.

  4. Validate roots before claiming namespaces unchanged. Resolve supported literals/constants and well-defined hash expressions, confirm that an ERC-7201 accessor uses its annotated root, and compare old/new root plus member layout. Detect duplicate roots and provable overlaps. Use compiler-derived struct layouts where available; ASTs alone do not guarantee emitted namespace layout data. Without such layouts, limit any unchanged proof to forms whose root, type dependencies, and compiler layout behavior are established. Changed or unsupported structures remain UNKNOWN. A broader namespace-layout engine or separately generated, build-bound artifacts should be a follow-up, not an implicit compiler fallback.

Regression / acceptance plan

  • UDVT unwrap/rewrap, rename, same-name underlying-type change, nested containers, duplicate names/import aliases, missing declarations, and compiler-version packing differences.
  • Used versus merely imported libraries; inherited and unannotated accessors; unchanged struct with changed root; annotation/root mismatch; duplicate roots; unresolved dynamic roots and delegatecalls.
  • Verdict precedence: a proven conflict remains INCOMPATIBLE even when another region is unknown; incomplete storage coverage never produces an unqualified COMPATIBLE.
  • Keep the existing 3Jane ABI/body/positional-layout regressions. USD3's positional layout can remain compatible even if the combined verdict becomes UNKNOWN until library storage coverage is established.
  • Freeze artifact payloads, including the no-AST case, and verify that the new regression assertions fail on this PR head before implementing the fixes. No compiler download/execution and no new network dependency in unit tests.

The Solidity docs support normalizing UDVTs to their underlying representation, but actual compiler layout checks remain necessary because of historical UDVT packing differences. ERC-7201 explicitly leaves enforcement of the declared namespace to the developer, which is why identical annotations/structs alone cannot establish root stability.

Review of #368 (comment 5759098600) showed storage could read COMPATIBLE
while it was changed or simply unseen, because "Unvalidated items" was
text that never touched the verdict:

1. `Id` unwrapped to the `bytes32` it wraps was INCOMPATIBLE;
2. `Id` redeclared over `uint256` (same name, same width) was COMPATIBLE;
3. storage a used library writes through a slot accessor wasn't covered;
4. an unannotated accessor could move its root unnoticed;
5. an ERC-7201 namespace was "unchanged" on struct text alone, even with
   its accessor now pointing at a different root.

The verdict is now evidence-based: any proven conflict is INCOMPATIBLE;
otherwise any storage the check cannot see is UNKNOWN; only full coverage
is COMPATIBLE. The positional result keeps its own line either way, and
unrelated omissions (modifier or inherited-body analysis) never downgrade
storage.

Custom value types are compared by what they wrap, resolved from each
side's source. A name declared once (or always over the same type) resolves;
an ambiguous or missing one is a gap, never a name match. Width stays
compiler-reported fact, so a resize is proven even when a type is not.

Non-positional storage is found in the code that can run against the
proxy: the inheritance chain, plus library functions actually reachable
from it (by name, over-approximating toward UNKNOWN) — a merely imported
library still contributes nothing. Namespaces now also need every accessor
to resolve to the root their annotation defines; a root that provably moved,
or two structs on one root, is a conflict; unannotated accessors, raw
sload/sstore and delegatecall are gaps. Roots resolve only from bounded
forms: literals, unique constants, one local/getter hop (all overrides must
agree), and the keccak256, EIP-1967 and ERC-7201 expressions. The root math
is checked against OpenZeppelin's and EIP-1967's published constants.

No compiler is downloaded or executed; Sourcify serves no AST for these
contracts, so none is assumed.

On 3Jane, USD3's positional layout stays COMPATIBLE and Initializable is
root-verified, but the combined verdict becomes UNKNOWN, with the gaps
named: USD3 sload/sstores computed slots in two functions, its base
delegatecalls TokenizedStrategy (which writes the same proxy storage), and
TokenizedStrategyStorageLib aims a struct at a custom root.

The new regression tests were confirmed failing on the previous head
(14 of 18; the other 4 guard behavior that must not regress).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
spalen0 and others added 2 commits September 21, 2026 14:27
Brings in the six commits that landed on main since this branch forked,
including #366 (governance explanation evidence), which reworked
ai_explainer.py; both sides' edits auto-merged cleanly.

uv.lock was the only conflict. main had independently bumped anyio to
4.14.2, so its lock supersedes this branch's anyio-only bump and is taken
as-is; this branch never changed pyproject.toml, and `uv lock --check`
passes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second review of #368 found ways storage could still read wrongly. Each was
reproduced against the head first; the regression tests were confirmed
failing (10 of 12) before any fix, the other two guarding behavior that must
not change.

1. Aliased imports bypassed coverage. `import {Lib as State}` made
   `State.write()` resolve to nothing, so a library `sstore` read COMPATIBLE.
   Library calls, `using … for` and `is` bases are now read through each
   file's import aliases — the base-contract case wasn't in the review but
   dropped the base from the inheritance chain the same way.

2. Free functions were never inspected. Reachable file-level functions now
   join the storage scope; an unreached one still contributes nothing.

3. A local root was trusted after reassignment. `root = … + 256` before
   `$.slot := root` still validated the initializer. Any write to the local —
   assignment, compound, Yul `:=`, tuple destructuring — now leaves the root
   unresolved, and a local shadowing a constant of the same name no longer
   resolves to the constant.

4. Structs were matched by bare name. An unannotated `Lib.Main` borrowed the
   validation of an annotated `Vault.Main` at the same root, so a member
   retype in the library passed. Structs are now identified by where they are
   declared, resolved the way Solidity does: qualified names through aliases,
   a library's own, the one along a contract's inheritance chain, or a
   file-level one — anything else unresolved.

5. A shared root was called a proven conflict. Two structs at one root can be
   deliberate views of the same data; identically shaped ones are now fine,
   and different shapes are an unresolved overlap (UNKNOWN), not INCOMPATIBLE.

The 3Jane result is unchanged apart from gaps now naming structs by their
declaration.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@spalen0
spalen0 marked this pull request as ready for review September 22, 2026 10:04
spalen0 and others added 2 commits September 22, 2026 12:09
The frozen USD3/sUSD3 Etherscan bundles and Sourcify layouts, and the test
module built on them, pinned one protocol's upgrade. The impl diff is general
tooling for every monitored protocol, and each behavior those tests checked is
already covered by a synthetic fixture: ABI-derived surface, interface and base
declarations not leaking in, commented-out code, body-only and moved-into-helper
changes, compilation-target fallback, gap consumption, renames, compiler type-id
churn, value-type resolution, and root verification through a getter.

Also strips protocol names left in general code: docstrings and test fixtures
now use neutral names (IToken, IExternal) instead of IMorpho and 3Jane.

tests/fixtures/accountable_3jane_dashboard.json is unrelated and predates this
work — test_accountable.py and test_3jane.py still use it — so it stays.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… as a write

Three more ways storage coverage could miss code, each reproduced as
COMPATIBLE on the previous head before fixing:

1. `using {Lib.write} for uint256; v.write()` never pulled `Lib.write` into
   scope — only `using Lib for` and `Lib.f()` were recognized. Function-list
   directives are now parsed: a listed `Lib.f` references `Lib`, and every
   listed function counts as called outright, since one bound to an operator
   (`using {add as +}`) is invoked with no call text at all. Free functions
   in the list are covered the same way.

2. Two files each declaring a `Lib` with `write()` were deduplicated on
   (library, signature), so whichever came first masked the other — a
   harmless definition could hide the one doing `sstore`. The key now
   includes the file; with the binding unresolved, both stay in scope.

3. `delete root;` before `$.slot := root` zeroes the root, but the checker
   still validated the initializer. Deletion now counts as a write, which
   leaves the root unresolved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@spalen0
spalen0 merged commit 164f5ac into main Sep 22, 2026
3 checks passed
@spalen0
spalen0 deleted the llm-imp branch September 22, 2026 13:55
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.

bug(llm): implementation diff loses contract provenance and can misreport storage safety

1 participant