Skip to content

Callgraph improvements#52

Merged
emduc merged 6 commits intomainfrom
callgraph-improvements
Mar 6, 2026
Merged

Callgraph improvements#52
emduc merged 6 commits intomainfrom
callgraph-improvements

Conversation

@emduc
Copy link
Member

@emduc emduc commented Mar 6, 2026

Improving the callgraph for a special case in liquity where the contract calls 2 different oracles,

we now differentiate the calls and register the two dependencies.

Some small fixes in the liquity-v2 config as well.

@vercel
Copy link

vercel bot commented Mar 6, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
defiscan-v2-defiscan-frontend Ready Ready Preview, Comment Mar 6, 2026 9:37am
defiscan-v2-docs Ready Ready Preview, Comment Mar 6, 2026 9:37am
defiscan-v2-tool-frontend Ready Ready Preview, Comment Mar 6, 2026 9:37am

Request Review

@claude
Copy link

claude bot commented Mar 6, 2026

test

@claude
Copy link

claude bot commented Mar 6, 2026

Code review

4 issues found. Checked for bugs and CLAUDE.md compliance.


Bug 1: undefined can be stored as a Map key in buildTypeSubstitutionMap

File: callGraph.ts line 924–948

paramType is string | undefined due to optional chaining calledFunc.parameters[i]?.type. The guard if (!substitutions.has(paramType)) does not exclude undefined, so when a parameter has no type the code executes substitutions.set(undefined, argValue). JavaScript Map silently accepts undefined as a valid key. The resulting entry can never be matched by a real type string lookup, and on a second iteration the first-wins guard silently drops a legitimate substitution.

    if (paramType !== undefined && !substitutions.has(paramType)) {
      substitutions.set(paramType, argValue)
    }

Bug 2: REF_/TMP_ fallback picks an arbitrary map entry when multiple substitutions exist

File: callGraph.ts lines 938–941 and lines 1020–1026

Both fallback sites use:

parentSubstitutions.values().next().value  // line 938
typeSubstitutions.values().next().value     // line 1022

This picks the first inserted entry by Map insertion order. When the map has exactly one entry (the single-parameter case this PR was designed for) it is correct. But buildTypeSubstitutionMap creates one entry per distinct-typed parameter, so a function with two typed parameters — e.g., _compute(OracleA a, OracleB b) — produces {OracleA → ethUsdOracle, OracleB → btcUsdOracle}. If the REF_ was derived from b.aggregator, the fallback silently resolves to ethUsdOracle (wrong) instead of btcUsdOracle.

The comment at line 1020 acknowledges this with "internal functions typically have one relevant parameter" — this is a heuristic assumption, not a guarantee, and produces a wrong call graph edge with no warning when it doesn't hold.


Bug 3: VariableChainHeuristic nested-object branch lacks ABI validation

File: callGraphHeuristics.ts lines 134–145

The new nested-object branch returns all eth: addresses found in the struct with confidence 100 (single address) or 70 (multiple), without checking whether those addresses implement the called function. The DiscoveredValuesScanHeuristic added in the same PR correctly calls contractHasFunction() before returning any candidate — but VariableChainHeuristic always wins on confidence (100 vs 95), so the less-validated path overrides the more careful one.

Concrete failure case: a struct { fallbackFeed: "eth:0xWrong", aggregator: "eth:0xChainlink" } where only 0xChainlink has latestRoundData() would resolve to 0xWrong if it appears first in JSON order, with confidence 100 and no error.

Suggested fix: add the same contractHasFunction filter used by DiscoveredValuesScanHeuristic inside this branch, mirroring lines 364–374.


Issue 4: contractHasFunction is undocumented as unexported but CLAUDE.md describes it as a shared helper

File: callGraphHeuristics.ts line 45

contractHasFunction is defined without export, making it a private module-level function. However, CLAUDE.md — updated in this same PR — lists it under "Shared Traversal Helpers" alongside genuinely exported functions from callGraph.ts (traverseWithPaths, extractEthAddresses, etc.), implying it is importable by other modules. Any developer following the docs and trying to import contractHasFunction in e.g. functionAnalysis.ts would get a compile error.

Either add export to the function definition (if cross-module use is intended) or update CLAUDE.md to clarify it is internal to callGraphHeuristics.ts.

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