fix: convert every struct field, not just the first of a repeated type - #3065
Open
ataberk-xyz wants to merge 1 commit into
Open
fix: convert every struct field, not just the first of a repeated type#3065ataberk-xyz wants to merge 1 commit into
ataberk-xyz wants to merge 1 commit into
Conversation
`convert_type_for_solidity_signature` guards against recursive struct
definitions with a `seen` set, but that set was mutated in place and shared
across the whole traversal rather than tracking the current descent. A type
appearing twice as a *sibling* -- two fields of the same user defined value
type, enum, or contract -- was therefore treated as a recursive back-edge, and
every occurrence after the first was returned unconverted:
struct Pair { Amount a; Amount b; } // type Amount is uint256
actual f((uint256,Amount))
expected f((uint256,uint256))
Since `get_function_from_signature` matches strictly, affected functions became
unresolvable by signature, silently and with no error.
Track ancestors instead: rebind `seen` to a new set per descent so siblings do
not shadow each other. The array branch of the string conversion re-enters the
type conversion, so it now passes the element type down as an ancestor -- that
is what keeps a struct reaching itself through an array terminating.
Fixes crytic#3064
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Fixes #3064.
The bug
convert_type_for_solidity_signatureuses aseenset to stop recursive struct definitions fromlooping forever. That set was mutated in place and shared across the entire traversal, including
across sibling fields of the same struct — so a type used by two fields was treated as a recursive
back-edge on its second appearance and returned unconverted.
It is not specific to user-defined value types — any sibling type that needs conversion collides:
Amount a; Amount b;(UDVT)fAlias((uint256,Amount))fAlias((uint256,uint256))Flag a; Flag b;(enum)fEnum((uint8,Flag))fEnum((uint8,uint8))IThing a; IThing b;(interface)fContract((address,IThing))fContract((address,address))Two
uint256fields were never affected, which is why this stayed hidden: returning a typeunconverted is harmless exactly when no conversion was needed.
Because
get_function_from_signaturematches strictly, affected functions were unresolvable bysignature — silently, with no exception or warning.
The fix
Treat
seenas the ancestors of the current descent rather than every type visited anywhere:rebind it to a new set on entry instead of mutating the caller's. Siblings then each see only their
shared ancestors, not each other.
One subtlety worth flagging for review: the shared mutable set was also load-bearing for
termination.
_convert_type_for_solidity_signature_to_stringre-entersconvert_type_for_solidity_signaturewhen it unwraps anArrayType, and relied on entries leftbehind by the first phase to stop
struct St { St[] a; uint b; }from recursing forever. Scopingseenwithout accounting for that reintroduces infinite recursion — I hit it while testing. So thearray branch now passes the element type down as an ancestor, which preserves the guard across the
phase boundary.
The existing
test_function_id_rec_structure(the recursive-struct case) still passes. Its renderingdoes change: that struct now expands one level further before the cycle is cut, since the outer
Stis no longer in
seenwhen the array branch is first reached. The existing test asserts only that asignature is produced, and the value is not part of any ABI-meaningful signature — recursive structs
cannot appear in public/external functions, which is the only reason they are permitted at all.
Tests
Added
test_solidity_signature_repeated_struct_field_typescovering all three conversion kinds(UDVT, enum, contract). It fails on
master:tests/unit/utils/andtests/unit/core/test_function_declaration.pypass — 36 passed, with theone unrelated
test_vyper_functionsfailure reproducing identically on unpatchedmasterin myenvironment (local vyper version).
ruff format --checkandruff checkare clean.