fix: add cycle detection to EIP-712 typed data encoding to prevent stack overflow DoS - #210
Merged
Merged
Conversation
…ack overflow DoS Cyclic type definitions (e.g. A→B→A) caused infinite recursion in EncodeType/encodeValue, leading to an unrecoverable stack overflow. This was reachable via unauthenticated endpoints with a ~200-byte payload. Add ValidateTypeGraph() with DFS cycle detection, called in both UnmarshalJSON and Encode to reject cycles before any recursion begins. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9608992df0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+29
to
+30
| for typeName := range t { | ||
| if err := t.walkTypeGraph(typeName, make(map[string]bool)); err != nil { |
There was a problem hiding this comment.
Because this loops over every entry in Types, Encode and JSON unmarshalling now reject cycles that are never reachable from PrimaryType or EIP712Domain. For example, a signable payload with primary type Mail plus an unused A -> B -> A helper definition now fails before encoding, even though the existing encoder only traverses the domain and primary-type dependency graph and the unused types cannot cause recursion. Limit the DFS roots to the types actually encoded so irrelevant definitions do not break otherwise valid typed data.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Unreachable types are irrelevant, I don't think we need to make an exception for them. Open to changing this though, if needed.
pkieltyka
pushed a commit
that referenced
this pull request
Aug 17, 2026
…formed types (#214) * fix: harden EIP-712 typed data encoding against amplification and malformed types Follow-up to #210. Retesting surfaced a value-driven amplification path that the schema-only cycle check does not cover, plus a panic reachable from any caller that decodes untrusted typed data. - Memoize EncodeType/TypeHash per Encode call. HashStruct previously recomputed a type's hash for every array element, so a message holding an array of custom structs multiplied schema-processing cost by the element count. The cache is shared across the whole call tree, domain and message alike, so type-dependent work happens at most once per distinct type. - Add functional Options bounding both the schema and the message: WithMaxTypes, WithMaxFieldsPerType, WithMaxWalkVisits, WithMaxArrayElements, WithMaxRecursionDepth and WithMaxTotalValues. The zero value means unlimited, so existing callers are unaffected. Value-driven checks run before allocating or recursing, so an oversized array is rejected up front. - Fix a panic in typedDataDecodePrimitiveValue. ABIUnmarshalStringValuesAny returns fewer values than requested, with a nil error, for a type token it does not recognize; the caller then indexed out[0] and panicked on input as simple as {"type": ""} or {"type": "foobar"}. - Make ValidateTypeGraph's walk an explicit-stack DFS and cap nesting at maxTypeGraphDepth. The encoders below it still recurse one frame per level, and a long enough type chain overflowed the goroutine stack fatally. The ceiling is measured from each type's longest downward path rather than the live DFS stack, which memoization can cut short depending on map iteration order. - Reject field types no encoder can handle (unknown names, uint0/uint7/uint2560, bytes0/bytes33, bare uint/int, malformed array suffixes) instead of letting them fail deep inside the encoders. Note: schema validation is stricter than before. A schema declaring a type with an invalid field type previously decoded and only failed if that type was actually encoded; it is now rejected at decode. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(ethcoder): trim typed-data comments to non-obvious rationale Drop comments that restated what the code already shows, and compress the rest to the reason a reader cannot infer: why the depth ceiling is unconditional, why depth is tracked separately from the live DFS stack, why bare uint/int are rejected, and why the budget is checked before allocating. Removes the duplicated memoization rationale that appeared on encodeTypeCached, hashStruct and Encode, keeping it only on encodeTypeCached. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ethcoder): review follow-ups on typed-data hardening - Rename Option to TypedDataOption: ethcoder already exports Options for merkle proofs, and the two were one letter apart. - Guard encodeTypeCached against cycles via an in-progress sentinel, so EncodeType/TypeHash/HashStruct called directly (skipping ValidateTypeGraph) fail cleanly instead of overflowing the stack. - Require canonical width spellings in isPrimitiveType: uint0256, bytes01 etc. matched the regex but aren't valid EIP-712 types. - Change budget option fields from int to uint so a negative value is a compile error instead of silently meaning unlimited. --------- 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
2 participants
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.
Summary
A→B→A) cause infinite recursion inEncodeType/encodeValue, leading to an unrecoverable stack overflow (recover()cannot catch it — the process dies)ValidateTypeGraph()using DFS with grey-node cycle detection, called at two entry points:UnmarshalJSON— rejects malicious payloads before any recursive traversalEncode— catches programmatically constructed cyclic typesB[]) are handled by stripping the suffix before graph traversalTest plan