Detector: constant functions contains assembly

aderyn_core/src/detect/detector.rs

@@ -73,6 +73,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
         Box::<WeakRandomnessDetector>::default(),
         Box::<PreDeclaredLocalVariableUsageDetector>::default(),
         Box::<DeletionNestedMappingDetector>::default(),
+        Box::<ConstantFunctionContainsAssemblyDetector>::default(),
     ]
 }
 
@@ -142,6 +143,7 @@ pub(crate) enum IssueDetectorNamePool {
     WeakRandomness,
     PreDeclaredLocalVariableUsage,
     DeleteNestedMapping,
+    ConstantFunctionsAssembly,
     // 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,
@@ -297,6 +299,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option<Box<dyn Iss
         IssueDetectorNamePool::DeleteNestedMapping => {
             Some(Box::<DeletionNestedMappingDetector>::default())
         }
+        IssueDetectorNamePool::ConstantFunctionsAssembly => {
+            Some(Box::<ConstantFunctionContainsAssemblyDetector>::default())
+        }
         IssueDetectorNamePool::Undecided => None,
     }
 }

aderyn_core/src/detect/low/constant_funcs_assembly.rs

@@ -0,0 +1,131 @@
+use std::collections::BTreeMap;
+use std::error::Error;
+
+use crate::ast::{ASTNode, NodeID, StateMutability};
+
+use crate::capture;
+use crate::context::browser::ExtractInlineAssemblys;
+use crate::context::investigator::{
+    StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor,
+};
+use crate::detect::detector::IssueDetectorNamePool;
+use crate::detect::helpers;
+use crate::{
+    context::workspace_context::WorkspaceContext,
+    detect::detector::{IssueDetector, IssueSeverity},
+};
+use eyre::Result;
+
+#[derive(Default)]
+pub struct ConstantFunctionContainsAssemblyDetector {
+    // 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 ConstantFunctionContainsAssemblyDetector {
+    fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
+        for function in helpers::get_implemented_external_and_public_functions(context) {
+            if function.state_mutability() == &StateMutability::View
+                || function.state_mutability() == &StateMutability::Pure
+            {
+                let mut tracker = AssemblyTracker {
+                    has_assembly: false,
+                };
+                let investigator = StandardInvestigator::new(
+                    context,
+                    &[&(function.into())],
+                    StandardInvestigationStyle::Downstream,
+                )?;
+                investigator.investigate(context, &mut tracker)?;
+
+                if tracker.has_assembly {
+                    capture!(self, context, function);
+                }
+            }
+        }
+
+        Ok(!self.found_instances.is_empty())
+    }
+
+    fn severity(&self) -> IssueSeverity {
+        IssueSeverity::Low
+    }
+
+    fn title(&self) -> String {
+        String::from("Functions declared `pure` / `view` but contains assembly")
+    }
+
+    fn description(&self) -> String {
+        String::from("If the assembly code contains bugs or unintended side effects, it can lead to incorrect results \
+            or vulnerabilities, which are hard to debug and resolve, especially when the function is meant to be simple \
+            and predictable.")
+    }
+
+    fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> {
+        self.found_instances.clone()
+    }
+
+    fn name(&self) -> String {
+        format!("{}", IssueDetectorNamePool::ConstantFunctionsAssembly)
+    }
+}
+
+struct AssemblyTracker {
+    has_assembly: bool,
+}
+
+impl StandardInvestigatorVisitor for AssemblyTracker {
+    fn visit_any(&mut self, node: &crate::ast::ASTNode) -> eyre::Result<()> {
+        // If we are already satisifed, do not bother checking
+        if self.has_assembly {
+            return Ok(());
+        }
+
+        if let ASTNode::FunctionDefinition(function) = node {
+            // Ignore checking functions that start with `_`
+            // Example - templegold contains math functions like `_rpow()`, etc that are used by view functions
+            // That should be okay .. I guess? (idk ... it's open for dicussion)
+            if function.name.starts_with("_") {
+                return Ok(());
+            }
+        }
+
+        // Check if this node has assembly code
+        let assemblies = ExtractInlineAssemblys::from(node).extracted;
+        if !assemblies.is_empty() {
+            self.has_assembly = true;
+        }
+        Ok(())
+    }
+}
+
+#[cfg(test)]
+mod constant_functions_assembly_detector {
+    use serial_test::serial;
+
+    use crate::detect::{
+        detector::IssueDetector,
+        low::constant_funcs_assembly::ConstantFunctionContainsAssemblyDetector,
+    };
+
+    #[test]
+    #[serial]
+    fn test_constant_functions_assembly() {
+        let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs(
+            "../tests/contract-playground/src/ConstantFuncsAssembly.sol",
+        );
+
+        let mut detector = ConstantFunctionContainsAssemblyDetector::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(), 3);
+        // assert the severity is low
+        assert_eq!(
+            detector.severity(),
+            crate::detect::detector::IssueSeverity::Low
+        );
+    }
+}

aderyn_core/src/detect/low/mod.rs

@@ -1,4 +1,5 @@
 pub(crate) mod centralization_risk;
+pub(crate) mod constant_funcs_assembly;
 pub(crate) mod constants_instead_of_literals;
 pub(crate) mod contracts_with_todos;
 pub(crate) mod deprecated_oz_functions;
@@ -25,6 +26,7 @@ pub(crate) mod useless_public_function;
 pub(crate) mod zero_address_check;
 
 pub use centralization_risk::CentralizationRiskDetector;
+pub use constant_funcs_assembly::ConstantFunctionContainsAssemblyDetector;
 pub use constants_instead_of_literals::ConstantsInsteadOfLiteralsDetector;
 pub use contracts_with_todos::ContractsWithTodosDetector;
 pub use deprecated_oz_functions::DeprecatedOZFunctionsDetector;

cli/reportgen.sh

@@ -2,10 +2,10 @@
 
 #### MARKDOWN REPORTS ######
 
-# Basic report.md 
+# Basic report.md
 cargo run --  -i src/ -x lib/ ./tests/contract-playground -o ./reports/report.md --skip-update-check &
 
-# Adhoc sol files report.md 
+# Adhoc sol files report.md
 cargo run --  ./tests/adhoc-sol-files -o ./reports/adhoc-sol-files-report.md --skip-update-check &
 
 # Aderyn.toml with nested root
@@ -41,4 +41,4 @@ cargo run --  ./tests/adhoc-sol-files -o ./reports/adhoc-sol-files-highs-only-re
 # Basic report.sarif
 cargo run -- ./tests/contract-playground -o ./reports/report.sarif --skip-update-check &
 
-wait
+wait

reports/report.json

@@ -1,7 +1,7 @@
 {
   "files_summary": {
-    "total_source_units": 73,
-    "total_sloc": 1996
+    "total_source_units": 74,
+    "total_sloc": 2022
   },
   "files_details": {
     "files_details": [
@@ -33,6 +33,10 @@
         "file_path": "src/CompilerBugStorageSignedIntegerArray.sol",
         "n_sloc": 13
       },
+      {
+        "file_path": "src/ConstantFuncsAssembly.sol",
+        "n_sloc": 26
+      },
       {
         "file_path": "src/ConstantsLiterals.sol",
         "n_sloc": 28
@@ -301,7 +305,7 @@
   },
   "issue_count": {
     "high": 32,
-    "low": 25
+    "low": 26
   },
   "high_issues": {
     "issues": [
@@ -1267,6 +1271,12 @@
             "src": "97:1",
             "src_char": "97:1"
           },
+          {
+            "contract_path": "src/ConstantFuncsAssembly.sol",
+            "line_no": 6,
+            "src": "110:20",
+            "src_char": "110:20"
+          },
           {
             "contract_path": "src/DelegateCallWithoutAddressCheck.sol",
             "line_no": 9,
@@ -2001,6 +2011,12 @@
             "src": "32:23",
             "src_char": "32:23"
           },
+          {
+            "contract_path": "src/ConstantFuncsAssembly.sol",
+            "line_no": 2,
+            "src": "32:23",
+            "src_char": "32:23"
+          },
           {
             "contract_path": "src/ContractWithTodo.sol",
             "line_no": 2,
@@ -3321,6 +3337,12 @@
             "src": "1206:18",
             "src_char": "1206:18"
           },
+          {
+            "contract_path": "src/ConstantFuncsAssembly.sol",
+            "line_no": 26,
+            "src": "651:232",
+            "src_char": "651:232"
+          },
           {
             "contract_path": "src/InternalFunctions.sol",
             "line_no": 28,
@@ -3665,6 +3687,49 @@
             "src_char": "1175:14"
           }
         ]
+      },
+      {
+        "title": "Functions declared `pure` / `view` but contains assembly",
+        "description": "If the assembly code contains bugs or unintended side effects, it can lead to incorrect results or vulnerabilities, which are hard to debug and resolve, especially when the function is meant to be simple and predictable.",
+        "detector_name": "constant-functions-assembly",
+        "instances": [
+          {
+            "contract_path": "src/AssemblyExample.sol",
+            "line_no": 6,
+            "src": "113:1",
+            "src_char": "113:1"
+          },
+          {
+            "contract_path": "src/ConstantFuncsAssembly.sol",
+            "line_no": 9,
+            "src": "182:175",
+            "src_char": "182:175"
+          },
+          {
+            "contract_path": "src/ConstantFuncsAssembly.sol",
+            "line_no": 17,
+            "src": "408:237",
+            "src_char": "408:237"
+          },
+          {
+            "contract_path": "src/ConstantFuncsAssembly.sol",
+            "line_no": 36,
+            "src": "934:98",
+            "src_char": "934:98"
+          },
+          {
+            "contract_path": "src/TestERC20.sol",
+            "line_no": 17,
+            "src": "498:10",
+            "src_char": "498:10"
+          },
+          {
+            "contract_path": "src/YulReturn.sol",
+            "line_no": 6,
+            "src": "92:12",
+            "src_char": "92:12"
+          }
+        ]
       }
     ]
   },
@@ -3725,6 +3790,7 @@
     "public-variable-read-in-external-context",
     "weak-randomness",
     "pre-declared-local-variable-usage",
-    "delete-nested-mapping"
+    "delete-nested-mapping",
+    "constant-functions-assembly"
   ]
 }