diff --git a/aderyn/README.md b/aderyn/README.md index 2894e358a..43a3c4e9b 100644 --- a/aderyn/README.md +++ b/aderyn/README.md @@ -1,4 +1,9 @@ +> ⚠️ **Installing via crates is no longer fully supported. `cyfrinup` is the preferred installation method.**. +> +> For the best experience, please remove the legacy crate installation by running `cargo uninstall aderyn`, and use `cyfrinup` instead. +> +> Full install instructions are [here](#installation).


diff --git a/aderyn/src/lib.rs b/aderyn/src/lib.rs index 44d3fdc19..8d9f4b45b 100644 --- a/aderyn/src/lib.rs +++ b/aderyn/src/lib.rs @@ -114,15 +114,12 @@ pub fn aderyn_is_currently_running_newest_version() -> Result()?; - - let newest_version = data["crates"][0]["newest_version"].to_string(); - let newest_version = &newest_version[1..newest_version.len() - 1]; - - let newest = Version::parse(newest_version).unwrap(); + let newest = + Version::parse(data["tag_name"].as_str().unwrap().replace("v", "").as_str()).unwrap(); let current = Version::parse(env!("CARGO_PKG_VERSION")).unwrap(); Ok(current >= newest) diff --git a/aderyn/src/main.rs b/aderyn/src/main.rs index 4b808a120..48927d716 100644 --- a/aderyn/src/main.rs +++ b/aderyn/src/main.rs @@ -151,9 +151,7 @@ fn main() { if let Ok(yes) = aderyn_is_currently_running_newest_version() { if !yes { println!(); - println!( - "NEW VERSION OF ADERYN AVAILABLE! Please run `cargo install aderyn` to fully upgrade the current version" - ); + println!("NEW VERSION OF ADERYN AVAILABLE! Please run `cyfrinup` to upgrade."); } } } diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index bb5b14d9f..74305116a 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -75,6 +75,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), Box::::default(), Box::::default(), @@ -149,6 +150,7 @@ pub(crate) enum IssueDetectorNamePool { PreDeclaredLocalVariableUsage, DeleteNestedMapping, ConstantFunctionsAssembly, + BooleanEquality, TxOriginUsedForAuth, MsgValueInLoop, ContractLocksEther, @@ -313,6 +315,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 4f3006167..7d7663e41 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 constant_funcs_assembly; pub(crate) mod constants_instead_of_literals; @@ -25,6 +26,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 constant_funcs_assembly::ConstantFunctionContainsAssemblyDetector; pub use constants_instead_of_literals::ConstantsInsteadOfLiteralsDetector; diff --git a/reports/report.json b/reports/report.json index ca144e501..f2eda5e64 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 78, - "total_sloc": 2251 + "total_source_units": 79, + "total_sloc": 2278 }, "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 @@ -321,7 +325,7 @@ }, "issue_count": { "high": 36, - "low": 26 + "low": 27 }, "high_issues": { "issues": [ @@ -2580,6 +2584,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, @@ -4086,6 +4126,37 @@ "src_char": "934:98" } ] + }, + { + "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" + } + ] } ] }, @@ -4149,6 +4220,7 @@ "pre-declared-local-variable-usage", "delete-nested-mapping", "constant-functions-assembly", + "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 92aab2951..bf625ccd5 100644 --- a/reports/report.md +++ b/reports/report.md @@ -71,6 +71,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [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: Functions declared `pure` / `view` but contains assembly](#l-26-functions-declared-pure--view-but-contains-assembly) + - [L-27: Boolean equality is not required.](#l-27-boolean-equality-is-not-required) # Summary @@ -79,8 +80,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 78 | -| Total nSLOC | 2251 | +| .sol Files | 79 | +| Total nSLOC | 2278 | ## Files Details @@ -91,6 +92,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 | @@ -165,7 +167,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** | **2251** | +| **Total** | **2278** | ## Issue Summary @@ -173,7 +175,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | | High | 36 | -| Low | 26 | +| Low | 27 | # High Issues @@ -2611,9 +2613,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 @@ -4179,3 +4217,38 @@ If the assembly code contains bugs or unintended side effects, it can lead to in +## L-27: 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 5e700c745..53ddef2c4 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -3956,6 +3956,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": { @@ -6655,6 +6721,59 @@ "text": "If the assembly code contains bugs or unintended side effects, it can lead to incorrect results or vulnerabilities, which are hard to debug and resolve, especially when the function is meant to be simple and predictable." }, "ruleId": "constant-functions-assembly" + }, + { + "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; + } +}