Merge branch 'main' into core-mem-forget

.github/workflows/test-detectors.yml

@@ -99,6 +99,7 @@ jobs:
           [
             "core-mem-forget",
             "divide-before-multiply",
+            "insufficiently-random-values",
             "overflow-check",
             "unprotected-update-current-contract-wasm",
             "unsafe-expect",

README.md

@@ -25,7 +25,7 @@ cargo install cargo-dylint dylint-link
 Afterwards, install Scout with the following command:
 
 ```bash
-cargo install cargo-scout-audit
+cargo install --path apps/cargo-scout-audit
 ```
 
 To run Scout on your project, navigate to its root directory and execute the following command:
@@ -42,6 +42,8 @@ cargo scout-audit
 | [unsafe-unwrap](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-unwrap)      | Inappropriate usage of the unwrap method, causing unexpected program crashes.    | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-unwrap/unsafe-unwrap-1)| Medium    |
 | [unsafe-expect](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-expect)       | Improper usage of the expect method, leading to unexpected program crashes.    | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-expect/unsafe-expect-1)| Medium    |
 | [overflow-check](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/overflow-check)       | An arithmetic operation overflows or underflows the available memory allocated to the variable.    | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/overflow-check/overflow-check-1)| Critical    |
+| [insufficiently-random-values](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/insufficiently-random-values)       | Avoid using block attributes for random number generation to prevent manipulation.    | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/insufficiently-random-values/insufficiently-random-values-1)| Critical    |
+| [unprotected-update-current-contract-wasm](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm)       | If users are allowed to call `update_current_contract_wasm()`, they can intentionally modify the contract behaviour.    | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1)| Critical    |
 
 
 ## Tests

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/README.md

@@ -0,0 +1,51 @@
+# Insuficciently random values
+
+### What it does
+Checks the usage of `ledger().timestamp()` or `ledger().sequence()` for generation of random numbers.
+
+### Why is this bad?
+Using `ledger().timestamp()` is not recommended because it could be potentially manipulated by validator. On the other hand, `ledger().sequence()` is publicly available, an attacker could predict the random number to be generated.
+
+### Example
+
+```rust
+#[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)
+        }
+    }
+}
+```
+
+Use instead:
+
+```rust
+#[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)
+        }
+    }
+}
+```
+
+### Implementation
+
+The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/insufficiently-random-values).

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

detectors/overflow-check/src/lib.rs

@@ -32,12 +32,20 @@ impl EarlyLintPass for OverflowCheck {
 
         let toml: toml::Value = toml::from_str(&cargo_toml).unwrap();
 
-        let profiles = toml.get("profile").and_then(|p| p.as_table());
-
-        if profiles.is_some() {
-            for profile in profiles.unwrap() {
+        if let Some(profiles) = toml.get("profile").and_then(|p| p.as_table()) {
+            for profile in profiles {
                 let profile_name = profile.0;
-                let table = profile.1.as_table();
+                let mut table = profile.1.as_table();
+                let mut temp_table;
+                if table.is_some() && table.unwrap().contains_key("inherits") {
+                    let parent_name = table.unwrap().get("inherits").unwrap().as_str().unwrap();
+                    if profiles.contains_key(parent_name) {
+                        let parent_table = profiles.get(parent_name).unwrap().as_table().unwrap();
+                        temp_table = parent_table.clone();
+                        temp_table.extend(table.unwrap().clone().into_iter());
+                        table = Some(&temp_table);
+                    }
+                }
                 if table.is_some() && table.unwrap().contains_key("overflow-checks") {
                     let has_overflow_check = table
                         .unwrap()

detectors/unprotected-update-current-contract-wasm/README.md

@@ -0,0 +1,56 @@
+# Unprotected update of current contract wasm
+
+### What it does
+
+It warns you if `update_current_contract_wasm()` function is called without a previous check of the address of the caller.
+
+### Why is this bad?
+
+If users are allowed to call `update_current_contract_wasm()`, they can intentionally modify the contract behaviour, leading to the loss of all associated data/tokens and functionalities given by this contract or by others that depend on it.
+
+### Example
+
+```rust
+#[contractimpl]
+impl UpgradeableContract {
+    pub fn init(e: Env, admin: Address) {
+        e.storage().instance().set(&DataKey::Admin, &admin);
+    }
+
+    pub fn version() -> u32 {
+        1
+    }
+
+    pub fn upgrade(e: Env, new_wasm_hash: BytesN<32>) {
+        let admin: Address = e.storage().instance().get(&DataKey::Admin).unwrap();
+
+        e.deployer().update_current_contract_wasm(new_wasm_hash);
+    }
+}
+``` 
+
+Use instead:
+
+```rust
+#[contractimpl]
+impl UpgradeableContract {
+    pub fn init(e: Env, admin: Address) {
+        e.storage().instance().set(&DataKey::Admin, &admin);
+    }
+
+    pub fn version() -> u32 {
+        1
+    }
+
+    pub fn upgrade(e: Env, new_wasm_hash: BytesN<32>) {
+        let admin: Address = e.storage().instance().get(&DataKey::Admin).unwrap();
+        admin.require_auth();
+
+        e.deployer().update_current_contract_wasm(new_wasm_hash);
+    }
+}
+```
+
+### Implementation
+
+The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm)

scout-audit-internal/src/detector.rs

@@ -28,6 +28,7 @@ use strum::{Display, EnumIter};
 pub enum Detector {
     CoreMemForget,
     DivideBeforeMultiply,
+    InsufficientlyRandomValues,
     OverflowCheck,
     UnprotectedUpdateCurrentContractWasm,
     UnsafeExpect,
@@ -40,12 +41,13 @@ impl Detector {
         match self {
             Detector::CoreMemForget => CORE_MEM_FORGET_LINT_MESSAGE,
             Detector::DivideBeforeMultiply => DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE,
-            Detector::UnsafeExpect => UNSAFE_EXPECT_LINT_MESSAGE,
+            Detector::InsufficientlyRandomValues => INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE,
             Detector::OverflowCheck => OVERFLOW_CHECK_LINT_MESSAGE,
             Detector::UnprotectedUpdateCurrentContractWasm => {
-                UNPROTECTED_UPDATE_CURRENT_CONTRACT_MESSAGE
+                UNPROTECTED_UPDATE_CURRENT_CONTRACT_LINT_MESSAGE
             }
-            Detector::UnsafeUnwrap => UNSAFE_UNWRAP_MESSAGE,
+            Detector::UnsafeExpect => UNSAFE_EXPECT_LINT_MESSAGE,
+            Detector::UnsafeUnwrap => UNSAFE_UNWRAP_LINT_MESSAGE,
         }
     }
 

scout-audit-internal/src/detector/lint_message.rs

@@ -2,8 +2,9 @@ pub const CORE_MEM_FORGET_LINT_MESSAGE: &str =
     "Use the `let _ = ...` pattern or `.drop()` method to forget the value";
 pub const DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE: &str =
     "Division before multiplication might result in a loss of precision";
+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 OVERFLOW_CHECK_LINT_MESSAGE: &str = "Use `overflow-checks = true` in Cargo.toml profile";
-pub const UNPROTECTED_UPDATE_CURRENT_CONTRACT_MESSAGE: &str =
+pub const UNPROTECTED_UPDATE_CURRENT_CONTRACT_LINT_MESSAGE: &str =
     "This update_current_contract_wasm is called without access control";
 pub const UNSAFE_EXPECT_LINT_MESSAGE: &str = "Unsafe usage of `expect`";
-pub const UNSAFE_UNWRAP_MESSAGE: &str = "Unsafe usage of `unwrap`";
+pub const UNSAFE_UNWRAP_LINT_MESSAGE: &str = "Unsafe usage of `unwrap`";

test-cases/README.md

@@ -104,3 +104,17 @@ realized if `overflow-checks` is set to `False` in the `[profile.release]` secti
 Notwithstanding, there are contexts where developers do turn off checks for
 valid reasons and hence the reason for including this vulnerability in the
 list.
+
+
+### Insufficiently random values
+
+Using block attributes like ledger `timestamp()` and ledger `sequence()` for random number generation in Soroban smart contracts is not recommended due to the predictability of these values. Block attributes are publicly visible and deterministic, making it easy for malicious actors to anticipate their values and manipulate outcomes to their advantage. Furthermore, validators could potentially influence these attributes, further exacerbating the risk of manipulation. For truly random number generation, it's important to use a source that is both unpredictable and external to the blockchain environment, reducing the potential for malicious exploitation.
+
+This vulnerability again falls under the [Block attributes](#vulnerability-categories) category
+and has a Critical severity.
+
+### Unprotected update of current contract wasm
+
+If users are allowed to call `update_current_contract_wasm()`, they can intentionally modify the contract behaviour, leading to the loss of all associated data/tokens and functionalities given by this contract or by others that depend on it. To prevent this, the function should be restricted to administrators or authorized users only.
+
+This vulnerability falls under the [Authorization](#vulnerability-categories) category and has a Critical severity.

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