Detector: Out of Order Retryable (#660)

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

aderyn_core/src/detect/detector.rs

@@ -83,6 +83,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
         Box::<ContractLocksEtherDetector>::default(),
         Box::<IncorrectERC20InterfaceDetector>::default(),
         Box::<ReturnBombDetector>::default(),
+        Box::<OutOfOrderRetryableDetector>::default(),
         Box::<FunctionInitializingStateDetector>::default(),
     ]
 }
@@ -163,6 +164,7 @@ pub(crate) enum IssueDetectorNamePool {
     ContractLocksEther,
     IncorrectERC20Interface,
     ReturnBomb,
+    OutOfOrderRetryable,
     // 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,
@@ -172,6 +174,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::OutOfOrderRetryable => {
+            Some(Box::<OutOfOrderRetryableDetector>::default())
+        }
         IssueDetectorNamePool::FunctionInitializingState => {
             Some(Box::<FunctionInitializingStateDetector>::default())
         }

aderyn_core/src/detect/high/mod.rs

@@ -17,6 +17,7 @@ pub(crate) mod misused_boolean;
 pub(crate) mod msg_value_in_loops;
 pub(crate) mod multiple_constructors;
 pub(crate) mod nested_struct_in_mapping;
+pub(crate) mod out_of_order_retryable;
 pub(crate) mod pre_declared_variable_usage;
 pub(crate) mod reused_contract_name;
 pub(crate) mod rtlo;
@@ -55,6 +56,7 @@ pub use misused_boolean::MisusedBooleanDetector;
 pub use msg_value_in_loops::MsgValueUsedInLoopDetector;
 pub use multiple_constructors::MultipleConstructorsDetector;
 pub use nested_struct_in_mapping::NestedStructInMappingDetector;
+pub use out_of_order_retryable::OutOfOrderRetryableDetector;
 pub use pre_declared_variable_usage::PreDeclaredLocalVariableUsageDetector;
 pub use reused_contract_name::ReusedContractNameDetector;
 pub use rtlo::RTLODetector;

aderyn_core/src/detect/high/out_of_order_retryable.rs

@@ -0,0 +1,126 @@
+use std::collections::BTreeMap;
+use std::error::Error;
+
+use crate::ast::{Expression, MemberAccess, NodeID};
+
+use crate::capture;
+use crate::context::browser::ExtractFunctionCalls;
+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 OutOfOrderRetryableDetector {
+    // 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 OutOfOrderRetryableDetector {
+    fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
+        for func in helpers::get_implemented_external_and_public_functions(context) {
+            let mut tracker = OutOfOrderRetryableTracker {
+                number_of_retry_calls: 0,
+            };
+            let investigator = StandardInvestigator::new(
+                context,
+                &[&(func.into())],
+                StandardInvestigationStyle::Downstream,
+            )?;
+            investigator.investigate(context, &mut tracker)?;
+            if tracker.number_of_retry_calls >= 2 {
+                capture!(self, context, func);
+            }
+        }
+
+        Ok(!self.found_instances.is_empty())
+    }
+
+    fn severity(&self) -> IssueSeverity {
+        IssueSeverity::High
+    }
+
+    fn title(&self) -> String {
+        String::from("Out of order retryable transactions.")
+    }
+
+    fn description(&self) -> String {
+        String::from("Do not rely on the order or successful execution of retryable tickets. Functions like \
+            createRetryableTicket, outboundTransferCustomRefund, unsafeCreateRetryableTicket are free to be re-tried in any
+            order if they fail in the first go. Since this operation happens off chain, the sequencer is in control of the
+            order of these transactions. Therefore, restrict the use to at most 1 ticket call per function.")
+    }
+
+    fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> {
+        self.found_instances.clone()
+    }
+
+    fn name(&self) -> String {
+        format!("{}", IssueDetectorNamePool::OutOfOrderRetryable)
+    }
+}
+
+struct OutOfOrderRetryableTracker {
+    number_of_retry_calls: usize,
+}
+
+const SEQUENCER_FUNCTIONS: [&str; 3] = [
+    "createRetryableTicket",
+    "outboundTransferCustomRefund",
+    "unsafeCreateRetryableTicket",
+];
+
+impl StandardInvestigatorVisitor for OutOfOrderRetryableTracker {
+    fn visit_any(&mut self, node: &crate::ast::ASTNode) -> eyre::Result<()> {
+        if self.number_of_retry_calls >= 2 {
+            return Ok(());
+        }
+        let function_calls = ExtractFunctionCalls::from(node).extracted;
+        for func_call in function_calls {
+            if let Expression::MemberAccess(MemberAccess { member_name, .. }) =
+                func_call.expression.as_ref()
+            {
+                if SEQUENCER_FUNCTIONS.iter().any(|f| f == member_name) {
+                    self.number_of_retry_calls += 1;
+                }
+            }
+        }
+        Ok(())
+    }
+}
+
+#[cfg(test)]
+mod out_of_order_retryable_tests {
+    use serial_test::serial;
+
+    use crate::detect::{
+        detector::IssueDetector, high::out_of_order_retryable::OutOfOrderRetryableDetector,
+    };
+
+    #[test]
+    #[serial]
+    fn test_out_of_order_retryable() {
+        let context = crate::detect::test_utils::load_solidity_source_unit(
+            "../tests/contract-playground/src/OutOfOrderRetryable.sol",
+        );
+
+        let mut detector = OutOfOrderRetryableDetector::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
+        );
+    }
+}

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

@@ -198,6 +198,7 @@
     "tx-origin-used-for-auth",
     "msg-value-in-loop",
     "contract-locks-ether",
-    "incorrect-erc20-interface"
+    "incorrect-erc20-interface",
+    "out-of-order-retryable"
   ]
 }