From 148987417d84e2bf63164b97e3592d6a6026a4a3 Mon Sep 17 00:00:00 2001 From: jwinterm Date: Wed, 10 Jun 2026 14:28:35 -0400 Subject: [PATCH] Contracts: replace reachable panics with errors De-panic the contract paths flagged in review plus several found alongside them: - new: use unsigned_abs (abs() panics on i64::MIN). - proofs: surface InvoiceProof serialization errors instead of expect(); bounds- check the participant index before indexing participant_data. - utils/slate: propagate the error when a context input is missing from the db instead of unwrapping. - revoke: return an error when a locked input has no cached commitment. - selection: checked i64/u64 conversions so large amounts can't wrap into a wrong (possibly negative) change. - types: checked sum of output amounts. Addresses PR #754 review (new.rs, proofs.rs, utils.rs, selection.rs). --- libwallet/src/contract/actions/new.rs | 3 ++- libwallet/src/contract/actions/revoke.rs | 4 +++- libwallet/src/contract/proofs.rs | 17 +++++++++++++---- libwallet/src/contract/selection.rs | 18 +++++++++++++----- libwallet/src/contract/slate.rs | 2 +- libwallet/src/contract/types.rs | 5 ++++- libwallet/src/contract/utils.rs | 2 +- 7 files changed, 37 insertions(+), 14 deletions(-) diff --git a/libwallet/src/contract/actions/new.rs b/libwallet/src/contract/actions/new.rs index 04e0091..18ad453 100644 --- a/libwallet/src/contract/actions/new.rs +++ b/libwallet/src/contract/actions/new.rs @@ -69,7 +69,8 @@ where let num_participants = setup_args.num_participants; let mut slate = Slate::blank(num_participants, net_change > 0); // We set slate.amount to contain the _positive_ net_change for the other party so they can derive expectations. - slate.amount = net_change.abs() as u64; + // unsigned_abs avoids the i64::MIN overflow panic of abs(). + slate.amount = net_change.unsigned_abs(); debug!("contract::new => slate amount: {}", slate.amount); // Perform setup for the slate diff --git a/libwallet/src/contract/actions/revoke.rs b/libwallet/src/contract/actions/revoke.rs index 6e346fd..d0b584f 100644 --- a/libwallet/src/contract/actions/revoke.rs +++ b/libwallet/src/contract/actions/revoke.rs @@ -84,7 +84,9 @@ where if my_contributed_inputs.len() == 0 { return Ok(None); } - let input_commit = my_contributed_inputs[0].commit.as_ref().unwrap(); + let input_commit = my_contributed_inputs[0].commit.as_ref().ok_or_else(|| { + Error::GenericError("Locked input has no cached commitment".to_string()) + })?; // Account label for the self-spend, so recovered funds return to the inputs' account. let src_acct_name = w .acct_path_iter()? diff --git a/libwallet/src/contract/proofs.rs b/libwallet/src/contract/proofs.rs index 2095ae3..0874482 100644 --- a/libwallet/src/contract/proofs.rs +++ b/libwallet/src/contract/proofs.rs @@ -197,6 +197,15 @@ impl InvoiceProof { participant_index: usize, sender_address: Option, ) -> Result { + // Bounds-check the participant index before indexing participant_data, so a + // malformed slate returns an error rather than panicking. + if participant_index >= slate.participant_data.len() { + return Err(Error::GenericError(format!( + "Participant index {} out of range for slate with {} participant(s)", + participant_index, + slate.participant_data.len() + ))); + } // Sender address is either provided or in slate (or error) let sender_address = match sender_address { Some(a) => a, @@ -257,8 +266,8 @@ impl InvoiceProof { secret: d_skey, }; let mut sig_data_bin = Vec::new(); - let _ = grin_ser::serialize_default(&mut sig_data_bin, &InvoiceProofBin(self.clone())) - .expect("serialization failed"); + grin_ser::serialize_default(&mut sig_data_bin, &InvoiceProofBin(self.clone())) + .map_err(|e| Error::GenericError(format!("InvoiceProof serialization failed: {}", e)))?; Ok((keypair.sign(&sig_data_bin), pub_key)) } @@ -276,8 +285,8 @@ impl InvoiceProof { // Rebuild message let mut sig_data_bin = Vec::new(); - let _ = grin_ser::serialize_default(&mut sig_data_bin, &InvoiceProofBin(self.clone())) - .expect("serialization failed"); + grin_ser::serialize_default(&mut sig_data_bin, &InvoiceProofBin(self.clone())) + .map_err(|e| Error::GenericError(format!("InvoiceProof serialization failed: {}", e)))?; if recipient_address .verify(&sig_data_bin, self.promise_signature.as_ref().unwrap()) diff --git a/libwallet/src/contract/selection.rs b/libwallet/src/contract/selection.rs index def393f..b3cc6b6 100644 --- a/libwallet/src/contract/selection.rs +++ b/libwallet/src/contract/selection.rs @@ -142,9 +142,10 @@ where // or a single one if we're the receiver doing a payjoin. // If we are the receiver, we pretend we have a virtual input from the sender (for which we don't pay the fees) so we can easily // test that lhs >= rhs and see if we will be able to satisfy equation. We simulate this by starting with lhs = net_change. - let mut lhs = 0; + let mut lhs: u64 = 0; if net_change > 0 { - lhs = net_change as u64; // TODO: check bounds + lhs = u64::try_from(net_change) + .map_err(|_| Error::GenericError(format!("net change {} exceeds u64", net_change)))?; } // We want to count how many inputs we've picked _so far_. This is used to prevent picking // all 0*H +r*G outputs when we call with min_input_amount=0 and want just a payjoin. @@ -262,9 +263,16 @@ fn build_output_amount_list( let custom_outputs_sum = my_output_amounts.iter().sum::(); // We know that `Σmy_inputs >= Σmy_outputs + my_fee_cost` holds so we balance the equation by adding // an additional output holding the missing amount (change output or receiver output) - // TODO: check bounds when casting. - let my_change_output_amount = - (my_input_sum - custom_outputs_sum) as i64 + expected_net_change - my_fee_cost as i64; + // Checked conversions so a very large amount can't wrap into a wrong (possibly negative) change. + let input_minus_outputs = i64::try_from(my_input_sum - custom_outputs_sum).map_err(|_| { + Error::GenericError(format!( + "input sum {} exceeds i64", + my_input_sum - custom_outputs_sum + )) + })?; + let fee_cost = i64::try_from(my_fee_cost) + .map_err(|_| Error::GenericError(format!("fee cost {} exceeds i64", my_fee_cost)))?; + let my_change_output_amount = input_minus_outputs + expected_net_change - fee_cost; // A negative change output would mean the selection math is off; reject it explicitly. if my_change_output_amount < 0 { return Err(Error::GenericError(format!( diff --git a/libwallet/src/contract/slate.rs b/libwallet/src/contract/slate.rs index 86a1196..7c5810c 100644 --- a/libwallet/src/contract/slate.rs +++ b/libwallet/src/contract/slate.rs @@ -103,7 +103,7 @@ where let batch = w.batch(keychain_mask)?; for (key_id, mmr_index, _) in context.get_inputs() { // We have no information if the input is a coinbase or not, so we fetch the data from DB - let coin = batch.get(&key_id, &mmr_index).unwrap(); + let coin = batch.get(&key_id, &mmr_index)?; if coin.is_coinbase { slate.add_transaction_elements( &keychain, diff --git a/libwallet/src/contract/types.rs b/libwallet/src/contract/types.rs index b847a66..97cc387 100644 --- a/libwallet/src/contract/types.rs +++ b/libwallet/src/contract/types.rs @@ -55,7 +55,10 @@ impl OutputSelectionArgs { } /// Returns the sum of our output amounts pub fn sum_output_amounts(&self) -> Result { - Ok(self.output_amounts()?.iter().sum()) + self.output_amounts()? + .iter() + .try_fold(0u64, |acc, v| acc.checked_add(*v)) + .ok_or_else(|| Error::GenericError("output amounts sum overflow".to_string())) } /// Returns the number of custom outputs pub fn num_custom_outputs(&self) -> usize { diff --git a/libwallet/src/contract/utils.rs b/libwallet/src/contract/utils.rs index 4f55543..4f724e0 100644 --- a/libwallet/src/contract/utils.rs +++ b/libwallet/src/contract/utils.rs @@ -266,7 +266,7 @@ where } // Lock inputs for id in context.get_inputs() { - let mut coin = batch.get(&id.0, &id.1).unwrap(); + let mut coin = batch.get(&id.0, &id.1)?; // At this point we already have context.log_id set coin.tx_log_entry = context.log_id; batch.lock_output(&mut coin)?;