diff --git a/.github/workflows/cargo.yml b/.github/workflows/cargo.yml index 277ffde39..5471d6f45 100644 --- a/.github/workflows/cargo.yml +++ b/.github/workflows/cargo.yml @@ -141,11 +141,6 @@ jobs: useLockFile: false working-directory: tests/2024-05-Sablier/v2-core - - uses: bahmutov/npm-install@v1 - with: - useLockFile: false - working-directory: tests/2024-07-templegold/ - - uses: bahmutov/npm-install@v1 with: useLockFile: false @@ -213,16 +208,6 @@ jobs: cat ./reports/prb-math-report-workflow.md diff ./reports/prb-math-report.md ./reports/prb-math-report-workflow.md - - - name: Generate 2024-07-templegold-report-workflow.md - run: | - cargo run -- ./tests/2024-07-templegold/protocol -o ./reports/2024-07-templegold-report-workflow.md --skip-update-check - - - name: Check 2024-07-templegold-report.md vs 2024-07-templegold-report-workflow.md - run: | - cat ./reports/2024-07-templegold-report-workflow.md - diff ./reports/templegold-report.md ./reports/2024-07-templegold-report-workflow.md - # Verify report.json @@ -287,4 +272,4 @@ jobs: uses: actions-rs/cargo@v1 with: command: clippy - args: -- -D warnings + args: -- -D warnings \ No newline at end of file diff --git a/Cargo.lock b/Cargo.lock index cde268557..6db72ae99 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -23,7 +23,7 @@ dependencies = [ [[package]] name = "aderyn" -version = "0.1.7" +version = "0.1.8" dependencies = [ "aderyn_driver", "clap", @@ -38,10 +38,11 @@ dependencies = [ [[package]] name = "aderyn_core" -version = "0.1.7" +version = "0.1.8" dependencies = [ "crossbeam-channel", "cyfrin-foundry-compilers", + "derive_more", "eyre", "ignore", "once_cell", @@ -60,7 +61,7 @@ dependencies = [ [[package]] name = "aderyn_driver" -version = "0.1.7" +version = "0.1.8" dependencies = [ "aderyn_core", "criterion", @@ -75,7 +76,7 @@ dependencies = [ [[package]] name = "aderyn_py" -version = "0.1.7" +version = "0.1.8" dependencies = [ "aderyn_driver", "pyo3", diff --git a/Cargo.toml b/Cargo.toml index f70d04428..1209e1668 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,4 +11,8 @@ resolver="1" exclude = [ "bot_ci_cd", "foundry", -] \ No newline at end of file +] + +[profile.release] +codegen-units = 1 +lto = true \ No newline at end of file diff --git a/aderyn/Cargo.toml b/aderyn/Cargo.toml index 71231a57d..447696bd0 100644 --- a/aderyn/Cargo.toml +++ b/aderyn/Cargo.toml @@ -1,8 +1,8 @@ [package] name = "aderyn" -version = "0.1.7" +version = "0.1.8" edition = "2021" -authors = ["Alex Roan "] +authors = ["Cyfrin "] description = "Rust based Solidity AST analyzer" license = "MIT" default-run = "aderyn" @@ -10,7 +10,7 @@ default-run = "aderyn" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -aderyn_driver = { path = "../aderyn_driver", version = "0.1.7" } +aderyn_driver = { path = "../aderyn_driver", version = "0.1.8" } clap = { version = "4.4.6", features = ["derive"] } reqwest = { version = "0.12.2", default-features = false, features = ["blocking", "json", "rustls-tls"] } semver = "1.0.22" @@ -18,5 +18,4 @@ serde = { version = "1.0.160", features = ["derive"] } serde_json = { version = "1.0.96", features = ["preserve_order"] } strum = { version = "0.26", features = ["derive"] } notify-debouncer-full = "0.3.1" -cyfrin-foundry-compilers = { version = "0.3.20-aderyn", features = ["svm-solc"] } - +cyfrin-foundry-compilers = { version = "0.3.20-aderyn", features = ["svm-solc"] } \ No newline at end of file diff --git a/aderyn_core/Cargo.toml b/aderyn_core/Cargo.toml index 53990002d..5465f15b1 100644 --- a/aderyn_core/Cargo.toml +++ b/aderyn_core/Cargo.toml @@ -1,8 +1,8 @@ [package] name = "aderyn_core" -version = "0.1.7" +version = "0.1.8" edition = "2021" -authors = ["Alex Roan "] +authors = ["Cyfrin "] description = "Rust based Solidity AST analyzer backend" license = "MIT" @@ -23,6 +23,7 @@ serde_repr = "0.1.12" strum = { version = "0.26", features = ["derive"] } toml = "0.8.2" cyfrin-foundry-compilers = { version = "0.3.20-aderyn", features = ["svm-solc"] } +derive_more = "0.99.18" [dev-dependencies] serial_test = "3.0.0" diff --git a/aderyn_core/src/ast/ast.rs b/aderyn_core/src/ast/ast.rs index 843053a43..1daade1a9 100644 --- a/aderyn_core/src/ast/ast.rs +++ b/aderyn_core/src/ast/ast.rs @@ -80,3 +80,69 @@ impl From<&YulLiteral> for ASTNode { ASTNode::YulLiteral(value.clone()) } } + +impl From<&Expression> for ASTNode { + fn from(value: &Expression) -> Self { + match value { + Expression::Literal(literal) => ASTNode::Literal(literal.clone()), + Expression::Identifier(identifier) => ASTNode::Identifier(identifier.clone()), + Expression::UnaryOperation(unary_operation) => { + ASTNode::UnaryOperation(unary_operation.clone()) + } + Expression::BinaryOperation(binary_operation) => { + ASTNode::BinaryOperation(binary_operation.clone()) + } + Expression::Conditional(conditional) => ASTNode::Conditional(conditional.clone()), + Expression::Assignment(assignment) => ASTNode::Assignment(assignment.clone()), + Expression::FunctionCall(function_call) => ASTNode::FunctionCall(function_call.clone()), + Expression::FunctionCallOptions(function_call_ops) => { + ASTNode::FunctionCallOptions(function_call_ops.clone()) + } + Expression::IndexAccess(index_access) => ASTNode::IndexAccess(index_access.clone()), + Expression::IndexRangeAccess(index_range_access) => { + ASTNode::IndexRangeAccess(index_range_access.clone()) + } + Expression::MemberAccess(member_access) => ASTNode::MemberAccess(member_access.clone()), + Expression::ElementaryTypeNameExpression(elementary_type_name_expression) => { + ASTNode::ElementaryTypeNameExpression(elementary_type_name_expression.clone()) + } + Expression::TupleExpression(tuple_expression) => { + ASTNode::TupleExpression(tuple_expression.clone()) + } + Expression::NewExpression(new_expression) => { + ASTNode::NewExpression(new_expression.clone()) + } + } + } +} + +impl From for ASTNode { + fn from(value: Expression) -> Self { + match value { + Expression::Literal(literal) => ASTNode::Literal(literal), + Expression::Identifier(identifier) => ASTNode::Identifier(identifier), + Expression::UnaryOperation(unary_operation) => ASTNode::UnaryOperation(unary_operation), + Expression::BinaryOperation(binary_operation) => { + ASTNode::BinaryOperation(binary_operation) + } + Expression::Conditional(conditional) => ASTNode::Conditional(conditional), + Expression::Assignment(assignment) => ASTNode::Assignment(assignment), + Expression::FunctionCall(function_call) => ASTNode::FunctionCall(function_call), + Expression::FunctionCallOptions(function_call_ops) => { + ASTNode::FunctionCallOptions(function_call_ops) + } + Expression::IndexAccess(index_access) => ASTNode::IndexAccess(index_access), + Expression::IndexRangeAccess(index_range_access) => { + ASTNode::IndexRangeAccess(index_range_access) + } + Expression::MemberAccess(member_access) => ASTNode::MemberAccess(member_access), + Expression::ElementaryTypeNameExpression(elementary_type_name_expression) => { + ASTNode::ElementaryTypeNameExpression(elementary_type_name_expression) + } + Expression::TupleExpression(tuple_expression) => { + ASTNode::TupleExpression(tuple_expression) + } + Expression::NewExpression(new_expression) => ASTNode::NewExpression(new_expression), + } + } +} diff --git a/aderyn_core/src/audit/public_functions_no_sender.rs b/aderyn_core/src/audit/public_functions_no_sender.rs index ca48ce7e2..245e6f7f0 100644 --- a/aderyn_core/src/audit/public_functions_no_sender.rs +++ b/aderyn_core/src/audit/public_functions_no_sender.rs @@ -60,7 +60,7 @@ impl AuditorDetector for PublicFunctionsNoSenderChecksDetector { }); // Check if the function has a `msg.sender` BinaryOperation check let has_msg_sender_binary_operation = - has_msg_sender_binary_operation(function_definition); + has_msg_sender_binary_operation(&((*function_definition).into())); // TODO Check if the function has a hasRole identifier with msg.sender as an arg does_not_have_an_owner_modifier && !has_msg_sender_binary_operation }); diff --git a/aderyn_core/src/context/browser/extractor.rs b/aderyn_core/src/context/browser/extractor.rs index 5e35a5ca3..cece1c871 100644 --- a/aderyn_core/src/context/browser/extractor.rs +++ b/aderyn_core/src/context/browser/extractor.rs @@ -88,3 +88,40 @@ impl ASTConstVisitor for ExtractImmediateChildrenIDs { Ok(()) } } + +// Extract Reference Declaration IDs +#[derive(Default)] +pub struct ExtractReferencedDeclarations { + pub extracted: Vec, +} + +impl ExtractReferencedDeclarations { + pub fn from(node: &T) -> Self { + let mut extractor: ExtractReferencedDeclarations = Self::default(); + node.accept(&mut extractor).unwrap_or_default(); + extractor + } +} + +impl ASTConstVisitor for ExtractReferencedDeclarations { + fn visit_member_access(&mut self, node: &MemberAccess) -> Result { + if let Some(referenced_id) = node.referenced_declaration { + self.extracted.push(referenced_id); + } + Ok(true) + } + fn visit_identifier(&mut self, node: &Identifier) -> Result { + if let Some(referenced_id) = node.referenced_declaration { + self.extracted.push(referenced_id); + } + Ok(true) + } + fn visit_identifier_path(&mut self, node: &IdentifierPath) -> Result { + self.extracted.push(node.referenced_declaration as i64); + Ok(true) + } + fn visit_user_defined_type_name(&mut self, node: &UserDefinedTypeName) -> Result { + self.extracted.push(node.referenced_declaration); + Ok(true) + } +} diff --git a/aderyn_core/src/context/graph/mod.rs b/aderyn_core/src/context/graph/mod.rs new file mode 100644 index 000000000..3d688c447 --- /dev/null +++ b/aderyn_core/src/context/graph/mod.rs @@ -0,0 +1,32 @@ +pub mod traits; +mod workspace_callgraph; + +pub use workspace_callgraph::*; + +use derive_more::From; + +pub type Result = core::result::Result; + +#[derive(Debug, From)] +pub enum Error { + #[from] + Custom(String), + + // region: -- standard::* errors + WorkspaceCallGraphDFSError, + // endregion +} + +impl core::fmt::Display for Error { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{self:?}") + } +} + +impl From<&str> for Error { + fn from(value: &str) -> Self { + Error::Custom(value.to_string()) + } +} + +impl std::error::Error for Error {} diff --git a/aderyn_core/src/context/graph/traits.rs b/aderyn_core/src/context/graph/traits.rs new file mode 100644 index 000000000..661bbf571 --- /dev/null +++ b/aderyn_core/src/context/graph/traits.rs @@ -0,0 +1,4 @@ +/// Trait to support reversing of callgraph. (Because, direct impl is not allowed on Foreign Types) +pub trait Transpose { + fn reverse(&self) -> Self; +} diff --git a/aderyn_core/src/context/graph/workspace_callgraph.rs b/aderyn_core/src/context/graph/workspace_callgraph.rs new file mode 100644 index 000000000..745175474 --- /dev/null +++ b/aderyn_core/src/context/graph/workspace_callgraph.rs @@ -0,0 +1,134 @@ +use std::collections::{hash_map, HashMap, HashSet}; + +use crate::{ + ast::{Expression, IdentifierOrIdentifierPath, NodeID, NodeType}, + context::{ + browser::{ExtractFunctionCalls, ExtractModifierInvocations}, + workspace_context::WorkspaceContext, + }, +}; + +use super::traits::Transpose; + +#[derive(Debug)] +pub struct WorkspaceCallGraph { + pub graph: CallGraph, +} + +/** +* Every NodeID in CallGraph should corresponds to [`crate::ast::FunctionDefinition`] or [`crate::ast::ModifierDefinition`] +*/ +pub type CallGraph = HashMap>; + +impl WorkspaceCallGraph { + /// Formula to create [`WorkspaceCallGraph`] for global preprocessing . + pub fn from_context(context: &WorkspaceContext) -> super::Result { + let mut graph: CallGraph = HashMap::new(); + let mut visited: HashSet = HashSet::new(); + + let funcs = context + .function_definitions() + .into_iter() + .filter(|func| func.implemented) + .collect::>(); + + let modifier_definitions = context.modifier_definitions(); + + for func in funcs { + dfs_to_create_graph(func.id, &mut graph, &mut visited, context) + .map_err(|_| super::Error::WorkspaceCallGraphDFSError)?; + } + + for modifier in modifier_definitions { + dfs_to_create_graph(modifier.id, &mut graph, &mut visited, context) + .map_err(|_| super::Error::WorkspaceCallGraphDFSError)?; + } + + Ok(WorkspaceCallGraph { graph }) + } +} + +/// Make connections from each of the nodes of [`crate::ast::FunctionDefinition`] and [`crate::ast::ModifierDefinition`] +/// with their connected counterparts. +fn dfs_to_create_graph( + id: NodeID, + graph: &mut CallGraph, + visited: &mut HashSet, + context: &WorkspaceContext, +) -> super::Result<()> { + if visited.contains(&id) { + return Ok(()); + } + + visited.insert(id); + + // Only deal with `id`s that are in scope right now + if let Some(from_node) = context.nodes.get(&id) { + // referenced_declarations from previous calls in the recursion stack need to be vetted + if from_node.node_type() != NodeType::FunctionDefinition + && from_node.node_type() != NodeType::ModifierDefinition + { + return Ok(()); + } + + // connections to FunctionDefinition + let function_calls = ExtractFunctionCalls::from(from_node).extracted; + for function_call in function_calls { + if let Expression::Identifier(identifier) = function_call.expression.as_ref() { + if let Some(referenced_function_id) = identifier.referenced_declaration { + create_connection_if_not_exsits(id, referenced_function_id, graph); + dfs_to_create_graph(referenced_function_id, graph, visited, context)?; + } + } + } + + // connections to ModifierDefinition + let modifier_invocations = ExtractModifierInvocations::from(from_node).extracted; + for modifier_invocation in &modifier_invocations { + match &modifier_invocation.modifier_name { + IdentifierOrIdentifierPath::Identifier(identifier) => { + if let Some(reference_modifier_id) = identifier.referenced_declaration { + create_connection_if_not_exsits(id, reference_modifier_id, graph); + dfs_to_create_graph(reference_modifier_id, graph, visited, context)?; + } + } + IdentifierOrIdentifierPath::IdentifierPath(identifier_path) => { + let referenced_modifier_id = identifier_path.referenced_declaration; + create_connection_if_not_exsits(id, referenced_modifier_id as i64, graph); + dfs_to_create_graph(referenced_modifier_id as i64, graph, visited, context)?; + } + } + } + } + + // Change the default return to error later in "strict mode" maybe, because if we + // can't find the node that means, the file was not in scope and hence it is not + // available in the context although references to it exist. + Ok(()) +} + +fn create_connection_if_not_exsits(from_id: NodeID, to_id: NodeID, graph: &mut CallGraph) { + match graph.entry(from_id) { + hash_map::Entry::Occupied(mut o) => { + // Performance Tip: Maybe later use binary search (it requires keeping ascending order while inserting tho) + if !o.get().contains(&to_id) { + o.get_mut().push(to_id); + } + } + hash_map::Entry::Vacant(v) => { + v.insert(vec![to_id]); + } + } +} + +impl Transpose for CallGraph { + fn reverse(&self) -> Self { + let mut reversed_callgraph = CallGraph::default(); + for (from_id, tos) in self { + for to_id in tos { + create_connection_if_not_exsits(*to_id, *from_id, &mut reversed_callgraph); + } + } + reversed_callgraph + } +} diff --git a/aderyn_core/src/context/investigator/callgraph_tests.rs b/aderyn_core/src/context/investigator/callgraph_tests.rs new file mode 100644 index 000000000..faa0a1ccc --- /dev/null +++ b/aderyn_core/src/context/investigator/callgraph_tests.rs @@ -0,0 +1,275 @@ +#![allow(clippy::collapsible_match)] + +#[cfg(test)] +mod callgraph_tests { + use crate::{ + ast::{FunctionDefinition, ModifierDefinition}, + context::{ + investigator::{ + StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, + }, + workspace_context::{ASTNode, WorkspaceContext}, + }, + }; + + use StandardInvestigationStyle::*; + + fn get_function_by_name(context: &WorkspaceContext, name: &str) -> ASTNode { + ASTNode::from( + context + .function_definitions() + .into_iter() + .find(|&x| x.name == *name) + .unwrap(), + ) + } + + fn get_modifier_definition_by_name(context: &WorkspaceContext, name: &str) -> ASTNode { + ASTNode::from( + context + .modifier_definitions() + .into_iter() + .find(|&x| x.name == *name) + .unwrap(), + ) + } + + #[test] + fn test_callgraph_is_not_none() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + assert!(context.forward_callgraph.is_some()); + assert!(context.reverse_callgraph.is_some()); + } + + #[test] + fn test_tower1_modifier_has_no_downstream() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let visit_eighth_floor1 = get_function_by_name(&context, "visitEighthFloor1"); + + let investigator = + StandardInvestigator::new(&context, &[&visit_eighth_floor1], Downstream).unwrap(); + + let mut tracker = Tracker::new(&context); + investigator.investigate(&context, &mut tracker).unwrap(); + + assert!(tracker.downstream_func_definitions_names.is_empty()); + assert!(tracker.downstream_modifier_definitions_names.is_empty()); + } + + #[test] + fn test_tower1_modifier_has_upstream() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let visit_eighth_floor1 = get_function_by_name(&context, "visitEighthFloor1"); + + let investigator = + StandardInvestigator::new(&context, &[&visit_eighth_floor1], Upstream).unwrap(); + + let mut tracker = Tracker::new(&context); + investigator.investigate(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_upstream_modifiers_with_names(&["passThroughNinthFloor1"])); + assert!(tracker.has_found_upstream_functions_with_names(&["enterTenthFloor1"])); + } + + #[test] + fn test_tower2_modifier_has_both_upstream_and_downstream() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let pass_through_ninth_floor2 = + get_modifier_definition_by_name(&context, "passThroughNinthFloor2"); + + let investigator = + StandardInvestigator::new(&context, &[&pass_through_ninth_floor2], BothWays).unwrap(); + + let mut tracker = Tracker::new(&context); + investigator.investigate(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_downstream_functions_with_names(&["visitEighthFloor2"])); + assert!(tracker.has_found_upstream_functions_with_names(&["enterTenthFloor2"])); + } + + #[test] + fn test_tower3_modifier_has_both_upstream_and_downstream() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let pass_through_ninth_floor3 = + get_modifier_definition_by_name(&context, "passThroughNinthFloor3"); + + let investigator = + StandardInvestigator::new(&context, &[&pass_through_ninth_floor3], BothWays).unwrap(); + + let mut tracker = Tracker::new(&context); + investigator.investigate(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_upstream_functions_with_names(&["enterTenthFloor3"])); + assert!(tracker.has_found_downstream_functions_with_names(&["visitEighthFloor3"])); + assert!(tracker.has_not_found_any_upstream_functions_with_name("visitSeventhFloor3")); + assert!(tracker.has_found_upstream_side_effect_functions_with_name(&["visitSeventhFloor3"])); + } + + #[test] + fn test_tower3_functions_has_upstream() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let visit_eighth_floor3 = get_function_by_name(&context, "visitSeventhFloor3"); + + let investigator = + StandardInvestigator::new(&context, &[&visit_eighth_floor3], Upstream).unwrap(); + + let mut tracker = Tracker::new(&context); + investigator.investigate(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_upstream_functions_with_names(&["enterTenthFloor3"])); + } + + #[test] + fn test_tower4_functions_has_upstream_and_downstream() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let recurse = get_function_by_name(&context, "recurse"); + + let investigator = StandardInvestigator::new(&context, &[&recurse], BothWays).unwrap(); + + let mut tracker = Tracker::new(&context); + investigator.investigate(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_upstream_functions_with_names(&["recurse"])); + assert!(tracker.has_found_downstream_functions_with_names(&["recurse"])); + } + + struct Tracker<'a> { + context: &'a WorkspaceContext, + entry_points: Vec<(String, usize, String)>, + downstream_func_definitions_names: Vec, + upstream_func_definitions_names: Vec, + downstream_modifier_definitions_names: Vec, + upstream_modifier_definitions_names: Vec, + upstream_side_effects_func_definitions_names: Vec, + upstream_side_effects_modifier_definitions_names: Vec, + } + + impl<'a> Tracker<'a> { + fn new(context: &WorkspaceContext) -> Tracker { + Tracker { + context, + entry_points: vec![], + downstream_func_definitions_names: vec![], + downstream_modifier_definitions_names: vec![], + upstream_func_definitions_names: vec![], + upstream_modifier_definitions_names: vec![], + upstream_side_effects_func_definitions_names: vec![], + upstream_side_effects_modifier_definitions_names: vec![], + } + } + + // downstream functions + fn has_found_downstream_functions_with_names(&self, name: &[&str]) -> bool { + name.iter().all(|&n| { + self.downstream_func_definitions_names + .contains(&n.to_string()) + }) + } + + // upstream functions + fn has_found_upstream_functions_with_names(&self, name: &[&str]) -> bool { + name.iter().all(|&n| { + self.upstream_func_definitions_names + .contains(&n.to_string()) + }) + } + + fn has_not_found_any_upstream_functions_with_name(&self, name: &str) -> bool { + !self + .upstream_func_definitions_names + .contains(&name.to_string()) + } + + // upstream modifiers + fn has_found_upstream_modifiers_with_names(&self, name: &[&str]) -> bool { + name.iter().all(|&n| { + self.upstream_modifier_definitions_names + .contains(&n.to_string()) + }) + } + + // upstream side effects + fn has_found_upstream_side_effect_functions_with_name(&self, name: &[&str]) -> bool { + name.iter().all(|&n| { + self.upstream_side_effects_func_definitions_names + .contains(&n.to_string()) + }) + } + } + + impl StandardInvestigatorVisitor for Tracker<'_> { + fn visit_entry_point(&mut self, node: &ASTNode) -> eyre::Result<()> { + self.entry_points + .push(self.context.get_node_sort_key_pure(node)); + Ok(()) + } + fn visit_downstream_function_definition( + &mut self, + node: &crate::ast::FunctionDefinition, + ) -> eyre::Result<()> { + self.downstream_func_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_downstream_modifier_definition( + &mut self, + node: &crate::ast::ModifierDefinition, + ) -> eyre::Result<()> { + self.downstream_modifier_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_upstream_function_definition( + &mut self, + node: &crate::ast::FunctionDefinition, + ) -> eyre::Result<()> { + self.upstream_func_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_upstream_modifier_definition( + &mut self, + node: &crate::ast::ModifierDefinition, + ) -> eyre::Result<()> { + self.upstream_modifier_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_upstream_side_effect_function_definition( + &mut self, + node: &FunctionDefinition, + ) -> eyre::Result<()> { + self.upstream_side_effects_func_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_upstream_side_effect_modifier_definition( + &mut self, + node: &ModifierDefinition, + ) -> eyre::Result<()> { + self.upstream_side_effects_modifier_definitions_names + .push(node.name.to_string()); + Ok(()) + } + } +} diff --git a/aderyn_core/src/context/investigator/mod.rs b/aderyn_core/src/context/investigator/mod.rs new file mode 100644 index 000000000..79f660156 --- /dev/null +++ b/aderyn_core/src/context/investigator/mod.rs @@ -0,0 +1,46 @@ +mod callgraph_tests; +mod standard; +mod traits; + +pub use standard::*; +pub use traits::*; + +use derive_more::From; + +use crate::ast::{ASTNode, NodeID}; + +pub type Result = core::result::Result; + +#[derive(Debug, From)] +pub enum Error { + #[from] + Custom(String), + + // region: -- standard::* errors + ForwardCallgraphNotAvailable, + BackwardCallgraphNotAvailable, + UnidentifiedEntryPointNode(ASTNode), + InvalidEntryPointId(NodeID), + EntryPointVisitError, + UpstreamFunctionDefinitionVisitError, + UpstreamModifierDefinitionVisitError, + DownstreamFunctionDefinitionVisitError, + DownstreamModifierDefinitionVisitError, + UpstreamSideEffectFunctionDefinitionVisitError, + UpstreamSideEffectModifierDefinitionVisitError, + // endregion +} + +impl core::fmt::Display for Error { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{self:?}") + } +} + +impl From<&str> for Error { + fn from(value: &str) -> Self { + Error::Custom(value.to_string()) + } +} + +impl std::error::Error for Error {} diff --git a/aderyn_core/src/context/investigator/standard.rs b/aderyn_core/src/context/investigator/standard.rs new file mode 100644 index 000000000..70329a925 --- /dev/null +++ b/aderyn_core/src/context/investigator/standard.rs @@ -0,0 +1,372 @@ +//! This module helps with strategies on performing different types of investigations. +//! +//! Our first kind of investigator is [`StandardInvestigator`] it comes bundled with actions to help +//! application modules "hook in" and consume the graphs. +//! +//! + +use std::collections::HashSet; + +use crate::{ + ast::{NodeID, NodeType}, + context::{ + browser::{ExtractReferencedDeclarations, GetClosestAncestorOfTypeX}, + graph::WorkspaceCallGraph, + workspace_context::{ASTNode, WorkspaceContext}, + }, +}; + +use super::StandardInvestigatorVisitor; + +#[derive(PartialEq)] +pub enum StandardInvestigationStyle { + /// Picks the regular call graph (forward) + Downstream, + + /// Picks the reverse call graph + Upstream, + + /// Picks both the call graphs (choose this if upstream side effects also need to be tracked) + BothWays, +} + +pub struct StandardInvestigator { + /// Ad-hoc Nodes that we would like to explore downstream from. + pub entry_points: Vec, + + /// Surface points are calculated based on the entry points (input) + /// and only consists of [`crate::ast::FunctionDefinition`] and [`crate::ast::ModifierDefinition`] + /// These are nodes that are the *actual* starting points for traversal in the graph + pub forward_surface_points: Vec, + + /// Same as the forward one, but acts on reverse graph. + pub backward_surface_points: Vec, + + /// Decides what graph type to chose from [`WorkspaceContext`] + pub investigation_style: StandardInvestigationStyle, +} + +#[derive(PartialEq, Clone, Copy)] +enum CurrentDFSVector { + Forward, // Going downstream + Backward, // Going upstream + UpstreamSideEffect, // Going downstream from upstream nodes +} + +impl StandardInvestigator { + /// Creates a [`StandardInvestigator`] by exploring paths from given nodes. This is the starting point. + pub fn for_specific_nodes( + context: &WorkspaceContext, + nodes: &[&ASTNode], + investigation_style: StandardInvestigationStyle, + ) -> super::Result { + let mut entry_points = vec![]; + let mut forward_surface_points = vec![]; + let mut backward_surface_points = vec![]; + + // Construct entry points + for &node in nodes { + let node_id = node + .id() + .ok_or_else(|| super::Error::UnidentifiedEntryPointNode(node.clone()))?; + entry_points.push(node_id); + } + + // Construct forward surface points + for &node in nodes { + let referenced_declarations = ExtractReferencedDeclarations::from(node).extracted; + + for declared_id in referenced_declarations { + if let Some(node) = context.nodes.get(&declared_id) { + if node.node_type() == NodeType::ModifierDefinition { + forward_surface_points.push(declared_id); + } else if let ASTNode::FunctionDefinition(function_definition) = node { + if function_definition.implemented { + forward_surface_points.push(declared_id); + } + } + } + } + } + + // Construct backward surface points + for &node in nodes { + if node.node_type() == NodeType::FunctionDefinition + || node.node_type() == NodeType::ModifierDefinition + { + if let Some(id) = node.id() { + backward_surface_points.push(id); + } + } else { + let parent_surface_point = node + .closest_ancestor_of_type(context, NodeType::FunctionDefinition) + .or_else(|| { + node.closest_ancestor_of_type(context, NodeType::ModifierDefinition) + }); + if let Some(parent_surface_point) = parent_surface_point { + if let Some(parent_surface_point_id) = parent_surface_point.id() { + backward_surface_points.push(parent_surface_point_id); + } + } + } + } + + Ok(StandardInvestigator { + entry_points, + forward_surface_points, + backward_surface_points, + investigation_style, + }) + } + + pub fn new( + context: &WorkspaceContext, + nodes: &[&ASTNode], + investigation_style: StandardInvestigationStyle, + ) -> super::Result { + Self::for_specific_nodes(context, nodes, investigation_style) + } + + /// Visit the entry points and all the plausible function definitions and modifier definitions that + /// EVM may encounter during execution. + pub fn investigate(&self, context: &WorkspaceContext, visitor: &mut T) -> super::Result<()> + where + T: StandardInvestigatorVisitor, + { + self._investigate( + context, + context + .forward_callgraph + .as_ref() + .ok_or(super::Error::ForwardCallgraphNotAvailable)?, + context + .reverse_callgraph + .as_ref() + .ok_or(super::Error::BackwardCallgraphNotAvailable)?, + visitor, + ) + } + + /// Responsible for informing the trackers. + /// First, we visit the entry points. Then, we derive the subgraph from the [`WorkspaceCallGraph`] + /// which consists of all the nodes that can be reached by traversing the edges starting + /// from the surface points. + fn _investigate( + &self, + context: &WorkspaceContext, + forward_callgraph: &WorkspaceCallGraph, + reverse_callgraph: &WorkspaceCallGraph, + visitor: &mut T, + ) -> super::Result<()> + where + T: StandardInvestigatorVisitor, + { + // Visit entry point nodes (so that trackers can track the state across all code regions in 1 place) + for entry_point_id in &self.entry_points { + self.make_entry_point_visit_call(context, *entry_point_id, visitor)?; + } + + // Keep track of visited node IDs during DFS from surface nodes + let mut visited_downstream = HashSet::new(); + let mut visited_upstream = HashSet::new(); + let mut visited_upstream_side_effects = HashSet::new(); + + // Now decide, which points to visit upstream or downstream + if self.investigation_style == StandardInvestigationStyle::BothWays + || self.investigation_style == StandardInvestigationStyle::Downstream + { + // Visit the subgraph starting from surface points + for surface_point_id in &self.forward_surface_points { + self.dfs_and_visit_subgraph( + *surface_point_id, + &mut visited_downstream, + context, + forward_callgraph, + visitor, + CurrentDFSVector::Forward, + None, + )?; + } + } + + if self.investigation_style == StandardInvestigationStyle::BothWays + || self.investigation_style == StandardInvestigationStyle::Upstream + { + // Visit the subgraph starting from surface points + for surface_point_id in &self.backward_surface_points { + self.dfs_and_visit_subgraph( + *surface_point_id, + &mut visited_upstream, + context, + reverse_callgraph, + visitor, + CurrentDFSVector::Backward, + None, + )?; + } + } + + // Collect already visited nodes so that we don't repeat visit calls on them + // while traversing through side effect nodes. + let mut blacklisted = HashSet::new(); + blacklisted.extend(visited_downstream.iter()); + blacklisted.extend(visited_upstream.iter()); + blacklisted.extend(self.entry_points.iter()); + + if self.investigation_style == StandardInvestigationStyle::BothWays { + // Visit the subgraph from the upstream points (go downstream in forward graph) + // but do not re-visit the upstream nodes or the downstream nodes again + + for surface_point_id in &visited_upstream { + self.dfs_and_visit_subgraph( + *surface_point_id, + &mut visited_upstream_side_effects, + context, + forward_callgraph, + visitor, + CurrentDFSVector::UpstreamSideEffect, + Some(&blacklisted), + )?; + } + } + + Ok(()) + } + + #[allow(clippy::too_many_arguments)] + fn dfs_and_visit_subgraph( + &self, + node_id: NodeID, + visited: &mut HashSet, + context: &WorkspaceContext, + callgraph: &WorkspaceCallGraph, + visitor: &mut T, + current_investigation_direction: CurrentDFSVector, + blacklist: Option<&HashSet>, + ) -> super::Result<()> + where + T: StandardInvestigatorVisitor, + { + if visited.contains(&node_id) { + return Ok(()); + } + + visited.insert(node_id); + + if let Some(blacklist) = blacklist { + if !blacklist.contains(&node_id) { + self.make_relevant_visit_call( + context, + node_id, + visitor, + current_investigation_direction, + )?; + } + } else { + self.make_relevant_visit_call( + context, + node_id, + visitor, + current_investigation_direction, + )?; + } + + if let Some(pointing_to) = callgraph.graph.get(&node_id) { + for destination in pointing_to { + self.dfs_and_visit_subgraph( + *destination, + visited, + context, + callgraph, + visitor, + current_investigation_direction, + blacklist, + )?; + } + } + Ok(()) + } + + fn make_relevant_visit_call( + &self, + context: &WorkspaceContext, + node_id: NodeID, + visitor: &mut T, + current_investigation_direction: CurrentDFSVector, + ) -> super::Result<()> + where + T: StandardInvestigatorVisitor, + { + if let Some(node) = context.nodes.get(&node_id) { + if node.node_type() != NodeType::FunctionDefinition + && node.node_type() != NodeType::ModifierDefinition + { + return Ok(()); + } + + match current_investigation_direction { + CurrentDFSVector::Forward => { + if let ASTNode::FunctionDefinition(function) = node { + visitor + .visit_downstream_function_definition(function) + .map_err(|_| super::Error::DownstreamFunctionDefinitionVisitError)?; + } + if let ASTNode::ModifierDefinition(modifier) = node { + visitor + .visit_downstream_modifier_definition(modifier) + .map_err(|_| super::Error::DownstreamModifierDefinitionVisitError)?; + } + } + CurrentDFSVector::Backward => { + if let ASTNode::FunctionDefinition(function) = node { + visitor + .visit_upstream_function_definition(function) + .map_err(|_| super::Error::UpstreamFunctionDefinitionVisitError)?; + } + if let ASTNode::ModifierDefinition(modifier) = node { + visitor + .visit_upstream_modifier_definition(modifier) + .map_err(|_| super::Error::UpstreamModifierDefinitionVisitError)?; + } + } + CurrentDFSVector::UpstreamSideEffect => { + if let ASTNode::FunctionDefinition(function) = node { + visitor + .visit_upstream_side_effect_function_definition(function) + .map_err(|_| { + super::Error::UpstreamSideEffectFunctionDefinitionVisitError + })?; + } + if let ASTNode::ModifierDefinition(modifier) = node { + visitor + .visit_upstream_side_effect_modifier_definition(modifier) + .map_err(|_| { + super::Error::UpstreamSideEffectModifierDefinitionVisitError + })?; + } + } + } + } + + Ok(()) + } + + fn make_entry_point_visit_call( + &self, + context: &WorkspaceContext, + node_id: NodeID, + visitor: &mut T, + ) -> super::Result<()> + where + T: StandardInvestigatorVisitor, + { + let node = context + .nodes + .get(&node_id) + .ok_or(super::Error::InvalidEntryPointId(node_id))?; + visitor + .visit_entry_point(node) + .map_err(|_| super::Error::EntryPointVisitError)?; + Ok(()) + } +} diff --git a/aderyn_core/src/context/investigator/traits.rs b/aderyn_core/src/context/investigator/traits.rs new file mode 100644 index 000000000..c0c9a9b6d --- /dev/null +++ b/aderyn_core/src/context/investigator/traits.rs @@ -0,0 +1,78 @@ +//! Trackers can implement the following traits to interact with investigators +//! +//! NOTE +//! Upstream and downstream here is relative to [`super::StandardInvestigator::entry_points`] +//! which is initialized with [`super::StandardInvestigator::new`] function. + +use crate::{ + ast::{FunctionDefinition, ModifierDefinition}, + context::workspace_context::ASTNode, +}; + +/// Use with [`super::StandardInvestigator`] +pub trait StandardInvestigatorVisitor { + /// Shift all logic to tracker otherwise, you would track state at 2 different places + /// One at the tracker level, and other at the application level. Instead, we must + /// contain all of the tracking logic in the tracker. Therefore, visit entry point + /// is essential because the tracker can get to take a look at not just the + /// downstream functions and modifiers, but also the entry points that have invoked it. + fn visit_entry_point(&mut self, node: &ASTNode) -> eyre::Result<()> { + self.visit_any(node) + } + + /// Meant to be invoked while traversing [`crate::context::workspace_context::WorkspaceContext::forward_callgraph`] + fn visit_downstream_function_definition( + &mut self, + node: &FunctionDefinition, + ) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Meant to be invoked while traversing [`crate::context::workspace_context::WorkspaceContext::reverse_callgraph`] + fn visit_upstream_function_definition( + &mut self, + node: &FunctionDefinition, + ) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Meant to be invoked while traversing [`crate::context::workspace_context::WorkspaceContext::forward_callgraph`] + fn visit_downstream_modifier_definition( + &mut self, + node: &ModifierDefinition, + ) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Meant to be invoked while traversing [`crate::context::workspace_context::WorkspaceContext::reverse_callgraph`] + fn visit_upstream_modifier_definition( + &mut self, + node: &ModifierDefinition, + ) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Read as "upstream's downstream-side-effect" function definition + /// These are function definitions that are downstream from the upstream nodes + /// but are themselves neither upstream nor downstream to the entry points + fn visit_upstream_side_effect_function_definition( + &mut self, + node: &FunctionDefinition, + ) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Read as "upstream's downstream-side-effect" modifier definition + /// These are modifier definitions that are downstream from the upstream nodes + /// but are themselves neither upstream nor downstream to the entry points + fn visit_upstream_side_effect_modifier_definition( + &mut self, + node: &ModifierDefinition, + ) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + fn visit_any(&mut self, _node: &ASTNode) -> eyre::Result<()> { + Ok(()) + } +} diff --git a/aderyn_core/src/context/mod.rs b/aderyn_core/src/context/mod.rs index 4de4873cb..ca73f66c9 100644 --- a/aderyn_core/src/context/mod.rs +++ b/aderyn_core/src/context/mod.rs @@ -1,5 +1,7 @@ pub mod browser; pub mod capturable; +pub mod graph; +pub mod investigator; pub mod macros; pub mod meta_workspace; pub mod workspace_context; diff --git a/aderyn_core/src/context/workspace_context.rs b/aderyn_core/src/context/workspace_context.rs index 0adf3c650..5cd2cdb47 100644 --- a/aderyn_core/src/context/workspace_context.rs +++ b/aderyn_core/src/context/workspace_context.rs @@ -4,6 +4,7 @@ use std::collections::HashMap; use super::browser::GetImmediateParent; use super::capturable::Capturable; +use super::graph::WorkspaceCallGraph; pub use crate::ast::ASTNode; #[derive(Default, Debug, Clone, PartialEq, Eq, Hash)] @@ -27,6 +28,8 @@ pub struct WorkspaceContext { pub src_filepaths: Vec, pub sloc_stats: HashMap, pub ignore_lines_stats: HashMap>>, + pub forward_callgraph: Option, + pub reverse_callgraph: Option, pub nodes: HashMap, // Hashmaps of all nodes => source_unit_id diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 6d5633d9a..49ad141fa 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -4,31 +4,9 @@ use strum::{Display, EnumCount, EnumIter, EnumString}; use crate::{ ast::NodeID, context::workspace_context::WorkspaceContext, - detect::{ - high::{ - ArbitraryTransferFromDetector, AvoidAbiEncodePackedDetector, - BlockTimestampDeadlineDetector, DelegateCallInLoopDetector, - DynamicArrayLengthAssignmentDetector, EnumerableLoopRemovalDetector, - ExperimentalEncoderDetector, IncorrectShiftOrderDetector, - IncorrectUseOfCaretOperatorDetector, MultipleConstructorsDetector, - NestedStructInMappingDetector, ReusedContractNameDetector, - StateVariableShadowingDetector, StorageArrayEditWithMemoryDetector, - UnprotectedInitializerDetector, UnsafeCastingDetector, YulReturnDetector, - }, - low::{ - CentralizationRiskDetector, ConstantsInsteadOfLiteralsDetector, - ContractsWithTodosDetector, DeprecatedOZFunctionsDetector, - DivisionBeforeMultiplicationDetector, EcrecoverDetector, EmptyBlockDetector, - InconsistentTypeNamesDetector, LargeLiteralValueDetector, - NonReentrantBeforeOthersDetector, PushZeroOpcodeDetector, RequireWithStringDetector, - RevertsAndRequiresInLoopsDetector, SolmateSafeTransferLibDetector, - UnindexedEventsDetector, UnsafeERC20FunctionsDetector, UnsafeERC721MintDetector, - UnspecificSolidityPragmaDetector, UselessErrorDetector, - UselessInternalFunctionDetector, UselessModifierDetector, - UselessPublicFunctionDetector, ZeroAddressCheckDetector, - }, - }, + detect::{high::*, low::*}, }; + use std::{ collections::BTreeMap, error::Error, @@ -36,8 +14,6 @@ use std::{ str::FromStr, }; -use super::high::SelfdestructIdentifierDetector; - pub fn get_all_issue_detectors() -> Vec> { vec![ Box::::default(), @@ -78,9 +54,20 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), Box::::default(), Box::::default(), + Box::::default(), + Box::::default(), + Box::::default(), + Box::::default(), + Box::::default(), + Box::::default(), + Box::::default(), + Box::::default(), + Box::::default(), + Box::::default(), ] } @@ -130,9 +117,21 @@ pub(crate) enum IssueDetectorNamePool { NestedStructInMapping, SelfdestructIdentifier, DynamicArrayLengthAssignment, + UninitializedStateVariable, IncorrectCaretOperator, YulReturn, StateVariableShadowing, + MisusedBoolean, + SendEtherNoChecks, + DelegateCallUncheckedAddress, + TautologicalCompare, + #[allow(clippy::upper_case_acronyms)] + RTLO, + UncheckedReturn, + DangerousUnaryOperator, + WeakRandomness, + PreDeclaredLocalVariableUsage, + DeleteNestedMapping, // NOTE: `Undecided` will be the default name (for new bots). // If it's accepted, a new variant will be added to this enum before normalizing it in aderyn Undecided, @@ -242,6 +241,10 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + + IssueDetectorNamePool::UninitializedStateVariable => { + Some(Box::::default()) + } IssueDetectorNamePool::IncorrectCaretOperator => { Some(Box::::default()) } @@ -249,6 +252,28 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::MisusedBoolean => Some(Box::::default()), + IssueDetectorNamePool::SendEtherNoChecks => { + Some(Box::::default()) + } + IssueDetectorNamePool::DelegateCallUncheckedAddress => { + Some(Box::::default()) + } + IssueDetectorNamePool::TautologicalCompare => { + Some(Box::::default()) + } + IssueDetectorNamePool::RTLO => Some(Box::::default()), + IssueDetectorNamePool::UncheckedReturn => Some(Box::::default()), + IssueDetectorNamePool::DangerousUnaryOperator => { + Some(Box::::default()) + } + IssueDetectorNamePool::WeakRandomness => Some(Box::::default()), + IssueDetectorNamePool::PreDeclaredLocalVariableUsage => { + Some(Box::::default()) + } + IssueDetectorNamePool::DeleteNestedMapping => { + Some(Box::::default()) + } IssueDetectorNamePool::Undecided => None, } } diff --git a/aderyn_core/src/detect/helpers.rs b/aderyn_core/src/detect/helpers.rs index 2b045f011..97851e33d 100644 --- a/aderyn_core/src/detect/helpers.rs +++ b/aderyn_core/src/detect/helpers.rs @@ -1,9 +1,15 @@ use semver::{Error, VersionReq}; use crate::{ - ast::{Expression, FunctionDefinition, MemberAccess, NodeID, PragmaDirective, Visibility}, + ast::{ + ASTNode, Expression, FunctionDefinition, Identifier, LiteralKind, MemberAccess, NodeID, + PragmaDirective, VariableDeclaration, Visibility, + }, context::{ - browser::{ExtractBinaryOperations, ExtractMemberAccesses}, + browser::{ + ExtractBinaryOperations, ExtractFunctionCallOptions, ExtractFunctionCalls, + ExtractMemberAccesses, + }, workspace_context::WorkspaceContext, }, }; @@ -66,16 +72,16 @@ pub fn pragma_directive_to_semver(pragma_directive: &PragmaDirective) -> Result< VersionReq::parse(&version_string) } -// Check if a function definition has a `msg.sender` binary operation. +// Check if an ast_node has a `msg.sender` binary operation. // Examples: // ``` // function foo() public { // require(msg.sender == owner); // } // ``` -pub fn has_msg_sender_binary_operation(function_definition: &FunctionDefinition) -> bool { +pub fn has_msg_sender_binary_operation(ast_node: &ASTNode) -> bool { // Directly return the evaluation of the condition - ExtractBinaryOperations::from(function_definition) + ExtractBinaryOperations::from(ast_node) .extracted .iter() .any(|binary_operation| { @@ -94,3 +100,179 @@ pub fn has_msg_sender_binary_operation(function_definition: &FunctionDefinition) }) }) } + +// Check if an ast_node sends native eth +// Examples: +// ``` +// function foo() public { +// address(0x1).call{value: 10}("...") +// } +// ``` +pub fn has_calls_that_sends_native_eth(ast_node: &ASTNode) -> bool { + // Check for address(..).call{value: 10}("...") pattern + let function_call_ops = ExtractFunctionCallOptions::from(ast_node).extracted; + for function_call in &function_call_ops { + let call_carries_value = function_call.options.iter().any(|c| { + if let Expression::Literal(literal) = c { + return literal.value.is_some(); + } + false + }); + if !call_carries_value { + continue; + } + if let Expression::MemberAccess(member_access) = function_call.expression.as_ref() { + let is_call = member_access.member_name == "call"; + if !is_call { + continue; + } + } + return true; + } + + // Now, check for :- + + // payable(address(..)).transfer(100) + // payable(address(..)).send(100) + + let function_calls = ExtractFunctionCalls::from(ast_node).extracted; + + for function_call in function_calls { + if let Expression::MemberAccess(member_access) = function_call.expression.as_ref() { + if member_access.member_name == "transfer" || member_access.member_name == "send" { + if let Some(type_description) = member_access.expression.type_descriptions() { + if type_description + .type_string + .as_ref() + .is_some_and(|type_string| type_string.starts_with("address")) + { + return true; + } + } + } + } + } + + false +} + +/// Detects for the pattern +/// x.delegatecall("...") where x is not a state variable +/// That means, it can be +/// a) An Identifier that references a variable declaration which is not `state_variable` +/// b) A literal adresss +pub fn has_delegate_calls_on_non_state_variables( + ast_node: &ASTNode, + context: &WorkspaceContext, +) -> bool { + let member_accesses = ExtractMemberAccesses::from(ast_node).extracted; + member_accesses.into_iter().any(|member| { + let is_delegate_call = member.member_name == "delegatecall"; + let mut is_on_non_state_variable = false; + if let Expression::Identifier(identifier) = member.expression.as_ref() { + if let Some(referenced_id) = identifier.referenced_declaration { + if let Some(ASTNode::VariableDeclaration(v)) = context.nodes.get(&referenced_id) { + if !v.state_variable { + is_on_non_state_variable = true; + } + } + } + } else if let Expression::Literal(_) = member.expression.as_ref() { + is_on_non_state_variable = true; + } + is_delegate_call && is_on_non_state_variable + }) +} + +pub fn has_binary_checks_on_some_address(ast_node: &ASTNode) -> bool { + let binary_operations = ExtractBinaryOperations::from(ast_node).extracted; + binary_operations.into_iter().any(|op| { + [op.left_expression, op.right_expression].iter().any(|op| { + op.as_ref().type_descriptions().is_some_and(|desc| { + desc.type_string + .as_ref() + .is_some_and(|type_string| type_string == "address") + }) + }) + }) +} + +pub fn get_literal_value_or_constant_variable_value( + node_id: NodeID, + context: &WorkspaceContext, +) -> Option { + fn get_constant_variable_declaration_value(variable: &VariableDeclaration) -> Option { + if variable.mutability() == Some(&crate::ast::Mutability::Constant) { + if let Some(Expression::Literal(literal)) = variable.value.as_ref() { + return literal.value.to_owned(); + } + } + None + } + + if let Some(node) = context.nodes.get(&node_id) { + match node { + ASTNode::Literal(literal) => return literal.value.to_owned(), + ASTNode::VariableDeclaration(variable) => { + return get_constant_variable_declaration_value(variable); + } + _ => (), + } + } + None +} + +pub fn get_node_offset(node: &ASTNode) -> Option { + let src_location = node.src()?; + + let chopped_location = match src_location.rfind(':') { + Some(index) => &src_location[..index], + None => src_location, // No colon found, return the original string + } + .to_string(); + + let (offset, _) = chopped_location.split_once(':').unwrap(); + offset.parse::().ok() +} + +/* +Check if expression in constant +Expression::Literal whose value is false/true +Expression::Identifier that refers to a constant boolean variable declaration +Expression::UnaryOperation with ! operator followed by a sub expression that could be either of the above +*/ +pub fn is_constant_boolean(context: &WorkspaceContext, ast_node: &Expression) -> bool { + if let Expression::Literal(literal) = ast_node { + if literal.kind == LiteralKind::Bool + && literal + .value + .as_ref() + .is_some_and(|value| value == "false" || value == "true") + { + return true; + } + } + if let Expression::Identifier(Identifier { + referenced_declaration: Some(id), + .. + }) = ast_node + { + if let Some(ASTNode::VariableDeclaration(variable_declaration)) = context.nodes.get(id) { + if variable_declaration + .type_descriptions + .type_string + .as_ref() + .is_some_and(|value| value == "bool") + && variable_declaration.mutability() == Some(&crate::ast::Mutability::Constant) + { + return true; + } + } + } + if let Expression::UnaryOperation(operation) = ast_node { + if operation.operator == "!" { + return is_constant_boolean(context, operation.sub_expression.as_ref()); + } + } + false +} diff --git a/aderyn_core/src/detect/high/_template.rs b/aderyn_core/src/detect/high/_template.rs index fe7f03770..98b9864a8 100644 --- a/aderyn_core/src/detect/high/_template.rs +++ b/aderyn_core/src/detect/high/_template.rs @@ -57,9 +57,12 @@ impl IssueDetector for TemplateDetector { #[cfg(test)] mod template_detector_tests { + use serial_test::serial; + use crate::detect::{detector::IssueDetector, high::template_detector::TemplateDetector}; #[test] + #[serial] fn test_template_detector() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/ArbitraryTransferFrom.sol", diff --git a/aderyn_core/src/detect/high/dangerous_unary_operator.rs b/aderyn_core/src/detect/high/dangerous_unary_operator.rs new file mode 100644 index 000000000..3d079bc97 --- /dev/null +++ b/aderyn_core/src/detect/high/dangerous_unary_operator.rs @@ -0,0 +1,96 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::NodeID; + +use crate::capture; +use crate::context::browser::Peek; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct DangerousUnaryOperatorDetector { + // 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 DangerousUnaryOperatorDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for assignment in context.assignments() { + if let Some(content) = assignment.peek(context) { + if content.contains("=-") || content.contains("=+") { + capture!(self, context, assignment); + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Dangerous unary operator found in assignment.") + } + + fn description(&self) -> String { + String::from("Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in between.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::DangerousUnaryOperator.to_string() + } +} + +#[cfg(test)] +mod dangerous_unary_expression_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, high::dangerous_unary_operator::DangerousUnaryOperatorDetector, + }; + + #[test] + #[serial] + fn test_dangerous_unary_operator() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/DangerousUnaryOperator.sol", + ); + + let mut detector = DangerousUnaryOperatorDetector::default(); + let found = detector.detect(&context).unwrap(); + // assert that the detector found an issue + + println!("{:#?}", detector.instances()); + + assert!(found); + // assert that the detector found the correct number of instances + assert_eq!(detector.instances().len(), 2); + // assert the severity is high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + // assert the title is correct + assert_eq!( + detector.title(), + String::from("Dangerous unary operator found in assignment.") + ); + // assert the description is correct + assert_eq!( + detector.description(), + String::from("Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in between.") + ); + } +} diff --git a/aderyn_core/src/detect/high/delegate_call_no_address_check.rs b/aderyn_core/src/detect/high/delegate_call_no_address_check.rs new file mode 100644 index 000000000..19ec1e134 --- /dev/null +++ b/aderyn_core/src/detect/high/delegate_call_no_address_check.rs @@ -0,0 +1,135 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::NodeID; + +use crate::capture; +use crate::context::investigator::{ + StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, +}; +use crate::detect::detector::IssueDetectorNamePool; +use crate::detect::helpers; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct DelegateCallOnUncheckedAddressDetector { + // 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 DelegateCallOnUncheckedAddressDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for func in helpers::get_implemented_external_and_public_functions(context) { + let mut tracker = DelegateCallNoAddressChecksTracker { + has_address_checks: false, + has_delegate_call_on_non_state_variable_address: false, + context, + }; + let investigator = StandardInvestigator::new( + context, + &[&(func.into())], + StandardInvestigationStyle::Downstream, + )?; + investigator.investigate(context, &mut tracker)?; + + if tracker.has_delegate_call_on_non_state_variable_address + && !tracker.has_address_checks + { + capture!(self, context, func) + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Delegatecall made by the function without checks on any adress.") + } + + fn description(&self) -> String { + String::from("Introduce checks on the address") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::DelegateCallUncheckedAddress.to_string() + } +} + +struct DelegateCallNoAddressChecksTracker<'a> { + has_address_checks: bool, + has_delegate_call_on_non_state_variable_address: bool, + context: &'a WorkspaceContext, +} + +impl<'a> StandardInvestigatorVisitor for DelegateCallNoAddressChecksTracker<'a> { + fn visit_any(&mut self, node: &crate::context::workspace_context::ASTNode) -> eyre::Result<()> { + if !self.has_address_checks && helpers::has_binary_checks_on_some_address(node) { + self.has_address_checks = true; + } + if !self.has_delegate_call_on_non_state_variable_address + && helpers::has_delegate_calls_on_non_state_variables(node, self.context) + { + self.has_delegate_call_on_non_state_variable_address = true; + } + eyre::Ok(()) + } +} + +#[cfg(test)] +mod delegate_call_no_address_check_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, + high::delegate_call_no_address_check::DelegateCallOnUncheckedAddressDetector, + }; + + #[test] + #[serial] + fn test_delegate_call_without_checks() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol", + ); + + let mut detector = DelegateCallOnUncheckedAddressDetector::default(); + let found = detector.detect(&context).unwrap(); + + println!("{:#?}", detector.found_instances); + + // 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 high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + + // assert the title is correct + assert_eq!( + detector.title(), + String::from("Delegatecall made by the function without checks on any adress.") + ); + // assert the description is correct + assert_eq!( + detector.description(), + String::from("Introduce checks on the address") + ); + } +} diff --git a/aderyn_core/src/detect/high/deletion_nested_mapping.rs b/aderyn_core/src/detect/high/deletion_nested_mapping.rs new file mode 100644 index 000000000..8676956e2 --- /dev/null +++ b/aderyn_core/src/detect/high/deletion_nested_mapping.rs @@ -0,0 +1,138 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::{ + ASTNode, Expression, Identifier, IndexAccess, Mapping, NodeID, TypeName, UserDefinedTypeName, + VariableDeclaration, +}; + +use crate::capture; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct DeletionNestedMappingDetector { + // 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 DeletionNestedMappingDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for delete_operation in context + .unary_operations() + .into_iter() + .filter(|op| op.operator == "delete") + { + if let Expression::IndexAccess(IndexAccess { + base_expression, .. + }) = delete_operation.sub_expression.as_ref() + { + if let Expression::Identifier(Identifier { + referenced_declaration: Some(referenced_id), + type_descriptions, + .. + }) = base_expression.as_ref() + { + // Check if we're deleting a value from mapping + if type_descriptions + .type_string + .as_ref() + .is_some_and(|type_string| type_string.starts_with("mapping")) + { + // Check if the value in the mapping is of type struct that has a member which is also a mapping + if let Some(ASTNode::VariableDeclaration(VariableDeclaration { + type_name: Some(TypeName::Mapping(Mapping { value_type, .. })), + .. + })) = context.nodes.get(referenced_id) + { + if let TypeName::UserDefinedTypeName(UserDefinedTypeName { + referenced_declaration, + .. + }) = value_type.as_ref() + { + if let Some(ASTNode::StructDefinition(structure)) = + context.nodes.get(referenced_declaration) + { + // Check that a member of a struct is of type mapping + if structure.members.iter().any(|member| { + member.type_descriptions.type_string.as_ref().is_some_and( + |type_string| type_string.starts_with("mapping"), + ) + }) { + capture!(self, context, delete_operation); + } + } + } + } + } + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Deletion from a nested mappping.") + } + + fn description(&self) -> String { + String::from("A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::DeleteNestedMapping.to_string() + } +} + +#[cfg(test)] +mod deletion_nested_mapping_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, high::deletion_nested_mapping::DeletionNestedMappingDetector, + }; + + #[test] + #[serial] + fn test_deletion_nested_mapping() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/DeletionNestedMappingStructureContract.sol", + ); + + let mut detector = DeletionNestedMappingDetector::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 high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + // assert the title is correct + assert_eq!( + detector.title(), + String::from("Deletion from a nested mappping.") + ); + // assert the description is correct + assert_eq!( + detector.description(), + String::from("A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract.") + ); + } +} diff --git a/aderyn_core/src/detect/high/dynamic_array_length_assignment.rs b/aderyn_core/src/detect/high/dynamic_array_length_assignment.rs index 31e25ec8c..f1abb1ea0 100644 --- a/aderyn_core/src/detect/high/dynamic_array_length_assignment.rs +++ b/aderyn_core/src/detect/high/dynamic_array_length_assignment.rs @@ -71,12 +71,15 @@ impl IssueDetector for DynamicArrayLengthAssignmentDetector { #[cfg(test)] mod dynamic_array_length_assignment_tests { + use serial_test::serial; + use crate::detect::{ detector::IssueDetector, high::DynamicArrayLengthAssignmentDetector, test_utils::load_solidity_source_unit, }; #[test] + #[serial] fn test_reused_contract_name_detector() { let context = load_solidity_source_unit( "../tests/contract-playground/src/DynamicArrayLengthAssignment.sol", diff --git a/aderyn_core/src/detect/high/enumerable_loop_removal.rs b/aderyn_core/src/detect/high/enumerable_loop_removal.rs index d7c63f0a8..3e0ddeb5a 100644 --- a/aderyn_core/src/detect/high/enumerable_loop_removal.rs +++ b/aderyn_core/src/detect/high/enumerable_loop_removal.rs @@ -83,9 +83,12 @@ Consider using a different data structure or removing items by collecting them d #[cfg(test)] mod enuemrable_loop_removal_tests { + use serial_test::serial; + use crate::detect::{detector::IssueDetector, high::EnumerableLoopRemovalDetector}; #[test] + #[serial] fn test_enumerable_loop_detector() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/EnumerableSetIteration.sol", diff --git a/aderyn_core/src/detect/high/experimental_encoder.rs b/aderyn_core/src/detect/high/experimental_encoder.rs index 6044651f9..7752b1e5c 100644 --- a/aderyn_core/src/detect/high/experimental_encoder.rs +++ b/aderyn_core/src/detect/high/experimental_encoder.rs @@ -54,11 +54,14 @@ impl IssueDetector for ExperimentalEncoderDetector { #[cfg(test)] mod storage_array_encode_compiler_bug_detector_tests { + use serial_test::serial; + use crate::detect::{ detector::IssueDetector, high::experimental_encoder::ExperimentalEncoderDetector, }; #[test] + #[serial] fn test_storage_array_encode_compiler_bug_detector() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/ExperimentalEncoder.sol", diff --git a/aderyn_core/src/detect/high/incorrect_caret_operator.rs b/aderyn_core/src/detect/high/incorrect_caret_operator.rs index 5c3c913f6..51a3381fc 100644 --- a/aderyn_core/src/detect/high/incorrect_caret_operator.rs +++ b/aderyn_core/src/detect/high/incorrect_caret_operator.rs @@ -89,9 +89,12 @@ impl IssueDetector for IncorrectUseOfCaretOperatorDetector { #[cfg(test)] mod incorrect_use_of_caret_operator_tests { + use serial_test::serial; + use crate::detect::{detector::IssueDetector, high::IncorrectUseOfCaretOperatorDetector}; #[test] + #[serial] fn test_incorrect_use_of_operator_detector() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/IncorrectCaretOperator.sol", diff --git a/aderyn_core/src/detect/high/incorrect_shift_order.rs b/aderyn_core/src/detect/high/incorrect_shift_order.rs index 21534eb96..c96ced6c0 100644 --- a/aderyn_core/src/detect/high/incorrect_shift_order.rs +++ b/aderyn_core/src/detect/high/incorrect_shift_order.rs @@ -59,9 +59,12 @@ impl IssueDetector for IncorrectShiftOrderDetector { #[cfg(test)] mod incorrect_shift_order_detector_tests { + use serial_test::serial; + use crate::detect::{detector::IssueDetector, high::IncorrectShiftOrderDetector}; #[test] + #[serial] fn test_incorrect_shift_order_detector() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/IncorrectShift.sol", diff --git a/aderyn_core/src/detect/high/misused_boolean.rs b/aderyn_core/src/detect/high/misused_boolean.rs new file mode 100644 index 000000000..a726674f0 --- /dev/null +++ b/aderyn_core/src/detect/high/misused_boolean.rs @@ -0,0 +1,58 @@ +use crate::detect::helpers::is_constant_boolean; +use crate::issue_detector; +use eyre::Result; + +issue_detector! { + MisusedBooleanDetector; + + severity: High, + title: "Misused boolean with logical operators", + desc: "The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively.", + name: MisusedBoolean, + + |context| { + for binary_operation in context.binary_operations() { + if (binary_operation.operator == "||" || 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); + } + + } + + for if_statement in context.if_statements() + .iter() + .filter(|statement| is_constant_boolean(context, &statement.condition)) { + grab!(if_statement); + } + + } + +} + +#[cfg(test)] +mod misused_boolean_tests { + use serial_test::serial; + + use crate::detect::{detector::IssueDetector, high::misused_boolean::MisusedBooleanDetector}; + + #[test] + #[serial] + fn test_misused_boolean_by_loading_contract_directly() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/MisusedBoolean.sol", + ); + + let mut detector = MisusedBooleanDetector::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(), 10); + } +} diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index 1ecf8db8b..f5b609722 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -1,37 +1,59 @@ pub(crate) mod arbitrary_transfer_from; pub(crate) mod avoid_abi_encode_packed; pub(crate) mod block_timestamp_deadline; +pub(crate) mod dangerous_unary_operator; pub(crate) mod delegate_call_in_loop; +pub(crate) mod delegate_call_no_address_check; +pub(crate) mod deletion_nested_mapping; pub(crate) mod dynamic_array_length_assignment; pub(crate) mod enumerable_loop_removal; pub(crate) mod experimental_encoder; pub(crate) mod incorrect_caret_operator; pub(crate) mod incorrect_shift_order; +pub(crate) mod misused_boolean; pub(crate) mod multiple_constructors; pub(crate) mod nested_struct_in_mapping; +pub(crate) mod pre_declared_variable_usage; pub(crate) mod reused_contract_name; +pub(crate) mod rtlo; pub(crate) mod selfdestruct; +pub(crate) mod send_ether_no_checks; pub(crate) mod state_variable_shadowing; pub(crate) mod storage_array_edit_with_memory; +pub(crate) mod tautological_compare; +pub(crate) mod unchecked_return; +pub(crate) mod uninitialized_state_variable; pub(crate) mod unprotected_init_function; pub(crate) mod unsafe_casting; +pub(crate) mod weak_randomness; pub(crate) mod yul_return; pub use arbitrary_transfer_from::ArbitraryTransferFromDetector; pub use avoid_abi_encode_packed::AvoidAbiEncodePackedDetector; pub use block_timestamp_deadline::BlockTimestampDeadlineDetector; +pub use dangerous_unary_operator::DangerousUnaryOperatorDetector; pub use delegate_call_in_loop::DelegateCallInLoopDetector; +pub use delegate_call_no_address_check::DelegateCallOnUncheckedAddressDetector; +pub use deletion_nested_mapping::DeletionNestedMappingDetector; pub use dynamic_array_length_assignment::DynamicArrayLengthAssignmentDetector; pub use enumerable_loop_removal::EnumerableLoopRemovalDetector; pub use experimental_encoder::ExperimentalEncoderDetector; pub use incorrect_caret_operator::IncorrectUseOfCaretOperatorDetector; pub use incorrect_shift_order::IncorrectShiftOrderDetector; +pub use misused_boolean::MisusedBooleanDetector; pub use multiple_constructors::MultipleConstructorsDetector; pub use nested_struct_in_mapping::NestedStructInMappingDetector; +pub use pre_declared_variable_usage::PreDeclaredLocalVariableUsageDetector; pub use reused_contract_name::ReusedContractNameDetector; +pub use rtlo::RTLODetector; pub use selfdestruct::SelfdestructIdentifierDetector; +pub use send_ether_no_checks::SendEtherNoChecksDetector; pub use state_variable_shadowing::StateVariableShadowingDetector; pub use storage_array_edit_with_memory::StorageArrayEditWithMemoryDetector; +pub use tautological_compare::TautologicalCompareDetector; +pub use unchecked_return::UncheckedReturnDetector; +pub use uninitialized_state_variable::UninitializedStateVariableDetector; pub use unprotected_init_function::UnprotectedInitializerDetector; pub use unsafe_casting::UnsafeCastingDetector; +pub use weak_randomness::WeakRandomnessDetector; pub use yul_return::YulReturnDetector; diff --git a/aderyn_core/src/detect/high/multiple_constructors.rs b/aderyn_core/src/detect/high/multiple_constructors.rs index 6c3a2346f..4175b4686 100644 --- a/aderyn_core/src/detect/high/multiple_constructors.rs +++ b/aderyn_core/src/detect/high/multiple_constructors.rs @@ -64,9 +64,12 @@ impl IssueDetector for MultipleConstructorsDetector { #[cfg(test)] mod multiple_constructors_detector_tests { + use serial_test::serial; + use crate::detect::{detector::IssueDetector, high::MultipleConstructorsDetector}; #[test] + #[serial] fn test_multiple_constructors_detector() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/MultipleConstructorSchemes.sol", @@ -96,6 +99,7 @@ mod multiple_constructors_detector_tests { } #[test] + #[serial] fn test_multiple_constructors_detector_no_issue() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/ArbitraryTransferFrom.sol", diff --git a/aderyn_core/src/detect/high/nested_struct_in_mapping.rs b/aderyn_core/src/detect/high/nested_struct_in_mapping.rs index bc116d2ec..3d1313654 100644 --- a/aderyn_core/src/detect/high/nested_struct_in_mapping.rs +++ b/aderyn_core/src/detect/high/nested_struct_in_mapping.rs @@ -109,9 +109,12 @@ impl IssueDetector for NestedStructInMappingDetector { #[cfg(test)] mod nested_struct_in_mapping_detector_tests { + use serial_test::serial; + use crate::detect::{detector::IssueDetector, high::NestedStructInMappingDetector}; #[test] + #[serial] fn test_nested_struct_in_mapping_detector() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/nested_mappings/NestedMappings.sol", @@ -141,6 +144,7 @@ mod nested_struct_in_mapping_detector_tests { } #[test] + #[serial] fn test_nested_struct_in_mapping_detector_no_issue() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/nested_mappings/LaterVersion.sol", diff --git a/aderyn_core/src/detect/high/pre_declared_variable_usage.rs b/aderyn_core/src/detect/high/pre_declared_variable_usage.rs new file mode 100644 index 000000000..0873765b1 --- /dev/null +++ b/aderyn_core/src/detect/high/pre_declared_variable_usage.rs @@ -0,0 +1,139 @@ +use std::collections::{BTreeMap, HashSet}; +use std::error::Error; + +use crate::ast::{ASTNode, NodeID}; + +use crate::capture; +use crate::context::browser::{ExtractIdentifiers, ExtractVariableDeclarations}; +use crate::detect::detector::IssueDetectorNamePool; +use crate::detect::helpers; +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 PreDeclaredLocalVariableUsageDetector 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 PreDeclaredLocalVariableUsageDetector { + // 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 PreDeclaredLocalVariableUsageDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + // Since this is restricted to local variables, we examine each function independently + for function in context + .function_definitions() + .into_iter() + .filter(|&f| f.implemented) + { + let local_variable_declaration_ids = ExtractVariableDeclarations::from(function) + .extracted + .iter() + .map(|vd| vd.id) + .collect::>(); + + let used_local_variables = ExtractIdentifiers::from(function).extracted; + + let used_local_variables = used_local_variables + .iter() + .filter(|identifier| { + identifier + .referenced_declaration + .is_some_and(|referenced_declaration| { + local_variable_declaration_ids.contains(&referenced_declaration) + }) + }) + .collect::>(); + + for used in used_local_variables { + if let Some(id) = used.referenced_declaration { + if let Some(ASTNode::VariableDeclaration(variable_declaration)) = + context.nodes.get(&id) + { + let used_offset = helpers::get_node_offset(&used.into()); + let declaration_offset = + helpers::get_node_offset(&variable_declaration.into()); + + if let (Some(used_offset), Some(declaration_offset)) = + (used_offset, declaration_offset) + { + if used_offset < declaration_offset { + capture!(self, context, used); + } + } + } + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Usage of variable before declaration.") + } + + fn description(&self) -> String { + String::from("This is a bad practice that may lead to unintended consequences. Please declare the variable before using it.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::PreDeclaredLocalVariableUsage.to_string() + } +} + +#[cfg(test)] +mod pre_declared_variable_usage_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, + high::pre_declared_variable_usage::PreDeclaredLocalVariableUsageDetector, + }; + + #[test] + #[serial] + fn test_pre_declared_variable_usage() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/PreDeclaredVarUsage.sol", + ); + + let mut detector = PreDeclaredLocalVariableUsageDetector::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 high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + // assert the title is correct + assert_eq!( + detector.title(), + String::from("Usage of variable before declaration.") + ); + // assert the description is correct + assert_eq!( + detector.description(), + String::from("This is a bad practice that may lead to unintended consequences. Please declare the variable before using it.") + ); + } +} diff --git a/aderyn_core/src/detect/high/reused_contract_name.rs b/aderyn_core/src/detect/high/reused_contract_name.rs index 06761bd6b..9a828124f 100644 --- a/aderyn_core/src/detect/high/reused_contract_name.rs +++ b/aderyn_core/src/detect/high/reused_contract_name.rs @@ -63,12 +63,15 @@ impl IssueDetector for ReusedContractNameDetector { #[cfg(test)] mod reused_contract_name_detector_tests { + use serial_test::serial; + use crate::detect::{ detector::IssueDetector, high::ReusedContractNameDetector, test_utils::load_multiple_solidity_source_units_into_single_context, }; #[test] + #[serial] fn test_reused_contract_name_detector() { let context = load_multiple_solidity_source_units_into_single_context(&[ "../tests/contract-playground/src/reused_contract_name/ContractA.sol", diff --git a/aderyn_core/src/detect/high/rtlo.rs b/aderyn_core/src/detect/high/rtlo.rs new file mode 100644 index 000000000..83577d0fe --- /dev/null +++ b/aderyn_core/src/detect/high/rtlo.rs @@ -0,0 +1,95 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::NodeID; + +use crate::capture; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct RTLODetector { + // 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 RTLODetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + // When you have found an instance of the issue, + // use the following macro to add it to `found_instances`: + // + // capture!(self, context, item); + + for source_unit in context.source_units() { + if let Some(content) = &source_unit.source { + if content.contains('\u{202e}') { + capture!(self, context, source_unit); + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("RTLO character detected in file. \\u{202e}") + } + + fn description(&self) -> String { + String::from("Right to left override character may be misledaing and cause potential attacks by visually misordering method arguments!") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::RTLO.to_string() + } +} + +#[cfg(test)] +mod rtlo_detector_tests { + use serial_test::serial; + + use crate::detect::{detector::IssueDetector, high::rtlo::RTLODetector}; + + #[test] + #[serial] + fn c() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/RTLO.sol", + ); + + let mut detector = RTLODetector::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 high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + // assert the title is correct + assert_eq!( + detector.title(), + String::from("RTLO character detected in file. \\u{202e}") + ); + // assert the description is correct + assert_eq!( + detector.description(), + String::from("Right to left override character may be misledaing and cause potential attacks by visually misordering method arguments!") + ); + } +} diff --git a/aderyn_core/src/detect/high/selfdestruct.rs b/aderyn_core/src/detect/high/selfdestruct.rs index 13f3531e3..57132c156 100644 --- a/aderyn_core/src/detect/high/selfdestruct.rs +++ b/aderyn_core/src/detect/high/selfdestruct.rs @@ -51,9 +51,12 @@ impl IssueDetector for SelfdestructIdentifierDetector { #[cfg(test)] mod selfdestruct_identifier_tests { + use serial_test::serial; + use crate::detect::{detector::IssueDetector, high::SelfdestructIdentifierDetector}; #[test] + #[serial] fn test_selfdestruct_identifier_tests() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/UsingSelfdestruct.sol", diff --git a/aderyn_core/src/detect/high/send_ether_no_checks.rs b/aderyn_core/src/detect/high/send_ether_no_checks.rs new file mode 100644 index 000000000..a20f35667 --- /dev/null +++ b/aderyn_core/src/detect/high/send_ether_no_checks.rs @@ -0,0 +1,121 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::NodeID; + +use crate::capture; +use crate::context::investigator::{ + StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, +}; +use crate::context::workspace_context::ASTNode; +use crate::detect::detector::IssueDetectorNamePool; +use crate::detect::helpers; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct SendEtherNoChecksDetector { + // 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 SendEtherNoChecksDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for func in helpers::get_implemented_external_and_public_functions(context) { + let mut tracker = MsgSenderAndCallWithValueTracker::default(); + let investigator = StandardInvestigator::new( + context, + &[&(func.into())], + StandardInvestigationStyle::Downstream, + )?; + investigator.investigate(context, &mut tracker)?; + + if tracker.sends_native_eth && !tracker.has_msg_sender_checks { + capture!(self, context, func); + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Sending native Eth is not protected from these functions.") + } + + fn description(&self) -> String { + String::from("Introduce checks for `msg.sender` in the function") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::SendEtherNoChecks.to_string() + } +} + +#[cfg(test)] +mod send_ether_no_checks_detector_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, high::send_ether_no_checks::SendEtherNoChecksDetector, + }; + + #[test] + #[serial] + fn test_send_ether_no_checks() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/SendEtherNoChecks.sol", + ); + + let mut detector = SendEtherNoChecksDetector::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 high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + // assert the title is correct + assert_eq!( + detector.title(), + String::from("Sending native Eth is not protected from these functions.") + ); + // assert the description is correct + assert_eq!( + detector.description(), + String::from("Introduce checks for `msg.sender` in the function") + ); + } +} + +#[derive(Default)] +pub struct MsgSenderAndCallWithValueTracker { + pub has_msg_sender_checks: bool, + pub sends_native_eth: bool, +} + +impl StandardInvestigatorVisitor for MsgSenderAndCallWithValueTracker { + fn visit_any(&mut self, node: &ASTNode) -> eyre::Result<()> { + if !self.has_msg_sender_checks && helpers::has_msg_sender_binary_operation(node) { + self.has_msg_sender_checks = true; + } + if !self.sends_native_eth && helpers::has_calls_that_sends_native_eth(node) { + self.sends_native_eth = true; + } + eyre::Ok(()) + } +} diff --git a/aderyn_core/src/detect/high/state_variable_shadowing.rs b/aderyn_core/src/detect/high/state_variable_shadowing.rs index 4f6a563ca..cf55a790f 100644 --- a/aderyn_core/src/detect/high/state_variable_shadowing.rs +++ b/aderyn_core/src/detect/high/state_variable_shadowing.rs @@ -197,9 +197,12 @@ impl IssueDetector for StateVariableShadowingDetector { #[cfg(test)] mod state_variable_shadowing_detector_tests { + use serial_test::serial; + use crate::detect::{detector::IssueDetector, high::StateVariableShadowingDetector}; #[test] + #[serial] fn test_state_variable_shadowing_detector() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/StateShadowing.sol", diff --git a/aderyn_core/src/detect/high/storage_array_edit_with_memory.rs b/aderyn_core/src/detect/high/storage_array_edit_with_memory.rs index 03edf2a53..512aa191d 100644 --- a/aderyn_core/src/detect/high/storage_array_edit_with_memory.rs +++ b/aderyn_core/src/detect/high/storage_array_edit_with_memory.rs @@ -86,12 +86,15 @@ impl IssueDetector for StorageArrayEditWithMemoryDetector { #[cfg(test)] mod storage_array_edit_with_memory_tests { + use serial_test::serial; + use crate::detect::{ detector::IssueDetector, high::storage_array_edit_with_memory::StorageArrayEditWithMemoryDetector, }; #[test] + #[serial] fn test_storage_array_edit_with_memory() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/StorageParameters.sol", diff --git a/aderyn_core/src/detect/high/tautological_compare.rs b/aderyn_core/src/detect/high/tautological_compare.rs new file mode 100644 index 000000000..57d91dfaa --- /dev/null +++ b/aderyn_core/src/detect/high/tautological_compare.rs @@ -0,0 +1,200 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::{Expression, Identifier, MemberAccess, NodeID}; + +use crate::capture; +use crate::detect::detector::IssueDetectorNamePool; +use crate::detect::helpers::get_literal_value_or_constant_variable_value; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct TautologicalCompareDetector { + // 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 TautologicalCompareDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for binary_operation in context.binary_operations().into_iter().filter(|binary_op| { + ["&&", "||", ">=", ">", "<=", "<"] + .into_iter() + .any(|op| op == binary_op.operator) + }) { + let orientations = [ + ( + binary_operation.left_expression.as_ref(), + binary_operation.right_expression.as_ref(), + ), + ( + binary_operation.right_expression.as_ref(), + binary_operation.left_expression.as_ref(), + ), + ]; + + match ( + binary_operation.left_expression.as_ref(), + binary_operation.right_expression.as_ref(), + ) { + ( + Expression::Identifier(Identifier { + referenced_declaration: Some(id0), + .. + }), + Expression::Identifier(Identifier { + referenced_declaration: Some(id1), + .. + }), + ) + | ( + Expression::MemberAccess(MemberAccess { + referenced_declaration: Some(id0), + .. + }), + Expression::MemberAccess(MemberAccess { + referenced_declaration: Some(id1), + .. + }), + ) => { + if *id0 == *id1 { + capture!(self, context, binary_operation); + } else { + let v0 = get_literal_value_or_constant_variable_value(*id0, context); + let v1 = get_literal_value_or_constant_variable_value(*id1, context); + + let is_equal_in_value = match (v0, v1) { + (Some(ref s0), Some(ref s1)) => s0 == s1, + _ => false, + }; + + if is_equal_in_value { + capture!(self, context, binary_operation); + } + } + } + _ => (), + }; + + for (lhs, rhs) in orientations { + match (lhs, rhs) { + ( + Expression::Identifier(Identifier { + referenced_declaration: Some(id0), + .. + }), + Expression::MemberAccess(MemberAccess { + referenced_declaration: Some(id1), + .. + }), + ) => { + if *id0 == *id1 { + capture!(self, context, binary_operation); + } else { + let v0 = get_literal_value_or_constant_variable_value(*id0, context); + let v1 = get_literal_value_or_constant_variable_value(*id1, context); + + let is_equal_in_value = match (v0, v1) { + (Some(ref s0), Some(ref s1)) => s0 == s1, + _ => false, + }; + + if is_equal_in_value { + capture!(self, context, binary_operation); + } + } + } + ( + Expression::Literal(literal), + Expression::MemberAccess(MemberAccess { + referenced_declaration: Some(id1), + .. + }), + ) + | ( + Expression::Literal(literal), + Expression::Identifier(Identifier { + referenced_declaration: Some(id1), + .. + }), + ) => { + let v0 = literal.value.to_owned(); + let v1 = get_literal_value_or_constant_variable_value(*id1, context); + + let is_equal_in_value = match (v0, v1) { + (Some(ref s0), Some(ref s1)) => s0 == s1, + _ => false, + }; + + if is_equal_in_value { + capture!(self, context, binary_operation); + } + } + _ => (), + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Tautological comparison.") + } + + fn description(&self) -> String { + String::from("The left hand side and the right hand side of the binary operation has the same value. This makes the condition always true or always false.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::TautologicalCompare.to_string() + } +} + +#[cfg(test)] +mod tautological_compare_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, high::tautological_compare::TautologicalCompareDetector, + }; + + #[test] + #[serial] + fn test_tatulogical_detector() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/TautologicalCompare.sol", + ); + + let mut detector = TautologicalCompareDetector::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); + // assert the severity is high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + // assert the title is correct + assert_eq!(detector.title(), String::from("Tautological comparison.")); + // assert the description is correct + assert_eq!( + detector.description(), + String::from("The left hand side and the right hand side of the binary operation has the same value. This makes the condition always true or always false.") + ); + } +} diff --git a/aderyn_core/src/detect/high/unchecked_return.rs b/aderyn_core/src/detect/high/unchecked_return.rs new file mode 100644 index 000000000..cf693f533 --- /dev/null +++ b/aderyn_core/src/detect/high/unchecked_return.rs @@ -0,0 +1,119 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::{ASTNode, Expression, Identifier, MemberAccess, NodeID, NodeType}; + +use crate::capture; +use crate::context::browser::GetImmediateParent; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct UncheckedReturnDetector { + // 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 UncheckedReturnDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + // When you have found an instance of the issue, + // use the following macro to add it to `found_instances`: + // + // capture!(self, context, item); + + for function_call in context.function_calls() { + // Find the ID of FunctionDefinition that we're calling so that we may identify if there are returned params + match function_call.expression.as_ref() { + Expression::Identifier(Identifier { + referenced_declaration: Some(id), + .. + }) + | Expression::MemberAccess(MemberAccess { + referenced_declaration: Some(id), + .. + }) => { + if function_call + .parent(context) + .is_some_and(|node| node.node_type() == NodeType::Block) + { + // Now, we know that the return value is unused + if let Some(ASTNode::FunctionDefinition(function)) = context.nodes.get(id) { + if !function.return_parameters.parameters.is_empty() { + // Now, we know that the function has no return value + capture!(self, context, function_call); + } + } + } + } + _ => (), + }; + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Return value of the function call is not checked.") + } + + fn description(&self) -> String { + String::from("Function returns a value but it is ignored.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::UncheckedReturn.to_string() + } +} + +#[cfg(test)] +mod unchecked_return_tests { + use serial_test::serial; + + use crate::detect::{detector::IssueDetector, high::unchecked_return::UncheckedReturnDetector}; + + #[test] + #[serial] + fn test_unchecked_return_detector() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/UncheckedReturn.sol", + ); + + let mut detector = UncheckedReturnDetector::default(); + let found = detector.detect(&context).unwrap(); + // assert that the detector found an issue + + println!("{:?}", detector.instances()); + + assert!(found); + // assert that the detector found the correct number of instances + assert_eq!(detector.instances().len(), 2); + // assert the severity is high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + // assert the title is correct + assert_eq!( + detector.title(), + String::from("Return value of the function call is not checked.") + ); + // assert the description is correct + assert_eq!( + detector.description(), + String::from("Function returns a value but it is ignored.") + ); + } +} diff --git a/aderyn_core/src/detect/high/uninitialized_state_variable.rs b/aderyn_core/src/detect/high/uninitialized_state_variable.rs new file mode 100644 index 000000000..5006fa5e1 --- /dev/null +++ b/aderyn_core/src/detect/high/uninitialized_state_variable.rs @@ -0,0 +1,153 @@ +use std::collections::{BTreeMap, HashSet}; +use std::error::Error; + +use crate::ast::{Expression, NodeID}; + +use crate::capture; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct UninitializedStateVariableDetector { + // 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 UninitializedStateVariableDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + /* + * Plan (Maybe it can be improved) + * - Gather all the storage variables (VariableDeclarations) + * - Fitler out / Remove the ones where `value` property is not `None` + * - Fitler out / Remove the ones that are arrays, mappings and structs + * - Now, we're left with state variables that are not initialized at the same + * line where they are declared. + * - Gather all the `Assignments` and collect all the `referencedDeclarations` on + * `Identifier` expressions when they appear on LHS of the assginments + * - Remove the above ids from the initial storage variables list + * - Now we're left with storage variables that have never been initialized + */ + + let mut state_variable_ids = HashSet::new(); + + for var_decl in context + .variable_declarations() + .into_iter() + .filter(|s| s.state_variable && s.value.is_none()) + .filter(|s| { + s.type_descriptions + .type_string + .as_ref() + .is_some_and(|type_string| !type_string.starts_with("mapping")) + }) + .filter(|s| { + s.type_descriptions + .type_string + .as_ref() + .is_some_and(|type_string| !type_string.ends_with(']')) + }) + .filter(|s| { + s.type_descriptions + .type_string + .as_ref() + .is_some_and(|type_string| !type_string.starts_with("struct")) + }) + { + state_variable_ids.insert(var_decl.id); + } + + for assignment in context.assignments() { + if let Expression::Identifier(identifier) = assignment.left_hand_side.as_ref() { + if let Some(refers_to) = identifier.referenced_declaration { + let _ = state_variable_ids.remove(&refers_to); + } + } + } + + for id in state_variable_ids { + context + .nodes + .get(&id) + .inspect(|&x| capture!(self, context, x)); + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Uninitialized State Variables") + } + + fn description(&self) -> String { + String::from( + "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." + ) + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::UninitializedStateVariable.to_string() + } +} + +#[cfg(test)] +mod uninitialized_state_variable_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, + high::uninitialized_state_variable::UninitializedStateVariableDetector, + }; + + #[test] + #[serial] + fn test_uninitialized_state_variables() { + let context: crate::context::workspace_context::WorkspaceContext = + crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/UninitializedStateVariable.sol", + ); + + let mut detector = UninitializedStateVariableDetector::default(); + let found = detector.detect(&context).unwrap(); + + println!("{:?}", detector.instances()); + + // assert that the detector found an issue + assert!(found); + // assert that the detector found the correct number of instances + assert_eq!(detector.instances().len(), 2); + // assert the severity is high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + // assert the title is correct + assert_eq!( + detector.title(), + String::from("Uninitialized State Variables") + ); + // assert the description is correct + assert_eq!( + detector.description(), + String::from( + "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." + ) + ); + } +} diff --git a/aderyn_core/src/detect/high/unsafe_casting.rs b/aderyn_core/src/detect/high/unsafe_casting.rs index 226ae59e3..5f68007fb 100644 --- a/aderyn_core/src/detect/high/unsafe_casting.rs +++ b/aderyn_core/src/detect/high/unsafe_casting.rs @@ -268,9 +268,12 @@ static BYTES32_CASTING_MAP: phf::Map<&'static str, usize> = phf_map! { #[cfg(test)] mod unsafe_casting_detector_tests { + use serial_test::serial; + use crate::detect::{detector::IssueDetector, high::UnsafeCastingDetector}; #[test] + #[serial] fn test_unsafe_casting_detector() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/Casting.sol", diff --git a/aderyn_core/src/detect/high/weak_randomness.rs b/aderyn_core/src/detect/high/weak_randomness.rs new file mode 100644 index 000000000..079fd2cc2 --- /dev/null +++ b/aderyn_core/src/detect/high/weak_randomness.rs @@ -0,0 +1,190 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::{Expression, FunctionCall, FunctionCallKind, NodeID}; + +use crate::capture; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::{ASTNode, WorkspaceContext}, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct WeakRandomnessDetector { + // 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 WeakRandomnessDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + let keccaks: Vec<&FunctionCall> = context.function_calls() + .into_iter() + .filter(|x| matches!(*x.expression, Expression::Identifier(ref id) if id.name == "keccak256")) + .collect(); + + for keccak in keccaks { + // keccak256 must have exactly one argument + if let Some(arg) = keccak.arguments.first() { + if let Expression::FunctionCall(ref function_call) = *arg { + if check_encode(function_call) { + capture!(self, context, keccak); + } + } + // get variable definition + else if let Expression::Identifier(ref i) = *arg { + if let Some(node_id) = i.referenced_declaration { + let decleration = context.get_parent(node_id); + + if let Some(ASTNode::VariableDeclarationStatement(var)) = decleration { + if let Some(Expression::FunctionCall(function_call)) = + &var.initial_value + { + if check_encode(function_call) { + capture!(self, context, keccak); + } + } + } + } + } + } + } + + // check for modulo operations on block.timestamp, block.number and blockhash + for binary_operation in context + .binary_operations() + .into_iter() + .filter(|b| b.operator == "%") + { + // if left operand is a variable, get its definition and perform check + if let Expression::Identifier(ref i) = *binary_operation.left_expression { + if let Some(node_id) = i.referenced_declaration { + let decleration = context.get_parent(node_id); + + if let Some(ASTNode::VariableDeclarationStatement(var)) = decleration { + if let Some(expression) = &var.initial_value { + if check_operand(expression) { + capture!(self, context, binary_operation); + continue; + } + } + } + } + } + // otherwise perform check directly on the expression + else if check_operand(&binary_operation.left_expression) { + capture!(self, context, binary_operation); + } + } + + // check if contract uses block.prevrandao + for member_access in context.member_accesses() { + if member_access.member_name == "prevrandao" + && matches!(*member_access.expression, Expression::Identifier(ref id) if id.name == "block") + { + capture!(self, context, member_access); + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Weak Randomness") + } + + fn description(&self) -> String { + String::from("The use of keccak256 hash functions on predictable values like block.timestamp, block.number, or similar data, including modulo operations on these values, should be avoided for generating randomness, as they are easily predictable and manipulable. The `PREVRANDAO` opcode also should not be used as a source of randomness. Instead, utilize Chainlink VRF for cryptographically secure and provably random values to ensure protocol integrity.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + format!("{}", IssueDetectorNamePool::WeakRandomness) + } +} + +// returns whether block.timestamp or block.number is used in encode function +fn check_encode(function_call: &FunctionCall) -> bool { + if let Expression::MemberAccess(ref member_access) = *function_call.expression { + if member_access.member_name == "encodePacked" || member_access.member_name == "encode" { + for argument in &function_call.arguments { + if let Expression::MemberAccess(ref member_access) = *argument { + if ["timestamp", "number"].iter().any(|ma| { + ma == &member_access.member_name && + matches!(*member_access.expression, Expression::Identifier(ref id) if id.name == "block") + }) { + return true; + } + } + } + } + } + false +} + +// returns whether operand is dependent on block.timestamp, block.number or blockhash +fn check_operand(operand: &Expression) -> bool { + match operand { + Expression::MemberAccess(member_access) => { + if ["timestamp", "number"].iter().any(|ma| { + ma == &member_access.member_name && + matches!(*member_access.expression, Expression::Identifier(ref id) if id.name == "block") + }) { + return true; + } + }, + Expression::FunctionCall(function_call) => { + if function_call.kind == FunctionCallKind::TypeConversion { + // type conversion must have exactly one argument + if let Some(Expression::FunctionCall(ref inner_function_call)) = function_call.arguments.first() { + if matches!(*inner_function_call.expression, Expression::Identifier(ref id) if id.name == "blockhash") { + return true; + } + } + } + }, + _ => () + } + + false +} + +#[cfg(test)] +mod weak_randomness_detector_tests { + use crate::detect::{detector::IssueDetector, high::weak_randomness::WeakRandomnessDetector}; + + #[test] + fn test_weak_randomness_detector() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/WeakRandomness.sol", + ); + + let mut detector = WeakRandomnessDetector::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(), 9); + // assert the severity is high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + // assert the title is correct + assert_eq!(detector.title(), String::from("Weak Randomness")); + // assert the description is correct + assert_eq!( + detector.description(), + String::from("The use of keccak256 hash functions on predictable values like block.timestamp, block.number, or similar data, including modulo operations on these values, should be avoided for generating randomness, as they are easily predictable and manipulable. The `PREVRANDAO` opcode also should not be used as a source of randomness. Instead, utilize Chainlink VRF for cryptographically secure and provably random values to ensure protocol integrity.") + ); + } +} diff --git a/aderyn_core/src/detect/high/yul_return.rs b/aderyn_core/src/detect/high/yul_return.rs index e1a5ccbf6..9bbd37650 100644 --- a/aderyn_core/src/detect/high/yul_return.rs +++ b/aderyn_core/src/detect/high/yul_return.rs @@ -52,9 +52,12 @@ impl IssueDetector for YulReturnDetector { #[cfg(test)] mod yul_return_detector_tests { + use serial_test::serial; + use crate::detect::{detector::IssueDetector, high::yul_return::YulReturnDetector}; #[test] + #[serial] fn test_yul_return() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/YulReturn.sol", diff --git a/aderyn_core/src/detect/low/_template.rs b/aderyn_core/src/detect/low/_template.rs index 21c4975bd..c3fa0ec89 100644 --- a/aderyn_core/src/detect/low/_template.rs +++ b/aderyn_core/src/detect/low/_template.rs @@ -57,9 +57,12 @@ impl IssueDetector for TemplateDetector { #[cfg(test)] mod template_detector_tests { + use serial_test::serial; + use crate::detect::{detector::IssueDetector, low::template_detector::TemplateDetector}; #[test] + #[serial] fn test_template_detector() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/ArbitraryTransferFrom.sol", diff --git a/aderyn_core/src/detect/low/division_before_multiplication.rs b/aderyn_core/src/detect/low/division_before_multiplication.rs index 759ce5805..05c66ee45 100644 --- a/aderyn_core/src/detect/low/division_before_multiplication.rs +++ b/aderyn_core/src/detect/low/division_before_multiplication.rs @@ -58,10 +58,13 @@ impl IssueDetector for DivisionBeforeMultiplicationDetector { #[cfg(test)] mod division_before_multiplication_detector_tests { + use serial_test::serial; + use super::DivisionBeforeMultiplicationDetector; use crate::detect::detector::IssueDetector; #[test] + #[serial] fn test_template_detector() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/DivisionBeforeMultiplication.sol", diff --git a/aderyn_core/src/detect/low/reverts_and_requries_in_loops.rs b/aderyn_core/src/detect/low/reverts_and_requries_in_loops.rs index a8f0d59a1..937e5969b 100644 --- a/aderyn_core/src/detect/low/reverts_and_requries_in_loops.rs +++ b/aderyn_core/src/detect/low/reverts_and_requries_in_loops.rs @@ -65,12 +65,15 @@ impl IssueDetector for RevertsAndRequiresInLoopsDetector { #[cfg(test)] mod reevrts_and_requires_in_loops { + use serial_test::serial; + use crate::detect::{ detector::IssueDetector, low::reverts_and_requries_in_loops::RevertsAndRequiresInLoopsDetector, }; #[test] + #[serial] fn test_reverts_and_requires_in_loops_by_loading_contract_directly() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/RevertsAndRequriesInLoops.sol", diff --git a/aderyn_core/src/detect/low/useless_error.rs b/aderyn_core/src/detect/low/useless_error.rs index d675dfc42..a48db6d03 100644 --- a/aderyn_core/src/detect/low/useless_error.rs +++ b/aderyn_core/src/detect/low/useless_error.rs @@ -67,11 +67,14 @@ impl IssueDetector for UselessErrorDetector { #[cfg(test)] mod useless_error_tests { + use serial_test::serial; + use crate::detect::detector::IssueDetector; use super::UselessErrorDetector; #[test] + #[serial] fn test_unused_error_detection() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/UnusedError.sol", diff --git a/aderyn_core/src/detect/mod.rs b/aderyn_core/src/detect/mod.rs index 467a84bb3..fc80fd369 100644 --- a/aderyn_core/src/detect/mod.rs +++ b/aderyn_core/src/detect/mod.rs @@ -24,4 +24,74 @@ macro_rules! capture { }; } +#[macro_export] +macro_rules! issue_detector { + ( + $detector_struct:ident; + + severity: $detector_severity:ident, + title: $detector_title:tt, + desc: $detector_desc:tt, + name: $detector_name:ident, + + |$context: ident| $e:expr + ) => { + + #[derive(Default)] + pub struct $detector_struct { + found_instances: std::collections::BTreeMap<(String, usize, String), $crate::ast::NodeID>, + } + + impl $crate::detect::detector::IssueDetector for $detector_struct { + + fn detect(&mut self, context: &$crate::context::workspace_context::WorkspaceContext) -> Result> { + + let $context = context; + + macro_rules! grab { + ($item:expr) => { + if let Some(id) = context.get_node_id_of_capturable(&$item.clone().into()) { + self.found_instances.insert( + $context.get_node_sort_key_from_capturable(&$item.clone().into()), + id, + ); + } else { + self.found_instances.insert( + $context.get_node_sort_key_from_capturable(&$item.clone().into()), + 0, + ); + } + }; + } + + $e + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> $crate::detect::detector::IssueSeverity { + $crate::detect::detector::IssueSeverity::$detector_severity + } + + fn title(&self) -> String { + String::from($detector_title) + } + + fn description(&self) -> String { + String::from($detector_desc) + } + + fn instances(&self) -> std::collections::BTreeMap<(String, usize, String), $crate::ast::NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + $crate::detect::detector::IssueDetectorNamePool::$detector_name.to_string() + } + } + + }; +} + pub use capture; + +pub use issue_detector; diff --git a/aderyn_core/src/detect/test_utils/load_source_unit.rs b/aderyn_core/src/detect/test_utils/load_source_unit.rs index 1b4f1dc38..31844c086 100644 --- a/aderyn_core/src/detect/test_utils/load_source_unit.rs +++ b/aderyn_core/src/detect/test_utils/load_source_unit.rs @@ -5,13 +5,27 @@ use std::{ sync::Arc, }; -use crate::visitor::ast_visitor::Node; -use crate::{ast::SourceUnit, context::workspace_context::WorkspaceContext}; +use crate::{ + ast::SourceUnit, + context::{graph::traits::Transpose, workspace_context::WorkspaceContext}, +}; +use crate::{context::graph::WorkspaceCallGraph, visitor::ast_visitor::Node}; use super::ensure_valid_solidity_file; +#[cfg(test)] +pub fn load_solidity_source_unit_with_callgraphs(filepath: &str) -> WorkspaceContext { + _load_solidity_source_unit(filepath, true) +} + #[cfg(test)] pub fn load_solidity_source_unit(filepath: &str) -> WorkspaceContext { + println!("WARNING: Callgraph won't be loaded. Please use `load_solidity_source_unit_with_callgraphs` if callgraph is required!"); + _load_solidity_source_unit(filepath, false) +} + +#[cfg(test)] +fn _load_solidity_source_unit(filepath: &str, should_load_callgraphs: bool) -> WorkspaceContext { let solidity_file = &ensure_valid_solidity_file(filepath); let solidity_content = std::fs::read_to_string(solidity_file).unwrap(); @@ -81,6 +95,10 @@ pub fn load_solidity_source_unit(filepath: &str) -> WorkspaceContext { absorb_ast_content_into_context(&ast_content, solidity_content.clone(), &mut context); } + if should_load_callgraphs { + load_callgraphs(&mut context); + } + context } else { eprintln!("Error running solc command"); @@ -88,6 +106,15 @@ pub fn load_solidity_source_unit(filepath: &str) -> WorkspaceContext { } } +fn load_callgraphs(context: &mut WorkspaceContext) { + let forward_callgraph = WorkspaceCallGraph::from_context(context).unwrap(); + let reverse_callgraph = WorkspaceCallGraph { + graph: forward_callgraph.graph.reverse(), + }; + context.forward_callgraph = Some(forward_callgraph); + context.reverse_callgraph = Some(reverse_callgraph); +} + fn absorb_ast_content_into_context( ast_content: &str, solidity_content: String, diff --git a/aderyn_core/src/detect/test_utils/mod.rs b/aderyn_core/src/detect/test_utils/mod.rs index 10a76c7fd..b10ddc7df 100644 --- a/aderyn_core/src/detect/test_utils/mod.rs +++ b/aderyn_core/src/detect/test_utils/mod.rs @@ -7,6 +7,7 @@ use std::path::PathBuf; // Using `solc` to read AST given a source unit (i.e Solidity file) pub use load_source_unit::load_multiple_solidity_source_units_into_single_context; pub use load_source_unit::load_solidity_source_unit; +pub use load_source_unit::load_solidity_source_unit_with_callgraphs; pub(crate) fn take_loader_lock() -> impl Drop { static LOCK: Lazy> = Lazy::new(|| std::sync::Mutex::new(())); diff --git a/aderyn_driver/Cargo.toml b/aderyn_driver/Cargo.toml index 545242bb1..edebe3121 100644 --- a/aderyn_driver/Cargo.toml +++ b/aderyn_driver/Cargo.toml @@ -1,15 +1,15 @@ [package] name = "aderyn_driver" -version = "0.1.7" +version = "0.1.8" edition = "2021" -authors = ["Alex Roan "] +authors = ["Cyfrin "] description = "Rust based Solidity AST analyzer driver" license = "MIT" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -aderyn_core = { path = "../aderyn_core", version = "0.1.7" } +aderyn_core = { path = "../aderyn_core", version = "0.1.8" } rayon = "1.8.0" cyfrin-foundry-compilers = { version = "0.3.20-aderyn", features = ["svm-solc"] } serde_json = { version = "1.0.96", features = ["preserve_order"] } diff --git a/aderyn_driver/src/driver.rs b/aderyn_driver/src/driver.rs index 2f6c3e68d..3abef9b39 100644 --- a/aderyn_driver/src/driver.rs +++ b/aderyn_driver/src/driver.rs @@ -2,8 +2,9 @@ use crate::{ config_helpers::{append_from_foundry_toml, derive_from_aderyn_toml}, ensure_valid_root_path, process_auto, }; +use aderyn_core::context::graph::traits::Transpose; use aderyn_core::{ - context::workspace_context::WorkspaceContext, + context::{graph::WorkspaceCallGraph, workspace_context::WorkspaceContext}, detect::detector::{get_all_issue_detectors, IssueDetector, IssueSeverity}, fscloc, report::{ @@ -152,6 +153,13 @@ fn make_context(args: &Args) -> WorkspaceContextWrapper { // dbg!(&stats); context.set_sloc_stats(sloc_stats); context.set_ignore_lines_stats(ignore_line_stats); + + let forward_callgraph = WorkspaceCallGraph::from_context(context).unwrap(); + let reverse_callgraph = WorkspaceCallGraph { + graph: forward_callgraph.graph.reverse(), + }; + context.forward_callgraph = Some(forward_callgraph); + context.reverse_callgraph = Some(reverse_callgraph); } // Using the source path, calculate the sloc diff --git a/aderyn_py/Cargo.toml b/aderyn_py/Cargo.toml index cd67982ad..90a44c0e8 100644 --- a/aderyn_py/Cargo.toml +++ b/aderyn_py/Cargo.toml @@ -1,8 +1,8 @@ [package] name = "aderyn_py" -version = "0.1.7" +version = "0.1.8" edition = "2021" -authors = ["Alex Roan "] +authors = ["Cyfrin "] description = "Rust based Solidity AST analyzer python bindings" license = "MIT" @@ -14,7 +14,7 @@ name = "aderynpy" crate-type = ["cdylib"] [dependencies] -aderyn_driver = { path = "../aderyn_driver", version = "0.1.7" } +aderyn_driver = { path = "../aderyn_driver", version = "0.1.8" } [dependencies.pyo3] version = "0.19.0" diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 1d57375f2..5b6f13587 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -88,7 +88,7 @@ ] }, "issue_count": { - "high": 1, + "high": 3, "low": 0 }, "high_issues": { @@ -105,6 +105,56 @@ "src_char": "488:19" } ] + }, + { + "title": "Uninitialized State Variables", + "description": "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.", + "detector_name": "uninitialized-state-variable", + "instances": [ + { + "contract_path": "InconsistentUints.sol", + "line_no": 7, + "src": "197:11", + "src_char": "197:11" + }, + { + "contract_path": "InconsistentUints.sol", + "line_no": 8, + "src": "233:14", + "src_char": "233:14" + }, + { + "contract_path": "StateVariables.sol", + "line_no": 8, + "src": "199:19", + "src_char": "199:19" + }, + { + "contract_path": "StateVariables.sol", + "line_no": 9, + "src": "241:20", + "src_char": "241:20" + }, + { + "contract_path": "StateVariables.sol", + "line_no": 10, + "src": "282:18", + "src_char": "282:18" + } + ] + }, + { + "title": "Delegatecall made by the function without checks on any adress.", + "description": "Introduce checks on the address", + "detector_name": "delegate-call-unchecked-address", + "instances": [ + { + "contract_path": "inheritance/ExtendedInheritance.sol", + "line_no": 14, + "src": "391:15", + "src_char": "391:15" + } + ] } ] }, @@ -127,8 +177,19 @@ "nested-struct-in-mapping", "selfdestruct-identifier", "dynamic-array-length-assignment", + "uninitialized-state-variable", "incorrect-caret-operator", "yul-return", - "state-variable-shadowing" + "state-variable-shadowing", + "misused-boolean", + "send-ether-no-checks", + "delegate-call-unchecked-address", + "tautological-compare", + "rtlo", + "unchecked-return", + "dangerous-unary-operator", + "weak-randomness", + "pre-declared-local-variable-usage", + "delete-nested-mapping" ] } \ No newline at end of file diff --git a/reports/adhoc-sol-files-report.md b/reports/adhoc-sol-files-report.md index fe7c59c2d..17dbae04d 100644 --- a/reports/adhoc-sol-files-report.md +++ b/reports/adhoc-sol-files-report.md @@ -9,6 +9,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [Issue Summary](#issue-summary) - [High Issues](#high-issues) - [H-1: Using `delegatecall` in loop](#h-1-using-delegatecall-in-loop) + - [H-2: Uninitialized State Variables](#h-2-uninitialized-state-variables) + - [H-3: Delegatecall made by the function without checks on any adress.](#h-3-delegatecall-made-by-the-function-without-checks-on-any-adress) - [Low Issues](#low-issues) - [L-1: Centralization Risk for trusted owners](#l-1-centralization-risk-for-trusted-owners) - [L-2: `ecrecover` is susceptible to signature malleability](#l-2-ecrecover-is-susceptible-to-signature-malleability) @@ -69,7 +71,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 1 | +| High | 3 | | Low | 16 | @@ -92,6 +94,64 @@ When calling `delegatecall` the same `msg.value` amount will be accredited multi +## H-2: Uninitialized State Variables + +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. + +
5 Found Instances + + +- Found in InconsistentUints.sol [Line: 7](../tests/adhoc-sol-files/InconsistentUints.sol#L7) + + ```solidity + int public intVariable; // 1 + ``` + +- Found in InconsistentUints.sol [Line: 8](../tests/adhoc-sol-files/InconsistentUints.sol#L8) + + ```solidity + int256 public int256Variable; // 1 + ``` + +- Found in StateVariables.sol [Line: 8](../tests/adhoc-sol-files/StateVariables.sol#L8) + + ```solidity + uint256 private staticPrivateNumber; + ``` + +- Found in StateVariables.sol [Line: 9](../tests/adhoc-sol-files/StateVariables.sol#L9) + + ```solidity + uint256 internal staticInternalNumber; + ``` + +- Found in StateVariables.sol [Line: 10](../tests/adhoc-sol-files/StateVariables.sol#L10) + + ```solidity + uint256 public staticPublicNumber; + ``` + +
+ + + +## H-3: Delegatecall made by the function without checks on any adress. + +Introduce checks on the address + +
1 Found Instances + + +- Found in inheritance/ExtendedInheritance.sol [Line: 14](../tests/adhoc-sol-files/inheritance/ExtendedInheritance.sol#L14) + + ```solidity + function doSomethingElse(address target) external { + ``` + +
+ + + # Low Issues ## L-1: Centralization Risk for trusted owners diff --git a/reports/ccip-functions-report.md b/reports/ccip-functions-report.md index c1458888a..b6e995435 100644 --- a/reports/ccip-functions-report.md +++ b/reports/ccip-functions-report.md @@ -10,6 +10,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [High Issues](#high-issues) - [H-1: Unprotected initializer](#h-1-unprotected-initializer) - [H-2: Contract Name Reused in Different Files](#h-2-contract-name-reused-in-different-files) + - [H-3: Uninitialized State Variables](#h-3-uninitialized-state-variables) - [Low Issues](#low-issues) - [L-1: Centralization Risk for trusted owners](#l-1-centralization-risk-for-trusted-owners) - [L-2: `ecrecover` is susceptible to signature malleability](#l-2-ecrecover-is-susceptible-to-signature-malleability) @@ -99,7 +100,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 2 | +| High | 3 | | Low | 13 | @@ -463,6 +464,29 @@ When compiling contracts with certain development frameworks (for example: Truff +## H-3: Uninitialized State Variables + +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. + +
2 Found Instances + + +- Found in src/v0.8/functions/dev/v1_X/FunctionsSubscriptions.sol [Line: 39](../tests/ccip-contracts/contracts/src/v0.8/functions/dev/v1_X/FunctionsSubscriptions.sol#L39) + + ```solidity + uint64 private s_currentSubscriptionId; + ``` + +- Found in src/v0.8/functions/v1_0_0/FunctionsSubscriptions.sol [Line: 39](../tests/ccip-contracts/contracts/src/v0.8/functions/v1_0_0/FunctionsSubscriptions.sol#L39) + + ```solidity + uint64 private s_currentSubscriptionId; + ``` + +
+ + + # Low Issues ## L-1: Centralization Risk for trusted owners diff --git a/reports/report.json b/reports/report.json index 9f4a6ce5f..1e7161940 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 55, - "total_sloc": 1521 + "total_source_units": 67, + "total_sloc": 1904 }, "files_details": { "files_details": [ @@ -21,6 +21,10 @@ "file_path": "src/AssemblyExample.sol", "n_sloc": 9 }, + { + "file_path": "src/CallGraphTests.sol", + "n_sloc": 49 + }, { "file_path": "src/Casting.sol", "n_sloc": 126 @@ -41,6 +45,18 @@ "file_path": "src/CrazyPragma.sol", "n_sloc": 4 }, + { + "file_path": "src/DangerousUnaryOperator.sol", + "n_sloc": 13 + }, + { + "file_path": "src/DelegateCallWithoutAddressCheck.sol", + "n_sloc": 31 + }, + { + "file_path": "src/DeletionNestedMappingStructureContract.sol", + "n_sloc": 11 + }, { "file_path": "src/DeprecatedOZFunctions.sol", "n_sloc": 32 @@ -89,6 +105,10 @@ "file_path": "src/KeccakContract.sol", "n_sloc": 21 }, + { + "file_path": "src/MisusedBoolean.sol", + "n_sloc": 67 + }, { "file_path": "src/MultipleConstructorSchemes.sol", "n_sloc": 10 @@ -97,10 +117,22 @@ "file_path": "src/OnceModifierExample.sol", "n_sloc": 8 }, + { + "file_path": "src/PreDeclaredVarUsage.sol", + "n_sloc": 9 + }, + { + "file_path": "src/RTLO.sol", + "n_sloc": 7 + }, { "file_path": "src/RevertsAndRequriesInLoops.sol", "n_sloc": 27 }, + { + "file_path": "src/SendEtherNoChecks.sol", + "n_sloc": 58 + }, { "file_path": "src/StateShadowing.sol", "n_sloc": 17 @@ -121,10 +153,22 @@ "file_path": "src/T11sTranferer.sol", "n_sloc": 8 }, + { + "file_path": "src/TautologicalCompare.sol", + "n_sloc": 17 + }, { "file_path": "src/TestERC20.sol", "n_sloc": 62 }, + { + "file_path": "src/UncheckedReturn.sol", + "n_sloc": 33 + }, + { + "file_path": "src/UninitializedStateVariable.sol", + "n_sloc": 29 + }, { "file_path": "src/UnprotectedInitialize.sol", "n_sloc": 25 @@ -141,6 +185,10 @@ "file_path": "src/UsingSelfdestruct.sol", "n_sloc": 6 }, + { + "file_path": "src/WeakRandomness.sol", + "n_sloc": 59 + }, { "file_path": "src/WrongOrderOfLayout.sol", "n_sloc": 13 @@ -228,7 +276,7 @@ ] }, "issue_count": { - "high": 18, + "high": 29, "low": 23 }, "high_issues": { @@ -1184,6 +1232,91 @@ } ] }, + { + "title": "Uninitialized State Variables", + "description": "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.", + "detector_name": "uninitialized-state-variable", + "instances": [ + { + "contract_path": "src/AssemblyExample.sol", + "line_no": 5, + "src": "97:1", + "src_char": "97:1" + }, + { + "contract_path": "src/DelegateCallWithoutAddressCheck.sol", + "line_no": 9, + "src": "337:7", + "src_char": "337:7" + }, + { + "contract_path": "src/InconsistentUints.sol", + "line_no": 7, + "src": "197:11", + "src_char": "197:11" + }, + { + "contract_path": "src/InconsistentUints.sol", + "line_no": 8, + "src": "233:14", + "src_char": "233:14" + }, + { + "contract_path": "src/IncorrectCaretOperator.sol", + "line_no": 10, + "src": "355:7", + "src_char": "355:7" + }, + { + "contract_path": "src/StateShadowing.sol", + "line_no": 5, + "src": "87:13", + "src_char": "87:13" + }, + { + "contract_path": "src/StateVariables.sol", + "line_no": 8, + "src": "199:19", + "src_char": "199:19" + }, + { + "contract_path": "src/StateVariables.sol", + "line_no": 9, + "src": "241:20", + "src_char": "241:20" + }, + { + "contract_path": "src/StateVariables.sol", + "line_no": 10, + "src": "282:18", + "src_char": "282:18" + }, + { + "contract_path": "src/UninitializedStateVariable.sol", + "line_no": 7, + "src": "122:8", + "src_char": "122:8" + }, + { + "contract_path": "src/UninitializedStateVariable.sol", + "line_no": 15, + "src": "529:11", + "src_char": "529:11" + }, + { + "contract_path": "src/WrongOrderOfLayout.sol", + "line_no": 11, + "src": "257:10", + "src_char": "257:10" + }, + { + "contract_path": "src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol", + "line_no": 68, + "src": "1971:5", + "src_char": "1971:5" + } + ] + }, { "title": "Incorrect use of caret operator on a non hexadcimal constant", "description": "The caret operator is usually mistakenly thought of as an exponentiation operator but actually, it's a bitwise xor operator.", @@ -1246,6 +1379,304 @@ "src_char": "239:13" } ] + }, + { + "title": "Misused boolean with logical operators", + "description": "The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively.", + "detector_name": "misused-boolean", + "instances": [ + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 12, + "src": "257:19", + "src_char": "257:19" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 19, + "src": "419:20", + "src_char": "419:20" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 26, + "src": "582:20", + "src_char": "582:20" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 33, + "src": "745:19", + "src_char": "745:19" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 40, + "src": "908:51", + "src_char": "908:51" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 47, + "src": "1060:52", + "src_char": "1060:52" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 54, + "src": "1213:53", + "src_char": "1213:53" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 61, + "src": "1366:21", + "src_char": "1366:21" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 68, + "src": "1530:17", + "src_char": "1530:17" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 75, + "src": "1691:18", + "src_char": "1691:18" + } + ] + }, + { + "title": "Sending native Eth is not protected from these functions.", + "description": "Introduce checks for `msg.sender` in the function", + "detector_name": "send-ether-no-checks", + "instances": [ + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 38, + "src": "686:16", + "src_char": "686:16" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 53, + "src": "1060:5", + "src_char": "1060:5" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 77, + "src": "1405:5", + "src_char": "1405:5" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 99, + "src": "1795:5", + "src_char": "1795:5" + }, + { + "contract_path": "src/UninitializedStateVariable.sol", + "line_no": 17, + "src": "563:8", + "src_char": "563:8" + } + ] + }, + { + "title": "Delegatecall made by the function without checks on any adress.", + "description": "Introduce checks on the address", + "detector_name": "delegate-call-unchecked-address", + "instances": [ + { + "contract_path": "src/DelegateCallWithoutAddressCheck.sol", + "line_no": 15, + "src": "392:9", + "src_char": "392:9" + }, + { + "contract_path": "src/auditor_mode/ExternalCalls.sol", + "line_no": 38, + "src": "1253:28", + "src_char": "1253:28" + }, + { + "contract_path": "src/inheritance/ExtendedInheritance.sol", + "line_no": 14, + "src": "391:15", + "src_char": "391:15" + } + ] + }, + { + "title": "Tautological comparison.", + "description": "The left hand side and the right hand side of the binary operation has the same value. This makes the condition always true or always false.", + "detector_name": "tautological-compare", + "instances": [ + { + "contract_path": "src/TautologicalCompare.sol", + "line_no": 13, + "src": "255:6", + "src_char": "255:6" + }, + { + "contract_path": "src/TautologicalCompare.sol", + "line_no": 18, + "src": "359:6", + "src_char": "359:6" + }, + { + "contract_path": "src/TautologicalCompare.sol", + "line_no": 23, + "src": "463:5", + "src_char": "463:5" + }, + { + "contract_path": "src/TautologicalCompare.sol", + "line_no": 28, + "src": "566:5", + "src_char": "566:5" + } + ] + }, + { + "title": "RTLO character detected in file. \\u{202e}", + "description": "Right to left override character may be misledaing and cause potential attacks by visually misordering method arguments!", + "detector_name": "rtlo", + "instances": [ + { + "contract_path": "src/RTLO.sol", + "line_no": 3, + "src": "33:157", + "src_char": "33:155" + } + ] + }, + { + "title": "Return value of the function call is not checked.", + "description": "Function returns a value but it is ignored.", + "detector_name": "unchecked-return", + "instances": [ + { + "contract_path": "src/UncheckedReturn.sol", + "line_no": 14, + "src": "279:5", + "src_char": "279:5" + }, + { + "contract_path": "src/UncheckedReturn.sol", + "line_no": 27, + "src": "575:47", + "src_char": "575:47" + } + ] + }, + { + "title": "Dangerous unary operator found in assignment.", + "description": "Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in between.", + "detector_name": "dangerous-unary-operator", + "instances": [ + { + "contract_path": "src/DangerousUnaryOperator.sol", + "line_no": 12, + "src": "220:10", + "src_char": "220:10" + }, + { + "contract_path": "src/DangerousUnaryOperator.sol", + "line_no": 13, + "src": "247:10", + "src_char": "247:10" + } + ] + }, + { + "title": "Weak Randomness", + "description": "The use of keccak256 hash functions on predictable values like block.timestamp, block.number, or similar data, including modulo operations on these values, should be avoided for generating randomness, as they are easily predictable and manipulable. The `PREVRANDAO` opcode also should not be used as a source of randomness. Instead, utilize Chainlink VRF for cryptographically secure and provably random values to ensure protocol integrity.", + "detector_name": "weak-randomness", + "instances": [ + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 6, + "src": "188:70", + "src_char": "188:70" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 11, + "src": "386:41", + "src_char": "386:41" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 16, + "src": "597:20", + "src_char": "597:20" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 21, + "src": "793:20", + "src_char": "793:20" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 25, + "src": "915:20", + "src_char": "915:20" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 31, + "src": "1095:5", + "src_char": "1095:5" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 35, + "src": "1217:37", + "src_char": "1217:37" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 41, + "src": "1434:9", + "src_char": "1434:9" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 45, + "src": "1563:16", + "src_char": "1563:16" + } + ] + }, + { + "title": "Usage of variable before declaration.", + "description": "This is a bad practice that may lead to unintended consequences. Please declare the variable before using it.", + "detector_name": "pre-declared-local-variable-usage", + "instances": [ + { + "contract_path": "src/PreDeclaredVarUsage.sol", + "line_no": 8, + "src": "196:1", + "src_char": "196:1" + } + ] + }, + { + "title": "Deletion from a nested mappping.", + "description": "A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract.", + "detector_name": "delete-nested-mapping", + "instances": [ + { + "contract_path": "src/DeletionNestedMappingStructureContract.sol", + "line_no": 17, + "src": "426:25", + "src_char": "426:25" + } + ] } ] }, @@ -1434,11 +1865,23 @@ "src": "1598:18", "src_char": "1598:18" }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 67, + "src": "1255:19", + "src_char": "1255:19" + }, { "contract_path": "src/StateShadowing.sol", "line_no": 22, "src": "368:19", "src_char": "368:19" + }, + { + "contract_path": "src/UninitializedStateVariable.sol", + "line_no": 18, + "src": "599:29", + "src_char": "599:29" } ] }, @@ -1465,12 +1908,36 @@ "src": "32:32", "src_char": "32:32" }, + { + "contract_path": "src/DangerousUnaryOperator.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, + { + "contract_path": "src/DelegateCallWithoutAddressCheck.sol", + "line_no": 2, + "src": "32:21", + "src_char": "32:21" + }, + { + "contract_path": "src/DeletionNestedMappingStructureContract.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/InconsistentUints.sol", "line_no": 1, "src": "0:24", "src_char": "0:24" }, + { + "contract_path": "src/PreDeclaredVarUsage.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/UsingSelfdestruct.sol", "line_no": 2, @@ -1617,6 +2084,12 @@ "src": "2539:25", "src_char": "2539:25" }, + { + "contract_path": "src/UninitializedStateVariable.sol", + "line_no": 17, + "src": "563:8", + "src_char": "563:8" + }, { "contract_path": "src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol", "line_no": 16, @@ -1768,6 +2241,24 @@ "src": "1275:66", "src_char": "1275:66" }, + { + "contract_path": "src/DelegateCallWithoutAddressCheck.sol", + "line_no": 24, + "src": "718:1", + "src_char": "718:1" + }, + { + "contract_path": "src/DelegateCallWithoutAddressCheck.sol", + "line_no": 24, + "src": "771:1", + "src_char": "771:1" + }, + { + "contract_path": "src/DelegateCallWithoutAddressCheck.sol", + "line_no": 26, + "src": "838:1", + "src_char": "838:1" + }, { "contract_path": "src/DynamicArrayLengthAssignment.sol", "line_no": 13, @@ -1798,6 +2289,18 @@ "src": "606:2", "src_char": "606:2" }, + { + "contract_path": "src/PreDeclaredVarUsage.sol", + "line_no": 8, + "src": "200:3", + "src_char": "200:3" + }, + { + "contract_path": "src/PreDeclaredVarUsage.sol", + "line_no": 9, + "src": "222:3", + "src_char": "222:3" + }, { "contract_path": "src/RevertsAndRequriesInLoops.sol", "line_no": 10, @@ -1822,6 +2325,48 @@ "src": "745:2", "src_char": "745:2" }, + { + "contract_path": "src/UncheckedReturn.sol", + "line_no": 27, + "src": "607:7", + "src_char": "607:7" + }, + { + "contract_path": "src/UncheckedReturn.sol", + "line_no": 32, + "src": "805:7", + "src_char": "805:7" + }, + { + "contract_path": "src/UncheckedReturn.sol", + "line_no": 37, + "src": "995:7", + "src_char": "995:7" + }, + { + "contract_path": "src/UncheckedReturn.sol", + "line_no": 42, + "src": "1190:7", + "src_char": "1190:7" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 25, + "src": "933:2", + "src_char": "933:2" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 35, + "src": "1252:2", + "src_char": "1252:2" + }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 41, + "src": "1441:2", + "src_char": "1441:2" + }, { "contract_path": "src/eth2/DepositContract.sol", "line_no": 113, @@ -1853,6 +2398,18 @@ "src": "413:70", "src_char": "413:70" }, + { + "contract_path": "src/UncheckedReturn.sol", + "line_no": 17, + "src": "297:30", + "src_char": "297:30" + }, + { + "contract_path": "src/UninitializedStateVariable.sol", + "line_no": 21, + "src": "700:27", + "src_char": "700:27" + }, { "contract_path": "src/eth2/DepositContract.sol", "line_no": 19, @@ -1878,6 +2435,36 @@ "description": "Use descriptive reason strings or custom errors for revert paths.", "detector_name": "require-with-string", "instances": [ + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 7, + "src": "128:7", + "src_char": "128:7" + }, + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 28, + "src": "531:6", + "src_char": "531:6" + }, + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 50, + "src": "936:6", + "src_char": "936:6" + }, + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 65, + "src": "1246:7", + "src_char": "1246:7" + }, + { + "contract_path": "src/DelegateCallWithoutAddressCheck.sol", + "line_no": 31, + "src": "948:7", + "src_char": "948:7" + }, { "contract_path": "src/DeprecatedOZFunctions.sol", "line_no": 37, @@ -1896,6 +2483,24 @@ "src": "503:6", "src_char": "503:6" }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 12, + "src": "268:6", + "src_char": "268:6" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 27, + "src": "513:7", + "src_char": "513:7" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 43, + "src": "920:6", + "src_char": "920:6" + }, { "contract_path": "src/StateShadowing.sol", "line_no": 8, @@ -2007,6 +2612,18 @@ "src": "32:32", "src_char": "32:32" }, + { + "contract_path": "src/DelegateCallWithoutAddressCheck.sol", + "line_no": 2, + "src": "32:21", + "src_char": "32:21" + }, + { + "contract_path": "src/DeletionNestedMappingStructureContract.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/DeprecatedOZFunctions.sol", "line_no": 2, @@ -2043,12 +2660,24 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/TautologicalCompare.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/UnsafeERC721Mint.sol", "line_no": 2, "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/WeakRandomness.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/cloc/AnotherHeavilyCommentedContract.sol", "line_no": 6, @@ -2128,6 +2757,30 @@ "description": "", "detector_name": "useless-modifier", "instances": [ + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 10, + "src": "186:22", + "src_char": "186:22" + }, + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 32, + "src": "571:22", + "src_char": "571:22" + }, + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 54, + "src": "976:22", + "src_char": "976:22" + }, + { + "contract_path": "src/DelegateCallWithoutAddressCheck.sol", + "line_no": 23, + "src": "678:9", + "src_char": "678:9" + }, { "contract_path": "src/InternalFunctions.sol", "line_no": 18, @@ -2140,6 +2793,30 @@ "src": "103:8", "src_char": "103:8" }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 16, + "src": "308:4", + "src_char": "308:4" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 47, + "src": "960:4", + "src_char": "960:4" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 70, + "src": "1301:4", + "src_char": "1301:4" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 93, + "src": "1704:4", + "src_char": "1704:4" + }, { "contract_path": "src/StateShadowing.sol", "line_no": 7, @@ -2165,6 +2842,18 @@ "src": "457:23", "src_char": "457:23" }, + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 16, + "src": "291:16", + "src_char": "291:16" + }, + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 38, + "src": "686:16", + "src_char": "686:16" + }, { "contract_path": "src/ContractWithTodo.sol", "line_no": 7, @@ -2225,6 +2914,24 @@ "src": "147:7", "src_char": "147:7" }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 53, + "src": "1060:5", + "src_char": "1060:5" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 77, + "src": "1405:5", + "src_char": "1405:5" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 99, + "src": "1795:5", + "src_char": "1795:5" + }, { "contract_path": "src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol", "line_no": 11, @@ -2425,12 +3132,66 @@ "description": "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.", "detector_name": "useless-internal-function", "instances": [ + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 6, + "src": "89:17", + "src_char": "89:17" + }, + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 25, + "src": "398:17", + "src_char": "398:17" + }, + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 47, + "src": "803:17", + "src_char": "803:17" + }, + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 64, + "src": "1206:18", + "src_char": "1206:18" + }, { "contract_path": "src/InternalFunctions.sol", "line_no": 28, "src": "693:12", "src_char": "693:12" }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 9, + "src": "132:20", + "src_char": "132:20" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 26, + "src": "481:5", + "src_char": "481:5" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 40, + "src": "784:20", + "src_char": "784:20" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 66, + "src": "1209:15", + "src_char": "1209:15" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 88, + "src": "1551:11", + "src_char": "1551:11" + }, { "contract_path": "src/StorageParameters.sol", "line_no": 17, @@ -2517,6 +3278,12 @@ "src": "528:12", "src_char": "528:12" }, + { + "contract_path": "src/TautologicalCompare.sol", + "line_no": 11, + "src": "186:1", + "src_char": "186:1" + }, { "contract_path": "src/eth2/DepositContract.sol", "line_no": 59, @@ -2695,8 +3462,19 @@ "nested-struct-in-mapping", "selfdestruct-identifier", "dynamic-array-length-assignment", + "uninitialized-state-variable", "incorrect-caret-operator", "yul-return", - "state-variable-shadowing" + "state-variable-shadowing", + "misused-boolean", + "send-ether-no-checks", + "delegate-call-unchecked-address", + "tautological-compare", + "rtlo", + "unchecked-return", + "dangerous-unary-operator", + "weak-randomness", + "pre-declared-local-variable-usage", + "delete-nested-mapping" ] } \ No newline at end of file diff --git a/reports/report.md b/reports/report.md index 22439e871..d2cd2eeca 100644 --- a/reports/report.md +++ b/reports/report.md @@ -23,9 +23,20 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-13: Nested Structs in Mappings pre-0.5.0](#h-13-nested-structs-in-mappings-pre-050) - [H-14: Depracated EVM Instruction for `selfdestruct` should not be used.](#h-14-depracated-evm-instruction-for-selfdestruct-should-not-be-used) - [H-15: Array length value has a direct assignment.](#h-15-array-length-value-has-a-direct-assignment) - - [H-16: Incorrect use of caret operator on a non hexadcimal constant](#h-16-incorrect-use-of-caret-operator-on-a-non-hexadcimal-constant) - - [H-17: Yul block contains `return` function call.](#h-17-yul-block-contains-return-function-call) - - [H-18: High Issue Title](#h-18-high-issue-title) + - [H-16: Uninitialized State Variables](#h-16-uninitialized-state-variables) + - [H-17: Incorrect use of caret operator on a non hexadcimal constant](#h-17-incorrect-use-of-caret-operator-on-a-non-hexadcimal-constant) + - [H-18: Yul block contains `return` function call.](#h-18-yul-block-contains-return-function-call) + - [H-19: High Issue Title](#h-19-high-issue-title) + - [H-20: Misused boolean with logical operators](#h-20-misused-boolean-with-logical-operators) + - [H-21: Sending native Eth is not protected from these functions.](#h-21-sending-native-eth-is-not-protected-from-these-functions) + - [H-22: Delegatecall made by the function without checks on any adress.](#h-22-delegatecall-made-by-the-function-without-checks-on-any-adress) + - [H-23: Tautological comparison.](#h-23-tautological-comparison) + - [H-24: RTLO character detected in file. \u{202e}](#h-24-rtlo-character-detected-in-file-u202e) + - [H-25: Return value of the function call is not checked.](#h-25-return-value-of-the-function-call-is-not-checked) + - [H-26: Dangerous unary operator found in assignment.](#h-26-dangerous-unary-operator-found-in-assignment) + - [H-27: Weak Randomness](#h-27-weak-randomness) + - [H-28: Usage of variable before declaration.](#h-28-usage-of-variable-before-declaration) + - [H-29: Deletion from a nested mappping.](#h-29-deletion-from-a-nested-mappping) - [Low Issues](#low-issues) - [L-1: Centralization Risk for trusted owners](#l-1-centralization-risk-for-trusted-owners) - [L-2: Solmate's SafeTransferLib does not check for token contract's existence](#l-2-solmates-safetransferlib-does-not-check-for-token-contracts-existence) @@ -58,8 +69,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 55 | -| Total nSLOC | 1521 | +| .sol Files | 67 | +| Total nSLOC | 1904 | ## Files Details @@ -70,11 +81,15 @@ 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/CallGraphTests.sol | 49 | | src/Casting.sol | 126 | | src/ConstantsLiterals.sol | 28 | | src/ContractWithTodo.sol | 7 | | src/Counter.sol | 20 | | src/CrazyPragma.sol | 4 | +| src/DangerousUnaryOperator.sol | 13 | +| src/DelegateCallWithoutAddressCheck.sol | 31 | +| src/DeletionNestedMappingStructureContract.sol | 11 | | src/DeprecatedOZFunctions.sol | 32 | | src/DivisionBeforeMultiplication.sol | 22 | | src/DynamicArrayLengthAssignment.sol | 16 | @@ -87,19 +102,27 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/IncorrectShift.sol | 17 | | src/InternalFunctions.sol | 22 | | src/KeccakContract.sol | 21 | +| src/MisusedBoolean.sol | 67 | | src/MultipleConstructorSchemes.sol | 10 | | src/OnceModifierExample.sol | 8 | +| src/PreDeclaredVarUsage.sol | 9 | +| src/RTLO.sol | 7 | | src/RevertsAndRequriesInLoops.sol | 27 | +| src/SendEtherNoChecks.sol | 58 | | src/StateShadowing.sol | 17 | | src/StateVariables.sol | 58 | | src/StorageConditionals.sol | 59 | | src/StorageParameters.sol | 16 | | src/T11sTranferer.sol | 8 | +| src/TautologicalCompare.sol | 17 | | src/TestERC20.sol | 62 | +| src/UncheckedReturn.sol | 33 | +| src/UninitializedStateVariable.sol | 29 | | src/UnprotectedInitialize.sol | 25 | | src/UnsafeERC721Mint.sol | 18 | | src/UnusedError.sol | 19 | | src/UsingSelfdestruct.sol | 6 | +| src/WeakRandomness.sol | 59 | | src/WrongOrderOfLayout.sol | 13 | | src/YulReturn.sol | 8 | | src/ZeroAddressCheck.sol | 41 | @@ -121,14 +144,14 @@ 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** | **1521** | +| **Total** | **1904** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 18 | +| High | 29 | | Low | 23 | @@ -1147,7 +1170,96 @@ If the length of a dynamic array (storage variable) directly assigned to, it may -## H-16: Incorrect use of caret operator on a non hexadcimal constant +## H-16: Uninitialized State Variables + +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. + +
13 Found Instances + + +- Found in src/AssemblyExample.sol [Line: 5](../tests/contract-playground/src/AssemblyExample.sol#L5) + + ```solidity + uint b; + ``` + +- Found in src/DelegateCallWithoutAddressCheck.sol [Line: 9](../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol#L9) + + ```solidity + address public manager; + ``` + +- Found in src/InconsistentUints.sol [Line: 7](../tests/contract-playground/src/InconsistentUints.sol#L7) + + ```solidity + int public intVariable; // 1 + ``` + +- Found in src/InconsistentUints.sol [Line: 8](../tests/contract-playground/src/InconsistentUints.sol#L8) + + ```solidity + int256 public int256Variable; // 1 + ``` + +- Found in src/IncorrectCaretOperator.sol [Line: 10](../tests/contract-playground/src/IncorrectCaretOperator.sol#L10) + + ```solidity + uint256 private s_first; + ``` + +- Found in src/StateShadowing.sol [Line: 5](../tests/contract-playground/src/StateShadowing.sol#L5) + + ```solidity + address owner; + ``` + +- Found in src/StateVariables.sol [Line: 8](../tests/contract-playground/src/StateVariables.sol#L8) + + ```solidity + uint256 private staticPrivateNumber; + ``` + +- Found in src/StateVariables.sol [Line: 9](../tests/contract-playground/src/StateVariables.sol#L9) + + ```solidity + uint256 internal staticInternalNumber; + ``` + +- Found in src/StateVariables.sol [Line: 10](../tests/contract-playground/src/StateVariables.sol#L10) + + ```solidity + uint256 public staticPublicNumber; + ``` + +- Found in src/UninitializedStateVariable.sol [Line: 7](../tests/contract-playground/src/UninitializedStateVariable.sol#L7) + + ```solidity + string public s_author; // BAD (because it's used without initializing) + ``` + +- Found in src/UninitializedStateVariable.sol [Line: 15](../tests/contract-playground/src/UninitializedStateVariable.sol#L15) + + ```solidity + address destination; // BAD + ``` + +- Found in src/WrongOrderOfLayout.sol [Line: 11](../tests/contract-playground/src/WrongOrderOfLayout.sol#L11) + + ```solidity + uint256 public multiplier; + ``` + +- Found in src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol [Line: 68](../tests/contract-playground/src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol#L68) + + ```solidity + address public owner; + ``` + +
+ + + +## H-17: Incorrect use of caret operator on a non hexadcimal constant The caret operator is usually mistakenly thought of as an exponentiation operator but actually, it's a bitwise xor operator. @@ -1166,56 +1278,394 @@ The caret operator is usually mistakenly thought of as an exponentiation operato uint256 z = s_second^89 + 13; ``` -- Found in src/IncorrectCaretOperator.sol [Line: 18](../tests/contract-playground/src/IncorrectCaretOperator.sol#L18) +- Found in src/IncorrectCaretOperator.sol [Line: 18](../tests/contract-playground/src/IncorrectCaretOperator.sol#L18) + + ```solidity + uint256 w = s_second^s_first + 13; + ``` + +- Found in src/IncorrectCaretOperator.sol [Line: 19](../tests/contract-playground/src/IncorrectCaretOperator.sol#L19) + + ```solidity + uint256 y = s_first ^ 100; // s_first is not a constant but, 100 is. + ``` + +- Found in src/IncorrectCaretOperator.sol [Line: 20](../tests/contract-playground/src/IncorrectCaretOperator.sol#L20) + + ```solidity + uint256 p = s_third ^ 20; + ``` + + + + + +## H-18: Yul block contains `return` function call. + +Remove this, as this causes execution to halt. Nothing after that call will execute, including code following the assembly block. + +
1 Found Instances + + +- Found in src/YulReturn.sol [Line: 8](../tests/contract-playground/src/YulReturn.sol#L8) + + ```solidity + return(0, 0) + ``` + +
+ + + +## H-19: High Issue Title + +Description of the high issue. + +
1 Found Instances + + +- Found in src/StateShadowing.sol [Line: 15](../tests/contract-playground/src/StateShadowing.sol#L15) + + ```solidity + address owner; + ``` + +
+ + + +## H-20: Misused boolean with logical operators + +The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively. + +
10 Found Instances + + +- Found in src/MisusedBoolean.sol [Line: 12](../tests/contract-playground/src/MisusedBoolean.sol#L12) + + ```solidity + if (isEven(num) || true) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 19](../tests/contract-playground/src/MisusedBoolean.sol#L19) + + ```solidity + if (isEven(num) && false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 26](../tests/contract-playground/src/MisusedBoolean.sol#L26) + + ```solidity + if (false && isEven(num)) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 33](../tests/contract-playground/src/MisusedBoolean.sol#L33) + + ```solidity + if (true || isEven(num)) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 40](../tests/contract-playground/src/MisusedBoolean.sol#L40) + + ```solidity + if (true) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 47](../tests/contract-playground/src/MisusedBoolean.sol#L47) + + ```solidity + if (false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 54](../tests/contract-playground/src/MisusedBoolean.sol#L54) + + ```solidity + if (!false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 61](../tests/contract-playground/src/MisusedBoolean.sol#L61) + + ```solidity + if (isEven(num) && !false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 68](../tests/contract-playground/src/MisusedBoolean.sol#L68) + + ```solidity + if (isEven(num) && NO) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 75](../tests/contract-playground/src/MisusedBoolean.sol#L75) + + ```solidity + if (isEven(num) && !NO) { + ``` + +
+ + + +## H-21: Sending native Eth is not protected from these functions. + +Introduce checks for `msg.sender` in the function + +
5 Found Instances + + +- Found in src/CallGraphTests.sol [Line: 38](../tests/contract-playground/src/CallGraphTests.sol#L38) + + ```solidity + function enterTenthFloor2(address x) external passThroughNinthFloor2(x) { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 53](../tests/contract-playground/src/SendEtherNoChecks.sol#L53) + + ```solidity + function func1(address x) external mod1(x) { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 77](../tests/contract-playground/src/SendEtherNoChecks.sol#L77) + + ```solidity + function func1(address x) external mod1(x) { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 99](../tests/contract-playground/src/SendEtherNoChecks.sol#L99) + + ```solidity + function func1(address x) external mod1(x) { + ``` + +- Found in src/UninitializedStateVariable.sol [Line: 17](../tests/contract-playground/src/UninitializedStateVariable.sol#L17) + + ```solidity + function transfer() payable public { + ``` + +
+ + + +## H-22: Delegatecall made by the function without checks on any adress. + +Introduce checks on the address + +
3 Found Instances + + +- Found in src/DelegateCallWithoutAddressCheck.sol [Line: 15](../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol#L15) + + ```solidity + function delegate1(address to, bytes memory data) external { + ``` + +- Found in src/auditor_mode/ExternalCalls.sol [Line: 38](../tests/contract-playground/src/auditor_mode/ExternalCalls.sol#L38) + + ```solidity + function rawDelegateCallFromParameter(address myTarget, bytes calldata data) external returns (bytes memory) { + ``` + +- Found in src/inheritance/ExtendedInheritance.sol [Line: 14](../tests/contract-playground/src/inheritance/ExtendedInheritance.sol#L14) + + ```solidity + function doSomethingElse(address target) external { + ``` + +
+ + + +## H-23: Tautological comparison. + +The left hand side and the right hand side of the binary operation has the same value. This makes the condition always true or always false. + +
4 Found Instances + + +- Found in src/TautologicalCompare.sol [Line: 13](../tests/contract-playground/src/TautologicalCompare.sol#L13) + + ```solidity + return (a >= a); + ``` + +- Found in src/TautologicalCompare.sol [Line: 18](../tests/contract-playground/src/TautologicalCompare.sol#L18) + + ```solidity + return (f >= 7); + ``` + +- Found in src/TautologicalCompare.sol [Line: 23](../tests/contract-playground/src/TautologicalCompare.sol#L23) + + ```solidity + return (f < f); + ``` + +- Found in src/TautologicalCompare.sol [Line: 28](../tests/contract-playground/src/TautologicalCompare.sol#L28) + + ```solidity + return (f < g); + ``` + +
+ + + +## H-24: RTLO character detected in file. \u{202e} + +Right to left override character may be misledaing and cause potential attacks by visually misordering method arguments! + +
1 Found Instances + + +- Found in src/RTLO.sol [Line: 3](../tests/contract-playground/src/RTLO.sol#L3) + + ```solidity + pragma solidity 0.6.4; + ``` + +
+ + + +## H-25: Return value of the function call is not checked. + +Function returns a value but it is ignored. + +
2 Found Instances + + +- Found in src/UncheckedReturn.sol [Line: 14](../tests/contract-playground/src/UncheckedReturn.sol#L14) + + ```solidity + one(); + ``` + +- Found in src/UncheckedReturn.sol [Line: 27](../tests/contract-playground/src/UncheckedReturn.sol#L27) + + ```solidity + UncheckedHelperExternal(address(0x12345)).two(); + ``` + +
+ + + +## H-26: Dangerous unary operator found in assignment. + +Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in between. + +
2 Found Instances + + +- Found in src/DangerousUnaryOperator.sol [Line: 12](../tests/contract-playground/src/DangerousUnaryOperator.sol#L12) + + ```solidity + counter=+1; // BAD + ``` + +- Found in src/DangerousUnaryOperator.sol [Line: 13](../tests/contract-playground/src/DangerousUnaryOperator.sol#L13) + + ```solidity + counter=-1; // BAD + ``` + +
+ + + +## H-27: Weak Randomness + +The use of keccak256 hash functions on predictable values like block.timestamp, block.number, or similar data, including modulo operations on these values, should be avoided for generating randomness, as they are easily predictable and manipulable. The `PREVRANDAO` opcode also should not be used as a source of randomness. Instead, utilize Chainlink VRF for cryptographically secure and provably random values to ensure protocol integrity. + +
9 Found Instances + + +- Found in src/WeakRandomness.sol [Line: 6](../tests/contract-playground/src/WeakRandomness.sol#L6) + + ```solidity + uint256 randomNumber = uint256(keccak256(abi.encodePacked(msg.sender, block.number, block.timestamp))); + ``` + +- Found in src/WeakRandomness.sol [Line: 11](../tests/contract-playground/src/WeakRandomness.sol#L11) + + ```solidity + return uint256(keccak256(abi.encodePacked(block.number))); + ``` + +- Found in src/WeakRandomness.sol [Line: 16](../tests/contract-playground/src/WeakRandomness.sol#L16) + + ```solidity + return uint256(keccak256(someBytes)); + ``` + +- Found in src/WeakRandomness.sol [Line: 21](../tests/contract-playground/src/WeakRandomness.sol#L21) + + ```solidity + return uint256(keccak256(someBytes)); + ``` + +- Found in src/WeakRandomness.sol [Line: 25](../tests/contract-playground/src/WeakRandomness.sol#L25) + + ```solidity + return block.timestamp % 10; + ``` + +- Found in src/WeakRandomness.sol [Line: 31](../tests/contract-playground/src/WeakRandomness.sol#L31) + + ```solidity + return a % b; + ``` + +- Found in src/WeakRandomness.sol [Line: 35](../tests/contract-playground/src/WeakRandomness.sol#L35) ```solidity - uint256 w = s_second^s_first + 13; + uint256 randomNumber = uint256(blockhash(block.number)) % 10; ``` -- Found in src/IncorrectCaretOperator.sol [Line: 19](../tests/contract-playground/src/IncorrectCaretOperator.sol#L19) +- Found in src/WeakRandomness.sol [Line: 41](../tests/contract-playground/src/WeakRandomness.sol#L41) ```solidity - uint256 y = s_first ^ 100; // s_first is not a constant but, 100 is. + return hash % 10; ``` -- Found in src/IncorrectCaretOperator.sol [Line: 20](../tests/contract-playground/src/IncorrectCaretOperator.sol#L20) +- Found in src/WeakRandomness.sol [Line: 45](../tests/contract-playground/src/WeakRandomness.sol#L45) ```solidity - uint256 p = s_third ^ 20; + uint256 randomNumber = block.prevrandao; ```
-## H-17: Yul block contains `return` function call. +## H-28: Usage of variable before declaration. -Remove this, as this causes execution to halt. Nothing after that call will execute, including code following the assembly block. +This is a bad practice that may lead to unintended consequences. Please declare the variable before using it.
1 Found Instances -- Found in src/YulReturn.sol [Line: 8](../tests/contract-playground/src/YulReturn.sol#L8) +- Found in src/PreDeclaredVarUsage.sol [Line: 8](../tests/contract-playground/src/PreDeclaredVarUsage.sol#L8) ```solidity - return(0, 0) + a = 100; ```
-## H-18: High Issue Title +## H-29: Deletion from a nested mappping. -Description of the high issue. +A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract.
1 Found Instances -- Found in src/StateShadowing.sol [Line: 15](../tests/contract-playground/src/StateShadowing.sol#L15) +- Found in src/DeletionNestedMappingStructureContract.sol [Line: 17](../tests/contract-playground/src/DeletionNestedMappingStructureContract.sol#L17) ```solidity - address owner; + delete people[msg.sender]; ```
@@ -1377,7 +1827,7 @@ Openzeppelin has deprecated several functions and replaced with newer versions. ERC20 functions may not behave as expected. For example: return values are not always meaningful. It is recommended to use OpenZeppelin's SafeERC20 library. -
9 Found Instances +
11 Found Instances - Found in src/ArbitraryTransferFrom.sol [Line: 16](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L16) @@ -1428,12 +1878,24 @@ ERC20 functions may not behave as expected. For example: return values are not a token.transferFrom(from, to, value); ``` +- Found in src/SendEtherNoChecks.sol [Line: 67](../tests/contract-playground/src/SendEtherNoChecks.sol#L67) + + ```solidity + payable(x).transfer(BAL); + ``` + - Found in src/StateShadowing.sol [Line: 22](../tests/contract-playground/src/StateShadowing.sol#L22) ```solidity msg.sender.transfer(address(this).balance); ``` +- Found in src/UninitializedStateVariable.sol [Line: 18](../tests/contract-playground/src/UninitializedStateVariable.sol#L18) + + ```solidity + payable(destination).transfer(msg.value); // `destination` does not have any assignments. + ``` +
@@ -1442,7 +1904,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;` -
11 Found Instances +
15 Found Instances - Found in src/ContractWithTodo.sol [Line: 2](../tests/contract-playground/src/ContractWithTodo.sol#L2) @@ -1463,12 +1925,36 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity >=0.8.19 <0.9.1; ``` +- Found in src/DangerousUnaryOperator.sol [Line: 2](../tests/contract-playground/src/DangerousUnaryOperator.sol#L2) + + ```solidity + pragma solidity ^0.4.0; + ``` + +- Found in src/DelegateCallWithoutAddressCheck.sol [Line: 2](../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol#L2) + + ```solidity + pragma solidity ^0.8; + ``` + +- Found in src/DeletionNestedMappingStructureContract.sol [Line: 2](../tests/contract-playground/src/DeletionNestedMappingStructureContract.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/InconsistentUints.sol [Line: 1](../tests/contract-playground/src/InconsistentUints.sol#L1) ```solidity pragma solidity ^0.8.24; ``` +- Found in src/PreDeclaredVarUsage.sol [Line: 2](../tests/contract-playground/src/PreDeclaredVarUsage.sol#L2) + + ```solidity + pragma solidity ^0.4.0; + ``` + - Found in src/UsingSelfdestruct.sol [Line: 2](../tests/contract-playground/src/UsingSelfdestruct.sol#L2) ```solidity @@ -1566,7 +2052,7 @@ Check for `address(0)` when assigning values to address state variables. Instead of marking a function as `public`, consider marking it as `external` if it is not used internally. -
22 Found Instances +
23 Found Instances - Found in src/ArbitraryTransferFrom.sol [Line: 28](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L28) @@ -1623,6 +2109,12 @@ Instead of marking a function as `public`, consider marking it as `external` if function setNonEmptyAlteredNumbers( ``` +- Found in src/UninitializedStateVariable.sol [Line: 17](../tests/contract-playground/src/UninitializedStateVariable.sol#L17) + + ```solidity + function transfer() payable public { + ``` + - Found in src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol [Line: 16](../tests/contract-playground/src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol#L16) ```solidity @@ -1709,7 +2201,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. -
22 Found Instances +
34 Found Instances - Found in src/Casting.sol [Line: 16](../tests/contract-playground/src/Casting.sol#L16) @@ -1778,6 +2270,18 @@ If the same constant literal value is used multiple times, create a constant sta bytes32 multipleUseOfBytes32 = 0x8a1b3dbe6301650442bfa765d4de23775fc9a4ec4329ebb5995ec7f1e3777dc4; ``` +- Found in src/DelegateCallWithoutAddressCheck.sol [Line: 24](../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol#L24) + + ```solidity + address[3] memory allowed = [address(1), address(2), address(3)]; + ``` + +- Found in src/DelegateCallWithoutAddressCheck.sol [Line: 26](../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol#L26) + + ```solidity + for (uint256 i = 0; i < 3; i++) { + ``` + - Found in src/DynamicArrayLengthAssignment.sol [Line: 13](../tests/contract-playground/src/DynamicArrayLengthAssignment.sol#L13) ```solidity @@ -1808,6 +2312,18 @@ If the same constant literal value is used multiple times, create a constant sta uint256 w = s_second^s_first + 13; ``` +- Found in src/PreDeclaredVarUsage.sol [Line: 8](../tests/contract-playground/src/PreDeclaredVarUsage.sol#L8) + + ```solidity + a = 100; + ``` + +- Found in src/PreDeclaredVarUsage.sol [Line: 9](../tests/contract-playground/src/PreDeclaredVarUsage.sol#L9) + + ```solidity + uint b = 100; + ``` + - Found in src/RevertsAndRequriesInLoops.sol [Line: 10](../tests/contract-playground/src/RevertsAndRequriesInLoops.sol#L10) ```solidity @@ -1832,6 +2348,48 @@ If the same constant literal value is used multiple times, create a constant sta for (uint256 id = 0; id < 10; ++id) { ``` +- Found in src/UncheckedReturn.sol [Line: 27](../tests/contract-playground/src/UncheckedReturn.sol#L27) + + ```solidity + UncheckedHelperExternal(address(0x12345)).two(); + ``` + +- Found in src/UncheckedReturn.sol [Line: 32](../tests/contract-playground/src/UncheckedReturn.sol#L32) + + ```solidity + uint256 _answer = UncheckedHelperExternal(address(0x12345)).two(); + ``` + +- Found in src/UncheckedReturn.sol [Line: 37](../tests/contract-playground/src/UncheckedReturn.sol#L37) + + ```solidity + require(UncheckedHelperExternal(address(0x12345)).two() == 2, "Not two"); + ``` + +- Found in src/UncheckedReturn.sol [Line: 42](../tests/contract-playground/src/UncheckedReturn.sol#L42) + + ```solidity + if (UncheckedHelperExternal(address(0x12345)).two() != 2) { + ``` + +- Found in src/WeakRandomness.sol [Line: 25](../tests/contract-playground/src/WeakRandomness.sol#L25) + + ```solidity + return block.timestamp % 10; + ``` + +- Found in src/WeakRandomness.sol [Line: 35](../tests/contract-playground/src/WeakRandomness.sol#L35) + + ```solidity + uint256 randomNumber = uint256(blockhash(block.number)) % 10; + ``` + +- Found in src/WeakRandomness.sol [Line: 41](../tests/contract-playground/src/WeakRandomness.sol#L41) + + ```solidity + return hash % 10; + ``` + - Found in src/eth2/DepositContract.sol [Line: 113](../tests/contract-playground/src/eth2/DepositContract.sol#L113) ```solidity @@ -1846,7 +2404,7 @@ If the same constant literal value is used multiple times, create a constant sta Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed. -
5 Found Instances +
7 Found Instances - Found in src/TestERC20.sol [Line: 14](../tests/contract-playground/src/TestERC20.sol#L14) @@ -1861,6 +2419,18 @@ Index event fields make the field more quickly accessible to off-chain tools tha event Transfer(address indexed src, address indexed dst, uint256 wad); ``` +- Found in src/UncheckedReturn.sol [Line: 17](../tests/contract-playground/src/UncheckedReturn.sol#L17) + + ```solidity + event OneCalled(uint256 what); + ``` + +- Found in src/UninitializedStateVariable.sol [Line: 21](../tests/contract-playground/src/UninitializedStateVariable.sol#L21) + + ```solidity + event TellEveryone(string); + ``` + - Found in src/eth2/DepositContract.sol [Line: 19](../tests/contract-playground/src/eth2/DepositContract.sol#L19) ```solidity @@ -1887,8 +2457,38 @@ Index event fields make the field more quickly accessible to off-chain tools tha Use descriptive reason strings or custom errors for revert paths. -
11 Found Instances +
19 Found Instances + + +- Found in src/CallGraphTests.sol [Line: 7](../tests/contract-playground/src/CallGraphTests.sol#L7) + + ```solidity + require(msg.sender == address(0x11)); + ``` + +- Found in src/CallGraphTests.sol [Line: 28](../tests/contract-playground/src/CallGraphTests.sol#L28) + + ```solidity + revert(); + ``` +- Found in src/CallGraphTests.sol [Line: 50](../tests/contract-playground/src/CallGraphTests.sol#L50) + + ```solidity + revert(); + ``` + +- Found in src/CallGraphTests.sol [Line: 65](../tests/contract-playground/src/CallGraphTests.sol#L65) + + ```solidity + require(msg.sender == address(0x11)); + ``` + +- Found in src/DelegateCallWithoutAddressCheck.sol [Line: 31](../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol#L31) + + ```solidity + require(isLegit); + ``` - Found in src/DeprecatedOZFunctions.sol [Line: 37](../tests/contract-playground/src/DeprecatedOZFunctions.sol#L37) @@ -1908,6 +2508,24 @@ Use descriptive reason strings or custom errors for revert paths. revert(); ``` +- Found in src/SendEtherNoChecks.sol [Line: 12](../tests/contract-playground/src/SendEtherNoChecks.sol#L12) + + ```solidity + revert(); + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 27](../tests/contract-playground/src/SendEtherNoChecks.sol#L27) + + ```solidity + require(msg.sender == address(0x11)); + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 43](../tests/contract-playground/src/SendEtherNoChecks.sol#L43) + + ```solidity + revert(); + ``` + - Found in src/StateShadowing.sol [Line: 8](../tests/contract-playground/src/StateShadowing.sol#L8) ```solidity @@ -2004,7 +2622,7 @@ Using `ERC721::_mint()` can mint ERC721 tokens to addresses which don't support Solc compiler version 0.8.20 switches the default target EVM version to Shanghai, which means that the generated bytecode will include PUSH0 opcodes. Be sure to select the appropriate EVM version in case you intend to deploy on a chain other than mainnet like L2 chains that may not support PUSH0, otherwise deployment of your contracts will fail. -
23 Found Instances +
27 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -2031,6 +2649,18 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai pragma solidity >=0.8.19 <0.9.1; ``` +- Found in src/DelegateCallWithoutAddressCheck.sol [Line: 2](../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol#L2) + + ```solidity + pragma solidity ^0.8; + ``` + +- Found in src/DeletionNestedMappingStructureContract.sol [Line: 2](../tests/contract-playground/src/DeletionNestedMappingStructureContract.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/DeprecatedOZFunctions.sol [Line: 2](../tests/contract-playground/src/DeprecatedOZFunctions.sol#L2) ```solidity @@ -2067,12 +2697,24 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai pragma solidity 0.8.20; ``` +- Found in src/TautologicalCompare.sol [Line: 2](../tests/contract-playground/src/TautologicalCompare.sol#L2) + + ```solidity + pragma solidity 0.8.20; + ``` + - Found in src/UnsafeERC721Mint.sol [Line: 2](../tests/contract-playground/src/UnsafeERC721Mint.sol#L2) ```solidity pragma solidity 0.8.20; ``` +- Found in src/WeakRandomness.sol [Line: 2](../tests/contract-playground/src/WeakRandomness.sol#L2) + + ```solidity + pragma solidity 0.8.20; + ``` + - Found in src/cloc/AnotherHeavilyCommentedContract.sol [Line: 6](../tests/contract-playground/src/cloc/AnotherHeavilyCommentedContract.sol#L6) ```solidity @@ -2153,8 +2795,32 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai -
4 Found Instances +
12 Found Instances + +- Found in src/CallGraphTests.sol [Line: 10](../tests/contract-playground/src/CallGraphTests.sol#L10) + + ```solidity + modifier passThroughNinthFloor1() { + ``` + +- Found in src/CallGraphTests.sol [Line: 32](../tests/contract-playground/src/CallGraphTests.sol#L32) + + ```solidity + modifier passThroughNinthFloor2(address x) { + ``` + +- Found in src/CallGraphTests.sol [Line: 54](../tests/contract-playground/src/CallGraphTests.sol#L54) + + ```solidity + modifier passThroughNinthFloor3(address x) { + ``` + +- Found in src/DelegateCallWithoutAddressCheck.sol [Line: 23](../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol#L23) + + ```solidity + modifier isAllowed(address to) { + ``` - Found in src/InternalFunctions.sol [Line: 18](../tests/contract-playground/src/InternalFunctions.sol#L18) @@ -2168,6 +2834,30 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai modifier onlyOnce() { ``` +- Found in src/SendEtherNoChecks.sol [Line: 16](../tests/contract-playground/src/SendEtherNoChecks.sol#L16) + + ```solidity + modifier mod1(address x) { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 47](../tests/contract-playground/src/SendEtherNoChecks.sol#L47) + + ```solidity + modifier mod1(address x) { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 70](../tests/contract-playground/src/SendEtherNoChecks.sol#L70) + + ```solidity + modifier mod1(address x) { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 93](../tests/contract-playground/src/SendEtherNoChecks.sol#L93) + + ```solidity + modifier mod1(address x) { + ``` + - Found in src/StateShadowing.sol [Line: 7](../tests/contract-playground/src/StateShadowing.sol#L7) ```solidity @@ -2188,7 +2878,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai Consider removing empty blocks. -
20 Found Instances +
25 Found Instances - Found in src/AdminContract.sol [Line: 14](../tests/contract-playground/src/AdminContract.sol#L14) @@ -2197,6 +2887,18 @@ Consider removing empty blocks. function someOtherImportantThing() external nonReentrant onlyOwner { ``` +- Found in src/CallGraphTests.sol [Line: 16](../tests/contract-playground/src/CallGraphTests.sol#L16) + + ```solidity + function enterTenthFloor1() external passThroughNinthFloor1() { + ``` + +- Found in src/CallGraphTests.sol [Line: 38](../tests/contract-playground/src/CallGraphTests.sol#L38) + + ```solidity + function enterTenthFloor2(address x) external passThroughNinthFloor2(x) { + ``` + - Found in src/ContractWithTodo.sol [Line: 7](../tests/contract-playground/src/ContractWithTodo.sol#L7) ```solidity @@ -2257,6 +2959,24 @@ Consider removing empty blocks. function perform() external onlyOnce { ``` +- Found in src/SendEtherNoChecks.sol [Line: 53](../tests/contract-playground/src/SendEtherNoChecks.sol#L53) + + ```solidity + function func1(address x) external mod1(x) { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 77](../tests/contract-playground/src/SendEtherNoChecks.sol#L77) + + ```solidity + function func1(address x) external mod1(x) { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 99](../tests/contract-playground/src/SendEtherNoChecks.sol#L99) + + ```solidity + function func1(address x) external mod1(x) { + ``` + - Found in src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol [Line: 11](../tests/contract-playground/src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol#L11) ```solidity @@ -2462,8 +3182,32 @@ 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. -
2 Found Instances +
11 Found Instances + + +- Found in src/CallGraphTests.sol [Line: 6](../tests/contract-playground/src/CallGraphTests.sol#L6) + + ```solidity + function visitEighthFloor1() internal { + ``` + +- Found in src/CallGraphTests.sol [Line: 25](../tests/contract-playground/src/CallGraphTests.sol#L25) + + ```solidity + function visitEighthFloor2(address x) internal { + ``` + +- Found in src/CallGraphTests.sol [Line: 47](../tests/contract-playground/src/CallGraphTests.sol#L47) + ```solidity + function visitEighthFloor3(address x) internal { + ``` + +- Found in src/CallGraphTests.sol [Line: 64](../tests/contract-playground/src/CallGraphTests.sol#L64) + + ```solidity + function visitSeventhFloor3() internal { + ``` - Found in src/InternalFunctions.sol [Line: 28](../tests/contract-playground/src/InternalFunctions.sol#L28) @@ -2471,6 +3215,36 @@ Instead of separating the logic into a separate function, consider inlining the function internalSet2(uint256 _newValue) internal { ``` +- Found in src/SendEtherNoChecks.sol [Line: 9](../tests/contract-playground/src/SendEtherNoChecks.sol#L9) + + ```solidity + function callAndSendNativeEth(address x) internal { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 26](../tests/contract-playground/src/SendEtherNoChecks.sol#L26) + + ```solidity + function func2() internal view { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 40](../tests/contract-playground/src/SendEtherNoChecks.sol#L40) + + ```solidity + function callAndSendNativeEth(address x) internal { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 66](../tests/contract-playground/src/SendEtherNoChecks.sol#L66) + + ```solidity + function transferBalance(address x) internal { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 88](../tests/contract-playground/src/SendEtherNoChecks.sol#L88) + + ```solidity + function sendBalance(address x) internal { + ``` + - Found in src/StorageParameters.sol [Line: 17](../tests/contract-playground/src/StorageParameters.sol#L17) ```solidity @@ -2508,7 +3282,7 @@ Contract contains comments with TODOS Consider keeping the naming convention consistent in a given contract. Explicit size declarations are preferred (uint256, int256) over implicit ones (uint, int) to avoid confusion. -
19 Found Instances +
20 Found Instances - Found in src/Casting.sol [Line: 31](../tests/contract-playground/src/Casting.sol#L31) @@ -2565,6 +3339,12 @@ Consider keeping the naming convention consistent in a given contract. Explicit constructor(uint _uintInitial, uint256 _uint256Initial) { // 6 4 ``` +- Found in src/TautologicalCompare.sol [Line: 11](../tests/contract-playground/src/TautologicalCompare.sol#L11) + + ```solidity + function check(uint a) external pure returns(bool){ + ``` + - Found in src/eth2/DepositContract.sol [Line: 59](../tests/contract-playground/src/eth2/DepositContract.sol#L59) ```solidity diff --git a/reports/report.sarif b/reports/report.sarif index d23cf9358..914985a7e 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -1690,6 +1690,158 @@ }, "ruleId": "dynamic-array-length-assignment" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/AssemblyExample.sol" + }, + "region": { + "byteLength": 1, + "byteOffset": 97 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 337 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/InconsistentUints.sol" + }, + "region": { + "byteLength": 11, + "byteOffset": 197 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/InconsistentUints.sol" + }, + "region": { + "byteLength": 14, + "byteOffset": 233 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectCaretOperator.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 355 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateShadowing.sol" + }, + "region": { + "byteLength": 13, + "byteOffset": 87 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariables.sol" + }, + "region": { + "byteLength": 19, + "byteOffset": 199 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariables.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 241 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariables.sol" + }, + "region": { + "byteLength": 18, + "byteOffset": 282 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UninitializedStateVariable.sol" + }, + "region": { + "byteLength": 8, + "byteOffset": 122 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UninitializedStateVariable.sol" + }, + "region": { + "byteLength": 11, + "byteOffset": 529 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WrongOrderOfLayout.sol" + }, + "region": { + "byteLength": 10, + "byteOffset": 257 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol" + }, + "region": { + "byteLength": 5, + "byteOffset": 1971 + } + } + } + ], + "message": { + "text": "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." + }, + "ruleId": "uninitialized-state-variable" + }, { "level": "warning", "locations": [ @@ -1700,59 +1852,377 @@ }, "region": { "byteLength": 8, - "byteOffset": 519 + "byteOffset": 519 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectCaretOperator.sol" + }, + "region": { + "byteLength": 16, + "byteOffset": 549 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectCaretOperator.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 587 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectCaretOperator.sol" + }, + "region": { + "byteLength": 13, + "byteOffset": 631 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectCaretOperator.sol" + }, + "region": { + "byteLength": 12, + "byteOffset": 708 + } + } + } + ], + "message": { + "text": "The caret operator is usually mistakenly thought of as an exponentiation operator but actually, it's a bitwise xor operator." + }, + "ruleId": "incorrect-caret-operator" + }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/YulReturn.sol" + }, + "region": { + "byteLength": 12, + "byteOffset": 171 + } + } + } + ], + "message": { + "text": "Remove this, as this causes execution to halt. Nothing after that call will execute, including code following the assembly block." + }, + "ruleId": "yul-return" + }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateShadowing.sol" + }, + "region": { + "byteLength": 13, + "byteOffset": 239 + } + } + } + ], + "message": { + "text": "Description of the high issue." + }, + "ruleId": "state-variable-shadowing" + }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 19, + "byteOffset": 257 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 419 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 582 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 19, + "byteOffset": 745 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 51, + "byteOffset": 908 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 52, + "byteOffset": 1060 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 53, + "byteOffset": 1213 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 1366 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 17, + "byteOffset": 1530 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 18, + "byteOffset": 1691 + } + } + } + ], + "message": { + "text": "The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively." + }, + "ruleId": "misused-boolean" + }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 16, + "byteOffset": 686 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 5, + "byteOffset": 1060 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 5, + "byteOffset": 1405 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 5, + "byteOffset": 1795 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UninitializedStateVariable.sol" + }, + "region": { + "byteLength": 8, + "byteOffset": 563 + } + } + } + ], + "message": { + "text": "Introduce checks for `msg.sender` in the function" + }, + "ruleId": "send-ether-no-checks" + }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 9, + "byteOffset": 392 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/IncorrectCaretOperator.sol" + "uri": "src/auditor_mode/ExternalCalls.sol" }, "region": { - "byteLength": 16, - "byteOffset": 549 + "byteLength": 28, + "byteOffset": 1253 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/IncorrectCaretOperator.sol" + "uri": "src/inheritance/ExtendedInheritance.sol" }, "region": { - "byteLength": 21, - "byteOffset": 587 + "byteLength": 15, + "byteOffset": 391 + } + } + } + ], + "message": { + "text": "Introduce checks on the address" + }, + "ruleId": "delegate-call-unchecked-address" + }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/TautologicalCompare.sol" + }, + "region": { + "byteLength": 6, + "byteOffset": 255 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/IncorrectCaretOperator.sol" + "uri": "src/TautologicalCompare.sol" }, "region": { - "byteLength": 13, - "byteOffset": 631 + "byteLength": 6, + "byteOffset": 359 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/IncorrectCaretOperator.sol" + "uri": "src/TautologicalCompare.sol" }, "region": { - "byteLength": 12, - "byteOffset": 708 + "byteLength": 5, + "byteOffset": 463 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/TautologicalCompare.sol" + }, + "region": { + "byteLength": 5, + "byteOffset": 566 } } } ], "message": { - "text": "The caret operator is usually mistakenly thought of as an exponentiation operator but actually, it's a bitwise xor operator." + "text": "The left hand side and the right hand side of the binary operation has the same value. This makes the condition always true or always false." }, - "ruleId": "incorrect-caret-operator" + "ruleId": "tautological-compare" }, { "level": "warning", @@ -1760,19 +2230,19 @@ { "physicalLocation": { "artifactLocation": { - "uri": "src/YulReturn.sol" + "uri": "src/RTLO.sol" }, "region": { - "byteLength": 12, - "byteOffset": 171 + "byteLength": 157, + "byteOffset": 33 } } } ], "message": { - "text": "Remove this, as this causes execution to halt. Nothing after that call will execute, including code following the assembly block." + "text": "Right to left override character may be misledaing and cause potential attacks by visually misordering method arguments!" }, - "ruleId": "yul-return" + "ruleId": "rtlo" }, { "level": "warning", @@ -1780,19 +2250,209 @@ { "physicalLocation": { "artifactLocation": { - "uri": "src/StateShadowing.sol" + "uri": "src/UncheckedReturn.sol" }, "region": { - "byteLength": 13, - "byteOffset": 239 + "byteLength": 5, + "byteOffset": 279 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UncheckedReturn.sol" + }, + "region": { + "byteLength": 47, + "byteOffset": 575 } } } ], "message": { - "text": "Description of the high issue." + "text": "Function returns a value but it is ignored." }, - "ruleId": "state-variable-shadowing" + "ruleId": "unchecked-return" + }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DangerousUnaryOperator.sol" + }, + "region": { + "byteLength": 10, + "byteOffset": 220 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DangerousUnaryOperator.sol" + }, + "region": { + "byteLength": 10, + "byteOffset": 247 + } + } + } + ], + "message": { + "text": "Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in between." + }, + "ruleId": "dangerous-unary-operator" + }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 70, + "byteOffset": 188 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 41, + "byteOffset": 386 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 597 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 793 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 915 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 5, + "byteOffset": 1095 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 37, + "byteOffset": 1217 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 9, + "byteOffset": 1434 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 16, + "byteOffset": 1563 + } + } + } + ], + "message": { + "text": "The use of keccak256 hash functions on predictable values like block.timestamp, block.number, or similar data, including modulo operations on these values, should be avoided for generating randomness, as they are easily predictable and manipulable. The `PREVRANDAO` opcode also should not be used as a source of randomness. Instead, utilize Chainlink VRF for cryptographically secure and provably random values to ensure protocol integrity." + }, + "ruleId": "weak-randomness" + }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/PreDeclaredVarUsage.sol" + }, + "region": { + "byteLength": 1, + "byteOffset": 196 + } + } + } + ], + "message": { + "text": "This is a bad practice that may lead to unintended consequences. Please declare the variable before using it." + }, + "ruleId": "pre-declared-local-variable-usage" + }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DeletionNestedMappingStructureContract.sol" + }, + "region": { + "byteLength": 25, + "byteOffset": 426 + } + } + } + ], + "message": { + "text": "A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract." + }, + "ruleId": "delete-nested-mapping" }, { "level": "note", @@ -2108,6 +2768,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 19, + "byteOffset": 1255 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2118,6 +2789,17 @@ "byteOffset": 368 } } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UninitializedStateVariable.sol" + }, + "region": { + "byteLength": 29, + "byteOffset": 599 + } + } } ], "message": { @@ -2161,6 +2843,39 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DangerousUnaryOperator.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 32 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DeletionNestedMappingStructureContract.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2172,6 +2887,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/PreDeclaredVarUsage.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2432,6 +3158,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UninitializedStateVariable.sol" + }, + "region": { + "byteLength": 8, + "byteOffset": 563 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2705,6 +3442,39 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 1, + "byteOffset": 718 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 1, + "byteOffset": 771 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 1, + "byteOffset": 838 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2760,6 +3530,28 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/PreDeclaredVarUsage.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 200 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/PreDeclaredVarUsage.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 222 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2804,6 +3596,83 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UncheckedReturn.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 607 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UncheckedReturn.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 805 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UncheckedReturn.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 995 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UncheckedReturn.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 1190 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 933 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 1252 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 1441 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2857,6 +3726,28 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UncheckedReturn.sol" + }, + "region": { + "byteLength": 30, + "byteOffset": 297 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UninitializedStateVariable.sol" + }, + "region": { + "byteLength": 27, + "byteOffset": 700 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2899,6 +3790,61 @@ { "level": "note", "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 128 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 6, + "byteOffset": 531 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 6, + "byteOffset": 936 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 1246 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 948 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2913,22 +3859,55 @@ { "physicalLocation": { "artifactLocation": { - "uri": "src/DeprecatedOZFunctions.sol" + "uri": "src/DeprecatedOZFunctions.sol" + }, + "region": { + "byteLength": 6, + "byteOffset": 1389 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/RevertsAndRequriesInLoops.sol" + }, + "region": { + "byteLength": 6, + "byteOffset": 503 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 6, + "byteOffset": 268 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" }, "region": { - "byteLength": 6, - "byteOffset": 1389 + "byteLength": 7, + "byteOffset": 513 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/RevertsAndRequriesInLoops.sol" + "uri": "src/SendEtherNoChecks.sol" }, "region": { "byteLength": 6, - "byteOffset": 503 + "byteOffset": 920 } } }, @@ -3124,6 +4103,28 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 32 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DeletionNestedMappingStructureContract.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3190,6 +4191,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/TautologicalCompare.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3201,6 +4213,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3342,6 +4365,50 @@ { "level": "note", "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 22, + "byteOffset": 186 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 22, + "byteOffset": 571 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 22, + "byteOffset": 976 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 9, + "byteOffset": 678 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3364,6 +4431,50 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 4, + "byteOffset": 308 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 4, + "byteOffset": 960 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 4, + "byteOffset": 1301 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 4, + "byteOffset": 1704 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3406,6 +4517,28 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 16, + "byteOffset": 291 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 16, + "byteOffset": 686 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3516,6 +4649,39 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 5, + "byteOffset": 1060 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 5, + "byteOffset": 1405 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 5, + "byteOffset": 1795 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3875,6 +5041,50 @@ { "level": "note", "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 17, + "byteOffset": 89 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 17, + "byteOffset": 398 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 17, + "byteOffset": 803 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 18, + "byteOffset": 1206 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3886,6 +5096,61 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 132 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 5, + "byteOffset": 481 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 784 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 15, + "byteOffset": 1209 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 11, + "byteOffset": 1551 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -4036,6 +5301,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/TautologicalCompare.sol" + }, + "region": { + "byteLength": 1, + "byteOffset": 186 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -4285,8 +5561,8 @@ "informationUri": "https://github.com/Cyfrin/aderyn", "name": "Aderyn", "organization": "Cyfrin", - "semanticVersion": "0.1.7", - "version": "0.1.7" + "semanticVersion": "0.1.8", + "version": "0.1.8" } } } diff --git a/reports/templegold-report.md b/reports/templegold-report.md index 504a8d96c..b7a152e8d 100644 --- a/reports/templegold-report.md +++ b/reports/templegold-report.md @@ -12,6 +12,10 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-2: Unprotected initializer](#h-2-unprotected-initializer) - [H-3: Unsafe Casting](#h-3-unsafe-casting) - [H-4: Contract Name Reused in Different Files](#h-4-contract-name-reused-in-different-files) + - [H-5: Uninitialized State Variables](#h-5-uninitialized-state-variables) + - [H-6: Return value of the function call is not checked.](#h-6-return-value-of-the-function-call-is-not-checked) + - [H-7: Weak Randomness](#h-7-weak-randomness) + - [H-8: Deletion from a nested mappping.](#h-8-deletion-from-a-nested-mappping) - [Low Issues](#low-issues) - [L-1: Centralization Risk for trusted owners](#l-1-centralization-risk-for-trusted-owners) - [L-2: `ecrecover` is susceptible to signature malleability](#l-2-ecrecover-is-susceptible-to-signature-malleability) @@ -183,7 +187,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 4 | +| High | 8 | | Low | 18 | @@ -335,6 +339,188 @@ When compiling contracts with certain development frameworks (for example: Truff +## H-5: Uninitialized State Variables + +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. + +
8 Found Instances + + +- Found in contracts/amm/TempleUniswapV2Pair.sol [Line: 29](../tests/2024-07-templegold/protocol/contracts/amm/TempleUniswapV2Pair.sol#L29) + + ```solidity + uint public kLast; // reserve0 * reserve1, as of immediately after the most recent liquidity event + ``` + +- Found in contracts/fakes/templegold/TempleGoldStakingMock.sol [Line: 39](../tests/2024-07-templegold/protocol/contracts/fakes/templegold/TempleGoldStakingMock.sol#L39) + + ```solidity + uint256 public rewardPerTokenStored; + ``` + +- Found in contracts/fakes/templegold/TempleGoldStakingMock.sol [Line: 47](../tests/2024-07-templegold/protocol/contracts/fakes/templegold/TempleGoldStakingMock.sol#L47) + + ```solidity + uint256 public periodFinish; + ``` + +- Found in contracts/fakes/templegold/TempleGoldStakingMock.sol [Line: 48](../tests/2024-07-templegold/protocol/contracts/fakes/templegold/TempleGoldStakingMock.sol#L48) + + ```solidity + uint256 public lastUpdateTime; + ``` + +- Found in contracts/templegold/AuctionBase.sol [Line: 13](../tests/2024-07-templegold/protocol/contracts/templegold/AuctionBase.sol#L13) + + ```solidity + uint256 internal _currentEpochId; + ``` + +- Found in contracts/templegold/TempleGoldStaking.sol [Line: 40](../tests/2024-07-templegold/protocol/contracts/templegold/TempleGoldStaking.sol#L40) + + ```solidity + uint256 public override rewardPerTokenStored; + ``` + +- Found in contracts/templegold/TempleGoldStaking.sol [Line: 45](../tests/2024-07-templegold/protocol/contracts/templegold/TempleGoldStaking.sol#L45) + + ```solidity + uint256 public override periodFinish; + ``` + +- Found in contracts/templegold/TempleGoldStaking.sol [Line: 47](../tests/2024-07-templegold/protocol/contracts/templegold/TempleGoldStaking.sol#L47) + + ```solidity + uint256 public override lastUpdateTime; + ``` + +
+ + + +## H-6: Return value of the function call is not checked. + +Function returns a value but it is ignored. + +
13 Found Instances + + +- Found in contracts/amo/AuraStaking.sol [Line: 83](../tests/2024-07-templegold/protocol/contracts/amo/AuraStaking.sol#L83) + + ```solidity + booster.deposit(auraPoolInfo.pId, amount, true); + ``` + +- Found in contracts/amo/AuraStaking.sol [Line: 93](../tests/2024-07-templegold/protocol/contracts/amo/AuraStaking.sol#L93) + + ```solidity + IAuraBaseRewardPool(auraPoolInfo.rewards).withdrawAndUnwrap(toUnstake, claim); + ``` + +- Found in contracts/amo/AuraStaking.sol [Line: 111](../tests/2024-07-templegold/protocol/contracts/amo/AuraStaking.sol#L111) + + ```solidity + IAuraBaseRewardPool(auraPoolInfo.rewards).getReward(address(this), claimExtras); + ``` + +- Found in contracts/fakes/v2/TempleDebtTokenTestnetAdmin.sol [Line: 28](../tests/2024-07-templegold/protocol/contracts/fakes/v2/TempleDebtTokenTestnetAdmin.sol#L28) + + ```solidity + dUSD.burn(from, amount); + ``` + +- Found in contracts/fakes/v2/strategies/DsrBaseStrategyTestnet.sol [Line: 52](../tests/2024-07-templegold/protocol/contracts/fakes/v2/strategies/DsrBaseStrategyTestnet.sol#L52) + + ```solidity + _checkpointDaiBalance(daiToken.balanceOf(address(this))); + ``` + +- Found in contracts/fakes/v2/strategies/DsrBaseStrategyTestnet.sol [Line: 131](../tests/2024-07-templegold/protocol/contracts/fakes/v2/strategies/DsrBaseStrategyTestnet.sol#L131) + + ```solidity + _checkpointDaiBalance(daiToken.balanceOf(address(this))); + ``` + +- Found in contracts/fakes/v2/strategies/DsrBaseStrategyTestnet.sol [Line: 181](../tests/2024-07-templegold/protocol/contracts/fakes/v2/strategies/DsrBaseStrategyTestnet.sol#L181) + + ```solidity + _checkpointDaiBalance(balance - amount); + ``` + +- Found in contracts/v2/TempleDebtToken.sol [Line: 149](../tests/2024-07-templegold/protocol/contracts/v2/TempleDebtToken.sol#L149) + + ```solidity + _getBaseCache(); + ``` + +- Found in contracts/v2/TempleDebtToken.sol [Line: 160](../tests/2024-07-templegold/protocol/contracts/v2/TempleDebtToken.sol#L160) + + ```solidity + _getDebtorCache(_getBaseCache(), debtor, true); + ``` + +- Found in contracts/v2/TempleDebtToken.sol [Line: 638](../tests/2024-07-templegold/protocol/contracts/v2/TempleDebtToken.sol#L638) + + ```solidity + _initBaseCache(baseCache); + ``` + +- Found in contracts/v2/TempleDebtToken.sol [Line: 752](../tests/2024-07-templegold/protocol/contracts/v2/TempleDebtToken.sol#L752) + + ```solidity + _initDebtorCache(_baseCache, _debtor, debtorCache, _roundUp); + ``` + +- Found in contracts/v2/strategies/RamosStrategy.sol [Line: 252](../tests/2024-07-templegold/protocol/contracts/v2/strategies/RamosStrategy.sol#L252) + + ```solidity + ramos.removeLiquidity(params.requestData, params.bptAmount); + ``` + +- Found in contracts/v2/templeLineOfCredit/TempleLineOfCredit.sol [Line: 680](../tests/2024-07-templegold/protocol/contracts/v2/templeLineOfCredit/TempleLineOfCredit.sol#L680) + + ```solidity + _initDebtTokenCache(cache); + ``` + +
+ + + +## H-7: Weak Randomness + +The use of keccak256 hash functions on predictable values like block.timestamp, block.number, or similar data, including modulo operations on these values, should be avoided for generating randomness, as they are easily predictable and manipulable. The `PREVRANDAO` opcode also should not be used as a source of randomness. Instead, utilize Chainlink VRF for cryptographically secure and provably random values to ensure protocol integrity. + +
1 Found Instances + + +- Found in contracts/amm/TempleUniswapV2Pair.sol [Line: 82](../tests/2024-07-templegold/protocol/contracts/amm/TempleUniswapV2Pair.sol#L82) + + ```solidity + uint32 blockTimestamp = uint32(block.timestamp % 2**32); + ``` + +
+ + + +## H-8: Deletion from a nested mappping. + +A deletion in a structure containing a mapping will not delete the mapping. The remaining data may be used to compromise the contract. + +
1 Found Instances + + +- Found in contracts/v2/TreasuryReservesVault.sol [Line: 317](../tests/2024-07-templegold/protocol/contracts/v2/TreasuryReservesVault.sol#L317) + + ```solidity + delete strategies[strategy]; + ``` + +
+ + + # Low Issues ## L-1: Centralization Risk for trusted owners diff --git a/tests/contract-playground/src/CallGraphTests.sol b/tests/contract-playground/src/CallGraphTests.sol new file mode 100644 index 000000000..2d63ae789 --- /dev/null +++ b/tests/contract-playground/src/CallGraphTests.sol @@ -0,0 +1,75 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.19; + +contract Tower1 { + + function visitEighthFloor1() internal { + require(msg.sender == address(0x11)); + } + + modifier passThroughNinthFloor1() { + visitEighthFloor1(); + _; + } + + // Start Here + function enterTenthFloor1() external passThroughNinthFloor1() { + + } + +} + + +contract Tower2 { + + function visitEighthFloor2(address x) internal { + (bool success,) = x.call{value: 10}("calldata"); + if (!success) { + revert(); + } + } + + modifier passThroughNinthFloor2(address x) { + visitEighthFloor2(x); + _; + } + + // Start Here + function enterTenthFloor2(address x) external passThroughNinthFloor2(x) { + + } + +} + + +contract Tower3 { + + function visitEighthFloor3(address x) internal { + (bool success,) = x.call{value: 10}("calldata"); + if (!success) { + revert(); + } + } + + modifier passThroughNinthFloor3(address x) { + visitEighthFloor3(x); + _; + } + + // Start Here + function enterTenthFloor3(address x) external passThroughNinthFloor3(x) { + visitSeventhFloor3(); + } + + function visitSeventhFloor3() internal { + require(msg.sender == address(0x11)); + } + +} + +contract Tower4 { + // A recursive function should have itself as upstream and downstream + function recurse(string memory something) private { + recurse(something); + } +} \ No newline at end of file diff --git a/tests/contract-playground/src/DangerousUnaryOperator.sol b/tests/contract-playground/src/DangerousUnaryOperator.sol new file mode 100644 index 000000000..a8cb8f889 --- /dev/null +++ b/tests/contract-playground/src/DangerousUnaryOperator.sol @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.4.0; + +// =+ instead of += + +contract WronglyUseOperator { + + function wronglyUseOperators() external pure returns(int256){ + + int256 counter = 1000; + + counter=+1; // BAD + counter=-1; // BAD + + counter= +1; // GOOD (because there is a space, it's not a typo) + counter= -1; // GOOD (because there is a space, it's not a typo) + + counter+=1; // GOOD + counter-=1; // GOOD + + return counter; + } +} \ No newline at end of file diff --git a/tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol b/tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol new file mode 100644 index 000000000..6f65bbf98 --- /dev/null +++ b/tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8; + +contract DelegatecallWithoutAddressChecks { + + // Any delegate calls to addresses in state variables, we don't check! + // Because we assume that other detectors catch setting of variables + // either in the constructor or in other methods with modifiers. + address public manager; + + constructor() { + + } + + function delegate1(address to, bytes memory data) external { + to.delegatecall(data); // `to` is not protected, therefore BAD + } + + function delegate2(bytes memory data) external { + manager.delegatecall(data); // `manager` is state variable, therefore GOOD + } + + modifier isAllowed(address to) { + address[3] memory allowed = [address(1), address(2), address(3)]; + bool isLegit = false; + for (uint256 i = 0; i < 3; i++) { + if (allowed[i] == to) { + isLegit = true; + } + } + require(isLegit); + _; + } + + function delegate3(address to, bytes memory data) external isAllowed(to) { + to.delegatecall(data); // `to` is protected, therefore GOOD + } + + // Known false negative + function delegate4(address to, bytes memory data, address x) external { + if (x != address(0)) { + to.delegatecall(data); + // Although `to` is not protected, for now, we assume it is because + // there is a binary operation with `x` involved which is of type address. + // |---> So, we assume `to` (which also of type address is vetted) + // + // This is limitation of using callgraphs. Later, when we add support for CFG + // we can do a proper data dependency analysis. + } + } + +} \ No newline at end of file diff --git a/tests/contract-playground/src/DeletionNestedMappingStructureContract.sol b/tests/contract-playground/src/DeletionNestedMappingStructureContract.sol new file mode 100644 index 000000000..ee2f3a057 --- /dev/null +++ b/tests/contract-playground/src/DeletionNestedMappingStructureContract.sol @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + + +contract NestedMappingBalanceStructure { + + struct Person { + uint256[] names; + mapping(address => uint256) age; + } + + mapping(address => Person) private people; + + function remove() internal{ + // We are deleting from a mapping whose value is a struct which contains a member of type mapping. + // Therefore, capture it! + delete people[msg.sender]; + } + +} \ No newline at end of file diff --git a/tests/contract-playground/src/MisusedBoolean.sol b/tests/contract-playground/src/MisusedBoolean.sol new file mode 100644 index 000000000..591beee03 --- /dev/null +++ b/tests/contract-playground/src/MisusedBoolean.sol @@ -0,0 +1,82 @@ +pragma solidity 0.4.22; + +contract MisusedBoolean { + + bool public constant NO = false; + + function isEven(uint256 num) internal returns(bool) { + return num % 2 == 0; + } + + function misuse(uint256 num) external returns(uint256) { + if (isEven(num) || true) { + return num * num; + } + return 0; + } + + function misuse2(uint256 num) external returns(uint256) { + if (isEven(num) && false) { + return num * num; + } + return 0; + } + + function misuse3(uint256 num) external returns(uint256) { + if (false && isEven(num)) { + return num * num; + } + return 0; + } + + function misuse4(uint256 num) external returns(uint256) { + if (true || isEven(num)) { + return num * num; + } + return 0; + } + + function misuse5(uint256 num) external pure returns(uint256) { + if (true) { + return num * num; + } + return 0; + } + + function misuse6(uint256 num) external pure returns(uint256) { + if (false) { + return num * num; + } + return 0; + } + + function misuse7(uint256 num) external pure returns(uint256) { + if (!false) { + return num * num; + } + return 0; + } + + function misuse8(uint256 num) external returns(uint256) { + if (isEven(num) && !false) { + return num * num; + } + return 0; + } + + function misuse9(uint256 num) external returns(uint256) { + if (isEven(num) && NO) { + return num * num; + } + return 0; + } + + function misuse10(uint256 num) external returns(uint256) { + if (isEven(num) && !NO) { + return num * num; + } + return 0; + } + + +} \ No newline at end of file diff --git a/tests/contract-playground/src/PreDeclaredVarUsage.sol b/tests/contract-playground/src/PreDeclaredVarUsage.sol new file mode 100644 index 000000000..2bf6c4492 --- /dev/null +++ b/tests/contract-playground/src/PreDeclaredVarUsage.sol @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.4.0; + +contract PreDeclaredVariableUsage { + + function useBeforeDeclaring() external pure returns (uint) { + /* a, b used here */ + a = 100; + uint b = 100; + + /* But, a is declared here */ + uint a; + + return a + b; + } + +} \ No newline at end of file diff --git a/tests/contract-playground/src/RTLO.sol b/tests/contract-playground/src/RTLO.sol new file mode 100644 index 000000000..c2a9cf0d1 --- /dev/null +++ b/tests/contract-playground/src/RTLO.sol @@ -0,0 +1,13 @@ + +// SPDX-License-Identifier: MIT +pragma solidity 0.6.4; + + +contract RTLO { + + function rtlo() external pure returns(bytes memory _rtlo) + { + _rtlo = bytes("/*owner‮/*"); + } + +} \ No newline at end of file diff --git a/tests/contract-playground/src/SendEtherNoChecks.sol b/tests/contract-playground/src/SendEtherNoChecks.sol new file mode 100644 index 000000000..beea34154 --- /dev/null +++ b/tests/contract-playground/src/SendEtherNoChecks.sol @@ -0,0 +1,103 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.19; + + +////// GOOD //////////////// + +contract SendEtherNoChecks2 { + + function callAndSendNativeEth(address x) internal { + (bool success,) = x.call{value: 10}("calldata"); + if (!success) { + revert(); + } + } + + modifier mod1(address x) { + callAndSendNativeEth(x); + _; + } + + // Start Here + function func1(address x) external mod1(x) { + func2(); + } + + function func2() internal view { + require(msg.sender == address(0x11)); + } + +} + +/////////// BAD /////////////// + +// Sending eth from func1 in the following contracts is not safe because there is no check on any address +// before sending native eth. + +/// BAD +contract SendEtherNoChecks3 { + + function callAndSendNativeEth(address x) internal { + (bool success,) = x.call{value: 10}("calldata"); + if (!success) { + revert(); + } + } + + modifier mod1(address x) { + callAndSendNativeEth(x); + _; + } + + // Start Here + function func1(address x) external mod1(x) { + + } + +} + + + +// BAD +contract SendEtherNoChecks4 { + + uint256 public constant BAL = 100; + + function transferBalance(address x) internal { + payable(x).transfer(BAL); + } + + modifier mod1(address x) { + transferBalance(x); + + _; + } + + // Start Here + function func1(address x) external mod1(x) { + + } + +} + +// BAD +contract SendEtherNoChecks5 { + + uint256 public constant BAL = 100; + + function sendBalance(address x) internal { + (bool success) = payable(x).send(BAL); + require(success, "Unable to send balance"); + } + + modifier mod1(address x) { + sendBalance(x); + _; + } + + // Start Here + function func1(address x) external mod1(x) { + + } + +} diff --git a/tests/contract-playground/src/TautologicalCompare.sol b/tests/contract-playground/src/TautologicalCompare.sol new file mode 100644 index 000000000..1d53adc02 --- /dev/null +++ b/tests/contract-playground/src/TautologicalCompare.sol @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.20; + + +contract TautologicalCompare { + + uint256 public constant f = 7; + uint256 public constant g = 7; + + + function check(uint a) external pure returns(bool){ + // Tautology + return (a >= a); + } + + function check2() external pure returns(bool){ + // Tautology + return (f >= 7); + } + + function check3() external pure returns(bool){ + // Tautology + return (f < f); + } + + function check4() external pure returns(bool){ + // Tautology + return (f < g); + } + +} \ No newline at end of file diff --git a/tests/contract-playground/src/UncheckedReturn.sol b/tests/contract-playground/src/UncheckedReturn.sol new file mode 100644 index 000000000..6f86f6d9a --- /dev/null +++ b/tests/contract-playground/src/UncheckedReturn.sol @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity 0.8.19; + + + +contract UncheckedReturnExample { + + function one() public pure returns(uint256) { + return 1; + } + + function callOneAndDoNothing() internal pure { + // BAD we're not doing anything with 1 + one(); + } + + event OneCalled(uint256 what); + error NotTwo(); + + function callOneAndDoSomething() internal { + // GOOD (we're passing one to emit) + emit OneCalled(one()); + } + + function callTwoAndDoNothing() internal pure { + // BAD (we're doing nothing) + UncheckedHelperExternal(address(0x12345)).two(); + } + + function callTwoAndDoSomething() internal pure { + // GOOD (we're storing the return value in a variable) + uint256 _answer = UncheckedHelperExternal(address(0x12345)).two(); + } + + function callTwoAndRequireSomething() internal pure { + // GOOD (we're using the return value in a require) + require(UncheckedHelperExternal(address(0x12345)).two() == 2, "Not two"); + } + + function callTwoAndEmitError() internal pure { + // GOOD (we're using the return value in an error) + if (UncheckedHelperExternal(address(0x12345)).two() != 2) { + revert NotTwo(); + } + } + +} + + +contract UncheckedHelperExternal { + + function two() external pure returns(uint256) { + return 2; + } + +} \ No newline at end of file diff --git a/tests/contract-playground/src/UninitializedStateVariable.sol b/tests/contract-playground/src/UninitializedStateVariable.sol new file mode 100644 index 000000000..2b367f5d6 --- /dev/null +++ b/tests/contract-playground/src/UninitializedStateVariable.sol @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: No License + +pragma solidity 0.8.25; + +contract UninitializedStateVariable { + + string public s_author; // BAD (because it's used without initializing) + string public s_publisher = "Blockchain Ltd."; // GOOD (because it's initialized here.) + uint256 public numPages; // GOOD (because it's initialized in constructor) + + // For arrays and mappings, it's okay to use them without initializing + uint256[] public arr; // GOOD + mapping(uint256 => uint256[]) private map; // GOOD + + address destination; // BAD + + function transfer() payable public { + payable(destination).transfer(msg.value); // `destination` does not have any assignments. + } + + event TellEveryone(string); + + constructor() { + numPages = 100; // Initialize the numPages, but not s_author + } + + function tell() external { + emit TellEveryone(s_author); + string memory description = string.concat(s_author, s_publisher); + emit TellEveryone(description); + } + +} + + +contract UninitializedStateVariableBase { + uint256 public myVar; // initialized in extension, hence not captured +} + +contract UninitializedStateVariableExtension is UninitializedStateVariableBase { + constructor() { + myVar = 4; + } +} \ No newline at end of file diff --git a/tests/contract-playground/src/WeakRandomness.sol b/tests/contract-playground/src/WeakRandomness.sol new file mode 100644 index 000000000..ec8438fa7 --- /dev/null +++ b/tests/contract-playground/src/WeakRandomness.sol @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.20; + +contract WeakRandomness { + function getRandomNumber1() external view returns (uint256) { + uint256 randomNumber = uint256(keccak256(abi.encodePacked(msg.sender, block.number, block.timestamp))); + return randomNumber; + } + + function getRandomNumber2() external view returns (uint256) { + return uint256(keccak256(abi.encodePacked(block.number))); + } + + function getRandomNumber3() external view returns (uint256) { + bytes memory someBytes = abi.encode(block.number, msg.sender); + return uint256(keccak256(someBytes)); + } + + function getRandomNumber4() external view returns (uint256) { + bytes memory someBytes = abi.encodePacked(block.number, msg.sender); + return uint256(keccak256(someBytes)); + } + + function getRandomNumberUsingModulo1() external view returns (uint256) { + return block.timestamp % 10; + } + + function getRandomNumberUsingModulo2() external view returns (uint256) { + uint256 a = block.number; + uint256 b = 123; + return a % b; + } + + function getRandomNumberUsingModulo3() external view returns (uint256) { + uint256 randomNumber = uint256(blockhash(block.number)) % 10; + return randomNumber; + } + + function getRandomNumberUsingModulo4() external view returns (uint256) { + uint256 hash = uint256(blockhash(12345)); + return hash % 10; + } + + function getRandomNumberUsingPrevrandao() external view returns (uint256) { + uint256 randomNumber = block.prevrandao; + return randomNumber; + } + + // good + function timestamp() external view returns (uint256) { + return block.timestamp; + } + + function number() external view returns (uint256) { + return block.number; + } + + function encode() external view returns (bytes memory) { + return abi.encode(block.timestamp, block.number); + } + + function encodePacked() external view returns (bytes memory) { + return abi.encodePacked(block.timestamp, block.number); + } + + function moduloOperation(uint256 a, uint256 b) external pure returns (uint256) { + return a % b; + } + + function getBlockHashAsUint(uint256 blockNumber) external view returns (uint256) { + return uint256(blockhash(blockNumber)); + } + + function getDifficulty() external view returns (uint256) { + return block.difficulty; + } +}