Skip to content

Add missing OP_CAT result size check#100

Merged
louisinger merged 1 commit into
masterfrom
fix/stack-limits
Jun 16, 2026
Merged

Add missing OP_CAT result size check#100
louisinger merged 1 commit into
masterfrom
fix/stack-limits

Conversation

@msinkec

@msinkec msinkec commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Enforce stack element size limit in OP_CAT

Problem: OP_CAT concatenated two operands and pushed the result without checking it against MaxScriptElementSize (520 bytes). Two valid ≤520-byte elements could produce up to 1040 bytes, and repeated OP_DUP OP_CAT doubles the element each round — an unbounded exponential-growth DoS and a consensus-rule violation. It was the only element-producing opcode missing this check (all splice/bitwise/arithmetic/hash/introspection ops are already bounded).

Fix: Reject the operation with ErrElementTooBig when len(x1)+len(x2) > MaxScriptElementSize, matching the pattern already used by the packet-introspection opcodes.

Tests:

  • Opcode-level: rejects oversized concat (300+300), allows exact boundary (260+260=520).
  • Engine-level: OP_CAT_growth_limit proves the repeated-doubling attack now fails.

@msinkec msinkec self-assigned this Jun 16, 2026
@msinkec msinkec added the bug Something isn't working label Jun 16, 2026

@arkanaai arkanaai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved — clean consensus fix

Verdict: Approve. This is a correct, well-tested fix for a real consensus violation and DoS vector.

Code review

pkg/arkade/opcode.go:2157-2162 — the fix:

  • Check placement is correct: before append and before PushByteArray, so the oversized element never touches the stack.
  • Uses > (not >=), so 520-byte results are correctly allowed. Matches the MaxScriptElementSize semantics used everywhere else.
  • len(x1)+len(x2) cannot overflow: individual elements are already capped at 520 by the engine push-data check (engine.go:289), so the sum maxes at 1040 — well within int range.
  • Pattern exactly matches the existing checks in opcodeInspectPacket (opcode.go:2937) and opcodeInspectInputPacket (opcode.go:2999). Consistent.
  • No other element-producing opcodes are missing this check: AND/OR/XOR require equal-length operands and produce same-size output; Substr/Left/Right only shrink; BigNum ops go through PushBigNum which is bounded by maxBigNumLen.

pkg/arkade/opcode_test.go — unit tests:

  • result_at_max_element_size: 260+260=520 valid boundary. ✓
  • result_exceeds_max_element_size: 300+300=600 ErrElementTooBig. ✓

pkg/arkade/engine_test.go — integration test:

  • OP_CAT_growth_limit: 200→400→800 via OP_DUP OP_CAT loop. Proves the exponential-growth DoS path is killed. ✓

pkg/arkade/opcode_test.go:1518 — fuzz harness update:

  • ErrElementTooBig added to byteTransformPropertyChecker accepted errors. Prevents false-positive fuzz failures. ✓

Cross-repo impact

None. Internal opcode handler change only. No public API, proto, or type changes.


⚠️ Protocol-critical: consensus rule enforcement. Requires human sign-off before merge.

🤖 Reviewed by Arkana

@louisinger louisinger merged commit 1f0a246 into master Jun 16, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants