diff --git a/detectors/Cargo.toml b/detectors/Cargo.toml index 844c937b..a5288e26 100644 --- a/detectors/Cargo.toml +++ b/detectors/Cargo.toml @@ -8,3 +8,4 @@ dylint_linting = { package = "scout-audit-dylint-linting", version = "3.0.1" } if_chain = "1.0.2" scout-audit-clippy-utils = { version = "=0.2.3" } utils = { path = "../utils" } +itertools = { version = "0.12" } diff --git a/detectors/vec-could-be-mapping/Cargo.toml b/detectors/vec-could-be-mapping/Cargo.toml new file mode 100644 index 00000000..aec2b76e --- /dev/null +++ b/detectors/vec-could-be-mapping/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "vec-could-be-mapping" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { workspace = true } +if_chain = { workspace = true } +itertools = { workspace = true } + +scout-audit-clippy-utils = { workspace = true } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/detectors/vec-could-be-mapping/src/lib.rs b/detectors/vec-could-be-mapping/src/lib.rs new file mode 100644 index 00000000..e32df4b4 --- /dev/null +++ b/detectors/vec-could-be-mapping/src/lib.rs @@ -0,0 +1,378 @@ +#![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; +extern crate rustc_type_ir; + +use rustc_hir::{ + def::Res, + intravisit::{walk_expr, FnKind, Visitor}, + Body, Expr, ExprKind, FnDecl, GenericArg, HirId, Local, Path, PathSegment, QPath, Stmt, + StmtKind, Ty, TyKind, +}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{GenericArgKind, Interner, TyCtxt}; +use rustc_span::{def_id::LocalDefId, 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."; + +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 {} + +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 = FindIterations { + cx, + uses_as_hashmap: Vec::new(), + function_body: body, + }; + + walk_expr(&mut vec_mapping_storage, body.value); + + vec_mapping_storage.uses_as_hashmap.iter().for_each(|span| { + span_lint_and_help( + cx, + VEC_COULD_BE_MAPPING, + *span, + LINT_MESSAGE, + None, + "Change this to a parametrized enum as storage key", + ); + }); + } +} + +struct FindIterations<'a, 'b, 'c> { + cx: &'b LateContext<'a>, + uses_as_hashmap: Vec, + function_body: &'b Body<'c>, +} + +impl<'a, 'b, 'c> FindIterations<'a, 'b, 'c> { + fn vector_comes_from_local_set_from_storage<'hir>( + &mut self, + receiver: &'hir Expr<'hir>, + ) -> bool { + || -> Result { + let path = expr_to_path(&receiver.kind)?; + let (_, object_path) = path_to_resolved(&path)?; + let object_decl_hir_id = resolution_to_local(&object_path.res)?; + let mut fst = FindStorageLocal { + cx: self.cx, + result: false, + id: object_decl_hir_id, + }; + walk_expr(&mut fst, self.function_body.value); + + Ok(fst.result) + }() + .unwrap_or(false) + } + + fn vector_comes_directly_from_storage<'hir>(&mut self, receiver: &'hir Expr<'hir>) -> bool { + let mut fgse = FindGetStorageExpression { + cx: self.cx, + result: false, + }; + walk_expr(&mut fgse, receiver); + fgse.result + } + + fn visit_expr_internal(&mut self, expr: &Expr<'_>) -> Result<(), ()> { + let (function_name, receiver, _, _) = expr_to_method_call(&expr.kind)?; + if function_name.ident.as_str() != "find" { + return Ok(()); + } + let receiver_type = get_type_string(self.cx, &receiver.hir_id)?; + if receiver_type != "soroban_sdk::iter::UnwrappedIter" { + return Ok(()); + } + + let (function_name, receiver, _, _) = expr_to_method_call(&receiver.kind)?; + if function_name.ident.as_str() != "iter" { + return Ok(()); + } + + let (def, generic_args) = type_to_adt(get_node_type(self.cx, &receiver.hir_id).kind())?; + if definition_to_string(self.cx, def.did()) != "soroban_sdk::vec::Vec" { + return Ok(()); + } + + if generic_args.len() != 1 { + return Ok(()); + } + + let generic_arg = generic_args.first().unwrap().unpack(); + + let type_string = { + if let GenericArgKind::Type(x) = generic_arg { + Ok(x) + } else { + Err(()) + } + }? + .to_string(); + let n = type_string.len(); + if !(n > 2 + && type_string.chars().nth(0).unwrap() == '(' + && type_string.chars().nth(n - 1).unwrap() == ')') + { + return Ok(()); + } + + //Iterating over a vector of tuples. Does it come from storage? + if self.vector_comes_from_local_set_from_storage(receiver) + || self.vector_comes_directly_from_storage(receiver) + { + self.uses_as_hashmap.push(expr.span); + } + + Ok(()) + } +} + +struct FindStorageLocal<'a, 'b> { + cx: &'b LateContext<'a>, + result: bool, + id: &'b HirId, +} + +impl<'a, 'b> FindStorageLocal<'a, 'b> { + fn visit_stmt_internal(&mut self, stmt: &Stmt<'_>) -> Result { + let let_struct = stmt_to_local(&stmt.kind)?; + if let_struct.pat.hir_id != *self.id || let_struct.init.is_none() { + return Ok(false); + } + + let init = let_struct.init.unwrap(); + + let mut fgse = FindGetStorageExpression { + cx: self.cx, + result: false, + }; + walk_expr(&mut fgse, init); + + Ok(fgse.result) + } +} + +impl<'tcx, 'a, 'b> Visitor<'tcx> for FindStorageLocal<'a, 'b> { + fn visit_stmt(&mut self, stmt: &'tcx Stmt<'_>) { + if self.result { + return; + } + if let Ok(r) = self.visit_stmt_internal(stmt) { + self.result = r; + } + //walk_expr(self, expr); + } +} + +struct FindGetStorageExpression<'a, 'b> { + cx: &'b LateContext<'a>, + result: bool, +} + +impl<'tcx, 'a, 'b, 'c> Visitor<'tcx> for FindIterations<'a, 'b, 'c> { + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + let _ = self.visit_expr_internal(expr); + walk_expr(self, expr); + } +} + +fn get_node_type<'a>(cx: &LateContext<'a>, hir_id: &HirId) -> rustc_middle::ty::Ty<'a> { + cx.typeck_results().node_type(*hir_id) +} + +fn definition_to_string(cx: &LateContext<'_>, did: rustc_hir::def_id::DefId) -> String { + cx.get_def_path(did) + .iter() + .map(|x| x.to_string()) + .collect::>() + .join("::") +} + +fn get_type_string(cx: &LateContext<'_>, hir_id: &HirId) -> Result { + let (def, _generic_args) = type_to_adt(get_node_type(cx, hir_id).kind())?; + Ok(definition_to_string(cx, def.did())) +} + +fn is_storage(cx: &LateContext<'_>, hir_id: &HirId) -> bool { + let receiver_type = get_type_string(cx, hir_id); + if let Ok(receiver_type) = receiver_type { + receiver_type == "soroban_sdk::storage::Persistent" + || receiver_type != "soroban_sdk::storage::Temporary" + || receiver_type != "soroban_sdk::storage::Instance" + } else { + false + } +} + +impl<'a, 'b> FindGetStorageExpression<'a, 'b> { + fn visit_expr_internal(&mut self, expr: &Expr<'_>) -> Result<(), ()> { + let (function_name, receiver, _, _) = expr_to_method_call(&expr.kind)?; + if function_name.ident.as_str() != "get" { + return Ok(()); + } + if !is_storage(self.cx, &receiver.hir_id) { + return Ok(()); + } + let generic_args = { + if let Some(x) = function_name.args { + Ok(x) + } else { + Err(()) + } + }?; + let generic_args = generic_args.args; + if generic_args.len() != 2 { + return Ok(()); + } + let data_type = generic_args[1]; + if get_type_string(self.cx, &data_type.hir_id())? != "soroban_sdk::vec::Vec" { + return Ok(()); + } + let data_type = { + if let GenericArg::Type(x) = data_type { + Ok(x) + } else { + Err(()) + } + }?; + let path = { + if let TyKind::Path(x) = data_type.kind + && let QPath::Resolved(_, x) = x + { + Ok(x) + } else { + Err(()) + } + }?; + let last_segment = path.segments.last().ok_or(())?; + let args = { + if let Some(x) = last_segment.args { + Ok(x) + } else { + Err(()) + } + }? + .args; + if args.len() != 1 { + return Ok(()); + } + let argument = args.first().unwrap(); + let argument = { + if let GenericArg::Type(x) = argument { + Ok(x) + } else { + Err(()) + } + }?; + + self.result = matches!(argument.kind, TyKind::Tup(_)); + + Ok(()) + } +} + +fn resolution_to_local(resolution: &Res) -> Result<&HirId, ()> { + if let Res::Local(a) = resolution { + Ok(a) + } else { + Err(()) + } +} + +fn path_to_resolved<'hir>( + path: &'hir QPath<'hir>, +) -> Result<(&'hir Option<&'hir Ty<'hir>>, &'hir Path<'hir>), ()> { + if let QPath::Resolved(a, b) = path { + Ok((a, b)) + } else { + Err(()) + } +} + +fn expr_to_path<'hir>(kind: &'hir ExprKind<'hir>) -> Result, ()> { + if let ExprKind::Path(a) = kind { + Ok(*a) + } else { + Err(()) + } +} + +fn expr_to_method_call<'hir>( + kind: &'hir ExprKind<'hir>, +) -> Result< + ( + &'hir PathSegment<'hir>, + &'hir Expr<'hir>, + &'hir [Expr<'hir>], + Span, + ), + (), +> { + if let ExprKind::MethodCall(a, b, c, d) = kind { + Ok((a, b, c, *d)) + } else { + Err(()) + } +} + +fn stmt_to_local<'hir>(kind: &'hir StmtKind<'hir>) -> Result<&'hir Local<'hir>, ()> { + if let StmtKind::Local(a) = kind { + Ok(a) + } else { + Err(()) + } +} + +fn type_to_adt<'hir>( + kind: &'hir rustc_type_ir::TyKind>, +) -> Result< + ( + &'hir as Interner>::AdtDef, + &'hir as Interner>::GenericArgs, + ), + (), +> { + if let rustc_type_ir::TyKind::Adt(a, b) = kind { + Ok((&a, &b)) + } else { + Err(()) + } +} + +impl<'tcx, 'a, 'b> Visitor<'tcx> for FindGetStorageExpression<'a, 'b> { + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + let _ = self.visit_expr_internal(expr); + walk_expr(self, expr); + } +} diff --git a/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/Cargo.toml b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..8d726cb7 --- /dev/null +++ b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "vec-could-be-mapping-remediated-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = "20.0.0-rc2" + +[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 \ No newline at end of file diff --git a/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/rust-toolchain b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/rust-toolchain new file mode 100644 index 00000000..8811e92c --- /dev/null +++ b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/rust-toolchain @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2023-09-29" +components = ["llvm-tools-preview", "rustc-dev"] diff --git a/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/src/lib.rs b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..4bb70f0a --- /dev/null +++ b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/remediated-example/src/lib.rs @@ -0,0 +1,42 @@ +#![no_std] + +#[cfg(any(test, feature = "testutils"))] +extern crate std; + +use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Address, Env}; + +#[contracttype] +#[derive(Clone)] +enum DataKey { + Data(Address), +} + +#[contract] +pub struct NonPayableTransferredValueContract; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + Ununitialized = 1, + NoData = 2, + NotFound = 3, +} + +#[contractimpl] +impl NonPayableTransferredValueContract { + pub fn add_data(e: Env, key: Address, value: i64) -> Result<(), Error> { + e.storage().persistent().set(&DataKey::Data(key), &value); + Ok(()) + } + + pub fn get(e: Env, key: Address) -> Result { + e.storage() + .persistent() + .get::(&DataKey::Data(key)) + .ok_or(Error::NotFound) + } +} + +#[test] +fn simple_test() {} diff --git a/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/Cargo.toml b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..f6d61396 --- /dev/null +++ b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "vec-could-be-mapping-vulnerable-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = "20.0.0-rc2" + +[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 \ No newline at end of file diff --git a/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/rust-toolchain b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/rust-toolchain new file mode 100644 index 00000000..8811e92c --- /dev/null +++ b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/rust-toolchain @@ -0,0 +1,3 @@ +[toolchain] +channel = "nightly-2023-09-29" +components = ["llvm-tools-preview", "rustc-dev"] diff --git a/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/src/lib.rs b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..07a87700 --- /dev/null +++ b/test-cases/vec-could-be-mapping/vec-could-be-mapping-1/vulnerable-example/src/lib.rs @@ -0,0 +1,71 @@ +#![no_std] + +#[cfg(any(test, feature = "testutils"))] +extern crate std; + +use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Address, Env}; + +#[contracttype] +#[derive(Clone)] +enum DataKey { + Data, +} + +#[contract] +pub struct NonPayableTransferredValueContract; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + Ununitialized = 1, + NoData = 2, + NotFound = 3, +} + +#[contractimpl] +impl NonPayableTransferredValueContract { + pub fn init(e: Env) { + e.storage() + .persistent() + .set(&DataKey::Data, &soroban_sdk::Vec::<(Address, i64)>::new(&e)); + } + + pub fn add_data(e: Env, key: Address, value: i64) -> Result<(), Error> { + let mut data = e + .storage() + .persistent() + .get::>(&DataKey::Data) + .ok_or(Error::Ununitialized)?; + data.push_back((key, value)); + e.storage().persistent().set(&DataKey::Data, &data); + Ok(()) + } + + pub fn get(e: Env, key: Address) -> Result { + let data = e + .storage() + .persistent() + .get::>(&DataKey::Data) + .ok_or(Error::Ununitialized)?; + data.iter() + .find(|(a, _)| *a == key) + .map(|(_, b)| b) + .ok_or(Error::NotFound) + } + + //Second sub-optimal example. + pub fn get2(e: Env, key: Address) -> Result { + e.storage() + .persistent() + .get::>(&DataKey::Data) + .ok_or(Error::Ununitialized)? + .iter() + .find(|(a, _)| *a == key) + .map(|(_, b)| b) + .ok_or(Error::NotFound) + } +} + +#[test] +fn simple_test() {}