Push main stuff

.github/workflows/test-detectors.yml

@@ -109,6 +109,7 @@ jobs:
             "unprotected-update-current-contract-wasm",
             "unsafe-expect",
             "unsafe-unwrap",
+            "unused-return-enum",
           ]
     runs-on: ${{ matrix.os }}
     steps:

README.md

@@ -50,12 +50,13 @@ Install Scout from the Marketplace within the Extensions tab of Visual Studio Co
 | [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)  | Enhancement    |        
+| [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)  | Enhancement    |
 | [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    |
 | [avoid-panic-error](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-panic-error)                           | Code panics on error instead of using descriptive enum.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-panic-error/avoid-panic-error-1)                                                                                                                                              | Enhancement    |
 | [avoid-unsafe-block](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-unsafe-block)                           | Using unsafe blocks in risks code safety and reliability.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1)                                                                                                                                              | Critical   |
 | [dos-unbounded-operation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unbounded-operation)                           | DoS due to unbounded operation. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-3)                                                                                                                                              | Medium  |
 | [soroban-version](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/soroban-version)                           | Using a pinned version of Soroban can be dangerous, as it may have bugs or security issues. Use the latest version available. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/soroban-version/soroban-version-1)                 | Enhancement  |
+| [unused-return-enum](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unused-return-enum) | Return enum from a function is not completely used. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unused-return-enum/unused-return-enum-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unused-return-enum/unused-return-enum-2) | Minor |
 | [unprotected-mapping-operation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-mapping-opereation)                           | Modifying mappings with an arbitrary key given by the user could lead to unintented modifications of critical data, modifying data belonging to other users, causing denial of service, unathorized access, and other potential issues. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-mapping-opereation/unprotected-mapping-opereation-1)                 | Critical  |
 
 ## CLI Options

detectors/test-hashmap/Cargo.toml

@@ -0,0 +1,21 @@
+[package]
+name = "test-hashmap"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+dylint_linting = { workspace = true }
+semver = "1.0.4"
+serde_json = "1.0"
+ureq = { version = "2.7.1", features = ["json"] }
+
+scout-audit-internal = { workspace = true }
+
+[dev-dependencies]
+dylint_testing = { workspace = true }
+
+[package.metadata.rust-analyzer]
+rustc_private = true

detectors/test-hashmap/src/lib.rs

@@ -0,0 +1,125 @@
+#![feature(rustc_private)]
+
+extern crate rustc_ast;
+extern crate rustc_data_structures;
+extern crate rustc_hir;
+extern crate rustc_lint;
+extern crate rustc_session;
+extern crate rustc_span;
+
+use std::collections::HashMap;
+
+use rustc_ast::{
+    visit::walk_expr as walk_ast_expr, visit::walk_item, visit::Visitor as AstVisitor, Item,
+    ItemKind,
+};
+use rustc_hir::{
+    def_id::LocalDefId,
+    intravisit::{walk_body, walk_expr, FnKind, Visitor},
+    Body, Expr, ExprKind, FnDecl, ImplItemKind, QPath,
+};
+use rustc_lint::{EarlyLintPass, LateContext, LateLintPass};
+use rustc_session::{declare_lint, impl_lint_pass};
+use rustc_span::Span;
+
+dylint_linting::dylint_library!();
+
+declare_lint! {
+    pub TEST_HASMAP,
+    Warn,
+    "Warns about the use of the `hasmap` function"
+}
+
+pub struct TestHasmap;
+
+pub struct TestHasmapEarly;
+
+impl_lint_pass!(TestHasmap => [TEST_HASMAP]);
+impl_lint_pass!(TestHasmapEarly => [TEST_HASMAP]);
+
+pub struct TestHasmapFinder<'tcx, 'tcx_ref> {
+    // A map to track which functions call which other functions
+    cx: &'tcx_ref LateContext<'tcx>,
+    caller: String,
+    call_graph: HashMap<String, Vec<String>>,
+}
+
+#[allow(clippy::no_mangle_with_rust_abi)]
+#[no_mangle]
+pub fn register_lints(sess: &rustc_session::Session, lint_store: &mut rustc_lint::LintStore) {
+    // smoelius: Please keep the following `register_lints` calls sorted by crate name.
+    lint_store.register_late_pass(|_| Box::new(TestHasmap));
+    lint_store.register_early_pass(|| Box::new(TestHasmapEarly));
+}
+
+impl<'tcx> Visitor<'tcx> for TestHasmapFinder<'tcx, '_> {
+    fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
+        if let ExprKind::Call(mexpr, _) = &expr.kind {
+            if let ExprKind::Path(path) = &mexpr.kind {
+                if let QPath::TypeRelative(_, _) = &path {
+                    let def = self.cx.qpath_res(path, mexpr.hir_id);
+                    if let Some(def_id) = def.opt_def_id() {
+                        let func_name = self.cx.tcx.def_path_str(def_id);
+                        self.add_call_if_not_contains(self.caller.to_string(), func_name);
+                    }
+                }
+            }
+        }
+
+        walk_expr(self, expr);
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for TestHasmap {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        fn_kind: FnKind<'tcx>,
+        _: &'tcx FnDecl<'tcx>,
+        body: &'tcx Body<'tcx>,
+        _: Span,
+        def_id: LocalDefId,
+    ) {
+        // Return if the function is a method or comes from a macro
+        if !matches!(fn_kind, FnKind::Method(_, fnsig) if !fnsig.span.from_expansion()) {
+            return;
+        }
+
+        let fn_name = cx.tcx.def_path_str(def_id);
+        let mut visitor = TestHasmapFinder {
+            cx,
+            caller: fn_name.clone(),
+            call_graph: HashMap::new(),
+        };
+
+        // Initialize the function entry in the call graph
+        visitor.call_graph.entry(fn_name.clone()).or_default();
+
+        walk_body(&mut visitor, body);
+
+        println!("Function: {:?}", visitor.call_graph);
+    }
+}
+
+impl<'tcx> TestHasmapFinder<'tcx, '_> {
+    fn add_call_if_not_contains(&mut self, caller: String, callee: String) {
+        if let Some(calls) = self.call_graph.get_mut(&caller) {
+            if !calls.contains(&callee.to_string()) {
+                calls.push(callee.to_string());
+            }
+        }
+    }
+}
+
+impl EarlyLintPass for TestHasmapEarly {
+    fn check_expr(&mut self, _: &rustc_lint::EarlyContext<'_>, ex: &rustc_ast::Expr) {
+        walk_ast_expr(self, ex);
+    }
+}
+
+impl<'tcx> AstVisitor<'tcx> for TestHasmapEarly {
+    fn visit_expr(&mut self, ex: &'tcx rustc_ast::Expr) {
+        println!("Expr: {:?}", ex);
+        walk_ast_expr(self, ex);
+    }
+}

detectors/unused-return-enum/Cargo.toml

@@ -0,0 +1,16 @@
+[package]
+name = "unused-return-enum"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+[dependencies]
+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/unused-return-enum/README.md

@@ -0,0 +1,87 @@
+# Unused return enum
+
+### What it does
+
+It warns if a function that returns a Result type does not return the Result enum variant (Ok/Err).
+
+### Why is this bad?
+
+If any of the variants (Ok/Err) is not used, the code could be simplified or it could imply a bug.
+
+### Known problems
+
+If definitions of `Err()` and/or `Ok()` are in the code but do not flow to the return value due to the definition of a variable or because they are defined in a dead code block, the warning will not be shown. If the definitions are made in an auxiliary method, the warning will be shown, resulting in a false positive.
+
+### Example
+
+Instead of using:
+
+```rust
+#![no_std]
+
+use soroban_sdk::{contract, contracterror, contractimpl};
+
+#[contract]
+pub struct UnusedReturnEnum;
+
+#[contracterror]
+#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
+#[repr(u32)]
+pub enum Error {
+    /// An overflow was produced.
+    Overflow = 1,
+}
+
+#[contractimpl]
+impl UnusedReturnEnum {
+    /// Returns the percentage difference between two values.
+    pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result<u128, Error> {
+        let absolute_difference = balance1.abs_diff(balance2);
+        let sum = balance1 + balance2;
+
+        match 100u128.checked_mul(absolute_difference / sum) {
+            Some(result) => result,
+            None => panic!("Overflow"),
+        };
+
+        Err(Error::Overflow)
+    }
+}
+```
+
+Use this:
+
+```rust
+#![no_std]
+
+use soroban_sdk::{contract, contracterror, contractimpl, testutils::arbitrary::arbitrary::Result};
+
+#[contract]
+pub struct UnusedReturnEnum;
+
+#[contracterror]
+#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
+#[repr(u32)]
+pub enum Error {
+    /// An overflow was produced.
+    Overflow = 1,
+}
+
+#[contractimpl]
+impl UnusedReturnEnum {
+    /// Returns the percentage difference between two values.
+    pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result<u128, Error> {
+        let absolute_difference = balance1.abs_diff(balance2);
+        let sum = balance1 + balance2;
+
+        match 100u128.checked_mul(absolute_difference / sum) {
+            Some(result) => Ok(result),
+            None => Err(Error::Overflow),
+        }
+    }
+}
+```
+
+### Implementation
+
+The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unused-return-enum).

scout-audit-internal/src/detector.rs

@@ -39,6 +39,7 @@ pub enum Detector {
     UnprotectedUpdateCurrentContractWasm,
     UnsafeExpect,
     UnsafeUnwrap,
+    UnusedReturnEnum,
 }
 
 impl Detector {
@@ -60,6 +61,7 @@ impl Detector {
             }
             Detector::UnsafeExpect => UNSAFE_EXPECT_LINT_MESSAGE,
             Detector::UnsafeUnwrap => UNSAFE_UNWRAP_LINT_MESSAGE,
+            Detector::UnusedReturnEnum => UNUSED_RETURN_ENUM_LINT_MESSAGE,
         }
     }
 

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

@@ -18,3 +18,4 @@ 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_LINT_MESSAGE: &str = "Unsafe usage of `unwrap`";
+pub const UNUSED_RETURN_ENUM_LINT_MESSAGE : &str = "If any of the variants (Ok/Err) is not used, the code could be simplified or it could imply a bug";

test-cases/README.md

@@ -57,15 +57,13 @@ Check our
 [test-cases](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases)
 for code examples of these vulnerabilities and their respective remediations.
 
-
 ### Divide before multiply
 
 This vulnerability class relates to the order of operations in Rust, specifically in integer arithmetic. Performing a division operation before a multiplication can lead to a loss of precision. This issue becomes significant in programs like smart contracts where numerical precision is crucial.
 
 This vulnerability falls under the [Arithmetic](#vulnerability-categories) category
 and has a Medium Severity.
 
-
 ### Unsafe unrwap
 
 This vulnerability class pertains to the inappropriate usage of the `unwrap` method in Rust, which is commonly employed for error handling. The `unwrap` method retrieves the inner value of an `Option` or `Result`, but if an error or `None` occurs, it triggers a panic and crashes the program.
@@ -74,7 +72,6 @@ This vulnerability again falls under the [Validations and error handling](#vulne
 
 In our example, we consider an contract that utilizes the `unwrap` method to retrieve the balance of an account from a mapping. If there is no entry for the specified account, the contract will panic and abruptly halt execution, opening avenues for malicious exploitation.
 
-
 ### Unsafe expect
 
 In Rust, the `expect` method is commonly used for error handling. It retrieves the value from a `Result` or `Option` and panics with a specified error message if an error occurs. However, using `expect` can lead to unexpected program crashes.
@@ -105,7 +102,6 @@ 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.
@@ -192,6 +188,17 @@ Using a pinned version of Soroban can be dangerous, as it may have bugs or secur
 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.
 
+### Unused return enum
+
+`Rust` messages can return a `Result` `enum` with a custom error type. This is
+useful for the caller to know what went wrong when the message fails. The
+definition of the `Result` type enum consists of two variants: Ok and Err. If
+any of the variants is not used, the code could be simplified or it could imply
+a bug.
+
+We put this vulnerability under the [Validations and error handling category](#vulnerability-categories)
+with a Minor Severity.
+
 ### Unprotected mapping operation
 
 Modifying mappings with an arbitrary key given by the user could lead to unintented modifications of critical data, modifying data belonging to other users, causing denial of service, unathorized access, and other potential issues.