feat: add overflow checks to change and fee calculations

base_layer/core/src/blocks/block.rs

@@ -197,7 +197,8 @@ impl BlockBuilder {
     /// This function adds the provided transaction kernels to the block WITHOUT updating kernel_mmr_size in the header
     pub fn add_kernels(mut self, mut kernels: Vec<TransactionKernel>) -> Self {
         for kernel in &kernels {
-            self.total_fee += kernel.fee;
+            // Saturating add is used here to prevent overflow; invalid fees will be caught by block validation
+            self.total_fee = self.total_fee.saturating_add(kernel.fee);
         }
         self.kernels.append(&mut kernels);
         self

base_layer/core/src/mempool/unconfirmed_pool/unconfirmed_pool.rs

@@ -864,7 +864,7 @@ impl UnconfirmedPool {
                 "Shrunk reorg mempool memory usage ({}/{}) ~{}%",
                 new,
                 old,
-                (old - new).saturating_mul(100) / old  as usize
+                (old - new).saturating_mul(100) / old
             );
         }
     }

base_layer/core/src/transactions/fee.rs

@@ -52,7 +52,9 @@ impl Fee {
             num_outputs,
             rounded_features_and_scripts_byte_size,
         );
-        MicroMinotari::from(weight) * fee_per_gram
+        // Saturating multiplication is used here to prevent overflow only; invalid values will be caught with
+        // validation
+        MicroMinotari::from(weight.saturating_mul(fee_per_gram.0))
     }
 
     pub fn calculate_body(&self, fee_per_gram: MicroMinotari, body: &AggregateBody) -> std::io::Result<MicroMinotari> {

base_layer/core/src/transactions/tari_amount.rs

@@ -113,30 +113,34 @@ impl MicroMinotari {
         Self(0)
     }
 
-    pub fn checked_add(self, v: MicroMinotari) -> Option<MicroMinotari> {
-        self.as_u64().checked_add(v.as_u64()).map(Into::into)
+    pub fn checked_add<T>(&self, v: T) -> Option<MicroMinotari>
+    where T: AsRef<MicroMinotari> {
+        self.as_u64().checked_add(v.as_ref().as_u64()).map(Into::into)
     }
 
-    pub fn checked_sub(self, v: MicroMinotari) -> Option<MicroMinotari> {
-        if self >= v {
-            return Some(self - v);
-        }
-        None
+    pub fn checked_sub<T>(&self, v: T) -> Option<MicroMinotari>
+    where T: AsRef<MicroMinotari> {
+        self.as_u64().checked_sub(v.as_ref().as_u64()).map(Into::into)
     }
 
-    pub fn checked_mul(self, v: MicroMinotari) -> Option<MicroMinotari> {
-        self.as_u64().checked_mul(v.as_u64()).map(Into::into)
+    pub fn checked_mul<T>(&self, v: T) -> Option<MicroMinotari>
+    where T: AsRef<MicroMinotari> {
+        self.as_u64().checked_mul(v.as_ref().as_u64()).map(Into::into)
     }
 
-    pub fn checked_div(self, v: MicroMinotari) -> Option<MicroMinotari> {
-        self.as_u64().checked_div(v.as_u64()).map(Into::into)
+    pub fn checked_div<T>(&self, v: T) -> Option<MicroMinotari>
+    where T: AsRef<MicroMinotari> {
+        self.as_u64().checked_div(v.as_ref().as_u64()).map(Into::into)
     }
 
-    pub fn saturating_sub(self, v: MicroMinotari) -> MicroMinotari {
-        if self >= v {
-            return self - v;
-        }
-        Self(0)
+    pub fn saturating_sub<T>(&self, v: T) -> MicroMinotari
+    where T: AsRef<MicroMinotari> {
+        self.as_u64().saturating_sub(v.as_ref().as_u64()).into()
+    }
+
+    pub fn saturating_add<T>(&self, v: T) -> MicroMinotari
+    where T: AsRef<MicroMinotari> {
+        self.as_u64().saturating_add(v.as_ref().as_u64()).into()
     }
 
     #[inline]
@@ -149,6 +153,12 @@ impl MicroMinotari {
     }
 }
 
+impl AsRef<MicroMinotari> for MicroMinotari {
+    fn as_ref(&self) -> &MicroMinotari {
+        self
+    }
+}
+
 #[allow(clippy::identity_op)]
 impl Display for MicroMinotari {
     fn fmt(&self, f: &mut Formatter) -> Result<(), Error> {
@@ -166,16 +176,18 @@ impl From<MicroMinotari> for u64 {
     }
 }
 
-impl std::str::FromStr for MicroMinotari {
+impl FromStr for MicroMinotari {
     type Err = MicroMinotariError;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let processed = s.replace([',', ' '], "").to_ascii_lowercase();
         // Is this Tari or MicroMinotari
         let is_micro_tari = if processed.ends_with("ut") || processed.ends_with("µt") {
             true
+        } else if processed.ends_with('t') {
+            false
         } else {
-            !processed.ends_with('t')
+            !processed.contains('.')
         };
 
         let processed = processed.replace("ut", "").replace("µt", "").replace('t', "");
@@ -329,16 +341,22 @@ impl FromStr for Minotari {
     type Err = MicroMinotariError;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        let d = Decimal::from_str(s).map_err(|e| MicroMinotariError::ParseError(e.to_string()))?;
-        Self::try_from(d)
+        if s.to_ascii_lowercase().contains('t') {
+            let val = MicroMinotari::from_str(s)?;
+            Ok(Minotari::from(val))
+        } else {
+            let d = Decimal::from_str(s).map_err(|e| MicroMinotariError::ParseError(e.to_string()))?;
+            Self::try_from(d)
+        }
     }
 }
 
 impl Display for Minotari {
     fn fmt(&self, f: &mut Formatter) -> Result<(), Error> {
-        // User can choose decimal precision, but default is 6
+        let d1 = Decimal::try_from(self.0.as_u64()).expect("will succeed");
+        let d2 = Decimal::try_from(1_000_000f64).expect("will succeed");
         let precision = f.precision().unwrap_or(6);
-        write!(f, "{1:.*} T", precision, self.0.as_u64() as f64 / 1_000_000f64)
+        write!(f, "{1:.*} T", precision, d1 / d2)
     }
 }
 
@@ -505,4 +523,43 @@ mod test {
         );
         assert_eq!(s, "99.100000 T");
     }
+
+    #[test]
+    fn to_string_from_string_max_conversion() {
+        let max_value = MicroMinotari(u64::MAX);
+
+        assert_eq!(max_value.as_u64().to_string(), "18446744073709551615");
+        let max_str_with_currency = format!("{}", max_value);
+        assert_eq!(&max_str_with_currency, "18446744073709.551615 T");
+        let max_str_no_currency = max_str_with_currency[0..max_str_with_currency.len() - 2].to_string();
+        assert_eq!(&max_str_no_currency, "18446744073709.551615");
+
+        assert_eq!(max_value, MicroMinotari::from_str(&max_str_with_currency).unwrap());
+        assert_eq!(max_value, MicroMinotari::from_str(&max_str_no_currency).unwrap());
+        assert_eq!(
+            Minotari::from(max_value),
+            Minotari::from_str(&max_str_with_currency).unwrap()
+        );
+        assert_eq!(
+            Minotari::from(max_value),
+            Minotari::from_str(&max_str_no_currency).unwrap()
+        );
+
+        assert!(MicroMinotari::from_str("18446744073709.551615 T").is_ok());
+        assert!(MicroMinotari::from_str("18446744073709.551615 uT").is_err());
+        assert!(MicroMinotari::from_str("18446744073709.551615T").is_ok());
+        assert!(MicroMinotari::from_str("18446744073709.551615uT").is_err());
+        assert!(MicroMinotari::from_str("18446744073709.551615").is_ok());
+        assert!(MicroMinotari::from_str("18446744073709551615").is_ok());
+
+        assert!(Minotari::from_str("18446744073709.551615 T").is_ok());
+        assert!(Minotari::from_str("18446744073709.551615 uT").is_err());
+        assert!(Minotari::from_str("18446744073709.551615T").is_ok());
+        assert!(Minotari::from_str("18446744073709.551615uT").is_err());
+        assert!(Minotari::from_str("18446744073709.551615").is_ok());
+        assert_eq!(
+            &Minotari::from_str("18446744073709551615").unwrap_err().to_string(),
+            "Failed to convert value: numeric overflow"
+        );
+    }
 }

base_layer/core/src/transactions/test_helpers.rs

@@ -65,7 +65,7 @@ use crate::{
             WalletOutput,
             WalletOutputBuilder,
         },
-        transaction_protocol::TransactionMetadata,
+        transaction_protocol::{transaction_initializer::SenderTransactionInitializer, TransactionMetadata},
         weight::TransactionWeight,
         SenderTransactionProtocol,
     },
@@ -651,6 +651,22 @@ pub async fn create_stx_protocol(
     schema: TransactionSchema,
     key_manager: &TestKeyManager,
 ) -> (SenderTransactionProtocol, Vec<WalletOutput>) {
+    let mut outputs = Vec::with_capacity(schema.to.len());
+    let stx_builder = create_stx_protocol_internal(schema, key_manager, &mut outputs).await;
+
+    let stx_protocol = stx_builder.build().await.unwrap();
+    let change_output = stx_protocol.get_change_output().unwrap().unwrap();
+
+    outputs.push(change_output);
+    (stx_protocol, outputs)
+}
+
+#[allow(clippy::too_many_lines)]
+pub async fn create_stx_protocol_internal(
+    schema: TransactionSchema,
+    key_manager: &TestKeyManager,
+    outputs: &mut Vec<WalletOutput>,
+) -> SenderTransactionInitializer<TestKeyManager> {
     let constants = ConsensusManager::builder(Network::LocalNet)
         .build()
         .unwrap()
@@ -676,7 +692,6 @@ pub async fn create_stx_protocol(
     for tx_input in &schema.from {
         stx_builder.with_input(tx_input.clone()).await.unwrap();
     }
-    let mut outputs = Vec::with_capacity(schema.to.len());
     for val in schema.to {
         let (spending_key, _) = key_manager
             .get_next_key(TransactionKeyManagerBranch::CommitmentMask.get_branch_key())
@@ -741,11 +756,7 @@ pub async fn create_stx_protocol(
         stx_builder.with_output(utxo, sender_offset_key_id).await.unwrap();
     }
 
-    let stx_protocol = stx_builder.build().await.unwrap();
-    let change_output = stx_protocol.get_change_output().unwrap().unwrap();
-
-    outputs.push(change_output);
-    (stx_protocol, outputs)
+    stx_builder
 }
 
 pub async fn create_coinbase_kernel(spending_key_id: &TariKeyId, key_manager: &TestKeyManager) -> TransactionKernel {

base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs

@@ -294,12 +294,20 @@ where KM: TransactionKeyManagerInterface
         // The number of outputs excluding a possible residual change output
         let num_outputs = self.sender_custom_outputs.len() + usize::from(self.recipient.is_some());
         let num_inputs = self.inputs.len();
-        let total_being_spent = self.inputs.iter().map(|i| i.output.value).sum::<MicroMinotari>();
+        let total_being_spent = self
+            .inputs
+            .iter()
+            .map(|i| i.output.value)
+            .fold(Ok(MicroMinotari::zero()), |acc, x| {
+                acc?.checked_add(x).ok_or("Total inputs being spent amount overflow")
+            })?;
         let total_to_self = self
             .sender_custom_outputs
             .iter()
             .map(|o| o.output.value)
-            .sum::<MicroMinotari>();
+            .fold(Ok(MicroMinotari::zero()), |acc, x| {
+                acc?.checked_add(x).ok_or("Total outputs to self amount overflow")
+            })?;
         let total_amount = match &self.recipient {
             Some(data) => data.amount,
             None => 0.into(),
@@ -332,19 +340,23 @@ where KM: TransactionKeyManagerInterface
             .weighting()
             .round_up_features_and_scripts_size(change_features_and_scripts_size);
 
-        let change_fee = self
-            .fee()
-            .calculate(fee_per_gram, 0, 0, 1, change_features_and_scripts_size);
         // Subtract with a check on going negative
-        let total_input_value = total_to_self + total_amount + fee_without_change;
+        let total_input_value = [total_to_self, total_amount, fee_without_change]
+            .iter()
+            .fold(Ok(MicroMinotari::zero()), |acc, x| {
+                acc?.checked_add(x).ok_or("Total input value overflow")
+            })?;
         let change_amount = total_being_spent.checked_sub(total_input_value);
         match change_amount {
             None => Err(format!(
-                "You are spending ({}) more than you're providing ({}).",
-                total_input_value, total_being_spent
+                "You are spending more than you're providing: provided {}, required {}.",
+                total_being_spent, total_input_value
             )),
             Some(MicroMinotari(0)) => Ok((fee_without_change, MicroMinotari(0), None)),
             Some(v) => {
+                let change_fee = self
+                    .fee()
+                    .calculate(fee_per_gram, 0, 0, 1, change_features_and_scripts_size);
                 let change_amount = v.checked_sub(change_fee);
                 match change_amount {
                     // You can't win. Just add the change to the fee (which is less than the cost of adding another
@@ -909,7 +921,7 @@ mod test {
         let err = builder.build().await.unwrap_err();
         assert_eq!(
             err.message,
-            "You are spending (528 µT) more than you're providing (400 µT)."
+            "You are spending more than you're providing: provided 400 µT, required 528 µT."
         );
     }