Fix False Positive: Sends eth without checking msg.sender

aderyn_core/src/detect/high/send_ether_no_checks.rs

@@ -5,6 +5,7 @@ use crate::ast::NodeID;
 use crate::{
     capture,
     context::{
+        browser::ExtractModifierInvocations,
         graph::{CallGraph, CallGraphDirection, CallGraphVisitor},
         workspace_context::{ASTNode, WorkspaceContext},
     },
@@ -29,7 +30,20 @@ impl IssueDetector for SendEtherNoChecksDetector {
             let callgraph = CallGraph::new(context, &[&(func.into())], CallGraphDirection::Inward)?;
             callgraph.accept(context, &mut tracker)?;
 
-            if tracker.sends_native_eth && !tracker.has_binary_checks_on_some_address {
+            // Hacky way to check if the modifier is a know msg.sender checking modifier
+            // This is because our Callgraph doesn't navigate inside contracts that are outside
+            // the scope, this includes imported contracts.
+            let has_oz_modifier =
+                ExtractModifierInvocations::from(func).extracted.iter().any(|invocation| {
+                    invocation.modifier_name.name().contains("onlyRole")
+                        || invocation.modifier_name.name() == "onlyOwner"
+                        || invocation.modifier_name.name() == "requiresAuth"
+                });
+
+            if tracker.sends_native_eth
+                && !tracker.has_binary_checks_on_some_address
+                && !has_oz_modifier
+            {
                 capture!(self, context, func);
             }
         }
@@ -58,6 +72,26 @@ impl IssueDetector for SendEtherNoChecksDetector {
     }
 }
 
+#[derive(Default)]
+pub struct AddressChecksAndCallWithValueTracker {
+    pub has_binary_checks_on_some_address: bool,
+    pub sends_native_eth: bool,
+}
+
+impl CallGraphVisitor for AddressChecksAndCallWithValueTracker {
+    fn visit_any(&mut self, node: &ASTNode) -> eyre::Result<()> {
+        if !self.has_binary_checks_on_some_address
+            && helpers::has_binary_checks_on_some_address(node)
+        {
+            self.has_binary_checks_on_some_address = true;
+        }
+        if !self.sends_native_eth && helpers::has_calls_that_sends_native_eth(node) {
+            self.sends_native_eth = true;
+        }
+        eyre::Ok(())
+    }
+}
+
 #[cfg(test)]
 mod send_ether_no_checks_detector_tests {
     use serial_test::serial;
@@ -66,6 +100,23 @@ mod send_ether_no_checks_detector_tests {
         detector::IssueDetector, high::send_ether_no_checks::SendEtherNoChecksDetector,
     };
 
+    #[test]
+    #[serial]
+    fn test_send_ether_no_checks_lib_import() {
+        let context = crate::detect::test_utils::load_solidity_source_unit(
+            "../tests/contract-playground/src/SendEtherNoChecksLibImport.sol",
+        );
+
+        let mut detector = SendEtherNoChecksDetector::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(), 0);
+        // assert the severity is high
+        assert_eq!(detector.severity(), crate::detect::detector::IssueSeverity::High);
+    }
+
     #[test]
     #[serial]
     fn test_send_ether_no_checks() {
@@ -83,23 +134,3 @@ mod send_ether_no_checks_detector_tests {
         assert_eq!(detector.severity(), crate::detect::detector::IssueSeverity::High);
     }
 }
-
-#[derive(Default)]
-pub struct AddressChecksAndCallWithValueTracker {
-    pub has_binary_checks_on_some_address: bool,
-    pub sends_native_eth: bool,
-}
-
-impl CallGraphVisitor for AddressChecksAndCallWithValueTracker {
-    fn visit_any(&mut self, node: &ASTNode) -> eyre::Result<()> {
-        if !self.has_binary_checks_on_some_address
-            && helpers::has_binary_checks_on_some_address(node)
-        {
-            self.has_binary_checks_on_some_address = true;
-        }
-        if !self.sends_native_eth && helpers::has_calls_that_sends_native_eth(node) {
-            self.sends_native_eth = true;
-        }
-        eyre::Ok(())
-    }
-}

tests/contract-playground/src/SendEtherNoChecksLibImport.sol

@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.19;
+
+import {Ownable} from "../lib/openzeppelin-contracts/contracts/access/Ownable.sol";
+import {AccessControl} from "../lib/openzeppelin-contracts/contracts/access/AccessControl.sol";
+
+contract SendEtherCheckOZOnlyOWner is Ownable, AccessControl {
+
+    function callAndSendNativeEth(address x) internal {
+        (bool success,) = x.call{value: 10}("calldata");
+        if (!success) {
+            revert();
+        }
+    }
+
+    function send(address x) external onlyOwner {
+        callAndSendNativeEth(x);
+    }
+
+    function sendWithRole(address x) external onlyRole(DEFAULT_ADMIN_ROLE) {
+        callAndSendNativeEth(x);
+    }
+}