86 unused return enum

.github/workflows/test-detectors.yml

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

README.md

@@ -56,6 +56,7 @@ Install Scout from the Marketplace within the Extensions tab of Visual Studio Co
 | [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  |
 
 ## CLI Options
 

apps/cargo-scout-audit/tests/integration_tests/detectors.rs

@@ -44,7 +44,8 @@ fn test() {
         vec![false; integration_tests_to_run.as_ref().map_or(0, |v| v.len())];
 
     // Get the configuration
-    let detectors_config = Configuration::build().expect(&"Failed to get the configuration".red());
+    let detectors_config = Configuration::build()
+        .unwrap_or_else(|_| panic!("{}", "Failed to get the configuration".red().to_string()));
 
     // Run all integration tests
     for detector_config in detectors_config.detectors.iter() {
@@ -64,15 +65,10 @@ fn test() {
         println!("\n{} {}", "Testing detector:".bright_cyan(), detector_name);
         for example in detector_config.testcases.iter() {
             if let Some(vulnerable_path) = &example.vulnerable_path {
-                execute_and_validate_testcase(&detector_name, lint_message, &vulnerable_path, true);
+                execute_and_validate_testcase(&detector_name, lint_message, vulnerable_path, true);
             }
             if let Some(remediated_path) = &example.remediated_path {
-                execute_and_validate_testcase(
-                    &detector_name,
-                    lint_message,
-                    &remediated_path,
-                    false,
-                );
+                execute_and_validate_testcase(&detector_name, lint_message, remediated_path, false);
             }
         }
     }

apps/cargo-scout-audit/tests/integration_tests/detectors/configuration.rs

@@ -34,7 +34,6 @@ impl Configuration {
             .ok_or(anyhow!("Failed to find testcases path"))?
             .join("test-cases");
         let testcases_paths: Vec<PathBuf> = std::fs::read_dir(&testcases_root_path)?
-            .into_iter()
             .filter_map(|r| r.ok().map(|f| f.path()))
             .filter(|r| r.is_dir())
             .collect();
@@ -47,7 +46,6 @@ impl Configuration {
             let detector_name = detector.to_string();
             let testcases_root_path = testcases_root_path.join(detector_name);
             let testcases_paths: Vec<PathBuf> = std::fs::read_dir(testcases_root_path)?
-                .into_iter()
                 .filter_map(|r| r.ok().map(|f| f.path()))
                 .filter(|r| r.is_dir())
                 .collect();
@@ -101,7 +99,7 @@ impl Configuration {
             .into_iter()
             .sorted()
             .zip(Detector::iter().map(|d| d.to_string()).sorted())
-            .filter(|(p, d)| p.file_name().unwrap().to_string_lossy() != d.to_string())
+            .filter(|(p, d)| p.file_name().unwrap().to_string_lossy() != *d)
             .count();
 
         if count > 0 {

detectors/unused-return-enum/Cargo.toml

@@ -0,0 +1,19 @@
+[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).

detectors/unused-return-enum/src/lib.rs

@@ -0,0 +1,117 @@
+#![feature(rustc_private)]
+
+extern crate rustc_hir;
+extern crate rustc_span;
+
+use if_chain::if_chain;
+use rustc_hir::intravisit::{walk_expr, FnKind, Visitor};
+use rustc_hir::{Body, Expr, ExprKind, FnDecl, FnRetTy, MatchSource, QPath, TyKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_span::{def_id::LocalDefId, sym, Span};
+use scout_audit_internal::Detector;
+
+dylint_linting::declare_late_lint! {
+    pub UNUSED_RETURN_ENUM,
+    Warn,
+    Detector::UnusedReturnEnum.get_lint_message()
+}
+
+#[derive(Debug)]
+struct CounterVisitor {
+    count_err: u32,
+    count_ok: u32,
+    found_try: bool,
+    found_return: bool,
+    span: Vec<Span>,
+}
+
+impl<'tcx> Visitor<'tcx> for CounterVisitor {
+    fn visit_expr(&mut self, expr: &'tcx Expr) {
+        match expr.kind {
+            ExprKind::Call(function, _) => {
+                if_chain! {
+                    if let ExprKind::Path(QPath::Resolved(_, path)) = &function.kind;
+                    if let Some(last_segment) = path.segments.last();
+                    then {
+                        if last_segment.ident.name == sym::Ok {
+                            self.count_ok += 1;
+                            self.span.push(expr.span);
+                        } else if last_segment.ident.name == sym::Err {
+                            self.count_err += 1;
+                            self.span.push(expr.span);
+                        }
+                    }
+                }
+            }
+            ExprKind::Ret(Some(return_value)) => {
+                if_chain! {
+                    if let ExprKind::Call(function, _) = &return_value.kind;
+                    if let ExprKind::Path(QPath::Resolved(_, path)) = &function.kind;
+                    if let Some(last_segment) = path.segments.last();
+                    then {
+                        if last_segment.ident.name != sym::Ok && last_segment.ident.name != sym::Err {
+                            self.found_return = true;
+                        }
+                    }
+                }
+            }
+            ExprKind::Match(_, _, MatchSource::TryDesugar(_)) => {
+                self.found_try = true;
+            }
+            _ => {}
+        }
+        walk_expr(self, expr);
+    }
+}
+impl<'tcx> LateLintPass<'tcx> for UnusedReturnEnum {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        fnkind: FnKind<'tcx>,
+        decl: &'tcx FnDecl<'tcx>,
+        body: &'tcx Body<'tcx>,
+        _: Span,
+        _: LocalDefId,
+    ) {
+        // If the function is not a method, or it comes from a macro expansion, we ignore it
+        if !matches!(fnkind, FnKind::Method(_, fnsig) if !fnsig.span.from_expansion()) {
+            return;
+        }
+
+        // If the function returns a type different from Result, we ignore it
+        if_chain! {
+            if let FnRetTy::Return(return_type) = &decl.output;
+            if let TyKind::Path(qpath) = &return_type.kind;
+            if let QPath::Resolved(_ty, path) = qpath;
+            if let Some(path_segment) = path.segments.last();
+            if path_segment.ident.name != sym::Result;
+            then {
+                return;
+            }
+        }
+
+        let mut visitor = CounterVisitor {
+            count_ok: 0,
+            count_err: 0,
+            found_try: false,
+            found_return: false,
+            span: Vec::new(),
+        };
+
+        walk_expr(&mut visitor, body.value);
+
+        if !visitor.found_return
+            && !visitor.found_try
+            && (visitor.count_err == 0 || visitor.count_ok == 0)
+        {
+            visitor.span.iter().for_each(|span| {
+                Detector::UnusedReturnEnum.span_lint_and_help(
+                    cx,
+                    UNUSED_RETURN_ENUM,
+                    *span,
+                    "If any of the variants (Ok/Err) is not used, the code could be simplified or it could imply a bug",
+                );
+            });
+        }
+    }
+}

scout-audit-internal/src/detector.rs

@@ -38,6 +38,7 @@ pub enum Detector {
     UnprotectedUpdateCurrentContractWasm,
     UnsafeExpect,
     UnsafeUnwrap,
+    UnusedReturnEnum,
 }
 
 impl Detector {
@@ -58,6 +59,7 @@ impl Detector {
             Detector::AvoidUnsafeBlock => AVOID_UNSAFE_BLOCK_LINT_MESSAGE,
             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

@@ -16,3 +16,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

@@ -192,3 +192,13 @@ 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.

test-cases/unused-return-enum/unused-return-enum-1/remediated-example/Cargo.toml

@@ -0,0 +1,30 @@
+[package]
+name = "unused-return-enum-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