Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ban unsafe arithmetic operations #485

Merged
merged 2 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
6 changes: 4 additions & 2 deletions node/src/chain_spec/finney.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions node/src/chain_spec/testnet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions pallets/admin-utils/src/benchmarking.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Benchmarking setup
#![cfg(feature = "runtime-benchmarks")]
#![allow(clippy::arithmetic_side_effects)]
use super::*;

#[allow(unused)]
Expand Down
2 changes: 2 additions & 0 deletions pallets/admin-utils/tests/mock.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![allow(clippy::arithmetic_side_effects, clippy::unwrap_used)]

use frame_support::{
assert_ok, derive_impl, parameter_types,
traits::{Everything, Hooks},
Expand Down
3 changes: 2 additions & 1 deletion pallets/collective/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.")
Expand Down
29 changes: 18 additions & 11 deletions pallets/collective/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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) =
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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
orriin marked this conversation as resolved.
Show resolved Hide resolved
});
num_proposals as u32
}
Expand Down Expand Up @@ -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)),
})
}
Expand All @@ -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)),
})
}
Expand Down
2 changes: 1 addition & 1 deletion pallets/collective/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 6 additions & 1 deletion pallets/commitments/src/benchmarking.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Benchmarking setup
#![cfg(feature = "runtime-benchmarks")]
#![allow(clippy::arithmetic_side_effects)]
use super::*;

#[allow(unused)]
Expand All @@ -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(),
Expand Down
13 changes: 7 additions & 6 deletions pallets/commitments/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,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> =
Expand Down Expand Up @@ -136,12 +136,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;
Expand All @@ -156,12 +156,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());
}

Expand Down
7 changes: 4 additions & 3 deletions pallets/commitments/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ impl Decode for Data {
Ok(match b {
0 => Data::None,
n @ 1..=129 => {
let mut r: BoundedVec<_, _> = vec![0u8; n as usize - 1]
let mut r: BoundedVec<_, _> = vec![0u8; (n as usize).saturating_sub(1)]
.try_into()
.expect("bound checked in match arm condition; qed");
input.read(&mut r[..])?;
Expand All @@ -86,8 +86,8 @@ impl Encode for Data {
match self {
Data::None => vec![0u8; 1],
Data::Raw(ref x) => {
let l = x.len().min(128);
let mut r = vec![l as u8 + 1];
let l = x.len().min(128) as u8;
let mut r = vec![l.saturating_add(1)];
r.extend_from_slice(&x[..]);
r
}
Expand Down Expand Up @@ -344,6 +344,7 @@ impl<
}

#[cfg(test)]
#[allow(clippy::indexing_slicing, clippy::unwrap_used)]
mod tests {
use super::*;

Expand Down
7 changes: 6 additions & 1 deletion pallets/registry/src/benchmarking.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Benchmarking setup
#![cfg(feature = "runtime-benchmarks")]
#![allow(clippy::arithmetic_side_effects, clippy::unwrap_used)]
use super::*;

#[allow(unused)]
Expand All @@ -19,7 +20,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) -> IdentityInfo<T::MaxAdditionalFields> {
let data = Data::Raw(vec![0; 32].try_into().unwrap());
let data = Data::Raw(
vec![0; 32]
.try_into()
.expect("size does not exceed 64; qed"),
);

IdentityInfo {
additional: Default::default(),
Expand Down
15 changes: 8 additions & 7 deletions pallets/registry/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use frame_support::traits::tokens::{
fungible::{self, MutateHold as _},
Precision,
};
use sp_runtime::traits::Zero;
use sp_runtime::{traits::Zero, Saturating};
use sp_std::boxed::Box;

type BalanceOf<T> =
Expand Down Expand Up @@ -132,7 +132,7 @@ pub mod pallet {
Error::<T>::TooManyFieldsInIdentityInfo
);

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 <IdentityOf<T>>::get(&identified) {
Some(mut id) => {
id.info = *info;
Expand All @@ -145,23 +145,24 @@ 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::hold(
&HoldReason::RegistryIdentity.into(),
&who,
id.deposit - old_deposit,
id.deposit.saturating_sub(old_deposit),
)?;
}
if old_deposit > id.deposit {
let release_res = T::Currency::release(
&HoldReason::RegistryIdentity.into(),
&who,
old_deposit - id.deposit,
old_deposit.saturating_sub(id.deposit),
Precision::BestEffort,
);
debug_assert!(release_res
.is_ok_and(|released_amount| released_amount == (old_deposit - id.deposit)));
debug_assert!(release_res.is_ok_and(
|released_amount| released_amount == old_deposit.saturating_sub(id.deposit)
));
}

<IdentityOf<T>>::insert(&identified, id);
Expand Down
7 changes: 4 additions & 3 deletions pallets/registry/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ impl Decode for Data {
Ok(match b {
0 => Data::None,
n @ 1..=65 => {
let mut r: BoundedVec<_, _> = vec![0u8; n as usize - 1]
let mut r: BoundedVec<_, _> = vec![0u8; (n as usize).saturating_sub(1)]
.try_into()
.expect("bound checked in match arm condition; qed");
input.read(&mut r[..])?;
Expand All @@ -87,8 +87,8 @@ impl Encode for Data {
match self {
Data::None => vec![0u8; 1],
Data::Raw(ref x) => {
let l = x.len().min(64);
let mut r = vec![l as u8 + 1];
let l = x.len().min(64) as u8;
let mut r = vec![l.saturating_add(1)];
r.extend_from_slice(&x[..]);
r
}
Expand Down Expand Up @@ -403,6 +403,7 @@ impl<
}

#[cfg(test)]
#[allow(clippy::indexing_slicing, clippy::unwrap_used)]
mod tests {
use super::*;

Expand Down
2 changes: 1 addition & 1 deletion pallets/subtensor/src/benchmarks.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//! Subtensor pallet benchmarking.

#![allow(clippy::arithmetic_side_effects, clippy::unwrap_used)]
#![cfg(feature = "runtime-benchmarks")]

use crate::Pallet as Subtensor;
Expand Down
Loading
Loading