From b3f75ee2b5f27a3edc3f661c756f7658b775d1db Mon Sep 17 00:00:00 2001 From: jwinterm Date: Sun, 7 Jun 2026 16:37:54 -0400 Subject: [PATCH] Contracts: use checked conversions for amount -> i64 A large u64 amount could wrap when cast `as i64`. Convert with i64::try_from in the CLI net-change derivation and in contract view, returning an error instead. get_net_change / to_api_args now return Result (the two arg panics become errors too), and the to_api_args call moves out of the owner-session closure. Addresses PR #754 review (command.rs, view.rs). --- controller/src/command.rs | 60 +++++++++++++------------- libwallet/src/contract/actions/view.rs | 10 +++-- 2 files changed, 38 insertions(+), 32 deletions(-) diff --git a/controller/src/command.rs b/controller/src/command.rs index f7e5979..4d2bf74 100644 --- a/controller/src/command.rs +++ b/controller/src/command.rs @@ -1649,21 +1649,23 @@ pub struct ContractNewArgs { } impl ContractNewArgs { - fn get_net_change(&self) -> i64 { - // TODO: could the cast 'as i64' overflow or something? + fn get_net_change(&self) -> Result { + let to_i64 = |v: u64| { + i64::try_from(v).map_err(|_| Error::ArgumentError(format!("Amount {} is too large", v))) + }; match self.receive { None => match self.send { - None => panic!("Send or receive not specified."), - Some(v) => -(v as i64), // negative net change on send + None => Err(Error::ArgumentError("Send or receive not specified.".into())), + Some(v) => Ok(-to_i64(v)?), // negative net change on send }, - Some(v) => v as i64, // positive net change on receive + Some(v) => to_i64(v), // positive net change on receive } } // Create a ContractNewArgsAPI from the ContractNewArgs - fn to_api_args(&self) -> ContractNewArgsAPI { - let net_change = self.get_net_change(); - ContractNewArgsAPI { + fn to_api_args(&self) -> Result { + let net_change = self.get_net_change()?; + Ok(ContractNewArgsAPI { setup_args: ContractSetupArgsAPI { src_acct_name: match self.src_acct_name.as_ref() { Some(v) => Some(v.to_string()), @@ -1686,7 +1688,7 @@ impl ContractNewArgs { proof_args: Default::default(), }, ..Default::default() - } + }) } } @@ -1700,9 +1702,8 @@ where C: NodeClient + 'static, K: keychain::Keychain + 'static, { + let contract_new_args = args.to_api_args()?; controller::owner_single_use(None, keychain_mask, Some(owner_api), |api, m| { - let contract_new_args = args.to_api_args(); - let slate = api.contract_new(m, &contract_new_args)?; print_slatepack( @@ -1748,25 +1749,27 @@ pub struct ContractSetupArgs { } impl ContractSetupArgs { - fn get_net_change(&self) -> Option { - let mut net_change: Option = None; - // TODO: Check bounds before casting to i64. + fn get_net_change(&self) -> Result, Error> { if self.receive.is_some() && self.send.is_some() { - panic!("Can't pass both --receive and --send parameters."); + return Err(Error::ArgumentError( + "Can't pass both --receive and --send parameters.".into(), + )); } - if self.receive.is_some() { - net_change = Some(self.receive.unwrap() as i64); - } - if self.send.is_some() { - net_change = Some(-(self.send.unwrap() as i64)); - } - net_change + let to_i64 = |v: u64| { + i64::try_from(v).map_err(|_| Error::ArgumentError(format!("Amount {} is too large", v))) + }; + let net_change = match (self.receive, self.send) { + (Some(v), _) => Some(to_i64(v)?), + (_, Some(v)) => Some(-to_i64(v)?), + (None, None) => None, + }; + Ok(net_change) } // Create a ContractSetupArgsAPI from the ContractSetupArgs - fn to_api_args(&self) -> ContractSetupArgsAPI { - let net_change = self.get_net_change(); - ContractSetupArgsAPI { + fn to_api_args(&self) -> Result { + let net_change = self.get_net_change()?; + Ok(ContractSetupArgsAPI { // TODO: num_participants is derived here. It should be an Option and read from the slate. // Need to check no attack are possible regarding kernel fee contribution. net_change: net_change, @@ -1783,7 +1786,7 @@ impl ContractSetupArgs { ..Default::default() }, ..Default::default() - } + }) } } @@ -1864,6 +1867,8 @@ where C: NodeClient + 'static, K: keychain::Keychain + 'static, { + // Args for signing are just setup args + let contract_sign_args = args.to_api_args()?; controller::owner_single_use(None, keychain_mask, Some(owner_api), |api, m| { // Read the slatepack from stdin println!("Paste slatepack:"); @@ -1872,9 +1877,6 @@ where .read_line(&mut slatepack_msg) .expect("Failed to read from stdin"); - // Args for signing are just setup args - let contract_sign_args = args.to_api_args(); - // Decrypt the slate, sign it and encrypt it for the next party // TODO: Make sure you get the counterparty_addr and slate with 1 call. let slatepack = owner::decode_slatepack_message( diff --git a/libwallet/src/contract/actions/view.rs b/libwallet/src/contract/actions/view.rs index fcbda67..a28885e 100644 --- a/libwallet/src/contract/actions/view.rs +++ b/libwallet/src/contract/actions/view.rs @@ -37,10 +37,14 @@ where // Otherwise, you can't really predict who the party doing the next step should be. // TODO: Do we need to do any slate verification here? + // Checked conversion so an out-of-range amount can't wrap into a bogus net change. let suggested_net_change: Option = match slate.state { - // TODO: Check bounds against overflow/underflow - SlateState::Invoice1 => Some(slate.amount as i64), - SlateState::Standard1 => Some(-(slate.amount as i64)), + SlateState::Invoice1 => Some(i64::try_from(slate.amount).map_err(|_| { + Error::GenericError(format!("Slate amount {} exceeds i64", slate.amount)) + })?), + SlateState::Standard1 => Some(-i64::try_from(slate.amount).map_err(|_| { + Error::GenericError(format!("Slate amount {} exceeds i64", slate.amount)) + })?), _ => None, }; let is_executed = false;