fix: harden EIP-712 typed data encoding against amplification and malformed types - #214
Conversation
…formed 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>
![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: a6a0553036
ℹ️ 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".
| // WithMaxWalkVisits, WithMaxArrayElements, WithMaxRecursionDepth, and | ||
| // WithMaxTotalValues. With no opts, behavior is unbounded, matching prior | ||
| // versions of this function. | ||
| func (t *TypedData) Encode(opts ...Option) ([]byte, []byte, error) { |
There was a problem hiding this comment.
Changing this public method from Encode() ([]byte, []byte, error) to Encode(...Option) ([]byte, []byte, error) keeps direct td.Encode() calls working, but it changes the method set: downstream code that stores *TypedData behind an interface requiring Encode(), or assigns td.Encode to a func(), no longer compiles. Since this package is consumed as a library and the option-bearing path can be exposed with a separate helper/method, keeping the original signature avoids a source-compatibility break for existing callers.
Useful? React with 👍 / 👎.
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>
- 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b280d0a9fc
ℹ️ 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".
| func (t *TypedData) EncodeDigest() ([]byte, error) { | ||
| digest, _, err := t.Encode() | ||
| // EncodeDigest returns the digest of the typed data message. See Encode for opts. | ||
| func (t *TypedData) EncodeDigest(opts ...TypedDataOption) ([]byte, error) { |
There was a problem hiding this comment.
Changing this public method from EncodeDigest() ([]byte, error) to EncodeDigest(...TypedDataOption) ([]byte, error) breaks downstream interfaces requiring the original method and assignments of td.EncodeDigest to a func() ([]byte, error), even though direct no-argument calls still compile. Keep the original signature and expose the option-bearing path through a separate method to avoid a source-compatibility regression.
Useful? React with 👍 / 👎.
2 participants
Follow-up to #210. End-to-end retesting of that fix surfaced a residual value-driven amplification path the schema-only cycle check cannot cover, and auditing the surrounding code turned up a reachable panic and a latent stack overflow.
What's here
1. Memoize
EncodeType/TypeHashperEncodecallHashStructrecomputed a type'sTypeHash— and thereforeEncodeTypeover its whole dependency DAG — for every array element. A message with an array of custom structs multiplied schema-processing cost by the element count.A
typeInfocache is now created once perEncodeand shared across the entire call tree (domain and message alike), so type-dependent work happens at most once per distinct type. This also fixes a second-order case unrelated to arrays: a diamond-shaped DAG previously re-expanded shared sub-types multiple times within a singleEncodeTypecall.Measured on a depth-6 diamond struct in an array — scaling is now flat per element (~57µs), i.e. linear in encoded values:
Public signatures of
EncodeType,TypeHashandHashStructare unchanged; they delegate to the cached core with a fresh cache.encodeTypeCachedalso detects a cycle itself (via an in-progress sentinel in the cache), so calling these three directly — bypassingValidateTypeGraph— fails cleanly on a cyclic graph instead of overflowing the stack.2. Functional options for schema and value budgets
WithMaxTypes,WithMaxFieldsPerType,WithMaxWalkVisits,WithMaxArrayElements,WithMaxRecursionDepth,WithMaxTotalValues— accepted byValidateTypeGraph,EncodeandEncodeDigestvia aTypedDataOption. Every field isuint, so a negative value is a compile error rather than a silent "unlimited."The zero value of every option means unlimited, so existing callers are unaffected. Value-driven checks run before allocating or recursing, so an oversized array is rejected up front rather than after the work is done. This consolidates limits that downstream services were otherwise reimplementing on top of ethkit.
3. Fix a reachable panic in
typedDataDecodePrimitiveValueABIUnmarshalStringValuesAnyreturns fewer values than requested — with a nil error — for a type token matching none of its branches, silently violating its positional contract. The caller then indexedout[0]:Triggered by input as simple as
{"name":"x","type":""}or"type":"foobar","tuple","byte","String","address ". Any service decoding untrusted typed data without a panic barrier crashes on it.4. Iterative type-graph DFS + unconditional depth ceiling
ValidateTypeGraph's walk was recursive, one frame per type. An acyclic linear type chain therefore still overflowed the goroutine stack — fatally, not recoverably — at roughly 2M types. Cycle detection did not help; the graph is a valid DAG.The walk is now an explicit-stack DFS, and type nesting is capped at
maxTypeGraphDepth = 1024. The cap is unconditional rather than an option becauseUnmarshalJSONvalidates without options, and it protects the encoders below it (encodeTypeCached,hashStruct,encodeValue), which still recurse one frame per level.The ceiling is measured from each type's longest downward path, not the live DFS stack: memoization can cut that stack short depending on map iteration order, which would let an over-deep chain through non-deterministically. Verified exact and stable at the 1024/1025 boundary across repeated runs.
5. Reject field types no encoder can handle
ValidateTypeGraphnow validates every field type up front instead of letting bad ones fail deep inside the encoders: unknown names,uint0/uint7/uint2560,bytes0/bytes33, non-canonical width spellings (uint0256,bytes01), bareuint/int(EIP-712 requires the canonicaluint256/int256, and the packer cannot size them), and malformed array suffixes such asuint256[,uint256[a],[]uint256.Schema validation is stricter than before. A schema declaring a type with an invalid field type previously decoded fine and only failed if that type was actually encoded — it is now rejected at decode.
The existing corpus is unaffected: all real-world payloads in the test suite (including the Seaport cases) pass unchanged, and the typed-data tests only use
address,bytes,bytes32,string,uint8/128/256, arrays and custom types.Testing
ethcodersuite passes, including under-race.TestTypedDataMemoization,TestTypedDataBudgetLimits,TestTypedDataInvalidPrimitiveType,TestTypedDataTypeGraphHardening,TestTypedDataDirectCycleDetection.bytesN, null values, nested arrays, scalar/array/struct mismatches) — all return errors, none panic.Known issues left alone (pre-existing, out of scope)
uintNfield encodes as its absolute value with no error, because the 128/256 path builds abig.IntviaSetStringwithout a sign or range check (the 8/16/32/64 paths do check, viaParseUint).chainIdsuch as"0xzz"silently parses to0—UnmarshalJSONignoresSetString's ok bool.Happy to fold either of those into this PR or file them separately.
🤖 Generated with Claude Code