diff --git a/aderyn_core/src/ast/ast.rs b/aderyn_core/src/ast/ast.rs index 843053a43..1daade1a9 100644 --- a/aderyn_core/src/ast/ast.rs +++ b/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 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), + } + } +} diff --git a/aderyn_core/src/context/investigator/callgraph_tests.rs b/aderyn_core/src/context/investigator/callgraph_tests.rs index 2ab6290d6..faa0a1ccc 100644 --- a/aderyn_core/src/context/investigator/callgraph_tests.rs +++ b/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(), ) } diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 2359ab314..11b46f801 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/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::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), Box::::default(), Box::::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 { Some(Box::::default()) } + IssueDetectorNamePool::MisusedBoolean => Some(Box::::default()), IssueDetectorNamePool::SendEtherNoChecks => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/helpers.rs b/aderyn_core/src/detect/helpers.rs index d7b535eef..f19a79c50 100644 --- a/aderyn_core/src/detect/helpers.rs +++ b/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 +} diff --git a/aderyn_core/src/detect/high/delegate_call_no_address_check.rs b/aderyn_core/src/detect/high/delegate_call_no_address_check.rs index 238002154..10c816894 100644 --- a/aderyn_core/src/detect/high/delegate_call_no_address_check.rs +++ b/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(()) - } -} diff --git a/aderyn_core/src/detect/high/misused_boolean.rs b/aderyn_core/src/detect/high/misused_boolean.rs new file mode 100644 index 000000000..201471f37 --- /dev/null +++ b/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); + } +} diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index cacae63d6..e247206b9 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/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; diff --git a/aderyn_core/src/detect/mod.rs b/aderyn_core/src/detect/mod.rs index 467a84bb3..fc80fd369 100644 --- a/aderyn_core/src/detect/mod.rs +++ b/aderyn_core/src/detect/mod.rs @@ -24,4 +24,74 @@ macro_rules! capture { }; } +#[macro_export] +macro_rules! issue_detector { + ( + $detector_struct:ident; + + severity: $detector_severity:ident, + title: $detector_title:tt, + desc: $detector_desc:tt, + name: $detector_name:ident, + + |$context: ident| $e:expr + ) => { + + #[derive(Default)] + pub struct $detector_struct { + found_instances: std::collections::BTreeMap<(String, usize, String), $crate::ast::NodeID>, + } + + impl $crate::detect::detector::IssueDetector for $detector_struct { + + fn detect(&mut self, context: &$crate::context::workspace_context::WorkspaceContext) -> Result> { + + let $context = context; + + macro_rules! grab { + ($item:expr) => { + if let Some(id) = context.get_node_id_of_capturable(&$item.clone().into()) { + self.found_instances.insert( + $context.get_node_sort_key_from_capturable(&$item.clone().into()), + id, + ); + } else { + self.found_instances.insert( + $context.get_node_sort_key_from_capturable(&$item.clone().into()), + 0, + ); + } + }; + } + + $e + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> $crate::detect::detector::IssueSeverity { + $crate::detect::detector::IssueSeverity::$detector_severity + } + + fn title(&self) -> String { + String::from($detector_title) + } + + fn description(&self) -> String { + String::from($detector_desc) + } + + fn instances(&self) -> std::collections::BTreeMap<(String, usize, String), $crate::ast::NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + $crate::detect::detector::IssueDetectorNamePool::$detector_name.to_string() + } + } + + }; +} + pub use capture; + +pub use issue_detector; diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 828564194..d3797769c 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -181,6 +181,7 @@ "incorrect-caret-operator", "yul-return", "state-variable-shadowing", + "misused-boolean", "send-ether-no-checks", "delegate-call-unchecked-address", "tautological-compare", diff --git a/reports/report.json b/reports/report.json index 80d2ddcca..713071135 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 63, - "total_sloc": 1758 + "total_source_units": 64, + "total_sloc": 1825 }, "files_details": { "files_details": [ @@ -101,6 +101,10 @@ "file_path": "src/KeccakContract.sol", "n_sloc": 21 }, + { + "file_path": "src/MisusedBoolean.sol", + "n_sloc": 67 + }, { "file_path": "src/MultipleConstructorSchemes.sol", "n_sloc": 10 @@ -260,7 +264,7 @@ ] }, "issue_count": { - "high": 25, + "high": 26, "low": 23 }, "high_issues": { @@ -1364,6 +1368,73 @@ } ] }, + { + "title": "Misused boolean with logical operators", + "description": "The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively.", + "detector_name": "misused-boolean", + "instances": [ + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 12, + "src": "257:19", + "src_char": "257:19" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 19, + "src": "419:20", + "src_char": "419:20" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 26, + "src": "582:20", + "src_char": "582:20" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 33, + "src": "745:19", + "src_char": "745:19" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 40, + "src": "908:51", + "src_char": "908:51" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 47, + "src": "1060:52", + "src_char": "1060:52" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 54, + "src": "1213:53", + "src_char": "1213:53" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 61, + "src": "1366:21", + "src_char": "1366:21" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 68, + "src": "1530:17", + "src_char": "1530:17" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 75, + "src": "1691:18", + "src_char": "1691:18" + } + ] + }, { "title": "Sending native Eth is not protected from these functions.", "description": "Introduce checks for `msg.sender` in the function", @@ -3242,6 +3313,7 @@ "incorrect-caret-operator", "yul-return", "state-variable-shadowing", + "misused-boolean", "send-ether-no-checks", "delegate-call-unchecked-address", "tautological-compare", diff --git a/reports/report.md b/reports/report.md index 56b9e5d2b..0ccc72620 100644 --- a/reports/report.md +++ b/reports/report.md @@ -27,12 +27,13 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-17: Incorrect use of caret operator on a non hexadcimal constant](#h-17-incorrect-use-of-caret-operator-on-a-non-hexadcimal-constant) - [H-18: Yul block contains `return` function call.](#h-18-yul-block-contains-return-function-call) - [H-19: High Issue Title](#h-19-high-issue-title) - - [H-20: Sending native Eth is not protected from these functions.](#h-20-sending-native-eth-is-not-protected-from-these-functions) - - [H-21: Delegatecall made by the function without checks on any adress.](#h-21-delegatecall-made-by-the-function-without-checks-on-any-adress) - - [H-22: Tautological comparison.](#h-22-tautological-comparison) - - [H-23: RTLO character detected in file. \u{202e}](#h-23-rtlo-character-detected-in-file-u202e) - - [H-24: Return value of the function call is not checked.](#h-24-return-value-of-the-function-call-is-not-checked) - - [H-25: Dangerous unary operator found in assignment.](#h-25-dangerous-unary-operator-found-in-assignment) + - [H-20: Misused boolean with logical operators](#h-20-misused-boolean-with-logical-operators) + - [H-21: Sending native Eth is not protected from these functions.](#h-21-sending-native-eth-is-not-protected-from-these-functions) + - [H-22: Delegatecall made by the function without checks on any adress.](#h-22-delegatecall-made-by-the-function-without-checks-on-any-adress) + - [H-23: Tautological comparison.](#h-23-tautological-comparison) + - [H-24: RTLO character detected in file. \u{202e}](#h-24-rtlo-character-detected-in-file-u202e) + - [H-25: Return value of the function call is not checked.](#h-25-return-value-of-the-function-call-is-not-checked) + - [H-26: Dangerous unary operator found in assignment.](#h-26-dangerous-unary-operator-found-in-assignment) - [Low Issues](#low-issues) - [L-1: Centralization Risk for trusted owners](#l-1-centralization-risk-for-trusted-owners) - [L-2: Solmate's SafeTransferLib does not check for token contract's existence](#l-2-solmates-safetransferlib-does-not-check-for-token-contracts-existence) @@ -65,8 +66,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 63 | -| Total nSLOC | 1758 | +| .sol Files | 64 | +| Total nSLOC | 1825 | ## Files Details @@ -97,6 +98,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/IncorrectShift.sol | 17 | | src/InternalFunctions.sol | 22 | | src/KeccakContract.sol | 21 | +| src/MisusedBoolean.sol | 67 | | src/MultipleConstructorSchemes.sol | 10 | | src/OnceModifierExample.sol | 8 | | src/RTLO.sol | 7 | @@ -136,14 +138,14 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/reused_contract_name/ContractB.sol | 7 | | src/uniswap/UniswapV2Swapper.sol | 50 | | src/uniswap/UniswapV3Swapper.sol | 150 | -| **Total** | **1758** | +| **Total** | **1825** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 25 | +| High | 26 | | Low | 23 | @@ -1326,7 +1328,78 @@ Description of the high issue. -## H-20: Sending native Eth is not protected from these functions. +## H-20: Misused boolean with logical operators + +The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively. + +
10 Found Instances + + +- Found in src/MisusedBoolean.sol [Line: 12](../tests/contract-playground/src/MisusedBoolean.sol#L12) + + ```solidity + if (isEven(num) || true) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 19](../tests/contract-playground/src/MisusedBoolean.sol#L19) + + ```solidity + if (isEven(num) && false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 26](../tests/contract-playground/src/MisusedBoolean.sol#L26) + + ```solidity + if (false && isEven(num)) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 33](../tests/contract-playground/src/MisusedBoolean.sol#L33) + + ```solidity + if (true || isEven(num)) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 40](../tests/contract-playground/src/MisusedBoolean.sol#L40) + + ```solidity + if (true) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 47](../tests/contract-playground/src/MisusedBoolean.sol#L47) + + ```solidity + if (false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 54](../tests/contract-playground/src/MisusedBoolean.sol#L54) + + ```solidity + if (!false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 61](../tests/contract-playground/src/MisusedBoolean.sol#L61) + + ```solidity + if (isEven(num) && !false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 68](../tests/contract-playground/src/MisusedBoolean.sol#L68) + + ```solidity + if (isEven(num) && NO) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 75](../tests/contract-playground/src/MisusedBoolean.sol#L75) + + ```solidity + if (isEven(num) && !NO) { + ``` + +
+ + + +## H-21: Sending native Eth is not protected from these functions. Introduce checks for `msg.sender` in the function @@ -1367,7 +1440,7 @@ Introduce checks for `msg.sender` in the function -## H-21: Delegatecall made by the function without checks on any adress. +## H-22: Delegatecall made by the function without checks on any adress. Introduce checks on the address @@ -1396,7 +1469,7 @@ Introduce checks on the address -## H-22: Tautological comparison. +## H-23: Tautological comparison. The left hand side and the right hand side of the binary operation has the same value. This makes the condition always true or always false. @@ -1431,7 +1504,7 @@ The left hand side and the right hand side of the binary operation has the same -## H-23: RTLO character detected in file. \u{202e} +## H-24: RTLO character detected in file. \u{202e} Right to left override character may be misledaing and cause potential attacks by visually misordering method arguments! @@ -1448,7 +1521,7 @@ Right to left override character may be misledaing and cause potential attacks b -## H-24: Return value of the function call is not checked. +## H-25: Return value of the function call is not checked. Function returns a value but it is ignored. @@ -1471,7 +1544,7 @@ Function returns a value but it is ignored. -## H-25: Dangerous unary operator found in assignment. +## H-26: Dangerous unary operator found in assignment. Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in between. diff --git a/reports/report.sarif b/reports/report.sarif index 4cf416255..9ae28c6c8 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -1946,6 +1946,125 @@ }, "ruleId": "state-variable-shadowing" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 19, + "byteOffset": 257 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 419 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 582 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 19, + "byteOffset": 745 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 51, + "byteOffset": 908 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 52, + "byteOffset": 1060 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 53, + "byteOffset": 1213 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 1366 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 17, + "byteOffset": 1530 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 18, + "byteOffset": 1691 + } + } + } + ], + "message": { + "text": "The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively." + }, + "ruleId": "misused-boolean" + }, { "level": "warning", "locations": [ diff --git a/tests/contract-playground/src/MisusedBoolean.sol b/tests/contract-playground/src/MisusedBoolean.sol new file mode 100644 index 000000000..591beee03 --- /dev/null +++ b/tests/contract-playground/src/MisusedBoolean.sol @@ -0,0 +1,82 @@ +pragma solidity 0.4.22; + +contract MisusedBoolean { + + bool public constant NO = false; + + function isEven(uint256 num) internal returns(bool) { + return num % 2 == 0; + } + + function misuse(uint256 num) external returns(uint256) { + if (isEven(num) || true) { + return num * num; + } + return 0; + } + + function misuse2(uint256 num) external returns(uint256) { + if (isEven(num) && false) { + return num * num; + } + return 0; + } + + function misuse3(uint256 num) external returns(uint256) { + if (false && isEven(num)) { + return num * num; + } + return 0; + } + + function misuse4(uint256 num) external returns(uint256) { + if (true || isEven(num)) { + return num * num; + } + return 0; + } + + function misuse5(uint256 num) external pure returns(uint256) { + if (true) { + return num * num; + } + return 0; + } + + function misuse6(uint256 num) external pure returns(uint256) { + if (false) { + return num * num; + } + return 0; + } + + function misuse7(uint256 num) external pure returns(uint256) { + if (!false) { + return num * num; + } + return 0; + } + + function misuse8(uint256 num) external returns(uint256) { + if (isEven(num) && !false) { + return num * num; + } + return 0; + } + + function misuse9(uint256 num) external returns(uint256) { + if (isEven(num) && NO) { + return num * num; + } + return 0; + } + + function misuse10(uint256 num) external returns(uint256) { + if (isEven(num) && !NO) { + return num * num; + } + return 0; + } + + +} \ No newline at end of file