diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 6e97a787..33a403ef 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -101,6 +101,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), ] } @@ -112,6 +113,7 @@ pub fn get_all_detectors_names() -> Vec { #[derive(Debug, PartialEq, EnumString, Display)] #[strum(serialize_all = "kebab-case")] pub(crate) enum IssueDetectorNamePool { + MultiplePlaceholders, StateVariableChangesWithoutEvents, MissingInheritance, UnusedImport, @@ -207,6 +209,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { + Some(Box::::default()) + } IssueDetectorNamePool::StateVariableChangesWithoutEvents => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/low/mod.rs b/aderyn_core/src/detect/low/mod.rs index bc1be4ff..0d0855b7 100644 --- a/aderyn_core/src/detect/low/mod.rs +++ b/aderyn_core/src/detect/low/mod.rs @@ -18,6 +18,7 @@ pub(crate) mod inconsistent_type_names; pub(crate) mod large_literal_value; pub(crate) mod local_variable_shadowing; pub(crate) mod missing_inheritance; +pub(crate) mod multiple_placeholders; pub(crate) mod non_reentrant_before_others; pub(crate) mod public_variable_read_in_external_context; pub(crate) mod push_0_opcode; @@ -62,6 +63,7 @@ pub use inconsistent_type_names::InconsistentTypeNamesDetector; pub use large_literal_value::LargeLiteralValueDetector; pub use local_variable_shadowing::LocalVariableShadowingDetector; pub use missing_inheritance::MissingInheritanceDetector; +pub use multiple_placeholders::MultiplePlaceholdersDetector; pub use non_reentrant_before_others::NonReentrantBeforeOthersDetector; pub use public_variable_read_in_external_context::PublicVariableReadInExternalContextDetector; pub use push_0_opcode::PushZeroOpcodeDetector; diff --git a/aderyn_core/src/detect/low/multiple_placeholders.rs b/aderyn_core/src/detect/low/multiple_placeholders.rs new file mode 100644 index 00000000..9820885b --- /dev/null +++ b/aderyn_core/src/detect/low/multiple_placeholders.rs @@ -0,0 +1,92 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::NodeID; + +use crate::capture; +use crate::context::browser::ExtractPlaceholderStatements; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +// HOW TO USE THIS TEMPLATE: +// 1. Copy this file and rename it to the snake_case version of the issue you are detecting. +// 2. Rename the TemplateDetector struct and impl to your new issue name. +// 3. Add this file and detector struct to the mod.rs file in the same directory. +// 4. Implement the detect function to find instances of the issue. + +#[derive(Default)] +pub struct MultiplePlaceholdersDetector { + // 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>, + hints: BTreeMap<(String, usize, String), String>, +} + +impl IssueDetector for MultiplePlaceholdersDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for modifier in context.modifier_definitions() { + let placeholders = ExtractPlaceholderStatements::from(modifier).extracted; + if placeholders.len() > 1 { + capture!(self, context, modifier); + } + } + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::Low + } + + fn title(&self) -> String { + String::from("Modifier has multiple placeholders.") + } + + fn description(&self) -> String { + String::from("Design the modifier to only contain 1 placeholder statement. If it's not possible, split the logic into multiple modifiers.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn hints(&self) -> BTreeMap<(String, usize, String), String> { + self.hints.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::MultiplePlaceholders.to_string() + } +} + +#[cfg(test)] +mod multiple_placeholder_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, low::multiple_placeholders::MultiplePlaceholdersDetector, + }; + + #[test] + #[serial] + fn multiple_placeholders_test() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/MultiplePlaceholders.sol", + ); + + let mut detector = MultiplePlaceholdersDetector::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(), 1); + // assert the severity is low + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::Low + ); + } +} diff --git a/reports/report.json b/reports/report.json index 39705823..4dfcf10f 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 109, - "total_sloc": 3890 + "total_source_units": 110, + "total_sloc": 3904 }, "files_details": { "files_details": [ @@ -201,6 +201,10 @@ "file_path": "src/MultipleConstructorSchemes.sol", "n_sloc": 10 }, + { + "file_path": "src/MultiplePlaceholders.sol", + "n_sloc": 14 + }, { "file_path": "src/OnceModifierExample.sol", "n_sloc": 8 @@ -445,7 +449,7 @@ }, "issue_count": { "high": 42, - "low": 43 + "low": 44 }, "high_issues": { "issues": [ @@ -7118,6 +7122,19 @@ "src_char": "422:9" } ] + }, + { + "title": "Modifier has multiple placeholders.", + "description": "Design the modifier to only contain 1 placeholder statement. If it's not possible, split the logic into multiple modifiers.", + "detector_name": "multiple-placeholders", + "instances": [ + { + "contract_path": "src/MultiplePlaceholders.sol", + "line_no": 11, + "src": "186:10", + "src_char": "186:10" + } + ] } ] }, @@ -7206,6 +7223,7 @@ "unchecked-low-level-call", "function-pointer-in-constructor", "state-variable-could-be-declared-constant", - "state-variable-changes-without-events" + "state-variable-changes-without-events", + "multiple-placeholders" ] } \ No newline at end of file diff --git a/reports/report.md b/reports/report.md index bd91cd54..8d2530c7 100644 --- a/reports/report.md +++ b/reports/report.md @@ -94,6 +94,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [L-41: Function pointers used in constructors.](#l-41-function-pointers-used-in-constructors) - [L-42: State variable could be declared constant](#l-42-state-variable-could-be-declared-constant) - [L-43: State variable changes but no event is emitted.](#l-43-state-variable-changes-but-no-event-is-emitted) + - [L-44: Modifier has multiple placeholders.](#l-44-modifier-has-multiple-placeholders) # Summary @@ -102,8 +103,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 109 | -| Total nSLOC | 3890 | +| .sol Files | 110 | +| Total nSLOC | 3904 | ## Files Details @@ -159,6 +160,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/MisusedBoolean.sol | 67 | | src/MsgValueInLoop.sol | 55 | | src/MultipleConstructorSchemes.sol | 10 | +| src/MultiplePlaceholders.sol | 14 | | src/OnceModifierExample.sol | 8 | | src/OutOfOrderRetryable.sol | 165 | | src/PreDeclaredVarUsage.sol | 9 | @@ -219,7 +221,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** | **3890** | +| **Total** | **3904** | ## Issue Summary @@ -227,7 +229,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | | High | 42 | -| Low | 43 | +| Low | 44 | # High Issues @@ -7224,3 +7226,20 @@ State variable changes in this function but no event is emitted. +## L-44: Modifier has multiple placeholders. + +Design the modifier to only contain 1 placeholder statement. If it's not possible, split the logic into multiple modifiers. + +
1 Found Instances + + +- Found in src/MultiplePlaceholders.sol [Line: 11](../tests/contract-playground/src/MultiplePlaceholders.sol#L11) + + ```solidity + modifier checkOwner() { + ``` + +
+ + + diff --git a/reports/report.sarif b/reports/report.sarif index ff01ef8d..f1affc77 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -11904,6 +11904,26 @@ "text": "State variable changes in this function but no event is emitted." }, "ruleId": "state-variable-changes-without-events" + }, + { + "level": "note", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MultiplePlaceholders.sol" + }, + "region": { + "byteLength": 10, + "byteOffset": 186 + } + } + } + ], + "message": { + "text": "Design the modifier to only contain 1 placeholder statement. If it's not possible, split the logic into multiple modifiers." + }, + "ruleId": "multiple-placeholders" } ], "tool": { diff --git a/tests/contract-playground/src/MultiplePlaceholders.sol b/tests/contract-playground/src/MultiplePlaceholders.sol new file mode 100644 index 00000000..02366d9d --- /dev/null +++ b/tests/contract-playground/src/MultiplePlaceholders.sol @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.19; + +contract MultiplePlaceholders { + address internal owner; + + constructor() { + owner = msg.sender; + } + + modifier checkOwner() { + require(msg.sender == owner, "You are not the owner!"); + _; + _; + } + + // aderyn-ignore-next-line(empty-block) + function restrictedFunction1() external checkOwner {} + + // aderyn-ignore-next-line(empty-block) + function restrictedFunction2() external checkOwner {} +}