12 set contract storage (#48)

* Add set-contract-storage detector and test cases

.github/workflows/test-detectors.yml

@@ -101,6 +101,7 @@ jobs:
             "divide-before-multiply",
             "insufficiently-random-values",
             "overflow-check",
+            "set-contract-storage",
             "unprotected-update-current-contract-wasm",
             "unsafe-expect",
             "unsafe-unwrap",

detectors/set-contract-storage/Cargo.toml

@@ -0,0 +1,20 @@
+[package]
+name = "set-contract-storage"
+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/set-contract-storage/src/lib.rs

@@ -0,0 +1,89 @@
+#![feature(rustc_private)]
+#![warn(unused_extern_crates)]
+
+extern crate rustc_hir;
+extern crate rustc_span;
+
+use rustc_hir::def_id::LocalDefId;
+use rustc_hir::intravisit::Visitor;
+use rustc_hir::intravisit::{walk_expr, FnKind};
+use rustc_hir::{Body, FnDecl};
+use rustc_hir::{Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_span::Span;
+use scout_audit_internal::Detector;
+
+dylint_linting::declare_late_lint! {
+    /// ### What it does
+    /// Checks for calls to env.storage() without a prior call to env.require_auth()
+    ///
+    /// ### Why is this bad?
+    /// Functions using keys as variables without proper access control or input sanitation can allow users to perform changes in arbitrary memory locations.
+    ///
+    /// ### Known problems
+    /// Only check the function call, so false positives could result.
+    ///
+    /// ### Example
+    /// ```rust
+    /// fn set_contract_storage(env: Env) {
+    ///   let _storage = env.storage().instance();
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// fn set_contract_storage(env: Env, user: Address) {
+    ///   user.require_auth();
+    ///   let _storage = env.storage().instance();
+    /// }
+    /// ```
+    pub SET_STORAGE_WARN,
+    Warn,
+    Detector::SetContractStorage.get_lint_message()
+}
+
+impl<'tcx> LateLintPass<'tcx> for SetStorageWarn {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        _: FnKind<'tcx>,
+        _: &'tcx FnDecl<'_>,
+        body: &'tcx Body<'_>,
+        _: Span,
+        _: LocalDefId,
+    ) {
+        struct SetContractStorageVisitor {
+            found_auth: bool,
+            storage_without_auth: Vec<Span>,
+        }
+
+        impl<'tcx> Visitor<'tcx> for SetContractStorageVisitor {
+            fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
+                if let ExprKind::MethodCall(path, _, _, _) = &expr.kind {
+                    let method_name = path.ident.name.as_str();
+                    if method_name == "require_auth" {
+                        self.found_auth = true;
+                    } else if method_name == "storage" && !self.found_auth {
+                        self.storage_without_auth.push(expr.span);
+                    }
+                }
+                walk_expr(self, expr);
+            }
+        }
+
+        let mut visitor = SetContractStorageVisitor {
+            found_auth: false,
+            storage_without_auth: Vec::new(),
+        };
+
+        walk_expr(&mut visitor, body.value);
+
+        for span in visitor.storage_without_auth {
+            Detector::SetContractStorage.span_lint_and_help(
+                cx,
+                SET_STORAGE_WARN,
+                span,
+                "Ensure that the caller is authorized to use storage",
+            );
+        }
+    }
+}

scout-audit-internal/src/detector.rs

@@ -30,6 +30,7 @@ pub enum Detector {
     DivideBeforeMultiply,
     InsufficientlyRandomValues,
     OverflowCheck,
+    SetContractStorage,
     UnprotectedUpdateCurrentContractWasm,
     UnsafeExpect,
     UnsafeUnwrap,
@@ -43,6 +44,7 @@ impl Detector {
             Detector::DivideBeforeMultiply => DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE,
             Detector::InsufficientlyRandomValues => INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE,
             Detector::OverflowCheck => OVERFLOW_CHECK_LINT_MESSAGE,
+            Detector::SetContractStorage => SET_CONTRACT_STORAGE_LINT_MESSAGE,
             Detector::UnprotectedUpdateCurrentContractWasm => {
                 UNPROTECTED_UPDATE_CURRENT_CONTRACT_LINT_MESSAGE
             }

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

@@ -4,6 +4,7 @@ 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 SET_CONTRACT_STORAGE_LINT_MESSAGE:&str = "Abitrary users should not have control over keys because it implies writing any value of left mapping, lazy variable, or the main struct of the contract located in position 0 of the storage";
 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`";

test-cases/set-contract-storage/set-contract-storage-1/remediated-example/Cargo.toml

@@ -0,0 +1,30 @@
+[package]
+name = "set-contract-storage-remediated-1"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+soroban-sdk = { version = "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/set-contract-storage/set-contract-storage-1/remediated-example/src/lib.rs

@@ -0,0 +1,48 @@
+#![no_std]
+use soroban_sdk::{contract, contractimpl, symbol_short, Address, Env, Symbol};
+
+const COUNTER: Symbol = symbol_short!("COUNTER");
+
+#[contract]
+pub struct SetContractStorage;
+
+#[contractimpl]
+impl SetContractStorage {
+    /// Increment an internal counter; return the new value.
+    pub fn increment(env: Env, user: Address) -> u32 {
+        user.require_auth();
+        let storage = env.storage().instance();
+        let mut count: u32 = storage.get(&COUNTER).unwrap_or(0);
+        count += 1;
+        storage.set(&COUNTER, &count);
+        storage.bump(100, 100);
+        count
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use soroban_sdk::{testutils, Address, Env};
+
+    use crate::{SetContractStorage, SetContractStorageClient};
+
+    #[test]
+    fn increment() {
+        // Given
+        let env = Env::default();
+        let contract_id = env.register_contract(None, SetContractStorage);
+        let client = SetContractStorageClient::new(&env, &contract_id);
+        env.mock_all_auths();
+        let user = <Address as testutils::Address>::random(&env);
+
+        // When
+        let first_increment = client.increment(&user);
+        let second_increment = client.increment(&user);
+        let third_increment = client.increment(&user);
+
+        // Then
+        assert_eq!(first_increment, 1);
+        assert_eq!(second_increment, 2);
+        assert_eq!(third_increment, 3);
+    }
+}

test-cases/set-contract-storage/set-contract-storage-1/vulnerable-example/Cargo.toml

@@ -0,0 +1,30 @@
+[package]
+name = "set-contract-storage-vulnerable-1"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+soroban-sdk = { version = "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/set-contract-storage/set-contract-storage-1/vulnerable-example/src/lib.rs

@@ -0,0 +1,46 @@
+#![no_std]
+
+use soroban_sdk::{contract, contractimpl, symbol_short, Env, Symbol};
+
+const COUNTER: Symbol = symbol_short!("COUNTER");
+
+#[contract]
+pub struct SetContractStorage;
+
+#[contractimpl]
+impl SetContractStorage {
+    /// Increment an internal counter; return the new value.
+    pub fn increment(env: Env) -> u32 {
+        let storage = env.storage().instance();
+        let mut count: u32 = storage.get(&COUNTER).unwrap_or(0);
+        count += 1;
+        storage.set(&COUNTER, &count);
+        storage.bump(100, 100);
+        count
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use soroban_sdk::Env;
+
+    use crate::{SetContractStorage, SetContractStorageClient};
+
+    #[test]
+    fn increment() {
+        // Given
+        let env = Env::default();
+        let contract_id = env.register_contract(None, SetContractStorage);
+        let client = SetContractStorageClient::new(&env, &contract_id);
+
+        // When
+        let first_increment = client.increment();
+        let second_increment = client.increment();
+        let third_increment = client.increment();
+
+        // Then
+        assert_eq!(first_increment, 1);
+        assert_eq!(second_increment, 2);
+        assert_eq!(third_increment, 3);
+    }
+}

test-cases/set-contract-storage/set-contract-storage-2/remediated-example/Cargo.toml

@@ -0,0 +1,30 @@
+[package]
+name = "set-contract-storage-remediated-1"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+soroban-sdk = { version = "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/set-contract-storage/set-contract-storage-2/remediated-example/src/lib.rs

@@ -0,0 +1,48 @@
+#![no_std]
+use soroban_sdk::{contract, contractimpl, symbol_short, Address, Env, Symbol};
+
+const COUNTER: Symbol = symbol_short!("COUNTER");
+
+#[contract]
+pub struct SetContractStorage;
+
+#[contractimpl]
+impl SetContractStorage {
+    /// Increment an internal counter; return the new value.
+    pub fn increment(env: Env, user: Address) -> u32 {
+        user.require_auth();
+        let storage = env.storage().temporary();
+        let mut count: u32 = storage.get(&COUNTER).unwrap_or(0);
+        count += 1;
+        storage.set(&COUNTER, &count);
+        storage.bump(&COUNTER, 100, 100);
+        count
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use soroban_sdk::{testutils, Address, Env};
+
+    use crate::{SetContractStorage, SetContractStorageClient};
+
+    #[test]
+    fn increment() {
+        // Given
+        let env = Env::default();
+        let contract_id = env.register_contract(None, SetContractStorage);
+        let client = SetContractStorageClient::new(&env, &contract_id);
+        env.mock_all_auths();
+        let user = <Address as testutils::Address>::random(&env);
+
+        // When
+        let first_increment = client.increment(&user);
+        let second_increment = client.increment(&user);
+        let third_increment = client.increment(&user);
+
+        // Then
+        assert_eq!(first_increment, 1);
+        assert_eq!(second_increment, 2);
+        assert_eq!(third_increment, 3);
+    }
+}