Detector: Misused boolean (#607)

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

aderyn_core/src/ast/ast.rs

@@ -80,3 +80,69 @@ impl From<&YulLiteral> for ASTNode {
         ASTNode::YulLiteral(value.clone())
     }
 }
+
+impl From<&Expression> for ASTNode {
+    fn from(value: &Expression) -> Self {
+        match value {
+            Expression::Literal(literal) => ASTNode::Literal(literal.clone()),
+            Expression::Identifier(identifier) => ASTNode::Identifier(identifier.clone()),
+            Expression::UnaryOperation(unary_operation) => {
+                ASTNode::UnaryOperation(unary_operation.clone())
+            }
+            Expression::BinaryOperation(binary_operation) => {
+                ASTNode::BinaryOperation(binary_operation.clone())
+            }
+            Expression::Conditional(conditional) => ASTNode::Conditional(conditional.clone()),
+            Expression::Assignment(assignment) => ASTNode::Assignment(assignment.clone()),
+            Expression::FunctionCall(function_call) => ASTNode::FunctionCall(function_call.clone()),
+            Expression::FunctionCallOptions(function_call_ops) => {
+                ASTNode::FunctionCallOptions(function_call_ops.clone())
+            }
+            Expression::IndexAccess(index_access) => ASTNode::IndexAccess(index_access.clone()),
+            Expression::IndexRangeAccess(index_range_access) => {
+                ASTNode::IndexRangeAccess(index_range_access.clone())
+            }
+            Expression::MemberAccess(member_access) => ASTNode::MemberAccess(member_access.clone()),
+            Expression::ElementaryTypeNameExpression(elementary_type_name_expression) => {
+                ASTNode::ElementaryTypeNameExpression(elementary_type_name_expression.clone())
+            }
+            Expression::TupleExpression(tuple_expression) => {
+                ASTNode::TupleExpression(tuple_expression.clone())
+            }
+            Expression::NewExpression(new_expression) => {
+                ASTNode::NewExpression(new_expression.clone())
+            }
+        }
+    }
+}
+
+impl From<Expression> for ASTNode {
+    fn from(value: Expression) -> Self {
+        match value {
+            Expression::Literal(literal) => ASTNode::Literal(literal),
+            Expression::Identifier(identifier) => ASTNode::Identifier(identifier),
+            Expression::UnaryOperation(unary_operation) => ASTNode::UnaryOperation(unary_operation),
+            Expression::BinaryOperation(binary_operation) => {
+                ASTNode::BinaryOperation(binary_operation)
+            }
+            Expression::Conditional(conditional) => ASTNode::Conditional(conditional),
+            Expression::Assignment(assignment) => ASTNode::Assignment(assignment),
+            Expression::FunctionCall(function_call) => ASTNode::FunctionCall(function_call),
+            Expression::FunctionCallOptions(function_call_ops) => {
+                ASTNode::FunctionCallOptions(function_call_ops)
+            }
+            Expression::IndexAccess(index_access) => ASTNode::IndexAccess(index_access),
+            Expression::IndexRangeAccess(index_range_access) => {
+                ASTNode::IndexRangeAccess(index_range_access)
+            }
+            Expression::MemberAccess(member_access) => ASTNode::MemberAccess(member_access),
+            Expression::ElementaryTypeNameExpression(elementary_type_name_expression) => {
+                ASTNode::ElementaryTypeNameExpression(elementary_type_name_expression)
+            }
+            Expression::TupleExpression(tuple_expression) => {
+                ASTNode::TupleExpression(tuple_expression)
+            }
+            Expression::NewExpression(new_expression) => ASTNode::NewExpression(new_expression),
+        }
+    }
+}

aderyn_core/src/context/investigator/callgraph_tests.rs

@@ -19,7 +19,7 @@ mod callgraph_tests {
             context
                 .function_definitions()
                 .into_iter()
-                .find(|&x| x.name == name.to_string())
+                .find(|&x| x.name == *name)
                 .unwrap(),
         )
     }
@@ -29,7 +29,7 @@ mod callgraph_tests {
             context
                 .modifier_definitions()
                 .into_iter()
-                .find(|&x| x.name == name.to_string())
+                .find(|&x| x.name == *name)
                 .unwrap(),
         )
     }

aderyn_core/src/detect/detector.rs

@@ -4,35 +4,9 @@ use strum::{Display, EnumCount, EnumIter, EnumString};
 use crate::{
     ast::NodeID,
     context::workspace_context::WorkspaceContext,
-    detect::{
-        high::{
-            ArbitraryTransferFromDetector, AvoidAbiEncodePackedDetector,
-            BlockTimestampDeadlineDetector, DangerousUnaryOperatorDetector,
-            DelegateCallInLoopDetector, DelegateCallOnUncheckedAddressDetector,
-            DynamicArrayLengthAssignmentDetector, EnumerableLoopRemovalDetector,
-            ExperimentalEncoderDetector, IncorrectShiftOrderDetector,
-            IncorrectUseOfCaretOperatorDetector, MultipleConstructorsDetector,
-            NestedStructInMappingDetector, RTLODetector, ReusedContractNameDetector,
-            SelfdestructIdentifierDetector, SendEtherNoChecksDetector,
-            StateVariableShadowingDetector, StorageArrayEditWithMemoryDetector,
-            TautologicalCompareDetector, UncheckedReturnDetector,
-            UninitializedStateVariableDetector, UnprotectedInitializerDetector,
-            UnsafeCastingDetector, YulReturnDetector,
-        },
-        low::{
-            CentralizationRiskDetector, ConstantsInsteadOfLiteralsDetector,
-            ContractsWithTodosDetector, DeprecatedOZFunctionsDetector,
-            DivisionBeforeMultiplicationDetector, EcrecoverDetector, EmptyBlockDetector,
-            InconsistentTypeNamesDetector, LargeLiteralValueDetector,
-            NonReentrantBeforeOthersDetector, PushZeroOpcodeDetector, RequireWithStringDetector,
-            RevertsAndRequiresInLoopsDetector, SolmateSafeTransferLibDetector,
-            UnindexedEventsDetector, UnsafeERC20FunctionsDetector, UnsafeERC721MintDetector,
-            UnspecificSolidityPragmaDetector, UselessErrorDetector,
-            UselessInternalFunctionDetector, UselessModifierDetector,
-            UselessPublicFunctionDetector, ZeroAddressCheckDetector,
-        },
-    },
+    detect::{high::*, low::*},
 };
+
 use std::{
     collections::BTreeMap,
     error::Error,
@@ -84,6 +58,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
         Box::<IncorrectUseOfCaretOperatorDetector>::default(),
         Box::<YulReturnDetector>::default(),
         Box::<StateVariableShadowingDetector>::default(),
+        Box::<MisusedBooleanDetector>::default(),
         Box::<SendEtherNoChecksDetector>::default(),
         Box::<DelegateCallOnUncheckedAddressDetector>::default(),
         Box::<TautologicalCompareDetector>::default(),
@@ -143,6 +118,7 @@ pub(crate) enum IssueDetectorNamePool {
     IncorrectCaretOperator,
     YulReturn,
     StateVariableShadowing,
+    MisusedBoolean,
     SendEtherNoChecks,
     DelegateCallUncheckedAddress,
     TautologicalCompare,
@@ -270,6 +246,7 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option<Box<dyn Iss
         IssueDetectorNamePool::StateVariableShadowing => {
             Some(Box::<StateVariableShadowingDetector>::default())
         }
+        IssueDetectorNamePool::MisusedBoolean => Some(Box::<MisusedBooleanDetector>::default()),
         IssueDetectorNamePool::SendEtherNoChecks => {
             Some(Box::<SendEtherNoChecksDetector>::default())
         }

aderyn_core/src/detect/helpers.rs

@@ -2,8 +2,8 @@ use semver::{Error, VersionReq};
 
 use crate::{
     ast::{
-        ASTNode, Expression, FunctionDefinition, MemberAccess, NodeID, PragmaDirective,
-        VariableDeclaration, Visibility,
+        ASTNode, Expression, FunctionDefinition, Identifier, LiteralKind, MemberAccess, NodeID,
+        PragmaDirective, VariableDeclaration, Visibility,
     },
     context::{
         browser::{
@@ -221,3 +221,45 @@ pub fn get_literal_value_or_constant_variable_value(
     }
     None
 }
+
+/*
+Check if expression in constant
+Expression::Literal whose value is false/true
+Expression::Identifier that refers to a constant boolean variable declaration
+Expression::UnaryOperation with ! operator followed by a sub expression that could be either of the above
+*/
+pub fn is_constant_boolean(context: &WorkspaceContext, ast_node: &Expression) -> bool {
+    if let Expression::Literal(literal) = ast_node {
+        if literal.kind == LiteralKind::Bool
+            && literal
+                .value
+                .as_ref()
+                .is_some_and(|value| value == "false" || value == "true")
+        {
+            return true;
+        }
+    }
+    if let Expression::Identifier(Identifier {
+        referenced_declaration: Some(id),
+        ..
+    }) = ast_node
+    {
+        if let Some(ASTNode::VariableDeclaration(variable_declaration)) = context.nodes.get(id) {
+            if variable_declaration
+                .type_descriptions
+                .type_string
+                .as_ref()
+                .is_some_and(|value| value == "bool")
+                && variable_declaration.mutability() == Some(&crate::ast::Mutability::Constant)
+            {
+                return true;
+            }
+        }
+    }
+    if let Expression::UnaryOperation(operation) = ast_node {
+        if operation.operator == "!" {
+            return is_constant_boolean(context, operation.sub_expression.as_ref());
+        }
+    }
+    false
+}

aderyn_core/src/detect/high/delegate_call_no_address_check.rs

@@ -68,6 +68,26 @@ impl IssueDetector for DelegateCallOnUncheckedAddressDetector {
     }
 }
 
+struct DelegateCallNoAddressChecksTracker<'a> {
+    has_address_checks: bool,
+    has_delegate_call_on_non_state_variable_address: bool,
+    context: &'a WorkspaceContext,
+}
+
+impl<'a> StandardInvestigatorVisitor for DelegateCallNoAddressChecksTracker<'a> {
+    fn visit_any(&mut self, node: &crate::context::workspace_context::ASTNode) -> eyre::Result<()> {
+        if !self.has_address_checks && helpers::has_binary_checks_on_some_address(node) {
+            self.has_address_checks = true;
+        }
+        if !self.has_delegate_call_on_non_state_variable_address
+            && helpers::has_delegate_calls_on_non_state_variables(node, self.context)
+        {
+            self.has_delegate_call_on_non_state_variable_address = true;
+        }
+        eyre::Ok(())
+    }
+}
+
 #[cfg(test)]
 mod delegate_call_no_address_check_tests {
     use crate::detect::{
@@ -110,23 +130,3 @@ mod delegate_call_no_address_check_tests {
         );
     }
 }
-
-struct DelegateCallNoAddressChecksTracker<'a> {
-    has_address_checks: bool,
-    has_delegate_call_on_non_state_variable_address: bool,
-    context: &'a WorkspaceContext,
-}
-
-impl<'a> StandardInvestigatorVisitor for DelegateCallNoAddressChecksTracker<'a> {
-    fn visit_any(&mut self, node: &crate::context::workspace_context::ASTNode) -> eyre::Result<()> {
-        if !self.has_address_checks && helpers::has_binary_checks_on_some_address(node) {
-            self.has_address_checks = true;
-        }
-        if !self.has_delegate_call_on_non_state_variable_address
-            && helpers::has_delegate_calls_on_non_state_variables(node, self.context)
-        {
-            self.has_delegate_call_on_non_state_variable_address = true;
-        }
-        eyre::Ok(())
-    }
-}

aderyn_core/src/detect/high/misused_boolean.rs

@@ -0,0 +1,55 @@
+use crate::detect::helpers::is_constant_boolean;
+use crate::issue_detector;
+use eyre::Result;
+
+issue_detector! {
+    MisusedBooleanDetector;
+
+    severity: High,
+    title: "Misused boolean with logical operators",
+    desc: "The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively.",
+    name: MisusedBoolean,
+
+    |context| {
+        for binary_operation in context.binary_operations() {
+            if (binary_operation.operator == "||" || binary_operation.operator == "&&")
+                && [
+                    binary_operation.left_expression.as_ref(),
+                    binary_operation.right_expression.as_ref(),
+                ]
+                .iter()
+                .any(|&operand| is_constant_boolean(context, operand))
+            {
+                grab!(binary_operation);
+            }
+
+        }
+
+        for if_statement in context.if_statements()
+            .iter()
+            .filter(|statement| is_constant_boolean(context, &statement.condition)) {
+            grab!(if_statement);
+        }
+
+    }
+
+}
+
+#[cfg(test)]
+mod misused_boolean_tests {
+    use crate::detect::{detector::IssueDetector, high::misused_boolean::MisusedBooleanDetector};
+
+    #[test]
+    fn test_misused_boolean_by_loading_contract_directly() {
+        let context = crate::detect::test_utils::load_solidity_source_unit(
+            "../tests/contract-playground/src/MisusedBoolean.sol",
+        );
+
+        let mut detector = MisusedBooleanDetector::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(), 10);
+    }
+}

aderyn_core/src/detect/high/mod.rs

@@ -9,6 +9,7 @@ pub(crate) mod enumerable_loop_removal;
 pub(crate) mod experimental_encoder;
 pub(crate) mod incorrect_caret_operator;
 pub(crate) mod incorrect_shift_order;
+pub(crate) mod misused_boolean;
 pub(crate) mod multiple_constructors;
 pub(crate) mod nested_struct_in_mapping;
 pub(crate) mod reused_contract_name;
@@ -35,6 +36,7 @@ pub use enumerable_loop_removal::EnumerableLoopRemovalDetector;
 pub use experimental_encoder::ExperimentalEncoderDetector;
 pub use incorrect_caret_operator::IncorrectUseOfCaretOperatorDetector;
 pub use incorrect_shift_order::IncorrectShiftOrderDetector;
+pub use misused_boolean::MisusedBooleanDetector;
 pub use multiple_constructors::MultipleConstructorsDetector;
 pub use nested_struct_in_mapping::NestedStructInMappingDetector;
 pub use reused_contract_name::ReusedContractNameDetector;