From a6a0553036efb47d1109dd555e792a5d6f7a38ba Mon Sep 17 00:00:00 2001 From: Patryk Kalinowski Date: Fri, 14 Aug 2026 15:35:55 +0200 Subject: [PATCH 1/3] 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 --- ethcoder/typed_data.go | 324 ++++++++++++++++++++++++++++----- ethcoder/typed_data_json.go | 6 + ethcoder/typed_data_options.go | 76 ++++++++ ethcoder/typed_data_test.go | 285 +++++++++++++++++++++++++++++ 4 files changed, 648 insertions(+), 43 deletions(-) create mode 100644 ethcoder/typed_data_options.go diff --git a/ethcoder/typed_data.go b/ethcoder/typed_data.go index 8efa9558..8eee9f79 100644 --- a/ethcoder/typed_data.go +++ b/ethcoder/typed_data.go @@ -5,6 +5,7 @@ import ( "fmt" "math/big" "sort" + "strconv" "strings" "github.com/0xsequence/ethkit/go-ethereum/common" @@ -22,42 +23,235 @@ type TypedData struct { type TypedDataTypes map[string][]TypedDataArgument -// ValidateTypeGraph checks the type graph for cycles. A cycle would cause -// infinite recursion in EncodeType/encodeValue, leading to an unrecoverable -// stack overflow. This must be called before any recursive type traversal. -func (t TypedDataTypes) ValidateTypeGraph() error { - for typeName := range t { - if err := t.walkTypeGraph(typeName, make(map[string]bool)); err != nil { - return err +// maxTypeGraphDepth bounds how deeply types may nest. The encoders below this +// validation (encodeTypeCached, hashStruct, encodeValue) all recurse one frame +// per level, so this ceiling is what keeps a long chain of types from +// exhausting the goroutine stack. It is unconditional rather than an Option +// because UnmarshalJSON validates without any, and no real schema comes close. +const maxTypeGraphDepth = 1024 + +// ValidateTypeGraph checks the type graph for cycles, unknown field types, and +// excessive nesting depth. A cycle or a deep enough chain would otherwise cause +// runaway recursion in EncodeType/encodeValue and an unrecoverable stack +// overflow, and an unknown field type would fail deep inside the encoders. This +// must be called before any recursive type traversal. +// +// With no options, size is otherwise unbounded (matching prior behavior). +// WithMaxTypes, WithMaxFieldsPerType, and WithMaxWalkVisits additionally bound +// the schema's size and the cost of this traversal itself, which is +// combinatorial for diamond-shaped (but acyclic) type graphs. +func (t TypedDataTypes) ValidateTypeGraph(opts ...Option) error { + o := resolveOptions(opts) + + if o.maxTypes > 0 { + typeCount := len(t) + if _, ok := t["EIP712Domain"]; !ok { + typeCount++ + } + if typeCount > o.maxTypes { + return fmt.Errorf("too many types: %d exceeds limit of %d", typeCount, o.maxTypes) + } + } + if o.maxFieldsPerType > 0 { + for name, fields := range t { + if len(fields) > o.maxFieldsPerType { + return fmt.Errorf("type %q has %d fields, exceeds limit of %d", name, len(fields), o.maxFieldsPerType) + } + } + } + + for name, fields := range t { + for _, field := range fields { + if err := t.validateFieldType(field.Type); err != nil { + return fmt.Errorf("type %q field %q: %w", name, field.Name, err) + } + } + } + + const ( + visiting = 1 + done = 2 + ) + state := make(map[string]int, len(t)) + visits := make(map[string]int, len(t)) + + // The walk is an explicit-stack DFS rather than recursion so that its own + // depth is heap-bound; maxTypeGraphDepth still caps it, to protect the + // recursive encoders that run after this passes. + // + // depth is the longest downward path from a type, tracked separately from + // visits: the encoders start from primaryType with a cold cache, so the + // live DFS stack (which memoization can cut short depending on map + // iteration order) is not on its own a sound bound on their recursion. + type frame struct { + name string + field int + total int + depth int + } + depths := make(map[string]int, len(t)) + + tooComplex := func() error { + return fmt.Errorf("type graph too complex: exceeds %d traversal steps", o.maxWalkVisits) + } + + sum := 0 + for root := range t { + if state[root] == done { + sum += visits[root] + if o.maxWalkVisits > 0 && sum > o.maxWalkVisits { + return tooComplex() + } + continue + } + + state[root] = visiting + stack := []frame{{name: root, total: 1}} + + for len(stack) > 0 { + top := &stack[len(stack)-1] + + if top.field < len(t[top.name]) { + base := t[top.name][top.field].Type + top.field++ + if i := strings.Index(base, "["); i > 0 { + base = base[:i] + } + if _, ok := t[base]; !ok { + continue + } + switch state[base] { + case visiting: + return fmt.Errorf("cycle detected in type graph at %q", base) + case done: + top.total += visits[base] + if depths[base] > top.depth { + top.depth = depths[base] + } + if o.maxWalkVisits > 0 && top.total > o.maxWalkVisits { + return tooComplex() + } + continue + } + if len(stack) >= maxTypeGraphDepth { + return fmt.Errorf("type graph too deep: exceeds %d levels", maxTypeGraphDepth) + } + state[base] = visiting + stack = append(stack, frame{name: base, total: 1}) + continue + } + + depth := top.depth + 1 + if depth > maxTypeGraphDepth { + return fmt.Errorf("type graph too deep: exceeds %d levels", maxTypeGraphDepth) + } + state[top.name] = done + visits[top.name] = top.total + depths[top.name] = depth + total := top.total + stack = stack[:len(stack)-1] + + if len(stack) > 0 { + parent := &stack[len(stack)-1] + parent.total += total + if depth > parent.depth { + parent.depth = depth + } + if o.maxWalkVisits > 0 && parent.total > o.maxWalkVisits { + return tooComplex() + } + continue + } + sum += total + if o.maxWalkVisits > 0 && sum > o.maxWalkVisits { + return tooComplex() + } } } return nil } -func (t TypedDataTypes) walkTypeGraph(current string, visiting map[string]bool) error { - if visiting[current] { - return fmt.Errorf("cycle detected in type graph at %q", current) +// validateFieldType rejects any field type that is neither a type defined in +// this schema nor a well-formed EIP-712 primitive. Without this an unknown +// token reaches the primitive decoder, which has no branch for it. +func (t TypedDataTypes) validateFieldType(typ string) error { + base := typ + if i := strings.Index(base, "["); i > 0 { + if !validArraySuffix(typ[i:]) { + return fmt.Errorf("invalid array suffix in type %q", typ) + } + base = base[:i] } - visiting[current] = true - defer delete(visiting, current) - for _, field := range t[current] { - baseType := field.Type - if i := strings.Index(baseType, "["); i > 0 { - baseType = baseType[:i] + if _, ok := t[base]; ok { + return nil + } + if !isPrimitiveType(base) { + return fmt.Errorf("unknown type %q", typ) + } + return nil +} + +// validArraySuffix reports whether s is a run of "[]" / "[N]" groups. +func validArraySuffix(s string) bool { + for len(s) > 0 { + if s[0] != '[' { + return false } - if _, ok := t[baseType]; ok { - if err := t.walkTypeGraph(baseType, visiting); err != nil { - return err + end := strings.IndexByte(s, ']') + if end < 0 { + return false + } + for _, c := range s[1:end] { + if c < '0' || c > '9' { + return false } } + s = s[end+1:] } - return nil + return true } -func (t TypedDataTypes) EncodeType(primaryType string) (string, error) { +// isPrimitiveType reports whether typ is an EIP-712 atomic or dynamic type. +// Bare "uint"/"int" are rejected: EIP-712 requires the canonical uint256/int256 +// spelling, and the packer cannot size them. +func isPrimitiveType(typ string) bool { + switch typ { + case "address", "bool", "string", "bytes": + return true + } + if match := regexArgBytes.FindStringSubmatch(typ); len(match) > 0 { + size, err := strconv.Atoi(match[1]) + return err == nil && size >= 1 && size <= 32 + } + if match := regexArgNumber.FindStringSubmatch(typ); len(match) > 0 { + if match[2] == "" { + return false + } + size, err := strconv.Atoi(match[2]) + return err == nil && size >= 8 && size <= 256 && size%8 == 0 + } + return false +} + +// typeInfo is the memoized result of encoding one type's EIP-712 type string +// and its Keccak256 hash, keyed by type name for the lifetime of one cache. +type typeInfo struct { + encodeType string + hash []byte +} + +// encodeTypeCached is EncodeType's recursive core, sharing cache across the +// whole call tree so a type reached through multiple paths (a diamond in the +// dependency DAG, or the same struct type appearing in many array elements) +// is only encoded once. +func (t TypedDataTypes) encodeTypeCached(cache map[string]*typeInfo, primaryType string) (*typeInfo, error) { + if info, ok := cache[primaryType]; ok { + return info, nil + } + args, ok := t[primaryType] if !ok { - return "", fmt.Errorf("%s type is not defined", primaryType) + return nil, fmt.Errorf("%s type is not defined", primaryType) } subTypes := []string{} @@ -91,14 +285,24 @@ func (t TypedDataTypes) EncodeType(primaryType string) (string, error) { sort.Strings(subTypes) for _, subType := range subTypes { - subEncodeType, err := t.EncodeType(subType) + subInfo, err := t.encodeTypeCached(cache, subType) if err != nil { - return "", err + return nil, err } - s += subEncodeType + s += subInfo.encodeType } - return s, nil + info := &typeInfo{encodeType: s, hash: Keccak256([]byte(s))} + cache[primaryType] = info + return info, nil +} + +func (t TypedDataTypes) EncodeType(primaryType string) (string, error) { + info, err := t.encodeTypeCached(make(map[string]*typeInfo), primaryType) + if err != nil { + return "", err + } + return info.encodeType, nil } func (t TypedDataTypes) Map() map[string]map[string]string { @@ -114,11 +318,11 @@ func (t TypedDataTypes) Map() map[string]map[string]string { } func (t TypedDataTypes) TypeHash(primaryType string) ([]byte, error) { - encodeType, err := t.EncodeType(primaryType) + info, err := t.encodeTypeCached(make(map[string]*typeInfo), primaryType) if err != nil { return nil, err } - return Keccak256([]byte(encodeType)), nil + return info.hash, nil } type TypedDataArgument struct { @@ -155,22 +359,34 @@ func (t TypedDataDomain) Map() map[string]interface{} { } func (t *TypedData) HashStruct(primaryType string, data map[string]interface{}) ([]byte, error) { - typeHash, err := t.Types.TypeHash(primaryType) + return t.hashStruct(make(map[string]*typeInfo), &budgetState{}, 0, primaryType, data) +} + +// hashStruct is HashStruct's recursive core. cache and budget are shared +// across the whole call tree of one Encode/EncodeDigest call (domain and +// message alike), so a struct type reached through many array elements has +// its type-hash computed once, and value-driven traversal cost (array +// length, nesting depth) is checked against a single aggregate budget. +func (t *TypedData) hashStruct(cache map[string]*typeInfo, budget *budgetState, depth int, primaryType string, data map[string]interface{}) ([]byte, error) { + if err := budget.checkDepth(depth); err != nil { + return nil, err + } + info, err := t.Types.encodeTypeCached(cache, primaryType) if err != nil { return nil, err } - encodedData, err := t.encodeData(primaryType, data) + encodedData, err := t.encodeData(cache, budget, depth, primaryType, data) if err != nil { return nil, err } - v, err := SolidityPack([]string{"bytes32", "bytes"}, []interface{}{BytesToBytes32(typeHash), encodedData}) + v, err := SolidityPack([]string{"bytes32", "bytes"}, []interface{}{BytesToBytes32(info.hash), encodedData}) if err != nil { return nil, err } return Keccak256(v), nil } -func (t *TypedData) encodeData(primaryType string, data map[string]interface{}) ([]byte, error) { +func (t *TypedData) encodeData(cache map[string]*typeInfo, budget *budgetState, depth int, primaryType string, data map[string]interface{}) ([]byte, error) { args, ok := t.Types[primaryType] if !ok { return nil, fmt.Errorf("%s type is unknown", primaryType) @@ -188,7 +404,7 @@ func (t *TypedData) encodeData(primaryType string, data map[string]interface{}) return nil, fmt.Errorf("data value missing for type %s with argument name %s", primaryType, arg.Name) } - encValue, err := t.encodeValue(arg.Type, dataValue) + encValue, err := t.encodeValue(cache, budget, depth, arg.Type, dataValue) if err != nil { return nil, fmt.Errorf("failed to encode %s: %w", arg.Name, err) } @@ -200,7 +416,7 @@ func (t *TypedData) encodeData(primaryType string, data map[string]interface{}) } // encodeValue handles the recursive encoding of values according to their types -func (t *TypedData) encodeValue(typ string, value interface{}) ([]byte, error) { +func (t *TypedData) encodeValue(cache map[string]*typeInfo, budget *budgetState, depth int, typ string, value interface{}) ([]byte, error) { // Handle arrays if strings.Index(typ, "[") > 0 { baseType := typ[:strings.Index(typ, "[")] @@ -209,9 +425,18 @@ func (t *TypedData) encodeValue(typ string, value interface{}) ([]byte, error) { return nil, fmt.Errorf("expected array for type %s", typ) } + // Budget checks happen before allocating encodedValues or recursing + // into any element, so an oversized array is rejected up front. + if err := budget.checkArray(len(values)); err != nil { + return nil, err + } + if err := budget.checkDepth(depth + 1); err != nil { + return nil, err + } + encodedValues := make([][]byte, len(values)) for i, val := range values { - encoded, err := t.encodeValue(baseType, val) + encoded, err := t.encodeValue(cache, budget, depth+1, baseType, val) if err != nil { return nil, fmt.Errorf("failed to encode array element %d: %w", i, err) } @@ -242,7 +467,7 @@ func (t *TypedData) encodeValue(typ string, value interface{}) ([]byte, error) { if !ok { return nil, fmt.Errorf("invalid value for custom type %s", typ) } - encoded, err := t.HashStruct(typ, mapVal) + encoded, err := t.hashStruct(cache, budget, depth+1, typ, mapVal) if err != nil { return nil, fmt.Errorf("failed to encode custom type %s: %w", typ, err) } @@ -262,8 +487,14 @@ func (t *TypedData) encodeValue(typ string, value interface{}) ([]byte, error) { // NOTE: // * the digest is the hash of the fully encoded EIP712 message // * the encoded message is the fully encoded EIP712 message (0x1901 + domain + hashStruct(message)) -func (t *TypedData) Encode() ([]byte, []byte, error) { - if err := t.Types.ValidateTypeGraph(); err != nil { +// +// opts optionally bound both the schema (ValidateTypeGraph) and the message +// data being encoded — see WithMaxTypes, WithMaxFieldsPerType, +// 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) { + if err := t.Types.ValidateTypeGraph(opts...); err != nil { return nil, nil, err } @@ -273,14 +504,20 @@ func (t *TypedData) Encode() ([]byte, []byte, error) { return nil, nil, err } + // cache and budget are shared across the domain and message hash-struct + // calls below, so type-hash work and the value-traversal budget are + // scoped to this single Encode call, not per hash-struct invocation. + cache := make(map[string]*typeInfo) + budget := &budgetState{opts: resolveOptions(opts)} + // Prepare hash struct for the domain - domainHash, err := t.HashStruct("EIP712Domain", t.Domain.Map()) + domainHash, err := t.hashStruct(cache, budget, 0, "EIP712Domain", t.Domain.Map()) if err != nil { return nil, nil, err } // Prepare hash struct for the message object - messageHash, err := t.HashStruct(t.PrimaryType, t.Message) + messageHash, err := t.hashStruct(cache, budget, 0, t.PrimaryType, t.Message) if err != nil { return nil, nil, err } @@ -295,9 +532,10 @@ func (t *TypedData) Encode() ([]byte, []byte, error) { return digest, encodedMessage, nil } -// EncodeDigest returns the digest of the typed data message. -func (t *TypedData) EncodeDigest() ([]byte, error) { - digest, _, err := t.Encode() +// EncodeDigest returns the digest of the typed data message. See Encode for +// the optional resource-limit opts. +func (t *TypedData) EncodeDigest(opts ...Option) ([]byte, error) { + digest, _, err := t.Encode(opts...) if err != nil { return nil, err } diff --git a/ethcoder/typed_data_json.go b/ethcoder/typed_data_json.go index c9ade308..8dfb945e 100644 --- a/ethcoder/typed_data_json.go +++ b/ethcoder/typed_data_json.go @@ -341,5 +341,11 @@ func typedDataDecodePrimitiveValue(typ string, value interface{}) (interface{}, if err != nil { return nil, fmt.Errorf("typedDataDecodePrimitiveValue: %w", err) } + // ABIUnmarshalStringValuesAny silently returns fewer values than requested + // for an unrecognized type token (e.g. "", "foobar"), so guard the index + // rather than panic with out-of-range on attacker-controlled type strings. + if len(out) != 1 { + return nil, fmt.Errorf("typedDataDecodePrimitiveValue: unsupported type %q", typ) + } return out[0], nil } diff --git a/ethcoder/typed_data_options.go b/ethcoder/typed_data_options.go new file mode 100644 index 00000000..8fbf9668 --- /dev/null +++ b/ethcoder/typed_data_options.go @@ -0,0 +1,76 @@ +package ethcoder + +import "fmt" + +// Option configures optional resource limits for EIP-712 typed-data +// processing (ValidateTypeGraph, Encode, EncodeDigest). The zero value of +// every field means "unlimited", matching the unbounded behavior these +// functions have always had when called with no options. +type Option func(*options) + +type options struct { + maxTypes int + maxFieldsPerType int + maxWalkVisits int + maxArrayElements int + maxRecursionDepth int + maxTotalValues int +} + +func resolveOptions(opts []Option) options { + var o options + for _, opt := range opts { + opt(&o) + } + return o +} + +// WithMaxTypes caps the number of distinct types a schema may define, +// including the implicit EIP712Domain type when it isn't declared explicitly. +func WithMaxTypes(n int) Option { return func(o *options) { o.maxTypes = n } } + +// WithMaxFieldsPerType caps the number of fields any single type may declare. +func WithMaxFieldsPerType(n int) Option { return func(o *options) { o.maxFieldsPerType = n } } + +// WithMaxWalkVisits caps the total number of type-graph nodes ValidateTypeGraph +// visits, bounding its own traversal cost against diamond-shaped (but acyclic) +// type graphs where naive recursion would otherwise blow up combinatorially. +func WithMaxWalkVisits(n int) Option { return func(o *options) { o.maxWalkVisits = n } } + +// WithMaxArrayElements caps the number of elements in any single array value +// within the message being encoded. +func WithMaxArrayElements(n int) Option { return func(o *options) { o.maxArrayElements = n } } + +// WithMaxRecursionDepth caps how deeply nested arrays and structs in the +// message may be encoded. +func WithMaxRecursionDepth(n int) Option { return func(o *options) { o.maxRecursionDepth = n } } + +// WithMaxTotalValues caps the aggregate number of array elements encoded +// across the entire message for a single Encode/EncodeDigest call. +func WithMaxTotalValues(n int) Option { return func(o *options) { o.maxTotalValues = n } } + +// budgetState tracks the value-driven traversal budget for one Encode call. +// It is distinct from the type-hash cache: the cache is schema-derived (safe +// to reuse across domain + message), while this counts actual message data. +type budgetState struct { + opts options + totalValues int +} + +func (b *budgetState) checkArray(n int) error { + if b.opts.maxArrayElements > 0 && n > b.opts.maxArrayElements { + return fmt.Errorf("array has %d elements, exceeds limit of %d", n, b.opts.maxArrayElements) + } + b.totalValues += n + if b.opts.maxTotalValues > 0 && b.totalValues > b.opts.maxTotalValues { + return fmt.Errorf("typed data exceeds aggregate element budget of %d", b.opts.maxTotalValues) + } + return nil +} + +func (b *budgetState) checkDepth(depth int) error { + if b.opts.maxRecursionDepth > 0 && depth > b.opts.maxRecursionDepth { + return fmt.Errorf("typed data recursion depth %d exceeds limit of %d", depth, b.opts.maxRecursionDepth) + } + return nil +} diff --git a/ethcoder/typed_data_test.go b/ethcoder/typed_data_test.go index 8bc56d16..ef9077a2 100644 --- a/ethcoder/typed_data_test.go +++ b/ethcoder/typed_data_test.go @@ -2,6 +2,7 @@ package ethcoder_test import ( "encoding/json" + "fmt" "math/big" "strings" "testing" @@ -838,3 +839,287 @@ func TestTypedDataCycleDetection(t *testing.T) { assert.True(t, strings.Contains(err.Error(), "cycle detected")) }) } + +// diamondArrayTypedData builds a schema where "Item" is reachable from +// "Batch" through two separate fields ("a" and "b" both being "Shared"), and +// the message is an array of many "Item" elements — exercising both the +// diamond-shaped EncodeType cache and the per-array-element HashStruct cache. +func diamondArrayTypedData(itemCount int) *ethcoder.TypedData { + items := make([]interface{}, itemCount) + for i := range items { + items[i] = map[string]interface{}{ + "a": map[string]interface{}{"value": "hot"}, + "b": map[string]interface{}{"value": "hot"}, + } + } + return ðcoder.TypedData{ + Types: ethcoder.TypedDataTypes{ + "EIP712Domain": {}, + "Batch": {{Name: "items", Type: "Item[]"}}, + "Item": {{Name: "a", Type: "Shared"}, {Name: "b", Type: "Shared"}}, + "Shared": {{Name: "value", Type: "string"}}, + }, + PrimaryType: "Batch", + Domain: ethcoder.TypedDataDomain{}, + Message: map[string]interface{}{"items": items}, + } +} + +func TestTypedDataMemoization(t *testing.T) { + t.Run("memoized digest matches a freshly built equivalent message", func(t *testing.T) { + a := diamondArrayTypedData(25) + b := diamondArrayTypedData(25) + + digestA, err := a.EncodeDigest() + require.NoError(t, err) + digestB, err := b.EncodeDigest() + require.NoError(t, err) + require.Equal(t, ethcoder.HexEncode(digestA), ethcoder.HexEncode(digestB)) + }) + + t.Run("digest is unaffected by array length beyond the encoded content", func(t *testing.T) { + one := diamondArrayTypedData(1) + oneAgain := diamondArrayTypedData(1) + + digestOne, err := one.EncodeDigest() + require.NoError(t, err) + digestOneAgain, err := oneAgain.EncodeDigest() + require.NoError(t, err) + require.Equal(t, ethcoder.HexEncode(digestOne), ethcoder.HexEncode(digestOneAgain)) + }) + + t.Run("EncodeType and TypeHash still work standalone with no cache reuse across calls", func(t *testing.T) { + types := diamondArrayTypedData(1).Types + encodeType, err := types.EncodeType("Item") + require.NoError(t, err) + require.Equal(t, "Item(Shared a,Shared b)Shared(string value)", encodeType) + + typeHash, err := types.TypeHash("Item") + require.NoError(t, err) + require.Equal(t, ethcoder.Keccak256([]byte(encodeType)), typeHash) + }) +} + +func TestTypedDataBudgetLimits(t *testing.T) { + t.Run("WithMaxArrayElements rejects an oversized array before allocating", func(t *testing.T) { + typedData := diamondArrayTypedData(10) + _, err := typedData.EncodeDigest(ethcoder.WithMaxArrayElements(5)) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds limit of 5") + }) + + t.Run("WithMaxArrayElements allows an array within budget", func(t *testing.T) { + typedData := diamondArrayTypedData(5) + _, err := typedData.EncodeDigest(ethcoder.WithMaxArrayElements(5)) + require.NoError(t, err) + }) + + t.Run("WithMaxTotalValues bounds aggregate elements across multiple arrays", func(t *testing.T) { + typedData := ðcoder.TypedData{ + Types: ethcoder.TypedDataTypes{ + "EIP712Domain": {}, + "Batch": {{Name: "as", Type: "string[]"}, {Name: "bs", Type: "string[]"}}, + }, + PrimaryType: "Batch", + Domain: ethcoder.TypedDataDomain{}, + Message: map[string]interface{}{ + "as": []interface{}{"1", "2", "3"}, + "bs": []interface{}{"4", "5", "6"}, + }, + } + _, err := typedData.EncodeDigest(ethcoder.WithMaxTotalValues(5)) + require.Error(t, err) + assert.Contains(t, err.Error(), "aggregate element budget") + + _, err = typedData.EncodeDigest(ethcoder.WithMaxTotalValues(6)) + require.NoError(t, err) + }) + + t.Run("WithMaxRecursionDepth rejects deeply nested structs", func(t *testing.T) { + typedData := ðcoder.TypedData{ + Types: ethcoder.TypedDataTypes{ + "EIP712Domain": {}, + "A": {{Name: "b", Type: "B"}}, + "B": {{Name: "c", Type: "C"}}, + "C": {{Name: "value", Type: "string"}}, + }, + PrimaryType: "A", + Domain: ethcoder.TypedDataDomain{}, + Message: map[string]interface{}{ + "b": map[string]interface{}{"c": map[string]interface{}{"value": "x"}}, + }, + } + _, err := typedData.EncodeDigest(ethcoder.WithMaxRecursionDepth(1)) + require.Error(t, err) + assert.Contains(t, err.Error(), "recursion depth") + + _, err = typedData.EncodeDigest(ethcoder.WithMaxRecursionDepth(3)) + require.NoError(t, err) + }) + + t.Run("WithMaxTypes rejects schemas with too many distinct types", func(t *testing.T) { + types := ethcoder.TypedDataTypes{ + "EIP712Domain": {}, + "A": {{Name: "value", Type: "string"}}, + "B": {{Name: "value", Type: "string"}}, + } + err := types.ValidateTypeGraph(ethcoder.WithMaxTypes(2)) + require.Error(t, err) + assert.Contains(t, err.Error(), "too many types") + + require.NoError(t, types.ValidateTypeGraph(ethcoder.WithMaxTypes(3))) + }) + + t.Run("WithMaxFieldsPerType rejects a type with too many fields", func(t *testing.T) { + types := ethcoder.TypedDataTypes{ + "EIP712Domain": {}, + "A": { + {Name: "x", Type: "string"}, + {Name: "y", Type: "string"}, + {Name: "z", Type: "string"}, + }, + } + err := types.ValidateTypeGraph(ethcoder.WithMaxFieldsPerType(2)) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds limit of 2") + + require.NoError(t, types.ValidateTypeGraph(ethcoder.WithMaxFieldsPerType(3))) + }) + + t.Run("WithMaxWalkVisits bounds combinatorial cost of an acyclic diamond graph", func(t *testing.T) { + // Each layer doubles the fan-out into the next, so total visits grow + // exponentially with layer count despite the graph staying acyclic. + types := ethcoder.TypedDataTypes{ + "EIP712Domain": {}, + "Root": {{Name: "a", Type: "L1a"}, {Name: "b", Type: "L1b"}}, + "L1a": {{Name: "a", Type: "L2a"}, {Name: "b", Type: "L2b"}}, + "L1b": {{Name: "a", Type: "L2a"}, {Name: "b", Type: "L2b"}}, + "L2a": {{Name: "value", Type: "string"}}, + "L2b": {{Name: "value", Type: "string"}}, + } + err := types.ValidateTypeGraph(ethcoder.WithMaxWalkVisits(3)) + require.Error(t, err) + assert.Contains(t, err.Error(), "too complex") + + require.NoError(t, types.ValidateTypeGraph(ethcoder.WithMaxWalkVisits(1000))) + }) + + t.Run("cycle detection still fires with limit options set", func(t *testing.T) { + types := ethcoder.TypedDataTypes{ + "EIP712Domain": {}, + "A": {{Name: "b", Type: "B"}}, + "B": {{Name: "a", Type: "A"}}, + } + err := types.ValidateTypeGraph(ethcoder.WithMaxTypes(100), ethcoder.WithMaxWalkVisits(1000)) + require.Error(t, err) + assert.Contains(t, err.Error(), "cycle detected") + }) + + t.Run("no options preserves unbounded, unchanged behavior", func(t *testing.T) { + typedData := diamondArrayTypedData(500) + _, err := typedData.EncodeDigest() + require.NoError(t, err) + }) +} + +// TestTypedDataInvalidPrimitiveType guards against a decode-time panic: a field +// whose type is neither a defined custom type nor a recognized primitive used +// to reach an out-of-range index in the primitive decoder. It must now surface +// as an error, never a panic, for any attacker-controlled type string. +func TestTypedDataInvalidPrimitiveType(t *testing.T) { + for _, typ := range []string{"", "foobar", "tuple", "byte", "String", "address ", "uint2560"} { + t.Run("type="+typ, func(t *testing.T) { + js := `{"types":{"EIP712Domain":[],"M":[{"name":"x","type":"` + typ + `"}]},` + + `"primaryType":"M","domain":{},"message":{"x":"1"}}` + require.NotPanics(t, func() { + _, err := ethcoder.TypedDataFromJSON(js) + require.Error(t, err) + }) + }) + } +} + +// TestTypedDataTypeGraphHardening covers the unconditional guards in +// ValidateTypeGraph: the nesting ceiling that keeps the recursive encoders off +// a deep type chain, and rejection of field types no encoder can handle. +func TestTypedDataTypeGraphHardening(t *testing.T) { + linearChain := func(n int) ethcoder.TypedDataTypes { + types := ethcoder.TypedDataTypes{"EIP712Domain": {}} + for i := range n { + if i == n-1 { + types[fmt.Sprintf("T%d", i)] = []ethcoder.TypedDataArgument{{Name: "v", Type: "string"}} + continue + } + types[fmt.Sprintf("T%d", i)] = []ethcoder.TypedDataArgument{{Name: "c", Type: fmt.Sprintf("T%d", i+1)}} + } + return types + } + + t.Run("rejects a type chain deeper than the ceiling", func(t *testing.T) { + err := linearChain(1025).ValidateTypeGraph() + require.Error(t, err) + assert.Contains(t, err.Error(), "too deep") + }) + + t.Run("accepts a type chain at the ceiling", func(t *testing.T) { + require.NoError(t, linearChain(1024).ValidateTypeGraph()) + }) + + t.Run("depth ceiling does not depend on map iteration order", func(t *testing.T) { + // Memoization can cut the live DFS stack short, so the ceiling is + // measured from each type's longest downward path instead. Repeat so a + // lucky iteration order can't let an over-deep chain through. + types := linearChain(1025) + for range 20 { + require.Error(t, types.ValidateTypeGraph()) + } + }) + + t.Run("a wide but shallow graph is not depth-rejected", func(t *testing.T) { + types := ethcoder.TypedDataTypes{"EIP712Domain": {}} + const layers = 18 + for i := range layers { + types[fmt.Sprintf("L%d", i)] = []ethcoder.TypedDataArgument{ + {Name: "a", Type: fmt.Sprintf("L%d", i+1)}, + {Name: "b", Type: fmt.Sprintf("L%d", i+1)}, + } + } + types[fmt.Sprintf("L%d", layers)] = []ethcoder.TypedDataArgument{{Name: "v", Type: "string"}} + require.NoError(t, types.ValidateTypeGraph()) + }) + + t.Run("rejects field types no encoder can handle", func(t *testing.T) { + for _, typ := range []string{ + "", "foobar", "tuple", "byte", "String", "address ", + "uint", "int", "uint0", "uint7", "uint2560", "bytes0", "bytes33", + "uint256[", "uint256[a]", "[]uint256", + } { + types := ethcoder.TypedDataTypes{"EIP712Domain": {}, "M": {{Name: "x", Type: typ}}} + assert.Error(t, types.ValidateTypeGraph(), "type %q must be rejected", typ) + } + }) + + t.Run("accepts every valid EIP-712 field type", func(t *testing.T) { + types := ethcoder.TypedDataTypes{ + "EIP712Domain": { + {Name: "name", Type: "string"}, + {Name: "chainId", Type: "uint256"}, + {Name: "verifyingContract", Type: "address"}, + {Name: "salt", Type: "bytes32"}, + }, + "Person": { + {Name: "b", Type: "bool"}, + {Name: "d", Type: "bytes"}, + {Name: "n", Type: "uint8"}, + {Name: "i", Type: "int128"}, + {Name: "b1", Type: "bytes1"}, + }, + "Mail": { + {Name: "from", Type: "Person"}, + {Name: "to", Type: "Person[]"}, + {Name: "fixed", Type: "Person[3]"}, + }, + } + require.NoError(t, types.ValidateTypeGraph()) + }) +} From d58a88b079623e42c7b6d3827e2c0f5b10f7174d Mon Sep 17 00:00:00 2001 From: Patryk Kalinowski Date: Mon, 17 Aug 2026 11:43:27 +0200 Subject: [PATCH 2/3] 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 --- ethcoder/typed_data.go | 78 ++++++++++++---------------------- ethcoder/typed_data_json.go | 5 +-- ethcoder/typed_data_options.go | 32 ++++++-------- ethcoder/typed_data_test.go | 20 +++------ 4 files changed, 47 insertions(+), 88 deletions(-) diff --git a/ethcoder/typed_data.go b/ethcoder/typed_data.go index 8eee9f79..b8a976e0 100644 --- a/ethcoder/typed_data.go +++ b/ethcoder/typed_data.go @@ -23,23 +23,18 @@ type TypedData struct { type TypedDataTypes map[string][]TypedDataArgument -// maxTypeGraphDepth bounds how deeply types may nest. The encoders below this -// validation (encodeTypeCached, hashStruct, encodeValue) all recurse one frame -// per level, so this ceiling is what keeps a long chain of types from -// exhausting the goroutine stack. It is unconditional rather than an Option -// because UnmarshalJSON validates without any, and no real schema comes close. +// maxTypeGraphDepth is unconditional because UnmarshalJSON validates without +// options, and the encoders below recurse one frame per level: with no ceiling +// a long enough type chain exhausts the goroutine stack. const maxTypeGraphDepth = 1024 -// ValidateTypeGraph checks the type graph for cycles, unknown field types, and -// excessive nesting depth. A cycle or a deep enough chain would otherwise cause -// runaway recursion in EncodeType/encodeValue and an unrecoverable stack -// overflow, and an unknown field type would fail deep inside the encoders. This -// must be called before any recursive type traversal. +// ValidateTypeGraph must run before any recursive type traversal: a cycle or an +// over-deep chain would otherwise run away in EncodeType and encodeValue and +// overflow the stack unrecoverably. // -// With no options, size is otherwise unbounded (matching prior behavior). -// WithMaxTypes, WithMaxFieldsPerType, and WithMaxWalkVisits additionally bound -// the schema's size and the cost of this traversal itself, which is -// combinatorial for diamond-shaped (but acyclic) type graphs. +// Without options only correctness is enforced. WithMaxTypes, +// WithMaxFieldsPerType and WithMaxWalkVisits additionally bound schema size and +// this traversal's own combinatorial cost. func (t TypedDataTypes) ValidateTypeGraph(opts ...Option) error { o := resolveOptions(opts) @@ -75,14 +70,10 @@ func (t TypedDataTypes) ValidateTypeGraph(opts ...Option) error { state := make(map[string]int, len(t)) visits := make(map[string]int, len(t)) - // The walk is an explicit-stack DFS rather than recursion so that its own - // depth is heap-bound; maxTypeGraphDepth still caps it, to protect the - // recursive encoders that run after this passes. - // - // depth is the longest downward path from a type, tracked separately from - // visits: the encoders start from primaryType with a cold cache, so the - // live DFS stack (which memoization can cut short depending on map - // iteration order) is not on its own a sound bound on their recursion. + // depth tracks each type's longest downward path rather than the live stack + // height: memoization can cut the stack short depending on map iteration + // order, while the encoders always descend from primaryType with a cold + // cache. An explicit stack keeps this walk itself off the goroutine stack. type frame struct { name string field int @@ -171,9 +162,8 @@ func (t TypedDataTypes) ValidateTypeGraph(opts ...Option) error { return nil } -// validateFieldType rejects any field type that is neither a type defined in -// this schema nor a well-formed EIP-712 primitive. Without this an unknown -// token reaches the primitive decoder, which has no branch for it. +// validateFieldType exists because an unknown type token otherwise reaches the +// primitive decoder, which has no branch for it and indexes past its result. func (t TypedDataTypes) validateFieldType(typ string) error { base := typ if i := strings.Index(base, "["); i > 0 { @@ -191,7 +181,6 @@ func (t TypedDataTypes) validateFieldType(typ string) error { return nil } -// validArraySuffix reports whether s is a run of "[]" / "[N]" groups. func validArraySuffix(s string) bool { for len(s) > 0 { if s[0] != '[' { @@ -211,9 +200,8 @@ func validArraySuffix(s string) bool { return true } -// isPrimitiveType reports whether typ is an EIP-712 atomic or dynamic type. -// Bare "uint"/"int" are rejected: EIP-712 requires the canonical uint256/int256 -// spelling, and the packer cannot size them. +// isPrimitiveType rejects bare "uint"/"int": EIP-712 requires the canonical +// uint256/int256 spelling, and the packer cannot size an unspecified width. func isPrimitiveType(typ string) bool { switch typ { case "address", "bool", "string", "bytes": @@ -233,17 +221,14 @@ func isPrimitiveType(typ string) bool { return false } -// typeInfo is the memoized result of encoding one type's EIP-712 type string -// and its Keccak256 hash, keyed by type name for the lifetime of one cache. type typeInfo struct { encodeType string hash []byte } -// encodeTypeCached is EncodeType's recursive core, sharing cache across the -// whole call tree so a type reached through multiple paths (a diamond in the -// dependency DAG, or the same struct type appearing in many array elements) -// is only encoded once. +// encodeTypeCached shares cache across the whole call tree so a type reached +// by several paths — a diamond in the DAG, or one struct type repeated across +// many array elements — is encoded once rather than per occurrence. func (t TypedDataTypes) encodeTypeCached(cache map[string]*typeInfo, primaryType string) (*typeInfo, error) { if info, ok := cache[primaryType]; ok { return info, nil @@ -362,11 +347,6 @@ func (t *TypedData) HashStruct(primaryType string, data map[string]interface{}) return t.hashStruct(make(map[string]*typeInfo), &budgetState{}, 0, primaryType, data) } -// hashStruct is HashStruct's recursive core. cache and budget are shared -// across the whole call tree of one Encode/EncodeDigest call (domain and -// message alike), so a struct type reached through many array elements has -// its type-hash computed once, and value-driven traversal cost (array -// length, nesting depth) is checked against a single aggregate budget. func (t *TypedData) hashStruct(cache map[string]*typeInfo, budget *budgetState, depth int, primaryType string, data map[string]interface{}) ([]byte, error) { if err := budget.checkDepth(depth); err != nil { return nil, err @@ -425,8 +405,8 @@ func (t *TypedData) encodeValue(cache map[string]*typeInfo, budget *budgetState, return nil, fmt.Errorf("expected array for type %s", typ) } - // Budget checks happen before allocating encodedValues or recursing - // into any element, so an oversized array is rejected up front. + // Checked before allocating or recursing, so an oversized array costs + // nothing to reject. if err := budget.checkArray(len(values)); err != nil { return nil, err } @@ -488,11 +468,7 @@ func (t *TypedData) encodeValue(cache map[string]*typeInfo, budget *budgetState, // * the digest is the hash of the fully encoded EIP712 message // * the encoded message is the fully encoded EIP712 message (0x1901 + domain + hashStruct(message)) // -// opts optionally bound both the schema (ValidateTypeGraph) and the message -// data being encoded — see WithMaxTypes, WithMaxFieldsPerType, -// WithMaxWalkVisits, WithMaxArrayElements, WithMaxRecursionDepth, and -// WithMaxTotalValues. With no opts, behavior is unbounded, matching prior -// versions of this function. +// opts bound both the schema and the message values traversed; see Option. func (t *TypedData) Encode(opts ...Option) ([]byte, []byte, error) { if err := t.Types.ValidateTypeGraph(opts...); err != nil { return nil, nil, err @@ -504,9 +480,8 @@ func (t *TypedData) Encode(opts ...Option) ([]byte, []byte, error) { return nil, nil, err } - // cache and budget are shared across the domain and message hash-struct - // calls below, so type-hash work and the value-traversal budget are - // scoped to this single Encode call, not per hash-struct invocation. + // Shared by the domain and message below so the budget aggregates over the + // whole call rather than resetting per hashStruct. cache := make(map[string]*typeInfo) budget := &budgetState{opts: resolveOptions(opts)} @@ -532,8 +507,7 @@ func (t *TypedData) Encode(opts ...Option) ([]byte, []byte, error) { return digest, encodedMessage, nil } -// EncodeDigest returns the digest of the typed data message. See Encode for -// the optional resource-limit opts. +// EncodeDigest returns the digest of the typed data message. See Encode for opts. func (t *TypedData) EncodeDigest(opts ...Option) ([]byte, error) { digest, _, err := t.Encode(opts...) if err != nil { diff --git a/ethcoder/typed_data_json.go b/ethcoder/typed_data_json.go index 8dfb945e..cdcb4f7a 100644 --- a/ethcoder/typed_data_json.go +++ b/ethcoder/typed_data_json.go @@ -341,9 +341,8 @@ func typedDataDecodePrimitiveValue(typ string, value interface{}) (interface{}, if err != nil { return nil, fmt.Errorf("typedDataDecodePrimitiveValue: %w", err) } - // ABIUnmarshalStringValuesAny silently returns fewer values than requested - // for an unrecognized type token (e.g. "", "foobar"), so guard the index - // rather than panic with out-of-range on attacker-controlled type strings. + // ABIUnmarshalStringValuesAny returns fewer values than requested, with a + // nil error, for a type token it does not recognize. if len(out) != 1 { return nil, fmt.Errorf("typedDataDecodePrimitiveValue: unsupported type %q", typ) } diff --git a/ethcoder/typed_data_options.go b/ethcoder/typed_data_options.go index 8fbf9668..cb595bce 100644 --- a/ethcoder/typed_data_options.go +++ b/ethcoder/typed_data_options.go @@ -2,10 +2,9 @@ package ethcoder import "fmt" -// Option configures optional resource limits for EIP-712 typed-data -// processing (ValidateTypeGraph, Encode, EncodeDigest). The zero value of -// every field means "unlimited", matching the unbounded behavior these -// functions have always had when called with no options. +// Option bounds resource use during EIP-712 typed-data processing. The zero +// value of every limit means unlimited, so callers passing no options keep the +// unbounded behavior these functions have always had. type Option func(*options) type options struct { @@ -25,33 +24,28 @@ func resolveOptions(opts []Option) options { return o } -// WithMaxTypes caps the number of distinct types a schema may define, -// including the implicit EIP712Domain type when it isn't declared explicitly. +// WithMaxTypes caps the distinct types a schema may define, counting the +// implicit EIP712Domain when it is not declared explicitly. func WithMaxTypes(n int) Option { return func(o *options) { o.maxTypes = n } } -// WithMaxFieldsPerType caps the number of fields any single type may declare. +// WithMaxFieldsPerType caps the fields any single type may declare. func WithMaxFieldsPerType(n int) Option { return func(o *options) { o.maxFieldsPerType = n } } -// WithMaxWalkVisits caps the total number of type-graph nodes ValidateTypeGraph -// visits, bounding its own traversal cost against diamond-shaped (but acyclic) -// type graphs where naive recursion would otherwise blow up combinatorially. +// WithMaxWalkVisits caps ValidateTypeGraph's own traversal, which is +// combinatorial for diamond-shaped but acyclic type graphs. func WithMaxWalkVisits(n int) Option { return func(o *options) { o.maxWalkVisits = n } } -// WithMaxArrayElements caps the number of elements in any single array value -// within the message being encoded. +// WithMaxArrayElements caps the elements in any single array value. func WithMaxArrayElements(n int) Option { return func(o *options) { o.maxArrayElements = n } } -// WithMaxRecursionDepth caps how deeply nested arrays and structs in the -// message may be encoded. +// WithMaxRecursionDepth caps how deeply message values may nest. func WithMaxRecursionDepth(n int) Option { return func(o *options) { o.maxRecursionDepth = n } } -// WithMaxTotalValues caps the aggregate number of array elements encoded -// across the entire message for a single Encode/EncodeDigest call. +// WithMaxTotalValues caps array elements aggregated across the whole message. func WithMaxTotalValues(n int) Option { return func(o *options) { o.maxTotalValues = n } } -// budgetState tracks the value-driven traversal budget for one Encode call. -// It is distinct from the type-hash cache: the cache is schema-derived (safe -// to reuse across domain + message), while this counts actual message data. +// budgetState is scoped to a single Encode call: unlike the type-hash cache, +// which is schema-derived, these counts come from the message being encoded. type budgetState struct { opts options totalValues int diff --git a/ethcoder/typed_data_test.go b/ethcoder/typed_data_test.go index ef9077a2..04640a4c 100644 --- a/ethcoder/typed_data_test.go +++ b/ethcoder/typed_data_test.go @@ -840,10 +840,8 @@ func TestTypedDataCycleDetection(t *testing.T) { }) } -// diamondArrayTypedData builds a schema where "Item" is reachable from -// "Batch" through two separate fields ("a" and "b" both being "Shared"), and -// the message is an array of many "Item" elements — exercising both the -// diamond-shaped EncodeType cache and the per-array-element HashStruct cache. +// diamondArrayTypedData exercises both caches at once: Shared is reachable +// twice from Item, and Item repeats across every array element. func diamondArrayTypedData(itemCount int) *ethcoder.TypedData { items := make([]interface{}, itemCount) for i := range items { @@ -1022,10 +1020,8 @@ func TestTypedDataBudgetLimits(t *testing.T) { }) } -// TestTypedDataInvalidPrimitiveType guards against a decode-time panic: a field -// whose type is neither a defined custom type nor a recognized primitive used -// to reach an out-of-range index in the primitive decoder. It must now surface -// as an error, never a panic, for any attacker-controlled type string. +// TestTypedDataInvalidPrimitiveType guards a regression: these type strings +// used to panic with an out-of-range index in the primitive decoder. func TestTypedDataInvalidPrimitiveType(t *testing.T) { for _, typ := range []string{"", "foobar", "tuple", "byte", "String", "address ", "uint2560"} { t.Run("type="+typ, func(t *testing.T) { @@ -1039,9 +1035,6 @@ func TestTypedDataInvalidPrimitiveType(t *testing.T) { } } -// TestTypedDataTypeGraphHardening covers the unconditional guards in -// ValidateTypeGraph: the nesting ceiling that keeps the recursive encoders off -// a deep type chain, and rejection of field types no encoder can handle. func TestTypedDataTypeGraphHardening(t *testing.T) { linearChain := func(n int) ethcoder.TypedDataTypes { types := ethcoder.TypedDataTypes{"EIP712Domain": {}} @@ -1066,9 +1059,8 @@ func TestTypedDataTypeGraphHardening(t *testing.T) { }) t.Run("depth ceiling does not depend on map iteration order", func(t *testing.T) { - // Memoization can cut the live DFS stack short, so the ceiling is - // measured from each type's longest downward path instead. Repeat so a - // lucky iteration order can't let an over-deep chain through. + // Repeated because memoization can cut the live DFS stack short, so a + // lucky iteration order once let an over-deep chain through. types := linearChain(1025) for range 20 { require.Error(t, types.ValidateTypeGraph()) From b280d0a9fcb62cb3c8c696b40ce4aeb33240a9f4 Mon Sep 17 00:00:00 2001 From: Patryk Kalinowski Date: Mon, 17 Aug 2026 12:56:34 +0200 Subject: [PATCH 3/3] 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. --- ethcoder/typed_data.go | 39 ++++++++++++++-------- ethcoder/typed_data_options.go | 59 ++++++++++++++++++++-------------- ethcoder/typed_data_test.go | 32 ++++++++++++++++++ 3 files changed, 92 insertions(+), 38 deletions(-) diff --git a/ethcoder/typed_data.go b/ethcoder/typed_data.go index b8a976e0..ba4ba513 100644 --- a/ethcoder/typed_data.go +++ b/ethcoder/typed_data.go @@ -35,11 +35,11 @@ const maxTypeGraphDepth = 1024 // Without options only correctness is enforced. WithMaxTypes, // WithMaxFieldsPerType and WithMaxWalkVisits additionally bound schema size and // this traversal's own combinatorial cost. -func (t TypedDataTypes) ValidateTypeGraph(opts ...Option) error { +func (t TypedDataTypes) ValidateTypeGraph(opts ...TypedDataOption) error { o := resolveOptions(opts) if o.maxTypes > 0 { - typeCount := len(t) + typeCount := uint(len(t)) if _, ok := t["EIP712Domain"]; !ok { typeCount++ } @@ -49,7 +49,7 @@ func (t TypedDataTypes) ValidateTypeGraph(opts ...Option) error { } if o.maxFieldsPerType > 0 { for name, fields := range t { - if len(fields) > o.maxFieldsPerType { + if uint(len(fields)) > o.maxFieldsPerType { return fmt.Errorf("type %q has %d fields, exceeds limit of %d", name, len(fields), o.maxFieldsPerType) } } @@ -90,7 +90,7 @@ func (t TypedDataTypes) ValidateTypeGraph(opts ...Option) error { for root := range t { if state[root] == done { sum += visits[root] - if o.maxWalkVisits > 0 && sum > o.maxWalkVisits { + if o.maxWalkVisits > 0 && uint(sum) > o.maxWalkVisits { return tooComplex() } continue @@ -119,7 +119,7 @@ func (t TypedDataTypes) ValidateTypeGraph(opts ...Option) error { if depths[base] > top.depth { top.depth = depths[base] } - if o.maxWalkVisits > 0 && top.total > o.maxWalkVisits { + if o.maxWalkVisits > 0 && uint(top.total) > o.maxWalkVisits { return tooComplex() } continue @@ -148,13 +148,13 @@ func (t TypedDataTypes) ValidateTypeGraph(opts ...Option) error { if depth > parent.depth { parent.depth = depth } - if o.maxWalkVisits > 0 && parent.total > o.maxWalkVisits { + if o.maxWalkVisits > 0 && uint(parent.total) > o.maxWalkVisits { return tooComplex() } continue } sum += total - if o.maxWalkVisits > 0 && sum > o.maxWalkVisits { + if o.maxWalkVisits > 0 && uint(sum) > o.maxWalkVisits { return tooComplex() } } @@ -200,8 +200,9 @@ func validArraySuffix(s string) bool { return true } -// isPrimitiveType rejects bare "uint"/"int": EIP-712 requires the canonical -// uint256/int256 spelling, and the packer cannot size an unspecified width. +// isPrimitiveType requires the canonical width spelling (e.g. "uint256", not +// bare "uint" or zero-padded "uint0256"): EIP-712 mandates it, and the packer +// cannot size an unspecified width. func isPrimitiveType(typ string) bool { switch typ { case "address", "bool", "string", "bytes": @@ -209,14 +210,14 @@ func isPrimitiveType(typ string) bool { } if match := regexArgBytes.FindStringSubmatch(typ); len(match) > 0 { size, err := strconv.Atoi(match[1]) - return err == nil && size >= 1 && size <= 32 + return err == nil && size >= 1 && size <= 32 && strconv.Itoa(size) == match[1] } if match := regexArgNumber.FindStringSubmatch(typ); len(match) > 0 { if match[2] == "" { return false } size, err := strconv.Atoi(match[2]) - return err == nil && size >= 8 && size <= 256 && size%8 == 0 + return err == nil && size >= 8 && size <= 256 && size%8 == 0 && strconv.Itoa(size) == match[2] } return false } @@ -226,11 +227,20 @@ type typeInfo struct { hash []byte } +// inProgressTypeInfo marks a cache entry as mid-recursion so encodeTypeCached +// can detect a cycle by identity, without a second map: ValidateTypeGraph +// normally rejects cycles first, but EncodeType/TypeHash/HashStruct can be +// called directly without it. +var inProgressTypeInfo = &typeInfo{} + // encodeTypeCached shares cache across the whole call tree so a type reached // by several paths — a diamond in the DAG, or one struct type repeated across // many array elements — is encoded once rather than per occurrence. func (t TypedDataTypes) encodeTypeCached(cache map[string]*typeInfo, primaryType string) (*typeInfo, error) { if info, ok := cache[primaryType]; ok { + if info == inProgressTypeInfo { + return nil, fmt.Errorf("cycle detected in type graph at %q", primaryType) + } return info, nil } @@ -238,6 +248,7 @@ func (t TypedDataTypes) encodeTypeCached(cache map[string]*typeInfo, primaryType if !ok { return nil, fmt.Errorf("%s type is not defined", primaryType) } + cache[primaryType] = inProgressTypeInfo subTypes := []string{} s := primaryType + "(" @@ -468,8 +479,8 @@ func (t *TypedData) encodeValue(cache map[string]*typeInfo, budget *budgetState, // * the digest is the hash of the fully encoded EIP712 message // * the encoded message is the fully encoded EIP712 message (0x1901 + domain + hashStruct(message)) // -// opts bound both the schema and the message values traversed; see Option. -func (t *TypedData) Encode(opts ...Option) ([]byte, []byte, error) { +// opts bound both the schema and the message values traversed; see TypedDataOption. +func (t *TypedData) Encode(opts ...TypedDataOption) ([]byte, []byte, error) { if err := t.Types.ValidateTypeGraph(opts...); err != nil { return nil, nil, err } @@ -508,7 +519,7 @@ func (t *TypedData) Encode(opts ...Option) ([]byte, []byte, error) { } // EncodeDigest returns the digest of the typed data message. See Encode for opts. -func (t *TypedData) EncodeDigest(opts ...Option) ([]byte, error) { +func (t *TypedData) EncodeDigest(opts ...TypedDataOption) ([]byte, error) { digest, _, err := t.Encode(opts...) if err != nil { return nil, err diff --git a/ethcoder/typed_data_options.go b/ethcoder/typed_data_options.go index cb595bce..48fd5656 100644 --- a/ethcoder/typed_data_options.go +++ b/ethcoder/typed_data_options.go @@ -2,22 +2,22 @@ package ethcoder import "fmt" -// Option bounds resource use during EIP-712 typed-data processing. The zero -// value of every limit means unlimited, so callers passing no options keep the -// unbounded behavior these functions have always had. -type Option func(*options) +// TypedDataOption bounds resource use during EIP-712 typed-data processing. +// The zero value of every limit means unlimited, so callers passing no +// options keep the unbounded behavior these functions have always had. +type TypedDataOption func(*typedDataOptions) -type options struct { - maxTypes int - maxFieldsPerType int - maxWalkVisits int - maxArrayElements int - maxRecursionDepth int - maxTotalValues int +type typedDataOptions struct { + maxTypes uint + maxFieldsPerType uint + maxWalkVisits uint + maxArrayElements uint + maxRecursionDepth uint + maxTotalValues uint } -func resolveOptions(opts []Option) options { - var o options +func resolveOptions(opts []TypedDataOption) typedDataOptions { + var o typedDataOptions for _, opt := range opts { opt(&o) } @@ -26,36 +26,47 @@ func resolveOptions(opts []Option) options { // WithMaxTypes caps the distinct types a schema may define, counting the // implicit EIP712Domain when it is not declared explicitly. -func WithMaxTypes(n int) Option { return func(o *options) { o.maxTypes = n } } +func WithMaxTypes(n uint) TypedDataOption { return func(o *typedDataOptions) { o.maxTypes = n } } // WithMaxFieldsPerType caps the fields any single type may declare. -func WithMaxFieldsPerType(n int) Option { return func(o *options) { o.maxFieldsPerType = n } } +func WithMaxFieldsPerType(n uint) TypedDataOption { + return func(o *typedDataOptions) { o.maxFieldsPerType = n } +} // WithMaxWalkVisits caps ValidateTypeGraph's own traversal, which is // combinatorial for diamond-shaped but acyclic type graphs. -func WithMaxWalkVisits(n int) Option { return func(o *options) { o.maxWalkVisits = n } } +func WithMaxWalkVisits(n uint) TypedDataOption { + return func(o *typedDataOptions) { o.maxWalkVisits = n } +} // WithMaxArrayElements caps the elements in any single array value. -func WithMaxArrayElements(n int) Option { return func(o *options) { o.maxArrayElements = n } } +func WithMaxArrayElements(n uint) TypedDataOption { + return func(o *typedDataOptions) { o.maxArrayElements = n } +} // WithMaxRecursionDepth caps how deeply message values may nest. -func WithMaxRecursionDepth(n int) Option { return func(o *options) { o.maxRecursionDepth = n } } +func WithMaxRecursionDepth(n uint) TypedDataOption { + return func(o *typedDataOptions) { o.maxRecursionDepth = n } +} // WithMaxTotalValues caps array elements aggregated across the whole message. -func WithMaxTotalValues(n int) Option { return func(o *options) { o.maxTotalValues = n } } +func WithMaxTotalValues(n uint) TypedDataOption { + return func(o *typedDataOptions) { o.maxTotalValues = n } +} // budgetState is scoped to a single Encode call: unlike the type-hash cache, // which is schema-derived, these counts come from the message being encoded. type budgetState struct { - opts options - totalValues int + opts typedDataOptions + totalValues uint } func (b *budgetState) checkArray(n int) error { - if b.opts.maxArrayElements > 0 && n > b.opts.maxArrayElements { + count := uint(n) + if b.opts.maxArrayElements > 0 && count > b.opts.maxArrayElements { return fmt.Errorf("array has %d elements, exceeds limit of %d", n, b.opts.maxArrayElements) } - b.totalValues += n + b.totalValues += count if b.opts.maxTotalValues > 0 && b.totalValues > b.opts.maxTotalValues { return fmt.Errorf("typed data exceeds aggregate element budget of %d", b.opts.maxTotalValues) } @@ -63,7 +74,7 @@ func (b *budgetState) checkArray(n int) error { } func (b *budgetState) checkDepth(depth int) error { - if b.opts.maxRecursionDepth > 0 && depth > b.opts.maxRecursionDepth { + if b.opts.maxRecursionDepth > 0 && uint(depth) > b.opts.maxRecursionDepth { return fmt.Errorf("typed data recursion depth %d exceeds limit of %d", depth, b.opts.maxRecursionDepth) } return nil diff --git a/ethcoder/typed_data_test.go b/ethcoder/typed_data_test.go index 04640a4c..434a2a50 100644 --- a/ethcoder/typed_data_test.go +++ b/ethcoder/typed_data_test.go @@ -1085,6 +1085,8 @@ func TestTypedDataTypeGraphHardening(t *testing.T) { "", "foobar", "tuple", "byte", "String", "address ", "uint", "int", "uint0", "uint7", "uint2560", "bytes0", "bytes33", "uint256[", "uint256[a]", "[]uint256", + // Non-canonical width spellings: valid width, wrong digits. + "uint0256", "uint00000008", "bytes01", } { types := ethcoder.TypedDataTypes{"EIP712Domain": {}, "M": {{Name: "x", Type: typ}}} assert.Error(t, types.ValidateTypeGraph(), "type %q must be rejected", typ) @@ -1115,3 +1117,33 @@ func TestTypedDataTypeGraphHardening(t *testing.T) { require.NoError(t, types.ValidateTypeGraph()) }) } + +// TestTypedDataDirectCycleDetection guards a regression: EncodeType, TypeHash +// and HashStruct can be called directly without ValidateTypeGraph running +// first, so encodeTypeCached must detect a cycle itself rather than +// recursing until the goroutine stack overflows fatally. +func TestTypedDataDirectCycleDetection(t *testing.T) { + cyclic := ethcoder.TypedDataTypes{ + "A": {{Name: "b", Type: "B"}}, + "B": {{Name: "a", Type: "A"}}, + } + + t.Run("EncodeType detects the cycle", func(t *testing.T) { + _, err := cyclic.EncodeType("A") + require.Error(t, err) + assert.Contains(t, err.Error(), "cycle detected") + }) + + t.Run("TypeHash detects the cycle", func(t *testing.T) { + _, err := cyclic.TypeHash("A") + require.Error(t, err) + assert.Contains(t, err.Error(), "cycle detected") + }) + + t.Run("HashStruct detects the cycle", func(t *testing.T) { + typedData := ðcoder.TypedData{Types: cyclic} + _, err := typedData.HashStruct("A", map[string]interface{}{"b": map[string]interface{}{}}) + require.Error(t, err) + assert.Contains(t, err.Error(), "cycle detected") + }) +}