4 dos unbounded operation (#56)

* Add dos-unbounded-operation detector and test cases

.github/workflows/test-detectors.yml

@@ -100,6 +100,7 @@ jobs:
             "avoid-core-mem-forget",
             "avoid-panic-error",
             "divide-before-multiply",
+            "dos-unbounded-operation",
             "insufficiently-random-values",
             "overflow-check",
             "set-contract-storage",

detectors/Cargo.toml

@@ -4,9 +4,9 @@ exclude = [".cargo", "target"]
 resolver = "2"
 
 [workspace.dependencies]
-clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "7671c283a50b5d1168841f3014b14000f01dd204" }
 dylint_linting = "2.1.5"
 dylint_testing = "2.1.5"
 if_chain = "1.0.2"
 
+scout-audit-clippy-utils = { version = "=0.2.1", path = "../scout-audit-clippy-utils" }
 scout-audit-internal = { path = "../scout-audit-internal", features = ["detector", "lint_helper"] }

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

@@ -7,7 +7,6 @@ edition = "2021"
 crate-type = ["cdylib"]
 
 [dependencies]
-clippy_utils = { workspace = true }
 dylint_linting = { workspace = true }
 if_chain = { workspace = true }
 

detectors/avoid-panic-error/Cargo.toml

@@ -7,10 +7,10 @@ edition = "2021"
 crate-type = ["cdylib"]
 
 [dependencies]
-clippy_utils = { workspace = true }
 dylint_linting = { workspace = true }
 if_chain = { workspace = true }
 
+scout-audit-clippy-utils = { workspace = true }
 scout-audit-internal = { workspace = true }
 
 [dev-dependencies]

detectors/avoid-panic-error/src/lib.rs

@@ -4,7 +4,6 @@
 extern crate rustc_ast;
 extern crate rustc_span;
 
-use clippy_utils::sym;
 use if_chain::if_chain;
 use rustc_ast::{
     ptr::P,
@@ -14,6 +13,7 @@ use rustc_ast::{
 };
 use rustc_lint::{EarlyContext, EarlyLintPass};
 use rustc_span::{sym, Span};
+use scout_audit_clippy_utils::sym;
 use scout_audit_internal::Detector;
 
 dylint_linting::impl_pre_expansion_lint! {

detectors/dos-unbounded-operation/Cargo.toml

@@ -0,0 +1,19 @@
+[package]
+name = "dos-unbounded-operation"
+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/dos-unbounded-operation/src/lib.rs

@@ -0,0 +1,142 @@
+#![feature(rustc_private)]
+#![recursion_limit = "256"]
+extern crate rustc_ast;
+extern crate rustc_hir;
+extern crate rustc_span;
+
+use if_chain::if_chain;
+use rustc_hir::def::DefKind;
+use rustc_hir::StmtKind;
+use rustc_hir::{
+    def::Res,
+    intravisit::{walk_body, walk_expr, FnKind, Visitor},
+    Body, Expr, ExprKind, FnDecl, HirId, LangItem, MatchSource, QPath,
+};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_span::{def_id::LocalDefId, Span};
+use scout_audit_internal::Detector;
+
+dylint_linting::declare_late_lint!(
+    pub DOS_UNBOUNDED_OPERATION,
+    Warn,
+    "This loop seems to do not have a fixed number of iterations"
+);
+
+struct ForLoopVisitor {
+    constants: Vec<HirId>,
+    span_constant: Vec<Span>,
+}
+
+impl ForLoopVisitor {
+    fn is_qpath_constant(&self, path: &QPath) -> bool {
+        if let QPath::Resolved(_, path) = path {
+            // We search the path, if it has been previously defined or is a constant then we are good
+            match path.res {
+                Res::Def(def_kind, _) => matches!(
+                    def_kind,
+                    DefKind::AnonConst
+                        | DefKind::AssocConst
+                        | DefKind::Const
+                        | DefKind::InlineConst
+                ),
+                Res::Local(hir_id) => self.constants.contains(&hir_id),
+                _ => false,
+            }
+        } else {
+            false
+        }
+    }
+
+    fn is_expr_constant(&self, current_expr: &Expr) -> bool {
+        match current_expr.kind {
+            ExprKind::Array(expr_array) => expr_array
+                .iter()
+                .all(|expr_in_array| self.is_expr_constant(expr_in_array)),
+            ExprKind::Binary(_, left_expr, right_expr) => {
+                self.is_expr_constant(left_expr) && self.is_expr_constant(right_expr)
+            }
+            ExprKind::Cast(cast_expr, _) => self.is_expr_constant(cast_expr),
+            ExprKind::Field(field_expr, _) => self.is_expr_constant(field_expr),
+            ExprKind::Index(array_expr, index_expr, _) => {
+                self.is_expr_constant(array_expr) && self.is_expr_constant(index_expr)
+            }
+            ExprKind::Lit(_) => true,
+            ExprKind::MethodCall(_, call_expr, _, _) => self.is_expr_constant(call_expr),
+            ExprKind::Path(qpath_expr) => self.is_qpath_constant(&qpath_expr),
+            ExprKind::Repeat(repeat_expr, _) => self.is_expr_constant(repeat_expr),
+            ExprKind::Struct(_, expr_fields, _) => expr_fields
+                .iter()
+                .all(|field_expr| self.is_expr_constant(field_expr.expr)),
+            _ => false,
+        }
+    }
+}
+
+impl<'tcx> Visitor<'tcx> for ForLoopVisitor {
+    fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
+        // Constant detection
+        if let ExprKind::Block(a, _) = expr.kind {
+            a.stmts.iter().for_each(|func| {
+                if let StmtKind::Local(sd) = func.kind {
+                    if sd.init.is_some() && self.is_expr_constant(sd.init.as_ref().unwrap()) {
+                        self.constants.push(sd.pat.hir_id);
+                    }
+                }
+            })
+        }
+
+        // For loop detection
+        if_chain! {
+            // Check if the expression is a for loop
+            if let ExprKind::Match(match_expr, _, MatchSource::ForLoopDesugar) = expr.kind;
+            if let ExprKind::Call(call_func, call_args) = match_expr.kind;
+            // Check the function call
+            if let ExprKind::Path(qpath) = &call_func.kind;
+            if let QPath::LangItem(LangItem::IntoIterIntoIter, _, _) = qpath;
+            // Check if a Range is used
+            if let ExprKind::Struct(struct_lang_item, struct_expr, _) = call_args.first().unwrap().kind;
+            if let QPath::LangItem(
+                LangItem::Range | LangItem::RangeInclusiveStruct | LangItem::RangeInclusiveNew,
+                _,
+                _,
+            ) = struct_lang_item;
+            // Get the start and end of the range
+            if let Some(start_expr) = struct_expr.first();
+            if let Some(end_expr) = struct_expr.last();
+            then {
+                if !self.is_expr_constant(start_expr.expr) || !self.is_expr_constant(end_expr.expr) {
+                    self.span_constant.push(expr.span);
+                }
+            }
+        }
+        walk_expr(self, expr);
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for DosUnboundedOperation {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        _: FnKind<'tcx>,
+        _: &'tcx FnDecl<'tcx>,
+        body: &'tcx Body<'tcx>,
+        _: Span,
+        _: LocalDefId,
+    ) {
+        let mut visitor = ForLoopVisitor {
+            span_constant: Vec::new(),
+            constants: Vec::new(),
+        };
+
+        walk_body(&mut visitor, body);
+
+        for span in visitor.span_constant {
+            Detector::DosUnboundedOperation.span_lint_and_help(
+                cx,
+                DOS_UNBOUNDED_OPERATION,
+                span,
+                "This loop seems to do not have a fixed number of iterations",
+            );
+        }
+    }
+}

detectors/insufficiently-random-values/Cargo.toml

@@ -7,7 +7,6 @@ edition = "2021"
 crate-type = ["cdylib"]
 
 [dependencies]
-clippy_utils = { workspace = true }
 dylint_linting = { workspace = true }
 if_chain = { workspace = true }
 

detectors/set-contract-storage/Cargo.toml

@@ -7,7 +7,6 @@ edition = "2021"
 crate-type = ["cdylib"]
 
 [dependencies]
-clippy_utils = { workspace = true }
 dylint_linting = { workspace = true }
 if_chain = { workspace = true }
 

detectors/unprotected-update-current-contract-wasm/Cargo.toml

@@ -7,7 +7,6 @@ edition = "2021"
 crate-type = ["cdylib"]
 
 [dependencies]
-clippy_utils = { workspace = true }
 dylint_linting = { workspace = true }
 if_chain = { workspace = true }
 

detectors/unsafe-expect/Cargo.toml

@@ -7,7 +7,6 @@ edition = "2021"
 crate-type = ["cdylib"]
 
 [dependencies]
-clippy_utils = { workspace = true }
 dylint_linting = { workspace = true }
 if_chain = { workspace = true }
 

detectors/unsafe-unwrap/Cargo.toml

@@ -7,7 +7,6 @@ edition = "2021"
 crate-type = ["cdylib"]
 
 [dependencies]
-clippy_utils = { workspace = true }
 dylint_linting = { workspace = true }
 if_chain = { workspace = true }
 

scout-audit-internal/src/detector.rs

@@ -29,6 +29,7 @@ pub enum Detector {
     AvoidCoreMemForget,
     AvoidPanicError,
     DivideBeforeMultiply,
+    DosUnboundedOperation,
     InsufficientlyRandomValues,
     OverflowCheck,
     SetContractStorage,
@@ -43,8 +44,9 @@ impl Detector {
         match self {
             Detector::AvoidCoreMemForget => AVOID_CORE_MEM_FORGET_LINT_MESSAGE,
             Detector::AvoidPanicError => AVOID_PANIC_ERROR_LINT_MESSAGE,
-            Detector::InsufficientlyRandomValues => INSUFFICIENTLY_RANDOM_VALUES_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 => {

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

@@ -1,10 +1,12 @@
 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 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 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";
 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 =

test-cases/dos-unbounded-operation/dos-unbounded-operation-1/remediated-example/Cargo.toml

@@ -0,0 +1,30 @@
+[package]
+name = "dos-unbounded-operation-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

test-cases/dos-unbounded-operation/dos-unbounded-operation-1/remediated-example/src/lib.rs

@@ -0,0 +1,39 @@
+#![no_std]
+use soroban_sdk::{contract, contractimpl};
+
+#[contract]
+pub struct DosUnboundedOperation;
+
+const FIXED_COUNT: u64 = 1000;
+
+#[contractimpl]
+impl DosUnboundedOperation {
+    pub fn restricted_loop_with_const() -> u64 {
+        let mut sum = 0;
+        for i in 0..FIXED_COUNT {
+            sum += i;
+        }
+        sum
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use soroban_sdk::Env;
+
+    use crate::{DosUnboundedOperation, DosUnboundedOperationClient};
+
+    #[test]
+    fn test_for_loop() {
+        // Given
+        let env = Env::default();
+        let contract_id = env.register_contract(None, DosUnboundedOperation);
+        let client = DosUnboundedOperationClient::new(&env, &contract_id);
+
+        // When
+        let count = client.restricted_loop_with_const();
+
+        // Then
+        assert_eq!(count, 499500);
+    }
+}

test-cases/dos-unbounded-operation/dos-unbounded-operation-1/vulnerable-example/Cargo.toml

@@ -0,0 +1,30 @@
+[package]
+name = "dos-unbounded-operation-vulnerable-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