From 6fd700e54f69111874a63fa0a6c001906bf94389 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Tue, 3 Dec 2024 12:33:24 -0300 Subject: [PATCH 01/30] Print info --- apps/cargo-scout-audit/src/cli/mod.rs | 3 +++ apps/cargo-scout-audit/src/detectors/builder.rs | 7 ++++++- apps/cargo-scout-audit/src/scout/nightly_runner.rs | 8 ++++---- apps/cargo-scout-audit/src/startup.rs | 4 +++- apps/cargo-scout-audit/src/utils/print.rs | 8 ++++++++ .../assert-violation/vulnerable/vulnerable-1/src/lib.rs | 1 + 6 files changed, 25 insertions(+), 6 deletions(-) diff --git a/apps/cargo-scout-audit/src/cli/mod.rs b/apps/cargo-scout-audit/src/cli/mod.rs index c5b45d41..b7a48447 100644 --- a/apps/cargo-scout-audit/src/cli/mod.rs +++ b/apps/cargo-scout-audit/src/cli/mod.rs @@ -3,6 +3,8 @@ use clap::{Parser, Subcommand, ValueEnum}; use std::path::PathBuf; use thiserror::Error; +use crate::utils::print::print_info; + #[derive(Debug, Parser)] #[clap(display_name = "cargo")] pub struct Cli { @@ -148,6 +150,7 @@ impl Scout { } pub fn validate(&self) -> Result<()> { + print_info("Validating CLI arguments..."); if let Some(path) = &self.output_path { if path.is_dir() { bail!(CliError::OutputPathIsDirectory(path.clone())); diff --git a/apps/cargo-scout-audit/src/detectors/builder.rs b/apps/cargo-scout-audit/src/detectors/builder.rs index 4c2ff2b6..af3f2207 100644 --- a/apps/cargo-scout-audit/src/detectors/builder.rs +++ b/apps/cargo-scout-audit/src/detectors/builder.rs @@ -10,7 +10,10 @@ use super::{ library::Library, source::download_git_repo, }; -use crate::{scout::blockchain::BlockChain, utils::telemetry::TracedError}; +use crate::{ + scout::blockchain::BlockChain, + utils::{print::print_info, telemetry::TracedError}, +}; #[derive(Error, Debug)] pub enum BuilderError { @@ -61,12 +64,14 @@ impl<'a> DetectorBuilder<'a> { #[tracing::instrument(skip_all, level = "debug")] pub fn build(&self, bc: &BlockChain, used_detectors: &[String]) -> Result> { + print_info("Compiling detectors..."); let all_library_paths = self.build_all_libraries(bc)?; self.filter_detectors(&all_library_paths, used_detectors) } #[tracing::instrument(skip_all, level = "debug")] pub fn get_detector_names(&self) -> Result> { + print_info("Getting detector names..."); let mut all_names = Vec::new(); let libraries = self.get_all_libraries()?; diff --git a/apps/cargo-scout-audit/src/scout/nightly_runner.rs b/apps/cargo-scout-audit/src/scout/nightly_runner.rs index fc42edad..2affa3c4 100644 --- a/apps/cargo-scout-audit/src/scout/nightly_runner.rs +++ b/apps/cargo-scout-audit/src/scout/nightly_runner.rs @@ -1,5 +1,5 @@ -#![allow(unused_imports)] -use anyhow::{anyhow, Context, Result}; +use crate::utils::print::{print_error, print_info}; +use anyhow::{Context, Result}; use current_platform::CURRENT_PLATFORM; use lazy_static::lazy_static; use std::{ @@ -8,8 +8,6 @@ use std::{ process::{Child, Command}, }; -use crate::utils::print::print_error; - lazy_static! { static ref LIBRARY_PATH_VAR: &'static str = match env::consts::OS { "linux" => "LD_LIBRARY_PATH", @@ -40,6 +38,7 @@ pub fn run_scout_in_nightly(toolchain: &str) -> Result> { unsafe { let _ = SetDllDirectoryW(PCWSTR(directory.as_ptr())); } + print_info("Re-running scout with nightly toolchain..."); return Ok(None); } @@ -72,5 +71,6 @@ pub fn run_scout_in_nightly(toolchain: &str) -> Result> { let child = command .spawn() .with_context(|| "Failed to spawn scout with nightly toolchain")?; + print_info("Re-running scout with nightly toolchain..."); Ok(Some(child)) } diff --git a/apps/cargo-scout-audit/src/startup.rs b/apps/cargo-scout-audit/src/startup.rs index ae6209a4..cd6b4b69 100644 --- a/apps/cargo-scout-audit/src/startup.rs +++ b/apps/cargo-scout-audit/src/startup.rs @@ -18,7 +18,7 @@ use crate::{ config::{open_config_and_sync_detectors, profile_enabled_detectors}, detectors::{get_excluded_detectors, get_filtered_detectors, list_detectors}, detectors_info::get_detectors_info, - print::{print_error, print_warning}, + print::{print_error, print_info, print_warning}, }, }; use anyhow::{anyhow, bail, Context, Ok, Result}; @@ -254,6 +254,8 @@ fn run_dylint( opts: &Scout, inside_vscode: bool, ) -> Result<(bool, NamedTempFile)> { + print_info("Running scout..."); + // Convert detectors paths to string let detectors_paths: Vec = detectors_paths .iter() diff --git a/apps/cargo-scout-audit/src/utils/print.rs b/apps/cargo-scout-audit/src/utils/print.rs index ece63825..6a9e4273 100644 --- a/apps/cargo-scout-audit/src/utils/print.rs +++ b/apps/cargo-scout-audit/src/utils/print.rs @@ -8,6 +8,10 @@ pub fn print_error(message: &str) { println!("{}", pretty_error(message)); } +pub fn print_info(message: &str) { + println!("{}", pretty_info(message)); +} + pub fn pretty_warning(message: &str) -> String { format!("{} {}", "[WARNING]".yellow(), message) } @@ -15,3 +19,7 @@ pub fn pretty_warning(message: &str) -> String { pub fn pretty_error(message: &str) -> String { format!("{} {}", "[ERROR]".red(), message) } + +pub fn pretty_info(message: &str) -> String { + format!("{} {}", "[INFO]".blue(), message) +} diff --git a/test-cases/substrate-pallets/assert-violation/vulnerable/vulnerable-1/src/lib.rs b/test-cases/substrate-pallets/assert-violation/vulnerable/vulnerable-1/src/lib.rs index 084c695a..5e6ff5b3 100644 --- a/test-cases/substrate-pallets/assert-violation/vulnerable/vulnerable-1/src/lib.rs +++ b/test-cases/substrate-pallets/assert-violation/vulnerable/vulnerable-1/src/lib.rs @@ -39,6 +39,7 @@ pub mod pallet { #[pallet::call_index(0)] pub fn unsafe_check_balance(origin: OriginFor, amount: u32) -> DispatchResult { let who = ensure_signed(origin)?; + ensure_none(who); assert!( BalanceStorage::::get().unwrap_or(0) >= amount, "Insufficient balance" From 791e532d5517ea293e5f8336d3bcbade3f7275f1 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Tue, 3 Dec 2024 14:34:58 -0300 Subject: [PATCH 02/30] Add test-cases and detector --- detectors/substrate-pallets/Cargo.lock | 10 +++ .../unsigned-extrinsic/Cargo.toml | 16 ++++ .../unsigned-extrinsic/src/lib.rs | 89 +++++++++++++++++++ test-cases/substrate-pallets/Cargo.lock | 30 +++++++ .../remediated/remediated-1/Cargo.toml | 34 +++++++ .../remediated/remediated-1/src/lib.rs | 63 +++++++++++++ .../remediated/remediated-1/src/weights.rs | 23 +++++ .../vulnerable/vulnerable-1/Cargo.toml | 34 +++++++ .../vulnerable/vulnerable-1/src/lib.rs | 63 +++++++++++++ .../vulnerable/vulnerable-1/src/weights.rs | 23 +++++ 10 files changed, 385 insertions(+) create mode 100644 detectors/substrate-pallets/unsigned-extrinsic/Cargo.toml create mode 100644 detectors/substrate-pallets/unsigned-extrinsic/src/lib.rs create mode 100644 test-cases/substrate-pallets/unsigned-extrinsic/remediated/remediated-1/Cargo.toml create mode 100644 test-cases/substrate-pallets/unsigned-extrinsic/remediated/remediated-1/src/lib.rs create mode 100644 test-cases/substrate-pallets/unsigned-extrinsic/remediated/remediated-1/src/weights.rs create mode 100644 test-cases/substrate-pallets/unsigned-extrinsic/vulnerable/vulnerable-1/Cargo.toml create mode 100644 test-cases/substrate-pallets/unsigned-extrinsic/vulnerable/vulnerable-1/src/lib.rs create mode 100644 test-cases/substrate-pallets/unsigned-extrinsic/vulnerable/vulnerable-1/src/weights.rs diff --git a/detectors/substrate-pallets/Cargo.lock b/detectors/substrate-pallets/Cargo.lock index b98c6ba7..8de9c7b0 100644 --- a/detectors/substrate-pallets/Cargo.lock +++ b/detectors/substrate-pallets/Cargo.lock @@ -2846,6 +2846,16 @@ dependencies = [ "if_chain", ] +[[package]] +name = "unsigned-extrinsic" +version = "0.1.0" +dependencies = [ + "clippy_utils", + "common", + "dylint_linting", + "if_chain", +] + [[package]] name = "untrusted" version = "0.9.0" diff --git a/detectors/substrate-pallets/unsigned-extrinsic/Cargo.toml b/detectors/substrate-pallets/unsigned-extrinsic/Cargo.toml new file mode 100644 index 00000000..3b934ec9 --- /dev/null +++ b/detectors/substrate-pallets/unsigned-extrinsic/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "unsigned-extrinsic" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +clippy_utils = { workspace = true } +common = { workspace = true } +dylint_linting = { workspace = true } +if_chain = { workspace = true } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/detectors/substrate-pallets/unsigned-extrinsic/src/lib.rs b/detectors/substrate-pallets/unsigned-extrinsic/src/lib.rs new file mode 100644 index 00000000..7e01f7ea --- /dev/null +++ b/detectors/substrate-pallets/unsigned-extrinsic/src/lib.rs @@ -0,0 +1,89 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_span; + +use clippy_utils::{diagnostics::span_lint_and_sugg, is_from_proc_macro, source::snippet}; +use common::{ + declarations::{Severity, VulnerabilityClass}, + macros::expose_lint_info, +}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{ + def_id::LocalDefId, + intravisit::{walk_expr, FnKind, Visitor}, + Body, Expr, ExprKind, FnDecl, QPath, +}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_span::{Span, Symbol}; + +const LINT_MESSAGE: &str = "Using unsigned extrinsics without fees exposes the chain to potential DoS attacks"; + +#[expose_lint_info] +pub static UNSIGNED_EXTRINSIC_INFO: LintInfo = LintInfo { + name: env!("CARGO_PKG_NAME"), + short_message: LINT_MESSAGE, + long_message: "Unsigned extrinsics allow transactions to be submitted without any associated fees \ + or signatures. This can be exploited by malicious actors to flood the network with \ + transactions at no cost, potentially causing denial of service. Consider using signed \ + extrinsics with appropriate fee mechanisms unless there's a specific security reason \ + for allowing unsigned transactions.", + severity: Severity::Critical, + help: "https://coinfabrik.github.io/scout-substrate/docs/detectors/unsigned-extrinsic", + vulnerability_class: VulnerabilityClass::DoS, +}; + +dylint_linting::declare_late_lint! { + pub UNSIGNED_EXTRINSIC, + Warn, + LINT_MESSAGE +} + +struct UnsignedExtrinsicVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, +} + +impl<'a, 'tcx> Visitor<'tcx> for UnsignedExtrinsicVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + if_chain! { + // Ignore expressions from proc macros + if !is_from_proc_macro(self.cx, expr); + // Check if the expression is `ensure_none` + if let ExprKind::Call(callee, args) = &expr.kind; + if let ExprKind::Path(QPath::Resolved(None, path)) = &callee.kind; + if path.segments.len() == 1; + if path.segments[0].ident.name == Symbol::intern("ensure_none"); + then { + let first_arg_str = snippet(self.cx, args[0].span, ".."); + span_lint_and_sugg( + self.cx, + UNSIGNED_EXTRINSIC, + expr.span, + LINT_MESSAGE, + "consider signing this extrinsic to prevent DoS attacks", + format!("ensure_signed({})", first_arg_str), + Applicability::MaybeIncorrect, + ); + } + } + + walk_expr(self, expr); + } +} + +impl<'tcx> LateLintPass<'tcx> for UnsignedExtrinsic { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + _: FnKind<'tcx>, + _: &'tcx FnDecl<'tcx>, + body: &'tcx Body<'tcx>, + _: Span, + _: LocalDefId, + ) { + let mut visitor = UnsignedExtrinsicVisitor { cx }; + walk_expr(&mut visitor, body.value); + } +} diff --git a/test-cases/substrate-pallets/Cargo.lock b/test-cases/substrate-pallets/Cargo.lock index 4e05a56d..458317d2 100644 --- a/test-cases/substrate-pallets/Cargo.lock +++ b/test-cases/substrate-pallets/Cargo.lock @@ -3541,6 +3541,36 @@ dependencies = [ "sp-runtime", ] +[[package]] +name = "unsigned-extrinsic-remediated-1" +version = "0.1.0" +dependencies = [ + "frame-support", + "frame-system", + "log", + "pallet-balances", + "parity-scale-codec", + "scale-info", + "sp-core", + "sp-io", + "sp-runtime", +] + +[[package]] +name = "unsigned-extrinsic-vulnerable-1" +version = "0.1.0" +dependencies = [ + "frame-support", + "frame-system", + "log", + "pallet-balances", + "parity-scale-codec", + "scale-info", + "sp-core", + "sp-io", + "sp-runtime", +] + [[package]] name = "valuable" version = "0.1.0" diff --git a/test-cases/substrate-pallets/unsigned-extrinsic/remediated/remediated-1/Cargo.toml b/test-cases/substrate-pallets/unsigned-extrinsic/remediated/remediated-1/Cargo.toml new file mode 100644 index 00000000..1927b1a9 --- /dev/null +++ b/test-cases/substrate-pallets/unsigned-extrinsic/remediated/remediated-1/Cargo.toml @@ -0,0 +1,34 @@ +[package] +edition = "2021" +name = "unsigned-extrinsic-remediated-1" +version = "0.1.0" + +[package.metadata.docs.rs] +targets = ["x86_64-unknown-linux-gnu"] + +[dependencies] +codec = { workspace = true } +frame-support = { workspace = true } +frame-system = { workspace = true } +log = { workspace = true } +pallet-balances = { workspace = true } +scale-info = { features = ["derive"], workspace = true } +sp-io = { workspace = true } +sp-runtime = { workspace = true } + +[dev-dependencies] +sp-core = { workspace = true } + +[features] +default = ["std"] +std = [ + "codec/std", + "frame-support/std", + "frame-system/std", + "log/std", + "pallet-balances/std", + "scale-info/std", + "sp-core/std", + "sp-io/std", + "sp-runtime/std", +] diff --git a/test-cases/substrate-pallets/unsigned-extrinsic/remediated/remediated-1/src/lib.rs b/test-cases/substrate-pallets/unsigned-extrinsic/remediated/remediated-1/src/lib.rs new file mode 100644 index 00000000..f2afa14c --- /dev/null +++ b/test-cases/substrate-pallets/unsigned-extrinsic/remediated/remediated-1/src/lib.rs @@ -0,0 +1,63 @@ +// lib.rs +#![cfg_attr(not(feature = "std"), no_std)] + +extern crate alloc; + +pub use pallet::*; + +pub mod weights; +pub use weights::*; + +#[frame_support::pallet] +pub mod pallet { + use super::*; + use frame_support::{pallet_prelude::*, traits::BuildGenesisConfig}; + use frame_system::pallet_prelude::*; + + #[pallet::config] + pub trait Config: pallet_balances::Config + frame_system::Config { + type RuntimeEvent: From> + IsType<::RuntimeEvent>; + type WeightInfo: WeightInfo; + } + + #[pallet::pallet] + pub struct Pallet(_); + + #[pallet::storage] + #[pallet::getter(fn balance_storage)] + pub type BalanceStorage = StorageValue<_, u32>; + + #[pallet::hooks] + impl Hooks> for Pallet { + fn on_initialize(_n: BlockNumberFor) -> Weight { + Weight::zero() + } + } + + #[pallet::call(weight(::WeightInfo))] + impl Pallet { + #[pallet::call_index(0)] + pub fn safe_call(origin: OriginFor) -> DispatchResult { + let _ = ensure_signed(origin)?; + + Ok(()) + } + } + + #[pallet::event] + pub enum Event {} + + #[pallet::genesis_config] + #[derive(frame_support::DefaultNoBound)] + pub struct GenesisConfig { + #[serde(skip)] + pub _phantom: PhantomData, + } + + #[pallet::genesis_build] + impl BuildGenesisConfig for GenesisConfig { + fn build(&self) { + BalanceStorage::::put(0); + } + } +} diff --git a/test-cases/substrate-pallets/unsigned-extrinsic/remediated/remediated-1/src/weights.rs b/test-cases/substrate-pallets/unsigned-extrinsic/remediated/remediated-1/src/weights.rs new file mode 100644 index 00000000..b199e02d --- /dev/null +++ b/test-cases/substrate-pallets/unsigned-extrinsic/remediated/remediated-1/src/weights.rs @@ -0,0 +1,23 @@ +#![cfg_attr(rustfmt, rustfmt_skip)] +#![allow(unused_parens)] +#![allow(unused_imports)] + +use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; +use core::marker::PhantomData; + +pub trait WeightInfo { + fn safe_call() -> Weight; +} + +pub struct SubstrateWeight(PhantomData); +impl WeightInfo for SubstrateWeight { + fn safe_call() -> Weight { + Weight::from_parts(19_000_000_u64, 0) + } +} + +impl WeightInfo for () { + fn safe_call() -> Weight { + Weight::from_parts(19_000_000_u64, 0) + } +} diff --git a/test-cases/substrate-pallets/unsigned-extrinsic/vulnerable/vulnerable-1/Cargo.toml b/test-cases/substrate-pallets/unsigned-extrinsic/vulnerable/vulnerable-1/Cargo.toml new file mode 100644 index 00000000..70a765b3 --- /dev/null +++ b/test-cases/substrate-pallets/unsigned-extrinsic/vulnerable/vulnerable-1/Cargo.toml @@ -0,0 +1,34 @@ +[package] +edition = "2021" +name = "unsigned-extrinsic-vulnerable-1" +version = "0.1.0" + +[package.metadata.docs.rs] +targets = ["x86_64-unknown-linux-gnu"] + +[dependencies] +codec = { workspace = true } +frame-support = { workspace = true } +frame-system = { workspace = true } +log = { workspace = true } +pallet-balances = { workspace = true } +scale-info = { features = ["derive"], workspace = true } +sp-io = { workspace = true } +sp-runtime = { workspace = true } + +[dev-dependencies] +sp-core = { workspace = true } + +[features] +default = ["std"] +std = [ + "codec/std", + "frame-support/std", + "frame-system/std", + "log/std", + "pallet-balances/std", + "scale-info/std", + "sp-core/std", + "sp-io/std", + "sp-runtime/std", +] diff --git a/test-cases/substrate-pallets/unsigned-extrinsic/vulnerable/vulnerable-1/src/lib.rs b/test-cases/substrate-pallets/unsigned-extrinsic/vulnerable/vulnerable-1/src/lib.rs new file mode 100644 index 00000000..3d8747b8 --- /dev/null +++ b/test-cases/substrate-pallets/unsigned-extrinsic/vulnerable/vulnerable-1/src/lib.rs @@ -0,0 +1,63 @@ +// lib.rs +#![cfg_attr(not(feature = "std"), no_std)] + +extern crate alloc; + +pub use pallet::*; + +pub mod weights; +pub use weights::*; + +#[frame_support::pallet] +pub mod pallet { + use super::*; + use frame_support::{pallet_prelude::*, traits::BuildGenesisConfig}; + use frame_system::pallet_prelude::*; + + #[pallet::config] + pub trait Config: pallet_balances::Config + frame_system::Config { + type RuntimeEvent: From> + IsType<::RuntimeEvent>; + type WeightInfo: WeightInfo; + } + + #[pallet::pallet] + pub struct Pallet(_); + + #[pallet::storage] + #[pallet::getter(fn balance_storage)] + pub type BalanceStorage = StorageValue<_, u32>; + + #[pallet::hooks] + impl Hooks> for Pallet { + fn on_initialize(_n: BlockNumberFor) -> Weight { + Weight::zero() + } + } + + #[pallet::call(weight(::WeightInfo))] + impl Pallet { + #[pallet::call_index(0)] + pub fn unsafe_call(origin: OriginFor) -> DispatchResult { + ensure_none(origin)?; + + Ok(()) + } + } + + #[pallet::event] + pub enum Event {} + + #[pallet::genesis_config] + #[derive(frame_support::DefaultNoBound)] + pub struct GenesisConfig { + #[serde(skip)] + pub _phantom: PhantomData, + } + + #[pallet::genesis_build] + impl BuildGenesisConfig for GenesisConfig { + fn build(&self) { + BalanceStorage::::put(0); + } + } +} diff --git a/test-cases/substrate-pallets/unsigned-extrinsic/vulnerable/vulnerable-1/src/weights.rs b/test-cases/substrate-pallets/unsigned-extrinsic/vulnerable/vulnerable-1/src/weights.rs new file mode 100644 index 00000000..8cdda511 --- /dev/null +++ b/test-cases/substrate-pallets/unsigned-extrinsic/vulnerable/vulnerable-1/src/weights.rs @@ -0,0 +1,23 @@ +#![cfg_attr(rustfmt, rustfmt_skip)] +#![allow(unused_parens)] +#![allow(unused_imports)] + +use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; +use core::marker::PhantomData; + +pub trait WeightInfo { + fn unsafe_call() -> Weight; +} + +pub struct SubstrateWeight(PhantomData); +impl WeightInfo for SubstrateWeight { + fn unsafe_call() -> Weight { + Weight::from_parts(19_000_000_u64, 0) + } +} + +impl WeightInfo for () { + fn unsafe_call() -> Weight { + Weight::from_parts(19_000_000_u64, 0) + } +} From 858d98837b7e2c2fa5e6eec413c183ab99031690 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Tue, 3 Dec 2024 14:39:44 -0300 Subject: [PATCH 03/30] Only use last segment of the call --- detectors/substrate-pallets/unsigned-extrinsic/src/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/detectors/substrate-pallets/unsigned-extrinsic/src/lib.rs b/detectors/substrate-pallets/unsigned-extrinsic/src/lib.rs index 7e01f7ea..411c3bc9 100644 --- a/detectors/substrate-pallets/unsigned-extrinsic/src/lib.rs +++ b/detectors/substrate-pallets/unsigned-extrinsic/src/lib.rs @@ -19,7 +19,8 @@ use rustc_hir::{ use rustc_lint::{LateContext, LateLintPass}; use rustc_span::{Span, Symbol}; -const LINT_MESSAGE: &str = "Using unsigned extrinsics without fees exposes the chain to potential DoS attacks"; +const LINT_MESSAGE: &str = + "Using unsigned extrinsics without fees exposes the chain to potential DoS attacks"; #[expose_lint_info] pub static UNSIGNED_EXTRINSIC_INFO: LintInfo = LintInfo { @@ -53,8 +54,8 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsignedExtrinsicVisitor<'a, 'tcx> { // Check if the expression is `ensure_none` if let ExprKind::Call(callee, args) = &expr.kind; if let ExprKind::Path(QPath::Resolved(None, path)) = &callee.kind; - if path.segments.len() == 1; - if path.segments[0].ident.name == Symbol::intern("ensure_none"); + if let Some(last_segment) = path.segments.last(); + if last_segment.ident.name == Symbol::intern("ensure_none"); then { let first_arg_str = snippet(self.cx, args[0].span, ".."); span_lint_and_sugg( From 169ca12af110f26aae33b949614b0729aa8ef99a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20M=2E=20Gonz=C3=A1lez?= Date: Wed, 4 Dec 2024 09:35:50 -0300 Subject: [PATCH 04/30] Delete incorrectly added file. --- test-cases/substrate-pallets/basic.tar | Bin 20480 -> 0 bytes 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 test-cases/substrate-pallets/basic.tar diff --git a/test-cases/substrate-pallets/basic.tar b/test-cases/substrate-pallets/basic.tar deleted file mode 100644 index b7d171bec9d29a9fd0a3aa7e5c93dd761b817b12..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 20480 zcmeHOdsExUme0TAPtlO7tek5c+Zf0y9;Wgjcc`5tvw=Hzw^Ec!NNrGIOTKypxP`_kp!jH$|8RS_Q_6p9umkyTZViS{#NZ*2X~O&$m;aZAADsCo@~9L0b18ln z>kEH2llg|c@aKz}++YP+?|hYM79}xF813{2{lR+2FY?nQ&HCp_dX@nUtH*RY@Ec!`Vl~cav|eT#z7>pEJ#8bV9}Lmk|?e_v*cveP!{-LTog|eo-vhvUZhfO zAIdcPD%X#CZ7`=PWUz6J{ZHq9dKSfMi-h^(&j5}1d&APKSS*q>Uxk>0pIv6ToUfXr z#eeM2{5W8wTglHB8<8Tfg*O;Xi#U&1XsyeM)n7O2?8d4fNl80aEHR8cg4#^|Vn%d+ zSP3!dF&&vL1=|jcQ-)yq*Y> z7_oRvGd?H5S@!N&ja*;9VsPXfT$dc9OkMZ)Q%p_f@8IZPbtoEuZv=Ru*6fq>z3Til zjYTGNFD&NsORsdSPFHN~>Y1;2k{<-cyqNj9RCAYSWT4FZDk({DE?iM;j(Va7=2~Hq z!9@qTI4F+s4e%5`lfzxd!WfU;R~@ixHaT<+pV;HL?%O1uMkfY6CS;nVEdcLEAIq2yG?|n5^2{0jyN6@R|BCH#PYk-4_ftQkSjEo~dU$>qI)|=n#NfJ!p`332JdJXR z^&XaWvFQ3%2ChWmAmRp2oDmw0_1hv!nf29VkNZhIjt*hEK_(!OM zEkrs#e@^6W_WDP^tNi~gI(`7(Px#*K|A$*UJ7xW6xINLDTtlQw({y$D~ zN9S0`r8u$b_C%a`NT$fA%Zps5u?Wbm35o~;cwTb!{M~io9bNYiiMRBU^DwygcapyK z#9mCFxBK$8$e|)_^#$E?D+F>IWLVE9I(Mc>rMLW_(*QEl0 zdgRQ|qi5G8=;YM($imYpAgHRLa`h%DP>KjET4>d$X-^y|7cE0X3c4&)zJ%Y#cgAE7 z(u%2OQ_&b_en6!b@2fwHP^7sun-Y8XV}5&>`es^BSPy^bC9N;n?ezrDvDPRqVt??B zLo|}DN}pZ#Io>^`;L|}kkwd@cNhs)RrYF<0mcXlWbtw{Bo`aX-jjB|9nj)xz+D;D1 zajH!`Q^hEwD%aK1U!l5#a8rEP|LwQ^w|gHC4>+a+fMFSwz;0m^9ndKic%P&PGJi+m zBt>qkwsCBv5b2en-l_$aMoye(_TpZ5SCFS~*?t#Zodky%m$~9m(aU+3$r+SHmAqWh zgl2`TdO3kW!A~IQ$fLz9k|~%#K>DDb2I&tm&T|E_KM+wS@>87pIW#l}4>BYLB0@CI zuVE$92#Ob!iwQake_Vj8pu7+ho6ubqojR33z&EwI(A3xBS4Tm7ZsFZE?iV=yUj2NO@f_2@|fz(aH6HV)KFOdY=M0v(+-AOVvGiC`Wnp*tdZRgw&`iiuKLPEng z`mBL;-A@Zi@qH5evvS(CkSL+vyzXO?&g~`N!A~;2FuV6+;{t2Fsg#C$-i?Y)J6UAV zqwD^+%qnD`eraT->O>x~fgy{do;47%SIA!}--Nv(Znk(QeWn+woE0H5)a9AlQLLEw z6qRc#xw|e*5>me--l)oGRYJJZ*oeQhCXf~zY8!Bht+pU~*o1!YPom&sF+T>O0@?ZG z0LQ;HmAkNCG?o5%`e#+!-TR6&?TYuvDJT4$m+kHXq5X{9+=O3{Ij%cQMGy|W4QOc> zW%0=Ax%&Jow@GN=6w^mVnBri?Ic>CmXU0Gn!G`T zbEen=yALSJ6qi6P7Q+U^twu2#AMlg^l#Um>VrhN5@g)Qlv99}1nI?|1RrFfZNnh0t z(+KDG2KZNv71L>O>PN9hMS7X8!V0Ub-72o4k1=!Dx8#oUtEK2F>2%?`X?2r_ZEUK8 z2ERgi;S4kY0zz63N|(wpNgAC*VCg6RRMP0d=LEo98OLGvJr@`*h|wC=iw7l@)1KgQ2Ac)da15O5LpJ3%(tv zx;27;A0#(u^pf`1YE_m3of0b=ZB?$?j0&G1nWNR29|>YR0_H;6npbh;b{*7OmnO)% zR_I&=!Qe6M;JnL;te`s;#_p&O5v9q zirb}4oL#chtaZv>R6ISAF~uY(y(-;wATEd9ZkLQfO=Go^kU(Wz%!P712*r)*u7aPN zmAa%!8x77MD*5T8iHN8eQPYX<0`LZROe)L>Doh-O&#XPbl)mqW)&4Z)ZaXaTI75k= zJNg8}VkP@-KHn7m8h*iwsQF0V3OVtR-<5{c_~c4dH^K&yt&l(X3xfM*=AX%tHypjh z-hq#-96ME{-+$ltNz%G+3q_ox7Vl*scWZBm?G-(+2nA8`1HhKR6i<{-~mp$uw81Ue%0&3ZT?{ ztYD3#NP&zmqX33k2&n0`Y}A~`X4v#p_ufMEJfJe6)Jjl~k^o*i5f+OMtW~sh&p7Ej zVU_XS+@!*PSG4(gi$v)s#zwSx@d+{&o2P&;-=`Rf=#@Ceo+dA{F4y} z6)4ht&0+NZVn;CI=btq~wqvvc7Zy=@I8_M~6**nfd^<~Gs%-v)N{cCl{`8enA4*jq zsHb15(w{@!RF8}>b>RGY<{VOpLZ``;n5eqhFwjV*4iKl($7LL}G!e_vl2V|?d{kkn zX^O5c8hP@#I8ntUYa?^Cpih`(((I`#oFwmRu9&TCULwIzqMMV0Ob0OX0ITjsC}H16m>i*yDji^tI|+mqEXEYyu~V0FIOm)94Z<+`Y&cRbEV-ddvE`$8 ze}1Um!JlAAi<~V_hw37T6T4zG7+9}KqN&7vfVibO;JSMkr#^0sWMFnuq_`{Cr=qLT zQd(>kBVam$Q)Xu$p-)yEvnAyIHO(}CoY9hmUAIiukbw02NI1mLwpN-InbkpApzxeK z>($D0)SglqNJLg)f887wvy?u`C9&kZoSY3G0*hGWdY35k?`Q{skeb<3Sp{E41|L*gs^-w zsm>cU#-SUn&}LLI69rqc@b9A;g2{@j>gu}d=K7W(Dm&?AehLm-VG*-g6$9x$y>T{` zlzx-(Nlw2Cs12DrY|ZzzJPCufXKUM!C|JWw)_55$E$yk)6|d|1t7D^b9h9&{$Y?KrTJJTYTtTZa+Z;`};)B4aeyWzp z5AJ-hmX_Xdwlr7Bf)O2x8?Q{$Ti5(rPo>oY#6m^Q_IOge3}GwP-IQlZ5MT_Kk7BM} z=P(T(?hB~fwus~+x*&s>bay4CPFIc};%MVRksAHtL+@Ox5LscG`gMzo9G!n6Vg*~3 zX)Lwy##!26wi{5o#mr?qEsd*M+@|Yo<59II9Y?OS;UFZqCxy#WYhRpUPmGX;Ik-%` z**&VyMokAdNypVo?W9x?*XJS_u4ZwuSBH}QB0f)%p_oijN93dcPrneik2=T{(f#2c z?RcnxdFIDOCV17Xq)LA!p1!=0L4gYWd6b_z-|RY#4xopS%y^^@F_EfYkMMf5Q@z$f zg8avt!-Wp269Fw$%J_0I?k-VF3-Rud8eb((%h+PgF>cNE%bu0KvaJ0_7C5xI|{6QJ}`Q9hR^e110rF@MwA8~6 zH)w{=Qtqo57IwPSR}=JTe0Rp+Bsi<3Thi0uN$aj#de>#z%dAlyjxsg9qH47ndC{dPwBOVnzUn-+P1N0)O>(8x%)3G# zuA=Z@t6%9BCbY3$=26yG?xrq4oY%ew2KPJ%++s6Y8td!s)5WLbFSvY>JHiqJJ& zFPSF8zQVeXP}ig1;R5kpT!K`vEtZsOYp<%L&UllbQ&A3_TNTl{%N|%ADD1*uiG%;L^< Date: Wed, 4 Dec 2024 09:37:19 -0300 Subject: [PATCH 05/30] Update .gitignore. --- .gitignore | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index e32934bb..baba65c3 100644 --- a/.gitignore +++ b/.gitignore @@ -53,9 +53,10 @@ report.md env/ venv/ !/detectors/ink/.vscode/ -__pycache__ +__pycache__ .vs /scripts/cs/run-tests/bin/ /scripts/cs/run-tests/obj/ launchSettings.json +basic.tar From 80d1ce3bbd1f3d33c391cade95dd8b1bcde16f05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20M=2E=20Gonz=C3=A1lez?= Date: Wed, 4 Dec 2024 09:49:31 -0300 Subject: [PATCH 06/30] Added test cases. --- .../vulnerable/vulnerable-1/Cargo.toml | 41 +++ .../vulnerable/vulnerable-1/src/lib.rs | 247 ++++++++++++++++++ .../vulnerable/vulnerable-1/src/tests.rs | 153 +++++++++++ .../vulnerable/vulnerable-1/src/weights.rs | 46 ++++ .../vulnerable/vulnerable-2/Cargo.toml | 41 +++ .../vulnerable/vulnerable-2/src/lib.rs | 218 ++++++++++++++++ .../vulnerable/vulnerable-2/src/tests.rs | 153 +++++++++++ .../vulnerable/vulnerable-2/src/weights.rs | 46 ++++ 8 files changed, 945 insertions(+) create mode 100644 test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/Cargo.toml create mode 100644 test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/src/lib.rs create mode 100644 test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/src/tests.rs create mode 100644 test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/src/weights.rs create mode 100644 test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/Cargo.toml create mode 100644 test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/src/lib.rs create mode 100644 test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/src/tests.rs create mode 100644 test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/src/weights.rs diff --git a/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/Cargo.toml b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/Cargo.toml new file mode 100644 index 00000000..73ea3b48 --- /dev/null +++ b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/Cargo.toml @@ -0,0 +1,41 @@ +[package] +name = "pallet-satirating-arithmetic-vulnerable-1" +version = "27.0.0" +authors.workspace = true +edition.workspace = true +license = "MIT-0" +description = "FRAME example pallet" +readme = "README.md" +publish = false + +[package.metadata.docs.rs] +targets = ["x86_64-unknown-linux-gnu"] + +[dependencies] +codec = { workspace = true } +log = { workspace = true } +scale-info = { features = ["derive"], workspace = true } +frame-benchmarking = { optional = true, workspace = true } +frame-support = { workspace = true } +frame-system = { workspace = true } +pallet-balances = { workspace = true } +sp-io = { workspace = true } +sp-runtime = { workspace = true } + +[dev-dependencies] +sp-core = { workspace = true } + +[features] +default = ["std"] +std = [ + "codec/std", + "frame-benchmarking?/std", + "frame-support/std", + "frame-system/std", + "log/std", + "pallet-balances/std", + "scale-info/std", + "sp-core/std", + "sp-io/std", + "sp-runtime/std", +] diff --git a/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/src/lib.rs b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/src/lib.rs new file mode 100644 index 00000000..0094aebb --- /dev/null +++ b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/src/lib.rs @@ -0,0 +1,247 @@ +#![cfg_attr(not(feature = "std"), no_std)] + +extern crate alloc; + +use alloc::vec::Vec; +use codec::{Decode, Encode}; +use core::marker::PhantomData; +use frame_support::{ + dispatch::{ClassifyDispatch, DispatchClass, DispatchResult, Pays, PaysFee, WeighData}, + traits::IsSubType, + weights::Weight, +}; +use frame_system::ensure_signed; +use log::info; +use scale_info::TypeInfo; +use sp_runtime::{ + traits::{Bounded, DispatchInfoOf, SaturatedConversion, Saturating, SignedExtension}, + transaction_validity::{ + InvalidTransaction, TransactionValidity, TransactionValidityError, ValidTransaction, + }, +}; + +pub use pallet::*; + +#[cfg(test)] +mod tests; + +pub mod weights; +pub use weights::*; + +type BalanceOf = ::Balance; +const MILLICENTS: u32 = 1_000_000_000; + +struct WeightForSetDummy(BalanceOf); + +impl WeighData<(&BalanceOf,)> for WeightForSetDummy { + fn weigh_data(&self, target: (&BalanceOf,)) -> Weight { + let multiplier = self.0; + // *target.0 is the amount passed into the extrinsic + let cents = *target.0 / >::from(MILLICENTS); + Weight::from_parts((cents * multiplier).saturated_into::(), 0) + } +} + +impl ClassifyDispatch<(&BalanceOf,)> for WeightForSetDummy { + fn classify_dispatch(&self, target: (&BalanceOf,)) -> DispatchClass { + if *target.0 > >::from(1000u32) { + DispatchClass::Operational + } else { + DispatchClass::Normal + } + } +} + +impl PaysFee<(&BalanceOf,)> for WeightForSetDummy { + fn pays_fee(&self, _target: (&BalanceOf,)) -> Pays { + Pays::Yes + } +} + +#[frame_support::pallet] +pub mod pallet { + use super::*; + use frame_support::pallet_prelude::*; + use frame_system::pallet_prelude::*; + #[pallet::config] + pub trait Config: pallet_balances::Config + frame_system::Config { + #[pallet::constant] + type MagicNumber: Get; + + type RuntimeEvent: From> + IsType<::RuntimeEvent>; + + type WeightInfo: WeightInfo; + } + + #[pallet::pallet] + pub struct Pallet(_); + + #[pallet::hooks] + impl Hooks> for Pallet { + fn on_initialize(_n: BlockNumberFor) -> Weight { + Weight::zero() + } + + fn on_finalize(_n: BlockNumberFor) {} + + fn offchain_worker(_n: BlockNumberFor) {} + } + + #[pallet::call(weight(::WeightInfo))] + impl Pallet { + #[pallet::call_index(0)] + pub fn accumulate_dummy(origin: OriginFor, increase_by: T::Balance) -> DispatchResult { + let _sender = ensure_signed(origin)?; + + >::mutate(|dummy| { + let new_dummy = dummy.map_or(increase_by, |d| d.saturating_add(increase_by)); + *dummy = Some(new_dummy); + }); + + Self::deposit_event(Event::AccumulateDummy { + balance: increase_by, + }); + + Ok(()) + } + + #[pallet::call_index(1)] + #[pallet::weight(WeightForSetDummy::(>::from(100u32)))] + pub fn set_dummy( + origin: OriginFor, + #[pallet::compact] new_value: T::Balance, + ) -> DispatchResult { + ensure_root(origin)?; + + info!("New value is now: {:?}", new_value); + + >::put(new_value); + + Self::deposit_event(Event::SetDummy { balance: new_value }); + + Ok(()) + } + } + + #[pallet::event] + #[pallet::generate_deposit(pub(super) fn deposit_event)] + pub enum Event { + AccumulateDummy { + balance: BalanceOf, + }, + SetDummy { + balance: BalanceOf, + }, + SetBar { + account: T::AccountId, + balance: BalanceOf, + }, + } + + #[pallet::storage] + pub(super) type Dummy = StorageValue<_, T::Balance>; + + #[pallet::storage] + pub(super) type Bar = StorageMap<_, Blake2_128Concat, T::AccountId, T::Balance>; + + #[pallet::storage] + pub(super) type Foo = StorageValue<_, T::Balance, ValueQuery>; + + #[pallet::storage] + pub type CountedMap = CountedStorageMap<_, Blake2_128Concat, u8, u16>; + + #[pallet::genesis_config] + #[derive(frame_support::DefaultNoBound)] + pub struct GenesisConfig { + pub dummy: T::Balance, + pub bar: Vec<(T::AccountId, T::Balance)>, + pub foo: T::Balance, + } + + #[pallet::genesis_build] + impl BuildGenesisConfig for GenesisConfig { + fn build(&self) { + >::put(self.dummy); + for (a, b) in &self.bar { + >::insert(a, b); + } + >::put(self.foo); + } + } +} + +impl Pallet { + #[allow(dead_code)] + fn accumulate_foo(origin: T::RuntimeOrigin, increase_by: T::Balance) -> DispatchResult { + let _sender = ensure_signed(origin)?; + + let prev = Foo::::get(); + let result = Foo::::mutate(|x| { + *x = x.saturating_add(increase_by); + *x + }); + assert!(prev + increase_by == result); + + Ok(()) + } +} + +#[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)] +#[scale_info(skip_type_params(T))] +pub struct WatchDummy(PhantomData); + +impl core::fmt::Debug for WatchDummy { + fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { + write!(f, "WatchDummy") + } +} + +impl SignedExtension for WatchDummy +where + ::RuntimeCall: IsSubType>, +{ + const IDENTIFIER: &'static str = "WatchDummy"; + type AccountId = T::AccountId; + type Call = ::RuntimeCall; + type AdditionalSigned = (); + type Pre = (); + + fn additional_signed(&self) -> core::result::Result<(), TransactionValidityError> { + Ok(()) + } + + fn pre_dispatch( + self, + who: &Self::AccountId, + call: &Self::Call, + info: &DispatchInfoOf, + len: usize, + ) -> Result { + self.validate(who, call, info, len).map(|_| ()) + } + + fn validate( + &self, + _who: &Self::AccountId, + call: &Self::Call, + _info: &DispatchInfoOf, + len: usize, + ) -> TransactionValidity { + if len > 200 { + return InvalidTransaction::ExhaustsResources.into(); + } + + match call.is_sub_type() { + Some(Call::set_dummy { .. }) => { + sp_runtime::print("set_dummy was received."); + + let valid_tx = ValidTransaction { + priority: Bounded::max_value(), + ..Default::default() + }; + Ok(valid_tx) + } + _ => Ok(Default::default()), + } + } +} diff --git a/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/src/tests.rs b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/src/tests.rs new file mode 100644 index 00000000..c424e185 --- /dev/null +++ b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/src/tests.rs @@ -0,0 +1,153 @@ +use crate as pallet_example_basic; +use crate::*; +use frame_support::{ + assert_ok, derive_impl, + dispatch::{DispatchInfo, GetDispatchInfo}, + traits::{ConstU64, OnInitialize}, +}; +use sp_core::H256; +use sp_runtime::{ + traits::{BlakeTwo256, IdentityLookup}, + BuildStorage, +}; + +type Block = frame_system::mocking::MockBlock; + +frame_support::construct_runtime!( + pub enum Test + { + System: frame_system, + Balances: pallet_balances, + Example: pallet_example_basic, + } +); + +#[derive_impl(frame_system::config_preludes::TestDefaultConfig)] +impl frame_system::Config for Test { + type BaseCallFilter = frame_support::traits::Everything; + type BlockWeights = (); + type BlockLength = (); + type DbWeight = (); + type RuntimeOrigin = RuntimeOrigin; + type Nonce = u64; + type Hash = H256; + type RuntimeCall = RuntimeCall; + type Hashing = BlakeTwo256; + type AccountId = u64; + type Lookup = IdentityLookup; + type Block = Block; + type RuntimeEvent = RuntimeEvent; + type Version = (); + type PalletInfo = PalletInfo; + type AccountData = pallet_balances::AccountData; + type OnNewAccount = (); + type OnKilledAccount = (); + type SystemWeightInfo = (); + type SS58Prefix = (); + type OnSetCode = (); + type MaxConsumers = frame_support::traits::ConstU32<16>; +} + +#[derive_impl(pallet_balances::config_preludes::TestDefaultConfig)] +impl pallet_balances::Config for Test { + type AccountStore = System; +} + +impl Config for Test { + type MagicNumber = ConstU64<1_000_000_000>; + type RuntimeEvent = RuntimeEvent; + type WeightInfo = (); +} + +pub fn new_test_ext() -> sp_io::TestExternalities { + let t = RuntimeGenesisConfig { + system: Default::default(), + balances: Default::default(), + example: pallet_example_basic::GenesisConfig { + dummy: 42, + bar: alloc::vec![(1, 2), (2, 3)], + foo: 24, + }, + } + .build_storage() + .unwrap(); + t.into() +} + +#[test] +fn it_works_for_optional_value() { + new_test_ext().execute_with(|| { + let val1 = 42; + let val2 = 27; + assert_eq!(Dummy::::get(), Some(val1)); + + assert_ok!(Example::accumulate_dummy(RuntimeOrigin::signed(1), val2)); + assert_eq!(Dummy::::get(), Some(val1 + val2)); + + >::on_initialize(2); + assert_ok!(Example::accumulate_dummy(RuntimeOrigin::signed(1), val1)); + assert_eq!(Dummy::::get(), Some(val1 + val2 + val1)); + }); +} + +#[test] +fn it_works_for_default_value() { + new_test_ext().execute_with(|| { + assert_eq!(Foo::::get(), 24); + assert_ok!(Example::accumulate_foo(RuntimeOrigin::signed(1), 1)); + assert_eq!(Foo::::get(), 25); + }); +} + +#[test] +fn set_dummy_works() { + new_test_ext().execute_with(|| { + let test_val = 133; + assert_ok!(Example::set_dummy(RuntimeOrigin::root(), test_val)); + assert_eq!(Dummy::::get(), Some(test_val)); + }); +} + +#[test] +fn signed_ext_watch_dummy_works() { + new_test_ext().execute_with(|| { + let call = pallet_example_basic::Call::set_dummy { new_value: 10 }.into(); + let info = DispatchInfo::default(); + + assert_eq!( + WatchDummy::(PhantomData) + .validate(&1, &call, &info, 150) + .unwrap() + .priority, + u64::MAX, + ); + assert_eq!( + WatchDummy::(PhantomData).validate(&1, &call, &info, 250), + InvalidTransaction::ExhaustsResources.into(), + ); + }) +} + +#[test] +fn counted_map_works() { + new_test_ext().execute_with(|| { + assert_eq!(CountedMap::::count(), 0); + CountedMap::::insert(3, 3); + assert_eq!(CountedMap::::count(), 1); + }) +} + +#[test] +fn weights_work() { + let default_call = pallet_example_basic::Call::::accumulate_dummy { increase_by: 10 }; + let info1 = default_call.get_dispatch_info(); + assert!(info1.weight.ref_time() > 0); + assert_eq!( + info1.weight, + ::WeightInfo::accumulate_dummy() + ); + + let custom_call = pallet_example_basic::Call::::set_dummy { new_value: 20 }; + let info2 = custom_call.get_dispatch_info(); + assert!(info1.weight.ref_time() > info2.weight.ref_time()); +} diff --git a/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/src/weights.rs b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/src/weights.rs new file mode 100644 index 00000000..43d6814b --- /dev/null +++ b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-1/src/weights.rs @@ -0,0 +1,46 @@ +#![cfg_attr(rustfmt, rustfmt_skip)] +#![allow(unused_parens)] +#![allow(unused_imports)] + +use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; +use core::marker::PhantomData; + +pub trait WeightInfo { + fn set_dummy_benchmark() -> Weight; + fn accumulate_dummy() -> Weight; + fn sort_vector(x: u32, ) -> Weight; +} + +pub struct SubstrateWeight(PhantomData); +impl WeightInfo for SubstrateWeight { + fn set_dummy_benchmark() -> Weight { + Weight::from_parts(19_000_000_u64, 0) + .saturating_add(T::DbWeight::get().writes(1_u64)) + } + fn accumulate_dummy() -> Weight { + Weight::from_parts(18_000_000_u64, 0) + .saturating_add(T::DbWeight::get().reads(1_u64)) + .saturating_add(T::DbWeight::get().writes(1_u64)) + } + fn sort_vector(x: u32, ) -> Weight { + Weight::from_parts(0_u64, 0) + // Standard Error: 2 + .saturating_add(Weight::from_parts(520_u64, 0).saturating_mul(x as u64)) + } +} + +impl WeightInfo for () { + fn set_dummy_benchmark() -> Weight { + Weight::from_parts(19_000_000_u64, 0) + .saturating_add(RocksDbWeight::get().writes(1_u64)) + } + fn accumulate_dummy() -> Weight { + Weight::from_parts(18_000_000_u64, 0) + .saturating_add(RocksDbWeight::get().reads(1_u64)) + .saturating_add(RocksDbWeight::get().writes(1_u64)) + } + fn sort_vector(x: u32, ) -> Weight { + Weight::from_parts(0_u64, 0) + .saturating_add(Weight::from_parts(520_u64, 0).saturating_mul(x as u64)) + } +} diff --git a/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/Cargo.toml b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/Cargo.toml new file mode 100644 index 00000000..a08e6401 --- /dev/null +++ b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/Cargo.toml @@ -0,0 +1,41 @@ +[package] +name = "pallet-satirating-arithmetic-vulnerable-2" +version = "27.0.0" +authors.workspace = true +edition.workspace = true +license = "MIT-0" +description = "FRAME example pallet" +readme = "README.md" +publish = false + +[package.metadata.docs.rs] +targets = ["x86_64-unknown-linux-gnu"] + +[dependencies] +codec = { workspace = true } +log = { workspace = true } +scale-info = { features = ["derive"], workspace = true } +frame-benchmarking = { optional = true, workspace = true } +frame-support = { workspace = true } +frame-system = { workspace = true } +pallet-balances = { workspace = true } +sp-io = { workspace = true } +sp-runtime = { workspace = true } + +[dev-dependencies] +sp-core = { workspace = true } + +[features] +default = ["std"] +std = [ + "codec/std", + "frame-benchmarking?/std", + "frame-support/std", + "frame-system/std", + "log/std", + "pallet-balances/std", + "scale-info/std", + "sp-core/std", + "sp-io/std", + "sp-runtime/std", +] diff --git a/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/src/lib.rs b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/src/lib.rs new file mode 100644 index 00000000..706b2f77 --- /dev/null +++ b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/src/lib.rs @@ -0,0 +1,218 @@ +#![cfg_attr(not(feature = "std"), no_std)] + +extern crate alloc; + +use alloc::vec::Vec; +use codec::{Decode, Encode}; +use core::marker::PhantomData; +use frame_support::{ + dispatch::{ClassifyDispatch, DispatchClass, DispatchResult, Pays, PaysFee, WeighData}, + traits::IsSubType, + weights::Weight, +}; +use frame_system::ensure_signed; +use log::info; +use scale_info::TypeInfo; +use sp_runtime::{ + traits::{Bounded, DispatchInfoOf, SaturatedConversion, Saturating, SignedExtension}, + transaction_validity::{ + InvalidTransaction, TransactionValidity, TransactionValidityError, ValidTransaction, + }, +}; + +pub use pallet::*; + +#[cfg(test)] +mod tests; + +pub mod weights; +pub use weights::*; + +type BalanceOf = ::Balance; +const MILLICENTS: u32 = 1_000_000_000; + +struct WeightForSetDummy(BalanceOf); + +impl WeighData<(&BalanceOf,)> for WeightForSetDummy { + fn weigh_data(&self, target: (&BalanceOf,)) -> Weight { + let multiplier = self.0; + // *target.0 is the amount passed into the extrinsic + let cents = *target.0 / >::from(MILLICENTS); + Weight::from_parts((cents * multiplier).saturated_into::(), 0) + } +} + +impl ClassifyDispatch<(&BalanceOf,)> for WeightForSetDummy { + fn classify_dispatch(&self, target: (&BalanceOf,)) -> DispatchClass { + if *target.0 > >::from(1000u32) { + DispatchClass::Operational + } else { + DispatchClass::Normal + } + } +} + +impl PaysFee<(&BalanceOf,)> for WeightForSetDummy { + fn pays_fee(&self, _target: (&BalanceOf,)) -> Pays { + Pays::Yes + } +} + +#[frame_support::pallet] +pub mod pallet { + use super::*; + use frame_support::pallet_prelude::*; + use frame_system::pallet_prelude::*; + #[pallet::config] + pub trait Config: pallet_balances::Config + frame_system::Config { + #[pallet::constant] + type MagicNumber: Get; + + type RuntimeEvent: From> + IsType<::RuntimeEvent>; + + type WeightInfo: WeightInfo; + } + + #[pallet::pallet] + pub struct Pallet(_); + + #[pallet::hooks] + impl Hooks> for Pallet { + fn on_initialize(_n: BlockNumberFor) -> Weight { + Weight::zero() + } + + fn on_finalize(_n: BlockNumberFor) {} + + fn offchain_worker(_n: BlockNumberFor) {} + } + + #[pallet::call(weight(::WeightInfo))] + impl Pallet { + #[pallet::call_index(0)] + pub fn multiply_dummy(origin: OriginFor, factor: T::Balance) -> DispatchResult { + let _sender = ensure_signed(origin)?; + + >::mutate(|dummy| { + *dummy = dummy.and_then(|d| Some(d.saturating_mul(factor))); + }); + + Self::deposit_event(Event::AccumulateDummy { + balance: factor, + }); + + Ok(()) + } + + #[pallet::call_index(1)] + #[pallet::weight(WeightForSetDummy::(>::from(100u32)))] + pub fn set_dummy( + origin: OriginFor, + #[pallet::compact] new_value: T::Balance, + ) -> DispatchResult { + ensure_root(origin)?; + + info!("New value is now: {:?}", new_value); + + >::put(new_value); + + Self::deposit_event(Event::SetDummy { balance: new_value }); + + Ok(()) + } + } + + #[pallet::event] + #[pallet::generate_deposit(pub(super) fn deposit_event)] + pub enum Event { + AccumulateDummy { + balance: BalanceOf, + }, + SetDummy { + balance: BalanceOf, + }, + SetBar { + account: T::AccountId, + balance: BalanceOf, + }, + } + + #[pallet::storage] + pub(super) type Dummy = StorageValue<_, T::Balance>; + + #[pallet::storage] + pub type CountedMap = CountedStorageMap<_, Blake2_128Concat, u8, u16>; + + #[pallet::genesis_config] + #[derive(frame_support::DefaultNoBound)] + pub struct GenesisConfig { + pub dummy: T::Balance, + } + + #[pallet::genesis_build] + impl BuildGenesisConfig for GenesisConfig { + fn build(&self) { + >::put(self.dummy); + } + } +} + +#[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)] +#[scale_info(skip_type_params(T))] +pub struct WatchDummy(PhantomData); + +impl core::fmt::Debug for WatchDummy { + fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { + write!(f, "WatchDummy") + } +} + +impl SignedExtension for WatchDummy +where + ::RuntimeCall: IsSubType>, +{ + const IDENTIFIER: &'static str = "WatchDummy"; + type AccountId = T::AccountId; + type Call = ::RuntimeCall; + type AdditionalSigned = (); + type Pre = (); + + fn additional_signed(&self) -> core::result::Result<(), TransactionValidityError> { + Ok(()) + } + + fn pre_dispatch( + self, + who: &Self::AccountId, + call: &Self::Call, + info: &DispatchInfoOf, + len: usize, + ) -> Result { + self.validate(who, call, info, len).map(|_| ()) + } + + fn validate( + &self, + _who: &Self::AccountId, + call: &Self::Call, + _info: &DispatchInfoOf, + len: usize, + ) -> TransactionValidity { + if len > 200 { + return InvalidTransaction::ExhaustsResources.into(); + } + + match call.is_sub_type() { + Some(Call::set_dummy { .. }) => { + sp_runtime::print("set_dummy was received."); + + let valid_tx = ValidTransaction { + priority: Bounded::max_value(), + ..Default::default() + }; + Ok(valid_tx) + } + _ => Ok(Default::default()), + } + } +} diff --git a/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/src/tests.rs b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/src/tests.rs new file mode 100644 index 00000000..c424e185 --- /dev/null +++ b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/src/tests.rs @@ -0,0 +1,153 @@ +use crate as pallet_example_basic; +use crate::*; +use frame_support::{ + assert_ok, derive_impl, + dispatch::{DispatchInfo, GetDispatchInfo}, + traits::{ConstU64, OnInitialize}, +}; +use sp_core::H256; +use sp_runtime::{ + traits::{BlakeTwo256, IdentityLookup}, + BuildStorage, +}; + +type Block = frame_system::mocking::MockBlock; + +frame_support::construct_runtime!( + pub enum Test + { + System: frame_system, + Balances: pallet_balances, + Example: pallet_example_basic, + } +); + +#[derive_impl(frame_system::config_preludes::TestDefaultConfig)] +impl frame_system::Config for Test { + type BaseCallFilter = frame_support::traits::Everything; + type BlockWeights = (); + type BlockLength = (); + type DbWeight = (); + type RuntimeOrigin = RuntimeOrigin; + type Nonce = u64; + type Hash = H256; + type RuntimeCall = RuntimeCall; + type Hashing = BlakeTwo256; + type AccountId = u64; + type Lookup = IdentityLookup; + type Block = Block; + type RuntimeEvent = RuntimeEvent; + type Version = (); + type PalletInfo = PalletInfo; + type AccountData = pallet_balances::AccountData; + type OnNewAccount = (); + type OnKilledAccount = (); + type SystemWeightInfo = (); + type SS58Prefix = (); + type OnSetCode = (); + type MaxConsumers = frame_support::traits::ConstU32<16>; +} + +#[derive_impl(pallet_balances::config_preludes::TestDefaultConfig)] +impl pallet_balances::Config for Test { + type AccountStore = System; +} + +impl Config for Test { + type MagicNumber = ConstU64<1_000_000_000>; + type RuntimeEvent = RuntimeEvent; + type WeightInfo = (); +} + +pub fn new_test_ext() -> sp_io::TestExternalities { + let t = RuntimeGenesisConfig { + system: Default::default(), + balances: Default::default(), + example: pallet_example_basic::GenesisConfig { + dummy: 42, + bar: alloc::vec![(1, 2), (2, 3)], + foo: 24, + }, + } + .build_storage() + .unwrap(); + t.into() +} + +#[test] +fn it_works_for_optional_value() { + new_test_ext().execute_with(|| { + let val1 = 42; + let val2 = 27; + assert_eq!(Dummy::::get(), Some(val1)); + + assert_ok!(Example::accumulate_dummy(RuntimeOrigin::signed(1), val2)); + assert_eq!(Dummy::::get(), Some(val1 + val2)); + + >::on_initialize(2); + assert_ok!(Example::accumulate_dummy(RuntimeOrigin::signed(1), val1)); + assert_eq!(Dummy::::get(), Some(val1 + val2 + val1)); + }); +} + +#[test] +fn it_works_for_default_value() { + new_test_ext().execute_with(|| { + assert_eq!(Foo::::get(), 24); + assert_ok!(Example::accumulate_foo(RuntimeOrigin::signed(1), 1)); + assert_eq!(Foo::::get(), 25); + }); +} + +#[test] +fn set_dummy_works() { + new_test_ext().execute_with(|| { + let test_val = 133; + assert_ok!(Example::set_dummy(RuntimeOrigin::root(), test_val)); + assert_eq!(Dummy::::get(), Some(test_val)); + }); +} + +#[test] +fn signed_ext_watch_dummy_works() { + new_test_ext().execute_with(|| { + let call = pallet_example_basic::Call::set_dummy { new_value: 10 }.into(); + let info = DispatchInfo::default(); + + assert_eq!( + WatchDummy::(PhantomData) + .validate(&1, &call, &info, 150) + .unwrap() + .priority, + u64::MAX, + ); + assert_eq!( + WatchDummy::(PhantomData).validate(&1, &call, &info, 250), + InvalidTransaction::ExhaustsResources.into(), + ); + }) +} + +#[test] +fn counted_map_works() { + new_test_ext().execute_with(|| { + assert_eq!(CountedMap::::count(), 0); + CountedMap::::insert(3, 3); + assert_eq!(CountedMap::::count(), 1); + }) +} + +#[test] +fn weights_work() { + let default_call = pallet_example_basic::Call::::accumulate_dummy { increase_by: 10 }; + let info1 = default_call.get_dispatch_info(); + assert!(info1.weight.ref_time() > 0); + assert_eq!( + info1.weight, + ::WeightInfo::accumulate_dummy() + ); + + let custom_call = pallet_example_basic::Call::::set_dummy { new_value: 20 }; + let info2 = custom_call.get_dispatch_info(); + assert!(info1.weight.ref_time() > info2.weight.ref_time()); +} diff --git a/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/src/weights.rs b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/src/weights.rs new file mode 100644 index 00000000..43d6814b --- /dev/null +++ b/test-cases/substrate-pallets/saturating-arithmetic/vulnerable/vulnerable-2/src/weights.rs @@ -0,0 +1,46 @@ +#![cfg_attr(rustfmt, rustfmt_skip)] +#![allow(unused_parens)] +#![allow(unused_imports)] + +use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; +use core::marker::PhantomData; + +pub trait WeightInfo { + fn set_dummy_benchmark() -> Weight; + fn accumulate_dummy() -> Weight; + fn sort_vector(x: u32, ) -> Weight; +} + +pub struct SubstrateWeight(PhantomData); +impl WeightInfo for SubstrateWeight { + fn set_dummy_benchmark() -> Weight { + Weight::from_parts(19_000_000_u64, 0) + .saturating_add(T::DbWeight::get().writes(1_u64)) + } + fn accumulate_dummy() -> Weight { + Weight::from_parts(18_000_000_u64, 0) + .saturating_add(T::DbWeight::get().reads(1_u64)) + .saturating_add(T::DbWeight::get().writes(1_u64)) + } + fn sort_vector(x: u32, ) -> Weight { + Weight::from_parts(0_u64, 0) + // Standard Error: 2 + .saturating_add(Weight::from_parts(520_u64, 0).saturating_mul(x as u64)) + } +} + +impl WeightInfo for () { + fn set_dummy_benchmark() -> Weight { + Weight::from_parts(19_000_000_u64, 0) + .saturating_add(RocksDbWeight::get().writes(1_u64)) + } + fn accumulate_dummy() -> Weight { + Weight::from_parts(18_000_000_u64, 0) + .saturating_add(RocksDbWeight::get().reads(1_u64)) + .saturating_add(RocksDbWeight::get().writes(1_u64)) + } + fn sort_vector(x: u32, ) -> Weight { + Weight::from_parts(0_u64, 0) + .saturating_add(Weight::from_parts(520_u64, 0).saturating_mul(x as u64)) + } +} From 44742c3dfefbd9c52cc73d20209608936e1bcbd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20M=2E=20Gonz=C3=A1lez?= Date: Wed, 4 Dec 2024 11:03:27 -0300 Subject: [PATCH 07/30] Moved decomposers to reusable module. --- common/analysis/src/decomposers.rs | 175 ++++++++++++ common/analysis/src/lib.rs | 2 + .../detectors/src/iterators_over_indexing.rs | 249 +++--------------- 3 files changed, 218 insertions(+), 208 deletions(-) create mode 100644 common/analysis/src/decomposers.rs diff --git a/common/analysis/src/decomposers.rs b/common/analysis/src/decomposers.rs new file mode 100644 index 00000000..4e45c187 --- /dev/null +++ b/common/analysis/src/decomposers.rs @@ -0,0 +1,175 @@ +extern crate rustc_ast; +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; +extern crate rustc_type_ir; +extern crate rustc_lint; + +use rustc_ast::{BindingMode, Label, LitIntType, LitKind}; +use rustc_hir::{ + def::Res, + Block, Expr, ExprField, ExprKind, HirId, LangItem, LoopSource, MatchSource, Pat, PatField, + PatKind, Path, QPath, StmtKind, Ty, +}; +use rustc_middle::ty::{TyCtxt, TyKind}; +use rustc_span::{symbol::Ident, Span}; +use rustc_type_ir::Interner; + +pub fn type_to_adt<'hir>( + kind: &'hir rustc_type_ir::TyKind>, +) -> Option< + ( + &'hir as Interner>::AdtDef, + &'hir as Interner>::GenericArgs, + ) +> { + if let TyKind::Adt(a, b) = kind { + Some((a, b)) + } else { + None + } +} + +//--------------------------------------------------------------------- + +pub fn stmt_to_expr<'hir>(kind: &'hir StmtKind<'hir>) -> Option<&'hir Expr<'hir>> { + if let StmtKind::Expr(a) = kind { + Some(a) + } else { + None + } +} + +//--------------------------------------------------------------------- + +pub fn expr_to_drop_temps<'hir>(kind: &'hir ExprKind<'hir>) -> Option<&'hir Expr<'hir>> { + if let ExprKind::DropTemps(a) = kind { + Some(a) + } else { + None + } +} + +pub fn expr_to_match<'hir>( + kind: &'hir ExprKind<'hir>, +) -> Option<(&'hir Expr<'hir>, &'hir [rustc_hir::Arm<'hir>], MatchSource)> { + if let ExprKind::Match(a, b, c) = kind { + Some((a, b, *c)) + } else { + None + } +} + +pub fn expr_to_call<'hir>( + kind: &'hir ExprKind<'hir>, +) -> Option<(&'hir Expr<'hir>, &'hir [Expr<'hir>])> { + if let ExprKind::Call(a, b) = kind { + Some((a, b)) + } else { + None + } +} + +pub fn expr_to_path<'hir>(kind: &'hir ExprKind<'hir>) -> Option> { + if let ExprKind::Path(a) = kind { + Some(*a) + } else { + None + } +} + +pub fn expr_to_struct<'hir>( + kind: &'hir ExprKind<'hir>, +) -> Option< + ( + &'hir QPath<'hir>, + &'hir [ExprField<'hir>], + Option<&'hir Expr<'hir>>, + ), +> { + if let ExprKind::Struct(a, b, c) = kind { + Some((a, b, *c)) + } else { + None + } +} + +pub fn expr_to_lit<'hir>(kind: &'hir ExprKind<'hir>) -> Option<&'hir rustc_hir::Lit> { + if let ExprKind::Lit(a) = kind { + Some(a) + } else { + None + } +} + +pub fn expr_to_loop<'hir>( + kind: &'hir ExprKind<'hir>, +) -> Option<(&'hir Block<'hir>, &Option