Skip to content

fix(cfg): link fallback block from entry node in control flow graph#705

Merged
Jon-Becker merged 1 commit into
mainfrom
fix/627-cfg-fallback
Jul 5, 2026
Merged

fix(cfg): link fallback block from entry node in control flow graph#705
Jon-Becker merged 1 commit into
mainfrom
fix/627-cfg-fallback

Conversation

@Jon-Becker

@Jon-Becker Jon-Becker commented Jul 5, 2026

Copy link
Copy Markdown
Owner

What changed? Why?

Fixes #627. The cfg command was omitting the edge from the entry node to the fallback handler in the control flow graph. For contracts like WETH (0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2), node 0 should link to the fallback block when CALLDATASIZE < 4, but that edge was missing.

Root cause: build_cfg deduplicated blocks using a HashSet<String> and returned early whenever a block was already seen—before the parent→child edge was added. Since the symbolic-execution trace visits the dispatcher before the entry node's direct jump-taken branch, the fallback block gets mapped first as a dispatcher descendant. When the entry node later tries to link to it, the block is already in seen_nodes, so the edge is silently dropped.

Fix: Changed seen_nodes to a HashMap<String, NodeIndex<u32>>. When a block has already been seen, we look up its node index and still add the edge from the current parent before returning (no recursion, so loop protection is preserved).

Notes to reviewers

  • Modified files: crates/cfg/src/core/graph.rs and crates/cfg/src/core/mod.rs
  • Added live-contract regression test: test_cfg_links_fallback_block in crates/core/tests/test_cfg.rs
  • The fix is minimal: changed the deduplication data structure and added the missing edge logic

How has it been tested?

  • Verified on live WETH contract: before the fix node 0 only had 0 -> 1; after, it correctly has 0 -> 1 (fall-through) and 0 -> 67 (jump to fallback at 0xaf)
  • Added test_cfg_links_fallback_block, a live-contract regression test that uses the RPC endpoint to verify the entry node links to the 0xaf fallback block
  • All existing cfg integration and unit tests pass
  • Code is clean: cargo +nightly fmt --check and cargo clippy --all-features pass

The CFG builder deduplicated blocks with a `HashSet`, returning early
whenever a block had already been visited. This dropped the edge from the
current parent into the existing block, so when a fallback handler was
first reached through the dispatcher's fall-through path, the direct
`CALLDATASIZE < 4` edge from the entry node was never added.

Track visited blocks in a `HashMap<String, NodeIndex>` instead, so an
already-seen block's node index can be looked up and the parent edge still
linked (without recursing again). This restores the missing entry ->
fallback edge, e.g. for WETH (0xc02a...cc2) node 0 now links to 0xaf.

Closes #627
@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

❌ Eval Report for fea72d3

Test Case CFG Decompilation
NestedMappings 100 42
NestedMapping 100 10
TransientStorage 100 22
Mapping 100 88
SimpleStorage 100 85
SimpleLoop 85 10
WhileLoop 75 10
NestedLoop 55 10
WETH9 100 68
Events 92 63
Average 90 40
⚠️ 8 eval(s) scoring <70%

NestedMappings (CFG: 100, Decompilation: 42)

Decompilation

{
  "score": 42,
  "summary": "The decompilation captures that approve() writes the amount into a storage mapping, but it fails to represent the nested-mapping semantics and completely loses the read logic. Both getter functions (allowance and the public mapping accessor) are rendered as pure no-op requires that neither read storage nor return a value.",
  "differences": [
    "allowance(address,address) (selector 0xdd62ed3e) is decompiled as a single-argument pure function that only performs an address bounds check; it does not read the nested mapping storage and does not return the stored allowance value.",
    "The public mapping getter allowances(address,address) (selector 0x55b6ed5c) is likewise decompiled as a pure no-op with no storage read and no return value, losing the getter's entire behavior.",
    "In approve(), the nested mapping key derivation is incomplete: the storage write is keyed only on the spender argument (arg0) and omits the msg.sender dimension, so it does not reflect allowances[msg.sender][spender].",
    "State reads returning uint256 are entirely absent from the output; two of the three functions no longer perform their core storage-read-and-return operation."
  ]
}

NestedMapping (CFG: 100, Decompilation: 10)

Decompilation

{
  "score": 10,
  "summary": "The decompilation fails to capture the fundamental behavior of the contract. The original has setter/getter functions that write and read nested mappings (allowances, grid, deepNested), but the decompiled output contains only trivial pure functions with tautological requires (arg0 == arg0, arg0 == address(arg0)) and no storage operations whatsoever. Function selectors do not match, no SSTORE/SLOAD are present, and no mapping slot calculations appear.",
  "differences": [
    "All storage writes are missing: setAllowance, setGrid, and setDeepNested each perform an SSTORE into a nested mapping slot, but no storage assignment appears anywhere in the decompiled output.",
    "The storage read in getAllowance (SLOAD of allowances[owner][spender] with a return value) is missing; no function returns a value.",
    "Function mutability is wrong: original setters are state-mutating (public), but the decompiled functions are all marked pure.",
    "Function selectors do not correspond to the original functions; none of the decompiled selectors match setAllowance/setGrid/setDeepNested/getAllowance signatures.",
    "The require conditions are tautological (arg0 == arg0, arg0 == address(arg0)) and represent no logic present in the original source.",
    "Nested mapping slot (keccak) computations for two- and three-level mappings are entirely absent."
  ]
}

TransientStorage (CFG: 100, Decompilation: 22)

Decompilation

{
  "score": 22,
  "summary": "The decompilation fails to preserve the logic of five of the six functions. Transient storage reads/writes (TLOAD/TSTORE) were largely misinterpreted: incrementCounter, lock, and unlock are collapsed into empty/placeholder constants with no operations, and getCounter and isLocked are rendered as hardcoded constants (1 and true) rather than reading state. Only setTempOwner retains a recognizable transient-storage write.",
  "differences": [
    "incrementCounter: the increment logic (read counter, add 1, store back) is entirely lost; it is rendered as an empty constant with no state read or write.",
    "lock: the write of `true` to the locked transient variable is lost; rendered as an empty constant.",
    "unlock: the write of `false` to the locked transient variable is lost; rendered as an empty constant.",
    "getCounter: instead of reading the transient counter and returning it, the output returns a hardcoded constant value of 1, dropping the storage read.",
    "isLocked: instead of reading the transient locked flag and returning it, the output returns a hardcoded constant true, dropping the storage read.",
    "setTempOwner: captured as a transient storage write but marked `pure` despite modifying state, and the write is obscured by unnecessary masking/multiplication operations not present in the original simple assignment."
  ]
}

SimpleLoop (CFG: 85, Decompilation: 10)

Decompilation

{
  "score": 10,
  "summary": "The decompilation fails to capture the fundamental behavior of the contract. The original loop() iterates `loops` times, incrementing the storage variable `number` on each iteration. The decompiled output contains no loop, no storage write, and incorrectly marks the function as `view`. The body consists of nonsensical require statements that bear no relation to the original logic.",
  "differences": [
    "The for-loop control flow is entirely absent from the decompiled output",
    "The storage write (number++) is missing; no state change occurs, and the function is incorrectly marked as `view` when the original is a state-mutating public function",
    "The loop counter increment and bound comparison (i < loops) are not represented; instead there are meaningless require statements (e.g., require(arg0 == arg0), require(!0 < arg0))",
    "The storage read/modify/write of `number` is misrepresented as require(number - 0x...ff) rather than an increment"
  ]
}

WhileLoop (CFG: 75, Decompilation: 10)

Decompilation

{
  "score": 10,
  "summary": "The decompilation fails to capture the fundamental behavior of the function. The original is a state-mutating while loop that increments storage variable `number` by 1 for each iteration up to `loops`. The decompiled output contains no loop, performs no storage write, and is incorrectly marked as `view`. The loop body and the state change are entirely absent, leaving only meaningless tautological require statements.",
  "differences": [
    "The while loop control flow is completely missing; there is no iteration construct in the decompiled output.",
    "The storage write `number = number + 1` is not preserved; the function performs no SSTORE and instead only contains require checks.",
    "Function mutability is incorrect: original mutates state (non-view), decompiled is marked `public view`.",
    "The comparison `i < loops` and loop counter `i` logic is lost, replaced by nonsensical `require(!0 < arg0)` and `require(arg0 == arg0)`.",
    "The addition/increment logic is reduced to a require condition `require(!number > (number + 0x01))` rather than an actual computed and stored value."
  ]
}

NestedLoop (CFG: 55, Decompilation: 10)

Decompilation

{
  "score": 10,
  "summary": "The decompilation fails to capture the fundamental behavior of the contract. The original performs a nested loop that increments the storage variable `number` loops*loops times. The decompiled output contains no loop structure, no storage write, and is incorrectly marked as `view`.",
  "differences": [
    "Nested for-loop control flow is entirely absent; loop iteration and bounds (i < loops, j < loops) are reduced to meaningless static require() comparisons",
    "The storage write `number += 1` is missing; the accumulation into state is not performed, replaced by a nonsensical require on `number + 1`",
    "Function mutability is wrong: marked `public view` despite the original mutating storage",
    "Function performs no state change and effectively becomes a no-op/revert path instead of the incrementing loop logic"
  ]
}

CFG

{
  "score": 55,
  "summary": "The CFG fully captures the dispatcher, both function selectors (loop and number), the fallback, all revert/require paths, and the entry+condition of both nested for-loops. However, the defining feature of this loop-centric contract is incomplete: neither loop's back-edge (iteration cycle) is represented, the inner loop's exit edge is missing, and the loop-body's success continuation is absent, so the loops appear as forward-only branches rather than true cycles.",
  "missing_paths": [
    "Inner for-loop exit edge: node 8's condition (0x87 LT / 0x88 ISZERO) jumps to 0xb1 when the condition is false, but no node/edge for the inner-loop exit target 0xb1 exists.",
    "Inner for-loop back-edge: after 'number += 1', the increment (j++) and the jump back to the inner condition (0x84) are not represented — the loop iteration cycle is missing.",
    "Outer for-loop back-edge: after the inner loop exits, the outer increment (i++) and jump back to the outer condition (0x77) are not represented.",
    "Loop-body happy path: node 9 (number += 1) shows only the edge to the overflow-panic revert (9->10); its non-overflow continuation (jump to 0x01ac that resumes normal loop iteration) is missing, leaving the body a dead-end."
  ],
  "extra_paths": [
    "Callvalue non-zero revert (0->1) from the compiler-inserted non-payable check.",
    "Calldatasize < 4 revert (2->15) from the dispatcher.",
    "Calldata decode SLT/bounds revert paths (4->12, 5->6) added by the ABI decoder.",
    "Arithmetic overflow Panic(0x11) revert (9->10) from the compiler's checked-add of number += 1."
  ],
  "observations": [
    "Function dispatch is complete: both selectors (0x0b7d796e loop, 0x8381f58a number) and the fallback revert (15) are all captured with correct branch structure.",
    "Both loop condition tests are present with correct opcodes (outer at 0x7a LT/0x7b ISZERO, inner at 0x87 LT/0x88 ISZERO), and the outer loop correctly has both a body edge (7->8) and an exit edge (7->11->STOP).",
    "The asymmetry is telling: the outer loop's exit edge is present but the inner loop's exit edge is not, and neither loop's back-edge is present, indicating the CFG extraction traced entries and conditions but did not resolve the iteration/increment blocks and their back-jumps.",
    "Because this is specifically a nested-loop contract, the missing back-edges and inner-loop exit represent omission of the core control-flow structure the source is built around."
  ]
}

WETH9 (CFG: 100, Decompilation: 68)

Decompilation

{
  "score": 68,
  "summary": "Most functions are faithfully reconstructed (deposit, withdraw, totalSupply, and the core balance-transfer arithmetic/events), but the transferFrom allowance-check control flow is mangled into revert conditions, approve writes to the wrong storage mapping, and the balance/allowance mappings are split across two inconsistent slot names.",
  "differences": [
    "transferFrom: the original conditional `if (src != msg.sender && allowance[src][msg.sender] != uint(-1)) { require(allowance >= wad); allowance -= wad; }` is misreconstructed as unconditional `require(arg0 == msg.sender)` and `require(allowance == 0xff..ff)`, turning a branch into revert conditions; the allowance decrement only appears in unreachable duplicate code blocks rather than on a proper conditional path.",
    "transfer: same allowance branching logic (via the inlined transferFrom call) is misrepresented as spurious require statements, and the function body contains duplicated unreachable code after the first return.",
    "approve: writes to `storage_map_c[arg0]` (the single-key balance-style mapping) instead of the nested allowance mapping `allowance[msg.sender][guy]`; the two-level mapping key (msg.sender then guy) is not captured.",
    "The balance mapping is written as `storage_map_c` (in deposit/withdraw/transfer) but the balanceOf getter reads `storage_map_d`, and the allowance getter also reads `storage_map_d`, so reads and writes reference inconsistent slot names and the nested allowance mapping structure is flattened.",
    "withdraw: the ETH transfer is modeled as `.transfer(arg0)` returning (bool success, bytes) with the success value unused; the original `msg.sender.transfer(wad)` reverts on failure (behavior is roughly equivalent but the return handling representation differs)."
  ]
}

Events (CFG: 92, Decompilation: 63)

Decompilation

{
  "score": 63,
  "summary": "This is an events-only contract, so faithful event emission is its entire purpose. Five of the seven functions preserve their event emissions (Deposit, Withdrawal, LogBytes, Log, and the multi-event emitMultiple with Deposit+Transfer+Log). However, two functions lose their core behavior: the standalone Transfer emission and the entire Approval emission are dropped, and the Approval event is not even declared. Where events are preserved, the topic/parameters map correctly, though string/bytes payloads are shown in low-level memory-offset notation.",
  "differences": [
    "emitTransfer (Unresolved_5687f2b8): the Transfer event emission is missing entirely; the function only contains a require and shows a single address argument instead of (from, to, value), so no Transfer log is produced.",
    "emitApproval (Unresolved_23de6651): the Approval event emission is completely lost and the Approval event is not even declared in the decompiled output; the function reduces to a require with a single address argument.",
    "emitLog / emitLogBytes: the string and bytes payloads passed to Log/LogBytes are represented with raw memory-offset expressions (e.g. (var_b + 0x20) - var_b) and length/data selectors rather than the actual dynamic argument, obscuring the exact data emitted (the constant string in emitMultiple is preserved as a hex literal, however).",
    "Several functions include redundant/degenerate require checks (e.g. require(arg1 == arg1), require(!arg0 > 0x...)) that do not correspond to original logic, though these are largely no-ops."
  ]
}

@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

⚠️ Coverage Report for fea72d3

Metric Value
Base branch 66.50%
PR branch 66.15%
Diff -0.36%

@Jon-Becker Jon-Becker merged commit 1570fad into main Jul 5, 2026
13 of 15 checks passed
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.

link fallback block in cfg

1 participant