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

Summary

The LLM implementation-diff enrichment loses contract provenance for multi-file Etherscan verifications. It concatenates every source file in the compiler bundle and runs regex-based function/storage extraction over the result as if it were the deployed contract.

This produces both false positives and false negatives. In the 3Jane USD3/sUSD3 upgrade it attributed USD3 functions and IMorpho interface declarations to sUSD3, treated a commented-out function as deployed, skipped the positional storage check because an imported library uses namespaced storage, and completely missed sUSD3's actual body-only change.

This should be treated as a correctness issue in deterministic evidence, rather than something to solve with prompt wording.

Bad generated report: https://gist.wavey.info/jnyVBGWwgmFKpvzrzSn83IuY

Inspected at c15876492a4bf5669c1d85cddf2d23336ecc89bd.

Follow-up validation

Sourcify's v2 contract endpoint already exposes matched compiler storageLayout output, so the plan below uses Sourcify rather than downloading and executing solc binaries in the monitoring cron.

Verified coverage for this regression:

Implementation	Sourcify result	Layout entries
Old USD3	creation + runtime match	13
New USD3	creation + runtime match	16
Old sUSD3	creation + runtime match	8
New sUSD3	unavailable (match: null)	—

For USD3, the compiler layouts show slots 0–62 preserved, new fields in slots 63–65, and the reserved gap moving from uint256[40] at slot 63 to uint256[37] at slot 66. Both layouts end at the same slot boundary.

The implementation should use COMPATIBLE / INCOMPATIBLE / UNKNOWN: this is a storage-compatibility result, not an overall safety verdict.

Impact

The current implementation can:

• report functions from imported contracts/interfaces as callable on the upgraded proxy;
• report commented-out Solidity as deployed code;
• apply one implementation's changes to another implementation compiled from an overlapping source bundle;
• hide a real external addition when the same signature exists in an imported interface/base;
• miss every behavior change made only inside an existing function body;
• label storage checking as skipped/safe because any imported dependency contains a namespaced-storage getter;
• produce incorrect storage-safety results because declaration order is not Solidity storage layout.

The storage path is the highest-risk part: it can suppress or misclassify a genuinely incompatible proxy upgrade.

Reproduction

Implementations:

Contract	Old	New
USD3	0xB606fB370Eaaad03d71B49aE5E42AA4aEC7458D9	0xd1F1c3F485063712873285BF4ef25ab068f13893
sUSD3	0x529cbf11fFbC272D63858ca40A2C7F2695712073	0x6093d95f6C102163D19F5681E7D84997060BBAed

Running the current extractor over the exact Etherscan bundles produces the same eight additions for both upgrades:

_pendingLoss() internal view returns (bool)
_wrapUSDC(uint256,bool) private
_deployDepositedFunds() private
setSupplyCapExempt(address,bool) external onlyManagement
setRingFenceConduit(address,bool) external onlyManagement
releaseRingFence(uint256) external onlyManagement
clearMarketWindDown(Id) external
marketInWindDown(Id) external view returns (bool)

Their real provenance is:

• the first six are in src/usd3/USD3.sol;
• clearMarketWindDown and marketInWindDown are declarations in imported src/interfaces/IMorpho.sol;
• none of them is an sUSD3 external-surface addition.

clearMarketWindDown was consequently described as an unpermissioned function on USD3/sUSD3. The real implementation is on MorphoCredit and is onlyOwner.

The extractor also reports restartStrategy() as removed, although it is commented out in the old verified USD3 source. _extract_function_sigs() searches the unstripped source, unlike the state-variable extractor.

Correct ABI-level surface diff

The verified target ABIs give the following deterministic result.

USD3 additions:

releaseRingFence(uint256)
ringFenceConduit(address)
ringFencedLiquidity()
setProfitMaxUnlockTime(uint256)
setRingFenceConduit(address,bool)
setSupplyCapExempt(address,bool)
supplyCapExempt(address)

USD3 removals:

depositTimestamp(address)
depositorWhitelist(address)
initialize(address,bytes32,address,address)
minCommitmentTime()
reinitialize()
setDepositorWhitelist(address,bool)
setWhitelist(address,bool)
setWhitelistEnabled(bool)
whitelist(address)
whitelistEnabled()

sUSD3 has no external ABI additions or removals. Its relevant change is inside the existing availableDepositLimit(address) body, where a stale-accounting deposit guard was added. The current diff explicitly does not compare function bodies, so it misses the only material sUSD3 delta.

The current source diff also misses USD3's real setProfitMaxUnlockTime(uint256) addition because the same signature already occurs elsewhere in the concatenated old source bundle and the signature-keyed dict collapses provenance.

Root cause

1. Multi-file source is flattened

_fetch_etherscan_contract() stores _concat_sources(raw_source). _concat_sources() joins every sources[*].content value and discards filenames, the compilation target, and contract boundaries.

This representation is useful for best-effort text search, but it cannot support a contract-level semantic diff.

2. Function extraction is textual and unscoped

_FUNCTION_DEF_RE runs across the flattened raw text. It includes:

• the target contract;
• imported contracts and libraries;
• interfaces;
• public, external, internal, and private functions;
• commented-out functions.

_diff_functions() then converts the list to a dict keyed only by (name, args). Duplicate declarations overwrite each other according to arbitrary source-bundle order. An interface declaration can therefore replace an implementation declaration and erase modifiers/provenance.

3. Body changes are invisible

The implementation intentionally compares signatures/modifiers but not bodies. A proxy upgrade can preserve its ABI while changing all behavior, so the most important class of implementation change is absent.

4. Storage extraction is not a storage layout

The state-variable parser aggregates declarations from every contract/library at brace depth 1. It does not model:

• the selected compilation target;
• C3 inheritance linearization;
• packing and byte offsets;
• structs and fixed arrays consuming multiple slots;
• user-defined value types and compiler canonical types;
• inherited storage gaps;
• the fact that renaming a variable does not move its slot.

Gap consumption is calculated from declaration count rather than actual occupied slots.

5. Namespaced-storage detection is bundle-global

_is_namespaced_storage(old_src) or _is_namespaced_storage(new_src) becomes true if any imported dependency has a matching getter. The code then sets storage_layout_safe = True while skipping comparison.

In this incident the match came from imported TokenizedStrategyStorageLib, while USD3 itself still has ordinary positional storage.

6. The LLM amplifies the deterministic error

The prompt receives the diff without file/contract provenance. Once the summary adopts a risk level, the detail pass is required to remain consistent with it. The current risk anchor also says upgradeAndCall runs an initializer even when the data argument is empty.

Proposed solution

P0: fail closed immediately

1. Stop emitting regex-derived implementation function/storage claims from flattened source.

2. Replace the boolean storage result with a tri-state status: COMPATIBLE, INCOMPATIBLE, or UNKNOWN.

3. A skipped, unavailable, or namespaced layout check must be UNKNOWN, never internally represented as safe.

4. When both sides do not have matched Sourcify compiler layouts, render:

   Storage compatibility: not validated automatically; inspect compiler layouts manually.
   

5. Make the risk anchor conditional on the third upgradeAndCall argument:

   • empty bytes: replaces implementation only;
   • non-empty bytes: replaces implementation and delegatecalls the payload.

Ship this fail-closed storage cutover together with the ABI surface replacement below. There should be no intentional window without a function-surface section: the ABI is already present in the cached Etherscan response.

This removes dangerous deterministic misinformation without blocking alerts. If the Sourcify integration cannot land atomically, disable the old storage checker first and accept temporary UNKNOWN output rather than preserving a false compatibility verdict.

P1: preserve a structured verified-contract record

Replace the cached (contract_name, concatenated_source, abi_json) tuple with a versioned structure along these lines:

@dataclass(frozen=True)
class VerifiedContract:
    contract_name: str
    compiler_version: str
    contract_file: str | None
    compilation_target: tuple[str, str] | None
    language: str
    sources: dict[str, str]
    settings: dict[str, object]
    abi: list[dict]

Preserve Etherscan's standard-json source map and settings. Resolve the target using, in order:

1. Etherscan's contract-file/compilation-target field when available;
2. settings.compilationTarget when present;
3. a unique AST ContractDefinition matching ContractName;
4. otherwise leave target-body analysis unavailable; the ABI surface diff can still proceed.

Keep concatenation only as an explicitly named search helper; semantic consumers must use the structured record.

The disk cache needs a schema/namespace bump. Positive source entries do not expire, so retaining the current cache would preserve flattened records after deployment.

P0: replace function claims with an ABI diff in the same cutover

Use the verified old/new ABIs as the authoritative external/public surface:

• canonicalize tuple and array types;
• key functions by selector/canonical signature;
• report additions, removals, and stateMutability changes;
• include generated public getters naturally;
• never include interfaces, comments, internal functions, or private functions.

Each result should carry provenance:

USD3 ABI: + setSupplyCapExempt(address,bool)
sUSD3 ABI: no external-surface changes

Syntactic modifiers such as onlyOwner are not present in ABI data. Do not infer them from interfaces. Add them only after target-scoped AST resolution, and label them as source-level modifiers rather than a complete authorization proof.

P1: use Sourcify compiler storage layouts

Fetch both implementations from:

GET https://sourcify.dev/server/v2/contract/{chainId}/{address}?fields=storageLayout

Only produce a compatibility result when both responses have a verified match and a non-empty compiler storageLayout. A missing contract currently returns HTTP 200 with match: null, so HTTP status alone is not evidence of coverage.

Comparison requirements:

• key top-level entries by numeric slot and offset;
• recursively normalize types using encoding, numberOfBytes, array base/length, mapping key/value, and struct-member slot/offset/type;
• ignore compiler-internal type IDs, AST IDs, variable labels, and source-contract labels for compatibility;
• display renames for reviewer context, but do not classify a rename alone as incompatible;
• evaluate gap consumption using actual compiler slots and byte widths;
• cache successful layouts by chain/address;
• cache misses with a short TTL because Sourcify coverage can appear later;
• treat network errors, missing matches, malformed layouts, or one-sided coverage as UNKNOWN.

Sourcify has already performed the deployed-bytecode match, avoiding compiler-version management, library linking, IR-codegen drift, integrity verification, and sandboxing inside the monitoring cron.

Do not add automatic solc downloading/execution to the runtime path. A local compiler fallback can be reconsidered separately if Sourcify coverage proves insufficient, but it is not required for this fix.

P2: validate namespaced storage separately

Do not treat the presence of one ERC-7201-style getter as meaning the whole contract is namespaced.

For each namespace owned by the target/inherited implementation:

• resolve its namespace/location identifier;
• compare the namespace's struct-member layout independently;
• detect namespace-location collisions;
• separately compare any ordinary positional storage that still exists.

Until that validator exists, namespaced storage should produce UNKNOWN (namespaced layout not validated), not a compatible result.

OpenZeppelin upgrades-core may be worth evaluating for this part, but it should consume the exact verified build input/output rather than flattened source.

P1: surface target-defined body changes without a compiler

ABI equality is not behavioral equality. Once the compilation target and its source file are resolved, compare functions defined directly in that contract without waiting for compiler AST output:

1. Lexically isolate the selected contract body.
2. Locate target-defined function headers and bodies with comment/string-aware brace matching.
3. Match overloads by canonical signature, not function name alone.
4. Hash normalized headers and bodies so formatting/comments do not create noise, while literal changes remain visible.
5. Report additions/removals through ABI and changed target-defined bodies separately.
6. Include a small target-scoped unified diff or source links for changed functions; do not ask the LLM to infer semantics from a hash alone.

This catches the sUSD3 availableDepositLimit() change immediately.

The result must explicitly list what remains unvalidated: inherited function and modifier bodies, linked/inlined libraries, imported constants, and free functions can alter behavior even when the target-defined function body is unchanged. If target resolution or overload matching is ambiguous, report body analysis as unavailable rather than searching the flattened bundle.

P3: prompt and report guardrails

1. Give every diff fact a contract and file origin.
2. Separate deterministic categories:
   • External ABI changes
   • Changed function bodies
   • Storage compatibility
   • Unvalidated items
3. Do not call internal/private additions a new governance "control surface".
4. Add a consistency gate before invoking the LLM:
   • every external addition/removal must agree with the respective target ABI;
   • no interface-only declaration can support an access-control claim;
   • identical diffs across different target contracts should trigger a provenance check.

Regression tests

Add frozen raw Etherscan responses for the four addresses above and assert:

• USD3 gets only its target ABI additions/removals;
• sUSD3 gets no ABI additions/removals;
• clearMarketWindDown never appears in either diff;
• commented restartStrategy is ignored;
• setProfitMaxUnlockTime is not masked by an imported duplicate signature;
• sUSD3's availableDepositLimit body is marked changed;
• imported namespaced helpers do not suppress positional storage analysis;
• the resulting prompt never says that both implementations add the same functions.

Add focused fixtures for:

• duplicate signatures across interface/base/target;
• comments containing function-like text;
• overloaded tuple/array signatures;
• inherited storage and C3 ordering;
• packed variables and offsets;
• structs/fixed arrays spanning slots;
• safe variable rename;
• correct and incorrect storage-gap consumption;
• mixed positional and ERC-7201 storage;
• missing, unmatched, malformed, or one-sided Sourcify layout returning UNKNOWN.

Acceptance criteria

• No semantic diff consumes concatenated source.
• External function changes are ABI-derived and target-scoped.
• Body-only changes are visible as such.
• Storage is never called compatible without matched compiler layouts for both implementations.
• Namespaced and positional storage are evaluated independently or reported UNKNOWN.
• Every diff line includes target provenance.
• The 3Jane regression fixture produces the expected USD3 surface diff and an unchanged sUSD3 ABI.

Start at _fetch_etherscan_contract(), _concat_sources(), _FUNCTION_DEF_RE, _diff_functions(), and the storage-checking path described in the issue. Review the cached contract representation, ABI data, and Sourcify storageLayout endpoint before changing consumers. Done means provenance is preserved, ABI changes are authoritative, storage results are tri-state, and unavailable layouts fail closed as UNKNOWN.