feat: add overflow checks to change and fee calculations (#5834)

Description
---
- Added 3x overflow checks to the change calculation in the sender
transaction protocol.
- Added an additional unit test (`fn
test_sender_transaction_protocol_for_overflow`) to verify three sender
transaction protocol overflow errors are handled.
- **Edit:** Added overflow checks to kernel fee calculation in blocks.
- **Edit:** Added overflow checks to transaction fee calculation.
- **Edit:** Added an additional unit test (`fn test_fee_overflow`) to
verify the two fee overflow errors are handled.
- **Edit:** Fixed `Minotari` and `MicroMinotari` string conversion issue
(#5839) for big numbers (_with unit test_) to allow for the proper error
message to be printed in the `fn test_fee_overflow` unit test.

Motivation and Context
---
It is possible to crash the system in the change calculation before the
transaction is validated; this PR prevents it.
It is possible to crash the system with carefully crafted fees added to
transactions and blocks; this PR prevents it.

How Has This Been Tested?
---
Added additional unit tests.

What process can a PR reviewer use to test or verify this change?
---
See unit tests and code changes.

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->

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."
         );
     }