diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 2f764df71..7c55aa3c9 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -74,6 +74,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), Box::::default(), Box::::default(), @@ -147,6 +148,7 @@ pub(crate) enum IssueDetectorNamePool { WeakRandomness, PreDeclaredLocalVariableUsage, DeleteNestedMapping, + BooleanEquality, TxOriginUsedForAuth, MsgValueInLoop, ContractLocksEther, @@ -308,6 +310,7 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::BooleanEquality => Some(Box::::default()), IssueDetectorNamePool::TxOriginUsedForAuth => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/low/boolean_equality.rs b/aderyn_core/src/detect/low/boolean_equality.rs new file mode 100644 index 000000000..e967d933c --- /dev/null +++ b/aderyn_core/src/detect/low/boolean_equality.rs @@ -0,0 +1,50 @@ +use crate::detect::helpers::is_constant_boolean; +use crate::issue_detector; +use eyre::Result; + +issue_detector! { + BooleanEqualityDetector; + + severity: Low, + title: "Boolean equality is not required.", + desc: "If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. Just use `if(x)` and `if(!x)` respectively.", + name: BooleanEquality, + + |context| { + for binary_operation in context.binary_operations() { + if 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); + } + } + } + +} + +#[cfg(test)] +mod boolean_equality_tests { + use serial_test::serial; + + use crate::detect::{detector::IssueDetector, low::boolean_equality::BooleanEqualityDetector}; + + #[test] + #[serial] + fn test_boolean_equality_by_loading_contract_directly() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/BooleanEquality.sol", + ); + + let mut detector = BooleanEqualityDetector::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(), 4); + } +} diff --git a/aderyn_core/src/detect/low/mod.rs b/aderyn_core/src/detect/low/mod.rs index aeef32d20..c0aadcf8a 100644 --- a/aderyn_core/src/detect/low/mod.rs +++ b/aderyn_core/src/detect/low/mod.rs @@ -1,3 +1,4 @@ +pub(crate) mod boolean_equality; pub(crate) mod centralization_risk; pub(crate) mod constants_instead_of_literals; pub(crate) mod contracts_with_todos; @@ -24,6 +25,7 @@ pub(crate) mod useless_modifier; pub(crate) mod useless_public_function; pub(crate) mod zero_address_check; +pub use boolean_equality::BooleanEqualityDetector; pub use centralization_risk::CentralizationRiskDetector; pub use constants_instead_of_literals::ConstantsInsteadOfLiteralsDetector; pub use contracts_with_todos::ContractsWithTodosDetector; diff --git a/reports/report.json b/reports/report.json index 0eb69bb87..b9f9d63f8 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 77, - "total_sloc": 2225 + "total_source_units": 78, + "total_sloc": 2252 }, "files_details": { "files_details": [ @@ -21,6 +21,10 @@ "file_path": "src/AssemblyExample.sol", "n_sloc": 9 }, + { + "file_path": "src/BooleanEquality.sol", + "n_sloc": 27 + }, { "file_path": "src/CallGraphTests.sol", "n_sloc": 49 @@ -317,7 +321,7 @@ }, "issue_count": { "high": 36, - "low": 25 + "low": 26 }, "high_issues": { "issues": [ @@ -2564,6 +2568,42 @@ "description": "If the same constant literal value is used multiple times, create a constant state variable and reference it throughout the contract.", "detector_name": "constants-instead-of-literals", "instances": [ + { + "contract_path": "src/BooleanEquality.sol", + "line_no": 6, + "src": "170:3", + "src_char": "170:3" + }, + { + "contract_path": "src/BooleanEquality.sol", + "line_no": 13, + "src": "330:3", + "src_char": "330:3" + }, + { + "contract_path": "src/BooleanEquality.sol", + "line_no": 15, + "src": "360:3", + "src_char": "360:3" + }, + { + "contract_path": "src/BooleanEquality.sol", + "line_no": 20, + "src": "492:3", + "src_char": "492:3" + }, + { + "contract_path": "src/BooleanEquality.sol", + "line_no": 27, + "src": "653:3", + "src_char": "653:3" + }, + { + "contract_path": "src/BooleanEquality.sol", + "line_no": 29, + "src": "683:3", + "src_char": "683:3" + }, { "contract_path": "src/Casting.sol", "line_no": 16, @@ -4039,6 +4079,37 @@ "src_char": "1175:14" } ] + }, + { + "title": "Boolean equality is not required.", + "description": "If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. Just use `if(x)` and `if(!x)` respectively.", + "detector_name": "boolean-equality", + "instances": [ + { + "contract_path": "src/BooleanEquality.sol", + "line_no": 5, + "src": "133:14", + "src_char": "133:14" + }, + { + "contract_path": "src/BooleanEquality.sol", + "line_no": 12, + "src": "292:15", + "src_char": "292:15" + }, + { + "contract_path": "src/BooleanEquality.sol", + "line_no": 19, + "src": "454:15", + "src_char": "454:15" + }, + { + "contract_path": "src/BooleanEquality.sol", + "line_no": 26, + "src": "614:16", + "src_char": "614:16" + } + ] } ] }, @@ -4101,6 +4172,7 @@ "weak-randomness", "pre-declared-local-variable-usage", "delete-nested-mapping", + "boolean-equality", "tx-origin-used-for-auth", "msg-value-in-loop", "contract-locks-ether" diff --git a/reports/report.md b/reports/report.md index 9ef473456..b856fd2a7 100644 --- a/reports/report.md +++ b/reports/report.md @@ -70,6 +70,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [L-23: Incorrect Order of Division and Multiplication](#l-23-incorrect-order-of-division-and-multiplication) - [L-24: Redundant statements have no effect.](#l-24-redundant-statements-have-no-effect) - [L-25: Public variables of a contract read in an external context (using `this`).](#l-25-public-variables-of-a-contract-read-in-an-external-context-using-this) + - [L-26: Boolean equality is not required.](#l-26-boolean-equality-is-not-required) # Summary @@ -78,8 +79,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 77 | -| Total nSLOC | 2225 | +| .sol Files | 78 | +| Total nSLOC | 2252 | ## Files Details @@ -90,6 +91,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/AdminContract.sol | 11 | | src/ArbitraryTransferFrom.sol | 37 | | src/AssemblyExample.sol | 9 | +| src/BooleanEquality.sol | 27 | | src/CallGraphTests.sol | 49 | | src/Casting.sol | 126 | | src/CompilerBugStorageSignedIntegerArray.sol | 13 | @@ -163,7 +165,7 @@ 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** | **2225** | +| **Total** | **2252** | ## Issue Summary @@ -171,7 +173,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | | High | 36 | -| Low | 25 | +| Low | 26 | # High Issues @@ -2597,9 +2599,45 @@ Instead of marking a function as `public`, consider marking it as `external` if If the same constant literal value is used multiple times, create a constant state variable and reference it throughout the contract. -
38 Found Instances +
44 Found Instances +- Found in src/BooleanEquality.sol [Line: 6](../tests/contract-playground/src/BooleanEquality.sol#L6) + + ```solidity + return 100; + ``` + +- Found in src/BooleanEquality.sol [Line: 13](../tests/contract-playground/src/BooleanEquality.sol#L13) + + ```solidity + return 200; + ``` + +- Found in src/BooleanEquality.sol [Line: 15](../tests/contract-playground/src/BooleanEquality.sol#L15) + + ```solidity + return 130; + ``` + +- Found in src/BooleanEquality.sol [Line: 20](../tests/contract-playground/src/BooleanEquality.sol#L20) + + ```solidity + return 100; + ``` + +- Found in src/BooleanEquality.sol [Line: 27](../tests/contract-playground/src/BooleanEquality.sol#L27) + + ```solidity + return 200; + ``` + +- Found in src/BooleanEquality.sol [Line: 29](../tests/contract-playground/src/BooleanEquality.sol#L29) + + ```solidity + return 130; + ``` + - Found in src/Casting.sol [Line: 16](../tests/contract-playground/src/Casting.sol#L16) ```solidity @@ -4130,3 +4168,38 @@ The contract reads it's own variable using `this` which adds an unnecessary STAT +## L-26: Boolean equality is not required. + +If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. Just use `if(x)` and `if(!x)` respectively. + +
4 Found Instances + + +- Found in src/BooleanEquality.sol [Line: 5](../tests/contract-playground/src/BooleanEquality.sol#L5) + + ```solidity + if (isEven == true) { + ``` + +- Found in src/BooleanEquality.sol [Line: 12](../tests/contract-playground/src/BooleanEquality.sol#L12) + + ```solidity + if (isEven == !true) { + ``` + +- Found in src/BooleanEquality.sol [Line: 19](../tests/contract-playground/src/BooleanEquality.sol#L19) + + ```solidity + if (isEven == false) { + ``` + +- Found in src/BooleanEquality.sol [Line: 26](../tests/contract-playground/src/BooleanEquality.sol#L26) + + ```solidity + if (isEven == !false) { + ``` + +
+ + + diff --git a/reports/report.sarif b/reports/report.sarif index 240ed9ec5..a9df86e6b 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -3934,6 +3934,72 @@ { "level": "note", "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BooleanEquality.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 170 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BooleanEquality.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 330 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BooleanEquality.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 360 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BooleanEquality.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 492 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BooleanEquality.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 653 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BooleanEquality.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 683 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -6580,6 +6646,59 @@ "text": "The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage." }, "ruleId": "public-variable-read-in-external-context" + }, + { + "level": "note", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BooleanEquality.sol" + }, + "region": { + "byteLength": 14, + "byteOffset": 133 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BooleanEquality.sol" + }, + "region": { + "byteLength": 15, + "byteOffset": 292 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BooleanEquality.sol" + }, + "region": { + "byteLength": 15, + "byteOffset": 454 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BooleanEquality.sol" + }, + "region": { + "byteLength": 16, + "byteOffset": 614 + } + } + } + ], + "message": { + "text": "If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. Just use `if(x)` and `if(!x)` respectively." + }, + "ruleId": "boolean-equality" } ], "tool": { diff --git a/reports/templegold-report.md b/reports/templegold-report.md index 278751712..bb3f1c2ac 100644 --- a/reports/templegold-report.md +++ b/reports/templegold-report.md @@ -38,6 +38,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [L-17: Loop contains `require`/`revert` statements](#l-17-loop-contains-requirerevert-statements) - [L-18: Incorrect Order of Division and Multiplication](#l-18-incorrect-order-of-division-and-multiplication) - [L-19: Redundant statements have no effect.](#l-19-redundant-statements-have-no-effect) + - [L-20: Boolean equality is not required.](#l-20-boolean-equality-is-not-required) # Summary @@ -191,7 +192,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | | High | 10 | -| Low | 19 | +| Low | 20 | # High Issues @@ -8653,3 +8654,26 @@ Remove the redundant statements because no code will be generated and it just co +## L-20: Boolean equality is not required. + +If `x` is a boolean, there is no need to do `if(x == true)` or `if(x == false)`. Just use `if(x)` and `if(!x)` respectively. + +
2 Found Instances + + +- Found in contracts/deprecated/Faith.sol [Line: 32](../tests/2024-07-templegold/protocol/contracts/deprecated/Faith.sol#L32) + + ```solidity + require(canManageFaith[msg.sender] == true, "Faith: caller cannot manage faith"); + ``` + +- Found in contracts/deprecated/Faith.sol [Line: 40](../tests/2024-07-templegold/protocol/contracts/deprecated/Faith.sol#L40) + + ```solidity + require(canManageFaith[msg.sender] == true, "Faith: caller cannot manage faith"); + ``` + +
+ + + diff --git a/tests/contract-playground/src/BooleanEquality.sol b/tests/contract-playground/src/BooleanEquality.sol new file mode 100644 index 000000000..c8e8bd646 --- /dev/null +++ b/tests/contract-playground/src/BooleanEquality.sol @@ -0,0 +1,31 @@ +pragma solidity 0.4.22; + +contract BooleanEquality { + function badCheck(bool isEven) external pure returns (uint256) { + if (isEven == true) { + return 100; + } + return 0; + } + + function badCheck2(bool isEven) external pure returns (uint256) { + if (isEven == !true) { + return 200; + } + return 130; + } + + function badCheck3(bool isEven) external pure returns (uint256) { + if (isEven == false) { + return 100; + } + return 0; + } + + function badCheck4(bool isEven) external pure returns (uint256) { + if (isEven == !false) { + return 200; + } + return 130; + } +}