Merge pull request #348 from CoinFabrik/345-no-admin-use-on-function-…

…parameter

Add `unnecessary-admin-parameter` detector

detectors/unnecessary-admin-parameter/Cargo.toml

@@ -0,0 +1,17 @@
+[package]
+edition = "2021"
+name = "unnecessary-admin-parameter"
+version = "0.1.0"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+clippy_wrappers = { workspace = true }
+dylint_linting = { workspace = true }
+edit-distance = "=2.1.2"
+if_chain = { workspace = true }
+utils = { workspace = true }
+
+[package.metadata.rust-analyzer]
+rustc_private = true

detectors/unnecessary-admin-parameter/src/lib.rs

@@ -0,0 +1,150 @@
+#![feature(rustc_private)]
+
+extern crate rustc_hir;
+extern crate rustc_span;
+
+use clippy_wrappers::span_lint_and_help;
+use edit_distance::edit_distance;
+use if_chain::if_chain;
+use rustc_hir::{
+    def::Res,
+    intravisit::{walk_expr, FnKind, Visitor},
+    Body, Expr, ExprKind, FnDecl, HirId, Param, PatKind, QPath,
+};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_span::{
+    def_id::{DefId, LocalDefId},
+    Span, Symbol,
+};
+use std::collections::{HashMap, HashSet};
+use utils::{get_node_type_opt, is_soroban_address, is_soroban_function};
+
+const LINT_MESSAGE: &str = "Usage of admin parameter might be unnecessary";
+
+dylint_linting::impl_late_lint! {
+    pub UNNECESSARY_ADMIN_PARAMETER,
+    Warn,
+    LINT_MESSAGE,
+    UnnecessaryAdminParameter::default(),
+    {
+        name: "Unnecessary Admin Parameter",
+        long_message: "This function has an admin parameter that might be unnecessary. Consider retrieving the admin from storage instead.",
+        severity: "Medium",
+        help: "https://coinfabrik.github.io/scout-soroban/docs/detectors/unnecessary-admin-parameter",
+        vulnerability_class: "Access Control",
+    }
+}
+
+struct AdminInfo {
+    param_span: Span,
+    usage_span: Option<Span>,
+}
+
+#[derive(Default)]
+struct UnnecessaryAdminParameter {
+    checked_functions: HashSet<String>,
+    admin_params: HashMap<DefId, AdminInfo>,
+}
+
+impl<'tcx> LateLintPass<'tcx> for UnnecessaryAdminParameter {
+    fn check_crate_post(&mut self, cx: &LateContext<'tcx>) {
+        for (function_def_id, admin_info) in &self.admin_params {
+            if is_soroban_function(cx, &self.checked_functions, function_def_id) {
+                let help_message = if admin_info.usage_span.is_some() {
+                    "Consider retrieving the admin from storage instead of passing it as a parameter"
+                } else {
+                    "This admin parameter is not used for access control. Consider removing it or implementing proper access control"
+                };
+
+                span_lint_and_help(
+                    cx,
+                    UNNECESSARY_ADMIN_PARAMETER,
+                    admin_info.param_span,
+                    LINT_MESSAGE,
+                    admin_info.usage_span,
+                    help_message,
+                )
+            }
+        }
+    }
+
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        _: FnKind<'tcx>,
+        _: &'tcx FnDecl<'tcx>,
+        body: &'tcx Body<'tcx>,
+        span: Span,
+        local_def_id: LocalDefId,
+    ) {
+        let def_id = local_def_id.to_def_id();
+        self.checked_functions.insert(cx.tcx.def_path_str(def_id));
+
+        if span.from_expansion() {
+            return;
+        }
+
+        // Skip analysis for functions named "initialize"
+        let fn_name = cx.tcx.item_name(def_id);
+        if is_similar_to(&fn_name.as_str().to_lowercase(), "initialize") {
+            return;
+        }
+
+        // Step 1: Check for admin parameter
+        if let Some(admin_param) = find_admin_param(cx, body.params) {
+            // Step 2: Check function body for proper use of admin parameter
+            let mut visitor = UnnecessaryAdminParameterVisitor {
+                admin_param_id: admin_param.pat.hir_id,
+                access_control_span: None,
+            };
+            visitor.visit_body(body);
+
+            // Step 3: Store the information for later analysis
+            self.admin_params.insert(
+                def_id,
+                AdminInfo {
+                    param_span: admin_param.span,
+                    usage_span: visitor.access_control_span,
+                },
+            );
+        }
+    }
+}
+
+fn find_admin_param<'tcx>(
+    cx: &LateContext<'tcx>,
+    params: &'tcx [Param<'tcx>],
+) -> Option<&'tcx Param<'tcx>> {
+    params.iter().find(|param| {
+        matches!(param.pat.kind, PatKind::Binding(_, _, ident, _)
+            if is_similar_to(&ident.name.as_str().to_lowercase(), "admin"))
+            && get_node_type_opt(cx, &param.hir_id)
+                .map_or(false, |type_| is_soroban_address(cx, type_))
+    })
+}
+
+fn is_similar_to(s1: &str, s2: &str) -> bool {
+    edit_distance(s1, s2) <= 1
+}
+
+struct UnnecessaryAdminParameterVisitor {
+    admin_param_id: HirId,
+    access_control_span: Option<Span>,
+}
+
+impl<'tcx> Visitor<'tcx> for UnnecessaryAdminParameterVisitor {
+    fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
+        if_chain! {
+            if let ExprKind::MethodCall(path_segment, receiver, ..) = expr.kind;
+            if path_segment.ident.name == Symbol::intern("require_auth");
+            if let ExprKind::Path(QPath::Resolved(_, path)) = receiver.kind;
+            if let Res::Local(id) = path.res;
+            if id == self.admin_param_id;
+            then {
+                self.access_control_span = Some(expr.span);
+            }
+        }
+
+        walk_expr(self, expr);
+    }
+}

test-cases/unnecessary-admin-parameter/Cargo.toml

@@ -0,0 +1,21 @@
+[workspace]
+exclude = [".cargo", "target"]
+members = ["unnecessary-admin-parameter-*/*"]
+resolver = "2"
+
+[workspace.dependencies]
+soroban-sdk = { version = "=21.6.0" }
+
+[profile.release]
+codegen-units = 1
+debug = 0
+debug-assertions = false
+lto = true
+opt-level = "z"
+overflow-checks = true
+panic = "abort"
+strip = "symbols"
+
+[profile.release-with-logs]
+debug-assertions = true
+inherits = "release"

test-cases/unnecessary-admin-parameter/unnecessary-admin-parameter-1/remediated-example/Cargo.toml

@@ -0,0 +1,17 @@
+
+[package]
+edition = "2021"
+name = "unnecessary-admin-paramenter-remediated-1"
+version = "0.1.0"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+soroban-sdk = { workspace = true }
+
+[dev-dependencies]
+soroban-sdk = { workspace = true, features = ["testutils"] }
+
+[features]
+testutils = ["soroban-sdk/testutils"]

test-cases/unnecessary-admin-parameter/unnecessary-admin-parameter-1/remediated-example/src/lib.rs

@@ -0,0 +1,99 @@
+#![no_std]
+
+use soroban_sdk::{contract, contractimpl, contracttype, Address, Env};
+
+#[contract]
+pub struct UnnecessaryAdminParameter;
+
+#[contracttype]
+pub enum DataKey {
+    Admin,
+}
+
+#[contractimpl]
+impl UnnecessaryAdminParameter {
+    pub fn initialize(env: Env, admin: Address) {
+        env.storage().instance().set(&DataKey::Admin, &admin);
+    }
+
+    // This function is minimal and intends to showcase the correct retrieval of the admin.
+    pub fn set_admin(env: Env, new_admin: Address) {
+        // Initialize has already set the admin, so we can retrieve it directly.
+        let current_admin: Address = env.storage().instance().get(&DataKey::Admin).unwrap();
+        current_admin.require_auth();
+        env.storage().instance().set(&DataKey::Admin, &new_admin);
+    }
+}
+
+#[cfg(test)]
+mod tests {
+
+    use soroban_sdk::Env;
+    use soroban_sdk::{testutils::Address as _, Address};
+
+    use crate::{DataKey, UnnecessaryAdminParameter, UnnecessaryAdminParameterClient};
+
+    #[test]
+    fn test_vulnerable_initialize() {
+        // Given
+        let env = Env::default();
+        let contract_id = env.register_contract(None, UnnecessaryAdminParameter);
+        let client = UnnecessaryAdminParameterClient::new(&env, &contract_id);
+
+        // When
+        let admin = soroban_sdk::Address::generate(&env);
+        client.initialize(&admin);
+
+        // Then
+        let stored_admin: Address = env.as_contract(&contract_id, || {
+            env.storage().instance().get(&DataKey::Admin).unwrap()
+        });
+        assert_eq!(stored_admin, admin);
+    }
+
+    #[test]
+    fn test_remediated_set_admin_authorized() {
+        // Given
+        let env = Env::default();
+        let contract_id = env.register_contract(None, UnnecessaryAdminParameter);
+        let client = UnnecessaryAdminParameterClient::new(&env, &contract_id);
+
+        // When
+        let admin = Address::generate(&env);
+        client.initialize(&admin);
+
+        // Set new admin
+        let new_admin = Address::generate(&env);
+        env.mock_all_auths();
+        client.set_admin(&new_admin);
+
+        // Then
+        let stored_admin: Address = env.as_contract(&contract_id, || {
+            env.storage().instance().get(&DataKey::Admin).unwrap()
+        });
+        assert_eq!(stored_admin, new_admin);
+    }
+
+    #[test]
+    #[should_panic(expected = "Unauthorized function call for address")]
+    fn test_remediated_set_admin_unauthorized() {
+        // Given
+        let env = Env::default();
+        let contract_id = env.register_contract(None, UnnecessaryAdminParameter);
+        let client = UnnecessaryAdminParameterClient::new(&env, &contract_id);
+
+        // When
+        let admin = Address::generate(&env);
+        client.initialize(&admin);
+
+        let bad_actor = Address::generate(&env);
+
+        // No authorization mocking, simulating an unauthorized attempt
+
+        // Attempt to change admin without proper authorization
+        client.set_admin(&bad_actor);
+
+        // Then
+        // This point should not be reached due to the expected panic
+    }
+}

test-cases/unnecessary-admin-parameter/unnecessary-admin-parameter-1/vulnerable-example/Cargo.toml

@@ -0,0 +1,17 @@
+
+[package]
+edition = "2021"
+name = "unnecessary-admin-paramenter-vulnerable-1"
+version = "0.1.0"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+soroban-sdk = { workspace = true }
+
+[dev-dependencies]
+soroban-sdk = { workspace = true, features = ["testutils"] }
+
+[features]
+testutils = ["soroban-sdk/testutils"]