61 unprotected mapping operation

.github/workflows/test-detectors.yml

@@ -105,6 +105,7 @@ jobs:
             "insufficiently-random-values",
             "overflow-check",
             "set-contract-storage",
+            "unprotected-mapping-operation",
             "unprotected-update-current-contract-wasm",
             "unsafe-expect",
             "unsafe-unwrap",

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  |
+| [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/unprotected-mapping-operation/Cargo.toml

@@ -0,0 +1,20 @@
+[package]
+name = "unprotected-mapping-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/unprotected-mapping-operation/README.md

@@ -0,0 +1,60 @@
+# Unprotected Mapping Operation
+
+### What it does
+
+It warns you if a mapping operation (`insert`, `take`, `remove`) function is called with a user-given `key` field of the type `AccountId`.
+
+### Why is this bad?
+
+Modifying mappings with an arbitrary key given by users can be a significant vulnerability for several reasons:
+
+- Unintended Modifications: Allowing users to provide arbitrary keys can lead to unintended modifications of critical data within the smart contract. If the input validation and sanitation are not done properly, users may be able to manipulate the data in ways that were not intended by the contract's author.
+
+- Data Corruption: Malicious users could intentionally provide keys that result in the corruption or manipulation of important data stored in the mapping. This could lead to incorrect calculations, unauthorized access, or other undesirable outcomes.
+
+- Denial-of-Service (DoS) Attacks: If users can set arbitrary keys, they may be able to create mappings with a large number of entries, potentially causing the contract to exceed its gas limit. This could lead to denial-of-service attacks, making the contract unusable for other users.
+
+### Known problems
+
+### Example
+
+```rust
+    pub fn set_balance(env: Env, address: Address, balance: i128) -> State {
+        // Get the current state.
+        let mut state = Self::get_state(env.clone());
+
+        // Set the new account to have total supply if it doesn't exist.
+        if !state.balances.contains_key(address.clone()) {
+            state.balances.set(address, balance);
+            // Save the state.
+            env.storage().persistent().set(&STATE, &state);
+        }
+
+        state
+    }
+```
+
+Use instead:
+
+```rust
+    pub fn set_balance(env: Env, address: Address, balance: i128) -> State {
+        // Authenticate user
+        address.require_auth();
+
+        // Get the current state.
+        let mut state = Self::get_state(env.clone());
+
+        // Set the new account to have total supply if it doesn't exist.
+        if !state.balances.contains_key(address.clone()) {
+            state.balances.set(address, balance);
+            // Save the state.
+            env.storage().persistent().set(&STATE, &state);
+        }
+
+        state
+    }
+```
+
+### Implementation
+
+The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-mapping-operation).

detectors/unprotected-mapping-operation/src/lib.rs

@@ -0,0 +1,202 @@
+#![feature(rustc_private)]
+
+extern crate rustc_ast;
+extern crate rustc_hir;
+extern crate rustc_middle;
+extern crate rustc_span;
+
+use if_chain::if_chain;
+use rustc_hir::{
+    def::Res,
+    intravisit::{walk_body, walk_expr, Visitor},
+    Expr, ExprKind, HirId, Param, PatKind, QPath, StmtKind,
+};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_span::Span;
+use scout_audit_internal::Detector;
+
+dylint_linting::declare_late_lint! {
+    pub UNPROTECTED_MAPPING_OPERATION,
+    Warn,
+    Detector::UnprotectedMappingOperation.get_lint_message()
+}
+
+struct AuthStatus {
+    authed: bool,
+}
+
+struct UnauthorizedAddress {
+    span: Span,
+    name: String,
+}
+
+struct UnprotectedMappingOperationFinder<'tcx, 'tcx_ref> {
+    cx: &'tcx_ref LateContext<'tcx>,
+    linked_addresses: Vec<(AuthStatus, Vec<HirId>)>,
+    unauthorized_span: Vec<UnauthorizedAddress>,
+}
+
+const SOROBAN_MAP: &str = "soroban_sdk::Map";
+const SOROBAN_ADDRESS: &str = "soroban_sdk::Address";
+
+impl<'tcx> LateLintPass<'tcx> for UnprotectedMappingOperation {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        _: rustc_hir::intravisit::FnKind<'tcx>,
+        _: &'tcx rustc_hir::FnDecl<'tcx>,
+        body: &'tcx rustc_hir::Body<'tcx>,
+        _: Span,
+        _: rustc_span::def_id::LocalDefId,
+    ) {
+        let mut visitor = UnprotectedMappingOperationFinder {
+            cx,
+            linked_addresses: Vec::new(),
+            unauthorized_span: Vec::new(),
+        };
+
+        visitor.parse_body_params(body.params);
+
+        walk_body(&mut visitor, body);
+
+        visitor
+            .unauthorized_span
+            .iter()
+            .for_each(|unauthorized_address| {
+                Detector::UnprotectedMappingOperation.span_lint_and_help(
+                    cx,
+                    UNPROTECTED_MAPPING_OPERATION,
+                    unauthorized_address.span,
+                    &format!(
+                        "Address not authorized, please use `{}.require_auth();` prior to the mapping operation",
+                        unauthorized_address.name
+                    ),
+                );
+            });
+    }
+}
+
+impl<'tcx> Visitor<'tcx> for UnprotectedMappingOperationFinder<'tcx, '_> {
+    fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
+        if let ExprKind::Block(block, _) = &expr.kind {
+            block.stmts.iter().for_each(|stmt| {
+                if_chain! {
+                    if let StmtKind::Local(local) = &stmt.kind;
+                    if self.get_node_type(local.hir_id) == SOROBAN_ADDRESS;
+                    if let PatKind::Binding(_, target_hir_id, _, _) = &local.pat.kind;
+                    if let Some(init) = &local.init;
+                    let source_hir_id = self.get_expr_hir_id(init);
+                    if let Some(source_hir_id) = source_hir_id;
+                    then {
+                        // Insert the new address into the linked_addresses
+                        self.insert_new_address(source_hir_id, *target_hir_id);
+                    }
+                }
+            })
+        }
+
+        if let ExprKind::MethodCall(method_path, method_expr, method_args, _) = &expr.kind {
+            // Get the method expression type and check if it's a map with address
+            let method_expr_type = self.get_node_type(method_expr.hir_id);
+
+            if method_expr_type.starts_with(SOROBAN_MAP)
+                && method_expr_type.contains(SOROBAN_ADDRESS)
+            {
+                // Iterate through the method arguments and check if any of them is an address and not authed
+                method_args.iter().for_each(|arg| {
+                    if_chain! {
+                        if let Some(id) = self.get_expr_hir_id(arg);
+                        if self.get_node_type(id).contains(SOROBAN_ADDRESS);
+                        then {
+                            // Obtain the linked_addresses record in wich the address id is contained
+                            let linked_address = self.get_linked_address(id);
+
+                            // If the address does not exist, of if it does but the AuthStatus is false, then we need to add it to the unauthorized_span
+                            if linked_address.is_none() || !linked_address.unwrap().0.authed {
+                                self.unauthorized_span.push(UnauthorizedAddress {
+                                    span: expr.span,
+                                    name: self.cx.tcx.hir().name(id).to_string(),
+                                });
+                            }
+                        }
+                    }
+                });
+            }
+
+            // Check if the method call is a require_auth call and if it is, then we need to update the AuthStatus
+            if_chain! {
+                if method_expr_type.contains(SOROBAN_ADDRESS);
+                if method_path.ident.name.as_str() == "require_auth";
+                if let Some(id) = self.get_expr_hir_id(method_expr);
+                then {
+                    self.auth_address(id)
+                }
+            }
+        }
+
+        walk_expr(self, expr);
+    }
+}
+
+impl<'tcx> UnprotectedMappingOperationFinder<'tcx, '_> {
+    fn parse_body_params(&mut self, params: &'tcx [Param<'_>]) {
+        params.iter().for_each(|param| {
+            if self.get_node_type(param.hir_id) == SOROBAN_ADDRESS {
+                self.linked_addresses
+                    .push((AuthStatus { authed: false }, vec![param.pat.hir_id]));
+            }
+        });
+    }
+
+    fn get_node_type(&self, hir_id: HirId) -> String {
+        self.cx.typeck_results().node_type(hir_id).to_string()
+    }
+
+    fn insert_new_address(&mut self, source_hir_id: HirId, target_hir_id: HirId) {
+        if let Some((_, linked_addresses)) = self
+            .linked_addresses
+            .iter_mut()
+            .find(|(_, addresses)| addresses.iter().any(|&id| id == source_hir_id))
+        {
+            linked_addresses.push(target_hir_id);
+        }
+    }
+
+    fn get_expr_hir_id(&self, expr: &Expr) -> Option<HirId> {
+        let mut stack = vec![expr];
+
+        while let Some(current_expr) = stack.pop() {
+            match current_expr.kind {
+                ExprKind::MethodCall(_, call_expr, _, _) => stack.push(call_expr),
+                ExprKind::Path(QPath::Resolved(_, path)) => match path.res {
+                    Res::Local(hir_id) => return Some(hir_id),
+                    _ => continue,
+                },
+                _ => continue,
+            }
+        }
+
+        None
+    }
+
+    fn get_linked_address(&self, id: HirId) -> Option<&(AuthStatus, Vec<HirId>)> {
+        self.linked_addresses.iter().find(|(_, linked_addresses)| {
+            linked_addresses
+                .iter()
+                .any(|&address_hir_id| address_hir_id == id)
+        })
+    }
+
+    fn auth_address(&mut self, id: HirId) {
+        self.linked_addresses
+            .iter_mut()
+            .for_each(|(auth_status, linked_addresses)| {
+                if linked_addresses
+                    .iter()
+                    .any(|&address_hir_id| address_hir_id == id)
+                {
+                    auth_status.authed = true;
+                }
+            });
+    }
+}

scout-audit-internal/src/detector.rs

@@ -35,6 +35,7 @@ pub enum Detector {
     OverflowCheck,
     SetContractStorage,
     SorobanVersion,
+    UnprotectedMappingOperation,
     UnprotectedUpdateCurrentContractWasm,
     UnsafeExpect,
     UnsafeUnwrap,
@@ -46,16 +47,17 @@ impl Detector {
         match self {
             Detector::AvoidCoreMemForget => AVOID_CORE_MEM_FORGET_LINT_MESSAGE,
             Detector::AvoidPanicError => AVOID_PANIC_ERROR_LINT_MESSAGE,
+            Detector::AvoidUnsafeBlock => AVOID_UNSAFE_BLOCK_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::SorobanVersion => SOROBAN_VERSION_LINT_MESSAGE,
+            Detector::UnprotectedMappingOperation => UNPROTECTED_MAPPING_OPERATION_LINT_MESSAGE,
             Detector::UnprotectedUpdateCurrentContractWasm => {
                 UNPROTECTED_UPDATE_CURRENT_CONTRACT_LINT_MESSAGE
             }
-            Detector::AvoidUnsafeBlock => AVOID_UNSAFE_BLOCK_LINT_MESSAGE,
             Detector::UnsafeExpect => UNSAFE_EXPECT_LINT_MESSAGE,
             Detector::UnsafeUnwrap => UNSAFE_UNWRAP_LINT_MESSAGE,
         }

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

@@ -12,6 +12,8 @@ pub const DOS_UNBOUNDED_OPERATION_LINT_MESSAGE: &str =
 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 SOROBAN_VERSION_LINT_MESSAGE: &str = "Use the latest version of Soroban";
+pub const UNPROTECTED_MAPPING_OPERATION_LINT_MESSAGE: &str =
+    "This mapping operation is called without access control on a different key than the caller's address";
 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`";

test-cases/README.md

@@ -192,3 +192,8 @@ 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.
 
+### 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.
+
+This vulnerability falls under the [Validations and error handling](#vulnerability-categories) category and has a Critical severity.

test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example/Cargo.toml

@@ -0,0 +1,31 @@
+
+[package]
+name = "unprotected-mapping-operation"
+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