Low detector: Multiple placeholders in modifier (#726)

Co-authored-by: Alex Roan <[email protected]>

aderyn_core/src/detect/detector.rs

@@ -101,6 +101,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
         Box::<FucntionPointerInConstructorDetector>::default(),
         Box::<StateVariableCouldBeConstantDetector>::default(),
         Box::<StateVariableChangesWithoutEventDetector>::default(),
+        Box::<MultiplePlaceholdersDetector>::default(),
     ]
 }
 
@@ -112,6 +113,7 @@ pub fn get_all_detectors_names() -> Vec<String> {
 #[derive(Debug, PartialEq, EnumString, Display)]
 #[strum(serialize_all = "kebab-case")]
 pub(crate) enum IssueDetectorNamePool {
+    MultiplePlaceholders,
     StateVariableChangesWithoutEvents,
     MissingInheritance,
     UnusedImport,
@@ -207,6 +209,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::MultiplePlaceholders => {
+            Some(Box::<MultiplePlaceholdersDetector>::default())
+        }
         IssueDetectorNamePool::StateVariableChangesWithoutEvents => {
             Some(Box::<StateVariableChangesWithoutEventDetector>::default())
         }

aderyn_core/src/detect/low/mod.rs

@@ -18,6 +18,7 @@ pub(crate) mod inconsistent_type_names;
 pub(crate) mod large_literal_value;
 pub(crate) mod local_variable_shadowing;
 pub(crate) mod missing_inheritance;
+pub(crate) mod multiple_placeholders;
 pub(crate) mod non_reentrant_before_others;
 pub(crate) mod public_variable_read_in_external_context;
 pub(crate) mod push_0_opcode;
@@ -62,6 +63,7 @@ pub use inconsistent_type_names::InconsistentTypeNamesDetector;
 pub use large_literal_value::LargeLiteralValueDetector;
 pub use local_variable_shadowing::LocalVariableShadowingDetector;
 pub use missing_inheritance::MissingInheritanceDetector;
+pub use multiple_placeholders::MultiplePlaceholdersDetector;
 pub use non_reentrant_before_others::NonReentrantBeforeOthersDetector;
 pub use public_variable_read_in_external_context::PublicVariableReadInExternalContextDetector;
 pub use push_0_opcode::PushZeroOpcodeDetector;

aderyn_core/src/detect/low/multiple_placeholders.rs

@@ -0,0 +1,92 @@
+use std::collections::BTreeMap;
+use std::error::Error;
+
+use crate::ast::NodeID;
+
+use crate::capture;
+use crate::context::browser::ExtractPlaceholderStatements;
+use crate::detect::detector::IssueDetectorNamePool;
+use crate::{
+    context::workspace_context::WorkspaceContext,
+    detect::detector::{IssueDetector, IssueSeverity},
+};
+use eyre::Result;
+
+// HOW TO USE THIS TEMPLATE:
+// 1. Copy this file and rename it to the snake_case version of the issue you are detecting.
+// 2. Rename the TemplateDetector struct and impl to your new issue name.
+// 3. Add this file and detector struct to the mod.rs file in the same directory.
+// 4. Implement the detect function to find instances of the issue.
+
+#[derive(Default)]
+pub struct MultiplePlaceholdersDetector {
+    // 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>,
+    hints: BTreeMap<(String, usize, String), String>,
+}
+
+impl IssueDetector for MultiplePlaceholdersDetector {
+    fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
+        for modifier in context.modifier_definitions() {
+            let placeholders = ExtractPlaceholderStatements::from(modifier).extracted;
+            if placeholders.len() > 1 {
+                capture!(self, context, modifier);
+            }
+        }
+        Ok(!self.found_instances.is_empty())
+    }
+
+    fn severity(&self) -> IssueSeverity {
+        IssueSeverity::Low
+    }
+
+    fn title(&self) -> String {
+        String::from("Modifier has multiple placeholders.")
+    }
+
+    fn description(&self) -> String {
+        String::from("Design the modifier to only contain 1 placeholder statement. If it's not possible, split the logic into multiple modifiers.")
+    }
+
+    fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> {
+        self.found_instances.clone()
+    }
+
+    fn hints(&self) -> BTreeMap<(String, usize, String), String> {
+        self.hints.clone()
+    }
+
+    fn name(&self) -> String {
+        IssueDetectorNamePool::MultiplePlaceholders.to_string()
+    }
+}
+
+#[cfg(test)]
+mod multiple_placeholder_tests {
+    use serial_test::serial;
+
+    use crate::detect::{
+        detector::IssueDetector, low::multiple_placeholders::MultiplePlaceholdersDetector,
+    };
+
+    #[test]
+    #[serial]
+    fn multiple_placeholders_test() {
+        let context = crate::detect::test_utils::load_solidity_source_unit(
+            "../tests/contract-playground/src/MultiplePlaceholders.sol",
+        );
+
+        let mut detector = MultiplePlaceholdersDetector::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 low
+        assert_eq!(
+            detector.severity(),
+            crate::detect::detector::IssueSeverity::Low
+        );
+    }
+}

tests/contract-playground/src/MultiplePlaceholders.sol

@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.19;
+
+contract MultiplePlaceholders {
+    address internal owner;
+
+    constructor() {
+        owner = msg.sender;
+    }
+
+    modifier checkOwner() {
+        require(msg.sender == owner, "You are not the owner!");
+        _;
+        _;
+    }
+
+    // aderyn-ignore-next-line(empty-block)
+    function restrictedFunction1() external checkOwner {}
+
+    // aderyn-ignore-next-line(empty-block)
+    function restrictedFunction2() external checkOwner {}
+}