Merge pull request #159 from CoinFabrik/146-unrestricted-mapping-oper…

…ation-upgrades 146 unrestricted mapping operation upgrades
CoinFabrik · Aug 14, 2023 · 9d27b51 · 9d27b51
2 parents ba1f68a + 417bf0b
commit 9d27b51
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 7 deletions.
diff --git a/detectors/unprotected-mapping-operation/src/lib.rs b/detectors/unprotected-mapping-operation/src/lib.rs
@@ -59,8 +59,11 @@ impl<'tcx> LateLintPass<'tcx> for UnprotectedMappingOperation {
             fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
                 if let ExprKind::MethodCall(path, receiver, ..) = expr.kind {
                     let defid = self.cx.typeck_results().type_dependent_def_id(expr.hir_id);
-                    let ty = self.cx.tcx.mk_foreign(defid.unwrap());
-                    if ty.to_string().contains("ink::storage::Mapping"){
+
+                    let mapping_type = self.cx.typeck_results().expr_ty_adjusted(receiver);
+
+                    if mapping_type.to_string().contains("ink::storage::Mapping<ink::ink_primitives::AccountId") {
+
                         if path.ident.name.to_string() == "insert" {
                             self.insert_def_id = defid;
                         } else if path.ident.name.to_string() == "remove" {
@@ -75,7 +78,6 @@ impl<'tcx> LateLintPass<'tcx> for UnprotectedMappingOperation {
                         rec2_path.segments.first().map_or(false, |seg|seg.ident.to_string() == "self" &&
                         qualifier.is_none()) &&
                         path.ident.name.to_string() == "caller" {
-
                         self.caller_def_id = self.cx.typeck_results().type_dependent_def_id(expr.hir_id);
                     }
                 }
@@ -117,6 +119,8 @@ impl<'tcx> LateLintPass<'tcx> for UnprotectedMappingOperation {
                     if let Operand::Constant(fn_const) = func &&
                         let ConstantKind::Val(_const_val, ty) = fn_const.literal &&
                         let TyKind::FnDef(def, _subs) = ty.kind() {
+
+
                             if caller_def_id.is_some_and(|d: DefId|d==*def) {
                                 callers_vec.callers.push((bb_data, BasicBlock::from_usize(bb)));
                             } else {

diff --git a/docs/docs/detectors/22-unprotected-mapping-operation.md b/docs/docs/detectors/22-unprotected-mapping-operation.md
@@ -2,7 +2,7 @@
 
 ### What it does
 
-It warns you if a mapping operation (`insert`, `take`, `remove`) function is called with a user-given `key` field.
+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?
 
@@ -18,7 +18,6 @@ Modifying mappings with an arbitrary key given by users can be a significant vul
 
 ### Example
 
-
 ```rust
     #[ink(message)]
     pub fn withdraw(&mut self, amount: Balance, from: AccountId) -> Result<(), Error> {
@@ -54,4 +53,4 @@ Use instead:
 
 ### Implementation
 
-The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout/tree/main/detectors/unprotected-mapping-operation).
+The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout/tree/main/detectors/unprotected-mapping-operation).
diff --git a/...es/unprotected-mapping-operation/unprotected-mapping-operation1/vulnerable-example/lib.rs b/...es/unprotected-mapping-operation/unprotected-mapping-operation1/vulnerable-example/lib.rs
@@ -7,6 +7,7 @@ mod unprotected_mapping_operation {
     #[ink(storage)]
     pub struct UnprotectedMappingOperation {
         balances: Mapping<AccountId, Balance>,
+        another_mapping: Mapping<u128, AccountId>,
     }
 
     #[derive(Debug, PartialEq, Eq, Clone, scale::Encode, scale::Decode)]
@@ -21,12 +22,18 @@ mod unprotected_mapping_operation {
         pub fn new() -> Self {
             Self {
                 balances: Mapping::new(),
+                another_mapping: Mapping::new(),
             }
         }
 
+        #[ink(message)]
+        pub fn this_should_not_trigger(&mut self, key: u128, value: AccountId) {
+            self.another_mapping.insert(key, &value);
+        }
+
         #[ink(message, payable)]
         pub fn deposit(&mut self, dest: AccountId) {
-            let amount = self.env().transferred_value();
+            let amount: Balance = self.env().transferred_value();
             if let Some(current_bal) = self.balances.get(dest) {
                 self.balances.insert(dest, &(current_bal + amount));
             } else {