Detector: Contract that locks ether (#630)

Co-authored-by: Alex Roan <alex@cyfrin.io>

aderyn_core/src/detect/detector.rs

@@ -74,6 +74,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
         Box::<WeakRandomnessDetector>::default(),
         Box::<PreDeclaredLocalVariableUsageDetector>::default(),
         Box::<DeletionNestedMappingDetector>::default(),
+        Box::<ContractLocksEtherDetector>::default(),
     ]
 }
 
@@ -144,6 +145,7 @@ pub(crate) enum IssueDetectorNamePool {
     WeakRandomness,
     PreDeclaredLocalVariableUsage,
     DeleteNestedMapping,
+    ContractLocksEther,
     // 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,
@@ -302,6 +304,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option<Box<dyn Iss
         IssueDetectorNamePool::DeleteNestedMapping => {
             Some(Box::<DeletionNestedMappingDetector>::default())
         }
+        IssueDetectorNamePool::ContractLocksEther => {
+            Some(Box::<ContractLocksEtherDetector>::default())
+        }
         IssueDetectorNamePool::Undecided => None,
     }
 }

aderyn_core/src/detect/helpers.rs

@@ -116,6 +116,9 @@ pub fn has_calls_that_sends_native_eth(ast_node: &ASTNode) -> bool {
             if let Expression::Literal(literal) = c {
                 return literal.value.is_some();
             }
+            if let Expression::Identifier(_) = c {
+                return true;
+            }
             false
         });
         if !call_carries_value {
@@ -189,9 +192,9 @@ pub fn has_binary_checks_on_some_address(ast_node: &ASTNode) -> bool {
     binary_operations.into_iter().any(|op| {
         [op.left_expression, op.right_expression].iter().any(|op| {
             op.as_ref().type_descriptions().is_some_and(|desc| {
-                desc.type_string
-                    .as_ref()
-                    .is_some_and(|type_string| type_string == "address")
+                desc.type_string.as_ref().is_some_and(|type_string| {
+                    type_string == "address" || type_string == "address payable"
+                })
             })
         })
     })

aderyn_core/src/detect/high/contract_locks_ether.rs

@@ -0,0 +1,182 @@
+use std::collections::BTreeMap;
+
+use std::convert::identity;
+use std::error::Error;
+
+use crate::ast::NodeID;
+
+use crate::capture;
+use crate::detect::detector::IssueDetectorNamePool;
+use crate::{
+    context::workspace_context::WorkspaceContext,
+    detect::detector::{IssueDetector, IssueSeverity},
+};
+
+use eyre::Result;
+
+#[derive(Default)]
+pub struct ContractLocksEtherDetector {
+    // 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 ContractLocksEtherDetector {
+    fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
+        for contract in context.contract_definitions() {
+            // If a contract can accept eth, but doesn't allow for withdrawal capture it!
+            if contract.can_accept_eth(context).is_some_and(identity)
+                && !contract
+                    .allows_withdrawal_of_eth(context)
+                    .is_some_and(identity)
+            {
+                capture!(self, context, contract);
+            }
+        }
+        Ok(!self.found_instances.is_empty())
+    }
+
+    fn severity(&self) -> IssueSeverity {
+        IssueSeverity::High
+    }
+
+    fn title(&self) -> String {
+        String::from("Contract locks Ether without a withdraw function.")
+    }
+
+    fn description(&self) -> String {
+        String::from(
+            "It appears that the contract includes a payable function to accept Ether but lacks a corresponding function to withdraw it, \
+            which leads to the Ether being locked in the contract. To resolve this issue, please implement a public or external function \
+            that allows for the withdrawal of Ether from the contract."
+        )
+    }
+
+    fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> {
+        self.found_instances.clone()
+    }
+
+    fn name(&self) -> String {
+        IssueDetectorNamePool::ContractLocksEther.to_string()
+    }
+}
+
+/// Handles tasks related to contract level analysis for eth
+mod contract_eth_helper {
+    use crate::{
+        ast::{ASTNode, ContractDefinition, StateMutability, Visibility},
+        context::{
+            browser::ExtractFunctionDefinitions, investigator::*,
+            workspace_context::WorkspaceContext,
+        },
+        detect::helpers,
+    };
+
+    impl ContractDefinition {
+        pub fn can_accept_eth(&self, context: &WorkspaceContext) -> Option<bool> {
+            let contracts = self.linearized_base_contracts.as_ref()?;
+            for contract_id in contracts {
+                let funcs =
+                    ExtractFunctionDefinitions::from(context.nodes.get(contract_id)?).extracted;
+                let num_payable_funcs = funcs
+                    .into_iter()
+                    .filter(|f| f.implemented && *f.state_mutability() == StateMutability::Payable)
+                    .count();
+                if num_payable_funcs > 0 {
+                    return Some(true);
+                }
+            }
+            Some(false)
+        }
+
+        pub fn allows_withdrawal_of_eth(&self, context: &WorkspaceContext) -> Option<bool> {
+            /*
+                For all the contracts in the hirearchy try and see if there is exists a public/external function that
+                can be called which takes the execution flow in a path where there is possibility to send back eth away from
+                the contract using the low level `call{value: XXX}` or `transfer` or `send`.
+            */
+            let contracts = self.linearized_base_contracts.as_ref()?;
+            for contract_id in contracts {
+                if let ASTNode::ContractDefinition(contract) = context.nodes.get(contract_id)? {
+                    let funcs = contract
+                        .function_definitions()
+                        .into_iter()
+                        .filter(|f| {
+                            f.implemented
+                                && (f.visibility == Visibility::Public
+                                    || f.visibility == Visibility::External)
+                        })
+                        .map(|f| f.into())
+                        .collect::<Vec<ASTNode>>();
+
+                    let mut tracker = EthWithdrawalAllowerTracker::default();
+
+                    let investigator = StandardInvestigator::new(
+                        context,
+                        funcs.iter().collect::<Vec<_>>().as_slice(),
+                        StandardInvestigationStyle::Downstream,
+                    )
+                    .ok()?;
+
+                    investigator.investigate(context, &mut tracker).ok()?;
+
+                    if tracker.has_calls_that_sends_native_eth {
+                        return Some(true);
+                    }
+                }
+            }
+            // At this point we have successfully gone through all the contracts in the inheritance heirarchy
+            // but tracker has determined that none of them have have calls that sends native eth Even if they are by
+            // some chance, they are not reachable from external & public functions
+            Some(false)
+        }
+    }
+
+    #[derive(Default)]
+    struct EthWithdrawalAllowerTracker {
+        has_calls_that_sends_native_eth: bool,
+    }
+
+    impl StandardInvestigatorVisitor for EthWithdrawalAllowerTracker {
+        fn visit_any(&mut self, ast_node: &ASTNode) -> eyre::Result<()> {
+            if !self.has_calls_that_sends_native_eth
+                && helpers::has_calls_that_sends_native_eth(ast_node)
+            {
+                self.has_calls_that_sends_native_eth = true;
+            }
+            Ok(())
+        }
+    }
+}
+
+#[cfg(test)]
+mod contract_locks_ether_detector_tests {
+    use serial_test::serial;
+
+    use crate::detect::{
+        detector::IssueDetector, high::contract_locks_ether::ContractLocksEtherDetector,
+    };
+
+    #[test]
+    #[serial]
+    fn test_contract_locks_ether() {
+        let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs(
+            "../tests/contract-playground/src/ContractLocksEther.sol",
+        );
+
+        let mut detector = ContractLocksEtherDetector::default();
+        let found = detector.detect(&context).unwrap();
+
+        println!("{:#?}", detector.instances());
+
+        // 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
+        );
+    }
+}

aderyn_core/src/detect/high/mod.rs

@@ -1,6 +1,7 @@
 pub(crate) mod arbitrary_transfer_from;
 pub(crate) mod avoid_abi_encode_packed;
 pub(crate) mod block_timestamp_deadline;
+pub(crate) mod contract_locks_ether;
 pub(crate) mod dangerous_strict_equality_balance;
 pub(crate) mod dangerous_unary_operator;
 pub(crate) mod delegate_call_in_loop;
@@ -35,6 +36,7 @@ pub(crate) mod yul_return;
 pub use arbitrary_transfer_from::ArbitraryTransferFromDetector;
 pub use avoid_abi_encode_packed::AvoidAbiEncodePackedDetector;
 pub use block_timestamp_deadline::BlockTimestampDeadlineDetector;
+pub use contract_locks_ether::ContractLocksEtherDetector;
 pub use dangerous_strict_equality_balance::DangerousStrictEqualityOnBalanceDetector;
 pub use dangerous_unary_operator::DangerousUnaryOperatorDetector;
 pub use delegate_call_in_loop::DelegateCallInLoopDetector;

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

@@ -194,6 +194,7 @@
     "signed-storage-array",
     "weak-randomness",
     "pre-declared-local-variable-usage",
-    "delete-nested-mapping"
+    "delete-nested-mapping",
+    "contract-locks-ether"
   ]
 }