Detector: Incorrect ERC20 interface (#654)

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

aderyn_core/src/detect/detector.rs

@@ -1,3 +1,4 @@
+use incorrect_erc20_interface::IncorrectERC20InterfaceDetector;
 use serde::{Deserialize, Serialize};
 use strum::{Display, EnumCount, EnumIter, EnumString};
 
@@ -80,6 +81,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
         Box::<TxOriginUsedForAuthDetector>::default(),
         Box::<MsgValueUsedInLoopDetector>::default(),
         Box::<ContractLocksEtherDetector>::default(),
+        Box::<IncorrectERC20InterfaceDetector>::default(),
         Box::<ReturnBombDetector>::default(),
     ]
 }
@@ -157,6 +159,7 @@ pub(crate) enum IssueDetectorNamePool {
     TxOriginUsedForAuth,
     MsgValueInLoop,
     ContractLocksEther,
+    IncorrectERC20Interface,
     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
@@ -167,6 +170,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::IncorrectERC20Interface => {
+            Some(Box::<IncorrectERC20InterfaceDetector>::default())
+        }
         IssueDetectorNamePool::ReturnBomb => Some(Box::<ReturnBombDetector>::default()),
         IssueDetectorNamePool::UnusedStateVariable => {
             Some(Box::<UnusedStateVariablesDetector>::default())

aderyn_core/src/detect/high/incorrect_erc20_interface.rs

@@ -0,0 +1,252 @@
+use std::collections::BTreeMap;
+use std::convert::identity;
+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 IncorrectERC20InterfaceDetector {
+    // 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 IncorrectERC20InterfaceDetector {
+    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 ERC20
+            if let Some(contract_ids) = current_contract.linearized_base_contracts.as_ref() {
+                let current_contract_is_erc20 = contract_ids.iter().any(|i| {
+                    context.nodes.get(i).is_some_and(|c| {
+                        if let ASTNode::ContractDefinition(contract) = c {
+                            if contract.name.contains("ERC20") {
+                                return true;
+                            }
+                        }
+                        false
+                    })
+                });
+
+                if !current_contract_is_erc20 {
+                    continue;
+                }
+
+                // Now we know that current contract is an ERC20
+
+                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_erc20_transfer().is_some_and(identity)
+                                || func.represents_erc20_transfer_from().is_some_and(identity)
+                                || func.represents_erc20_approve().is_some_and(identity))
+                                && !func.returns_bool()
+                            {
+                                capture!(self, context, func);
+                            }
+
+                            if (func.represents_erc20_allowance().is_some_and(identity)
+                                || func.represents_erc20_balance_of().is_some_and(identity)
+                                || func.represents_erc20_total_supply().is_some_and(identity))
+                                && !func.returns_uint256()
+                            {
+                                capture!(self, context, func)
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
+        Ok(!self.found_instances.is_empty())
+    }
+
+    fn severity(&self) -> IssueSeverity {
+        IssueSeverity::High
+    }
+
+    fn title(&self) -> String {
+        String::from("Incorrect ERC20 interface.")
+    }
+
+    fn description(&self) -> String {
+        String::from("Incorrect return values for ERC20 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 ERC20 functions."
+        )
+    }
+
+    fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> {
+        self.found_instances.clone()
+    }
+
+    fn name(&self) -> String {
+        format!("{}", IssueDetectorNamePool::IncorrectERC20Interface)
+    }
+}
+
+mod erc_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) -> Option<bool> {
+            if func.name != self.name {
+                return Some(false);
+            }
+            let params = &func.parameters.parameters;
+            if params.len() != self.paramter_types.len() {
+                return Some(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 Some(false);
+                        }
+                    } else if func_param_type != target {
+                        return Some(false);
+                    }
+                } else {
+                    return None;
+                }
+            }
+            Some(true)
+        }
+    }
+
+    // ERC20 function signature matching
+    impl FunctionDefinition {
+        pub fn represents_erc20_transfer(&self) -> Option<bool> {
+            let satisifer = SignatureMatcher {
+                name: "transfer",
+                paramter_types: vec!["address", "uint256"],
+            };
+            satisifer.satisfies(self)
+        }
+
+        pub fn represents_erc20_transfer_from(&self) -> Option<bool> {
+            let satisifer = SignatureMatcher {
+                name: "transferFrom",
+                paramter_types: vec!["address", "address", "uint256"],
+            };
+            satisifer.satisfies(self)
+        }
+
+        pub fn represents_erc20_approve(&self) -> Option<bool> {
+            let satisifer = SignatureMatcher {
+                name: "approve",
+                paramter_types: vec!["address", "uint256"],
+            };
+            satisifer.satisfies(self)
+        }
+
+        pub fn represents_erc20_allowance(&self) -> Option<bool> {
+            let satisifer = SignatureMatcher {
+                name: "allowance",
+                paramter_types: vec!["address", "address"],
+            };
+            satisifer.satisfies(self)
+        }
+
+        pub fn represents_erc20_balance_of(&self) -> Option<bool> {
+            let satisifer = SignatureMatcher {
+                name: "balanceOf",
+                paramter_types: vec!["address"],
+            };
+            satisifer.satisfies(self)
+        }
+
+        pub fn represents_erc20_total_supply(&self) -> Option<bool> {
+            let satisifer = SignatureMatcher {
+                name: "totalSupply",
+                paramter_types: vec![],
+            };
+            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)]
+mod incorrect_erc20_tests {
+    use serial_test::serial;
+
+    use crate::detect::{
+        detector::IssueDetector, high::incorrect_erc20_interface::IncorrectERC20InterfaceDetector,
+    };
+
+    #[test]
+    #[serial]
+    fn test_incorrect_erc20_functions() {
+        let context = crate::detect::test_utils::load_solidity_source_unit(
+            "../tests/contract-playground/src/IncorrectERC20.sol",
+        );
+
+        let mut detector = IncorrectERC20InterfaceDetector::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(), 5);
+        // assert the severity is high
+        assert_eq!(
+            detector.severity(),
+            crate::detect::detector::IssueSeverity::High
+        );
+    }
+}

aderyn_core/src/detect/high/mod.rs

@@ -11,6 +11,7 @@ pub(crate) mod dynamic_array_length_assignment;
 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_shift_order;
 pub(crate) mod misused_boolean;
 pub(crate) mod msg_value_in_loops;
@@ -48,6 +49,7 @@ pub use dynamic_array_length_assignment::DynamicArrayLengthAssignmentDetector;
 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_shift_order::IncorrectShiftOrderDetector;
 pub use misused_boolean::MisusedBooleanDetector;
 pub use msg_value_in_loops::MsgValueUsedInLoopDetector;

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

@@ -197,6 +197,7 @@
     "delete-nested-mapping",
     "tx-origin-used-for-auth",
     "msg-value-in-loop",
-    "contract-locks-ether"
+    "contract-locks-ether",
+    "incorrect-erc20-interface"
   ]
 }