Merge branch 'main' into 4-dos-unbounded-operation

.github/workflows/test-detectors.yml

@@ -97,11 +97,12 @@ jobs:
           - macos-latest
         test:
           [
-            "core-mem-forget",
+            "avoid-core-mem-forget",
             "divide-before-multiply",
             "dos-unbounded-operation",
             "insufficiently-random-values",
             "overflow-check",
+            "set-contract-storage",
             "unprotected-update-current-contract-wasm",
             "unsafe-expect",
             "unsafe-unwrap",

detectors/avoid-core-mem-forget/Cargo.toml

@@ -0,0 +1,20 @@
+[package]
+name = "avoid-core-mem-forget"
+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/core-mem-forget/src/lib.rs → detectors/avoid-core-mem-forget/src/lib.rs

@@ -17,32 +17,42 @@ dylint_linting::impl_pre_expansion_lint! {
     /// This is a bad practice because it can lead to memory leaks, resource leaks and logic errors.
     /// ### Example
     /// ```rust
-    /// pub fn forget_something(n: WithoutCopy) -> u64 {
-    ///     core::mem::forget(n);
-    ///     0
-    /// }
-    /// ```
+    ///    pub fn forget_value(&mut self) {
+    ///        let forgotten_value = self.value;
+    ///        self.value = false;
+    ///        core::mem::forget(forgotten_value);
+    ///    }
     ///
+    ///     ```
     /// Use instead:
-    /// ```rust
-    /// pub fn forget_something(n: WithoutCopy) -> u64 {
-    ///     let _ = n;
-    ///     0
-    /// }
-    /// ```
+    ///```rust
+    ///    pub fn forget_value(&mut self) {
+    ///        let forgotten_value = self.value;
+    ///        self.value = false;
+    ///        let _ = forgotten_value;
+    ///    }
+    ///
+    /// // or use drop if droppable
+    ///
+    ///    pub fn drop_value(&mut self) {
+    ///        let forgotten_value = self.value;
+    ///        self.value = false;
+    ///        forget_value.drop();
+    ///    }
+    ///```
 
-    pub CORE_MEM_FORGET,
+    pub AVOID_CORE_MEM_FORGET,
     Warn,
-    Detector::CoreMemForget.get_lint_message(),
-    CoreMemForget::default()
+    Detector::AvoidCoreMemForget.get_lint_message(),
+    AvoidCoreMemForget::default()
 }
 
 #[derive(Default)]
-pub struct CoreMemForget {
+pub struct AvoidCoreMemForget {
     stack: Vec<NodeId>,
 }
 
-impl EarlyLintPass for CoreMemForget {
+impl EarlyLintPass for AvoidCoreMemForget {
     fn check_item(&mut self, _cx: &EarlyContext, item: &Item) {
         if self.in_test_item() || is_test_item(item) {
             self.stack.push(item.id);
@@ -59,9 +69,10 @@ impl EarlyLintPass for CoreMemForget {
             if path.segments[1].ident.name.to_string() == "mem";
             if path.segments[2].ident.name.to_string() == "forget";
             then {
-                Detector::CoreMemForget.span_lint_and_help(
+
+                Detector::AvoidCoreMemForget.span_lint_and_help(
                     cx,
-                    CORE_MEM_FORGET,
+                    AVOID_CORE_MEM_FORGET,
                     expr.span,
                     "Instead, use the `let _ = ...` pattern or `.drop` method to forget the value.",
                 );
@@ -91,7 +102,7 @@ fn is_test_item(item: &Item) -> bool {
     })
 }
 
-impl CoreMemForget {
+impl AvoidCoreMemForget {
     fn in_test_item(&self) -> bool {
         !self.stack.is_empty()
     }

detectors/core-mem-forget/Cargo.toml → detectors/set-contract-storage/Cargo.toml

@@ -1,5 +1,5 @@
 [package]
-name = "core-mem-forget"
+name = "set-contract-storage"
 version = "0.1.0"
 edition = "2021"
 

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

@@ -26,11 +26,12 @@ use strum::{Display, EnumIter};
 #[derive(Debug, Display, Clone, EnumIter, PartialEq, Eq, Hash)]
 #[strum(serialize_all = "kebab-case")]
 pub enum Detector {
-    CoreMemForget,
+    AvoidCoreMemForget,
     DivideBeforeMultiply,
     DosUnboundedOperation,
     InsufficientlyRandomValues,
     OverflowCheck,
+    SetContractStorage,
     UnprotectedUpdateCurrentContractWasm,
     UnsafeExpect,
     UnsafeUnwrap,
@@ -40,11 +41,12 @@ impl Detector {
     /// Returns the lint message for the detector.
     pub const fn get_lint_message(&self) -> &'static str {
         match self {
-            Detector::CoreMemForget => CORE_MEM_FORGET_LINT_MESSAGE,
+            Detector::AvoidCoreMemForget => AVOID_CORE_MEM_FORGET_LINT_MESSAGE,
             Detector::DivideBeforeMultiply => DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE,
             Detector::DosUnboundedOperation => DOS_UNBOUNDED_OPERATION_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

@@ -1,11 +1,13 @@
-pub const CORE_MEM_FORGET_LINT_MESSAGE: &str =
+pub const AVOID_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 DOS_UNBOUNDED_OPERATION_LINT_MESSAGE: &str =
-"In order to prevent a single transaction from consuming all the gas in a block, unbounded operations must be avoided";
-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";
+        "In order to prevent a single transaction from consuming all the gas in a block, unbounded operations must be avoided";
+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/avoid-core-mem-forget/avoid-core-mem-forget-1/remediated-example/Cargo.toml

@@ -0,0 +1,30 @@
+[package]
+name = "avoid-core-mem-forget-1-remediated"
+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/core-mem-forget/core-mem-forget-1/remediated-example/src/lib.rs → test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1/remediated-example/src/lib.rs

@@ -2,7 +2,7 @@
 use soroban_sdk::{contract, contractimpl, contracttype};
 
 #[contract]
-pub struct CoreMemForget;
+pub struct AvoidCoreMemForget;
 
 #[contracttype]
 #[derive(Eq, PartialEq)]
@@ -12,7 +12,7 @@ pub struct WithoutCopy {
 }
 
 #[contractimpl]
-impl CoreMemForget {
+impl AvoidCoreMemForget {
     pub fn forget_something(n: WithoutCopy) -> u64 {
         let _ = n;
         0
@@ -25,13 +25,10 @@ mod tests {
 
     #[test]
     fn test_forget_something() {
-        // Given
         let test_value: WithoutCopy = WithoutCopy { a: 80, b: 60 };
 
-        // When
-        let result = CoreMemForget::forget_something(test_value);
+        let result = AvoidCoreMemForget::forget_something(test_value);
 
-        // Then
         assert_eq!(result, 0);
     }
 }

test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1/vulnerable-example/Cargo.toml

@@ -0,0 +1,30 @@
+[package]
+name = "avoid-core-mem-forget-1-vulnerable"
+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/core-mem-forget/core-mem-forget-1/vulnerable-example/src/lib.rs → test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1/vulnerable-example/src/lib.rs

@@ -2,7 +2,7 @@
 use soroban_sdk::{contract, contractimpl, contracttype};
 
 #[contract]
-pub struct CoreMemForget;
+pub struct AvoidCoreMemForget;
 
 #[contracttype]
 #[derive(Eq, PartialEq)]
@@ -12,7 +12,7 @@ pub struct WithoutCopy {
 }
 
 #[contractimpl]
-impl CoreMemForget {
+impl AvoidCoreMemForget {
     pub fn forget_something(n: WithoutCopy) -> u64 {
         core::mem::forget(n);
         0
@@ -25,13 +25,10 @@ mod tests {
 
     #[test]
     fn test_forget_something() {
-        // Given
         let test_value: WithoutCopy = WithoutCopy { a: 80, b: 60 };
 
-        // When
-        let result = CoreMemForget::forget_something(test_value);
+        let result = AvoidCoreMemForget::forget_something(test_value);
 
-        // Then
         assert_eq!(result, 0);
     }
 }

test-cases/core-mem-forget/core-mem-forget-1/vulnerable-example/Cargo.toml → test-cases/set-contract-storage/set-contract-storage-1/remediated-example/Cargo.toml

@@ -1,5 +1,5 @@
 [package]
-name = "core-mem-forget-1-vulnerable"
+name = "set-contract-storage-remediated-1"
 version = "0.1.0"
 edition = "2021"