feat(decompile): loop detection and analysis#674
Open
Jon-Becker wants to merge 14 commits into
Open
Conversation
Contributor
✅ Coverage Report for 592674d
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Early filter: skip loop detection for conditions that can't be loops | ||
| // (tautological or constant-only comparisons like overflow checks) | ||
| let condition_could_be_loop = | ||
| jump_condition.as_ref().map_or(true, |cond| { |
Contributor
There was a problem hiding this comment.
warning: this `map_or` can be simplified
--> crates/vm/src/ext/exec/mod.rs:252:29
|
252 | / ... jump_condition.as_ref().map_or(true, |cond| {
253 | | ... !is_tautologically_false_condition(cond) &&
254 | | ... !is_tautologically_true_condition(cond)
255 | | ... });
| |________________________^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#unnecessary_map_or
= note: `#[warn(clippy::unnecessary_map_or)]` on by default
help: use is_none_or instead
|
252 - jump_condition.as_ref().map_or(true, |cond| {
252 + jump_condition.as_ref().is_none_or(|cond| {
|
| } | ||
|
|
||
| // check for mutated memory accesses in the jump condition | ||
| if jump_condition_contains_mutated_memory_access(&diff, cond) { |
Contributor
There was a problem hiding this comment.
warning: this `if` statement can be collapsed
--> crates/vm/src/ext/exec/mod.rs:299:37
|
299 | / ... if jump_condition_contains_mutated_memory_access(&diff, cond) {
300 | | ... if stack_diff_shows_iteration(&diff, cond) {
301 | | ... detected_loop_info = Some((diff, cond.clone()));
302 | | ... break;
303 | | ... }
304 | | ... }
| |_______________________^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#collapsible_if
= note: `#[warn(clippy::collapsible_if)]` on by default
help: collapse nested if block
|
299 ~ if jump_condition_contains_mutated_memory_access(&diff, cond)
300 ~ && stack_diff_shows_iteration(&diff, cond) {
301 | detected_loop_info = Some((diff, cond.clone()));
302 | break;
303 ~ }
|
| } | ||
|
|
||
| // check for mutated storage accesses in the jump condition | ||
| if jump_condition_contains_mutated_storage_access(&diff, cond) { |
Contributor
There was a problem hiding this comment.
warning: this `if` statement can be collapsed
--> crates/vm/src/ext/exec/mod.rs:307:37
|
307 | / ... if jump_condition_contains_mutated_storage_access(&diff, cond) {
308 | | ... if stack_diff_shows_iteration(&diff, cond) {
309 | | ... detected_loop_info = Some((diff, cond.clone()));
310 | | ... break;
311 | | ... }
312 | | ... }
| |_______________________^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#collapsible_if
help: collapse nested if block
|
307 ~ if jump_condition_contains_mutated_storage_access(&diff, cond)
308 ~ && stack_diff_shows_iteration(&diff, cond) {
309 | detected_loop_info = Some((diff, cond.clone()));
310 | break;
311 ~ }
|
Contributor
|
| Test Case | CFG | Decompilation |
|---|---|---|
| WhileLoop | 100 | 100 |
| NestedMappings | 100 | 30 |
| SimpleLoop | 100 | 100 |
| TransientStorage | 100 | 15 |
| Mapping | 100 | 25 |
| NestedLoop | 100 | 100 |
| Events | 100 | 70 |
| SimpleStorage | 100 | 35 |
| NestedMapping | 100 | 35 |
| WETH9 | 100 | 45 |
| Average | 100 | 55 |
⚠️ 6 eval(s) scoring <70%
NestedMappings (CFG: 100, Decompilation: 30)
Decompilation
{
"score": 30,
"summary": "Decompilation fails to preserve nested mapping structure, resulting in incorrect storage access patterns that fundamentally change program behavior",
"differences": [
"Nested mapping storage access is simplified to single-key mapping lookups, losing the two-dimensional structure required for allowances[owner][spender]",
"allowance() function ignores the owner (arg0) parameter and only uses spender (arg1) for storage lookup, when it should compute storage slot from both addresses",
"approve() function ignores msg.sender in storage calculation and only uses spender (arg0), when it should compute storage slot from both msg.sender and spender",
"The public allowances() getter uses incorrect logic (var_b = arg0; var_b = arg1;) that overwrites the first address with the second, losing the owner parameter entirely"
]
}TransientStorage (CFG: 100, Decompilation: 15)
Decompilation
{
"score": 15,
"summary": "Critical decompilation failure - only 1 of 6 functions partially recovered, with incorrect logic. Transient storage operations not properly handled, resulting in missing functions and incorrect constant declarations instead of function implementations.",
"differences": [
"incrementCounter function missing - declared as empty constant instead of function that increments transient counter",
"lock function missing - declared as empty constant instead of function that sets transient locked to true",
"unlock function missing - declared as empty constant instead of function that sets transient locked to false",
"getCounter function missing - declared as constant uint256=1 instead of view function returning transient counter value",
"isLocked function missing - declared as constant bool=true instead of view function returning transient locked state",
"setTempOwner has incorrect logic with unnecessary bit manipulation and redundant require check - should be simple assignment to transient storage",
"All transient storage read/write operations failed to decompile correctly"
]
}Mapping (CFG: 100, Decompilation: 25)
Decompilation
{
"score": 25,
"summary": "Critical storage mapping errors cause fundamental logic failure. Getter functions read from wrong storage locations, and setter functions use incorrect bit-packing operations instead of simple assignments.",
"differences": [
"Public getter functions (balances, owners, registered) read from storage_map_b while setter functions write to storage_map_a, causing complete disconnect between writes and reads",
"setOwner uses incorrect bit-packing logic (address * 0x01 | uint96(...)) instead of simple address assignment to owners[tokenId]",
"register uses incorrect bit-packing logic (0x01 * 0x01 | uint248(...)) instead of simple boolean assignment to registered[address]",
"Storage slot calculations are fundamentally wrong - contract conflates three distinct mappings into two storage maps with incorrect access patterns",
"getBalance correctly reads from storage_map_a matching setBalance, but the public balances() getter reads from storage_map_b, creating inconsistent state access"
]
}SimpleStorage (CFG: 100, Decompilation: 35)
Decompilation
{
"score": 35,
"summary": "Decompilation captures only 1 of 4 functions correctly. Critical failures in storage layout interpretation lead to fundamentally incorrect logic in setOwner, initialize, and reset functions. The decompiler appears to incorrectly pack storage variables and generates nonsensical bitwise operations instead of simple assignments.",
"differences": [
"Missing 'initialized' boolean state variable - not declared in decompiled contract",
"setOwner function uses incorrect bitwise operations (preserving upper bits) instead of clean address assignment",
"initialize function incorrectly modifies owner variable with bitwise operations instead of setting a separate boolean to true",
"reset function has fundamentally broken logic: performs 'owner = value | (uint96(owner))' which incorrectly mixes value and owner variables",
"reset function fails to properly reset owner to address(0), using incorrect bitwise operations instead",
"reset function does not reset the initialized boolean (missing entirely from decompiled output)"
]
}NestedMapping (CFG: 100, Decompilation: 35)
Decompilation
{
"score": 35,
"summary": "Decompilation fails to correctly handle nested mapping storage calculations in setter functions, causing critical functional bugs. While public getter functions appear correct, the corresponding setter functions only use partial keys for storage slot computation, which would cause storage collisions and incorrect state updates.",
"differences": [
"setAllowance function only uses the spender address (arg1) for storage calculation, completely omitting the owner address (arg0) from the nested mapping key computation. This breaks the two-level mapping structure.",
"getAllowance function has the same issue as setAllowance - only considers spender (arg1) and ignores owner (arg0) in storage lookup, causing incorrect reads.",
"setGrid function only uses the y-coordinate (arg1) for storage calculation, missing the x-coordinate (arg0). Different x values with the same y would overwrite each other instead of occupying separate storage slots.",
"setDeepNested function only uses the last two keys (arg1 and arg2) in storage calculation, omitting the first key (arg0) from the three-level nested mapping computation."
]
}WETH9 (CFG: 100, Decompilation: 45)
Decompilation
{
"score": 45,
"summary": "Decompilation captures basic structure but has critical logic errors in transfer functions, incorrect storage mapping in approve, and malformed control flow with unreachable code.",
"differences": [
"transfer() function has severely corrupted logic: includes a nonsensical 'for' loop with unreachable code after early return, and incorrect condition checks that don't match the original transferFrom delegation",
"transferFrom() has similar corruption with unreachable for-loop code after return statement and malformed conditional logic that doesn't properly represent the allowance check for non-self transfers",
"approve() function incorrectly writes to storage_map_c instead of storage_map_d (allowance mapping), causing it to modify balances instead of allowances",
"Both balanceOf() and allowance() incorrectly read from storage_map_d when they should read from different mappings (balanceOf should use storage_map_c based on other function usage)",
"withdraw() uses incorrect transfer method syntax - should be a low-level call, not .transfer() which returns void, not (bool, bytes)",
"transfer() and transferFrom() have inverted logic conditions (using '!storage_map_c[var_a] < arg1' instead of 'storage_map_c[var_a] >= arg1'), though these may be functionally equivalent",
"transferFrom() has incorrect allowance check logic - the condition 'require(address(arg0) == (address(msg.sender)))' followed by uint max check doesn't properly represent the original 'if (src != msg.sender && allowance != uint(-1))' pattern",
"Multiple redundant and unreachable code blocks after return statements in both transfer functions indicate severe control flow decompilation failure"
]
}
Contributor
Benchmark for 592674dClick to view benchmark
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Solution