-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add enum ranges to echidna printer output #2908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hello maintainers! This adds enum range information to the echidna printer as requested in #1203. The implementation follows the format suggested in the issue. Could you please approve the CI workflows to run? Thank you! |
6f2452a to
aa1dc0c
Compare
dguido
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review for PR #2908: Add enum ranges to echidna printer output
Files Reviewed
slither/printers/guidance/echidna.py(+24 lines)
Summary
This PR adds enum range information to the Echidna printer output to help Echidna generate valid calldata for enum parameters. The implementation is clean and follows existing patterns in the codebase.
Critical Issues (90-100)
None found.
Important Issues (80-89)
1. Uses contract.enums instead of contract.enums_declared - may cause duplicate processing (Confidence: 85)
File: slither/printers/guidance/echidna.py, line 68
Code:
for enum in contract.enums:Issue: The CLAUDE.md guidelines state: "Use _declared suffix - functions_declared, state_variables_declared avoid processing inherited items multiple times." Using contract.enums includes inherited enums, which means if contract B inherits from A, and A has an enum Status, it will appear in both A's and B's output. This creates redundant data in the JSON.
The codebase has contract.enums_declared (see slither/core/declarations/contract.py:299) specifically for this purpose, and other detectors use it (e.g., naming_convention.py:192).
Suggested fix:
for enum in contract.enums_declared:2. Missing handling of top-level enums (Confidence: 82)
File: slither/printers/guidance/echidna.py
Issue: Solidity 0.8+ supports file-level (top-level) enum declarations that are not associated with any contract. These are accessible via compilation_unit.enums_top_level. The current implementation only processes contract-level enums.
If Echidna needs to handle functions that use top-level enums as parameters, this information would be missing from the output.
Suggested fix: Consider adding a separate key for top-level enums:
def _extract_enum_ranges(contracts: list[Contract]) -> dict[str, dict[str, int]]:
# ... existing implementation using enums_declared ...
# In output():
# Add top-level enums handling
top_level_enums = {}
for unit in self.slither.compilation_units:
for enum in unit.enums_top_level:
top_level_enums[enum.canonical_name] = enum.max + 1
d = {
# ... existing fields ...
"enum_ranges": enum_ranges,
"enum_ranges_top_level": top_level_enums if top_level_enums else {},
}Alternatively, if this is intentionally out of scope, a brief comment explaining this limitation would be helpful.
Minor Observations (Not blocking)
-
Import
Enumis unused after this change - TheEnumclass is imported on line 7 but not used by the new function. It's used elsewhere in the file (_extract_constants_from_irs), so this is fine, just noting for completeness. -
Empty enums edge case - If a contract declares an enum with zero values (which would be a Solidity compile error anyway),
enum.max + 1would correctly return 0. This is handled correctly. -
Tests - No tests were added for this feature. While not strictly required given the straightforward nature of the change, a simple unit test would help prevent regressions.
What's Good
- Follows the established pattern of similar extraction functions (
_extract_payable,_extract_constant_functions) - Clean, readable implementation with good docstring
- Output format matches the example in the linked issue #1203
- Uses
enum.max + 1which correctly leverages the existingEnumclass API
Recommendation
Address the enums_declared issue before merging. The top-level enums issue is less urgent but worth considering for completeness.
aa1dc0c to
7ca52ce
Compare
Export enum definitions and their valid ranges to help Echidna
generate valid calldata for enum parameters. Since enums are
converted to uint256 in the ABI, Echidna needs to know the valid
range to avoid producing invalid calldata.
Example output:
```json
{
"enum_ranges": {
"MyContract": {
"Status": 4
}
}
}
```
This indicates Status enum has 4 valid values (0, 1, 2, 3).
Fixes crytic#1203
- Use contract.enums_declared instead of contract.enums to avoid processing inherited enums multiple times - Add enum_ranges_top_level field to JSON output for file-level enums accessible via compilation_unit.enums_top_level Addresses review feedback from @dguido
08b5529 to
035bbce
Compare
Auto code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Summary
Fixes #1203
Adds enum range information to the echidna printer output to help Echidna generate valid calldata for enum parameters. Since enums are converted to
uint256in the ABI, Echidna needs to know the valid range to avoid producing invalid calldata.Changes
_extract_enum_ranges()function that extracts enum definitions from contractsenum_rangesfield to the echidna printer JSON outputExample Output
{ "enum_ranges": { "MyContract": { "Status": 4, "Priority": 3 } } }This indicates:
Statusenum has 4 valid values (0, 1, 2, 3)Priorityenum has 3 valid values (0, 1, 2)Test Plan
🤖 Generated with Claude Code