diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 7c55aa3c9..74305116a 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(), @@ -148,6 +149,7 @@ pub(crate) enum IssueDetectorNamePool { WeakRandomness, PreDeclaredLocalVariableUsage, DeleteNestedMapping, + ConstantFunctionsAssembly, BooleanEquality, TxOriginUsedForAuth, MsgValueInLoop, @@ -310,6 +312,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::ConstantFunctionsAssembly => { + Some(Box::::default()) + } IssueDetectorNamePool::BooleanEquality => Some(Box::::default()), IssueDetectorNamePool::TxOriginUsedForAuth => { Some(Box::::default()) diff --git a/aderyn_core/src/detect/low/constant_funcs_assembly.rs b/aderyn_core/src/detect/low/constant_funcs_assembly.rs new file mode 100644 index 000000000..59b1f4c18 --- /dev/null +++ b/aderyn_core/src/detect/low/constant_funcs_assembly.rs @@ -0,0 +1,166 @@ +use std::collections::BTreeMap; +use std::error::Error; +use std::str::FromStr; + +use crate::ast::{ASTNode, NodeID, NodeType, StateMutability}; + +use crate::capture; +use crate::context::browser::{ + ExtractInlineAssemblys, ExtractPragmaDirectives, GetClosestAncestorOfTypeX, +}; +use crate::context::investigator::{ + StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, +}; +use crate::detect::detector::IssueDetectorNamePool; +use crate::detect::helpers::{self, pragma_directive_to_semver}; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; +use semver::{Version, VersionReq}; + +#[derive(Default)] +pub struct ConstantFunctionContainsAssemblyDetector { + // 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 ConstantFunctionContainsAssemblyDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for function in helpers::get_implemented_external_and_public_functions(context) { + // First, check the eligibility for this function by checking + if let Some(ASTNode::SourceUnit(source_unit)) = + function.closest_ancestor_of_type(context, NodeType::SourceUnit) + { + // Store the extracted directives in a variable to extend its lifetime + let extracted_directives = ExtractPragmaDirectives::from(source_unit).extracted; + let pragma_directive = extracted_directives.first(); + + if let Some(pragma_directive) = pragma_directive { + let version_req = pragma_directive_to_semver(pragma_directive); + if let Ok(version_req) = version_req { + if version_req_allows_below_0_5_0(&version_req) { + // Only run the logic if pragma is allowed to run on solc <0.5.0 + + if function.state_mutability() == &StateMutability::View + || function.state_mutability() == &StateMutability::Pure + { + let mut tracker = AssemblyTracker { + has_assembly: false, + }; + let investigator = StandardInvestigator::new( + context, + &[&(function.into())], + StandardInvestigationStyle::Downstream, + )?; + investigator.investigate(context, &mut tracker)?; + + if tracker.has_assembly { + capture!(self, context, function); + } + } + } + } + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::Low + } + + fn title(&self) -> String { + String::from("Functions declared `pure` / `view` but contains assembly") + } + + fn description(&self) -> String { + String::from("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.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + format!("{}", IssueDetectorNamePool::ConstantFunctionsAssembly) + } +} + +fn version_req_allows_below_0_5_0(version_req: &VersionReq) -> bool { + // If it matches any 0.4.0 to 0.4.26, return true + for i in 0..=26 { + let version: semver::Version = Version::from_str(&format!("0.4.{}", i)).unwrap(); + if version_req.matches(&version) { + return true; + } + } + + // Else, return false + false +} + +struct AssemblyTracker { + has_assembly: bool, +} + +impl StandardInvestigatorVisitor for AssemblyTracker { + fn visit_any(&mut self, node: &crate::ast::ASTNode) -> eyre::Result<()> { + // If we are already satisifed, do not bother checking + if self.has_assembly { + return Ok(()); + } + + if let ASTNode::FunctionDefinition(function) = node { + // Ignore checking functions that start with `_` + // Example - templegold contains math functions like `_rpow()`, etc that are used by view functions + // That should be okay .. I guess? (idk ... it's open for dicussion) + if function.name.starts_with("_") { + return Ok(()); + } + } + + // Check if this node has assembly code + let assemblies = ExtractInlineAssemblys::from(node).extracted; + if !assemblies.is_empty() { + self.has_assembly = true; + } + Ok(()) + } +} + +#[cfg(test)] +mod constant_functions_assembly_detector { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, + low::constant_funcs_assembly::ConstantFunctionContainsAssemblyDetector, + }; + + #[test] + #[serial] + fn test_constant_functions_assembly() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/ConstantFuncsAssembly.sol", + ); + + let mut detector = ConstantFunctionContainsAssemblyDetector::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(), 3); + // assert the severity is low + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::Low + ); + } +} diff --git a/aderyn_core/src/detect/low/mod.rs b/aderyn_core/src/detect/low/mod.rs index c0aadcf8a..7d7663e41 100644 --- a/aderyn_core/src/detect/low/mod.rs +++ b/aderyn_core/src/detect/low/mod.rs @@ -1,5 +1,6 @@ pub(crate) mod boolean_equality; pub(crate) mod centralization_risk; +pub(crate) mod constant_funcs_assembly; pub(crate) mod constants_instead_of_literals; pub(crate) mod contracts_with_todos; pub(crate) mod deprecated_oz_functions; @@ -27,6 +28,7 @@ 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; pub use contracts_with_todos::ContractsWithTodosDetector; pub use deprecated_oz_functions::DeprecatedOZFunctionsDetector; diff --git a/cli/reportgen.sh b/cli/reportgen.sh index 28e7deca1..90afc66f2 100755 --- a/cli/reportgen.sh +++ b/cli/reportgen.sh @@ -2,10 +2,10 @@ #### MARKDOWN REPORTS ###### -# Basic report.md +# Basic report.md cargo run -- -i src/ -x lib/ ./tests/contract-playground -o ./reports/report.md --skip-update-check & -# Adhoc sol files report.md +# Adhoc sol files report.md cargo run -- ./tests/adhoc-sol-files -o ./reports/adhoc-sol-files-report.md --skip-update-check & # Aderyn.toml with nested root @@ -41,4 +41,4 @@ cargo run -- ./tests/adhoc-sol-files -o ./reports/adhoc-sol-files-highs-only-re # Basic report.sarif cargo run -- ./tests/contract-playground -o ./reports/report.sarif --skip-update-check & -wait \ No newline at end of file +wait diff --git a/reports/report.json b/reports/report.json index b9f9d63f8..f2eda5e64 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 78, - "total_sloc": 2252 + "total_source_units": 79, + "total_sloc": 2278 }, "files_details": { "files_details": [ @@ -37,6 +37,10 @@ "file_path": "src/CompilerBugStorageSignedIntegerArray.sol", "n_sloc": 13 }, + { + "file_path": "src/ConstantFuncsAssembly.sol", + "n_sloc": 26 + }, { "file_path": "src/ConstantsLiterals.sol", "n_sloc": 28 @@ -321,7 +325,7 @@ }, "issue_count": { "high": 36, - "low": 26 + "low": 27 }, "high_issues": { "issues": [ @@ -1287,6 +1291,12 @@ "src": "97:1", "src_char": "97:1" }, + { + "contract_path": "src/ConstantFuncsAssembly.sol", + "line_no": 6, + "src": "110:20", + "src_char": "110:20" + }, { "contract_path": "src/DelegateCallWithoutAddressCheck.sol", "line_no": 9, @@ -2205,6 +2215,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/ConstantFuncsAssembly.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/ContractLocksEther.sol", "line_no": 2, @@ -3711,6 +3727,12 @@ "src": "1206:18", "src_char": "1206:18" }, + { + "contract_path": "src/ConstantFuncsAssembly.sol", + "line_no": 26, + "src": "651:232", + "src_char": "651:232" + }, { "contract_path": "src/InternalFunctions.sol", "line_no": 28, @@ -4080,6 +4102,31 @@ } ] }, + { + "title": "Functions declared `pure` / `view` but contains assembly", + "description": "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.", + "detector_name": "constant-functions-assembly", + "instances": [ + { + "contract_path": "src/ConstantFuncsAssembly.sol", + "line_no": 9, + "src": "182:175", + "src_char": "182:175" + }, + { + "contract_path": "src/ConstantFuncsAssembly.sol", + "line_no": 17, + "src": "408:237", + "src_char": "408:237" + }, + { + "contract_path": "src/ConstantFuncsAssembly.sol", + "line_no": 36, + "src": "934:98", + "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.", @@ -4172,6 +4219,7 @@ "weak-randomness", "pre-declared-local-variable-usage", "delete-nested-mapping", + "constant-functions-assembly", "boolean-equality", "tx-origin-used-for-auth", "msg-value-in-loop", diff --git a/reports/report.md b/reports/report.md index b856fd2a7..bf625ccd5 100644 --- a/reports/report.md +++ b/reports/report.md @@ -70,7 +70,8 @@ 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) + - [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 | 2252 | +| .sol Files | 79 | +| Total nSLOC | 2278 | ## Files Details @@ -95,6 +96,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/CallGraphTests.sol | 49 | | src/Casting.sol | 126 | | src/CompilerBugStorageSignedIntegerArray.sol | 13 | +| src/ConstantFuncsAssembly.sol | 26 | | src/ConstantsLiterals.sol | 28 | | src/ContractLocksEther.sol | 121 | | src/ContractWithTodo.sol | 7 | @@ -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** | **2252** | +| **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 @@ -1195,7 +1197,7 @@ If the length of a dynamic array (storage variable) directly assigned to, it may Solidity does initialize variables by default when you declare them, however it's good practice to explicitly declare an initial value. For example, if you transfer money to an address we must make sure that the address has been initialized. -
16 Found Instances +
17 Found Instances - Found in src/AssemblyExample.sol [Line: 5](../tests/contract-playground/src/AssemblyExample.sol#L5) @@ -1204,6 +1206,12 @@ Solidity does initialize variables by default when you declare them, however it' uint b; ``` +- Found in src/ConstantFuncsAssembly.sol [Line: 6](../tests/contract-playground/src/ConstantFuncsAssembly.sol#L6) + + ```solidity + uint256 public value; + ``` + - Found in src/DelegateCallWithoutAddressCheck.sol [Line: 9](../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol#L9) ```solidity @@ -2218,7 +2226,7 @@ ERC20 functions may not behave as expected. For example: return values are not a Consider using a specific version of Solidity in your contracts instead of a wide version. For example, instead of `pragma solidity ^0.8.0;`, use `pragma solidity 0.8.0;` -
23 Found Instances +
24 Found Instances - Found in src/CompilerBugStorageSignedIntegerArray.sol [Line: 2](../tests/contract-playground/src/CompilerBugStorageSignedIntegerArray.sol#L2) @@ -2227,6 +2235,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity ^0.4.0; ``` +- Found in src/ConstantFuncsAssembly.sol [Line: 2](../tests/contract-playground/src/ConstantFuncsAssembly.sol#L2) + + ```solidity + pragma solidity ^0.4.0; + ``` + - Found in src/ContractLocksEther.sol [Line: 2](../tests/contract-playground/src/ContractLocksEther.sol#L2) ```solidity @@ -3742,7 +3756,7 @@ Use `e` notation, for example: `1e18`, instead of its full numeric value. Instead of separating the logic into a separate function, consider inlining the logic into the calling function. This can reduce the number of function calls and improve readability. -
15 Found Instances +
16 Found Instances - Found in src/CallGraphTests.sol [Line: 6](../tests/contract-playground/src/CallGraphTests.sol#L6) @@ -3769,6 +3783,12 @@ Instead of separating the logic into a separate function, consider inlining the function visitSeventhFloor3() internal { ``` +- Found in src/ConstantFuncsAssembly.sol [Line: 26](../tests/contract-playground/src/ConstantFuncsAssembly.sol#L26) + + ```solidity + function useAssembly() internal pure returns (uint256) { + ``` + - Found in src/InternalFunctions.sol [Line: 28](../tests/contract-playground/src/InternalFunctions.sol#L28) ```solidity @@ -4168,7 +4188,36 @@ The contract reads it's own variable using `this` which adds an unnecessary STAT -## L-26: Boolean equality is not required. +## L-26: Functions declared `pure` / `view` but contains assembly + +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. + +
3 Found Instances + + +- Found in src/ConstantFuncsAssembly.sol [Line: 9](../tests/contract-playground/src/ConstantFuncsAssembly.sol#L9) + + ```solidity + function setValue(uint256 _value) external view { + ``` + +- Found in src/ConstantFuncsAssembly.sol [Line: 17](../tests/contract-playground/src/ConstantFuncsAssembly.sol#L17) + + ```solidity + function getConstantValue() external pure returns (uint256) { + ``` + +- Found in src/ConstantFuncsAssembly.sol [Line: 36](../tests/contract-playground/src/ConstantFuncsAssembly.sol#L36) + + ```solidity + function getConstantValue2() external pure returns (uint256) { + ``` + +
+ + + +## 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. diff --git a/reports/report.sarif b/reports/report.sarif index a9df86e6b..53ddef2c4 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -1704,6 +1704,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ConstantFuncsAssembly.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 110 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3280,6 +3291,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ConstantFuncsAssembly.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -5995,6 +6017,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ConstantFuncsAssembly.sol" + }, + "region": { + "byteLength": 232, + "byteOffset": 651 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -6647,6 +6680,48 @@ }, "ruleId": "public-variable-read-in-external-context" }, + { + "level": "note", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ConstantFuncsAssembly.sol" + }, + "region": { + "byteLength": 175, + "byteOffset": 182 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ConstantFuncsAssembly.sol" + }, + "region": { + "byteLength": 237, + "byteOffset": 408 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ConstantFuncsAssembly.sol" + }, + "region": { + "byteLength": 98, + "byteOffset": 934 + } + } + } + ], + "message": { + "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": [ diff --git a/tests/contract-playground/src/ConstantFuncsAssembly.sol b/tests/contract-playground/src/ConstantFuncsAssembly.sol new file mode 100644 index 000000000..f18a50b39 --- /dev/null +++ b/tests/contract-playground/src/ConstantFuncsAssembly.sol @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.4.0; + +contract AssemblyExample { + // State variable + uint256 public value; + + // BAD (view function contains assembly) + function setValue(uint256 _value) external view { + assembly { + // Load the location of the 'value' storage slot + sstore(0, _value) + } + } + + // BAD (pure function contains assembly) + function getConstantValue() external pure returns (uint256) { + uint256 result; + assembly { + // Inline assembly to set the result to a constant value + result := 42 + } + return result; + } + + function useAssembly() internal pure returns (uint256) { + uint256 result; + assembly { + // Inline assembly to set the result to a constant value + result := 42 + } + return result; + } + + // BAD (pure function contains assembly) + function getConstantValue2() external pure returns (uint256) { + return useAssembly(); + } +}