From 57a13281b01be02a6c0844e61af4da5280d91a08 Mon Sep 17 00:00:00 2001 From: Tilak Madichetti Date: Sun, 28 Jul 2024 23:22:28 +0530 Subject: [PATCH 1/9] Global pre-processing for leveraging callgraphs (#605) Co-authored-by: Alex Roan Co-authored-by: Tilak Madichetti --- Cargo.lock | 1 + aderyn_core/Cargo.toml | 1 + .../src/audit/public_functions_no_sender.rs | 2 +- aderyn_core/src/context/browser/extractor.rs | 37 ++ aderyn_core/src/context/graph/mod.rs | 32 ++ aderyn_core/src/context/graph/traits.rs | 4 + .../src/context/graph/workspace_callgraph.rs | 134 +++++ .../context/investigator/callgraph_tests.rs | 275 ++++++++++ aderyn_core/src/context/investigator/mod.rs | 46 ++ .../src/context/investigator/standard.rs | 372 +++++++++++++ .../src/context/investigator/traits.rs | 78 +++ aderyn_core/src/context/mod.rs | 2 + aderyn_core/src/context/workspace_context.rs | 3 + aderyn_core/src/detect/detector.rs | 21 +- aderyn_core/src/detect/helpers.rs | 107 +++- .../high/delegate_call_no_address_check.rs | 132 +++++ aderyn_core/src/detect/high/mod.rs | 4 + .../src/detect/high/send_ether_no_checks.rs | 118 ++++ .../src/detect/test_utils/load_source_unit.rs | 31 +- aderyn_core/src/detect/test_utils/mod.rs | 1 + aderyn_driver/src/driver.rs | 10 +- .../adhoc-sol-files-highs-only-report.json | 17 +- reports/adhoc-sol-files-report.md | 20 +- reports/report.json | 304 ++++++++++- reports/report.md | 333 +++++++++++- reports/report.sarif | 513 ++++++++++++++++++ .../src/CallGraphTests.sol | 75 +++ .../src/DelegateCallWithoutAddressCheck.sol | 52 ++ .../src/SendEtherNoChecks.sol | 103 ++++ 29 files changed, 2789 insertions(+), 39 deletions(-) create mode 100644 aderyn_core/src/context/graph/mod.rs create mode 100644 aderyn_core/src/context/graph/traits.rs create mode 100644 aderyn_core/src/context/graph/workspace_callgraph.rs create mode 100644 aderyn_core/src/context/investigator/callgraph_tests.rs create mode 100644 aderyn_core/src/context/investigator/mod.rs create mode 100644 aderyn_core/src/context/investigator/standard.rs create mode 100644 aderyn_core/src/context/investigator/traits.rs create mode 100644 aderyn_core/src/detect/high/delegate_call_no_address_check.rs create mode 100644 aderyn_core/src/detect/high/send_ether_no_checks.rs create mode 100644 tests/contract-playground/src/CallGraphTests.sol create mode 100644 tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol create mode 100644 tests/contract-playground/src/SendEtherNoChecks.sol diff --git a/Cargo.lock b/Cargo.lock index cde268557..1f422648c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -42,6 +42,7 @@ version = "0.1.7" dependencies = [ "crossbeam-channel", "cyfrin-foundry-compilers", + "derive_more", "eyre", "ignore", "once_cell", diff --git a/aderyn_core/Cargo.toml b/aderyn_core/Cargo.toml index 53990002d..33bd0e902 100644 --- a/aderyn_core/Cargo.toml +++ b/aderyn_core/Cargo.toml @@ -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/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..2ab6290d6 --- /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.to_string()) + .unwrap(), + ) + } + + fn get_modifier_definition_by_name(context: &WorkspaceContext, name: &str) -> ASTNode { + ASTNode::from( + context + .modifier_definitions() + .into_iter() + .find(|&x| x.name == name.to_string()) + .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 2ba2c48b1..2359ab314 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -8,11 +8,12 @@ use crate::{ high::{ ArbitraryTransferFromDetector, AvoidAbiEncodePackedDetector, BlockTimestampDeadlineDetector, DangerousUnaryOperatorDetector, - DelegateCallInLoopDetector, DynamicArrayLengthAssignmentDetector, - EnumerableLoopRemovalDetector, ExperimentalEncoderDetector, - IncorrectShiftOrderDetector, IncorrectUseOfCaretOperatorDetector, - MultipleConstructorsDetector, NestedStructInMappingDetector, RTLODetector, - ReusedContractNameDetector, SelfdestructIdentifierDetector, + DelegateCallInLoopDetector, DelegateCallOnUncheckedAddressDetector, + DynamicArrayLengthAssignmentDetector, EnumerableLoopRemovalDetector, + ExperimentalEncoderDetector, IncorrectShiftOrderDetector, + IncorrectUseOfCaretOperatorDetector, MultipleConstructorsDetector, + NestedStructInMappingDetector, RTLODetector, ReusedContractNameDetector, + SelfdestructIdentifierDetector, SendEtherNoChecksDetector, StateVariableShadowingDetector, StorageArrayEditWithMemoryDetector, TautologicalCompareDetector, UncheckedReturnDetector, UninitializedStateVariableDetector, UnprotectedInitializerDetector, @@ -83,6 +84,8 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), + Box::::default(), Box::::default(), Box::::default(), Box::::default(), @@ -140,6 +143,8 @@ pub(crate) enum IssueDetectorNamePool { IncorrectCaretOperator, YulReturn, StateVariableShadowing, + SendEtherNoChecks, + DelegateCallUncheckedAddress, TautologicalCompare, #[allow(clippy::upper_case_acronyms)] RTLO, @@ -265,6 +270,12 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::SendEtherNoChecks => { + Some(Box::::default()) + } + IssueDetectorNamePool::DelegateCallUncheckedAddress => { + Some(Box::::default()) + } IssueDetectorNamePool::TautologicalCompare => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/helpers.rs b/aderyn_core/src/detect/helpers.rs index 0bf8e34f3..d7b535eef 100644 --- a/aderyn_core/src/detect/helpers.rs +++ b/aderyn_core/src/detect/helpers.rs @@ -6,7 +6,10 @@ use crate::{ VariableDeclaration, Visibility, }, context::{ - browser::{ExtractBinaryOperations, ExtractMemberAccesses}, + browser::{ + ExtractBinaryOperations, ExtractFunctionCallOptions, ExtractFunctionCalls, + ExtractMemberAccesses, + }, workspace_context::WorkspaceContext, }, }; @@ -69,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| { @@ -98,6 +101,102 @@ 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, 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..238002154 --- /dev/null +++ b/aderyn_core/src/detect/high/delegate_call_no_address_check.rs @@ -0,0 +1,132 @@ +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() + } +} + +#[cfg(test)] +mod delegate_call_no_address_check_tests { + use crate::detect::{ + detector::IssueDetector, + high::delegate_call_no_address_check::DelegateCallOnUncheckedAddressDetector, + }; + + #[test] + 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") + ); + } +} + +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(()) + } +} diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index c7fa87a45..cacae63d6 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -3,6 +3,7 @@ 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 dynamic_array_length_assignment; pub(crate) mod enumerable_loop_removal; pub(crate) mod experimental_encoder; @@ -13,6 +14,7 @@ pub(crate) mod nested_struct_in_mapping; 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; @@ -27,6 +29,7 @@ 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 dynamic_array_length_assignment::DynamicArrayLengthAssignmentDetector; pub use enumerable_loop_removal::EnumerableLoopRemovalDetector; pub use experimental_encoder::ExperimentalEncoderDetector; @@ -37,6 +40,7 @@ pub use nested_struct_in_mapping::NestedStructInMappingDetector; 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; 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..d86b0774c --- /dev/null +++ b/aderyn_core/src/detect/high/send_ether_no_checks.rs @@ -0,0 +1,118 @@ +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 crate::detect::{ + detector::IssueDetector, high::send_ether_no_checks::SendEtherNoChecksDetector, + }; + + #[test] + 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/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/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/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 6a54a00ca..828564194 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": 2, + "high": 3, "low": 0 }, "high_issues": { @@ -142,6 +142,19 @@ "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" + } + ] } ] }, @@ -168,6 +181,8 @@ "incorrect-caret-operator", "yul-return", "state-variable-shadowing", + "send-ether-no-checks", + "delegate-call-unchecked-address", "tautological-compare", "rtlo", "unchecked-return", diff --git a/reports/adhoc-sol-files-report.md b/reports/adhoc-sol-files-report.md index 654785eb4..17dbae04d 100644 --- a/reports/adhoc-sol-files-report.md +++ b/reports/adhoc-sol-files-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: 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) @@ -70,7 +71,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 2 | +| High | 3 | | Low | 16 | @@ -134,6 +135,23 @@ Solidity does initialize variables by default when you declare them, however it' +## 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/report.json b/reports/report.json index 8e01d3d6d..80d2ddcca 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 60, - "total_sloc": 1620 + "total_source_units": 63, + "total_sloc": 1758 }, "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 @@ -45,6 +49,10 @@ "file_path": "src/DangerousUnaryOperator.sol", "n_sloc": 13 }, + { + "file_path": "src/DelegateCallWithoutAddressCheck.sol", + "n_sloc": 31 + }, { "file_path": "src/DeprecatedOZFunctions.sol", "n_sloc": 32 @@ -109,6 +117,10 @@ "file_path": "src/RevertsAndRequriesInLoops.sol", "n_sloc": 27 }, + { + "file_path": "src/SendEtherNoChecks.sol", + "n_sloc": 58 + }, { "file_path": "src/StateShadowing.sol", "n_sloc": 17 @@ -248,7 +260,7 @@ ] }, "issue_count": { - "high": 23, + "high": 25, "low": 23 }, "high_issues": { @@ -1215,6 +1227,12 @@ "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, @@ -1346,6 +1364,68 @@ } ] }, + { + "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.", @@ -1615,6 +1695,12 @@ "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, @@ -1658,6 +1744,12 @@ "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/InconsistentUints.sol", "line_no": 1, @@ -1967,6 +2059,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, @@ -2113,6 +2223,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, @@ -2131,6 +2271,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, @@ -2242,6 +2400,12 @@ "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/DeprecatedOZFunctions.sol", "line_no": 2, @@ -2369,6 +2533,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, @@ -2381,6 +2569,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, @@ -2406,6 +2618,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, @@ -2466,6 +2690,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, @@ -2666,12 +2908,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, @@ -2946,6 +3242,8 @@ "incorrect-caret-operator", "yul-return", "state-variable-shadowing", + "send-ether-no-checks", + "delegate-call-unchecked-address", "tautological-compare", "rtlo", "unchecked-return", diff --git a/reports/report.md b/reports/report.md index bea26761b..56b9e5d2b 100644 --- a/reports/report.md +++ b/reports/report.md @@ -27,10 +27,12 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [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: Tautological comparison.](#h-20-tautological-comparison) - - [H-21: RTLO character detected in file. \u{202e}](#h-21-rtlo-character-detected-in-file-u202e) - - [H-22: Return value of the function call is not checked.](#h-22-return-value-of-the-function-call-is-not-checked) - - [H-23: Dangerous unary operator found in assignment.](#h-23-dangerous-unary-operator-found-in-assignment) + - [H-20: Sending native Eth is not protected from these functions.](#h-20-sending-native-eth-is-not-protected-from-these-functions) + - [H-21: Delegatecall made by the function without checks on any adress.](#h-21-delegatecall-made-by-the-function-without-checks-on-any-adress) + - [H-22: Tautological comparison.](#h-22-tautological-comparison) + - [H-23: RTLO character detected in file. \u{202e}](#h-23-rtlo-character-detected-in-file-u202e) + - [H-24: Return value of the function call is not checked.](#h-24-return-value-of-the-function-call-is-not-checked) + - [H-25: Dangerous unary operator found in assignment.](#h-25-dangerous-unary-operator-found-in-assignment) - [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) @@ -63,8 +65,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 60 | -| Total nSLOC | 1620 | +| .sol Files | 63 | +| Total nSLOC | 1758 | ## Files Details @@ -75,12 +77,14 @@ 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/DeprecatedOZFunctions.sol | 32 | | src/DivisionBeforeMultiplication.sol | 22 | | src/DynamicArrayLengthAssignment.sol | 16 | @@ -97,6 +101,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/OnceModifierExample.sol | 8 | | src/RTLO.sol | 7 | | src/RevertsAndRequriesInLoops.sol | 27 | +| src/SendEtherNoChecks.sol | 58 | | src/StateShadowing.sol | 17 | | src/StateVariables.sol | 58 | | src/StorageConditionals.sol | 59 | @@ -131,14 +136,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** | **1620** | +| **Total** | **1758** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 23 | +| High | 25 | | Low | 23 | @@ -1161,7 +1166,7 @@ If the length of a dynamic array (storage variable) directly assigned to, it may Solidity does initialize variables by default when you declare them, however it's good practice to explicitly declare an initial value. For example, if you transfer money to an address we must make sure that the address has been initialized. -
12 Found Instances +
13 Found Instances - Found in src/AssemblyExample.sol [Line: 5](../tests/contract-playground/src/AssemblyExample.sol#L5) @@ -1170,6 +1175,12 @@ Solidity does initialize variables by default when you declare them, however it' 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 @@ -1315,7 +1326,77 @@ Description of the high issue. -## H-20: Tautological comparison. +## H-20: 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-21: 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-22: 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. @@ -1350,7 +1431,7 @@ The left hand side and the right hand side of the binary operation has the same -## H-21: RTLO character detected in file. \u{202e} +## H-23: RTLO character detected in file. \u{202e} Right to left override character may be misledaing and cause potential attacks by visually misordering method arguments! @@ -1367,7 +1448,7 @@ Right to left override character may be misledaing and cause potential attacks b -## H-22: Return value of the function call is not checked. +## H-24: Return value of the function call is not checked. Function returns a value but it is ignored. @@ -1390,7 +1471,7 @@ Function returns a value but it is ignored. -## H-23: Dangerous unary operator found in assignment. +## H-25: Dangerous unary operator found in assignment. Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in between. @@ -1568,7 +1649,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. -
10 Found Instances +
11 Found Instances - Found in src/ArbitraryTransferFrom.sol [Line: 16](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L16) @@ -1619,6 +1700,12 @@ 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 @@ -1639,7 +1726,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;` -
12 Found Instances +
13 Found Instances - Found in src/ContractWithTodo.sol [Line: 2](../tests/contract-playground/src/ContractWithTodo.sol#L2) @@ -1666,6 +1753,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid 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/InconsistentUints.sol [Line: 1](../tests/contract-playground/src/InconsistentUints.sol#L1) ```solidity @@ -1918,7 +2011,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. -
26 Found Instances +
29 Found Instances - Found in src/Casting.sol [Line: 16](../tests/contract-playground/src/Casting.sol#L16) @@ -1987,6 +2080,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 @@ -2132,8 +2237,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) @@ -2153,6 +2288,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 @@ -2249,7 +2402,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. -
24 Found Instances +
25 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -2276,6 +2429,12 @@ 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/DeprecatedOZFunctions.sol [Line: 2](../tests/contract-playground/src/DeprecatedOZFunctions.sol#L2) ```solidity @@ -2404,8 +2563,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) @@ -2419,6 +2602,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 @@ -2439,7 +2646,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) @@ -2448,6 +2655,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 @@ -2508,6 +2727,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 @@ -2713,8 +2950,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) @@ -2722,6 +2983,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 diff --git a/reports/report.sarif b/reports/report.sarif index d1edc11fb..4cf416255 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -1704,6 +1704,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 337 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -1935,6 +1946,112 @@ }, "ruleId": "state-variable-shadowing" }, + { + "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/auditor_mode/ExternalCalls.sol" + }, + "region": { + "byteLength": 28, + "byteOffset": 1253 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/inheritance/ExtendedInheritance.sol" + }, + "region": { + "byteLength": 15, + "byteOffset": 391 + } + } + } + ], + "message": { + "text": "Introduce checks on the address" + }, + "ruleId": "delegate-call-unchecked-address" + }, { "level": "warning", "locations": [ @@ -2384,6 +2501,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 19, + "byteOffset": 1255 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2459,6 +2587,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3014,6 +3153,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": { @@ -3274,6 +3446,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": { @@ -3307,6 +3534,39 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 6, + "byteOffset": 268 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 513 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 6, + "byteOffset": 920 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3499,6 +3759,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3728,6 +3999,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": { @@ -3750,6 +4065,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": { @@ -3792,6 +4151,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": { @@ -3902,6 +4283,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": { @@ -4261,6 +4675,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": { @@ -4272,6 +4730,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": { 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/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/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) { + + } + +} From a9cccd8c17cf61b859d1cced7d772bdd36c8fc09 Mon Sep 17 00:00:00 2001 From: Tilak Madichetti Date: Sun, 28 Jul 2024 23:43:02 +0530 Subject: [PATCH 2/9] Detector: Misused boolean (#607) Co-authored-by: Alex Roan --- aderyn_core/src/ast/ast.rs | 66 ++++++++++ .../context/investigator/callgraph_tests.rs | 4 +- aderyn_core/src/detect/detector.rs | 33 +---- aderyn_core/src/detect/helpers.rs | 46 ++++++- .../high/delegate_call_no_address_check.rs | 40 +++--- .../src/detect/high/misused_boolean.rs | 55 ++++++++ aderyn_core/src/detect/high/mod.rs | 2 + aderyn_core/src/detect/mod.rs | 70 +++++++++++ .../adhoc-sol-files-highs-only-report.json | 1 + reports/report.json | 78 +++++++++++- reports/report.md | 105 +++++++++++++--- reports/report.sarif | 119 ++++++++++++++++++ .../src/MisusedBoolean.sol | 82 ++++++++++++ 13 files changed, 630 insertions(+), 71 deletions(-) create mode 100644 aderyn_core/src/detect/high/misused_boolean.rs create mode 100644 tests/contract-playground/src/MisusedBoolean.sol 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/context/investigator/callgraph_tests.rs b/aderyn_core/src/context/investigator/callgraph_tests.rs index 2ab6290d6..faa0a1ccc 100644 --- a/aderyn_core/src/context/investigator/callgraph_tests.rs +++ b/aderyn_core/src/context/investigator/callgraph_tests.rs @@ -19,7 +19,7 @@ mod callgraph_tests { context .function_definitions() .into_iter() - .find(|&x| x.name == name.to_string()) + .find(|&x| x.name == *name) .unwrap(), ) } @@ -29,7 +29,7 @@ mod callgraph_tests { context .modifier_definitions() .into_iter() - .find(|&x| x.name == name.to_string()) + .find(|&x| x.name == *name) .unwrap(), ) } diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 2359ab314..11b46f801 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -4,35 +4,9 @@ use strum::{Display, EnumCount, EnumIter, EnumString}; use crate::{ ast::NodeID, context::workspace_context::WorkspaceContext, - detect::{ - high::{ - ArbitraryTransferFromDetector, AvoidAbiEncodePackedDetector, - BlockTimestampDeadlineDetector, DangerousUnaryOperatorDetector, - DelegateCallInLoopDetector, DelegateCallOnUncheckedAddressDetector, - DynamicArrayLengthAssignmentDetector, EnumerableLoopRemovalDetector, - ExperimentalEncoderDetector, IncorrectShiftOrderDetector, - IncorrectUseOfCaretOperatorDetector, MultipleConstructorsDetector, - NestedStructInMappingDetector, RTLODetector, ReusedContractNameDetector, - SelfdestructIdentifierDetector, SendEtherNoChecksDetector, - StateVariableShadowingDetector, StorageArrayEditWithMemoryDetector, - TautologicalCompareDetector, UncheckedReturnDetector, - UninitializedStateVariableDetector, 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, @@ -84,6 +58,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), Box::::default(), Box::::default(), @@ -143,6 +118,7 @@ pub(crate) enum IssueDetectorNamePool { IncorrectCaretOperator, YulReturn, StateVariableShadowing, + MisusedBoolean, SendEtherNoChecks, DelegateCallUncheckedAddress, TautologicalCompare, @@ -270,6 +246,7 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::MisusedBoolean => Some(Box::::default()), IssueDetectorNamePool::SendEtherNoChecks => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/helpers.rs b/aderyn_core/src/detect/helpers.rs index d7b535eef..f19a79c50 100644 --- a/aderyn_core/src/detect/helpers.rs +++ b/aderyn_core/src/detect/helpers.rs @@ -2,8 +2,8 @@ use semver::{Error, VersionReq}; use crate::{ ast::{ - ASTNode, Expression, FunctionDefinition, MemberAccess, NodeID, PragmaDirective, - VariableDeclaration, Visibility, + ASTNode, Expression, FunctionDefinition, Identifier, LiteralKind, MemberAccess, NodeID, + PragmaDirective, VariableDeclaration, Visibility, }, context::{ browser::{ @@ -221,3 +221,45 @@ pub fn get_literal_value_or_constant_variable_value( } None } + +/* +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/delegate_call_no_address_check.rs b/aderyn_core/src/detect/high/delegate_call_no_address_check.rs index 238002154..10c816894 100644 --- a/aderyn_core/src/detect/high/delegate_call_no_address_check.rs +++ b/aderyn_core/src/detect/high/delegate_call_no_address_check.rs @@ -68,6 +68,26 @@ impl IssueDetector for DelegateCallOnUncheckedAddressDetector { } } +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 crate::detect::{ @@ -110,23 +130,3 @@ mod delegate_call_no_address_check_tests { ); } } - -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(()) - } -} 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..201471f37 --- /dev/null +++ b/aderyn_core/src/detect/high/misused_boolean.rs @@ -0,0 +1,55 @@ +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 crate::detect::{detector::IssueDetector, high::misused_boolean::MisusedBooleanDetector}; + + #[test] + 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 cacae63d6..e247206b9 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -9,6 +9,7 @@ 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 reused_contract_name; @@ -35,6 +36,7 @@ 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 reused_contract_name::ReusedContractNameDetector; 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/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 828564194..d3797769c 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -181,6 +181,7 @@ "incorrect-caret-operator", "yul-return", "state-variable-shadowing", + "misused-boolean", "send-ether-no-checks", "delegate-call-unchecked-address", "tautological-compare", diff --git a/reports/report.json b/reports/report.json index 80d2ddcca..713071135 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 63, - "total_sloc": 1758 + "total_source_units": 64, + "total_sloc": 1825 }, "files_details": { "files_details": [ @@ -101,6 +101,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 @@ -260,7 +264,7 @@ ] }, "issue_count": { - "high": 25, + "high": 26, "low": 23 }, "high_issues": { @@ -1364,6 +1368,73 @@ } ] }, + { + "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", @@ -3242,6 +3313,7 @@ "incorrect-caret-operator", "yul-return", "state-variable-shadowing", + "misused-boolean", "send-ether-no-checks", "delegate-call-unchecked-address", "tautological-compare", diff --git a/reports/report.md b/reports/report.md index 56b9e5d2b..0ccc72620 100644 --- a/reports/report.md +++ b/reports/report.md @@ -27,12 +27,13 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [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: Sending native Eth is not protected from these functions.](#h-20-sending-native-eth-is-not-protected-from-these-functions) - - [H-21: Delegatecall made by the function without checks on any adress.](#h-21-delegatecall-made-by-the-function-without-checks-on-any-adress) - - [H-22: Tautological comparison.](#h-22-tautological-comparison) - - [H-23: RTLO character detected in file. \u{202e}](#h-23-rtlo-character-detected-in-file-u202e) - - [H-24: Return value of the function call is not checked.](#h-24-return-value-of-the-function-call-is-not-checked) - - [H-25: Dangerous unary operator found in assignment.](#h-25-dangerous-unary-operator-found-in-assignment) + - [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) - [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) @@ -65,8 +66,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 63 | -| Total nSLOC | 1758 | +| .sol Files | 64 | +| Total nSLOC | 1825 | ## Files Details @@ -97,6 +98,7 @@ 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/RTLO.sol | 7 | @@ -136,14 +138,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** | **1758** | +| **Total** | **1825** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 25 | +| High | 26 | | Low | 23 | @@ -1326,7 +1328,78 @@ Description of the high issue. -## H-20: Sending native Eth is not protected from these functions. +## 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 @@ -1367,7 +1440,7 @@ Introduce checks for `msg.sender` in the function -## H-21: Delegatecall made by the function without checks on any adress. +## H-22: Delegatecall made by the function without checks on any adress. Introduce checks on the address @@ -1396,7 +1469,7 @@ Introduce checks on the address -## H-22: Tautological comparison. +## 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. @@ -1431,7 +1504,7 @@ The left hand side and the right hand side of the binary operation has the same -## H-23: RTLO character detected in file. \u{202e} +## 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! @@ -1448,7 +1521,7 @@ Right to left override character may be misledaing and cause potential attacks b -## H-24: Return value of the function call is not checked. +## H-25: Return value of the function call is not checked. Function returns a value but it is ignored. @@ -1471,7 +1544,7 @@ Function returns a value but it is ignored. -## H-25: Dangerous unary operator found in assignment. +## H-26: Dangerous unary operator found in assignment. Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in between. diff --git a/reports/report.sarif b/reports/report.sarif index 4cf416255..9ae28c6c8 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -1946,6 +1946,125 @@ }, "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": [ 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 From e949a4d68ec5c349395793ede44821a47877408b Mon Sep 17 00:00:00 2001 From: Tilak Madichetti Date: Mon, 29 Jul 2024 00:37:29 +0530 Subject: [PATCH 3/9] Detector: Deletion of nested mapping (#616) Co-authored-by: Alex Roan --- aderyn_core/src/detect/detector.rs | 5 + .../detect/high/deletion_nested_mapping.rs | 135 ++++++++++++++++++ aderyn_core/src/detect/high/mod.rs | 2 + .../adhoc-sol-files-highs-only-report.json | 3 +- reports/report.json | 38 ++++- reports/report.md | 43 +++++- reports/report.sarif | 42 ++++++ reports/templegold-report.md | 20 ++- ...DeletionNestedMappingStructureContract.sol | 20 +++ 9 files changed, 296 insertions(+), 12 deletions(-) create mode 100644 aderyn_core/src/detect/high/deletion_nested_mapping.rs create mode 100644 tests/contract-playground/src/DeletionNestedMappingStructureContract.sol diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 11b46f801..91264b88c 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -65,6 +65,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), ] } @@ -126,6 +127,7 @@ pub(crate) enum IssueDetectorNamePool { RTLO, UncheckedReturn, DangerousUnaryOperator, + 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, @@ -261,6 +263,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::DeleteNestedMapping => { + Some(Box::::default()) + } IssueDetectorNamePool::Undecided => None, } } 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..17820714f --- /dev/null +++ b/aderyn_core/src/detect/high/deletion_nested_mapping.rs @@ -0,0 +1,135 @@ +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 crate::detect::{ + detector::IssueDetector, high::deletion_nested_mapping::DeletionNestedMappingDetector, + }; + + #[test] + 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/mod.rs b/aderyn_core/src/detect/high/mod.rs index e247206b9..b23275b99 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -4,6 +4,7 @@ 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; @@ -31,6 +32,7 @@ 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; diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index d3797769c..5ade44f99 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -187,6 +187,7 @@ "tautological-compare", "rtlo", "unchecked-return", - "dangerous-unary-operator" + "dangerous-unary-operator", + "delete-nested-mapping" ] } \ No newline at end of file diff --git a/reports/report.json b/reports/report.json index 713071135..96bc6360c 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 64, - "total_sloc": 1825 + "total_source_units": 65, + "total_sloc": 1836 }, "files_details": { "files_details": [ @@ -53,6 +53,10 @@ "file_path": "src/DelegateCallWithoutAddressCheck.sol", "n_sloc": 31 }, + { + "file_path": "src/DeletionNestedMappingStructureContract.sol", + "n_sloc": 11 + }, { "file_path": "src/DeprecatedOZFunctions.sol", "n_sloc": 32 @@ -264,7 +268,7 @@ ] }, "issue_count": { - "high": 26, + "high": 27, "low": 23 }, "high_issues": { @@ -1578,6 +1582,19 @@ "src_char": "247:10" } ] + }, + { + "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" + } + ] } ] }, @@ -1821,6 +1838,12 @@ "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, @@ -2477,6 +2500,12 @@ "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, @@ -3319,6 +3348,7 @@ "tautological-compare", "rtlo", "unchecked-return", - "dangerous-unary-operator" + "dangerous-unary-operator", + "delete-nested-mapping" ] } \ No newline at end of file diff --git a/reports/report.md b/reports/report.md index 0ccc72620..c4bf1036c 100644 --- a/reports/report.md +++ b/reports/report.md @@ -34,6 +34,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [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: Deletion from a nested mappping.](#h-27-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) @@ -66,8 +67,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 64 | -| Total nSLOC | 1825 | +| .sol Files | 65 | +| Total nSLOC | 1836 | ## Files Details @@ -86,6 +87,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | 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 | @@ -138,14 +140,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** | **1825** | +| **Total** | **1836** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 26 | +| High | 27 | | Low | 23 | @@ -1567,6 +1569,23 @@ Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in +## H-27: 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 src/DeletionNestedMappingStructureContract.sol [Line: 17](../tests/contract-playground/src/DeletionNestedMappingStructureContract.sol#L17) + + ```solidity + delete people[msg.sender]; + ``` + +
+ + + # Low Issues ## L-1: Centralization Risk for trusted owners @@ -1799,7 +1818,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;` -
13 Found Instances +
14 Found Instances - Found in src/ContractWithTodo.sol [Line: 2](../tests/contract-playground/src/ContractWithTodo.sol#L2) @@ -1832,6 +1851,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid 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 @@ -2475,7 +2500,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. -
25 Found Instances +
26 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -2508,6 +2533,12 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai 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 diff --git a/reports/report.sarif b/reports/report.sarif index 9ae28c6c8..d8bda7569 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -2306,6 +2306,26 @@ }, "ruleId": "dangerous-unary-operator" }, + { + "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", "locations": [ @@ -2717,6 +2737,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DeletionNestedMappingStructureContract.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3889,6 +3920,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DeletionNestedMappingStructureContract.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { diff --git a/reports/templegold-report.md b/reports/templegold-report.md index ebc825a93..97bac719f 100644 --- a/reports/templegold-report.md +++ b/reports/templegold-report.md @@ -14,6 +14,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [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: Deletion from a nested mappping.](#h-7-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) @@ -185,7 +186,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 6 | +| High | 7 | | Low | 18 | @@ -485,6 +486,23 @@ Function returns a value but it is ignored. +## H-7: 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/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 From f1c84b6684ea4f81791dd5d64279161ac05b309f Mon Sep 17 00:00:00 2001 From: Alex Roan Date: Sun, 28 Jul 2024 21:10:32 +0100 Subject: [PATCH 4/9] Bump version 0.1.8 (#626) --- Cargo.lock | 8 ++++---- aderyn/Cargo.toml | 6 +++--- aderyn_core/Cargo.toml | 4 ++-- aderyn_driver/Cargo.toml | 6 +++--- aderyn_py/Cargo.toml | 6 +++--- reports/report.sarif | 4 ++-- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1f422648c..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,7 +38,7 @@ dependencies = [ [[package]] name = "aderyn_core" -version = "0.1.7" +version = "0.1.8" dependencies = [ "crossbeam-channel", "cyfrin-foundry-compilers", @@ -61,7 +61,7 @@ dependencies = [ [[package]] name = "aderyn_driver" -version = "0.1.7" +version = "0.1.8" dependencies = [ "aderyn_core", "criterion", @@ -76,7 +76,7 @@ dependencies = [ [[package]] name = "aderyn_py" -version = "0.1.7" +version = "0.1.8" dependencies = [ "aderyn_driver", "pyo3", diff --git a/aderyn/Cargo.toml b/aderyn/Cargo.toml index c41649eaa..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" diff --git a/aderyn_core/Cargo.toml b/aderyn_core/Cargo.toml index 33bd0e902..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" 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_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/report.sarif b/reports/report.sarif index d8bda7569..756692d50 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -5356,8 +5356,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" } } } From bb4cecfebf1b432a6ea4d8fb13e05a6d22d51f38 Mon Sep 17 00:00:00 2001 From: Alex Roan Date: Sun, 28 Jul 2024 21:32:26 +0100 Subject: [PATCH 5/9] Remove templegold from CI report checks (#627) --- .github/workflows/cargo.yml | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/.github/workflows/cargo.yml b/.github/workflows/cargo.yml index 9263a295e..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,15 +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 From fc3d7609c8c403b33c88506f6cce03c14eb455fe Mon Sep 17 00:00:00 2001 From: Tilak Madichetti Date: Mon, 29 Jul 2024 02:21:02 +0530 Subject: [PATCH 6/9] Detector: Usage of pre-declared variables. (#617) Co-authored-by: Alex Roan --- aderyn_core/src/detect/detector.rs | 5 + aderyn_core/src/detect/helpers.rs | 13 ++ aderyn_core/src/detect/high/_template.rs | 3 + .../detect/high/dangerous_unary_operator.rs | 3 + .../high/delegate_call_no_address_check.rs | 3 + .../detect/high/deletion_nested_mapping.rs | 3 + .../high/dynamic_array_length_assignment.rs | 3 + .../detect/high/enumerable_loop_removal.rs | 3 + .../src/detect/high/experimental_encoder.rs | 3 + .../detect/high/incorrect_caret_operator.rs | 3 + .../src/detect/high/incorrect_shift_order.rs | 3 + .../src/detect/high/misused_boolean.rs | 3 + aderyn_core/src/detect/high/mod.rs | 2 + .../src/detect/high/multiple_constructors.rs | 4 + .../detect/high/nested_struct_in_mapping.rs | 4 + .../high/pre_declared_variable_usage.rs | 139 ++++++++++++++++++ .../src/detect/high/reused_contract_name.rs | 3 + aderyn_core/src/detect/high/rtlo.rs | 3 + aderyn_core/src/detect/high/selfdestruct.rs | 3 + .../src/detect/high/send_ether_no_checks.rs | 3 + .../detect/high/state_variable_shadowing.rs | 3 + .../high/storage_array_edit_with_memory.rs | 3 + .../src/detect/high/tautological_compare.rs | 3 + .../src/detect/high/unchecked_return.rs | 3 + .../high/uninitialized_state_variable.rs | 3 + aderyn_core/src/detect/high/unsafe_casting.rs | 3 + aderyn_core/src/detect/high/yul_return.rs | 3 + aderyn_core/src/detect/low/_template.rs | 3 + .../low/division_before_multiplication.rs | 3 + .../low/reverts_and_requries_in_loops.rs | 3 + aderyn_core/src/detect/low/useless_error.rs | 3 + .../adhoc-sol-files-highs-only-report.json | 1 + reports/report.json | 42 +++++- reports/report.md | 53 ++++++- reports/report.sarif | 53 +++++++ .../src/PreDeclaredVarUsage.sol | 17 +++ 36 files changed, 397 insertions(+), 11 deletions(-) create mode 100644 aderyn_core/src/detect/high/pre_declared_variable_usage.rs create mode 100644 tests/contract-playground/src/PreDeclaredVarUsage.sol diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 91264b88c..5fb7ee435 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -65,6 +65,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), ] } @@ -127,6 +128,7 @@ pub(crate) enum IssueDetectorNamePool { RTLO, UncheckedReturn, DangerousUnaryOperator, + 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 @@ -263,6 +265,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::PreDeclaredLocalVariableUsage => { + Some(Box::::default()) + } IssueDetectorNamePool::DeleteNestedMapping => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/helpers.rs b/aderyn_core/src/detect/helpers.rs index f19a79c50..97851e33d 100644 --- a/aderyn_core/src/detect/helpers.rs +++ b/aderyn_core/src/detect/helpers.rs @@ -222,6 +222,19 @@ pub fn get_literal_value_or_constant_variable_value( 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 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 index 92d75d2d0..3d079bc97 100644 --- a/aderyn_core/src/detect/high/dangerous_unary_operator.rs +++ b/aderyn_core/src/detect/high/dangerous_unary_operator.rs @@ -55,11 +55,14 @@ impl IssueDetector for DangerousUnaryOperatorDetector { #[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", 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 index 10c816894..19ec1e134 100644 --- a/aderyn_core/src/detect/high/delegate_call_no_address_check.rs +++ b/aderyn_core/src/detect/high/delegate_call_no_address_check.rs @@ -90,12 +90,15 @@ impl<'a> StandardInvestigatorVisitor for DelegateCallNoAddressChecksTracker<'a> #[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", diff --git a/aderyn_core/src/detect/high/deletion_nested_mapping.rs b/aderyn_core/src/detect/high/deletion_nested_mapping.rs index 17820714f..8676956e2 100644 --- a/aderyn_core/src/detect/high/deletion_nested_mapping.rs +++ b/aderyn_core/src/detect/high/deletion_nested_mapping.rs @@ -100,11 +100,14 @@ impl IssueDetector for DeletionNestedMappingDetector { #[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", 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 index 201471f37..a726674f0 100644 --- a/aderyn_core/src/detect/high/misused_boolean.rs +++ b/aderyn_core/src/detect/high/misused_boolean.rs @@ -37,9 +37,12 @@ issue_detector! { #[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", diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index b23275b99..8d88f74ec 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -13,6 +13,7 @@ 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; @@ -41,6 +42,7 @@ 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; 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 index 32a567cd6..83577d0fe 100644 --- a/aderyn_core/src/detect/high/rtlo.rs +++ b/aderyn_core/src/detect/high/rtlo.rs @@ -59,9 +59,12 @@ impl IssueDetector for RTLODetector { #[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", 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 index d86b0774c..a20f35667 100644 --- a/aderyn_core/src/detect/high/send_ether_no_checks.rs +++ b/aderyn_core/src/detect/high/send_ether_no_checks.rs @@ -65,11 +65,14 @@ impl IssueDetector for SendEtherNoChecksDetector { #[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", 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 index 067cae476..57d91dfaa 100644 --- a/aderyn_core/src/detect/high/tautological_compare.rs +++ b/aderyn_core/src/detect/high/tautological_compare.rs @@ -165,11 +165,14 @@ impl IssueDetector for TautologicalCompareDetector { #[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", diff --git a/aderyn_core/src/detect/high/unchecked_return.rs b/aderyn_core/src/detect/high/unchecked_return.rs index 798f1e12c..cf693f533 100644 --- a/aderyn_core/src/detect/high/unchecked_return.rs +++ b/aderyn_core/src/detect/high/unchecked_return.rs @@ -80,9 +80,12 @@ impl IssueDetector for UncheckedReturnDetector { #[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", diff --git a/aderyn_core/src/detect/high/uninitialized_state_variable.rs b/aderyn_core/src/detect/high/uninitialized_state_variable.rs index 936623ab6..5006fa5e1 100644 --- a/aderyn_core/src/detect/high/uninitialized_state_variable.rs +++ b/aderyn_core/src/detect/high/uninitialized_state_variable.rs @@ -106,12 +106,15 @@ impl IssueDetector for UninitializedStateVariableDetector { #[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( 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/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/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 5ade44f99..24dab397e 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -188,6 +188,7 @@ "rtlo", "unchecked-return", "dangerous-unary-operator", + "pre-declared-local-variable-usage", "delete-nested-mapping" ] } \ No newline at end of file diff --git a/reports/report.json b/reports/report.json index 96bc6360c..b73738b3e 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 65, - "total_sloc": 1836 + "total_source_units": 66, + "total_sloc": 1845 }, "files_details": { "files_details": [ @@ -117,6 +117,10 @@ "file_path": "src/OnceModifierExample.sol", "n_sloc": 8 }, + { + "file_path": "src/PreDeclaredVarUsage.sol", + "n_sloc": 9 + }, { "file_path": "src/RTLO.sol", "n_sloc": 7 @@ -268,7 +272,7 @@ ] }, "issue_count": { - "high": 27, + "high": 28, "low": 23 }, "high_issues": { @@ -1583,6 +1587,19 @@ } ] }, + { + "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.", @@ -1850,6 +1867,12 @@ "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, @@ -2201,6 +2224,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, @@ -3349,6 +3384,7 @@ "rtlo", "unchecked-return", "dangerous-unary-operator", + "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 c4bf1036c..38e6b06bc 100644 --- a/reports/report.md +++ b/reports/report.md @@ -34,7 +34,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [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: Deletion from a nested mappping.](#h-27-deletion-from-a-nested-mappping) + - [H-27: Usage of variable before declaration.](#h-27-usage-of-variable-before-declaration) + - [H-28: Deletion from a nested mappping.](#h-28-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) @@ -67,8 +68,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 65 | -| Total nSLOC | 1836 | +| .sol Files | 66 | +| Total nSLOC | 1845 | ## Files Details @@ -103,6 +104,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | 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 | @@ -140,14 +142,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** | **1836** | +| **Total** | **1845** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 27 | +| High | 28 | | Low | 23 | @@ -1569,7 +1571,24 @@ Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in -## H-27: Deletion from a nested mappping. +## H-27: Usage of variable before declaration. + +This is a bad practice that may lead to unintended consequences. Please declare the variable before using it. + +
1 Found Instances + + +- Found in src/PreDeclaredVarUsage.sol [Line: 8](../tests/contract-playground/src/PreDeclaredVarUsage.sol#L8) + + ```solidity + a = 100; + ``` + +
+ + + +## H-28: 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. @@ -1818,7 +1837,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;` -
14 Found Instances +
15 Found Instances - Found in src/ContractWithTodo.sol [Line: 2](../tests/contract-playground/src/ContractWithTodo.sol#L2) @@ -1863,6 +1882,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid 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 @@ -2109,7 +2134,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. -
29 Found Instances +
31 Found Instances - Found in src/Casting.sol [Line: 16](../tests/contract-playground/src/Casting.sol#L16) @@ -2220,6 +2245,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 diff --git a/reports/report.sarif b/reports/report.sarif index 756692d50..1e0500698 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -2306,6 +2306,26 @@ }, "ruleId": "dangerous-unary-operator" }, + { + "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": [ @@ -2759,6 +2779,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/PreDeclaredVarUsage.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3391,6 +3422,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": { 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 From c9c251d0651cc21193584931e44b18693a9ef0fc Mon Sep 17 00:00:00 2001 From: David Drobnak <74869019+DavidDrob@users.noreply.github.com> Date: Sun, 28 Jul 2024 23:11:48 +0200 Subject: [PATCH 7/9] Detector: Weak Randomness (#618) Co-authored-by: Alex Roan --- aderyn_core/src/detect/detector.rs | 3 + aderyn_core/src/detect/high/mod.rs | 2 + .../src/detect/high/weak_randomness.rs | 190 ++++++++++++++++++ .../adhoc-sol-files-highs-only-report.json | 1 + reports/report.json | 96 ++++++++- reports/report.md | 111 +++++++++- reports/report.sarif | 152 ++++++++++++++ reports/templegold-report.md | 24 ++- .../src/WeakRandomness.sol | 77 +++++++ 9 files changed, 640 insertions(+), 16 deletions(-) create mode 100644 aderyn_core/src/detect/high/weak_randomness.rs create mode 100644 tests/contract-playground/src/WeakRandomness.sol diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 5fb7ee435..49ad141fa 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -65,6 +65,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), Box::::default(), ] @@ -128,6 +129,7 @@ pub(crate) enum IssueDetectorNamePool { RTLO, UncheckedReturn, DangerousUnaryOperator, + WeakRandomness, PreDeclaredLocalVariableUsage, DeleteNestedMapping, // NOTE: `Undecided` will be the default name (for new bots). @@ -265,6 +267,7 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::WeakRandomness => Some(Box::::default()), IssueDetectorNamePool::PreDeclaredLocalVariableUsage => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index 8d88f74ec..f5b609722 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -25,6 +25,7 @@ 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; @@ -54,4 +55,5 @@ 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/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/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 24dab397e..5b6f13587 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -188,6 +188,7 @@ "rtlo", "unchecked-return", "dangerous-unary-operator", + "weak-randomness", "pre-declared-local-variable-usage", "delete-nested-mapping" ] diff --git a/reports/report.json b/reports/report.json index b73738b3e..1e7161940 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 66, - "total_sloc": 1845 + "total_source_units": 67, + "total_sloc": 1904 }, "files_details": { "files_details": [ @@ -185,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 @@ -272,7 +276,7 @@ ] }, "issue_count": { - "high": 28, + "high": 29, "low": 23 }, "high_issues": { @@ -1587,6 +1591,67 @@ } ] }, + { + "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.", @@ -2284,6 +2349,24 @@ "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, @@ -2589,6 +2672,12 @@ "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, @@ -3384,6 +3473,7 @@ "rtlo", "unchecked-return", "dangerous-unary-operator", + "weak-randomness", "pre-declared-local-variable-usage", "delete-nested-mapping" ] diff --git a/reports/report.md b/reports/report.md index 38e6b06bc..d2cd2eeca 100644 --- a/reports/report.md +++ b/reports/report.md @@ -34,8 +34,9 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [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: Usage of variable before declaration.](#h-27-usage-of-variable-before-declaration) - - [H-28: Deletion from a nested mappping.](#h-28-deletion-from-a-nested-mappping) + - [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) @@ -68,8 +69,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 66 | -| Total nSLOC | 1845 | +| .sol Files | 67 | +| Total nSLOC | 1904 | ## Files Details @@ -121,6 +122,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | 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 | @@ -142,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** | **1845** | +| **Total** | **1904** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 28 | +| High | 29 | | Low | 23 | @@ -1571,7 +1573,72 @@ Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in -## H-27: Usage of variable before declaration. +## 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 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/WeakRandomness.sol [Line: 45](../tests/contract-playground/src/WeakRandomness.sol#L45) + + ```solidity + uint256 randomNumber = block.prevrandao; + ``` + +
+ + + +## H-28: Usage of variable before declaration. This is a bad practice that may lead to unintended consequences. Please declare the variable before using it. @@ -1588,7 +1655,7 @@ This is a bad practice that may lead to unintended consequences. Please declare -## H-28: Deletion from a nested mappping. +## H-29: 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. @@ -2134,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. -
31 Found Instances +
34 Found Instances - Found in src/Casting.sol [Line: 16](../tests/contract-playground/src/Casting.sol#L16) @@ -2305,6 +2372,24 @@ If the same constant literal value is used multiple times, create a constant sta 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 @@ -2537,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. -
26 Found Instances +
27 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -2624,6 +2709,12 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai 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 diff --git a/reports/report.sarif b/reports/report.sarif index 1e0500698..914985a7e 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -2306,6 +2306,114 @@ }, "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": [ @@ -3532,6 +3640,39 @@ } } }, + { + "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": { @@ -4072,6 +4213,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/WeakRandomness.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { diff --git a/reports/templegold-report.md b/reports/templegold-report.md index 97bac719f..b7a152e8d 100644 --- a/reports/templegold-report.md +++ b/reports/templegold-report.md @@ -14,7 +14,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [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: Deletion from a nested mappping.](#h-7-deletion-from-a-nested-mappping) + - [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) @@ -186,7 +187,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | -| High | 7 | +| High | 8 | | Low | 18 | @@ -486,7 +487,24 @@ Function returns a value but it is ignored. -## H-7: Deletion from a nested mappping. +## 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. 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; + } +} From 5273e5c31f15a5c3d333fed18094ea944fee9283 Mon Sep 17 00:00:00 2001 From: Tilak Madichetti Date: Mon, 29 Jul 2024 18:58:22 +0530 Subject: [PATCH 8/9] Detector: Unchecked `send()` on address (#611) Co-authored-by: Alex Roan --- aderyn_core/src/ast/impls/node/statements.rs | 7 ++ aderyn_core/src/detect/detector.rs | 3 + aderyn_core/src/detect/high/mod.rs | 2 + aderyn_core/src/detect/high/unchecked_send.rs | 113 +++++++++++++++++ .../src/detect/high/weak_randomness.rs | 3 + .../adhoc-sol-files-highs-only-report.json | 1 + reports/report.json | 66 +++++++++- reports/report.md | 117 +++++++++++++----- reports/report.sarif | 97 +++++++++++++++ .../contract-playground/src/UncheckedSend.sol | 31 +++++ 10 files changed, 409 insertions(+), 31 deletions(-) create mode 100644 aderyn_core/src/detect/high/unchecked_send.rs create mode 100644 tests/contract-playground/src/UncheckedSend.sol diff --git a/aderyn_core/src/ast/impls/node/statements.rs b/aderyn_core/src/ast/impls/node/statements.rs index 5ceb70fea..9f1bb74c1 100644 --- a/aderyn_core/src/ast/impls/node/statements.rs +++ b/aderyn_core/src/ast/impls/node/statements.rs @@ -40,9 +40,16 @@ impl Node for ExpressionStatement { fn accept(&self, visitor: &mut impl ASTConstVisitor) -> Result<()> { if visitor.visit_expression_statement(self)? { self.expression.accept(visitor)?; + self.accept_metadata(visitor)?; } visitor.end_visit_expression_statement(self) } + fn accept_metadata(&self, visitor: &mut impl ASTConstVisitor) -> Result<()> { + if let Some(child_id) = self.expression.get_node_id() { + visitor.visit_immediate_children(self.id, vec![child_id])?; + } + Ok(()) + } } impl Node for VariableDeclarationStatement { diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 49ad141fa..eef406e72 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -58,6 +58,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), Box::::default(), Box::::default(), @@ -121,6 +122,7 @@ pub(crate) enum IssueDetectorNamePool { IncorrectCaretOperator, YulReturn, StateVariableShadowing, + UncheckedSend, MisusedBoolean, SendEtherNoChecks, DelegateCallUncheckedAddress, @@ -252,6 +254,7 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::UncheckedSend => Some(Box::::default()), IssueDetectorNamePool::MisusedBoolean => Some(Box::::default()), IssueDetectorNamePool::SendEtherNoChecks => { Some(Box::::default()) diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index f5b609722..1e9e9912f 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -22,6 +22,7 @@ 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 unchecked_send; pub(crate) mod uninitialized_state_variable; pub(crate) mod unprotected_init_function; pub(crate) mod unsafe_casting; @@ -52,6 +53,7 @@ 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 unchecked_send::UncheckedSendDetector; pub use uninitialized_state_variable::UninitializedStateVariableDetector; pub use unprotected_init_function::UnprotectedInitializerDetector; pub use unsafe_casting::UnsafeCastingDetector; diff --git a/aderyn_core/src/detect/high/unchecked_send.rs b/aderyn_core/src/detect/high/unchecked_send.rs new file mode 100644 index 000000000..977272b30 --- /dev/null +++ b/aderyn_core/src/detect/high/unchecked_send.rs @@ -0,0 +1,113 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::{ASTNode, 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 UncheckedSendDetector { + // 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 UncheckedSendDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for member_access in context.member_accesses() { + if member_access.member_name == "send" + && member_access + .expression + .type_descriptions() + .is_some_and(|type_desc| { + type_desc + .type_string + .as_ref() + .is_some_and(|type_string| type_string.starts_with("address")) + }) + { + if let Some(ASTNode::FunctionCall(func_call)) = member_access.parent(context) { + if func_call + .parent(context) + .is_some_and(|node| node.node_type() == NodeType::Block) + { + capture!(self, context, func_call); + } + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Unchecked `bool success` value for send call.") + } + + fn description(&self) -> String { + String::from("The transaction `address(payable?).send(address)` may fail because of reasons like out-of-gas, \ + invalid receipient address or revert from the recipient. Therefore, the boolean returned by this function call must be checked \ + to be `true` in order to verify that the transaction was successful") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::UncheckedSend.to_string() + } +} + +#[cfg(test)] +mod unchecked_send_tests { + use serial_test::serial; + + use crate::detect::{detector::IssueDetector, high::unchecked_send::UncheckedSendDetector}; + + #[test] + #[serial] + fn test_unchecked_send() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/UncheckedSend.sol", + ); + + let mut detector = UncheckedSendDetector::default(); + let found = detector.detect(&context).unwrap(); + + println!("Instances {:#?}", 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(), 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("Unchecked `bool success` value for send call.") + ); + // assert the description is correct + assert_eq!( + detector.description(), + String::from("The transaction `address(payable?).send(address)` may fail because of reasons like out-of-gas, \ + invalid receipient address or revert from the recipient. Therefore, the boolean returned by this function call must be checked \ + to be `true` in order to verify that the transaction was successful") + ); + } +} diff --git a/aderyn_core/src/detect/high/weak_randomness.rs b/aderyn_core/src/detect/high/weak_randomness.rs index 079fd2cc2..16280ee4a 100644 --- a/aderyn_core/src/detect/high/weak_randomness.rs +++ b/aderyn_core/src/detect/high/weak_randomness.rs @@ -160,9 +160,12 @@ fn check_operand(operand: &Expression) -> bool { #[cfg(test)] mod weak_randomness_detector_tests { + use serial_test::serial; + use crate::detect::{detector::IssueDetector, high::weak_randomness::WeakRandomnessDetector}; #[test] + #[serial] fn test_weak_randomness_detector() { let context = crate::detect::test_utils::load_solidity_source_unit( "../tests/contract-playground/src/WeakRandomness.sol", diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 5b6f13587..3f591e527 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -181,6 +181,7 @@ "incorrect-caret-operator", "yul-return", "state-variable-shadowing", + "unchecked-send", "misused-boolean", "send-ether-no-checks", "delegate-call-unchecked-address", diff --git a/reports/report.json b/reports/report.json index 1e7161940..caa2ecc85 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 67, - "total_sloc": 1904 + "total_source_units": 68, + "total_sloc": 1922 }, "files_details": { "files_details": [ @@ -165,6 +165,10 @@ "file_path": "src/UncheckedReturn.sol", "n_sloc": 33 }, + { + "file_path": "src/UncheckedSend.sol", + "n_sloc": 18 + }, { "file_path": "src/UninitializedStateVariable.sol", "n_sloc": 29 @@ -276,7 +280,7 @@ ] }, "issue_count": { - "high": 29, + "high": 30, "low": 23 }, "high_issues": { @@ -1380,6 +1384,19 @@ } ] }, + { + "title": "Unchecked `bool success` value for send call.", + "description": "The transaction `address(payable?).send(address)` may fail because of reasons like out-of-gas, invalid receipient address or revert from the recipient. Therefore, the boolean returned by this function call must be checked to be `true` in order to verify that the transaction was successful", + "detector_name": "unchecked-send", + "instances": [ + { + "contract_path": "src/UncheckedSend.sol", + "line_no": 24, + "src": "815:22", + "src_char": "815:22" + } + ] + }, { "title": "Misused boolean with logical operators", "description": "The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively.", @@ -1476,6 +1493,30 @@ "src": "1795:5", "src_char": "1795:5" }, + { + "contract_path": "src/UncheckedSend.sol", + "line_no": 6, + "src": "85:246", + "src_char": "85:246" + }, + { + "contract_path": "src/UncheckedSend.sol", + "line_no": 12, + "src": "337:190", + "src_char": "337:190" + }, + { + "contract_path": "src/UncheckedSend.sol", + "line_no": 17, + "src": "533:184", + "src_char": "533:184" + }, + { + "contract_path": "src/UncheckedSend.sol", + "line_no": 22, + "src": "723:186", + "src_char": "723:186" + }, { "contract_path": "src/UninitializedStateVariable.sol", "line_no": 17, @@ -1938,6 +1979,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/UncheckedSend.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/UsingSelfdestruct.sol", "line_no": 2, @@ -2932,6 +2979,12 @@ "src": "1795:5", "src_char": "1795:5" }, + { + "contract_path": "src/UncheckedSend.sol", + "line_no": 27, + "src": "915:65", + "src_char": "915:65" + }, { "contract_path": "src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol", "line_no": 11, @@ -3197,6 +3250,12 @@ "line_no": 17, "src": "388:11", "src_char": "388:11" + }, + { + "contract_path": "src/UncheckedSend.sol", + "line_no": 27, + "src": "915:65", + "src_char": "915:65" } ] }, @@ -3466,6 +3525,7 @@ "incorrect-caret-operator", "yul-return", "state-variable-shadowing", + "unchecked-send", "misused-boolean", "send-ether-no-checks", "delegate-call-unchecked-address", diff --git a/reports/report.md b/reports/report.md index d2cd2eeca..dba746648 100644 --- a/reports/report.md +++ b/reports/report.md @@ -27,16 +27,17 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [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) + - [H-20: Unchecked `bool success` value for send call.](#h-20-unchecked-bool-success-value-for-send-call) + - [H-21: Misused boolean with logical operators](#h-21-misused-boolean-with-logical-operators) + - [H-22: Sending native Eth is not protected from these functions.](#h-22-sending-native-eth-is-not-protected-from-these-functions) + - [H-23: Delegatecall made by the function without checks on any adress.](#h-23-delegatecall-made-by-the-function-without-checks-on-any-adress) + - [H-24: Tautological comparison.](#h-24-tautological-comparison) + - [H-25: RTLO character detected in file. \u{202e}](#h-25-rtlo-character-detected-in-file-u202e) + - [H-26: Return value of the function call is not checked.](#h-26-return-value-of-the-function-call-is-not-checked) + - [H-27: Dangerous unary operator found in assignment.](#h-27-dangerous-unary-operator-found-in-assignment) + - [H-28: Weak Randomness](#h-28-weak-randomness) + - [H-29: Usage of variable before declaration.](#h-29-usage-of-variable-before-declaration) + - [H-30: Deletion from a nested mappping.](#h-30-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) @@ -69,8 +70,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 67 | -| Total nSLOC | 1904 | +| .sol Files | 68 | +| Total nSLOC | 1922 | ## Files Details @@ -117,6 +118,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/TautologicalCompare.sol | 17 | | src/TestERC20.sol | 62 | | src/UncheckedReturn.sol | 33 | +| src/UncheckedSend.sol | 18 | | src/UninitializedStateVariable.sol | 29 | | src/UnprotectedInitialize.sol | 25 | | src/UnsafeERC721Mint.sol | 18 | @@ -144,14 +146,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** | **1904** | +| **Total** | **1922** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 29 | +| High | 30 | | Low | 23 | @@ -1334,7 +1336,24 @@ Description of the high issue. -## H-20: Misused boolean with logical operators +## H-20: Unchecked `bool success` value for send call. + +The transaction `address(payable?).send(address)` may fail because of reasons like out-of-gas, invalid receipient address or revert from the recipient. Therefore, the boolean returned by this function call must be checked to be `true` in order to verify that the transaction was successful + +
1 Found Instances + + +- Found in src/UncheckedSend.sol [Line: 24](../tests/contract-playground/src/UncheckedSend.sol#L24) + + ```solidity + recipient.send(amount); // parent of Send FunctionCall is Block (return value is unused) + ``` + +
+ + + +## H-21: Misused boolean with logical operators The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively. @@ -1405,11 +1424,11 @@ The patterns `if (… || true)` and `if (.. && false)` will always evaluate to t -## H-21: Sending native Eth is not protected from these functions. +## H-22: Sending native Eth is not protected from these functions. Introduce checks for `msg.sender` in the function -
5 Found Instances +
9 Found Instances - Found in src/CallGraphTests.sol [Line: 38](../tests/contract-playground/src/CallGraphTests.sol#L38) @@ -1436,6 +1455,30 @@ Introduce checks for `msg.sender` in the function function func1(address x) external mod1(x) { ``` +- Found in src/UncheckedSend.sol [Line: 6](../tests/contract-playground/src/UncheckedSend.sol#L6) + + ```solidity + function send1(address payable recipient, uint256 amount) external { + ``` + +- Found in src/UncheckedSend.sol [Line: 12](../tests/contract-playground/src/UncheckedSend.sol#L12) + + ```solidity + function send2(address payable recipient, uint256 amount) external { + ``` + +- Found in src/UncheckedSend.sol [Line: 17](../tests/contract-playground/src/UncheckedSend.sol#L17) + + ```solidity + function send3(address payable recipient, uint256 amount) external returns(bool) { + ``` + +- Found in src/UncheckedSend.sol [Line: 22](../tests/contract-playground/src/UncheckedSend.sol#L22) + + ```solidity + function send4(address payable recipient, uint256 amount) external { + ``` + - Found in src/UninitializedStateVariable.sol [Line: 17](../tests/contract-playground/src/UninitializedStateVariable.sol#L17) ```solidity @@ -1446,7 +1489,7 @@ Introduce checks for `msg.sender` in the function -## H-22: Delegatecall made by the function without checks on any adress. +## H-23: Delegatecall made by the function without checks on any adress. Introduce checks on the address @@ -1475,7 +1518,7 @@ Introduce checks on the address -## H-23: Tautological comparison. +## H-24: 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. @@ -1510,7 +1553,7 @@ The left hand side and the right hand side of the binary operation has the same -## H-24: RTLO character detected in file. \u{202e} +## H-25: RTLO character detected in file. \u{202e} Right to left override character may be misledaing and cause potential attacks by visually misordering method arguments! @@ -1527,7 +1570,7 @@ Right to left override character may be misledaing and cause potential attacks b -## H-25: Return value of the function call is not checked. +## H-26: Return value of the function call is not checked. Function returns a value but it is ignored. @@ -1550,7 +1593,7 @@ Function returns a value but it is ignored. -## H-26: Dangerous unary operator found in assignment. +## H-27: Dangerous unary operator found in assignment. Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in between. @@ -1573,7 +1616,7 @@ Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in -## H-27: Weak Randomness +## H-28: 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. @@ -1638,7 +1681,7 @@ The use of keccak256 hash functions on predictable values like block.timestamp, -## H-28: Usage of variable before declaration. +## H-29: Usage of variable before declaration. This is a bad practice that may lead to unintended consequences. Please declare the variable before using it. @@ -1655,7 +1698,7 @@ This is a bad practice that may lead to unintended consequences. Please declare -## H-29: Deletion from a nested mappping. +## H-30: 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. @@ -1904,7 +1947,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;` -
15 Found Instances +
16 Found Instances - Found in src/ContractWithTodo.sol [Line: 2](../tests/contract-playground/src/ContractWithTodo.sol#L2) @@ -1955,6 +1998,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity ^0.4.0; ``` +- Found in src/UncheckedSend.sol [Line: 2](../tests/contract-playground/src/UncheckedSend.sol#L2) + + ```solidity + pragma solidity ^0.7.0; + ``` + - Found in src/UsingSelfdestruct.sol [Line: 2](../tests/contract-playground/src/UsingSelfdestruct.sol#L2) ```solidity @@ -2878,7 +2927,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai Consider removing empty blocks. -
25 Found Instances +
26 Found Instances - Found in src/AdminContract.sol [Line: 14](../tests/contract-playground/src/AdminContract.sol#L14) @@ -2977,6 +3026,12 @@ Consider removing empty blocks. function func1(address x) external mod1(x) { ``` +- Found in src/UncheckedSend.sol [Line: 27](../tests/contract-playground/src/UncheckedSend.sol#L27) + + ```solidity + function doSomething(bool success) internal pure { + ``` + - Found in src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol [Line: 11](../tests/contract-playground/src/auditor_mode/PublicFunctionsWithoutSenderCheck.sol#L11) ```solidity @@ -3182,7 +3237,7 @@ Use `e` notation, for example: `1e18`, instead of its full numeric value. Instead of separating the logic into a separate function, consider inlining the logic into the calling function. This can reduce the number of function calls and improve readability. -
11 Found Instances +
12 Found Instances - Found in src/CallGraphTests.sol [Line: 6](../tests/contract-playground/src/CallGraphTests.sol#L6) @@ -3251,6 +3306,12 @@ Instead of separating the logic into a separate function, consider inlining the function editStorage(uint[1] storage arr) internal { ``` +- Found in src/UncheckedSend.sol [Line: 27](../tests/contract-playground/src/UncheckedSend.sol#L27) + + ```solidity + function doSomething(bool success) internal pure { + ``` +
diff --git a/reports/report.sarif b/reports/report.sarif index 914985a7e..ddc8f5e5d 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -1946,6 +1946,26 @@ }, "ruleId": "state-variable-shadowing" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UncheckedSend.sol" + }, + "region": { + "byteLength": 22, + "byteOffset": 815 + } + } + } + ], + "message": { + "text": "The transaction `address(payable?).send(address)` may fail because of reasons like out-of-gas, invalid receipient address or revert from the recipient. Therefore, the boolean returned by this function call must be checked to be `true` in order to verify that the transaction was successful" + }, + "ruleId": "unchecked-send" + }, { "level": "warning", "locations": [ @@ -2112,6 +2132,50 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UncheckedSend.sol" + }, + "region": { + "byteLength": 246, + "byteOffset": 85 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UncheckedSend.sol" + }, + "region": { + "byteLength": 190, + "byteOffset": 337 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UncheckedSend.sol" + }, + "region": { + "byteLength": 184, + "byteOffset": 533 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UncheckedSend.sol" + }, + "region": { + "byteLength": 186, + "byteOffset": 723 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2898,6 +2962,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UncheckedSend.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -4682,6 +4757,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UncheckedSend.sol" + }, + "region": { + "byteLength": 65, + "byteOffset": 915 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -5161,6 +5247,17 @@ "byteOffset": 388 } } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UncheckedSend.sol" + }, + "region": { + "byteLength": 65, + "byteOffset": 915 + } + } } ], "message": { diff --git a/tests/contract-playground/src/UncheckedSend.sol b/tests/contract-playground/src/UncheckedSend.sol new file mode 100644 index 000000000..6822b75fa --- /dev/null +++ b/tests/contract-playground/src/UncheckedSend.sol @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.7.0; + +contract SendExample { + + function send1(address payable recipient, uint256 amount) external { + // GOOD + bool success = recipient.send(amount); // parent of Send FunctionCall is VariableDeclarationStatement + require(success, "Send successful!"); + } + + function send2(address payable recipient, uint256 amount) external { + // GOOD + doSomething(recipient.send(amount)); // parent of Send FunctionCall is another FunctionCall + } + + function send3(address payable recipient, uint256 amount) external returns(bool) { + // GOOD + return recipient.send(amount); // parent of Send FunctionCall is return + } + + function send4(address payable recipient, uint256 amount) external { + // BAD + recipient.send(amount); // parent of Send FunctionCall is Block (return value is unused) + } + + function doSomething(bool success) internal pure { + + } + +} From 25a368c11b5df5e093b8955f1cb0ee37d85afa2a Mon Sep 17 00:00:00 2001 From: Tilak Madichetti Date: Mon, 29 Jul 2024 19:45:28 +0530 Subject: [PATCH 9/9] Detector: Public variable read in an external context (#619) Co-authored-by: Alex Roan --- aderyn_core/src/detect/detector.rs | 5 + aderyn_core/src/detect/low/mod.rs | 2 + ...ublic_variable_read_in_external_context.rs | 170 ++++++++++++++++++ reports/report.json | 48 ++++- reports/report.md | 53 +++++- reports/report.sarif | 64 +++++++ .../PublicVariableReadInExternalContext.sol | 50 ++++++ 7 files changed, 384 insertions(+), 8 deletions(-) create mode 100644 aderyn_core/src/detect/low/public_variable_read_in_external_context.rs create mode 100644 tests/contract-playground/src/PublicVariableReadInExternalContext.sol diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index eef406e72..80ed73457 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -66,6 +66,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), Box::::default(), Box::::default(), @@ -131,6 +132,7 @@ pub(crate) enum IssueDetectorNamePool { RTLO, UncheckedReturn, DangerousUnaryOperator, + PublicVariableReadInExternalContext, WeakRandomness, PreDeclaredLocalVariableUsage, DeleteNestedMapping, @@ -270,6 +272,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::PublicVariableReadInExternalContext => { + Some(Box::::default()) + } IssueDetectorNamePool::WeakRandomness => Some(Box::::default()), IssueDetectorNamePool::PreDeclaredLocalVariableUsage => { Some(Box::::default()) diff --git a/aderyn_core/src/detect/low/mod.rs b/aderyn_core/src/detect/low/mod.rs index 6b8270129..a20f696f5 100644 --- a/aderyn_core/src/detect/low/mod.rs +++ b/aderyn_core/src/detect/low/mod.rs @@ -8,6 +8,7 @@ pub(crate) mod empty_blocks; pub(crate) mod inconsistent_type_names; pub(crate) mod large_literal_value; pub(crate) mod non_reentrant_before_others; +pub(crate) mod public_variable_read_in_external_context; pub(crate) mod push_0_opcode; pub(crate) mod require_with_string; pub(crate) mod reverts_and_requries_in_loops; @@ -32,6 +33,7 @@ pub use empty_blocks::EmptyBlockDetector; pub use inconsistent_type_names::InconsistentTypeNamesDetector; pub use large_literal_value::LargeLiteralValueDetector; pub use non_reentrant_before_others::NonReentrantBeforeOthersDetector; +pub use public_variable_read_in_external_context::PublicVariableReadInExternalContextDetector; pub use push_0_opcode::PushZeroOpcodeDetector; pub use require_with_string::RequireWithStringDetector; pub use reverts_and_requries_in_loops::RevertsAndRequiresInLoopsDetector; diff --git a/aderyn_core/src/detect/low/public_variable_read_in_external_context.rs b/aderyn_core/src/detect/low/public_variable_read_in_external_context.rs new file mode 100644 index 000000000..a0c95573f --- /dev/null +++ b/aderyn_core/src/detect/low/public_variable_read_in_external_context.rs @@ -0,0 +1,170 @@ +use std::collections::{BTreeMap, HashSet}; +use std::error::Error; + +use crate::ast::{ + ASTNode, ContractDefinition, Expression, Identifier, MemberAccess, NodeID, Visibility, +}; + +use crate::capture; +use crate::context::browser::{ExtractFunctionCalls, ExtractVariableDeclarations}; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::{eyre, Result}; + +#[derive(Default)] +pub struct PublicVariableReadInExternalContextDetector { + // 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 PublicVariableReadInExternalContextDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for contract in context.contract_definitions() { + // Public state variables including the base contracts + if let Ok(public_state_variable_names) = + find_all_public_state_variables_names_for_contract(context, contract) + { + // Find all the `X`s that appear with the pattern `this.X()` + let this_member_accesses = + find_all_public_member_names_called_using_this_keyword_in_contract( + context, contract, + ); + + for member_access in this_member_accesses { + if public_state_variable_names.contains(&member_access.member_name) { + capture!(self, context, member_access); + } + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::Low + } + + fn title(&self) -> String { + String::from("Public variables of a contract read in an external context (using `this`).") + } + + fn description(&self) -> String { + String::from( + "The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage.", + ) + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::PublicVariableReadInExternalContext.to_string() + } +} + +fn find_all_public_member_names_called_using_this_keyword_in_contract<'a>( + context: &'a WorkspaceContext, + contract: &ContractDefinition, +) -> Vec<&'a MemberAccess> { + let mut member_names = vec![]; + + let function_calls = ExtractFunctionCalls::from(contract).extracted; + for function_call in function_calls { + if let Expression::MemberAccess(MemberAccess { id, expression, .. }) = + function_call.expression.as_ref() + { + if let Expression::Identifier(Identifier { name, .. }) = expression.as_ref() { + if name == "this" { + if let Some(ASTNode::MemberAccess(member_acess)) = context.nodes.get(id) { + member_names.push(member_acess) + } + } + } + } + } + + member_names +} + +// Scans the linearized base contracts and returns a list of all the NodeIDs of public variable declarations +fn find_all_public_state_variables_names_for_contract( + context: &WorkspaceContext, + contract: &ContractDefinition, +) -> Result, Box> { + let inheritance_ancestors = contract + .linearized_base_contracts + .as_ref() + .ok_or(eyre!("base contracts not found!"))?; + + Ok(inheritance_ancestors + .iter() + .flat_map(|ancestor_id| { + if let Some(ancestor) = context.nodes.get(ancestor_id) { + let public_variable_declaraions = + ExtractVariableDeclarations::from(ancestor).extracted; + return Some( + public_variable_declaraions + .into_iter() + .filter(|declaration| { + declaration.state_variable + && declaration.visibility == Visibility::Public + }) + .collect::>(), + ); + } + None + }) + .flatten() + .map(|v| v.name.clone()) + .collect()) +} + +#[cfg(test)] +mod public_variable_read_in_external_context_detector_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, + low::public_variable_read_in_external_context::PublicVariableReadInExternalContextDetector, + }; + + #[test] + #[serial] + fn test_public_variable_read_in_external_context() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/PublicVariableReadInExternalContext.sol", + ); + + let mut detector = PublicVariableReadInExternalContextDetector::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 low + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::Low + ); + // assert the title is correct + assert_eq!( + detector.title(), + String::from( + "Public variables of a contract read in an external context (using `this`)." + ) + ); + // assert the description is correct + assert_eq!( + detector.description(), + String::from( + "The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage.", + ) + ); + } +} diff --git a/reports/report.json b/reports/report.json index caa2ecc85..c941b7001 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 68, - "total_sloc": 1922 + "total_source_units": 69, + "total_sloc": 1954 }, "files_details": { "files_details": [ @@ -121,6 +121,10 @@ "file_path": "src/PreDeclaredVarUsage.sol", "n_sloc": 9 }, + { + "file_path": "src/PublicVariableReadInExternalContext.sol", + "n_sloc": 32 + }, { "file_path": "src/RTLO.sol", "n_sloc": 7 @@ -281,7 +285,7 @@ }, "issue_count": { "high": 30, - "low": 23 + "low": 24 }, "high_issues": { "issues": [ @@ -1271,6 +1275,12 @@ "src": "355:7", "src_char": "355:7" }, + { + "contract_path": "src/PublicVariableReadInExternalContext.sol", + "line_no": 6, + "src": "130:26", + "src_char": "130:26" + }, { "contract_path": "src/StateShadowing.sol", "line_no": 5, @@ -3479,6 +3489,37 @@ "src_char": "541:5" } ] + }, + { + "title": "Public variables of a contract read in an external context (using `this`).", + "description": "The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage.", + "detector_name": "public-variable-read-in-external-context", + "instances": [ + { + "contract_path": "src/PublicVariableReadInExternalContext.sol", + "line_no": 12, + "src": "355:14", + "src_char": "355:14" + }, + { + "contract_path": "src/PublicVariableReadInExternalContext.sol", + "line_no": 16, + "src": "457:16", + "src_char": "457:16" + }, + { + "contract_path": "src/PublicVariableReadInExternalContext.sol", + "line_no": 20, + "src": "553:12", + "src_char": "553:12" + }, + { + "contract_path": "src/PublicVariableReadInExternalContext.sol", + "line_no": 42, + "src": "1175:14", + "src_char": "1175:14" + } + ] } ] }, @@ -3533,6 +3574,7 @@ "rtlo", "unchecked-return", "dangerous-unary-operator", + "public-variable-read-in-external-context", "weak-randomness", "pre-declared-local-variable-usage", "delete-nested-mapping" diff --git a/reports/report.md b/reports/report.md index dba746648..b813542af 100644 --- a/reports/report.md +++ b/reports/report.md @@ -62,6 +62,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [L-21: Unused Custom Error](#l-21-unused-custom-error) - [L-22: Loop contains `require`/`revert` statements](#l-22-loop-contains-requirerevert-statements) - [L-23: Incorrect Order of Division and Multiplication](#l-23-incorrect-order-of-division-and-multiplication) + - [L-24: Public variables of a contract read in an external context (using `this`).](#l-24-public-variables-of-a-contract-read-in-an-external-context-using-this) # Summary @@ -70,8 +71,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 68 | -| Total nSLOC | 1922 | +| .sol Files | 69 | +| Total nSLOC | 1954 | ## Files Details @@ -107,6 +108,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/MultipleConstructorSchemes.sol | 10 | | src/OnceModifierExample.sol | 8 | | src/PreDeclaredVarUsage.sol | 9 | +| src/PublicVariableReadInExternalContext.sol | 32 | | src/RTLO.sol | 7 | | src/RevertsAndRequriesInLoops.sol | 27 | | src/SendEtherNoChecks.sol | 58 | @@ -146,7 +148,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/reused_contract_name/ContractB.sol | 7 | | src/uniswap/UniswapV2Swapper.sol | 50 | | src/uniswap/UniswapV3Swapper.sol | 150 | -| **Total** | **1922** | +| **Total** | **1954** | ## Issue Summary @@ -154,7 +156,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Category | No. of Issues | | --- | --- | | High | 30 | -| Low | 23 | +| Low | 24 | # High Issues @@ -1176,7 +1178,7 @@ If the length of a dynamic array (storage variable) directly assigned to, it may Solidity does initialize variables by default when you declare them, however it's good practice to explicitly declare an initial value. For example, if you transfer money to an address we must make sure that the address has been initialized. -
13 Found Instances +
14 Found Instances - Found in src/AssemblyExample.sol [Line: 5](../tests/contract-playground/src/AssemblyExample.sol#L5) @@ -1209,6 +1211,12 @@ Solidity does initialize variables by default when you declare them, however it' uint256 private s_first; ``` +- Found in src/PublicVariableReadInExternalContext.sol [Line: 6](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L6) + + ```solidity + uint256 public testUint256; + ``` + - Found in src/StateShadowing.sol [Line: 5](../tests/contract-playground/src/StateShadowing.sol#L5) ```solidity @@ -3557,3 +3565,38 @@ Division operations followed directly by multiplication operations can lead to p +## L-24: Public variables of a contract read in an external context (using `this`). + +The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage. + +
4 Found Instances + + +- Found in src/PublicVariableReadInExternalContext.sol [Line: 12](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L12) + + ```solidity + return this.testArray(0); + ``` + +- Found in src/PublicVariableReadInExternalContext.sol [Line: 16](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L16) + + ```solidity + return this.testUint256(); + ``` + +- Found in src/PublicVariableReadInExternalContext.sol [Line: 20](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L20) + + ```solidity + return this.testMap(0); + ``` + +- Found in src/PublicVariableReadInExternalContext.sol [Line: 42](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L42) + + ```solidity + return this.testArray(0); + ``` + +
+ + + diff --git a/reports/report.sarif b/reports/report.sarif index ddc8f5e5d..3d415b864 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -1748,6 +1748,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/PublicVariableReadInExternalContext.sol" + }, + "region": { + "byteLength": 26, + "byteOffset": 130 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -5650,6 +5661,59 @@ "text": "Division operations followed directly by multiplication operations can lead to precision loss due to the way integer arithmetic is handled in Solidity." }, "ruleId": "division-before-multiplication" + }, + { + "level": "note", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/PublicVariableReadInExternalContext.sol" + }, + "region": { + "byteLength": 14, + "byteOffset": 355 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/PublicVariableReadInExternalContext.sol" + }, + "region": { + "byteLength": 16, + "byteOffset": 457 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/PublicVariableReadInExternalContext.sol" + }, + "region": { + "byteLength": 12, + "byteOffset": 553 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/PublicVariableReadInExternalContext.sol" + }, + "region": { + "byteLength": 14, + "byteOffset": 1175 + } + } + } + ], + "message": { + "text": "The contract reads it's own variable using `this` which adds an unnecessary STATICCALL. Remove `this` and access the variable like storage." + }, + "ruleId": "public-variable-read-in-external-context" } ], "tool": { diff --git a/tests/contract-playground/src/PublicVariableReadInExternalContext.sol b/tests/contract-playground/src/PublicVariableReadInExternalContext.sol new file mode 100644 index 000000000..7635d4db9 --- /dev/null +++ b/tests/contract-playground/src/PublicVariableReadInExternalContext.sol @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.5.0; + +contract PublicVariableReadUsingThis { + string[] public testArray; + uint256 public testUint256; + mapping(uint256 => bool) public testMap; + + // BAD ways of reading (using this.) + + function readStringArray() external view returns (string memory) { + return this.testArray(0); + } + + function readUint256() external view returns (uint256) { + return this.testUint256(); + } + + function readMap() external view returns (bool) { + return this.testMap(0); + } + + // GOOD ways of reading (using ) + + function readStringArrayGood() external view returns (string memory) { + return testArray[0]; + } + + function readUint256Good() external view returns (uint256) { + return testUint256; + } + + function readMapGood() external view returns (bool) { + return testMap[0]; + } +} + +contract DerivedFromPublicVariableReadUsingThis is PublicVariableReadUsingThis { + // BAD ways of reading (using this.) + + function readStringArray() external view returns (string memory) { + return this.testArray(0); + } + + // GOOD ways of reading (using ) + + function readStringArrayGood() external view returns (string memory) { + return testArray[0]; + } +}