Repository navigation
Reject invalid introspected transaction values - #128
chris-ricketts merged 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe change validates input and output satoshi amounts before opcode stack conversion. It rejects missing previous outputs, negative values, and values above ChangesSatoshi value validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review — #128
Summary: Two valid safety fixes — a nil-dereference panic removed from opcodeInspectInputValue, and a silent sign-extend/wrap-around bug removed from both value opcodes. The go.mod promotion is correct. No cross-repo breakage: go-sdk, ts-sdk, and compiler consumers operate on well-formed Bitcoin transactions and will not be affected by the tighter bounds.
Bugs / Must-fix
pkg/arkade/opcode.go — wrong error code in pushSatoshiValue
// opcode.go (new helper)
func pushSatoshiValue(value int64, vm *Engine) error {
if value < 0 || value > btcutil.MaxSatoshi {
return scriptError(txscript.ErrInvalidIndex, "value out of range") // ← wrong
}
return vm.dstack.PushBigNum(BigNumFromUint64(uint64(value)))
}txscript.ErrInvalidIndex is reserved for bounds violations on indices. Using it here means any caller that dispatches on the error kind to distinguish "bad index" from "bad monetary value" will misclassify this. The natural choice for "value too large to represent" is txscript.ErrNumberTooBig, which is already used at opcode.go:1618 and 1628 for analogous BigNum overflow cases. Same applies to the prevOut == nil sentinel — that could legitimately be ErrInvalidIndex (the outpoint doesn't resolve), so the nil-check error code is defensible; the range error is not.
Security observations
Nil-dereference panic — fixed correctly
Old opcode.go:2028 accessed prevOut.Value without a nil guard. The test infrastructure (opcode_test.go:5375, engine_fuzz_test.go:257) both defensively check != nil before using the result, confirming the fetcher can return nil in practice. A malformed transaction referencing an unknown outpoint could have caused a panic (script-execution DoS). The added guard at the new opcode.go:2028–2030 closes that.
Silent uint64 wrap-around — fixed correctly
Before this PR, uint64(vm.tx.TxOut[index].Value) with a negative int64 would silently produce a value near 2^64 − 1 and push it as a BigNum, making value-comparison covenants trivially bypassable for any output carrying a dust-negative value. Same issue existed for input values via prevOut.Value. Both are now gated by pushSatoshiValue.
Minor observations
pkg/arkade/opcode_test.go — missing_prevout vector relies on mock-fetcher nil semantics
The missing_prevout invalid vector sets w.tx.TxIn[0].PreviousOutPoint = wire.OutPoint{} and expects txscript.ErrInvalidIndex. This only fires if the test's mock fetcher returns nil for an unregistered outpoint (confirmed at opcode_test.go:5375). The production ArkPrevOutFetcher contract (via txscript.PrevOutputFetcher) does not mandate nil returns; btcd's built-in fetchers return &wire.TxOut{}. Worth a comment in the test clarifying the assumption, or a companion test that sets w.prevFetcher to a fetcher whose FetchPrevOutput always returns &wire.TxOut{} with Value = 0 and verifies the opcode still succeeds (Value 0 is in range). Not a correctness bug today, but fragile if a new fetcher implementation follows btcd convention.
go.mod — correct
btcutil moves from indirect to direct; version v1.1.5 was already pinned in the module graph, so no new dependency enters the build.
Cross-repo impact
go-sdk/vhtlc/noninteractive.go:61–63 and ts-sdk tests construct scripts that call OP_INSPECTOUTPUTVALUE / OP_INSPECTINPUTVALUE on real Bitcoin VTXOs. Those VTXOs carry values in [0, MaxSatoshi] by construction, so the new range gate is a no-op for legitimate transactions. No SDK or compiler changes needed.
Required before merge: Fix the txscript.ErrInvalidIndex → txscript.ErrNumberTooBig (or a purpose-specific error code) in pushSatoshiValue for the value-out-of-range path. Everything else is clean.
chris-ricketts
left a comment
chris-ricketts
left a comment
There was a problem hiding this comment.
LGTM - updated the property checkers and ran FuzzOpcodes for 10 minutes
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Pass 2 — 1 commit since last review (7d6d248); 0 of 1 prior issues fixed; 1 still open (blocker).
Prior issue status
❌ Still open — wrong error code in (opcode.go:2037)
The only item I marked Required before merge in pass 1 is unchanged. The new commit (7d6d248) touches only opcode_test.go; opcode.go is unmodified since 782208c.
// pkg/arkade/opcode.go:2035-2038 (unchanged since 782208c)
func pushSatoshiValue(value int64, vm *Engine) error {
if value < 0 || value > btcutil.MaxSatoshi {
return scriptError(txscript.ErrInvalidIndex, "value out of range") // ← still wrong
}ErrInvalidIndex means a script-index operand was out of bounds. Returning it for a monetary-value range violation means any downstream code that dispatches on error kind to distinguish the two conditions will misclassify this. txscript.ErrNumberTooBig (already used at lines 1619, 1629, 1645, 2482, 2886 for analogous overflow cases) is the correct sentinel. This stays a blocker.
New findings — 7d6d248 (opcode_test.go only)
The 8 added lines instrument both property checkers (inspectInputPropertyChecker and inspectOutputPropertyChecker) to assert that a successfully-pushed value satisfies 0 ≤ result ≤ btcutil.MaxSatoshi.
Correctness of the new assertions — sound. Cmp returns −1/0/+1, so:
require.LessOrEqual(t, topBigNum.Cmp(BigNumFromUint64(btcutil.MaxSatoshi)), 0)→ value ≤ MaxSatoshi ✓require.GreaterOrEqual(t, topBigNum.Cmp(BigNumFromUint64(0)), 0)→ value ≥ 0 ✓
Tautology today, useful regression guard — because pushSatoshiValue already rejects out-of-range values before any push happens, a successful execution will always satisfy the new asserts. The value is that if a future change weakens the guard in pushSatoshiValue, the property tests will catch it. This is fine and worth keeping.
Minor (non-blocking): the want calculation at opcode_test.go:5117 still does BigNumFromUint64(uint64(prevOut.Value)) with a raw int64→uint64 cast. For in-range values (the only ones that reach this path) the cast is safe. No action needed.
No new findings.
Fix required before merge: change txscript.ErrInvalidIndex → txscript.ErrNumberTooBig at pkg/arkade/opcode.go:2037.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Pass 2 — 1 commit since last review (7d6d248); 0 of 1 prior issues fixed; 1 still open (blocker).
Prior issue status
❌ Still open — wrong error code in pushSatoshiValue (opcode.go:2037)
The only item I marked Required before merge in pass 1 is unchanged. The new commit (7d6d248) touches only opcode_test.go; opcode.go is unmodified since 782208c.
// pkg/arkade/opcode.go:2035-2038 (unchanged since 782208c)
func pushSatoshiValue(value int64, vm *Engine) error {
if value < 0 || value > btcutil.MaxSatoshi {
return scriptError(txscript.ErrInvalidIndex, "value out of range") // ← still wrong
}ErrInvalidIndex means a script-index operand was out of bounds. Returning it for a monetary-value range violation means any downstream code that dispatches on error kind to distinguish the two conditions will misclassify this. txscript.ErrNumberTooBig (already used at opcode.go:1619, 1629, 1645, 2482, 2886 for analogous overflow cases) is the correct sentinel. This stays a blocker.
New findings — 7d6d248 (opcode_test.go only)
The 8 added lines instrument both property checkers (inspectInputPropertyChecker and inspectOutputPropertyChecker) to assert that a successfully-pushed value satisfies 0 ≤ result ≤ btcutil.MaxSatoshi.
Correctness of the new assertions — sound. Cmp returns −1/0/+1, so:
require.LessOrEqual(t, topBigNum.Cmp(BigNumFromUint64(btcutil.MaxSatoshi)), 0)→ value ≤ MaxSatoshi ✓require.GreaterOrEqual(t, topBigNum.Cmp(BigNumFromUint64(0)), 0)→ value ≥ 0 ✓
Tautology today, useful regression guard — because pushSatoshiValue already rejects out-of-range values before any push happens, a successful execution will always satisfy the new asserts. The value is that if a future change weakens the guard in pushSatoshiValue, the property tests will catch it. Fine; worth keeping.
Minor (non-blocking): the want calculation at opcode_test.go:5117 still does BigNumFromUint64(uint64(prevOut.Value)) with a raw int64→uint64 cast. For in-range values (the only ones that reach this path after pushSatoshiValue gates them) the cast is safe. No action needed.
No new findings beyond the above.
Fix required before merge: change txscript.ErrInvalidIndex → txscript.ErrNumberTooBig at pkg/arkade/opcode.go:2037.
Closes #124.
Rejects missing previous outputs and transaction values outside Bitcoin's monetary range before either value-introspection opcode converts them to an unsigned script number. Both
OP_INSPECTINPUTVALUEandOP_INSPECTOUTPUTVALUEshare the same range check.Tests cover
-1, exactlyMaxSatoshi,MaxSatoshi + 1, and a missing previous output.Validation:
make testfrom a clean clonego vet ./...inpkg/arkademake integrationtestagainst a clean rebuilt regtest stack (376.148s)Summary by CodeRabbit