Merge pull request #255 from CoinFabrik/vec-could-be-mapping

New detector: Vec could be Mapping

.vscode/settings.json

@@ -1,8 +1,5 @@
 {
   "rust-analyzer.rustfmt.extraArgs": ["+nightly"],
   "rust-analyzer.showUnlinkedFileNotification": false,
-  "rust-analyzer.linkedProjects": [
-    "apps/cargo-scout-audit/Cargo.toml",
-    "detectors/Cargo.toml"
-  ]
+  "rust-analyzer.linkedProjects": ["detectors/Cargo.toml"]
 }

detectors/vec-could-be-mapping/Cargo.toml

@@ -0,0 +1,19 @@
+[package]
+name = "vec-could-be-mapping"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+scout-audit-clippy-utils = { workspace = true }
+dylint_linting = { workspace = true }
+if_chain = { workspace = true }
+itertools = {workspace = true}
+
+[dev-dependencies]
+dylint_testing = { workspace = true }
+
+[package.metadata.rust-analyzer]
+rustc_private = true

detectors/vec-could-be-mapping/src/lib.rs

@@ -0,0 +1,176 @@
+#![feature(rustc_private)]
+#![recursion_limit = "256"]
+#![feature(let_chains)]
+extern crate rustc_ast;
+extern crate rustc_hir;
+extern crate rustc_middle;
+extern crate rustc_span;
+
+use std::collections::HashMap;
+
+use itertools::Itertools;
+use rustc_hir::intravisit::{walk_expr, FnKind, Visitor};
+use rustc_hir::{Body, Expr, ExprKind, FnDecl, GenericArg, GenericArgs, PathSegment, QPath};
+use rustc_hir::{Ty, TyKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_span::def_id::LocalDefId;
+use rustc_span::Span;
+use scout_audit_clippy_utils::diagnostics::span_lint_and_help;
+
+const LINT_MESSAGE: &str =
+    "You are iterating over a vector of tuples using `find`. Consider using a mapping instead.";
+
+const ITERABLE_METHODS: [&str; 1] = ["find"];
+
+dylint_linting::impl_late_lint! {
+    pub VEC_COULD_BE_MAPPING,
+    Warn,
+    LINT_MESSAGE,
+    VecCouldBeMapping::default(),
+    {
+        name: "Vec could be Mapping",
+        long_message: "This vector could be a mapping. Consider changing it, because you are using `find` method in a vector of tuples",
+        severity: "Enhancement",
+        help: "https://coinfabrik.github.io/scout/docs/vulnerabilities/vec-could-be-mapping",
+        vulnerability_class: "Gas Usage",
+    }
+}
+
+#[derive(Default)]
+pub struct VecCouldBeMapping {
+    storage_names: HashMap<String, Span>,
+}
+
+impl<'tcx> LateLintPass<'tcx> for VecCouldBeMapping {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        _: FnKind<'tcx>,
+        _: &'tcx FnDecl<'_>,
+        body: &'tcx Body<'_>,
+        _: Span,
+        _: LocalDefId,
+    ) {
+        let mut vec_mapping_storage = VecMappingStorage {
+            storage_names: self.storage_names.clone(),
+            uses_as_hashmap: Vec::new(),
+        };
+
+        walk_expr(&mut vec_mapping_storage, body.value);
+
+        vec_mapping_storage
+            .uses_as_hashmap
+            .iter()
+            .for_each(|(span, field)| {
+                let field_sp = self.storage_names.get(field).copied();
+
+                span_lint_and_help(
+                    cx,
+                    VEC_COULD_BE_MAPPING,
+                    *span,
+                    LINT_MESSAGE,
+                    field_sp,
+                    "Change this to a `ink::storage::Mapping<...>`",
+                );
+            });
+    }
+
+    fn check_field_def(&mut self, _: &LateContext<'tcx>, fdef: &'tcx rustc_hir::FieldDef<'tcx>) {
+        println!("{:#?}", fdef);
+        if is_vec_type_with_tuple_of_2_elems(fdef) {
+            self.storage_names
+                .insert(fdef.ident.name.to_string(), fdef.span);
+        }
+    }
+}
+
+struct VecMappingStorage {
+    storage_names: HashMap<String, Span>,
+    uses_as_hashmap: Vec<(Span, String)>,
+}
+
+impl VecMappingStorage {
+    fn call_storage_and_any_or_find(&self, methods: &[String]) -> bool {
+        methods.first() == Some(&"self".to_string())
+            && methods
+                .iter()
+                .any(|method| ITERABLE_METHODS.contains(&method.as_str()))
+            && methods
+                .iter()
+                .any(|method| self.storage_names.keys().contains(method))
+    }
+}
+
+impl<'tcx> Visitor<'tcx> for VecMappingStorage {
+    fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
+        let (methods, field) = get_method_path_names(expr);
+
+        if !methods.is_empty() && self.call_storage_and_any_or_find(&methods) {
+            self.uses_as_hashmap.push((expr.span, field));
+        } else {
+            walk_expr(self, expr);
+        }
+    }
+}
+
+fn get_method_path_names(expr: &Expr<'_>) -> (Vec<String>, String) {
+    let mut full_method_ident = Vec::new();
+    let mut expr = expr;
+    let mut fields = Vec::new();
+
+    'names: loop {
+        match expr.kind {
+            ExprKind::MethodCall(PathSegment { ident, .. }, rec, ..) => {
+                full_method_ident.push(ident.name.to_string());
+                expr = rec;
+            }
+            ExprKind::Field(rec, ident) => {
+                full_method_ident.push(ident.name.to_string());
+                fields.push(ident.name.to_string());
+                expr = rec;
+            }
+            ExprKind::Path(QPath::Resolved(_, b)) => {
+                if !full_method_ident.is_empty() {
+                    for seg in b.segments.iter().rev() {
+                        full_method_ident.push(seg.ident.name.to_string());
+                    }
+                }
+                break 'names;
+            }
+            _ => {
+                break 'names;
+            }
+        }
+    }
+    full_method_ident.reverse();
+
+    let field = if full_method_ident.len() > 1
+        && full_method_ident[0] == "self"
+        && fields.contains(&full_method_ident[1])
+    {
+        full_method_ident[1].clone()
+    } else {
+        "".to_string()
+    };
+
+    (full_method_ident, field)
+}
+
+fn is_vec_type_with_tuple_of_2_elems(fdef: &'_ rustc_hir::FieldDef<'_>) -> bool {
+    if let TyKind::Path(QPath::Resolved(Some(Ty { kind, .. }), ..)) = fdef.ty.kind
+        && let TyKind::Path(QPath::Resolved(_, b)) = *kind
+        && let Some(last) = b.segments.iter().last()
+        && last.ident.name.to_string() == "Vec"
+        && let Some(GenericArgs { args, .. }) = last.args
+    {
+        for arg in args.iter() {
+            if let GenericArg::Type(ins) = *arg
+                && let TyKind::Tup(insides) = ins.kind
+                && insides.len() == 2
+            {
+                return true;
+            }
+        }
+    }
+    false
+}

test-cases/vec-could-be-mapping/Cargo.toml

@@ -0,0 +1,29 @@
+[workspace]
+exclude = [".cargo", "target"]
+members = ["vec-could-be-mapping-*/*"]
+resolver = "2"
+
+[workspace.dependencies]
+ink = { version = "5.0.0", default-features = false }
+scale = { package = "parity-scale-codec", version = "3", default-features = false, features = ["derive"] }
+scale-info = { version = "2.6", default-features = false, features = ["derive"] }
+ink_e2e = { version = "=5.0.0" }
+
+[profile.release]
+codegen-units = 1
+debug = 0
+debug-assertions = false
+lto = true
+opt-level = "z"
+overflow-checks = false
+panic = "abort"
+strip = "symbols"
+
+[profile.release-with-logs]
+debug-assertions = true
+inherits = "release"
+
+[workspace.metadata.dylint]
+libraries = [
+    { path = "/home/flerena/coinfabrik/scout/detectors/vec-could-be-mapping" },
+]

test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/Cargo.toml

@@ -0,0 +1,29 @@
+[package]
+name = "vec-could-be-mapping-remediated"
+version = "0.1.0"
+edition = "2021"
+authors = ["[your_name] <[your_email]>"]
+
+[lib]
+path = "src/lib.rs"
+
+[features]
+default = ["std"]
+std = [
+    "ink/std",
+    "scale/std",
+    "scale-info/std",
+]
+ink-as-dependency = []
+e2e-tests = []
+
+[dependencies]
+ink = { workspace = true }
+scale = { workspace = true }
+scale-info = { workspace = true }
+
+
+[dev-dependencies]
+ink_e2e = { workspace = true }
+
+

test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/src/lib.rs

@@ -0,0 +1,32 @@
+#![cfg_attr(not(feature = "std"), no_std, no_main)]
+
+#[ink::contract]
+mod vec_could_be_mapping {
+
+    use ink::storage::Mapping;
+
+    #[derive(Debug, PartialEq, Eq, scale::Encode, scale::Decode)]
+    #[cfg_attr(feature = "std", derive(scale_info::TypeInfo))]
+    pub enum Error {
+        NotFound,
+    }
+    #[ink(storage)]
+    pub struct VecCouldBeMapping {
+        balances: Mapping<AccountId, Balance>,
+    }
+
+    impl VecCouldBeMapping {
+        /// Creates a new instance of the contract.
+        #[ink(constructor)]
+        pub fn new() -> Self {
+            Self {
+                balances: Mapping::new(),
+            }
+        }
+        /// Returns the percentage difference between two values.
+        #[ink(message)]
+        pub fn get_balance(&mut self, acc: AccountId) -> Result<Balance, Error> {
+            self.balances.get(&acc).ok_or(Error::NotFound)
+        }
+    }
+}

test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/Cargo.toml

@@ -0,0 +1,27 @@
+[package]
+name = "vec-could-be-mapping-vulnerable"
+version = "0.1.0"
+edition = "2021"
+authors = ["[your_name] <[your_email]>"]
+
+[lib]
+path = "src/lib.rs"
+
+[features]
+default = ["std"]
+std = [
+    "ink/std",
+    "scale/std",
+    "scale-info/std",
+]
+ink-as-dependency = []
+e2e-tests = []
+
+[dependencies]
+ink = { workspace = true }
+scale = { workspace = true }
+scale-info = { workspace = true }
+
+
+[dev-dependencies]
+ink_e2e = { workspace = true }

test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/src/lib.rs

@@ -0,0 +1,36 @@
+#![cfg_attr(not(feature = "std"), no_std, no_main)]
+
+#[ink::contract]
+mod vec_could_be_mapping {
+
+    use ink::prelude::vec::Vec;
+
+    #[derive(Debug, PartialEq, Eq, scale::Encode, scale::Decode)]
+    #[cfg_attr(feature = "std", derive(scale_info::TypeInfo))]
+    pub enum Error {
+        NotFound,
+    }
+    #[ink(storage)]
+    pub struct VecCouldBeMapping {
+        balances: Vec<(AccountId, Balance)>,
+    }
+
+    impl VecCouldBeMapping {
+        /// Creates a new instance of the contract.
+        #[ink(constructor)]
+        pub fn new() -> Self {
+            Self {
+                balances: Vec::new(),
+            }
+        }
+        /// Returns the percentage difference between two values.
+        #[ink(message)]
+        pub fn get_balance(&mut self, acc: AccountId) -> Result<Balance, Error> {
+            self.balances
+                .iter()
+                .find(|(a, _)| *a == acc)
+                .map(|(_, b)| *b)
+                .ok_or(Error::NotFound)
+        }
+    }
+}