Merge pull request #41 from CoinFabrik/insufficiently-random-values

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

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

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

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