assert violation detector + test cases

detectors/assert-violation/Cargo.toml

@@ -0,0 +1,16 @@
+[package]
+name = "assert-violation"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+dylint_linting = { workspace = true }
+if_chain = { workspace = true }
+scout-audit-clippy-utils = { workspace = true }
+
+
+[package.metadata.rust-analyzer]
+rustc_private = true

detectors/assert-violation/src/lib.rs

@@ -0,0 +1,146 @@
+#![feature(rustc_private)]
+
+extern crate rustc_ast;
+extern crate rustc_span;
+
+use if_chain::if_chain;
+use rustc_ast::{
+    ptr::P,
+    tokenstream::{TokenStream, TokenTree},
+    AttrArgs, AttrKind, Expr, ExprKind, Item, MacCall, Stmt, StmtKind,
+};
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_span::{sym, Span};
+use scout_audit_clippy_utils::{diagnostics::span_lint_and_help, sym};
+
+const LINT_MESSAGE: &str = "Assert causes panic. Instead, return a proper error.";
+
+dylint_linting::impl_pre_expansion_lint! {
+    /// ### What it does
+    /// Checks for `assert!` usage.
+    /// ### Why is this bad?
+    /// `assert!` causes a panic, and panicking it's not a good practice. Instead, use proper error handling.
+
+    pub ASSERT_VIOLATION,
+    Warn,
+    LINT_MESSAGE,
+    AssertViolation::default(),
+    {
+        name: "Assert Violation",
+        long_message: "Assert causes panic. Instead, return a proper error.",
+        severity: "Medium",
+        help: "https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/assert-violation",
+        vulnerability_class: "Panic",
+    }
+}
+
+#[derive(Default)]
+struct AssertViolation {
+    in_test_span: Option<Span>,
+}
+
+impl AssertViolation {
+    fn in_test_item(&self) -> bool {
+        self.in_test_span.is_some()
+    }
+}
+
+impl EarlyLintPass for AssertViolation {
+    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: &Stmt) {
+        if self.in_test_item() {
+            return;
+        }
+
+        if let StmtKind::MacCall(mac) = &stmt.kind {
+            check_macro_call(cx, stmt.span, &mac.mac)
+        }
+    }
+    fn check_expr(&mut self, cx: &EarlyContext, expr: &Expr) {
+        if self.in_test_item() {
+            return;
+        }
+
+        if let ExprKind::MacCall(mac) = &expr.kind {
+            check_macro_call(cx, expr.span, mac)
+        }
+    }
+}
+
+fn check_macro_call(cx: &EarlyContext, span: Span, mac: &P<MacCall>) {
+    if [
+        sym!(assert),
+        sym!(assert_eq),
+        sym!(assert_ne),
+        sym!(debug_assert),
+        sym!(debug_assert_eq),
+        sym!(debug_assert_ne),
+    ]
+    .iter()
+    .any(|sym| &mac.path == sym)
+    {
+        span_lint_and_help(
+            cx,
+            ASSERT_VIOLATION,
+            span,
+            LINT_MESSAGE,
+            None,
+            "You could use instead an Error enum.",
+        );
+    }
+}
+
+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
+    })
+}
+
+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),
+    })
+}

test-cases/assert-violation/assert-violation-1/remediated-example/Cargo.toml

@@ -0,0 +1,31 @@
+[package]
+name = "assert-violation-remediated-1"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+soroban-sdk = "=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/assert-violation/assert-violation-1/remediated-example/src/lib.rs

@@ -0,0 +1,60 @@
+#![no_std]
+
+use soroban_sdk::{contract, contracterror, contractimpl, contracttype, symbol_short, Env, Symbol};
+
+#[contracterror]
+#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
+#[repr(u32)]
+pub enum AVError {
+    GreaterThan10 = 1,
+}
+
+#[derive(Debug, Clone)]
+#[contracttype]
+pub struct State {
+    value: u128,
+}
+
+const STATE: Symbol = symbol_short!("STATE");
+#[contract]
+pub struct AssertViolation;
+
+#[contractimpl]
+impl AssertViolation {
+    pub fn init(env: Env, init_value: u128) -> State {
+        let state = State { value: init_value };
+
+        env.storage().instance().set(&STATE, &state);
+        state
+    }
+
+    pub fn assert_if_greater_than_10(_env: Env, value: u128) -> Result<bool, AVError> {
+        if value <= 10 {
+            Ok(true)
+        } else {
+            Err(AVError::GreaterThan10)
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn does_not_revert_if_greater() {
+        let env = Env::default();
+        let contract =
+            AssertViolationClient::new(&env, &env.register_contract(None, AssertViolation {}));
+        assert!(contract.assert_if_greater_than_10(&5));
+    }
+
+    #[test]
+    #[should_panic(expected = "1")] // The custom error number is 1
+    fn reverts_if_greater() {
+        let env = Env::default();
+        let contract =
+            AssertViolationClient::new(&env, &env.register_contract(None, AssertViolation {}));
+        contract.assert_if_greater_than_10(&11);
+    }
+}

test-cases/assert-violation/assert-violation-1/vulnerable-example/Cargo.toml

@@ -0,0 +1,31 @@
+[package]
+name = "assert-violation-vulnerable-1"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+soroban-sdk = "=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/assert-violation/assert-violation-1/vulnerable-example/src/lib.rs

@@ -0,0 +1,50 @@
+#![no_std]
+
+use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Env, Symbol};
+
+#[derive(Debug, Clone)]
+#[contracttype]
+pub struct State {
+    value: u128,
+}
+
+const STATE: Symbol = symbol_short!("STATE");
+#[contract]
+pub struct AssertViolation;
+
+#[contractimpl]
+impl AssertViolation {
+    pub fn init(env: Env, init_value: u128) -> State {
+        let state = State { value: init_value };
+
+        env.storage().instance().set(&STATE, &state);
+        state
+    }
+
+    pub fn assert_if_greater_than_10(_env: Env, value: u128) -> bool {
+        assert!(value <= 10, "value should be less than 10");
+        true
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn does_not_revert_if_greater() {
+        let env = Env::default();
+        let contract =
+            AssertViolationClient::new(&env, &env.register_contract(None, AssertViolation {}));
+        assert!(contract.assert_if_greater_than_10(&5));
+    }
+
+    #[test]
+    #[should_panic(expected = "value should be less than 10")]
+    fn reverts_if_greater() {
+        let env = Env::default();
+        let contract =
+            AssertViolationClient::new(&env, &env.register_contract(None, AssertViolation {}));
+        contract.assert_if_greater_than_10(&11);
+    }
+}