Merge pull request #328 from CoinFabrik/327-instance-storage-detector…

…-refinement

Edit `dynamic-storage`

detectors/dynamic-instance-storage/Cargo.toml → detectors/dynamic-storage/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 edition = "2021"
-name = "dynamic-instance-storage"
+name = "dynamic-storage"
 version = "0.1.0"
 
 [lib]

detectors/dynamic-instance-storage/src/lib.rs → detectors/dynamic-storage/src/lib.rs

@@ -15,26 +15,26 @@ use rustc_middle::ty::{Ty, TyKind};
 use rustc_span::{def_id::LocalDefId, Span, Symbol};
 use utils::{get_node_type_opt, is_soroban_storage, SorobanStorageType};
 
-const LINT_MESSAGE: &str = "This function may lead to excessive instance storage growth, which could increase execution costs or potentially cause DoS";
+const LINT_MESSAGE: &str = "Using dynamic types in instance or persistent storage can lead to unnecessary growth or storage-related vulnerabilities.";
 
 dylint_linting::impl_late_lint! {
-    pub DYNAMIC_INSTANCE_STORAGE,
+    pub DYNAMIC_STORAGE,
     Warn,
     LINT_MESSAGE,
-    DynamicInstanceStorage,
+    DynamicStorage,
     {
-        name: "Dynamic Instance Storage Analyzer",
-        long_message: "Detects potential misuse of instance storage that could lead to unnecessary growth or storage-related vulnerabilities.",
+        name: "Dynamic Storage Analyzer",
+        long_message: "Using dynamic types in instance or persistent storage can lead to unnecessary growth or storage-related vulnerabilities.",
         severity: "Warning",
-        help: "https://coinfabrik.github.io/scout-soroban/docs/detectors/dynamic-instance-storage",
+        help: "https://coinfabrik.github.io/scout-soroban/docs/detectors/dynamic-storage",
         vulnerability_class: "Resource Management",
     }
 }
 
 #[derive(Default)]
-struct DynamicInstanceStorage;
+struct DynamicStorage;
 
-impl<'tcx> LateLintPass<'tcx> for DynamicInstanceStorage {
+impl<'tcx> LateLintPass<'tcx> for DynamicStorage {
     fn check_fn(
         &mut self,
         cx: &LateContext<'tcx>,
@@ -48,30 +48,31 @@ impl<'tcx> LateLintPass<'tcx> for DynamicInstanceStorage {
             return;
         }
 
-        let mut storage_warn_visitor = DynamicInstanceStorageVisitor { cx };
+        let mut storage_warn_visitor = DynamicStorageVisitor { cx };
         storage_warn_visitor.visit_body(body);
     }
 }
 
-struct DynamicInstanceStorageVisitor<'a, 'tcx> {
+struct DynamicStorageVisitor<'a, 'tcx> {
     cx: &'a LateContext<'tcx>,
 }
 
-impl<'a, 'tcx> Visitor<'tcx> for DynamicInstanceStorageVisitor<'a, 'tcx> {
+impl<'a, 'tcx> Visitor<'tcx> for DynamicStorageVisitor<'a, 'tcx> {
     fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
         if_chain! {
             // Detect calls to `set` method
             if let ExprKind::MethodCall(path, receiver, args, _) = &expr.kind;
             if path.ident.name == Symbol::intern("set");
-            // Get the type of the receiver and check if it is an instance storage
+            // Get the type of the receiver and check if it is an instance or persistent storage
             if let Some(receiver_ty) = get_node_type_opt(self.cx, &receiver.hir_id);
-            if is_soroban_storage(self.cx, receiver_ty, SorobanStorageType::Instance);
+            if is_soroban_storage(self.cx, receiver_ty, SorobanStorageType::Instance)
+                || is_soroban_storage(self.cx, receiver_ty, SorobanStorageType::Persistent);
             // Check if the value being set is a dynamic type
             if args.len() >= 2;
             if let Some(value_type) = get_node_type_opt(self.cx, &args[1].hir_id);
             if is_dynamic_type(self.cx, &value_type);
             then {
-                span_lint(self.cx, DYNAMIC_INSTANCE_STORAGE, expr.span, LINT_MESSAGE)
+                span_lint(self.cx, DYNAMIC_STORAGE, expr.span, LINT_MESSAGE)
             }
         }
 

test-cases/dynamic-instance-storage/Cargo.toml → test-cases/dynamic-storage/Cargo.toml

@@ -1,10 +1,10 @@
 [workspace]
 exclude = [".cargo", "target"]
-members = ["dynamic-instance-storage-*/*"]
+members = ["dynamic-storage-*/*"]
 resolver = "2"
 
 [workspace.dependencies]
-soroban-sdk = { version = "=21.4.0" }
+soroban-sdk = { version = "=21.5.1" }
 
 [profile.release]
 codegen-units = 1

test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/Cargo.toml → test-cases/dynamic-storage/dynamic-storage-1/remediated-example/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 edition = "2021"
-name = "dynamic-instance-storage-vulnerable-2"
+name = "dynamic-storage-remediated-1"
 version = "0.1.0"
 
 [lib]

test-cases/dynamic-storage/dynamic-storage-1/remediated-example/src/lib.rs

@@ -0,0 +1,63 @@
+#![no_std]
+use soroban_sdk::{contract, contractimpl, contracttype, Env, Vec};
+
+#[contract]
+pub struct VectorStorage;
+
+#[derive(Clone)]
+#[contracttype]
+pub enum DataKey {
+    VecElement(u32),
+}
+
+#[contractimpl]
+impl VectorStorage {
+    pub fn store_vector(e: Env, data: Vec<i32>) {
+        for (i, value) in data.iter().enumerate() {
+            let key = DataKey::VecElement(i as u32);
+            e.storage().persistent().set(&key, &value);
+        }
+    }
+
+    pub fn get_vector(e: Env) -> Vec<i32> {
+        let mut result = Vec::new(&e);
+        let mut i = 0;
+
+        while let Some(value) = VectorStorage::get_element(e.clone(), i) {
+            result.push_back(value);
+            i += 1;
+        }
+
+        result
+    }
+
+    pub fn get_element(e: Env, index: u32) -> Option<i32> {
+        let key = DataKey::VecElement(index);
+        e.storage().persistent().get(&key)
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use soroban_sdk::{vec, Env};
+
+    #[test]
+    fn test_vector_storage() {
+        // Given
+        let env = Env::default();
+        let contract_id = env.register_contract(None, VectorStorage);
+        let client = VectorStorageClient::new(&env, &contract_id);
+
+        // When
+        let test_vec = vec![&env, 1, 2, 3, 4, 5];
+        client.store_vector(&test_vec);
+
+        // Then
+        let retrieved_vec = client.get_vector();
+        assert_eq!(test_vec, retrieved_vec);
+
+        assert_eq!(client.get_element(&2), Some(3));
+        assert_eq!(client.get_element(&5), None);
+    }
+}

test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/Cargo.toml → test-cases/dynamic-storage/dynamic-storage-1/vulnerable-example/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 edition = "2021"
-name = "dynamic-instance-storage-vulnerable-1"
+name = "dynamic-storage-vulnerable-1"
 version = "0.1.0"
 
 [lib]

test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/Cargo.toml → test-cases/dynamic-storage/dynamic-storage-2/remediated-example/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 edition = "2021"
-name = "dynamic-instance-storage-remediated-2"
+name = "dynamic-storage-remediated-2"
 version = "0.1.0"
 
 [lib]

test-cases/dynamic-storage/dynamic-storage-2/remediated-example/src/lib.rs

@@ -0,0 +1,44 @@
+#![no_std]
+use soroban_sdk::{contract, contractimpl, Env, Map, Symbol};
+
+#[contract]
+pub struct MapStorage;
+
+#[contractimpl]
+impl MapStorage {
+    pub fn store_map(e: Env, data: Map<Symbol, i32>) {
+        data.iter().for_each(|(key, value)| {
+            e.storage().persistent().set(&key, &value);
+        });
+    }
+
+    pub fn get_key(e: Env, key: Symbol) -> i32 {
+        e.storage().persistent().get(&key).unwrap()
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use soroban_sdk::{symbol_short, Env, Map};
+
+    #[test]
+    fn test_map_storage() {
+        // Given
+        let env = Env::default();
+        let contract_id = env.register_contract(None, MapStorage);
+        let client = MapStorageClient::new(&env, &contract_id);
+
+        // When
+        let mut test_map = Map::new(&env);
+        test_map.set(symbol_short!("key1"), 1);
+        test_map.set(symbol_short!("key2"), 2);
+        client.store_map(&test_map);
+
+        // Then
+        let key1 = client.get_key(&symbol_short!("key1"));
+        let key2 = client.get_key(&symbol_short!("key2"));
+        assert_eq!(test_map.get(symbol_short!("key1")).unwrap(), key1);
+        assert_eq!(test_map.get(symbol_short!("key2")).unwrap(), key2);
+    }
+}