diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index dfdf55f6..1302f49f 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -83,6 +83,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), ] } @@ -94,6 +95,7 @@ pub fn get_all_detectors_names() -> Vec { #[derive(Debug, PartialEq, EnumString, Display)] #[strum(serialize_all = "kebab-case")] pub(crate) enum IssueDetectorNamePool { + FunctionInitializingState, DelegateCallInLoop, CentralizationRisk, SolmateSafeTransferLib, @@ -170,6 +172,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { + Some(Box::::default()) + } IssueDetectorNamePool::IncorrectERC20Interface => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/low/function_init_state_vars.rs b/aderyn_core/src/detect/low/function_init_state_vars.rs new file mode 100644 index 00000000..67fc8718 --- /dev/null +++ b/aderyn_core/src/detect/low/function_init_state_vars.rs @@ -0,0 +1,155 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::{ASTNode, Expression, FunctionCall, Identifier, NodeID}; + +use crate::capture; +use crate::context::browser::ExtractReferencedDeclarations; +use crate::context::investigator::{ + StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, +}; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct FunctionInitializingStateDetector { + // 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 FunctionInitializingStateDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + // PLAN + // Capture state variables that are initialized directly by calling a non constant function. + // Go throough state variable declarations with initial value (this will be true when value is set outside constructor) + // See if the function references non-constant state variables. If it does, then capture it + + for variable_declaration in context + .variable_declarations() + .into_iter() + .filter(|v| v.state_variable) + { + if let Some(Expression::FunctionCall(FunctionCall { expression, .. })) = + variable_declaration.value.as_ref() + { + if let Expression::Identifier(Identifier { + referenced_declaration: Some(func_id), + .. + }) = expression.as_ref() + { + if let Some(ASTNode::FunctionDefinition(func)) = context.nodes.get(func_id) { + let mut tracker = + NonConstantStateVariableReferenceDeclarationTracker::new(context); + + let investigator = StandardInvestigator::new( + context, + &[&(func.into())], + StandardInvestigationStyle::Downstream, + )?; + + investigator.investigate(context, &mut tracker)?; + + if tracker.makes_a_reference { + capture!(self, context, variable_declaration); + } + } + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::Low + } + + fn title(&self) -> String { + String::from("Function initializing state.") + } + + fn description(&self) -> String { + String::from("Detects the immediate initialization of state variables through function calls that are not pure/constant, or that use \ + non-constant state variable. Remove any initialization of state variables via non-constant state variables or function calls. If variables \ + must be set upon contract deployment, locate initialization in the constructor instead.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + format!("{}", IssueDetectorNamePool::FunctionInitializingState) + } +} + +struct NonConstantStateVariableReferenceDeclarationTracker<'a> { + makes_a_reference: bool, + context: &'a WorkspaceContext, +} + +impl<'a> NonConstantStateVariableReferenceDeclarationTracker<'a> { + fn new(context: &'a WorkspaceContext) -> Self { + Self { + makes_a_reference: false, + context, + } + } +} + +impl<'a> StandardInvestigatorVisitor for NonConstantStateVariableReferenceDeclarationTracker<'a> { + fn visit_any(&mut self, node: &ASTNode) -> eyre::Result<()> { + // We already know the condition is satisifed + if self.makes_a_reference { + return Ok(()); + } + + let references = ExtractReferencedDeclarations::from(node).extracted; + + for reference in references { + if let Some(ASTNode::VariableDeclaration(variable_declaration)) = + self.context.nodes.get(&reference) + { + if variable_declaration.state_variable && !variable_declaration.constant { + self.makes_a_reference = true; + } + } + } + + Ok(()) + } +} + +#[cfg(test)] +mod function_initializing_state_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, low::function_init_state_vars::FunctionInitializingStateDetector, + }; + + #[test] + #[serial] + fn test_function_initializing_state() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/FunctionInitializingState.sol", + ); + + let mut detector = FunctionInitializingStateDetector::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 bebd9f34..3ca3bdbd 100644 --- a/aderyn_core/src/detect/low/mod.rs +++ b/aderyn_core/src/detect/low/mod.rs @@ -7,6 +7,7 @@ pub(crate) mod deprecated_oz_functions; pub(crate) mod division_before_multiplication; pub(crate) mod ecrecover; pub(crate) mod empty_blocks; +pub(crate) mod function_init_state_vars; pub(crate) mod inconsistent_type_names; pub(crate) mod large_literal_value; pub(crate) mod non_reentrant_before_others; @@ -37,6 +38,7 @@ pub use deprecated_oz_functions::DeprecatedOZFunctionsDetector; pub use division_before_multiplication::DivisionBeforeMultiplicationDetector; pub use ecrecover::EcrecoverDetector; pub use empty_blocks::EmptyBlockDetector; +pub use function_init_state_vars::FunctionInitializingStateDetector; pub use inconsistent_type_names::InconsistentTypeNamesDetector; pub use large_literal_value::LargeLiteralValueDetector; pub use non_reentrant_before_others::NonReentrantBeforeOthersDetector; diff --git a/reports/report.json b/reports/report.json index fa835389..440b799e 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 82, - "total_sloc": 2431 + "total_source_units": 83, + "total_sloc": 2480 }, "files_details": { "files_details": [ @@ -105,6 +105,10 @@ "file_path": "src/ExperimentalEncoder.sol", "n_sloc": 4 }, + { + "file_path": "src/FunctionInitializingState.sol", + "n_sloc": 49 + }, { "file_path": "src/HugeConstants.sol", "n_sloc": 36 @@ -337,7 +341,7 @@ }, "issue_count": { "high": 37, - "low": 29 + "low": 30 }, "high_issues": { "issues": [ @@ -2360,6 +2364,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/FunctionInitializingState.sol", + "line_no": 2, + "src": "32:21", + "src_char": "32:21" + }, { "contract_path": "src/InconsistentUints.sol", "line_no": 1, @@ -2903,6 +2913,54 @@ "src": "457:3", "src_char": "457:3" }, + { + "contract_path": "src/FunctionInitializingState.sol", + "line_no": 23, + "src": "790:2", + "src_char": "790:2" + }, + { + "contract_path": "src/FunctionInitializingState.sol", + "line_no": 26, + "src": "820:2", + "src_char": "820:2" + }, + { + "contract_path": "src/FunctionInitializingState.sol", + "line_no": 34, + "src": "1109:2", + "src_char": "1109:2" + }, + { + "contract_path": "src/FunctionInitializingState.sol", + "line_no": 37, + "src": "1139:2", + "src_char": "1139:2" + }, + { + "contract_path": "src/FunctionInitializingState.sol", + "line_no": 42, + "src": "1268:4", + "src_char": "1268:4" + }, + { + "contract_path": "src/FunctionInitializingState.sol", + "line_no": 42, + "src": "1275:4", + "src_char": "1275:4" + }, + { + "contract_path": "src/FunctionInitializingState.sol", + "line_no": 48, + "src": "1459:4", + "src_char": "1459:4" + }, + { + "contract_path": "src/FunctionInitializingState.sol", + "line_no": 48, + "src": "1466:4", + "src_char": "1466:4" + }, { "contract_path": "src/IncorrectCaretOperator.sol", "line_no": 17, @@ -4514,6 +4572,31 @@ "src_char": "591:115" } ] + }, + { + "title": "Function initializing state.", + "description": "Detects the immediate initialization of state variables through function calls that are not pure/constant, or that use non-constant state variable. Remove any initialization of state variables via non-constant state variables or function calls. If variables must be set upon contract deployment, locate initialization in the constructor instead.", + "detector_name": "function-initializing-state", + "instances": [ + { + "contract_path": "src/FunctionInitializingState.sol", + "line_no": 6, + "src": "108:21", + "src_char": "108:21" + }, + { + "contract_path": "src/FunctionInitializingState.sol", + "line_no": 8, + "src": "199:21", + "src_char": "199:21" + }, + { + "contract_path": "src/FunctionInitializingState.sol", + "line_no": 9, + "src": "267:21", + "src_char": "267:21" + } + ] } ] }, @@ -4583,6 +4666,7 @@ "msg-value-in-loop", "contract-locks-ether", "incorrect-erc20-interface", - "return-bomb" + "return-bomb", + "function-initializing-state" ] } \ No newline at end of file diff --git a/reports/report.md b/reports/report.md index c03f1741..b76291bb 100644 --- a/reports/report.md +++ b/reports/report.md @@ -75,6 +75,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [L-27: Functions declared `pure` / `view` but contains assembly](#l-27-functions-declared-pure--view-but-contains-assembly) - [L-28: Boolean equality is not required.](#l-28-boolean-equality-is-not-required) - [L-29: Return Bomb](#l-29-return-bomb) + - [L-30: Function initializing state.](#l-30-function-initializing-state) # Summary @@ -83,8 +84,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 82 | -| Total nSLOC | 2431 | +| .sol Files | 83 | +| Total nSLOC | 2480 | ## Files Details @@ -116,6 +117,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/EmptyBlocks.sol | 48 | | src/EnumerableSetIteration.sol | 55 | | src/ExperimentalEncoder.sol | 4 | +| src/FunctionInitializingState.sol | 49 | | src/HugeConstants.sol | 36 | | src/InconsistentUints.sol | 17 | | src/IncorrectCaretOperator.sol | 16 | @@ -173,7 +175,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** | **2431** | +| **Total** | **2480** | ## Issue Summary @@ -181,7 +183,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | | High | 37 | -| Low | 29 | +| Low | 30 | # High Issues @@ -2315,7 +2317,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;` -
26 Found Instances +
27 Found Instances - Found in src/CompilerBugStorageSignedIntegerArray.sol [Line: 2](../tests/contract-playground/src/CompilerBugStorageSignedIntegerArray.sol#L2) @@ -2378,6 +2380,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity ^0.8.0; ``` +- Found in src/FunctionInitializingState.sol [Line: 2](../tests/contract-playground/src/FunctionInitializingState.sol#L2) + + ```solidity + pragma solidity ^0.4; + ``` + - Found in src/InconsistentUints.sol [Line: 1](../tests/contract-playground/src/InconsistentUints.sol#L1) ```solidity @@ -2768,7 +2776,7 @@ 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. -
44 Found Instances +
52 Found Instances - Found in src/BooleanEquality.sol [Line: 6](../tests/contract-playground/src/BooleanEquality.sol#L6) @@ -2927,6 +2935,42 @@ If the same constant literal value is used multiple times, create a constant sta myArray.length += 200; ``` +- Found in src/FunctionInitializingState.sol [Line: 23](../tests/contract-playground/src/FunctionInitializingState.sol#L23) + + ```solidity + return 77; + ``` + +- Found in src/FunctionInitializingState.sol [Line: 26](../tests/contract-playground/src/FunctionInitializingState.sol#L26) + + ```solidity + return 88; + ``` + +- Found in src/FunctionInitializingState.sol [Line: 34](../tests/contract-playground/src/FunctionInitializingState.sol#L34) + + ```solidity + return 77; + ``` + +- Found in src/FunctionInitializingState.sol [Line: 37](../tests/contract-playground/src/FunctionInitializingState.sol#L37) + + ```solidity + return 88; + ``` + +- Found in src/FunctionInitializingState.sol [Line: 42](../tests/contract-playground/src/FunctionInitializingState.sol#L42) + + ```solidity + f = 1e10 * 1e12; + ``` + +- Found in src/FunctionInitializingState.sol [Line: 48](../tests/contract-playground/src/FunctionInitializingState.sol#L48) + + ```solidity + f = 1e10 * 1e12; + ``` + - Found in src/IncorrectCaretOperator.sol [Line: 17](../tests/contract-playground/src/IncorrectCaretOperator.sol#L17) ```solidity @@ -4616,3 +4660,32 @@ A low level callee may consume all callers gas unexpectedly. Avoid unlimited imp +## L-30: Function initializing state. + +Detects the immediate initialization of state variables through function calls that are not pure/constant, or that use non-constant state variable. Remove any initialization of state variables via non-constant state variables or function calls. If variables must be set upon contract deployment, locate initialization in the constructor instead. + +
3 Found Instances + + +- Found in src/FunctionInitializingState.sol [Line: 6](../tests/contract-playground/src/FunctionInitializingState.sol#L6) + + ```solidity + uint public v = set(); // Initialize from function (sets to 77) + ``` + +- Found in src/FunctionInitializingState.sol [Line: 8](../tests/contract-playground/src/FunctionInitializingState.sol#L8) + + ```solidity + uint public x = set(); // Initialize from function (sets to 88) + ``` + +- Found in src/FunctionInitializingState.sol [Line: 9](../tests/contract-playground/src/FunctionInitializingState.sol#L9) + + ```solidity + uint public f = tes(); + ``` + +
+ + + diff --git a/reports/report.sarif b/reports/report.sarif index 423fe532..b4167d74 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -3531,6 +3531,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionInitializingState.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -4515,6 +4526,94 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionInitializingState.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 790 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionInitializingState.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 820 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionInitializingState.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 1109 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionInitializingState.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 1139 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionInitializingState.sol" + }, + "region": { + "byteLength": 4, + "byteOffset": 1268 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionInitializingState.sol" + }, + "region": { + "byteLength": 4, + "byteOffset": 1275 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionInitializingState.sol" + }, + "region": { + "byteLength": 4, + "byteOffset": 1459 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionInitializingState.sol" + }, + "region": { + "byteLength": 4, + "byteOffset": 1466 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -7395,6 +7494,48 @@ "text": "A low level callee may consume all callers gas unexpectedly. Avoid unlimited implicit decoding of returndata on calls to unchecked addresses. You can limit the gas by passing a gas limit as an option to the call. For example, `unknownAdress.call{gas: gasLimitHere}(\"calldata\")` That would act as a safety net from OOG errors.\n " }, "ruleId": "return-bomb" + }, + { + "level": "note", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionInitializingState.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 108 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionInitializingState.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 199 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/FunctionInitializingState.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 267 + } + } + } + ], + "message": { + "text": "Detects the immediate initialization of state variables through function calls that are not pure/constant, or that use non-constant state variable. Remove any initialization of state variables via non-constant state variables or function calls. If variables must be set upon contract deployment, locate initialization in the constructor instead." + }, + "ruleId": "function-initializing-state" } ], "tool": { diff --git a/tests/contract-playground/src/FunctionInitializingState.sol b/tests/contract-playground/src/FunctionInitializingState.sol new file mode 100644 index 00000000..a9b24fd1 --- /dev/null +++ b/tests/contract-playground/src/FunctionInitializingState.sol @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.4; + +// WRONG WAY +contract StateVarInitFromFunction { + uint public v = set(); // Initialize from function (sets to 77) + uint public w = 5; + uint public x = set(); // Initialize from function (sets to 88) + uint public f = tes(); + uint public h = okay(); + uint public m = okay2(); + address public constant shouldntBeReported = address(8); + + constructor() { + // The constructor is run after all state variables are initialized. + } + + // BAD (not marked as view) + function set() public returns (uint) { + // If this function is being used to initialize a state variable declared + // before w, w will be zero. If it is declared after w, w will be set. + if (w == 0) { + return 77; + } + + return 88; + } + + // BAD (marked as view) + function tes() public view returns (uint) { + // If this function is being used to initialize a state variable declared + // before w, w will be zero. If it is declared after w, w will be set. + if (w == 0) { + return 77; + } + + return 88; + } + + // GOOD (doesn't reference non constant state variable) + function okay() public returns (uint f) { + f = 1e10 * 1e12; + } + + // GOOD (doesn't reference non constant state variable) + function okay2() public returns (uint f) { + if (msg.sender == shouldntBeReported) { + f = 1e10 * 1e12; + } else { + f = 10; + } + } +} + +// CORRECT WAY +contract StateVarInitFromFunction2 { + uint public v; + uint public w = 5; + uint public x; + + constructor() { + v = set(); + x = set(); + } + + // GOOD here, although it refrences non constant variable, it is called from inside the constructor! + function set() public returns (uint) { + // If this function is being used to initialize a state variable declared + // before w, w will be zero. If it is declared after w, w will be set. + if (w == 0) { + return 77; + } + + return 88; + } +}