diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index edad1258..b91bb9bf 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -98,6 +98,7 @@ jobs: test: [ "divide-before-multiply", + "insufficiently-random-values", "overflow-check", "unprotected-update-current-contract-wasm", "unsafe-expect", diff --git a/detectors/insufficiently-random-values/Cargo.toml b/detectors/insufficiently-random-values/Cargo.toml new file mode 100644 index 00000000..e6fc20e3 --- /dev/null +++ b/detectors/insufficiently-random-values/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "insufficiently-random-values" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +clippy_utils = { workspace = true } +dylint_linting = { workspace = true } +if_chain = { workspace = true } + +scout-audit-internal = { workspace = true } + +[dev-dependencies] +dylint_testing = { workspace = true } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/detectors/insufficiently-random-values/src/lib.rs b/detectors/insufficiently-random-values/src/lib.rs new file mode 100644 index 00000000..5841f123 --- /dev/null +++ b/detectors/insufficiently-random-values/src/lib.rs @@ -0,0 +1,45 @@ +#![feature(rustc_private)] + +extern crate rustc_hir; + +use if_chain::if_chain; +use rustc_hir::{BinOpKind, Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use scout_audit_internal::Detector; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// This detector prevents the usage of timestamp/sequence number and modulo operator as a random number source. + /// + /// ### Why is this bad? + /// The value of the block timestamp and block sequence can be manipulated by validators, which means they're not a secure source of randomness. Therefore, they shouldn't be used for generating random numbers, especially in the context of a betting contract where the outcomes of bets could be manipulated. + /// + /// ### Example + /// ```rust + /// let pseudo_random = env.ledger().timestamp() % max_val; + /// ``` + /// + pub INSUFFICIENTLY_RANDOM_VALUES, + Warn, + Detector::InsufficientlyRandomValues.get_lint_message() +} + +impl<'tcx> LateLintPass<'tcx> for InsufficientlyRandomValues { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if_chain! { + if let ExprKind::Binary(op, lexp, _rexp) = expr.kind; + if op.node == BinOpKind::Rem; + if let ExprKind::MethodCall(path, _, _, _) = lexp.kind; + if path.ident.as_str() == "timestamp" || + path.ident.as_str() == "sequence"; + then { + Detector::InsufficientlyRandomValues.span_lint_and_help( + cx, + INSUFFICIENTLY_RANDOM_VALUES, + expr.span, + &format!("This expression seems to use ledger().{}() as a pseudo random number",path.ident.as_str()), + ); + } + } + } +} diff --git a/scout-audit-internal/src/detector.rs b/scout-audit-internal/src/detector.rs index a6ed3fba..f6c58dab 100644 --- a/scout-audit-internal/src/detector.rs +++ b/scout-audit-internal/src/detector.rs @@ -27,6 +27,7 @@ use strum::{Display, EnumIter}; #[strum(serialize_all = "kebab-case")] pub enum Detector { DivideBeforeMultiply, + InsufficientlyRandomValues, OverflowCheck, UnprotectedUpdateCurrentContractWasm, UnsafeExpect, @@ -37,12 +38,13 @@ impl Detector { /// Returns the lint message for the detector. pub const fn get_lint_message(&self) -> &'static str { match self { + Detector::InsufficientlyRandomValues => INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE, Detector::DivideBeforeMultiply => DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE, - Detector::UnsafeExpect => UNSAFE_EXPECT_LINT_MESSAGE, Detector::OverflowCheck => OVERFLOW_CHECK_LINT_MESSAGE, Detector::UnprotectedUpdateCurrentContractWasm => { UNPROTECTED_UPDATE_CURRENT_CONTRACT_MESSAGE } + Detector::UnsafeExpect => UNSAFE_EXPECT_LINT_MESSAGE, Detector::UnsafeUnwrap => UNSAFE_UNWRAP_MESSAGE, } } diff --git a/scout-audit-internal/src/detector/lint_message.rs b/scout-audit-internal/src/detector/lint_message.rs index cf433c9e..0b32f51e 100644 --- a/scout-audit-internal/src/detector/lint_message.rs +++ b/scout-audit-internal/src/detector/lint_message.rs @@ -1,3 +1,4 @@ +pub const INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE: &str = "Use env.prng() to generate random numbers, and remember that all random numbers are under the control of validators"; pub const DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE: &str = "Division before multiplication might result in a loss of precision"; pub const OVERFLOW_CHECK_LINT_MESSAGE: &str = "Use `overflow-checks = true` in Cargo.toml profile"; diff --git a/test-cases/insufficiently-random-values/insufficiently-random-values-1/remediated-example/Cargo.toml b/test-cases/insufficiently-random-values/insufficiently-random-values-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..99e57ca8 --- /dev/null +++ b/test-cases/insufficiently-random-values/insufficiently-random-values-1/remediated-example/Cargo.toml @@ -0,0 +1,31 @@ + +[package] +name = "insufficiently-random-values-remediated-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = "20.0.0-rc2" + +[dev_dependencies] +soroban-sdk = { version = "20.0.0-rc2", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] + +[profile.release] +opt-level = "z" +overflow-checks = true +debug = 0 +strip = "symbols" +debug-assertions = false +panic = "abort" +codegen-units = 1 +lto = true + +[profile.release-with-logs] +inherits = "release" +debug-assertions = true diff --git a/test-cases/insufficiently-random-values/insufficiently-random-values-1/remediated-example/src/lib.rs b/test-cases/insufficiently-random-values/insufficiently-random-values-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..a05d3ed3 --- /dev/null +++ b/test-cases/insufficiently-random-values/insufficiently-random-values-1/remediated-example/src/lib.rs @@ -0,0 +1,54 @@ +#![no_std] + +use soroban_sdk::{contract, contracterror, contractimpl, Env}; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + MaxValZero = 1, +} + +#[contract] +pub struct Contract; + +#[contractimpl] +impl Contract { + pub fn generate_random_value(env: Env, max_val: u64) -> Result { + if max_val == 0 { + Err(Error::MaxValZero) + } else { + let val = env.prng().u64_in_range(0..max_val); + Ok(val) + } + } +} + +#[cfg(test)] +mod test { + use soroban_sdk::Env; + + use crate::{Contract, ContractClient}; + + #[test] + fn random_value_sequence() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, Contract); + let client = ContractClient::new(&env, &contract_id); + + // When + let first_random_value = client.generate_random_value(&10); + let second_random_value = client.generate_random_value(&10); + let third_random_value = client.generate_random_value(&10); + let fourth_random_value = client.generate_random_value(&10); + let fifth_random_value = client.generate_random_value(&10); + + // Then + assert_eq!(first_random_value, 6); + assert_eq!(second_random_value, 5); + assert_eq!(third_random_value, 8); + assert_eq!(fourth_random_value, 8); + assert_eq!(fifth_random_value, 4); + } +} diff --git a/test-cases/insufficiently-random-values/insufficiently-random-values-1/vulnerable-example/Cargo.toml b/test-cases/insufficiently-random-values/insufficiently-random-values-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..8d0e40bb --- /dev/null +++ b/test-cases/insufficiently-random-values/insufficiently-random-values-1/vulnerable-example/Cargo.toml @@ -0,0 +1,31 @@ + +[package] +name = "insufficiently-random-values-vulnerable-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = "20.0.0-rc2" + +[dev_dependencies] +soroban-sdk = { version = "20.0.0-rc2", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] + +[profile.release] +opt-level = "z" +overflow-checks = true +debug = 0 +strip = "symbols" +debug-assertions = false +panic = "abort" +codegen-units = 1 +lto = true + +[profile.release-with-logs] +inherits = "release" +debug-assertions = true diff --git a/test-cases/insufficiently-random-values/insufficiently-random-values-1/vulnerable-example/src/lib.rs b/test-cases/insufficiently-random-values/insufficiently-random-values-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..d5bbeedf --- /dev/null +++ b/test-cases/insufficiently-random-values/insufficiently-random-values-1/vulnerable-example/src/lib.rs @@ -0,0 +1,101 @@ +#![no_std] + +use soroban_sdk::{contract, contracterror, contractimpl, Env}; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + MaxValZero = 1, +} + +#[contract] +pub struct Contract; + +#[contractimpl] +impl Contract { + pub fn generate_random_value_timestamp(env: Env, max_val: u64) -> Result { + if max_val == 0 { + Err(Error::MaxValZero) + } else { + let val = env.ledger().timestamp() % max_val; + Ok(val) + } + } + pub fn generate_random_value_sequence(env: Env, max_val: u32) -> Result { + if max_val == 0 { + Err(Error::MaxValZero) + } else { + let val = env.ledger().sequence() % max_val; + Ok(val) + } + } +} + +#[cfg(test)] +mod test { + use soroban_sdk::{ + testutils::{Ledger, LedgerInfo}, + Env, + }; + + use crate::{Contract, ContractClient}; + + #[test] + fn random_value_sequence() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, Contract); + let client = ContractClient::new(&env, &contract_id); + + // When + let mut ledger = LedgerInfo { + sequence_number: 0, + ..Default::default() + }; + env.ledger().set(ledger.clone()); + let first_random_value = client.generate_random_value_sequence(&10); + + ledger.sequence_number = 1; + env.ledger().set(ledger.clone()); + let second_random_value = client.generate_random_value_sequence(&10); + + ledger.sequence_number = 11; + env.ledger().set(ledger.clone()); + let third_random_value = client.generate_random_value_sequence(&10); + + // Then + assert_eq!(first_random_value, 0); + assert_eq!(second_random_value, 1); + assert_eq!(third_random_value, 1); + } + + #[test] + fn random_value_timestamp() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, Contract); + let client = ContractClient::new(&env, &contract_id); + + // When + let mut ledger = LedgerInfo { + timestamp: 0, + ..Default::default() + }; + env.ledger().set(ledger.clone()); + let first_random_value = client.generate_random_value_timestamp(&10); + + ledger.timestamp = 1; + env.ledger().set(ledger.clone()); + let second_random_value = client.generate_random_value_timestamp(&10); + + ledger.timestamp = 11; + env.ledger().set(ledger.clone()); + let third_random_value = client.generate_random_value_timestamp(&10); + + // Then + assert_eq!(first_random_value, 0); + assert_eq!(second_random_value, 1); + assert_eq!(third_random_value, 1); + } +}