Detector: Incorrect ERC721 contract (#655)

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

aderyn_core/src/detect/detector.rs

@@ -81,6 +81,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
         Box::<TxOriginUsedForAuthDetector>::default(),
         Box::<MsgValueUsedInLoopDetector>::default(),
         Box::<ContractLocksEtherDetector>::default(),
+        Box::<IncorrectERC721InterfaceDetector>::default(),
         Box::<IncorrectERC20InterfaceDetector>::default(),
         Box::<ReturnBombDetector>::default(),
         Box::<OutOfOrderRetryableDetector>::default(),
@@ -96,6 +97,7 @@ pub fn get_all_detectors_names() -> Vec<String> {
 #[derive(Debug, PartialEq, EnumString, Display)]
 #[strum(serialize_all = "kebab-case")]
 pub(crate) enum IssueDetectorNamePool {
+    IncorrectERC721Interface,
     FunctionInitializingState,
     DelegateCallInLoop,
     CentralizationRisk,
@@ -174,6 +176,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::IncorrectERC721Interface => {
+            Some(Box::<IncorrectERC721InterfaceDetector>::default())
+        }
         IssueDetectorNamePool::OutOfOrderRetryable => {
             Some(Box::<OutOfOrderRetryableDetector>::default())
         }

aderyn_core/src/detect/high/incorrect_erc20_interface.rs

@@ -197,29 +197,6 @@ mod erc_matching_function_signature_helper {
             satisifer.satisfies(self)
         }
     }
-
-    // Helpers to match return types (bool & uint256)
-    impl FunctionDefinition {
-        pub fn returns_bool(&self) -> bool {
-            let params = &self.return_parameters.parameters;
-            params.len() == 1
-                && params[0]
-                    .type_descriptions
-                    .type_string
-                    .as_ref()
-                    .is_some_and(|type_string| type_string == "bool")
-        }
-
-        pub fn returns_uint256(&self) -> bool {
-            let params = &self.return_parameters.parameters;
-            params.len() == 1
-                && params[0]
-                    .type_descriptions
-                    .type_string
-                    .as_ref()
-                    .is_some_and(|type_string| type_string == "uint256")
-        }
-    }
 }
 
 #[cfg(test)]

aderyn_core/src/detect/high/incorrect_erc721_interface.rs

@@ -0,0 +1,309 @@
+use std::collections::BTreeMap;
+use std::error::Error;
+
+use crate::ast::{ASTNode, NodeID, Visibility};
+
+use crate::capture;
+use crate::context::browser::ExtractFunctionDefinitions;
+use crate::detect::detector::IssueDetectorNamePool;
+use crate::{
+    context::workspace_context::WorkspaceContext,
+    detect::detector::{IssueDetector, IssueSeverity},
+};
+use eyre::Result;
+
+#[derive(Default)]
+pub struct IncorrectERC721InterfaceDetector {
+    // 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 IncorrectERC721InterfaceDetector {
+    fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
+        // Analyze each contract in context
+        for current_contract in context.contract_definitions() {
+            // Look through it's inheritance heirarchy to determine if it's an ERC721
+            if let Some(contract_ids) = current_contract.linearized_base_contracts.as_ref() {
+                let current_contract_is_erc721 = contract_ids.iter().any(|i| {
+                    context.nodes.get(i).is_some_and(|c| {
+                        if let ASTNode::ContractDefinition(contract) = c {
+                            if contract.name.contains("ERC721") {
+                                return true;
+                            }
+                        }
+                        false
+                    })
+                });
+
+                if !current_contract_is_erc721 {
+                    continue;
+                }
+
+                // Now we know that current contract is an ERC721
+
+                for contract_id in contract_ids {
+                    if let Some(ASTNode::ContractDefinition(contract)) =
+                        context.nodes.get(contract_id)
+                    {
+                        let functions = ExtractFunctionDefinitions::from(contract).extracted;
+
+                        for func in functions {
+                            if (func.visibility != Visibility::Public
+                                && func.visibility != Visibility::External)
+                                || !func.implemented
+                            {
+                                continue;
+                            }
+
+                            if func.represents_erc721_balance_of() && !func.returns_uint256() {
+                                capture!(self, context, func);
+                            }
+
+                            if (func.represents_erc721_get_approved()
+                                || func.represents_erc721_owner_of())
+                                && !func.returns_address()
+                            {
+                                capture!(self, context, func);
+                            }
+
+                            if (func.represents_erc721_safe_transfer_from()
+                                || func.represents_erc721_transfer_from())
+                                && !func.returns_nothing()
+                            {
+                                capture!(self, context, func);
+                            }
+
+                            if (func.represents_erc721_approve()
+                                || func.represents_erc721_set_approval_for_all())
+                                && !func.returns_nothing()
+                            {
+                                capture!(self, context, func);
+                            }
+
+                            if func.represents_erc721_is_approved_for_all() && !func.returns_bool()
+                            {
+                                capture!(self, context, func);
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
+        Ok(!self.found_instances.is_empty())
+    }
+
+    fn severity(&self) -> IssueSeverity {
+        IssueSeverity::High
+    }
+
+    fn title(&self) -> String {
+        String::from("Incorrect ERC721 interface.")
+    }
+
+    fn description(&self) -> String {
+        String::from("Incorrect return values for ERC721 functions. A contract compiled with Solidity > 0.4.22 \
+            interacting with these functions will fail to execute them, as the return value is missing. Set the \
+            appropriate return values and types for the defined ERC721 functions."
+        )
+    }
+
+    fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> {
+        self.found_instances.clone()
+    }
+
+    fn name(&self) -> String {
+        format!("{}", IssueDetectorNamePool::IncorrectERC721Interface)
+    }
+}
+
+mod erc721_matching_function_signature_helper {
+    //! This module matches function signature only (name + parameters)
+    //! This means, that the return value could be different.
+
+    use crate::ast::FunctionDefinition;
+
+    struct SignatureMatcher<'a> {
+        name: &'a str,
+        paramter_types: Vec<&'a str>,
+    }
+
+    // Helps with checking if a function definition satisifed a signature matcher
+    impl<'a> SignatureMatcher<'a> {
+        fn satisfies(&self, func: &FunctionDefinition) -> bool {
+            if func.name != self.name {
+                return false;
+            }
+            let params = &func.parameters.parameters;
+            if params.len() != self.paramter_types.len() {
+                return false;
+            }
+            #[allow(clippy::needless_range_loop)]
+            for idx in 0..params.len() {
+                if let Some(func_param_type) = params[idx].type_descriptions.type_string.as_ref() {
+                    let target = &self.paramter_types[idx];
+                    if *target == "address" {
+                        if func_param_type == "address" || func_param_type == "address payable" {
+                            continue;
+                        } else {
+                            return false;
+                        }
+                    } else if func_param_type != target {
+                        return false;
+                    }
+                } else {
+                    return false;
+                }
+            }
+            true
+        }
+    }
+
+    // ERC721 function signature matching
+    impl FunctionDefinition {
+        pub fn represents_erc721_get_approved(&self) -> bool {
+            let satisifer = SignatureMatcher {
+                name: "getApproved",
+                paramter_types: vec!["uint256"],
+            };
+            satisifer.satisfies(self)
+        }
+
+        pub fn represents_erc721_transfer_from(&self) -> bool {
+            let satisifer = SignatureMatcher {
+                name: "transferFrom",
+                paramter_types: vec!["address", "address", "uint256"],
+            };
+            satisifer.satisfies(self)
+        }
+
+        pub fn represents_erc721_approve(&self) -> bool {
+            let satisifer = SignatureMatcher {
+                name: "approve",
+                paramter_types: vec!["address", "uint256"],
+            };
+            satisifer.satisfies(self)
+        }
+
+        pub fn represents_erc721_is_approved_for_all(&self) -> bool {
+            let satisifer = SignatureMatcher {
+                name: "isApprovedForAll",
+                paramter_types: vec!["address", "address"],
+            };
+            satisifer.satisfies(self)
+        }
+
+        pub fn represents_erc721_balance_of(&self) -> bool {
+            let satisifer = SignatureMatcher {
+                name: "balanceOf",
+                paramter_types: vec!["address"],
+            };
+            satisifer.satisfies(self)
+        }
+
+        pub fn represents_erc721_owner_of(&self) -> bool {
+            let satisifer = SignatureMatcher {
+                name: "ownerOf",
+                paramter_types: vec!["uint256"],
+            };
+            satisifer.satisfies(self)
+        }
+
+        pub fn represents_erc721_safe_transfer_from(&self) -> bool {
+            let type_1_staisfier = SignatureMatcher {
+                name: "safeTransferFrom",
+                paramter_types: vec!["address", "address", "uint256", "bytes"],
+            };
+
+            let type_2_satisifer = SignatureMatcher {
+                name: "safeTransferFrom",
+                paramter_types: vec!["address", "address", "uint256"],
+            };
+
+            type_1_staisfier.satisfies(self) || type_2_satisifer.satisfies(self)
+        }
+
+        pub fn represents_erc721_set_approval_for_all(&self) -> bool {
+            let satisfier = SignatureMatcher {
+                name: "setApprovalForAll",
+                paramter_types: vec!["address", "bool"],
+            };
+            satisfier.satisfies(self)
+        }
+    }
+
+    // Helpers to match return types (bool & uint256)
+    impl FunctionDefinition {
+        pub fn returns_nothing(&self) -> bool {
+            self.return_parameters.parameters.is_empty()
+        }
+
+        pub fn returns_bool(&self) -> bool {
+            let params = &self.return_parameters.parameters;
+            params.len() == 1
+                && params[0]
+                    .type_descriptions
+                    .type_string
+                    .as_ref()
+                    .is_some_and(|type_string| type_string == "bool")
+        }
+
+        pub fn returns_uint256(&self) -> bool {
+            let params = &self.return_parameters.parameters;
+            params.len() == 1
+                && params[0]
+                    .type_descriptions
+                    .type_string
+                    .as_ref()
+                    .is_some_and(|type_string| type_string == "uint256")
+        }
+
+        pub fn returns_address(&self) -> bool {
+            let params = &self.return_parameters.parameters;
+            params.len() == 1
+                && params[0]
+                    .type_descriptions
+                    .type_string
+                    .as_ref()
+                    .is_some_and(|type_string| {
+                        type_string == "address" || type_string == "address payable"
+                    })
+        }
+    }
+}
+
+#[cfg(test)]
+mod incorrect_erc721_tests {
+    use serial_test::serial;
+
+    use crate::detect::{
+        detector::IssueDetector, high::incorrect_erc721_interface::IncorrectERC721InterfaceDetector,
+    };
+
+    #[test]
+    #[serial]
+    fn test_incorrect_erc721_functions() {
+        let context = crate::detect::test_utils::load_solidity_source_unit(
+            "../tests/contract-playground/src/IncorrectERC721.sol",
+        );
+
+        let mut detector = IncorrectERC721InterfaceDetector::default();
+        let found = detector.detect(&context).unwrap();
+
+        // We capture every faulty method in the IncorrectERC721 contract that has the wrong return type
+        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(), 8);
+
+        // assert the severity is high
+        assert_eq!(
+            detector.severity(),
+            crate::detect::detector::IssueSeverity::High
+        );
+    }
+}

aderyn_core/src/detect/high/mod.rs

@@ -12,6 +12,7 @@ pub(crate) mod enumerable_loop_removal;
 pub(crate) mod experimental_encoder;
 pub(crate) mod incorrect_caret_operator;
 pub(crate) mod incorrect_erc20_interface;
+pub(crate) mod incorrect_erc721_interface;
 pub(crate) mod incorrect_shift_order;
 pub(crate) mod misused_boolean;
 pub(crate) mod msg_value_in_loops;
@@ -51,6 +52,7 @@ pub use enumerable_loop_removal::EnumerableLoopRemovalDetector;
 pub use experimental_encoder::ExperimentalEncoderDetector;
 pub use incorrect_caret_operator::IncorrectUseOfCaretOperatorDetector;
 pub use incorrect_erc20_interface::IncorrectERC20InterfaceDetector;
+pub use incorrect_erc721_interface::IncorrectERC721InterfaceDetector;
 pub use incorrect_shift_order::IncorrectShiftOrderDetector;
 pub use misused_boolean::MisusedBooleanDetector;
 pub use msg_value_in_loops::MsgValueUsedInLoopDetector;

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-erc721-interface",
     "incorrect-erc20-interface",
     "out-of-order-retryable"
   ]