Handling overflows during arithmetic operations (#1466)

Closes #1335

Handling overflows during arithmetic operations by denying
`clippy::arithmetic_side_effects`.

Also, it is the last issue that we plan to fix right now in the scope of
the ToB audit report. So this PR closes
FuelLabs/fuel-vm#513.

---------

Co-authored-by: Brandon Kite <brandonkite92@gmail.com>

CHANGELOG.md

@@ -42,6 +42,7 @@ Description of the upcoming release here.
 
 ### Changed
 
+- [#1466](https://github.com/FuelLabs/fuel-core/pull/1466): Handling overflows during arithmetic operations.
 - [#1468](https://github.com/FuelLabs/fuel-core/pull/1468): Bumped version of the `fuel-vm` to `v0.40.0`. It brings some breaking changes into consensus parameters API because of changes in the underlying types.
 - [#1460](https://github.com/FuelLabs/fuel-core/pull/1460): Change tracking branch from main to master for releasy tests.
 - [#1440](https://github.com/FuelLabs/fuel-core/pull/1440): Don't report reserved nodes that send invalid transactions.

bin/fuel-core-client/src/main.rs

@@ -1,3 +1,5 @@
+#![deny(clippy::arithmetic_side_effects)]
+#![deny(clippy::cast_possible_truncation)]
 #![deny(unused_crate_dependencies)]
 #![deny(warnings)]
 

bin/fuel-core/src/main.rs

@@ -1,3 +1,5 @@
+#![deny(clippy::arithmetic_side_effects)]
+#![deny(clippy::cast_possible_truncation)]
 #![deny(unused_crate_dependencies)]
 #![deny(warnings)]
 

crates/chain-config/src/lib.rs

@@ -1,4 +1,5 @@
 #![deny(clippy::cast_possible_truncation)]
+#![deny(clippy::arithmetic_side_effects)]
 #![deny(unused_crate_dependencies)]
 #![deny(warnings)]
 

crates/chain-config/src/serialization.rs

@@ -140,21 +140,17 @@ macro_rules! impl_hex_number {
             {
                 const SIZE: usize = core::mem::size_of::<$i>();
                 let mut bytes: Vec<u8> = serde_hex::deserialize(deserializer)?;
-                match bytes.len() {
-                    len if len > SIZE => {
-                        return Err(D::Error::custom(format!(
+                let pad =
+                    SIZE.checked_sub(bytes.len())
+                        .ok_or(D::Error::custom(format!(
                             "value cant exceed {WORD_SIZE} bytes"
-                        )))
-                    }
-                    len if len < SIZE => {
-                        // pad if length < word size
-                        bytes = (0..SIZE - len)
-                            .map(|_| 0u8)
-                            .chain(bytes.into_iter())
-                            .collect();
-                    }
-                    _ => {}
+                        )))?;
+
+                if pad != 0 {
+                    // pad if length < word size
+                    bytes = (0..pad).map(|_| 0u8).chain(bytes.into_iter()).collect();
                 }
+
                 // We've already verified the bytes.len == WORD_SIZE, force the conversion here.
                 Ok($i::from_be_bytes(
                     bytes.try_into().expect("byte lengths checked"),

crates/client/src/lib.rs

@@ -1,3 +1,4 @@
+#![deny(clippy::arithmetic_side_effects)]
 #![deny(clippy::cast_possible_truncation)]
 #![deny(unused_crate_dependencies)]
 #![deny(warnings)]

crates/database/src/lib.rs

@@ -4,6 +4,7 @@
 //! defined here are used by services but are flexible enough to customize the
 //! logic when the `Database` is known.
 
+#![deny(clippy::arithmetic_side_effects)]
 #![deny(clippy::cast_possible_truncation)]
 #![deny(missing_docs)]
 #![deny(unused_crate_dependencies)]

crates/fuel-core/src/coins_query.rs

@@ -185,9 +185,12 @@ pub fn random_improve(
                 }
 
                 // Break if adding doesn't improve the distance
-                let change_amount = collected_amount - target;
+                let change_amount = collected_amount
+                    .checked_sub(target)
+                    .expect("We checked it above");
                 let distance = target.abs_diff(change_amount);
-                let next_distance = target.abs_diff(change_amount + coin.amount());
+                let next_distance =
+                    target.abs_diff(change_amount.saturating_add(coin.amount()));
                 if next_distance >= distance {
                     break
                 }
@@ -215,6 +218,7 @@ impl From<StorageError> for CoinsQueryError {
     }
 }
 
+#[allow(clippy::arithmetic_side_effects)]
 #[cfg(test)]
 mod tests {
     use crate::{

crates/fuel-core/src/database/block.rs

@@ -275,7 +275,10 @@ impl Database {
             MerkleTree::load(storage, commit_merkle_metadata.version)
                 .map_err(|err| StorageError::Other(anyhow::anyhow!(err)))?;
 
-        let proof_index = message_merkle_metadata.version - 1;
+        let proof_index = message_merkle_metadata
+            .version
+            .checked_sub(1)
+            .ok_or(anyhow::anyhow!("The count of leafs - messages is zero"))?;
         let (_, proof_set) = tree
             .prove(proof_index)
             .map_err(|err| StorageError::Other(anyhow::anyhow!(err)))?;
@@ -287,6 +290,7 @@ impl Database {
     }
 }
 
+#[allow(clippy::arithmetic_side_effects)]
 #[cfg(test)]
 mod tests {
     use super::*;

crates/fuel-core/src/database/vm_database.rs

@@ -610,7 +610,13 @@ mod tests {
         // check stored data
         let results: Vec<_> = (0..remove_count)
             .filter_map(|i| {
-                let current_key = U256::from_big_endian(&start_key) + i;
+                let (current_key, overflow) =
+                    U256::from_big_endian(&start_key).overflowing_add(i.into());
+
+                if overflow {
+                    return None
+                }
+
                 let current_key = u256_to_bytes32(current_key);
                 let result = db
                     .merkle_contract_state(&contract_id, &current_key)

crates/fuel-core/src/executor.rs

@@ -1780,6 +1780,7 @@ impl Fee for CreateCheckedMetadata {
     }
 }
 
+#[allow(clippy::arithmetic_side_effects)]
 #[allow(clippy::cast_possible_truncation)]
 #[cfg(test)]
 mod tests {

crates/fuel-core/src/lib.rs

@@ -1,3 +1,4 @@
+#![deny(clippy::arithmetic_side_effects)]
 #![deny(clippy::cast_possible_truncation)]
 #![deny(unused_crate_dependencies)]
 #![deny(warnings)]

crates/fuel-core/src/query/balance.rs

@@ -63,7 +63,7 @@ impl BalanceQueryData for Database {
             let amount = res?;
 
             // Increase the balance
-            balance += amount;
+            balance = balance.saturating_add(amount);
 
             Ok(balance)
         })?;
@@ -87,9 +87,10 @@ impl BalanceQueryData for Database {
         for coin in AssetsQuery::new(&owner, None, None, self, &base_asset_id).coins() {
             match coin {
                 Ok(coin) => {
-                    *amounts_per_asset
+                    let amount: &mut u64 = amounts_per_asset
                         .entry(*coin.asset_id(&base_asset_id))
-                        .or_default() += coin.amount();
+                        .or_default();
+                    *amount = amount.saturating_add(coin.amount());
                 }
                 Err(err) => {
                     errors.push(err);

crates/fuel-core/src/schema.rs

@@ -154,7 +154,7 @@ where
                 false
             });
 
-            let mut count = count + 1 /* for `has_next_page` */;
+            let mut count = count.saturating_add(1) /* for `has_next_page` */;
             let entries = entries.take(count).take_while(|result| {
                 if let Ok((key, _)) = result {
                     if let Some(end) = end.as_ref() {
@@ -164,7 +164,7 @@ where
                             return false
                         }
                     }
-                    count -= 1;
+                    count = count.saturating_sub(1);
                     has_next_page |= count == 0;
                     count != 0
                 } else {

crates/fuel-core/src/service/genesis.rs

@@ -176,7 +176,9 @@ fn init_coin_state(
                         .expect("Incorrect genesis transaction id byte length")
                     }),
                     coin.output_index.unwrap_or_else(|| {
-                        generated_output_index += 1;
+                        generated_output_index = generated_output_index
+                            .checked_add(1)
+                            .expect("The maximum number of UTXOs supported in the genesis configuration has been exceeded.");
                         (generated_output_index % 255) as u8
                     }),
                 );

crates/fuel-core/src/state/rocks_db.rs

@@ -159,9 +159,9 @@ impl RocksDb {
                     let key_as_vec = Vec::from(key);
 
                     database_metrics().read_meter.inc();
-                    database_metrics()
-                        .bytes_read
-                        .observe((key_as_vec.len() + value_as_vec.len()) as f64);
+                    database_metrics().bytes_read.observe(
+                        (key_as_vec.len().saturating_add(value_as_vec.len())) as f64,
+                    );
 
                     (key_as_vec, Arc::new(value_as_vec))
                 })

crates/keygen/src/lib.rs

@@ -1,3 +1,4 @@
+#![deny(clippy::arithmetic_side_effects)]
 #![deny(clippy::cast_possible_truncation)]
 
 use clap::ValueEnum;

crates/metrics/src/future_tracker.rs

@@ -45,7 +45,7 @@ impl<'a> Entered<'a> {
 impl<'a> Drop for Entered<'a> {
     #[inline(always)]
     fn drop(&mut self) {
-        self.span.busy += self.busy_instant.elapsed();
+        self.span.busy = self.span.busy.saturating_add(self.busy_instant.elapsed());
         self.span.do_exit()
     }
 }
@@ -77,7 +77,7 @@ impl Span {
         let idle_instant = core::mem::take(&mut self.idle_instant);
 
         if let Some(idle_instant) = idle_instant {
-            self.idle += idle_instant.elapsed();
+            self.idle = self.idle.saturating_add(idle_instant.elapsed());
         }
     }
 

crates/metrics/src/lib.rs

@@ -1,3 +1,4 @@
+#![deny(clippy::arithmetic_side_effects)]
 #![deny(clippy::cast_possible_truncation)]
 #![deny(unused_crate_dependencies)]
 #![deny(warnings)]

crates/services/consensus_module/bft/src/lib.rs

@@ -1,3 +1,4 @@
+#![deny(clippy::arithmetic_side_effects)]
 #![deny(clippy::cast_possible_truncation)]
 #![deny(unused_crate_dependencies)]
 #![deny(warnings)]

crates/services/consensus_module/poa/src/deadline_clock.rs

@@ -131,7 +131,13 @@ impl DeadlineClock {
 
     /// Sets the timeout, optionally overwriting the existing value
     pub async fn set_timeout(&self, after: Duration, on_conflict: OnConflict) {
-        self.set_deadline(Instant::now() + after, on_conflict).await;
+        self.set_deadline(
+            Instant::now()
+                .checked_add(after)
+                .expect("Setting timeout after many years doesn't make a lot of sense"),
+            on_conflict,
+        )
+        .await;
     }
 
     /// Clears the timeout, so that now event is produced when it expires.

crates/services/consensus_module/poa/src/lib.rs

@@ -1,3 +1,4 @@
+#![deny(clippy::arithmetic_side_effects)]
 #![deny(clippy::cast_possible_truncation)]
 #![deny(unused_crate_dependencies)]
 #![deny(unused_must_use)]

crates/services/consensus_module/poa/src/service.rs

@@ -201,7 +201,9 @@ where
         let last_timestamp = last_block.time();
         let duration =
             Duration::from_secs(Tai64::now().0.saturating_sub(last_timestamp.0));
-        let last_block_created = Instant::now() - duration;
+        let last_block_created = Instant::now()
+            .checked_sub(duration)
+            .unwrap_or(Instant::now());
         let last_height = *last_block.height();
         (last_height, last_timestamp, last_block_created)
     }
@@ -340,13 +342,13 @@ where
             }
             (Trigger::Instant, _) => {}
             (Trigger::Interval { block_time }, RequestType::Trigger) => {
-                self.timer
-                    .set_deadline(last_block_created + block_time, OnConflict::Min)
-                    .await;
+                let deadline = last_block_created.checked_add(block_time).expect("It is impossible to overflow except in the case where we don't want to produce a block.");
+                self.timer.set_deadline(deadline, OnConflict::Min).await;
             }
             (Trigger::Interval { block_time }, RequestType::Manual) => {
+                let deadline = last_block_created.checked_add(block_time).expect("It is impossible to overflow except in the case where we don't want to produce a block.");
                 self.timer
-                    .set_deadline(last_block_created + block_time, OnConflict::Overwrite)
+                    .set_deadline(deadline, OnConflict::Overwrite)
                     .await;
             }
         }

crates/services/consensus_module/poa/src/service_test.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::arithmetic_side_effects)]
+
 use crate::{
     new_service,
     ports::{

crates/services/consensus_module/poa/src/sync.rs

@@ -264,6 +264,7 @@ impl InnerSyncState {
     }
 }
 
+#[allow(clippy::arithmetic_side_effects)]
 #[cfg(test)]
 mod tests {
     use super::*;

crates/services/consensus_module/src/lib.rs

@@ -1,4 +1,5 @@
 //! Common traits and logic for managing the lifecycle of services
+#![deny(clippy::arithmetic_side_effects)]
 #![deny(clippy::cast_possible_truncation)]
 #![deny(unused_crate_dependencies)]
 #![deny(missing_docs)]

crates/services/executor/src/lib.rs

@@ -1,3 +1,4 @@
+#![deny(clippy::arithmetic_side_effects)]
 #![deny(clippy::cast_possible_truncation)]
 #![deny(unused_crate_dependencies)]
 #![deny(warnings)]

crates/services/importer/src/lib.rs

@@ -1,3 +1,4 @@
+#![deny(clippy::arithmetic_side_effects)]
 #![deny(clippy::cast_possible_truncation)]
 #![deny(unused_crate_dependencies)]
 #![deny(warnings)]

crates/services/p2p/src/discovery.rs

@@ -149,8 +149,10 @@ impl NetworkBehaviour for DiscoveryBehaviour {
                     Box::pin(tokio::time::sleep(self.duration_to_next_kad));
                 // duration to next random walk should either be exponentially bigger than the previous
                 // or at max 60 seconds
-                self.duration_to_next_kad =
-                    std::cmp::min(self.duration_to_next_kad * 2, SIXTY_SECONDS);
+                self.duration_to_next_kad = std::cmp::min(
+                    self.duration_to_next_kad.saturating_mul(2),
+                    SIXTY_SECONDS,
+                );
             }
         }
 

crates/services/p2p/src/gossipsub/config.rs

@@ -116,7 +116,11 @@ fn initialize_topic_score_params(topic_weight: f64) -> TopicScoreParams {
 
     // The decay time for the first message delivered score, set to 100 times the epoch duration.
     // This means that the score given for first message deliveries will decay over this time period.
-    params.first_message_deliveries_decay = score_parameter_decay(EPOCH * 100);
+    params.first_message_deliveries_decay = score_parameter_decay(
+        EPOCH
+            .checked_mul(100)
+            .expect("`EPOCH` is usually not more than a year"),
+    );
     params.first_message_deliveries_cap = 1000.0;
     params.first_message_deliveries_weight = 0.5;
 
@@ -130,7 +134,11 @@ fn initialize_topic_score_params(topic_weight: f64) -> TopicScoreParams {
     params.mesh_failure_penalty_weight = 0.0;
 
     params.invalid_message_deliveries_weight = -10.0 / params.topic_weight; // -200 per invalid message
-    params.invalid_message_deliveries_decay = score_parameter_decay(EPOCH * 50);
+    params.invalid_message_deliveries_decay = score_parameter_decay(
+        EPOCH
+            .checked_mul(50)
+            .expect("`EPOCH` is usually not more than a year"),
+    );
 
     params
 }
@@ -144,11 +152,17 @@ fn initialize_peer_score_params(thresholds: &PeerScoreThresholds) -> PeerScorePa
     let mut params = PeerScoreParams {
         decay_interval: DECAY_INTERVAL,
         decay_to_zero: DECAY_TO_ZERO,
-        retain_score: EPOCH * 100,
+        retain_score: EPOCH
+            .checked_mul(100)
+            .expect("`EPOCH` is usually not more than a year"),
         app_specific_weight: 0.0,
         ip_colocation_factor_threshold: 8.0, // Allow up to 8 nodes per IP
         behaviour_penalty_threshold: 6.0,
-        behaviour_penalty_decay: score_parameter_decay(EPOCH * 10),
+        behaviour_penalty_decay: score_parameter_decay(
+            EPOCH
+                .checked_mul(10)
+                .expect("`EPOCH` is usually not more than a year"),
+        ),
         ..Default::default()
     };