Detector: Deletion of nested mapping

aderyn_core/src/detect/detector.rs

@@ -8,15 +8,15 @@ use crate::{
         high::{
             ArbitraryTransferFromDetector, AvoidAbiEncodePackedDetector,
             BlockTimestampDeadlineDetector, DangerousUnaryOperatorDetector,
-            DelegateCallInLoopDetector, DynamicArrayLengthAssignmentDetector,
-            EnumerableLoopRemovalDetector, ExperimentalEncoderDetector,
-            IncorrectShiftOrderDetector, IncorrectUseOfCaretOperatorDetector,
-            MultipleConstructorsDetector, NestedStructInMappingDetector, RTLODetector,
-            ReusedContractNameDetector, SelfdestructIdentifierDetector,
-            StateVariableShadowingDetector, StorageArrayEditWithMemoryDetector,
-            TautologicalCompareDetector, UncheckedReturnDetector,
-            UninitializedStateVariableDetector, UnprotectedInitializerDetector,
-            UnsafeCastingDetector, YulReturnDetector,
+            DelegateCallInLoopDetector, DeletionNestedMappingDetector,
+            DynamicArrayLengthAssignmentDetector, EnumerableLoopRemovalDetector,
+            ExperimentalEncoderDetector, IncorrectShiftOrderDetector,
+            IncorrectUseOfCaretOperatorDetector, MultipleConstructorsDetector,
+            NestedStructInMappingDetector, RTLODetector, ReusedContractNameDetector,
+            SelfdestructIdentifierDetector, StateVariableShadowingDetector,
+            StorageArrayEditWithMemoryDetector, TautologicalCompareDetector,
+            UncheckedReturnDetector, UninitializedStateVariableDetector,
+            UnprotectedInitializerDetector, UnsafeCastingDetector, YulReturnDetector,
         },
         low::{
             CentralizationRiskDetector, ConstantsInsteadOfLiteralsDetector,
@@ -87,6 +87,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
         Box::<RTLODetector>::default(),
         Box::<UncheckedReturnDetector>::default(),
         Box::<DangerousUnaryOperatorDetector>::default(),
+        Box::<DeletionNestedMappingDetector>::default(),
     ]
 }
 
@@ -145,6 +146,7 @@ pub(crate) enum IssueDetectorNamePool {
     RTLO,
     UncheckedReturn,
     DangerousUnaryOperator,
+    DeleteNestedMapping,
     // NOTE: `Undecided` will be the default name (for new bots).
     // If it's accepted, a new variant will be added to this enum before normalizing it in aderyn
     Undecided,
@@ -273,6 +275,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option<Box<dyn Iss
         IssueDetectorNamePool::DangerousUnaryOperator => {
             Some(Box::<DangerousUnaryOperatorDetector>::default())
         }
+        IssueDetectorNamePool::DeleteNestedMapping => {
+            Some(Box::<DeletionNestedMappingDetector>::default())
+        }
         IssueDetectorNamePool::Undecided => None,
     }
 }

aderyn_core/src/detect/high/deletion_nested_mapping.rs

@@ -0,0 +1,135 @@
+use std::collections::BTreeMap;
+use std::error::Error;
+
+use crate::ast::{
+    ASTNode, Expression, Identifier, IndexAccess, Mapping, NodeID, TypeName, UserDefinedTypeName,
+    VariableDeclaration,
+};
+
+use crate::capture;
+use crate::detect::detector::IssueDetectorNamePool;
+use crate::{
+    context::workspace_context::WorkspaceContext,
+    detect::detector::{IssueDetector, IssueSeverity},
+};
+use eyre::Result;
+
+#[derive(Default)]
+pub struct DeletionNestedMappingDetector {
+    // Keys are: [0] source file name, [1] line number, [2] character location of node.
+    // Do not add items manually, use `capture!` to add nodes to this BTreeMap.
+    found_instances: BTreeMap<(String, usize, String), NodeID>,
+}
+
+impl IssueDetector for DeletionNestedMappingDetector {
+    fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
+        for delete_operation in context
+            .unary_operations()
+            .into_iter()
+            .filter(|op| op.operator == "delete")
+        {
+            if let Expression::IndexAccess(IndexAccess {
+                base_expression, ..
+            }) = delete_operation.sub_expression.as_ref()
+            {
+                if let Expression::Identifier(Identifier {
+                    referenced_declaration: Some(referenced_id),
+                    type_descriptions,
+                    ..
+                }) = base_expression.as_ref()
+                {
+                    // Check if we're deleting a value from mapping
+                    if type_descriptions
+                        .type_string
+                        .as_ref()
+                        .is_some_and(|type_string| type_string.starts_with("mapping"))
+                    {
+                        // Check if the value in the mapping is of type struct that has a member which is also a mapping
+                        if let Some(ASTNode::VariableDeclaration(VariableDeclaration {
+                            type_name: Some(TypeName::Mapping(Mapping { value_type, .. })),
+                            ..
+                        })) = context.nodes.get(referenced_id)
+                        {
+                            if let TypeName::UserDefinedTypeName(UserDefinedTypeName {
+                                referenced_declaration,
+                                ..
+                            }) = value_type.as_ref()
+                            {
+                                if let Some(ASTNode::StructDefinition(structure)) =
+                                    context.nodes.get(referenced_declaration)
+                                {
+                                    // Check that a member of a struct is of type mapping
+                                    if structure.members.iter().any(|member| {
+                                        member.type_descriptions.type_string.as_ref().is_some_and(
+                                            |type_string| type_string.starts_with("mapping"),
+                                        )
+                                    }) {
+                                        capture!(self, context, delete_operation);
+                                    }
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
+        Ok(!self.found_instances.is_empty())
+    }
+
+    fn severity(&self) -> IssueSeverity {
+        IssueSeverity::High
+    }
+
+    fn title(&self) -> String {
+        String::from("Deletion from a nested mappping.")
+    }
+
+    fn description(&self) -> String {
+        String::from("A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract.")
+    }
+
+    fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> {
+        self.found_instances.clone()
+    }
+
+    fn name(&self) -> String {
+        IssueDetectorNamePool::DeleteNestedMapping.to_string()
+    }
+}
+
+#[cfg(test)]
+mod deletion_nested_mapping_tests {
+    use crate::detect::{
+        detector::IssueDetector, high::deletion_nested_mapping::DeletionNestedMappingDetector,
+    };
+
+    #[test]
+    fn test_deletion_nested_mapping() {
+        let context = crate::detect::test_utils::load_solidity_source_unit(
+            "../tests/contract-playground/src/DeletionNestedMappingStructureContract.sol",
+        );
+
+        let mut detector = DeletionNestedMappingDetector::default();
+        let found = detector.detect(&context).unwrap();
+        // assert that the detector found an issue
+        assert!(found);
+        // assert that the detector found the correct number of instances
+        assert_eq!(detector.instances().len(), 1);
+        // assert the severity is high
+        assert_eq!(
+            detector.severity(),
+            crate::detect::detector::IssueSeverity::High
+        );
+        // assert the title is correct
+        assert_eq!(
+            detector.title(),
+            String::from("Deletion from a nested mappping.")
+        );
+        // assert the description is correct
+        assert_eq!(
+            detector.description(),
+            String::from("A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract.")
+        );
+    }
+}

aderyn_core/src/detect/high/mod.rs

@@ -3,6 +3,7 @@ pub(crate) mod avoid_abi_encode_packed;
 pub(crate) mod block_timestamp_deadline;
 pub(crate) mod dangerous_unary_operator;
 pub(crate) mod delegate_call_in_loop;
+pub(crate) mod deletion_nested_mapping;
 pub(crate) mod dynamic_array_length_assignment;
 pub(crate) mod enumerable_loop_removal;
 pub(crate) mod experimental_encoder;
@@ -27,6 +28,7 @@ pub use avoid_abi_encode_packed::AvoidAbiEncodePackedDetector;
 pub use block_timestamp_deadline::BlockTimestampDeadlineDetector;
 pub use dangerous_unary_operator::DangerousUnaryOperatorDetector;
 pub use delegate_call_in_loop::DelegateCallInLoopDetector;
+pub use deletion_nested_mapping::DeletionNestedMappingDetector;
 pub use dynamic_array_length_assignment::DynamicArrayLengthAssignmentDetector;
 pub use enumerable_loop_removal::EnumerableLoopRemovalDetector;
 pub use experimental_encoder::ExperimentalEncoderDetector;

reports/adhoc-sol-files-highs-only-report.json

@@ -171,6 +171,7 @@
     "tautological-compare",
     "rtlo",
     "unchecked-return",
-    "dangerous-unary-operator"
+    "dangerous-unary-operator",
+    "delete-nested-mapping"
   ]
 }

reports/report.json

@@ -1,7 +1,7 @@
 {
   "files_summary": {
-    "total_source_units": 60,
-    "total_sloc": 1620
+    "total_source_units": 61,
+    "total_sloc": 1631
   },
   "files_details": {
     "files_details": [
@@ -45,6 +45,10 @@
         "file_path": "src/DangerousUnaryOperator.sol",
         "n_sloc": 13
       },
+      {
+        "file_path": "src/DeletionNestedMappingStructureContract.sol",
+        "n_sloc": 11
+      },
       {
         "file_path": "src/DeprecatedOZFunctions.sol",
         "n_sloc": 32
@@ -248,7 +252,7 @@
     ]
   },
   "issue_count": {
-    "high": 23,
+    "high": 24,
     "low": 23
   },
   "high_issues": {
@@ -1427,6 +1431,19 @@
             "src_char": "247:10"
           }
         ]
+      },
+      {
+        "title": "Deletion from a nested mappping.",
+        "description": "A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract.",
+        "detector_name": "delete-nested-mapping",
+        "instances": [
+          {
+            "contract_path": "src/DeletionNestedMappingStructureContract.sol",
+            "line_no": 17,
+            "src": "426:25",
+            "src_char": "426:25"
+          }
+        ]
       }
     ]
   },
@@ -1658,6 +1675,12 @@
             "src": "32:23",
             "src_char": "32:23"
           },
+          {
+            "contract_path": "src/DeletionNestedMappingStructureContract.sol",
+            "line_no": 2,
+            "src": "32:23",
+            "src_char": "32:23"
+          },
           {
             "contract_path": "src/InconsistentUints.sol",
             "line_no": 1,
@@ -2242,6 +2265,12 @@
             "src": "32:32",
             "src_char": "32:32"
           },
+          {
+            "contract_path": "src/DeletionNestedMappingStructureContract.sol",
+            "line_no": 2,
+            "src": "32:23",
+            "src_char": "32:23"
+          },
           {
             "contract_path": "src/DeprecatedOZFunctions.sol",
             "line_no": 2,
@@ -2949,6 +2978,7 @@
     "tautological-compare",
     "rtlo",
     "unchecked-return",
-    "dangerous-unary-operator"
+    "dangerous-unary-operator",
+    "delete-nested-mapping"
   ]
 }