Detector: Function signature collision

aderyn_core/src/detect/detector.rs

@@ -92,6 +92,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
         Box::<CostlyOperationsInsideLoopsDetector>::default(),
         Box::<ConstantFunctionChangingStateDetector>::default(),
         Box::<BuiltinSymbolShadowDetector>::default(),
+        Box::<FunctionSelectorCollisionDetector>::default(),
     ]
 }
 
@@ -103,6 +104,7 @@ pub fn get_all_detectors_names() -> Vec<String> {
 #[derive(Debug, PartialEq, EnumString, Display)]
 #[strum(serialize_all = "kebab-case")]
 pub(crate) enum IssueDetectorNamePool {
+    FunctionSelectorCollision,
     CacheArrayLength,
     AssertStateChange,
     CostlyOperationsInsideLoops,
@@ -187,6 +189,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option<Box<dyn Iss
     // Expects a valid detector_name
     let detector_name = IssueDetectorNamePool::from_str(detector_name).ok()?;
     match detector_name {
+        IssueDetectorNamePool::FunctionSelectorCollision => {
+            Some(Box::<FunctionSelectorCollisionDetector>::default())
+        }
         IssueDetectorNamePool::CacheArrayLength => Some(Box::<CacheArrayLengthDetector>::default()),
         IssueDetectorNamePool::AssertStateChange => {
             Some(Box::<AssertStateChangeDetector>::default())

aderyn_core/src/detect/high/function_selector_collision.rs

@@ -0,0 +1,117 @@
+use std::collections::{BTreeMap, HashMap};
+use std::error::Error;
+
+use crate::ast::NodeID;
+
+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 FunctionSelectorCollisionDetector {
+    // 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 FunctionSelectorCollisionDetector {
+    fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
+        // function_selector -> (function_name -> function_id)
+        let mut selectors: HashMap<String, HashMap<String, Vec<NodeID>>> = HashMap::new();
+
+        // PLAN
+        // If we have > 1 function_name entries for any function_selector, then capture all the corresponding NodeIDs
+
+        for function in context.function_definitions() {
+            if let Some(selector) = function.function_selector.as_ref() {
+                let name = &function.name;
+                match selectors.entry(selector.clone()) {
+                    std::collections::hash_map::Entry::Occupied(mut o) => {
+                        match o.get_mut().entry(name.clone()) {
+                            std::collections::hash_map::Entry::Occupied(mut o) => {
+                                o.get_mut().push(function.id);
+                            }
+                            std::collections::hash_map::Entry::Vacant(v) => {
+                                v.insert(vec![function.id]);
+                            }
+                        };
+                    }
+                    std::collections::hash_map::Entry::Vacant(v) => {
+                        let mut nested_entry = HashMap::new();
+                        nested_entry.insert(name.clone(), vec![function.id]);
+                        v.insert(nested_entry);
+                    }
+                }
+            }
+        }
+
+        for function_entries in selectors.values() {
+            if function_entries.len() >= 2 {
+                // Now we know that there is a collision + at least 2 different function names found for that selector.
+                for function_ids in function_entries.values() {
+                    for function_id in function_ids {
+                        if let Some(node) = context.nodes.get(function_id) {
+                            capture!(self, context, node);
+                        }
+                    }
+                }
+            }
+        }
+
+        Ok(!self.found_instances.is_empty())
+    }
+
+    fn severity(&self) -> IssueSeverity {
+        IssueSeverity::High
+    }
+
+    fn title(&self) -> String {
+        String::from("Function selector collides with other functions")
+    }
+
+    fn description(&self) -> String {
+        String::from("Function selector collides with other functions. This may cause the solidity function dispatcher to invoke the wrong function if the functions happen to be included in the same contract through an inheritance hirearchy later down the line. It is recommended to rename this function or change its parameters.")
+    }
+
+    fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> {
+        self.found_instances.clone()
+    }
+
+    fn name(&self) -> String {
+        format!("{}", IssueDetectorNamePool::FunctionSelectorCollision)
+    }
+}
+
+#[cfg(test)]
+mod function_signature_collision {
+    use serial_test::serial;
+
+    use crate::detect::{
+        detector::IssueDetector,
+        high::function_selector_collision::FunctionSelectorCollisionDetector,
+    };
+
+    #[test]
+    #[serial]
+    fn test_function_signature_collision() {
+        let context = crate::detect::test_utils::load_solidity_source_unit(
+            "../tests/contract-playground/src/FunctionSignatureCollision.sol",
+        );
+
+        let mut detector = FunctionSelectorCollisionDetector::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(), 2);
+        // assert the severity is high
+        assert_eq!(
+            detector.severity(),
+            crate::detect::detector::IssueSeverity::High
+        );
+    }
+}

aderyn_core/src/detect/high/mod.rs

@@ -11,6 +11,7 @@ pub(crate) mod deletion_nested_mapping;
 pub(crate) mod dynamic_array_length_assignment;
 pub(crate) mod enumerable_loop_removal;
 pub(crate) mod experimental_encoder;
+pub(crate) mod function_selector_collision;
 pub(crate) mod incorrect_caret_operator;
 pub(crate) mod incorrect_erc20_interface;
 pub(crate) mod incorrect_erc721_interface;
@@ -52,6 +53,7 @@ pub use deletion_nested_mapping::DeletionNestedMappingDetector;
 pub use dynamic_array_length_assignment::DynamicArrayLengthAssignmentDetector;
 pub use enumerable_loop_removal::EnumerableLoopRemovalDetector;
 pub use experimental_encoder::ExperimentalEncoderDetector;
+pub use function_selector_collision::FunctionSelectorCollisionDetector;
 pub use incorrect_caret_operator::IncorrectUseOfCaretOperatorDetector;
 pub use incorrect_erc20_interface::IncorrectERC20InterfaceDetector;
 pub use incorrect_erc721_interface::IncorrectERC721InterfaceDetector;

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

@@ -201,6 +201,7 @@
     "incorrect-erc721-interface",
     "incorrect-erc20-interface",
     "out-of-order-retryable",
-    "constant-function-changing-state"
+    "constant-function-changing-state",
+    "function-selector-collision"
   ]
 }

reports/report.json

@@ -1,7 +1,7 @@
 {
   "files_summary": {
-    "total_source_units": 91,
-    "total_sloc": 3221
+    "total_source_units": 92,
+    "total_sloc": 3230
   },
   "files_details": {
     "files_details": [
@@ -129,6 +129,10 @@
         "file_path": "src/FunctionInitializingState.sol",
         "n_sloc": 49
       },
+      {
+        "file_path": "src/FunctionSignatureCollision.sol",
+        "n_sloc": 9
+      },
       {
         "file_path": "src/HugeConstants.sol",
         "n_sloc": 36
@@ -372,7 +376,7 @@
     ]
   },
   "issue_count": {
-    "high": 40,
+    "high": 41,
     "low": 34
   },
   "high_issues": {
@@ -2324,6 +2328,25 @@
             "src_char": "173:112"
           }
         ]
+      },
+      {
+        "title": "Function selector collides with other functions",
+        "description": "Function selector collides with other functions. This may cause the solidity function dispatcher to invoke the wrong function if the functions happen to be included in the same contract through an inheritance hirearchy later down the line. It is recommended to rename this function or change its parameters.",
+        "detector_name": "function-selector-collision",
+        "instances": [
+          {
+            "contract_path": "src/FunctionSignatureCollision.sol",
+            "line_no": 7,
+            "src": "166:8",
+            "src_char": "166:8"
+          },
+          {
+            "contract_path": "src/FunctionSignatureCollision.sol",
+            "line_no": 13,
+            "src": "231:22",
+            "src_char": "231:22"
+          }
+        ]
       }
     ]
   },
@@ -2639,6 +2662,12 @@
             "src": "32:21",
             "src_char": "32:21"
           },
+          {
+            "contract_path": "src/FunctionSignatureCollision.sol",
+            "line_no": 2,
+            "src": "32:23",
+            "src_char": "32:23"
+          },
           {
             "contract_path": "src/InconsistentUints.sol",
             "line_no": 1,
@@ -3871,6 +3900,12 @@
             "src": "32:23",
             "src_char": "32:23"
           },
+          {
+            "contract_path": "src/FunctionSignatureCollision.sol",
+            "line_no": 2,
+            "src": "32:23",
+            "src_char": "32:23"
+          },
           {
             "contract_path": "src/InconsistentUints.sol",
             "line_no": 1,
@@ -4221,6 +4256,18 @@
             "src": "1219:41",
             "src_char": "1219:41"
           },
+          {
+            "contract_path": "src/FunctionSignatureCollision.sol",
+            "line_no": 7,
+            "src": "166:8",
+            "src_char": "166:8"
+          },
+          {
+            "contract_path": "src/FunctionSignatureCollision.sol",
+            "line_no": 13,
+            "src": "231:22",
+            "src_char": "231:22"
+          },
           {
             "contract_path": "src/OnceModifierExample.sol",
             "line_no": 10,
@@ -5361,6 +5408,7 @@
     "assert-state-change",
     "costly-operations-inside-loops",
     "constant-function-changing-state",
-    "builtin-symbol-shadow"
+    "builtin-symbol-shadow",
+    "function-selector-collision"
   ]
 }