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",

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/src/lib.rs

@@ -0,0 +1,200 @@
+#![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_WITH_ADDRESS: &str = "soroban_sdk::Map<soroban_sdk::Address";
+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.contains(SOROBAN_MAP_WITH_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/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

test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example/src/lib.rs

@@ -0,0 +1,71 @@
+#![no_std]
+
+use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, Map, Symbol};
+
+#[contract]
+pub struct UnprotectedMappingOperation;
+
+#[contracttype]
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct State {
+    balances: Map<Address, i128>,
+}
+const STATE: Symbol = symbol_short!("STATE");
+
+#[contractimpl]
+impl UnprotectedMappingOperation {
+    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
+    }
+
+    /// Return the current state.
+    pub fn get_state(env: Env) -> State {
+        env.storage().persistent().get(&STATE).unwrap_or(State {
+            balances: Map::new(&env),
+        })
+    }
+}
+
+#[cfg(test)]
+const TOTAL_SUPPLY: i128 = 200;
+
+#[cfg(test)]
+mod tests {
+
+    use soroban_sdk::Env;
+
+    use crate::{UnprotectedMappingOperation, UnprotectedMappingOperationClient, TOTAL_SUPPLY};
+
+    #[test]
+    fn balance_of_works() {
+        // Given
+        let env = Env::default();
+        let contract_id = env.register_contract(None, UnprotectedMappingOperation);
+        let client = UnprotectedMappingOperationClient::new(&env, &contract_id);
+
+        // When
+        let state = client
+            .mock_all_auths()
+            .set_balance(&contract_id, &TOTAL_SUPPLY);
+
+        // Then
+        let balance = state
+            .balances
+            .get(contract_id)
+            .expect("Contract should have a balance");
+        assert_eq!(TOTAL_SUPPLY, balance);
+    }
+}