Fix: New Strategy for ArbitraryTransferFrom Detector (#776)

Author: TilakMaddy

aderyn_core/src/detect/high/arbitrary_transfer_from.rs

@@ -1,11 +1,13 @@
 use std::{collections::BTreeMap, error::Error};
 
-use crate::ast::{Expression, FunctionCall, NodeID, TypeName};
-
 use crate::{
+    ast::{Expression, Identifier, NodeID},
     capture,
-    context::workspace_context::WorkspaceContext,
-    detect::detector::{IssueDetector, IssueDetectorNamePool, IssueSeverity},
+    context::{browser::ExtractFunctionCalls, workspace_context::WorkspaceContext},
+    detect::{
+        detector::{IssueDetector, IssueDetectorNamePool, IssueSeverity},
+        helpers::{get_implemented_external_and_public_functions, has_msg_sender_binary_operation},
+    },
 };
 use eyre::Result;
 
@@ -16,51 +18,69 @@ pub struct ArbitraryTransferFromDetector {
     found_instances: BTreeMap<(String, usize, String), NodeID>,
 }
 
-// Check if the first argument of the function call is valid
-// In function calls with 3 args, the first arg [0] is the `from` address
-// In function calls with 4 args, the second arg [1] is the `from` address
-fn check_argument_validity(function_call: &FunctionCall) -> bool {
-    let arg_index = if function_call.arguments.len() == 3 {
-        0
-    } else if function_call.arguments.len() == 4 {
-        1
-    } else {
-        return false;
-    };
-
-    match &function_call.arguments[arg_index] {
-        Expression::MemberAccess(arg_member_access) => {
-            !(arg_member_access.member_name == "sender"
-                && matches!(&*arg_member_access.expression, Expression::Identifier(identifier) if identifier.name == "msg"))
-        }
-        Expression::FunctionCall(arg_function_call) => {
-            !(matches!(&*arg_function_call.expression, Expression::ElementaryTypeNameExpression(arg_el_type_name_exp) if matches!(&arg_el_type_name_exp.type_name, TypeName::ElementaryTypeName(type_name) if type_name.name == "address"))
-                && matches!(arg_function_call.arguments.first(), Some(Expression::Identifier(arg_identifier)) if arg_identifier.name == "this"))
-        }
-        _ => true,
-    }
-}
-
 impl IssueDetector for ArbitraryTransferFromDetector {
     fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
-        let transfer_from_function_calls =
-            context.function_calls().into_iter().filter(|&function_call| {
-                // For each function call, check if the function call is a member access
-                // and if the member name is "transferFrom" or "safeTransferFrom", then check if the
-                // first argument is valid If the first argument is valid, add the
-                // function call to found_instances
-                if let Expression::MemberAccess(member_access) = &*function_call.expression {
-                    if member_access.member_name == "transferFrom"
-                        || member_access.member_name == "safeTransferFrom"
-                    {
-                        return check_argument_validity(function_call);
-                    }
-                }
-                false
+        // Applying devtooligan's suggestion
+        // * Operate on public and external functions only
+        // * See that msg.sender is not checked
+        // * Check that the argument passed in is from the parameter list of the said function
+
+        let suspected_functions =
+            get_implemented_external_and_public_functions(context).filter(|function_definition| {
+                !has_msg_sender_binary_operation(&((*function_definition).into()))
+                    && function_definition.modifiers.is_empty() // If there are modifiers, assume
+                                                                // the function is safe because
+                                                                // sometime modifiers' definition
+                                                                // may not be in scope
             });
 
-        for item in transfer_from_function_calls {
-            capture!(self, context, item);
+        for func in suspected_functions {
+            let func_parameters_ids =
+                &func.parameters.parameters.iter().map(|f| f.id).collect::<Vec<_>>();
+
+            let transfer_func_calls = ExtractFunctionCalls::from(func)
+                .extracted
+                .into_iter()
+                .filter(|function_call| {
+                    // For each function call, check if the function call is a member access
+                    // and if the member name is "transferFrom" or "safeTransferFrom", then check if
+                    // the first argument is valid If the first argument is
+                    // valid, add the function call to found_instances
+                    if let Expression::MemberAccess(member_access) = &*function_call.expression {
+                        if member_access.member_name == "transferFrom"
+                            || member_access.member_name == "safeTransferFrom"
+                        {
+                            return true;
+                        }
+                    }
+                    false
+                })
+                .collect::<Vec<_>>();
+
+            for func in transfer_func_calls {
+                // Check if the first argument of the function call is valid
+                // In function calls with 3 args, the first arg [0] is the `from` address
+                // In function calls with 4 args, the second arg [1] is the `from` address
+                let arg_index = if func.arguments.len() == 3 {
+                    0
+                } else if func.arguments.len() == 4 {
+                    1
+                } else {
+                    continue;
+                };
+
+                let arg = &func.arguments[arg_index];
+
+                if let Expression::Identifier(Identifier {
+                    referenced_declaration: Some(referenced_id),
+                    ..
+                }) = arg
+                {
+                    if func_parameters_ids.iter().any(|r| r == referenced_id) {
+                        capture!(self, context, func);
+                    }
+                }
+            }
         }
 
         Ok(!self.found_instances.is_empty())
@@ -107,7 +127,7 @@ mod arbitrary_transfer_from_tests {
         // assert that the detector found an issue
         assert!(found);
         // assert that the detector found the correct number of instances
-        assert_eq!(detector.instances().len(), 4);
+        assert_eq!(detector.instances().len(), 2);
         // assert the severity is high
         assert_eq!(detector.severity(), crate::detect::detector::IssueSeverity::High);
         // assert the title is correct