Insufficiently random values

.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",

detectors/insufficiently-random-values/Cargo.toml

@@ -0,0 +1,24 @@
+[package]
+name = "insufficiently-random-values"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "7671c283a50b5d1168841f3014b14000f01dd204" }
+dylint_linting = { workspace = true }
+if_chain = { workspace = true }
+semver = "1.0.4"
+serde_json = "1.0"
+toml = "0.7.6"
+ureq = { version = "2.7.1", features = ["json"] }
+
+scout-audit-internal = { workspace = true }
+
+[dev-dependencies]
+dylint_testing = { workspace = true }
+
+[package.metadata.rust-analyzer]
+rustc_private = true

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()),
+                );
+            }
+        }
+    }
+}

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,
         }
     }

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";

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

test-cases/insufficiently-random-values/insufficiently-random-values-1/remediated-example/src/lib.rs

@@ -0,0 +1,28 @@
+#![no_std]
+
+use soroban_sdk::{contract, contracterror, contractimpl, symbol_short, vec, Env, Symbol, Vec};
+
+#[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<u64, Error> {
+        if max_val == 0 {
+            Err(Error::MaxValZero)
+        } else {
+            let val = env.prng().u64_in_range(0..max_val);
+            Ok(val)
+        }
+    }
+}
+
+#[cfg(test)]
+mod test;

test-cases/insufficiently-random-values/insufficiently-random-values-1/remediated-example/src/test.rs

@@ -0,0 +1,24 @@
+extern crate std;
+
+use std::dbg;
+
+use soroban_sdk::{
+    symbol_short,
+    testutils::{Ledger, LedgerInfo},
+    vec, Bytes, Env,
+};
+
+use crate::{Contract, ContractClient};
+
+#[test]
+fn random_value_sequence() {
+    let env = Env::default();
+    let contract_id = env.register_contract(None, Contract);
+    let client = ContractClient::new(&env, &contract_id);
+
+    assert_eq!(client.generate_random_value(&10), 6);
+    assert_eq!(client.generate_random_value(&10), 5);
+    assert_eq!(client.generate_random_value(&10), 8);
+    assert_eq!(client.generate_random_value(&10), 8);
+    assert_eq!(client.generate_random_value(&10), 4);
+}

test-cases/insufficiently-random-values/insufficiently-random-values-1/vulnerable-example/Cargo.toml

@@ -0,0 +1,36 @@
+
+[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
+
+[workspace.metadata.dylint]
+libraries = [
+    { path = "/home/tiki/trabajo/scout-soroban/detectors/insufficiently-random-values" },
+]

test-cases/insufficiently-random-values/insufficiently-random-values-1/vulnerable-example/src/lib.rs

@@ -0,0 +1,36 @@
+#![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<u64, Error> {
+        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<u32, Error> {
+        if max_val == 0 {
+            Err(Error::MaxValZero)
+        } else {
+            let val = env.ledger().sequence() % max_val;
+            Ok(val)
+        }
+    }
+}
+
+#[cfg(test)]
+mod test;

test-cases/insufficiently-random-values/insufficiently-random-values-1/vulnerable-example/src/test.rs

@@ -0,0 +1,59 @@
+extern crate std;
+
+use soroban_sdk::{
+    testutils::{Ledger, LedgerInfo},
+    Env,
+};
+
+use crate::{Contract, ContractClient};
+
+#[test]
+fn random_value_sequence() {
+    let env = Env::default();
+    let contract_id = env.register_contract(None, Contract);
+    let client = ContractClient::new(&env, &contract_id);
+
+    let mut ledger = LedgerInfo::default();
+    ledger.sequence_number = 0;
+    env.ledger().set(ledger.clone());
+
+    let mut random = client.generate_random_value_sequence(&10);
+    assert_eq!(random, 0);
+
+    ledger.sequence_number = 1;
+    env.ledger().set(ledger.clone());
+
+    random = client.generate_random_value_sequence(&10);
+    assert_eq!(random, 1);
+
+    ledger.sequence_number = 11;
+    env.ledger().set(ledger.clone());
+
+    random = client.generate_random_value_sequence(&10);
+    assert_eq!(random, 1);
+}
+#[test]
+fn random_value_timestamp() {
+    let env = Env::default();
+    let contract_id = env.register_contract(None, Contract);
+    let client = ContractClient::new(&env, &contract_id);
+
+    let mut ledger = LedgerInfo::default();
+    ledger.timestamp = 0;
+    env.ledger().set(ledger.clone());
+
+    let mut random = client.generate_random_value_timestamp(&10);
+    assert_eq!(random, 0);
+
+    ledger.timestamp = 1;
+    env.ledger().set(ledger.clone());
+
+    random = client.generate_random_value_timestamp(&10);
+    assert_eq!(random, 1);
+
+    ledger.timestamp = 11;
+    env.ledger().set(ledger.clone());
+
+    random = client.generate_random_value_timestamp(&10);
+    assert_eq!(random, 1);
+}