Detector: Return bomb

aderyn_core/src/detect/detector.rs

@@ -80,6 +80,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
         Box::<TxOriginUsedForAuthDetector>::default(),
         Box::<MsgValueUsedInLoopDetector>::default(),
         Box::<ContractLocksEtherDetector>::default(),
+        Box::<ReturnBombDetector>::default(),
     ]
 }
 
@@ -156,6 +157,7 @@ pub(crate) enum IssueDetectorNamePool {
     TxOriginUsedForAuth,
     MsgValueInLoop,
     ContractLocksEther,
+    ReturnBomb,
     // 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,
@@ -165,6 +167,7 @@ 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::ReturnBomb => Some(Box::<ReturnBombDetector>::default()),
         IssueDetectorNamePool::UnusedStateVariable => {
             Some(Box::<UnusedStateVariablesDetector>::default())
         }

aderyn_core/src/detect/helpers.rs

@@ -187,6 +187,39 @@ pub fn has_delegate_calls_on_non_state_variables(
     })
 }
 
+/// Detects for the pattern
+/// x.call("...") where x is not a state variable
+/// That means, it can be
+/// a) An Identifier that references a variable declaration which is not `state_variable`
+/// b) A literal adresss
+pub fn get_low_level_calls_on_non_state_variable_addresses(
+    ast_node: &ASTNode,
+    context: &WorkspaceContext,
+) -> Vec<MemberAccess> {
+    ExtractMemberAccesses::from(ast_node)
+        .extracted
+        .into_iter()
+        .filter_map(|member| {
+            if member.member_name != "call" {
+                return None;
+            }
+            if let Expression::Identifier(identifier) = member.expression.as_ref() {
+                if let Some(referenced_id) = identifier.referenced_declaration {
+                    if let Some(ASTNode::VariableDeclaration(v)) = context.nodes.get(&referenced_id)
+                    {
+                        if !v.state_variable {
+                            return Some(member);
+                        }
+                    }
+                }
+            } else if let Expression::Literal(_) = member.expression.as_ref() {
+                return Some(member);
+            }
+            None
+        })
+        .collect::<_>()
+}
+
 pub fn has_binary_checks_on_some_address(ast_node: &ASTNode) -> bool {
     let binary_operations = ExtractBinaryOperations::from(ast_node).extracted;
     binary_operations.into_iter().any(|op| {

aderyn_core/src/detect/low/mod.rs

@@ -14,6 +14,7 @@ pub(crate) mod public_variable_read_in_external_context;
 pub(crate) mod push_0_opcode;
 pub(crate) mod redundant_statements;
 pub(crate) mod require_with_string;
+pub(crate) mod return_bomb;
 pub(crate) mod reverts_and_requries_in_loops;
 pub(crate) mod solmate_safe_transfer_lib;
 pub(crate) mod unindexed_events;
@@ -43,6 +44,7 @@ pub use public_variable_read_in_external_context::PublicVariableReadInExternalCo
 pub use push_0_opcode::PushZeroOpcodeDetector;
 pub use redundant_statements::RedundantStatementsDetector;
 pub use require_with_string::RequireWithStringDetector;
+pub use return_bomb::ReturnBombDetector;
 pub use reverts_and_requries_in_loops::RevertsAndRequiresInLoopsDetector;
 pub use solmate_safe_transfer_lib::SolmateSafeTransferLibDetector;
 pub use unindexed_events::UnindexedEventsDetector;

aderyn_core/src/detect/low/return_bomb.rs

@@ -0,0 +1,169 @@
+use std::collections::BTreeMap;
+use std::error::Error;
+
+use crate::ast::{ASTNode, MemberAccess, NodeID};
+
+use crate::ast::NodeType;
+use crate::capture;
+use crate::context::browser::GetClosestAncestorOfTypeX;
+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 ReturnBombDetector {
+    // 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 ReturnBombDetector {
+    fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
+        // PLAN
+        // Look for calls on addresses that are unprotected. (non state variable address that has not undergone any binary checks)
+
+        // Capture the ones where no gas limit  is explicitly set *and* there is a `returndatacopy` operation
+        // Basially you are checking for the 2nd element in the tuple - (bool success, bytes memory ret) which invokes the
+        // above operation.
+
+        for func in helpers::get_implemented_external_and_public_functions(context) {
+            let mut tracker = CallNoAddressChecksTracker {
+                has_address_checks: false,
+                calls_on_non_state_variable_addresses: vec![], // collection of all `address.call` Member Accesses where address is not a state variable
+                context,
+            };
+            let investigator = StandardInvestigator::new(
+                context,
+                &[&(func.into())],
+                StandardInvestigationStyle::Downstream,
+            )?;
+            investigator.investigate(context, &mut tracker)?;
+
+            if !tracker.has_address_checks {
+                // Now we assume that in this region all addresses are unprotected (because they are not involved in any binary ops/checks)
+                for member_access in tracker.calls_on_non_state_variable_addresses {
+                    // Now we need to see if address.call{gas: xxx}() has been called with options and if so,
+                    // scan to see if the gaslimit is set. If it is, then it is not a vulnerability because
+                    // OOG is likely not possible when there is defined gas limit
+                    // Therefore, continue the for loop and investigate other instances
+
+                    if let Some(ASTNode::FunctionCallOptions(function_call_ops)) = member_access
+                        .closest_ancestor_of_type(context, NodeType::FunctionCallOptions)
+                    {
+                        if function_call_ops.names.contains(&String::from("gas")) {
+                            continue;
+                        }
+                    }
+
+                    // Here, we know that there is no gas limit set for the call. So we need to only check
+                    // for the cases where `returndatacopy` happens and then capture it.
+
+                    if let Some(ASTNode::FunctionCall(function_call)) =
+                        member_access.closest_ancestor_of_type(context, NodeType::FunctionCall)
+                    {
+                        // In this case there are no options like gas, etc, passed to the `address.call()`
+                        // So we need to check if `returndatacopy` is triggered. If yes, then it is a problem
+
+                        if let Some(ASTNode::Assignment(assignment)) =
+                            function_call.closest_ancestor_of_type(context, NodeType::Assignment)
+                        {
+                            // The following check will ensure that the last paramter which is `bytes memory retData`
+                            // is not unpacked. (there is nothing after comma)
+                            if !assignment.left_hand_side.type_descriptions().is_some_and(
+                                |type_desc| {
+                                    type_desc
+                                        .type_string
+                                        .as_ref()
+                                        .is_some_and(|type_string| type_string.ends_with(",)"))
+                                },
+                            ) {
+                                capture!(self, context, assignment);
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
+        Ok(!self.found_instances.is_empty())
+    }
+
+    fn severity(&self) -> IssueSeverity {
+        IssueSeverity::Low
+    }
+
+    fn title(&self) -> String {
+        String::from("Return Bomb")
+    }
+
+    fn description(&self) -> String {
+        String::from("A low level callee may consume all callers gas unexpectedly. Avoid unlimited implicit decoding of returndata on \
+            calls to unchecked addresses. You can limit the gas by passing a gas limit as an option to the call. For example, \
+            `unknownAdress.call{gas: gasLimitHere}(\"calldata\")` That would act as a safety net from OOG errors.
+        ")
+    }
+
+    fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> {
+        self.found_instances.clone()
+    }
+
+    fn name(&self) -> String {
+        format!("{}", IssueDetectorNamePool::ReturnBomb)
+    }
+}
+
+struct CallNoAddressChecksTracker<'a> {
+    has_address_checks: bool,
+    calls_on_non_state_variable_addresses: Vec<MemberAccess>,
+    context: &'a WorkspaceContext,
+}
+
+impl<'a> StandardInvestigatorVisitor for CallNoAddressChecksTracker<'a> {
+    fn visit_any(&mut self, node: &crate::context::workspace_context::ASTNode) -> eyre::Result<()> {
+        if !self.has_address_checks && helpers::has_binary_checks_on_some_address(node) {
+            self.has_address_checks = true;
+        }
+        self.calls_on_non_state_variable_addresses.extend(
+            helpers::get_low_level_calls_on_non_state_variable_addresses(node, self.context),
+        );
+        self.calls_on_non_state_variable_addresses.dedup();
+        eyre::Ok(())
+    }
+}
+
+#[cfg(test)]
+mod return_bomb_detector_tests {
+    use serial_test::serial;
+
+    use crate::detect::{detector::IssueDetector, low::return_bomb::ReturnBombDetector};
+
+    #[test]
+    #[serial]
+    fn test_return_bomb_detector() {
+        let context = crate::detect::test_utils::load_solidity_source_unit(
+            "../tests/contract-playground/src/ReturnBomb.sol",
+        );
+
+        let mut detector = ReturnBombDetector::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
+
+        println!("{:#?}", detector.instances());
+
+        assert_eq!(detector.instances().len(), 1);
+        // assert the severity is low
+        assert_eq!(
+            detector.severity(),
+            crate::detect::detector::IssueSeverity::Low
+        );
+    }
+}

reports/report.json

@@ -1,7 +1,7 @@
 {
   "files_summary": {
-    "total_source_units": 80,
-    "total_sloc": 2290
+    "total_source_units": 81,
+    "total_sloc": 2334
   },
   "files_details": {
     "files_details": [
@@ -161,6 +161,10 @@
         "file_path": "src/RedundantStatements.sol",
         "n_sloc": 14
       },
+      {
+        "file_path": "src/ReturnBomb.sol",
+        "n_sloc": 44
+      },
       {
         "file_path": "src/RevertsAndRequriesInLoops.sol",
         "n_sloc": 27
@@ -329,7 +333,7 @@
   },
   "issue_count": {
     "high": 36,
-    "low": 28
+    "low": 29
   },
   "high_issues": {
     "issues": [
@@ -1331,6 +1335,12 @@
             "src": "130:26",
             "src_char": "130:26"
           },
+          {
+            "contract_path": "src/ReturnBomb.sol",
+            "line_no": 61,
+            "src": "1623:7",
+            "src_char": "1623:7"
+          },
           {
             "contract_path": "src/StateShadowing.sol",
             "line_no": 5,
@@ -1601,6 +1611,12 @@
             "src": "6342:11",
             "src_char": "6342:11"
           },
+          {
+            "contract_path": "src/ReturnBomb.sol",
+            "line_no": 32,
+            "src": "839:4",
+            "src_char": "839:4"
+          },
           {
             "contract_path": "src/SendEtherNoChecks.sol",
             "line_no": 53,
@@ -2503,6 +2519,30 @@
             "src": "129:9",
             "src_char": "129:9"
           },
+          {
+            "contract_path": "src/ReturnBomb.sol",
+            "line_no": 16,
+            "src": "358:4",
+            "src_char": "358:4"
+          },
+          {
+            "contract_path": "src/ReturnBomb.sol",
+            "line_no": 32,
+            "src": "839:4",
+            "src_char": "839:4"
+          },
+          {
+            "contract_path": "src/ReturnBomb.sol",
+            "line_no": 47,
+            "src": "1273:4",
+            "src_char": "1273:4"
+          },
+          {
+            "contract_path": "src/ReturnBomb.sol",
+            "line_no": 63,
+            "src": "1646:4",
+            "src_char": "1646:4"
+          },
           {
             "contract_path": "src/StateVariables.sol",
             "line_no": 47,
@@ -3235,6 +3275,12 @@
             "src": "32:23",
             "src_char": "32:23"
           },
+          {
+            "contract_path": "src/ReturnBomb.sol",
+            "line_no": 2,
+            "src": "32:23",
+            "src_char": "32:23"
+          },
           {
             "contract_path": "src/StateVariables.sol",
             "line_no": 2,
@@ -4360,6 +4406,19 @@
             "src_char": "614:16"
           }
         ]
+      },
+      {
+        "title": "Return Bomb",
+        "description": "A low level callee may consume all callers gas unexpectedly. Avoid unlimited implicit decoding of returndata on calls to unchecked addresses. You can limit the gas by passing a gas limit as an option to the call. For example, `unknownAdress.call{gas: gasLimitHere}(\"calldata\")` That would act as a safety net from OOG errors.\n        ",
+        "detector_name": "return-bomb",
+        "instances": [
+          {
+            "contract_path": "src/ReturnBomb.sol",
+            "line_no": 22,
+            "src": "591:115",
+            "src_char": "591:115"
+          }
+        ]
       }
     ]
   },
@@ -4427,6 +4486,7 @@
     "boolean-equality",
     "tx-origin-used-for-auth",
     "msg-value-in-loop",
-    "contract-locks-ether"
+    "contract-locks-ether",
+    "return-bomb"
   ]
 }