PM-22018: Harden arithmetic using checked operations in single_tx.rs#1293
Open
PM-22018: Harden arithmetic using checked operations in single_tx.rs#1293
Conversation
|
|
…tions Signed-off-by: Mike Clay <mike.clay@shielded.io>
m2ux
force-pushed
the
task/PM-22018-harden-arithmetic-checked-operations
branch
from
91de2a7 to
785cddd
Compare
…arithmetic-checked-operations Signed-off-by: Mike Clay <mike.clay@shielded.io>
…22018) Replace unchecked u128 arithmetic with checked alternatives in select_inputs functions and convert .expect() panics to Result-based error propagation in build_shielded_offer/build_unshielded_intents. Add ArithmeticOverflow error variants to both ShieldedCoinSelectionError and UtxoSelectionError enums. Include boundary-value unit tests covering overflow, underflow, and normal paths. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mike Clay <mike.clay@shielded.io>
…into task/PM-22018-harden-arithmetic-checked-operations Signed-off-by: Mike Clay <mike.clay@shielded.io>
…(PM-22018) - Replace unchecked multiplication in batches.rs with checked_mul to prevent potential overflow panic in initial_shielded_offer - Fix stale comment referencing map_or(false, ...) to match actual is_some_and usage in input.rs - Add missing test cases to utxo_spend.rs for parity with input.rs: multiple_sum_to_required, zero_required, insufficient_returns_none Signed-off-by: Mike Clay <mike.clay@shielded.io>
Signed-off-by: Mike Clay <mike.clay@shielded.io>
…st_module (PM-22018) Signed-off-by: Mike Clay <mike.clay@shielded.io>
Signed-off-by: Mike Clay <mike.clay@shielded.io>
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
Replace unchecked arithmetic operations in
single_tx.rswith checked alternatives (checked_mul,checked_sub,checked_add) and proper error propagation, converting panics on overflow/underflow into recoverable errors. Addresses informational finding from the Least Authority Node DIFF Audit (Feb 2026).🎫 Ticket 📐 Engineering
Motivation
The transaction builder in
util/toolkit/src/tx_generator/builder/builders/single_tx.rsuseschecked_mulfor amount aggregation but calls.expect()on the result, converting arithmetic overflow into a panic rather than a recoverable error. Similarly, subtraction for change calculation may underflow with adversarial inputs. While the toolkit is not consensus-critical, panics in production tooling are undesirable and violate defence-in-depth principles.This change ensures that large or adversarial input values produce typed errors that callers can handle gracefully, rather than crashing the process.
Changes
Implementation:
.expect()calls onchecked_mulresults with proper error propagation via?/ok_or()ShieldedCoinSelectionError,UtxoSelectionError) with arithmetic overflow/underflow variants📌 Submission Checklist
🔱 Fork Strategy
🗹 TODO before merging