Improve mapping-deletion detector for nested mappings (#2084)

* Fix for nested mappings * Add test
crytic · Sep 8, 2023 · 8d5c033 · 8d5c033
1 parent 8b07fe5
commit 8d5c033
Show file tree

Hide file tree

Showing 13 changed files with 86 additions and 21 deletions.
diff --git a/slither/detectors/statements/mapping_deletion.py b/slither/detectors/statements/mapping_deletion.py
@@ -6,6 +6,7 @@
 from slither.core.cfg.node import Node
 from slither.core.declarations import Structure
 from slither.core.declarations.contract import Contract
+from slither.core.variables.variable import Variable
 from slither.core.declarations.function_contract import FunctionContract
 from slither.core.solidity_types import MappingType, UserDefinedType
 from slither.detectors.abstract_detector import (
@@ -69,14 +70,25 @@ def detect_mapping_deletion(
                 for ir in node.irs:
                     if isinstance(ir, Delete):
                         value = ir.variable
-                        if isinstance(value.type, UserDefinedType) and isinstance(
-                            value.type.type, Structure
-                        ):
-                            st = value.type.type
-                            if any(isinstance(e.type, MappingType) for e in st.elems.values()):
-                                ret.append((f, st, node))
+                        MappingDeletionDetection.check_if_mapping(value, ret, f, node)
+
         return ret
 
+    @staticmethod
+    def check_if_mapping(
+        value: Variable,
+        ret: List[Tuple[FunctionContract, Structure, Node]],
+        f: FunctionContract,
+        node: Node,
+    ):
+        if isinstance(value.type, UserDefinedType) and isinstance(value.type.type, Structure):
+            st = value.type.type
+            if any(isinstance(e.type, MappingType) for e in st.elems.values()):
+                ret.append((f, st, node))
+                return
+            for e in st.elems.values():
+                MappingDeletionDetection.check_if_mapping(e, ret, f, node)
+
     def _detect(self) -> List[Output]:
         """Detect mapping deletion
 

diff --git a/.../snapshots/detectors__detector_MappingDeletionDetection_0_4_25_MappingDeletion_sol__0.txt b/.../snapshots/detectors__detector_MappingDeletionDetection_0_4_25_MappingDeletion_sol__0.txt
@@ -1,6 +1,9 @@
+Balances.deleteNestedBalance() (tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol#40-42) deletes Balances.BalancesStruct (tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol#17-20) which contains a mapping:
+	-delete nestedStackBalance (tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol#41)
+
 Lib.deleteSt(Lib.MyStruct[1]) (tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol#9-11) deletes Lib.MyStruct (tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol#5-7) which contains a mapping:
 	-delete st[0] (tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol#10)
 
-Balances.deleteBalance(uint256) (tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol#28-31) deletes Balances.BalancesStruct (tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol#17-20) which contains a mapping:
-	-delete stackBalance[idx] (tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol#30)
+Balances.deleteBalance(uint256) (tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol#35-38) deletes Balances.BalancesStruct (tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol#17-20) which contains a mapping:
+	-delete stackBalance[idx] (tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol#37)
 
diff --git a/.../snapshots/detectors__detector_MappingDeletionDetection_0_5_16_MappingDeletion_sol__0.txt b/.../snapshots/detectors__detector_MappingDeletionDetection_0_5_16_MappingDeletion_sol__0.txt
@@ -1,6 +1,9 @@
 Lib.deleteSt(Lib.MyStruct[1]) (tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol#9-11) deletes Lib.MyStruct (tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol#5-7) which contains a mapping:
 	-delete st[0] (tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol#10)
 
-Balances.deleteBalance(uint256) (tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol#29-32) deletes Balances.BalancesStruct (tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol#17-20) which contains a mapping:
-	-delete stackBalance[idx] (tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol#31)
+Balances.deleteBalance(uint256) (tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol#35-38) deletes Balances.BalancesStruct (tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol#17-20) which contains a mapping:
+	-delete stackBalance[idx] (tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol#37)
+
+Balances.deleteNestedBalance() (tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol#40-42) deletes Balances.BalancesStruct (tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol#17-20) which contains a mapping:
+	-delete nestedStackBalance (tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol#41)
 
diff --git a/.../snapshots/detectors__detector_MappingDeletionDetection_0_6_11_MappingDeletion_sol__0.txt b/.../snapshots/detectors__detector_MappingDeletionDetection_0_6_11_MappingDeletion_sol__0.txt
@@ -1,6 +1,9 @@
-Balances.deleteBalance(uint256) (tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol#29-32) deletes Balances.BalancesStruct (tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol#17-20) which contains a mapping:
-	-delete stackBalance[idx] (tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol#31)
+Balances.deleteNestedBalance() (tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol#40-42) deletes Balances.BalancesStruct (tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol#17-20) which contains a mapping:
+	-delete nestedStackBalance (tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol#41)
 
 Lib.deleteSt(Lib.MyStruct[1]) (tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol#9-11) deletes Lib.MyStruct (tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol#5-7) which contains a mapping:
 	-delete st[0] (tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol#10)
 
+Balances.deleteBalance(uint256) (tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol#35-38) deletes Balances.BalancesStruct (tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol#17-20) which contains a mapping:
+	-delete stackBalance[idx] (tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol#37)
+
diff --git a/...s/snapshots/detectors__detector_MappingDeletionDetection_0_7_6_MappingDeletion_sol__0.txt b/...s/snapshots/detectors__detector_MappingDeletionDetection_0_7_6_MappingDeletion_sol__0.txt
@@ -1,5 +1,8 @@
-Balances.deleteBalance(uint256) (tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol#29-32) deletes Balances.BalancesStruct (tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol#17-20) which contains a mapping:
-	-delete stackBalance[idx] (tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol#31)
+Balances.deleteNestedBalance() (tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol#40-42) deletes Balances.BalancesStruct (tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol#17-20) which contains a mapping:
+	-delete nestedStackBalance (tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol#41)
+
+Balances.deleteBalance(uint256) (tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol#35-38) deletes Balances.BalancesStruct (tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol#17-20) which contains a mapping:
+	-delete stackBalance[idx] (tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol#37)
 
 Lib.deleteSt(Lib.MyStruct[1]) (tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol#9-11) deletes Lib.MyStruct (tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol#5-7) which contains a mapping:
 	-delete st[0] (tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol#10)

diff --git a/tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol b/tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol
@@ -6,7 +6,7 @@ library Lib{
         mapping(address => uint) maps;
     }
 
-    function deleteSt(MyStruct[1] storage st){
+    function deleteSt(MyStruct[1] storage st) internal {
         delete st[0];
     }
 
@@ -17,18 +17,29 @@ contract Balances {
     struct BalancesStruct{
         address owner;
         mapping(address => uint) balances;
-    } 
+    }
+
+    struct NestedBalanceStruct {
+        BalancesStruct balanceStruct;
+    }
 
     mapping(uint => BalancesStruct) public stackBalance;
+    NestedBalanceStruct internal nestedStackBalance;
+
     function createBalance(uint idx) public {
-        require(stackBalance[idx].owner == 0);
-        stackBalance[idx] = BalancesStruct(msg.sender);
+        require(stackBalance[idx].owner == address(0));
+        BalancesStruct storage str = stackBalance[idx];
+        str.owner = msg.sender;
     }    
 
     function deleteBalance(uint idx) public {
         require(stackBalance[idx].owner == msg.sender);
         delete stackBalance[idx];
     }
+
+    function deleteNestedBalance() public {
+        delete nestedStackBalance;
+    }
 
     function setBalance(uint idx, address addr, uint val) public {
         require(stackBalance[idx].owner == msg.sender);

diff --git a/tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol-0.4.25.zip b/tests/e2e/detectors/test_data/mapping-deletion/0.4.25/MappingDeletion.sol-0.4.25.zip
diff --git a/tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol b/tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol
@@ -17,9 +17,15 @@ contract Balances {
     struct BalancesStruct{
         address owner;
         mapping(address => uint) balances;
-    } 
+    }
+
+    struct NestedBalanceStruct {
+        BalancesStruct balanceStruct;
+    }
 
     mapping(uint => BalancesStruct) public stackBalance;
+    NestedBalanceStruct internal nestedStackBalance;
+
     function createBalance(uint idx) public {
         require(stackBalance[idx].owner == address(0));
         BalancesStruct storage str = stackBalance[idx];
@@ -30,6 +36,10 @@ contract Balances {
         require(stackBalance[idx].owner == msg.sender);
         delete stackBalance[idx];
     }
+
+    function deleteNestedBalance() public {
+        delete nestedStackBalance;
+    }
 
     function setBalance(uint idx, address addr, uint val) public {
         require(stackBalance[idx].owner == msg.sender);

diff --git a/tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol-0.5.16.zip b/tests/e2e/detectors/test_data/mapping-deletion/0.5.16/MappingDeletion.sol-0.5.16.zip
diff --git a/tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol b/tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol
@@ -17,9 +17,15 @@ contract Balances {
     struct BalancesStruct{
         address owner;
         mapping(address => uint) balances;
-    } 
+    }
+
+    struct NestedBalanceStruct {
+        BalancesStruct balanceStruct;
+    }
 
     mapping(uint => BalancesStruct) public stackBalance;
+    NestedBalanceStruct internal nestedStackBalance;
+
     function createBalance(uint idx) public {
         require(stackBalance[idx].owner == address(0));
         BalancesStruct storage str = stackBalance[idx];
@@ -30,6 +36,10 @@ contract Balances {
         require(stackBalance[idx].owner == msg.sender);
         delete stackBalance[idx];
     }
+
+    function deleteNestedBalance() public {
+        delete nestedStackBalance;
+    }
 
     function setBalance(uint idx, address addr, uint val) public {
         require(stackBalance[idx].owner == msg.sender);

diff --git a/tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol-0.6.11.zip b/tests/e2e/detectors/test_data/mapping-deletion/0.6.11/MappingDeletion.sol-0.6.11.zip
diff --git a/tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol b/tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol
@@ -17,9 +17,15 @@ contract Balances {
     struct BalancesStruct{
         address owner;
         mapping(address => uint) balances;
-    } 
+    }
+
+    struct NestedBalanceStruct {
+        BalancesStruct balanceStruct;
+    }
 
     mapping(uint => BalancesStruct) public stackBalance;
+    NestedBalanceStruct internal nestedStackBalance;
+
     function createBalance(uint idx) public {
         require(stackBalance[idx].owner == address(0));
         BalancesStruct storage str = stackBalance[idx];
@@ -30,6 +36,10 @@ contract Balances {
         require(stackBalance[idx].owner == msg.sender);
         delete stackBalance[idx];
     }
+
+    function deleteNestedBalance() public {
+        delete nestedStackBalance;
+    }
 
     function setBalance(uint idx, address addr, uint val) public {
         require(stackBalance[idx].owner == msg.sender);

diff --git a/tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol-0.7.6.zip b/tests/e2e/detectors/test_data/mapping-deletion/0.7.6/MappingDeletion.sol-0.7.6.zip