Merge pull request #529 from opentensor/ban-unsafe-arithmetic-devnet

(Devnet Ready) Ban unsafe arithmetic operations

Author: sam0x17

Cargo.toml

@@ -8,7 +8,10 @@ members = [
 resolver = "2"
 
 [workspace.lints.clippy]
+indexing-slicing = "deny"
+arithmetic-side-effects = "deny"
 type_complexity = "allow"
+unwrap-used = "deny"
 
 [workspace.dependencies]
 cargo-husky = { version = "1", default-features = false }

node/src/chain_spec/finney.rs

@@ -5,7 +5,7 @@ use super::*;
 
 pub fn finney_mainnet_config() -> Result<ChainSpec, String> {
     let path: PathBuf = std::path::PathBuf::from("./snapshot.json");
-    let wasm_binary = WASM_BINARY.ok_or_else(|| "Development wasm not available".to_string())?;
+    let wasm_binary = WASM_BINARY.ok_or("Development wasm not available".to_string())?;
 
     // We mmap the file into memory first, as this is *a lot* faster than using
     // `serde_json::from_reader`. See https://github.com/serde-rs/json/issues/160
@@ -53,7 +53,9 @@ pub fn finney_mainnet_config() -> Result<ChainSpec, String> {
         let key_account = sp_runtime::AccountId32::from(key);
 
         processed_balances.push((key_account, *amount));
-        balances_issuance += *amount;
+        balances_issuance = balances_issuance
+            .checked_add(*amount)
+            .ok_or("Balances issuance overflowed".to_string())?;
     }
 
     // Give front-ends necessary data to present to users

node/src/chain_spec/testnet.rs

@@ -20,7 +20,7 @@ pub fn finney_testnet_config() -> Result<ChainSpec, String> {
     };
 
     let old_state: ColdkeyHotkeys =
-        json::from_slice(&bytes).map_err(|e| format!("Error parsing genesis file: {}", e))?;
+        json::from_slice(&bytes).map_err(|e| format!("Error parsing genesis file: {e}"))?;
 
     let mut processed_stakes: Vec<(
         sp_runtime::AccountId32,
@@ -53,7 +53,9 @@ pub fn finney_testnet_config() -> Result<ChainSpec, String> {
         let key_account = sp_runtime::AccountId32::from(key);
 
         processed_balances.push((key_account, *amount));
-        balances_issuance += *amount;
+        balances_issuance = balances_issuance
+            .checked_add(*amount)
+            .ok_or("Balances issuance overflowed".to_string())?;
     }
 
     // Give front-ends necessary data to present to users

pallets/admin-utils/src/benchmarking.rs

@@ -1,5 +1,6 @@
 //! Benchmarking setup
 #![cfg(feature = "runtime-benchmarks")]
+#![allow(clippy::arithmetic_side_effects)]
 use super::*;
 
 #[allow(unused)]

pallets/admin-utils/tests/mock.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::arithmetic_side_effects, clippy::unwrap_used)]
+
 use frame_support::{
     assert_ok, derive_impl, parameter_types,
     traits::{Everything, Hooks},

pallets/collective/src/benchmarking.rs

@@ -16,6 +16,7 @@
 // limitations under the License.
 
 //! Staking pallet benchmarking.
+#![allow(clippy::arithmetic_side_effects, clippy::indexing_slicing)]
 
 use super::*;
 use crate::Pallet as Collective;
@@ -70,7 +71,7 @@ benchmarks_instance_pallet! {
                 // Proposals should be different so that different proposal hashes are generated
                 let proposal: T::Proposal = SystemCall::<T>::remark { remark: id_to_remark_data(i, length) }.into();
                 Collective::<T, I>::propose(
-                    SystemOrigin::Signed(old_members.last().unwrap().clone()).into(),
+                    SystemOrigin::Signed(old_members.last().expect("m is greater than 0; old_members must have at least 1 element; qed").clone()).into(),
                     Box::new(proposal.clone()),
                     MAX_BYTES,
                     TryInto::<BlockNumberFor<T>>::try_into(3u64).ok().expect("convert u64 to block number.")

pallets/collective/src/lib.rs

@@ -54,7 +54,7 @@ use frame_support::{
 use scale_info::TypeInfo;
 use sp_io::storage;
 use sp_runtime::traits::Dispatchable;
-use sp_runtime::{traits::Hash, RuntimeDebug};
+use sp_runtime::{traits::Hash, RuntimeDebug, Saturating};
 use sp_std::{marker::PhantomData, prelude::*, result};
 
 #[cfg(test)]
@@ -119,7 +119,7 @@ impl DefaultVote for MoreThanMajorityThenPrimeDefaultVote {
         _no_votes: MemberCount,
         len: MemberCount,
     ) -> bool {
-        let more_than_majority = yes_votes * 2 > len;
+        let more_than_majority = yes_votes.saturating_mul(2) > len;
         more_than_majority || prime_vote.unwrap_or(false)
     }
 }
@@ -545,7 +545,9 @@ pub mod pallet {
                 Error::<T, I>::DurationLowerThanConfiguredMotionDuration
             );
 
-            let threshold = (T::GetVotingMembers::get_count() / 2) + 1;
+            let threshold = T::GetVotingMembers::get_count()
+                .saturating_div(2)
+                .saturating_add(1);
 
             let members = Self::members();
             let (proposal_len, active_proposals) =
@@ -716,10 +718,15 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
             })?;
 
         let index = Self::proposal_count();
-        <ProposalCount<T, I>>::mutate(|i| *i += 1);
+        <ProposalCount<T, I>>::try_mutate(|i| {
+            *i = i
+                .checked_add(1)
+                .ok_or(Error::<T, I>::TooManyActiveProposals)?;
+            Ok::<(), Error<T, I>>(())
+        })?;
         <ProposalOf<T, I>>::insert(proposal_hash, proposal);
         let votes = {
-            let end = frame_system::Pallet::<T>::block_number() + duration;
+            let end = frame_system::Pallet::<T>::block_number().saturating_add(duration);
             Votes {
                 index,
                 threshold,
@@ -862,10 +869,10 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
         // default voting strategy.
         let default = T::DefaultVote::default_vote(prime_vote, yes_votes, no_votes, seats);
 
-        let abstentions = seats - (yes_votes + no_votes);
+        let abstentions = seats.saturating_sub(yes_votes.saturating_add(no_votes));
         match default {
-            true => yes_votes += abstentions,
-            false => no_votes += abstentions,
+            true => yes_votes = yes_votes.saturating_add(abstentions),
+            false => no_votes = no_votes.saturating_add(abstentions),
         }
         let approved = yes_votes >= voting.threshold;
 
@@ -981,7 +988,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
         Voting::<T, I>::remove(proposal_hash);
         let num_proposals = Proposals::<T, I>::mutate(|proposals| {
             proposals.retain(|h| h != &proposal_hash);
-            proposals.len() + 1 // calculate weight based on original length
+            proposals.len().saturating_add(1) // calculate weight based on original length
         });
         num_proposals as u32
     }
@@ -1154,7 +1161,7 @@ impl<
     type Success = ();
     fn try_origin(o: O) -> Result<Self::Success, O> {
         o.into().and_then(|o| match o {
-            RawOrigin::Members(n, m) if n * D > N * m => Ok(()),
+            RawOrigin::Members(n, m) if n.saturating_mul(D) > N.saturating_mul(m) => Ok(()),
             r => Err(O::from(r)),
         })
     }
@@ -1179,7 +1186,7 @@ impl<
     type Success = ();
     fn try_origin(o: O) -> Result<Self::Success, O> {
         o.into().and_then(|o| match o {
-            RawOrigin::Members(n, m) if n * D >= N * m => Ok(()),
+            RawOrigin::Members(n, m) if n.saturating_mul(D) >= N.saturating_mul(m) => Ok(()),
             r => Err(O::from(r)),
         })
     }

pallets/collective/src/tests.rs

@@ -15,7 +15,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#![allow(non_camel_case_types)]
+#![allow(non_camel_case_types, clippy::indexing_slicing, clippy::unwrap_used)]
 
 use super::{Event as CollectiveEvent, *};
 use crate as pallet_collective;

pallets/commitments/src/benchmarking.rs

@@ -1,5 +1,6 @@
 //! Benchmarking setup
 #![cfg(feature = "runtime-benchmarks")]
+#![allow(clippy::arithmetic_side_effects)]
 use super::*;
 
 #[allow(unused)]
@@ -17,7 +18,11 @@ fn assert_last_event<T: Config>(generic_event: <T as Config>::RuntimeEvent) {
 // This creates an `IdentityInfo` object with `num_fields` extra fields.
 // All data is pre-populated with some arbitrary bytes.
 fn create_identity_info<T: Config>(_num_fields: u32) -> CommitmentInfo<T::MaxFields> {
-    let _data = Data::Raw(vec![0; 32].try_into().unwrap());
+    let _data = Data::Raw(
+        vec![0; 32]
+            .try_into()
+            .expect("vec length is less than 64; qed"),
+    );
 
     CommitmentInfo {
         fields: Default::default(),

pallets/commitments/src/lib.rs

@@ -14,7 +14,7 @@ pub use types::*;
 pub use weights::WeightInfo;
 
 use frame_support::traits::Currency;
-use sp_runtime::traits::Zero;
+use sp_runtime::{traits::Zero, Saturating};
 use sp_std::boxed::Box;
 
 type BalanceOf<T> =
@@ -138,12 +138,12 @@ pub mod pallet {
             let cur_block = <frame_system::Pallet<T>>::block_number();
             if let Some(last_commit) = <LastCommitment<T>>::get(netuid, &who) {
                 ensure!(
-                    cur_block >= last_commit + T::RateLimit::get(),
+                    cur_block >= last_commit.saturating_add(T::RateLimit::get()),
                     Error::<T>::CommitmentSetRateLimitExceeded
                 );
             }
 
-            let fd = <BalanceOf<T>>::from(extra_fields) * T::FieldDeposit::get();
+            let fd = <BalanceOf<T>>::from(extra_fields).saturating_mul(T::FieldDeposit::get());
             let mut id = match <CommitmentOf<T>>::get(netuid, &who) {
                 Some(mut id) => {
                     id.info = *info;
@@ -158,12 +158,13 @@ pub mod pallet {
             };
 
             let old_deposit = id.deposit;
-            id.deposit = T::InitialDeposit::get() + fd;
+            id.deposit = T::InitialDeposit::get().saturating_add(fd);
             if id.deposit > old_deposit {
-                T::Currency::reserve(&who, id.deposit - old_deposit)?;
+                T::Currency::reserve(&who, id.deposit.saturating_sub(old_deposit))?;
             }
             if old_deposit > id.deposit {
-                let err_amount = T::Currency::unreserve(&who, old_deposit - id.deposit);
+                let err_amount =
+                    T::Currency::unreserve(&who, old_deposit.saturating_sub(id.deposit));
                 debug_assert!(err_amount.is_zero());
             }