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

.github/workflows/test-detectors.yml

@@ -98,6 +98,7 @@ jobs:
         test:
           [
             "avoid-core-mem-forget",
+            "avoid-panic-error",
             "divide-before-multiply",
             "dos-unbounded-operation",
             "insufficiently-random-values",

README.md

@@ -44,7 +44,8 @@ cargo scout-audit
 | [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    |
-
+| [avoid-core-mem-forget](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-core-mem-forget)                 | The use of `core::mem::forget()` could lead to memory leaks and logic errors.                                                | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1)        
+| [set-contract-storage](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/set-contract-storage)                           | Insufficient access control on `env.storage()` method.                                                         | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/set-contract-storage/set-contract-storage-3)                                                                                                                                                | Critical    |
 
 ## Tests
 

detectors/avoid-core-mem-forget/README.md

@@ -0,0 +1,31 @@
+# Avoid core::mem::forget usage
+
+### What it does
+
+Checks for `core::mem::forget` usage.
+
+### Why is this bad?
+
+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
+}
+```
+
+Use instead:
+
+```rust
+pub fn forget_something(n: WithoutCopy) -> u64 {
+    let _ = n;
+    0
+}
+```
+
+### Implementation
+
+The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-core-mem-forget).

detectors/avoid-panic-error/Cargo.toml

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

@@ -0,0 +1,187 @@
+#![feature(rustc_private)]
+#![warn(unused_extern_crates)]
+
+extern crate rustc_ast;
+extern crate rustc_span;
+
+use clippy_utils::sym;
+use if_chain::if_chain;
+use rustc_ast::{
+    ptr::P,
+    token::{LitKind, TokenKind},
+    tokenstream::{TokenStream, TokenTree},
+    AttrArgs, AttrKind, Expr, ExprKind, Item, MacCall, StmtKind,
+};
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_span::{sym, Span};
+use scout_audit_internal::Detector;
+
+dylint_linting::impl_pre_expansion_lint! {
+    /// ### What it does
+    /// The panic! macro is used to stop execution when a condition is not met.
+    /// This is useful for testing and prototyping, but should be avoided in production code
+    ///
+    /// ### Why is this bad?
+    /// The usage of panic! is not recommended because it will stop the execution of the caller contract.
+    ///
+    /// ### Known problems
+    /// While this linter detects explicit calls to panic!, there are some ways to raise a panic such as unwrap() or expect().
+    ///
+    /// ### Example
+    /// ```rust
+    /// pub fn add(env: Env, value: u32) -> u32 {
+    ///     let storage = env.storage().instance();
+    ///     let mut count: u32 = storage.get(&COUNTER).unwrap_or(0);
+    ///     match count.checked_add(value) {
+    ///         Some(value) => count = value,
+    ///         None => panic!("Overflow error"),
+    ///     }
+    ///     storage.set(&COUNTER, &count);
+    ///     storage.extend_ttl(100, 100);
+    ///     count
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// pub fn add(env: Env, value: u32) -> Result<u32, Error> {
+    ///     let storage = env.storage().instance();
+    ///     let mut count: u32 = storage.get(&COUNTER).unwrap_or(0);
+    ///     match count.checked_add(value) {
+    ///         Some(value) => count = value,
+    ///         None => return Err(Error::OverflowError),
+    ///     }
+    ///     storage.set(&COUNTER, &count);
+    ///     storage.extend_ttl(100, 100);
+    ///     Ok(count)
+    /// }
+    /// ```
+    pub AVOID_PANIC_ERROR,
+    Warn,
+    Detector::AvoidPanicError.get_lint_message(),
+    AvoidPanicError::default()
+}
+
+#[derive(Default)]
+pub struct AvoidPanicError {
+    in_test_span: Option<Span>,
+}
+
+impl EarlyLintPass for AvoidPanicError {
+    fn check_item(&mut self, _cx: &EarlyContext, item: &Item) {
+        match (is_test_item(item), self.in_test_span) {
+            (true, None) => self.in_test_span = Some(item.span),
+            (true, Some(test_span)) => {
+                if !test_span.contains(item.span) {
+                    self.in_test_span = Some(item.span);
+                }
+            }
+            (false, None) => {}
+            (false, Some(test_span)) => {
+                if !test_span.contains(item.span) {
+                    self.in_test_span = None;
+                }
+            }
+        };
+    }
+
+    fn check_stmt(&mut self, cx: &EarlyContext<'_>, stmt: &rustc_ast::Stmt) {
+        if_chain! {
+            if !self.in_test_item();
+            if let StmtKind::MacCall(mac) = &stmt.kind;
+            then {
+                check_macro_call(cx, stmt.span, &mac.mac)
+            }
+        }
+    }
+
+    fn check_expr(&mut self, cx: &EarlyContext, expr: &Expr) {
+        if_chain! {
+            if !self.in_test_item();
+            if let ExprKind::MacCall(mac) = &expr.kind;
+            then {
+                check_macro_call(cx, expr.span, mac)
+            }
+        }
+    }
+}
+
+fn check_macro_call(cx: &EarlyContext, span: Span, mac: &P<MacCall>) {
+    if_chain! {
+        if mac.path == sym!(panic);
+        if let [TokenTree::Token(token, _)] = mac
+            .args
+            .tokens
+            .clone()
+            .trees()
+            .collect::<Vec<_>>()
+            .as_slice();
+        if let TokenKind::Literal(lit) = token.kind;
+        if lit.kind == LitKind::Str;
+        then {
+            Detector::AvoidPanicError.span_lint_and_help(
+                cx,
+                AVOID_PANIC_ERROR,
+                span,
+                &format!("You could use instead an Error enum and then 'return Err(Error::{})'", capitalize_err_msg(lit.symbol.as_str()).replace(' ', "")),
+            );
+        }
+    }
+}
+
+fn is_test_item(item: &Item) -> bool {
+    item.attrs.iter().any(|attr| {
+        // Find #[cfg(all(test, feature = "e2e-tests"))]
+        if_chain!(
+            if let AttrKind::Normal(normal) = &attr.kind;
+            if let AttrArgs::Delimited(delim_args) = &normal.item.args;
+            if is_test_token_present(&delim_args.tokens);
+            then {
+                return true;
+            }
+        );
+
+        // Find unit or integration tests
+        if attr.has_name(sym::test) {
+            return true;
+        }
+
+        if_chain! {
+            if attr.has_name(sym::cfg);
+            if let Some(items) = attr.meta_item_list();
+            if let [item] = items.as_slice();
+            if let Some(feature_item) = item.meta_item();
+            if feature_item.has_name(sym::test);
+            then {
+                return true;
+            }
+        }
+
+        false
+    })
+}
+
+impl AvoidPanicError {
+    fn in_test_item(&self) -> bool {
+        self.in_test_span.is_some()
+    }
+}
+
+fn capitalize_err_msg(s: &str) -> String {
+    s.split_whitespace()
+        .map(|word| {
+            let mut chars = word.chars();
+            match chars.next() {
+                None => String::new(),
+                Some(f) => f.to_uppercase().collect::<String>() + chars.as_str(),
+            }
+        })
+        .collect::<Vec<String>>()
+        .join(" ")
+}
+
+fn is_test_token_present(token_stream: &TokenStream) -> bool {
+    token_stream.trees().any(|tree| match tree {
+        TokenTree::Token(token, _) => token.is_ident_named(sym::test),
+        TokenTree::Delimited(_, _, token_stream) => is_test_token_present(token_stream),
+    })
+}

detectors/set-contract-storage/README.md

@@ -0,0 +1,34 @@
+# Set contract storage
+
+### 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();
+}
+```
+
+### Implementation
+
+The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/set-contract-storage).

scout-audit-internal/src/detector.rs

@@ -27,6 +27,7 @@ use strum::{Display, EnumIter};
 #[strum(serialize_all = "kebab-case")]
 pub enum Detector {
     AvoidCoreMemForget,
+    AvoidPanicError,
     DivideBeforeMultiply,
     DosUnboundedOperation,
     InsufficientlyRandomValues,
@@ -42,6 +43,7 @@ impl Detector {
     pub const fn get_lint_message(&self) -> &'static str {
         match self {
             Detector::AvoidCoreMemForget => AVOID_CORE_MEM_FORGET_LINT_MESSAGE,
+            Detector::AvoidPanicError => AVOID_PANIC_ERROR_LINT_MESSAGE,
             Detector::DivideBeforeMultiply => DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE,
             Detector::DosUnboundedOperation => DOS_UNBOUNDED_OPERATION_LINT_MESSAGE,
             Detector::InsufficientlyRandomValues => INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE,

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

@@ -1,5 +1,6 @@
 pub const AVOID_CORE_MEM_FORGET_LINT_MESSAGE: &str =
     "Use the `let _ = ...` pattern or `.drop()` method to forget the value";
+pub const AVOID_PANIC_ERROR_LINT_MESSAGE: &str = "The panic! macro is used to stop execution when a condition is not met. Even when this does not break the execution of the contract, it is recommended to use Result instead of panic! because it will stop the execution of the caller contract";
 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 =

test-cases/README.md

@@ -118,3 +118,31 @@ and has a Critical severity.
 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.
+
+### Avoid core::mem::forget
+
+The `core::mem::forget` function is used to forget about a value without running its destructor. This could lead to memory leaks and logic errors.
+
+We classified this issue, a deviation from best practices which could have
+security implications, under the [Best practices](#vulnerability-categories) category and assigned it an Enhancement severity.
+
+### Set contract storage
+
+Smart contracts can store important information in memory which changes through the contract's lifecycle. Changes happen via user interaction with the smart contract. An _unauthorized_ set contract storage vulnerability happens when a smart contract call allows a user to set or modify contract memory when they were not supposed to be authorized.
+
+Common practice is to have functions with the ability to change
+security-relevant values in memory to be only accessible to specific roles,
+e.g, only an admin can call the function `reset()` which resets auction values.
+When this does not happen, arbitrary users may alter memory which may impose
+great damage to the smart contract users.
+
+In `Soroban`, the method `env.storage()` can be used
+to modify the contract storage under a given key. When a smart contract uses
+this method, the contract needs to check if the caller should be able to
+alter this storage. If this does not happen, an arbitary caller may modify
+balances and other relevant contract storage.
+
+We classified this type of vulnerability under
+the [Authorization](#vulnerability-categories) category and assigned it a
+Critical severity.
+

test-cases/avoid-panic-error/avoid-panic-error-1/remediated-example/Cargo.toml

@@ -0,0 +1,30 @@
+[package]
+name = "avoid-panic-error-remediated-1"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+soroban-sdk = { version = "=20.0.0" }
+
+[dev_dependencies]
+soroban-sdk = { version = "=20.0.0", 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