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/ast/ast.rs b/aderyn_core/src/ast/ast.rs index 843053a43..1daade1a9 100644 --- a/aderyn_core/src/ast/ast.rs +++ b/aderyn_core/src/ast/ast.rs @@ -80,3 +80,69 @@ impl From<&YulLiteral> for ASTNode { ASTNode::YulLiteral(value.clone()) } } + +impl From<&Expression> for ASTNode { + fn from(value: &Expression) -> Self { + match value { + Expression::Literal(literal) => ASTNode::Literal(literal.clone()), + Expression::Identifier(identifier) => ASTNode::Identifier(identifier.clone()), + Expression::UnaryOperation(unary_operation) => { + ASTNode::UnaryOperation(unary_operation.clone()) + } + Expression::BinaryOperation(binary_operation) => { + ASTNode::BinaryOperation(binary_operation.clone()) + } + Expression::Conditional(conditional) => ASTNode::Conditional(conditional.clone()), + Expression::Assignment(assignment) => ASTNode::Assignment(assignment.clone()), + Expression::FunctionCall(function_call) => ASTNode::FunctionCall(function_call.clone()), + Expression::FunctionCallOptions(function_call_ops) => { + ASTNode::FunctionCallOptions(function_call_ops.clone()) + } + Expression::IndexAccess(index_access) => ASTNode::IndexAccess(index_access.clone()), + Expression::IndexRangeAccess(index_range_access) => { + ASTNode::IndexRangeAccess(index_range_access.clone()) + } + Expression::MemberAccess(member_access) => ASTNode::MemberAccess(member_access.clone()), + Expression::ElementaryTypeNameExpression(elementary_type_name_expression) => { + ASTNode::ElementaryTypeNameExpression(elementary_type_name_expression.clone()) + } + Expression::TupleExpression(tuple_expression) => { + ASTNode::TupleExpression(tuple_expression.clone()) + } + Expression::NewExpression(new_expression) => { + ASTNode::NewExpression(new_expression.clone()) + } + } + } +} + +impl From for ASTNode { + fn from(value: Expression) -> Self { + match value { + Expression::Literal(literal) => ASTNode::Literal(literal), + Expression::Identifier(identifier) => ASTNode::Identifier(identifier), + Expression::UnaryOperation(unary_operation) => ASTNode::UnaryOperation(unary_operation), + Expression::BinaryOperation(binary_operation) => { + ASTNode::BinaryOperation(binary_operation) + } + Expression::Conditional(conditional) => ASTNode::Conditional(conditional), + Expression::Assignment(assignment) => ASTNode::Assignment(assignment), + Expression::FunctionCall(function_call) => ASTNode::FunctionCall(function_call), + Expression::FunctionCallOptions(function_call_ops) => { + ASTNode::FunctionCallOptions(function_call_ops) + } + Expression::IndexAccess(index_access) => ASTNode::IndexAccess(index_access), + Expression::IndexRangeAccess(index_range_access) => { + ASTNode::IndexRangeAccess(index_range_access) + } + Expression::MemberAccess(member_access) => ASTNode::MemberAccess(member_access), + Expression::ElementaryTypeNameExpression(elementary_type_name_expression) => { + ASTNode::ElementaryTypeNameExpression(elementary_type_name_expression) + } + Expression::TupleExpression(tuple_expression) => { + ASTNode::TupleExpression(tuple_expression) + } + Expression::NewExpression(new_expression) => ASTNode::NewExpression(new_expression), + } + } +} diff --git a/aderyn_core/src/audit/public_functions_no_sender.rs b/aderyn_core/src/audit/public_functions_no_sender.rs index ca48ce7e2..245e6f7f0 100644 --- a/aderyn_core/src/audit/public_functions_no_sender.rs +++ b/aderyn_core/src/audit/public_functions_no_sender.rs @@ -60,7 +60,7 @@ impl AuditorDetector for PublicFunctionsNoSenderChecksDetector { }); // Check if the function has a `msg.sender` BinaryOperation check let has_msg_sender_binary_operation = - has_msg_sender_binary_operation(function_definition); + has_msg_sender_binary_operation(&((*function_definition).into())); // TODO Check if the function has a hasRole identifier with msg.sender as an arg does_not_have_an_owner_modifier && !has_msg_sender_binary_operation }); diff --git a/aderyn_core/src/context/browser/extractor.rs b/aderyn_core/src/context/browser/extractor.rs index 5e35a5ca3..cece1c871 100644 --- a/aderyn_core/src/context/browser/extractor.rs +++ b/aderyn_core/src/context/browser/extractor.rs @@ -88,3 +88,40 @@ impl ASTConstVisitor for ExtractImmediateChildrenIDs { Ok(()) } } + +// Extract Reference Declaration IDs +#[derive(Default)] +pub struct ExtractReferencedDeclarations { + pub extracted: Vec, +} + +impl ExtractReferencedDeclarations { + pub fn from(node: &T) -> Self { + let mut extractor: ExtractReferencedDeclarations = Self::default(); + node.accept(&mut extractor).unwrap_or_default(); + extractor + } +} + +impl ASTConstVisitor for ExtractReferencedDeclarations { + fn visit_member_access(&mut self, node: &MemberAccess) -> Result { + if let Some(referenced_id) = node.referenced_declaration { + self.extracted.push(referenced_id); + } + Ok(true) + } + fn visit_identifier(&mut self, node: &Identifier) -> Result { + if let Some(referenced_id) = node.referenced_declaration { + self.extracted.push(referenced_id); + } + Ok(true) + } + fn visit_identifier_path(&mut self, node: &IdentifierPath) -> Result { + self.extracted.push(node.referenced_declaration as i64); + Ok(true) + } + fn visit_user_defined_type_name(&mut self, node: &UserDefinedTypeName) -> Result { + self.extracted.push(node.referenced_declaration); + Ok(true) + } +} diff --git a/aderyn_core/src/context/graph/mod.rs b/aderyn_core/src/context/graph/mod.rs new file mode 100644 index 000000000..3d688c447 --- /dev/null +++ b/aderyn_core/src/context/graph/mod.rs @@ -0,0 +1,32 @@ +pub mod traits; +mod workspace_callgraph; + +pub use workspace_callgraph::*; + +use derive_more::From; + +pub type Result = core::result::Result; + +#[derive(Debug, From)] +pub enum Error { + #[from] + Custom(String), + + // region: -- standard::* errors + WorkspaceCallGraphDFSError, + // endregion +} + +impl core::fmt::Display for Error { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{self:?}") + } +} + +impl From<&str> for Error { + fn from(value: &str) -> Self { + Error::Custom(value.to_string()) + } +} + +impl std::error::Error for Error {} diff --git a/aderyn_core/src/context/graph/traits.rs b/aderyn_core/src/context/graph/traits.rs new file mode 100644 index 000000000..661bbf571 --- /dev/null +++ b/aderyn_core/src/context/graph/traits.rs @@ -0,0 +1,4 @@ +/// Trait to support reversing of callgraph. (Because, direct impl is not allowed on Foreign Types) +pub trait Transpose { + fn reverse(&self) -> Self; +} diff --git a/aderyn_core/src/context/graph/workspace_callgraph.rs b/aderyn_core/src/context/graph/workspace_callgraph.rs new file mode 100644 index 000000000..745175474 --- /dev/null +++ b/aderyn_core/src/context/graph/workspace_callgraph.rs @@ -0,0 +1,134 @@ +use std::collections::{hash_map, HashMap, HashSet}; + +use crate::{ + ast::{Expression, IdentifierOrIdentifierPath, NodeID, NodeType}, + context::{ + browser::{ExtractFunctionCalls, ExtractModifierInvocations}, + workspace_context::WorkspaceContext, + }, +}; + +use super::traits::Transpose; + +#[derive(Debug)] +pub struct WorkspaceCallGraph { + pub graph: CallGraph, +} + +/** +* Every NodeID in CallGraph should corresponds to [`crate::ast::FunctionDefinition`] or [`crate::ast::ModifierDefinition`] +*/ +pub type CallGraph = HashMap>; + +impl WorkspaceCallGraph { + /// Formula to create [`WorkspaceCallGraph`] for global preprocessing . + pub fn from_context(context: &WorkspaceContext) -> super::Result { + let mut graph: CallGraph = HashMap::new(); + let mut visited: HashSet = HashSet::new(); + + let funcs = context + .function_definitions() + .into_iter() + .filter(|func| func.implemented) + .collect::>(); + + let modifier_definitions = context.modifier_definitions(); + + for func in funcs { + dfs_to_create_graph(func.id, &mut graph, &mut visited, context) + .map_err(|_| super::Error::WorkspaceCallGraphDFSError)?; + } + + for modifier in modifier_definitions { + dfs_to_create_graph(modifier.id, &mut graph, &mut visited, context) + .map_err(|_| super::Error::WorkspaceCallGraphDFSError)?; + } + + Ok(WorkspaceCallGraph { graph }) + } +} + +/// Make connections from each of the nodes of [`crate::ast::FunctionDefinition`] and [`crate::ast::ModifierDefinition`] +/// with their connected counterparts. +fn dfs_to_create_graph( + id: NodeID, + graph: &mut CallGraph, + visited: &mut HashSet, + context: &WorkspaceContext, +) -> super::Result<()> { + if visited.contains(&id) { + return Ok(()); + } + + visited.insert(id); + + // Only deal with `id`s that are in scope right now + if let Some(from_node) = context.nodes.get(&id) { + // referenced_declarations from previous calls in the recursion stack need to be vetted + if from_node.node_type() != NodeType::FunctionDefinition + && from_node.node_type() != NodeType::ModifierDefinition + { + return Ok(()); + } + + // connections to FunctionDefinition + let function_calls = ExtractFunctionCalls::from(from_node).extracted; + for function_call in function_calls { + if let Expression::Identifier(identifier) = function_call.expression.as_ref() { + if let Some(referenced_function_id) = identifier.referenced_declaration { + create_connection_if_not_exsits(id, referenced_function_id, graph); + dfs_to_create_graph(referenced_function_id, graph, visited, context)?; + } + } + } + + // connections to ModifierDefinition + let modifier_invocations = ExtractModifierInvocations::from(from_node).extracted; + for modifier_invocation in &modifier_invocations { + match &modifier_invocation.modifier_name { + IdentifierOrIdentifierPath::Identifier(identifier) => { + if let Some(reference_modifier_id) = identifier.referenced_declaration { + create_connection_if_not_exsits(id, reference_modifier_id, graph); + dfs_to_create_graph(reference_modifier_id, graph, visited, context)?; + } + } + IdentifierOrIdentifierPath::IdentifierPath(identifier_path) => { + let referenced_modifier_id = identifier_path.referenced_declaration; + create_connection_if_not_exsits(id, referenced_modifier_id as i64, graph); + dfs_to_create_graph(referenced_modifier_id as i64, graph, visited, context)?; + } + } + } + } + + // Change the default return to error later in "strict mode" maybe, because if we + // can't find the node that means, the file was not in scope and hence it is not + // available in the context although references to it exist. + Ok(()) +} + +fn create_connection_if_not_exsits(from_id: NodeID, to_id: NodeID, graph: &mut CallGraph) { + match graph.entry(from_id) { + hash_map::Entry::Occupied(mut o) => { + // Performance Tip: Maybe later use binary search (it requires keeping ascending order while inserting tho) + if !o.get().contains(&to_id) { + o.get_mut().push(to_id); + } + } + hash_map::Entry::Vacant(v) => { + v.insert(vec![to_id]); + } + } +} + +impl Transpose for CallGraph { + fn reverse(&self) -> Self { + let mut reversed_callgraph = CallGraph::default(); + for (from_id, tos) in self { + for to_id in tos { + create_connection_if_not_exsits(*to_id, *from_id, &mut reversed_callgraph); + } + } + reversed_callgraph + } +} diff --git a/aderyn_core/src/context/investigator/callgraph_tests.rs b/aderyn_core/src/context/investigator/callgraph_tests.rs new file mode 100644 index 000000000..faa0a1ccc --- /dev/null +++ b/aderyn_core/src/context/investigator/callgraph_tests.rs @@ -0,0 +1,275 @@ +#![allow(clippy::collapsible_match)] + +#[cfg(test)] +mod callgraph_tests { + use crate::{ + ast::{FunctionDefinition, ModifierDefinition}, + context::{ + investigator::{ + StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, + }, + workspace_context::{ASTNode, WorkspaceContext}, + }, + }; + + use StandardInvestigationStyle::*; + + fn get_function_by_name(context: &WorkspaceContext, name: &str) -> ASTNode { + ASTNode::from( + context + .function_definitions() + .into_iter() + .find(|&x| x.name == *name) + .unwrap(), + ) + } + + fn get_modifier_definition_by_name(context: &WorkspaceContext, name: &str) -> ASTNode { + ASTNode::from( + context + .modifier_definitions() + .into_iter() + .find(|&x| x.name == *name) + .unwrap(), + ) + } + + #[test] + fn test_callgraph_is_not_none() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + assert!(context.forward_callgraph.is_some()); + assert!(context.reverse_callgraph.is_some()); + } + + #[test] + fn test_tower1_modifier_has_no_downstream() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let visit_eighth_floor1 = get_function_by_name(&context, "visitEighthFloor1"); + + let investigator = + StandardInvestigator::new(&context, &[&visit_eighth_floor1], Downstream).unwrap(); + + let mut tracker = Tracker::new(&context); + investigator.investigate(&context, &mut tracker).unwrap(); + + assert!(tracker.downstream_func_definitions_names.is_empty()); + assert!(tracker.downstream_modifier_definitions_names.is_empty()); + } + + #[test] + fn test_tower1_modifier_has_upstream() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let visit_eighth_floor1 = get_function_by_name(&context, "visitEighthFloor1"); + + let investigator = + StandardInvestigator::new(&context, &[&visit_eighth_floor1], Upstream).unwrap(); + + let mut tracker = Tracker::new(&context); + investigator.investigate(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_upstream_modifiers_with_names(&["passThroughNinthFloor1"])); + assert!(tracker.has_found_upstream_functions_with_names(&["enterTenthFloor1"])); + } + + #[test] + fn test_tower2_modifier_has_both_upstream_and_downstream() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let pass_through_ninth_floor2 = + get_modifier_definition_by_name(&context, "passThroughNinthFloor2"); + + let investigator = + StandardInvestigator::new(&context, &[&pass_through_ninth_floor2], BothWays).unwrap(); + + let mut tracker = Tracker::new(&context); + investigator.investigate(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_downstream_functions_with_names(&["visitEighthFloor2"])); + assert!(tracker.has_found_upstream_functions_with_names(&["enterTenthFloor2"])); + } + + #[test] + fn test_tower3_modifier_has_both_upstream_and_downstream() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let pass_through_ninth_floor3 = + get_modifier_definition_by_name(&context, "passThroughNinthFloor3"); + + let investigator = + StandardInvestigator::new(&context, &[&pass_through_ninth_floor3], BothWays).unwrap(); + + let mut tracker = Tracker::new(&context); + investigator.investigate(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_upstream_functions_with_names(&["enterTenthFloor3"])); + assert!(tracker.has_found_downstream_functions_with_names(&["visitEighthFloor3"])); + assert!(tracker.has_not_found_any_upstream_functions_with_name("visitSeventhFloor3")); + assert!(tracker.has_found_upstream_side_effect_functions_with_name(&["visitSeventhFloor3"])); + } + + #[test] + fn test_tower3_functions_has_upstream() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let visit_eighth_floor3 = get_function_by_name(&context, "visitSeventhFloor3"); + + let investigator = + StandardInvestigator::new(&context, &[&visit_eighth_floor3], Upstream).unwrap(); + + let mut tracker = Tracker::new(&context); + investigator.investigate(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_upstream_functions_with_names(&["enterTenthFloor3"])); + } + + #[test] + fn test_tower4_functions_has_upstream_and_downstream() { + let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let recurse = get_function_by_name(&context, "recurse"); + + let investigator = StandardInvestigator::new(&context, &[&recurse], BothWays).unwrap(); + + let mut tracker = Tracker::new(&context); + investigator.investigate(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_upstream_functions_with_names(&["recurse"])); + assert!(tracker.has_found_downstream_functions_with_names(&["recurse"])); + } + + struct Tracker<'a> { + context: &'a WorkspaceContext, + entry_points: Vec<(String, usize, String)>, + downstream_func_definitions_names: Vec, + upstream_func_definitions_names: Vec, + downstream_modifier_definitions_names: Vec, + upstream_modifier_definitions_names: Vec, + upstream_side_effects_func_definitions_names: Vec, + upstream_side_effects_modifier_definitions_names: Vec, + } + + impl<'a> Tracker<'a> { + fn new(context: &WorkspaceContext) -> Tracker { + Tracker { + context, + entry_points: vec![], + downstream_func_definitions_names: vec![], + downstream_modifier_definitions_names: vec![], + upstream_func_definitions_names: vec![], + upstream_modifier_definitions_names: vec![], + upstream_side_effects_func_definitions_names: vec![], + upstream_side_effects_modifier_definitions_names: vec![], + } + } + + // downstream functions + fn has_found_downstream_functions_with_names(&self, name: &[&str]) -> bool { + name.iter().all(|&n| { + self.downstream_func_definitions_names + .contains(&n.to_string()) + }) + } + + // upstream functions + fn has_found_upstream_functions_with_names(&self, name: &[&str]) -> bool { + name.iter().all(|&n| { + self.upstream_func_definitions_names + .contains(&n.to_string()) + }) + } + + fn has_not_found_any_upstream_functions_with_name(&self, name: &str) -> bool { + !self + .upstream_func_definitions_names + .contains(&name.to_string()) + } + + // upstream modifiers + fn has_found_upstream_modifiers_with_names(&self, name: &[&str]) -> bool { + name.iter().all(|&n| { + self.upstream_modifier_definitions_names + .contains(&n.to_string()) + }) + } + + // upstream side effects + fn has_found_upstream_side_effect_functions_with_name(&self, name: &[&str]) -> bool { + name.iter().all(|&n| { + self.upstream_side_effects_func_definitions_names + .contains(&n.to_string()) + }) + } + } + + impl StandardInvestigatorVisitor for Tracker<'_> { + fn visit_entry_point(&mut self, node: &ASTNode) -> eyre::Result<()> { + self.entry_points + .push(self.context.get_node_sort_key_pure(node)); + Ok(()) + } + fn visit_downstream_function_definition( + &mut self, + node: &crate::ast::FunctionDefinition, + ) -> eyre::Result<()> { + self.downstream_func_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_downstream_modifier_definition( + &mut self, + node: &crate::ast::ModifierDefinition, + ) -> eyre::Result<()> { + self.downstream_modifier_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_upstream_function_definition( + &mut self, + node: &crate::ast::FunctionDefinition, + ) -> eyre::Result<()> { + self.upstream_func_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_upstream_modifier_definition( + &mut self, + node: &crate::ast::ModifierDefinition, + ) -> eyre::Result<()> { + self.upstream_modifier_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_upstream_side_effect_function_definition( + &mut self, + node: &FunctionDefinition, + ) -> eyre::Result<()> { + self.upstream_side_effects_func_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_upstream_side_effect_modifier_definition( + &mut self, + node: &ModifierDefinition, + ) -> eyre::Result<()> { + self.upstream_side_effects_modifier_definitions_names + .push(node.name.to_string()); + Ok(()) + } + } +} diff --git a/aderyn_core/src/context/investigator/mod.rs b/aderyn_core/src/context/investigator/mod.rs new file mode 100644 index 000000000..79f660156 --- /dev/null +++ b/aderyn_core/src/context/investigator/mod.rs @@ -0,0 +1,46 @@ +mod callgraph_tests; +mod standard; +mod traits; + +pub use standard::*; +pub use traits::*; + +use derive_more::From; + +use crate::ast::{ASTNode, NodeID}; + +pub type Result = core::result::Result; + +#[derive(Debug, From)] +pub enum Error { + #[from] + Custom(String), + + // region: -- standard::* errors + ForwardCallgraphNotAvailable, + BackwardCallgraphNotAvailable, + UnidentifiedEntryPointNode(ASTNode), + InvalidEntryPointId(NodeID), + EntryPointVisitError, + UpstreamFunctionDefinitionVisitError, + UpstreamModifierDefinitionVisitError, + DownstreamFunctionDefinitionVisitError, + DownstreamModifierDefinitionVisitError, + UpstreamSideEffectFunctionDefinitionVisitError, + UpstreamSideEffectModifierDefinitionVisitError, + // endregion +} + +impl core::fmt::Display for Error { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{self:?}") + } +} + +impl From<&str> for Error { + fn from(value: &str) -> Self { + Error::Custom(value.to_string()) + } +} + +impl std::error::Error for Error {} diff --git a/aderyn_core/src/context/investigator/standard.rs b/aderyn_core/src/context/investigator/standard.rs new file mode 100644 index 000000000..70329a925 --- /dev/null +++ b/aderyn_core/src/context/investigator/standard.rs @@ -0,0 +1,372 @@ +//! This module helps with strategies on performing different types of investigations. +//! +//! Our first kind of investigator is [`StandardInvestigator`] it comes bundled with actions to help +//! application modules "hook in" and consume the graphs. +//! +//! + +use std::collections::HashSet; + +use crate::{ + ast::{NodeID, NodeType}, + context::{ + browser::{ExtractReferencedDeclarations, GetClosestAncestorOfTypeX}, + graph::WorkspaceCallGraph, + workspace_context::{ASTNode, WorkspaceContext}, + }, +}; + +use super::StandardInvestigatorVisitor; + +#[derive(PartialEq)] +pub enum StandardInvestigationStyle { + /// Picks the regular call graph (forward) + Downstream, + + /// Picks the reverse call graph + Upstream, + + /// Picks both the call graphs (choose this if upstream side effects also need to be tracked) + BothWays, +} + +pub struct StandardInvestigator { + /// Ad-hoc Nodes that we would like to explore downstream from. + pub entry_points: Vec, + + /// Surface points are calculated based on the entry points (input) + /// and only consists of [`crate::ast::FunctionDefinition`] and [`crate::ast::ModifierDefinition`] + /// These are nodes that are the *actual* starting points for traversal in the graph + pub forward_surface_points: Vec, + + /// Same as the forward one, but acts on reverse graph. + pub backward_surface_points: Vec, + + /// Decides what graph type to chose from [`WorkspaceContext`] + pub investigation_style: StandardInvestigationStyle, +} + +#[derive(PartialEq, Clone, Copy)] +enum CurrentDFSVector { + Forward, // Going downstream + Backward, // Going upstream + UpstreamSideEffect, // Going downstream from upstream nodes +} + +impl StandardInvestigator { + /// Creates a [`StandardInvestigator`] by exploring paths from given nodes. This is the starting point. + pub fn for_specific_nodes( + context: &WorkspaceContext, + nodes: &[&ASTNode], + investigation_style: StandardInvestigationStyle, + ) -> super::Result { + let mut entry_points = vec![]; + let mut forward_surface_points = vec![]; + let mut backward_surface_points = vec![]; + + // Construct entry points + for &node in nodes { + let node_id = node + .id() + .ok_or_else(|| super::Error::UnidentifiedEntryPointNode(node.clone()))?; + entry_points.push(node_id); + } + + // Construct forward surface points + for &node in nodes { + let referenced_declarations = ExtractReferencedDeclarations::from(node).extracted; + + for declared_id in referenced_declarations { + if let Some(node) = context.nodes.get(&declared_id) { + if node.node_type() == NodeType::ModifierDefinition { + forward_surface_points.push(declared_id); + } else if let ASTNode::FunctionDefinition(function_definition) = node { + if function_definition.implemented { + forward_surface_points.push(declared_id); + } + } + } + } + } + + // Construct backward surface points + for &node in nodes { + if node.node_type() == NodeType::FunctionDefinition + || node.node_type() == NodeType::ModifierDefinition + { + if let Some(id) = node.id() { + backward_surface_points.push(id); + } + } else { + let parent_surface_point = node + .closest_ancestor_of_type(context, NodeType::FunctionDefinition) + .or_else(|| { + node.closest_ancestor_of_type(context, NodeType::ModifierDefinition) + }); + if let Some(parent_surface_point) = parent_surface_point { + if let Some(parent_surface_point_id) = parent_surface_point.id() { + backward_surface_points.push(parent_surface_point_id); + } + } + } + } + + Ok(StandardInvestigator { + entry_points, + forward_surface_points, + backward_surface_points, + investigation_style, + }) + } + + pub fn new( + context: &WorkspaceContext, + nodes: &[&ASTNode], + investigation_style: StandardInvestigationStyle, + ) -> super::Result { + Self::for_specific_nodes(context, nodes, investigation_style) + } + + /// Visit the entry points and all the plausible function definitions and modifier definitions that + /// EVM may encounter during execution. + pub fn investigate(&self, context: &WorkspaceContext, visitor: &mut T) -> super::Result<()> + where + T: StandardInvestigatorVisitor, + { + self._investigate( + context, + context + .forward_callgraph + .as_ref() + .ok_or(super::Error::ForwardCallgraphNotAvailable)?, + context + .reverse_callgraph + .as_ref() + .ok_or(super::Error::BackwardCallgraphNotAvailable)?, + visitor, + ) + } + + /// Responsible for informing the trackers. + /// First, we visit the entry points. Then, we derive the subgraph from the [`WorkspaceCallGraph`] + /// which consists of all the nodes that can be reached by traversing the edges starting + /// from the surface points. + fn _investigate( + &self, + context: &WorkspaceContext, + forward_callgraph: &WorkspaceCallGraph, + reverse_callgraph: &WorkspaceCallGraph, + visitor: &mut T, + ) -> super::Result<()> + where + T: StandardInvestigatorVisitor, + { + // Visit entry point nodes (so that trackers can track the state across all code regions in 1 place) + for entry_point_id in &self.entry_points { + self.make_entry_point_visit_call(context, *entry_point_id, visitor)?; + } + + // Keep track of visited node IDs during DFS from surface nodes + let mut visited_downstream = HashSet::new(); + let mut visited_upstream = HashSet::new(); + let mut visited_upstream_side_effects = HashSet::new(); + + // Now decide, which points to visit upstream or downstream + if self.investigation_style == StandardInvestigationStyle::BothWays + || self.investigation_style == StandardInvestigationStyle::Downstream + { + // Visit the subgraph starting from surface points + for surface_point_id in &self.forward_surface_points { + self.dfs_and_visit_subgraph( + *surface_point_id, + &mut visited_downstream, + context, + forward_callgraph, + visitor, + CurrentDFSVector::Forward, + None, + )?; + } + } + + if self.investigation_style == StandardInvestigationStyle::BothWays + || self.investigation_style == StandardInvestigationStyle::Upstream + { + // Visit the subgraph starting from surface points + for surface_point_id in &self.backward_surface_points { + self.dfs_and_visit_subgraph( + *surface_point_id, + &mut visited_upstream, + context, + reverse_callgraph, + visitor, + CurrentDFSVector::Backward, + None, + )?; + } + } + + // Collect already visited nodes so that we don't repeat visit calls on them + // while traversing through side effect nodes. + let mut blacklisted = HashSet::new(); + blacklisted.extend(visited_downstream.iter()); + blacklisted.extend(visited_upstream.iter()); + blacklisted.extend(self.entry_points.iter()); + + if self.investigation_style == StandardInvestigationStyle::BothWays { + // Visit the subgraph from the upstream points (go downstream in forward graph) + // but do not re-visit the upstream nodes or the downstream nodes again + + for surface_point_id in &visited_upstream { + self.dfs_and_visit_subgraph( + *surface_point_id, + &mut visited_upstream_side_effects, + context, + forward_callgraph, + visitor, + CurrentDFSVector::UpstreamSideEffect, + Some(&blacklisted), + )?; + } + } + + Ok(()) + } + + #[allow(clippy::too_many_arguments)] + fn dfs_and_visit_subgraph( + &self, + node_id: NodeID, + visited: &mut HashSet, + context: &WorkspaceContext, + callgraph: &WorkspaceCallGraph, + visitor: &mut T, + current_investigation_direction: CurrentDFSVector, + blacklist: Option<&HashSet>, + ) -> super::Result<()> + where + T: StandardInvestigatorVisitor, + { + if visited.contains(&node_id) { + return Ok(()); + } + + visited.insert(node_id); + + if let Some(blacklist) = blacklist { + if !blacklist.contains(&node_id) { + self.make_relevant_visit_call( + context, + node_id, + visitor, + current_investigation_direction, + )?; + } + } else { + self.make_relevant_visit_call( + context, + node_id, + visitor, + current_investigation_direction, + )?; + } + + if let Some(pointing_to) = callgraph.graph.get(&node_id) { + for destination in pointing_to { + self.dfs_and_visit_subgraph( + *destination, + visited, + context, + callgraph, + visitor, + current_investigation_direction, + blacklist, + )?; + } + } + Ok(()) + } + + fn make_relevant_visit_call( + &self, + context: &WorkspaceContext, + node_id: NodeID, + visitor: &mut T, + current_investigation_direction: CurrentDFSVector, + ) -> super::Result<()> + where + T: StandardInvestigatorVisitor, + { + if let Some(node) = context.nodes.get(&node_id) { + if node.node_type() != NodeType::FunctionDefinition + && node.node_type() != NodeType::ModifierDefinition + { + return Ok(()); + } + + match current_investigation_direction { + CurrentDFSVector::Forward => { + if let ASTNode::FunctionDefinition(function) = node { + visitor + .visit_downstream_function_definition(function) + .map_err(|_| super::Error::DownstreamFunctionDefinitionVisitError)?; + } + if let ASTNode::ModifierDefinition(modifier) = node { + visitor + .visit_downstream_modifier_definition(modifier) + .map_err(|_| super::Error::DownstreamModifierDefinitionVisitError)?; + } + } + CurrentDFSVector::Backward => { + if let ASTNode::FunctionDefinition(function) = node { + visitor + .visit_upstream_function_definition(function) + .map_err(|_| super::Error::UpstreamFunctionDefinitionVisitError)?; + } + if let ASTNode::ModifierDefinition(modifier) = node { + visitor + .visit_upstream_modifier_definition(modifier) + .map_err(|_| super::Error::UpstreamModifierDefinitionVisitError)?; + } + } + CurrentDFSVector::UpstreamSideEffect => { + if let ASTNode::FunctionDefinition(function) = node { + visitor + .visit_upstream_side_effect_function_definition(function) + .map_err(|_| { + super::Error::UpstreamSideEffectFunctionDefinitionVisitError + })?; + } + if let ASTNode::ModifierDefinition(modifier) = node { + visitor + .visit_upstream_side_effect_modifier_definition(modifier) + .map_err(|_| { + super::Error::UpstreamSideEffectModifierDefinitionVisitError + })?; + } + } + } + } + + Ok(()) + } + + fn make_entry_point_visit_call( + &self, + context: &WorkspaceContext, + node_id: NodeID, + visitor: &mut T, + ) -> super::Result<()> + where + T: StandardInvestigatorVisitor, + { + let node = context + .nodes + .get(&node_id) + .ok_or(super::Error::InvalidEntryPointId(node_id))?; + visitor + .visit_entry_point(node) + .map_err(|_| super::Error::EntryPointVisitError)?; + Ok(()) + } +} diff --git a/aderyn_core/src/context/investigator/traits.rs b/aderyn_core/src/context/investigator/traits.rs new file mode 100644 index 000000000..c0c9a9b6d --- /dev/null +++ b/aderyn_core/src/context/investigator/traits.rs @@ -0,0 +1,78 @@ +//! Trackers can implement the following traits to interact with investigators +//! +//! NOTE +//! Upstream and downstream here is relative to [`super::StandardInvestigator::entry_points`] +//! which is initialized with [`super::StandardInvestigator::new`] function. + +use crate::{ + ast::{FunctionDefinition, ModifierDefinition}, + context::workspace_context::ASTNode, +}; + +/// Use with [`super::StandardInvestigator`] +pub trait StandardInvestigatorVisitor { + /// Shift all logic to tracker otherwise, you would track state at 2 different places + /// One at the tracker level, and other at the application level. Instead, we must + /// contain all of the tracking logic in the tracker. Therefore, visit entry point + /// is essential because the tracker can get to take a look at not just the + /// downstream functions and modifiers, but also the entry points that have invoked it. + fn visit_entry_point(&mut self, node: &ASTNode) -> eyre::Result<()> { + self.visit_any(node) + } + + /// Meant to be invoked while traversing [`crate::context::workspace_context::WorkspaceContext::forward_callgraph`] + fn visit_downstream_function_definition( + &mut self, + node: &FunctionDefinition, + ) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Meant to be invoked while traversing [`crate::context::workspace_context::WorkspaceContext::reverse_callgraph`] + fn visit_upstream_function_definition( + &mut self, + node: &FunctionDefinition, + ) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Meant to be invoked while traversing [`crate::context::workspace_context::WorkspaceContext::forward_callgraph`] + fn visit_downstream_modifier_definition( + &mut self, + node: &ModifierDefinition, + ) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Meant to be invoked while traversing [`crate::context::workspace_context::WorkspaceContext::reverse_callgraph`] + fn visit_upstream_modifier_definition( + &mut self, + node: &ModifierDefinition, + ) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Read as "upstream's downstream-side-effect" function definition + /// These are function definitions that are downstream from the upstream nodes + /// but are themselves neither upstream nor downstream to the entry points + fn visit_upstream_side_effect_function_definition( + &mut self, + node: &FunctionDefinition, + ) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Read as "upstream's downstream-side-effect" modifier definition + /// These are modifier definitions that are downstream from the upstream nodes + /// but are themselves neither upstream nor downstream to the entry points + fn visit_upstream_side_effect_modifier_definition( + &mut self, + node: &ModifierDefinition, + ) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + fn visit_any(&mut self, _node: &ASTNode) -> eyre::Result<()> { + Ok(()) + } +} diff --git a/aderyn_core/src/context/mod.rs b/aderyn_core/src/context/mod.rs index 4de4873cb..ca73f66c9 100644 --- a/aderyn_core/src/context/mod.rs +++ b/aderyn_core/src/context/mod.rs @@ -1,5 +1,7 @@ pub mod browser; pub mod capturable; +pub mod graph; +pub mod investigator; pub mod macros; pub mod meta_workspace; pub mod workspace_context; diff --git a/aderyn_core/src/context/workspace_context.rs b/aderyn_core/src/context/workspace_context.rs index 0adf3c650..5cd2cdb47 100644 --- a/aderyn_core/src/context/workspace_context.rs +++ b/aderyn_core/src/context/workspace_context.rs @@ -4,6 +4,7 @@ use std::collections::HashMap; use super::browser::GetImmediateParent; use super::capturable::Capturable; +use super::graph::WorkspaceCallGraph; pub use crate::ast::ASTNode; #[derive(Default, Debug, Clone, PartialEq, Eq, Hash)] @@ -27,6 +28,8 @@ pub struct WorkspaceContext { pub src_filepaths: Vec, pub sloc_stats: HashMap, pub ignore_lines_stats: HashMap>>, + pub forward_callgraph: Option, + pub reverse_callgraph: Option, pub nodes: HashMap, // Hashmaps of all nodes => source_unit_id diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index 1d7d7b31e..91264b88c 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -4,34 +4,9 @@ use strum::{Display, EnumCount, EnumIter, EnumString}; use crate::{ ast::NodeID, context::workspace_context::WorkspaceContext, - detect::{ - high::{ - ArbitraryTransferFromDetector, AvoidAbiEncodePackedDetector, - BlockTimestampDeadlineDetector, DangerousUnaryOperatorDetector, - DelegateCallInLoopDetector, DeletionNestedMappingDetector, - DynamicArrayLengthAssignmentDetector, EnumerableLoopRemovalDetector, - ExperimentalEncoderDetector, IncorrectShiftOrderDetector, - IncorrectUseOfCaretOperatorDetector, MultipleConstructorsDetector, - NestedStructInMappingDetector, RTLODetector, ReusedContractNameDetector, - SelfdestructIdentifierDetector, 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, @@ -83,6 +58,9 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), + Box::::default(), + Box::::default(), Box::::default(), Box::::default(), Box::::default(), @@ -141,6 +119,9 @@ pub(crate) enum IssueDetectorNamePool { IncorrectCaretOperator, YulReturn, StateVariableShadowing, + MisusedBoolean, + SendEtherNoChecks, + DelegateCallUncheckedAddress, TautologicalCompare, #[allow(clippy::upper_case_acronyms)] RTLO, @@ -267,6 +248,13 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { Some(Box::::default()) } + IssueDetectorNamePool::MisusedBoolean => Some(Box::::default()), + IssueDetectorNamePool::SendEtherNoChecks => { + Some(Box::::default()) + } + IssueDetectorNamePool::DelegateCallUncheckedAddress => { + Some(Box::::default()) + } IssueDetectorNamePool::TautologicalCompare => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/helpers.rs b/aderyn_core/src/detect/helpers.rs index 0bf8e34f3..f19a79c50 100644 --- a/aderyn_core/src/detect/helpers.rs +++ b/aderyn_core/src/detect/helpers.rs @@ -2,11 +2,14 @@ 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::{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, @@ -122,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 new file mode 100644 index 000000000..10c816894 --- /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() + } +} + +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::{ + 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") + ); + } +} 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 847923b09..b23275b99 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -3,17 +3,20 @@ pub(crate) mod avoid_abi_encode_packed; pub(crate) mod block_timestamp_deadline; pub(crate) mod dangerous_unary_operator; pub(crate) mod delegate_call_in_loop; +pub(crate) mod delegate_call_no_address_check; pub(crate) mod deletion_nested_mapping; pub(crate) mod dynamic_array_length_assignment; pub(crate) mod enumerable_loop_removal; pub(crate) mod experimental_encoder; pub(crate) mod incorrect_caret_operator; pub(crate) mod incorrect_shift_order; +pub(crate) mod misused_boolean; pub(crate) mod multiple_constructors; pub(crate) mod nested_struct_in_mapping; pub(crate) mod 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; @@ -28,17 +31,20 @@ pub use avoid_abi_encode_packed::AvoidAbiEncodePackedDetector; pub use block_timestamp_deadline::BlockTimestampDeadlineDetector; pub use dangerous_unary_operator::DangerousUnaryOperatorDetector; pub use delegate_call_in_loop::DelegateCallInLoopDetector; +pub use delegate_call_no_address_check::DelegateCallOnUncheckedAddressDetector; pub use deletion_nested_mapping::DeletionNestedMappingDetector; pub use dynamic_array_length_assignment::DynamicArrayLengthAssignmentDetector; pub use enumerable_loop_removal::EnumerableLoopRemovalDetector; pub use experimental_encoder::ExperimentalEncoderDetector; pub use incorrect_caret_operator::IncorrectUseOfCaretOperatorDetector; pub use incorrect_shift_order::IncorrectShiftOrderDetector; +pub use misused_boolean::MisusedBooleanDetector; pub use multiple_constructors::MultipleConstructorsDetector; pub use nested_struct_in_mapping::NestedStructInMappingDetector; pub use 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/mod.rs b/aderyn_core/src/detect/mod.rs index 467a84bb3..fc80fd369 100644 --- a/aderyn_core/src/detect/mod.rs +++ b/aderyn_core/src/detect/mod.rs @@ -24,4 +24,74 @@ macro_rules! capture { }; } +#[macro_export] +macro_rules! issue_detector { + ( + $detector_struct:ident; + + severity: $detector_severity:ident, + title: $detector_title:tt, + desc: $detector_desc:tt, + name: $detector_name:ident, + + |$context: ident| $e:expr + ) => { + + #[derive(Default)] + pub struct $detector_struct { + found_instances: std::collections::BTreeMap<(String, usize, String), $crate::ast::NodeID>, + } + + impl $crate::detect::detector::IssueDetector for $detector_struct { + + fn detect(&mut self, context: &$crate::context::workspace_context::WorkspaceContext) -> Result> { + + let $context = context; + + macro_rules! grab { + ($item:expr) => { + if let Some(id) = context.get_node_id_of_capturable(&$item.clone().into()) { + self.found_instances.insert( + $context.get_node_sort_key_from_capturable(&$item.clone().into()), + id, + ); + } else { + self.found_instances.insert( + $context.get_node_sort_key_from_capturable(&$item.clone().into()), + 0, + ); + } + }; + } + + $e + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> $crate::detect::detector::IssueSeverity { + $crate::detect::detector::IssueSeverity::$detector_severity + } + + fn title(&self) -> String { + String::from($detector_title) + } + + fn description(&self) -> String { + String::from($detector_desc) + } + + fn instances(&self) -> std::collections::BTreeMap<(String, usize, String), $crate::ast::NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + $crate::detect::detector::IssueDetectorNamePool::$detector_name.to_string() + } + } + + }; +} + pub use capture; + +pub use issue_detector; diff --git a/aderyn_core/src/detect/test_utils/load_source_unit.rs b/aderyn_core/src/detect/test_utils/load_source_unit.rs index 1b4f1dc38..31844c086 100644 --- a/aderyn_core/src/detect/test_utils/load_source_unit.rs +++ b/aderyn_core/src/detect/test_utils/load_source_unit.rs @@ -5,13 +5,27 @@ use std::{ sync::Arc, }; -use crate::visitor::ast_visitor::Node; -use crate::{ast::SourceUnit, context::workspace_context::WorkspaceContext}; +use crate::{ + ast::SourceUnit, + context::{graph::traits::Transpose, workspace_context::WorkspaceContext}, +}; +use crate::{context::graph::WorkspaceCallGraph, visitor::ast_visitor::Node}; use super::ensure_valid_solidity_file; +#[cfg(test)] +pub fn load_solidity_source_unit_with_callgraphs(filepath: &str) -> WorkspaceContext { + _load_solidity_source_unit(filepath, true) +} + #[cfg(test)] pub fn load_solidity_source_unit(filepath: &str) -> WorkspaceContext { + println!("WARNING: Callgraph won't be loaded. Please use `load_solidity_source_unit_with_callgraphs` if callgraph is required!"); + _load_solidity_source_unit(filepath, false) +} + +#[cfg(test)] +fn _load_solidity_source_unit(filepath: &str, should_load_callgraphs: bool) -> WorkspaceContext { let solidity_file = &ensure_valid_solidity_file(filepath); let solidity_content = std::fs::read_to_string(solidity_file).unwrap(); @@ -81,6 +95,10 @@ pub fn load_solidity_source_unit(filepath: &str) -> WorkspaceContext { absorb_ast_content_into_context(&ast_content, solidity_content.clone(), &mut context); } + if should_load_callgraphs { + load_callgraphs(&mut context); + } + context } else { eprintln!("Error running solc command"); @@ -88,6 +106,15 @@ pub fn load_solidity_source_unit(filepath: &str) -> WorkspaceContext { } } +fn load_callgraphs(context: &mut WorkspaceContext) { + let forward_callgraph = WorkspaceCallGraph::from_context(context).unwrap(); + let reverse_callgraph = WorkspaceCallGraph { + graph: forward_callgraph.graph.reverse(), + }; + context.forward_callgraph = Some(forward_callgraph); + context.reverse_callgraph = Some(reverse_callgraph); +} + fn absorb_ast_content_into_context( ast_content: &str, solidity_content: String, diff --git a/aderyn_core/src/detect/test_utils/mod.rs b/aderyn_core/src/detect/test_utils/mod.rs index 10a76c7fd..b10ddc7df 100644 --- a/aderyn_core/src/detect/test_utils/mod.rs +++ b/aderyn_core/src/detect/test_utils/mod.rs @@ -7,6 +7,7 @@ use std::path::PathBuf; // Using `solc` to read AST given a source unit (i.e Solidity file) pub use load_source_unit::load_multiple_solidity_source_units_into_single_context; pub use load_source_unit::load_solidity_source_unit; +pub use load_source_unit::load_solidity_source_unit_with_callgraphs; pub(crate) fn take_loader_lock() -> impl Drop { static LOCK: Lazy> = Lazy::new(|| std::sync::Mutex::new(())); diff --git a/aderyn_driver/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 17c079877..5ade44f99 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,9 @@ "incorrect-caret-operator", "yul-return", "state-variable-shadowing", + "misused-boolean", + "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 b262e65f1..96bc6360c 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 61, - "total_sloc": 1631 + "total_source_units": 65, + "total_sloc": 1836 }, "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/DeletionNestedMappingStructureContract.sol", "n_sloc": 11 @@ -97,6 +105,10 @@ "file_path": "src/KeccakContract.sol", "n_sloc": 21 }, + { + "file_path": "src/MisusedBoolean.sol", + "n_sloc": 67 + }, { "file_path": "src/MultipleConstructorSchemes.sol", "n_sloc": 10 @@ -113,6 +125,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 @@ -252,7 +268,7 @@ ] }, "issue_count": { - "high": 24, + "high": 27, "low": 23 }, "high_issues": { @@ -1219,6 +1235,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, @@ -1350,6 +1372,135 @@ } ] }, + { + "title": "Misused boolean with logical operators", + "description": "The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively.", + "detector_name": "misused-boolean", + "instances": [ + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 12, + "src": "257:19", + "src_char": "257:19" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 19, + "src": "419:20", + "src_char": "419:20" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 26, + "src": "582:20", + "src_char": "582:20" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 33, + "src": "745:19", + "src_char": "745:19" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 40, + "src": "908:51", + "src_char": "908:51" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 47, + "src": "1060:52", + "src_char": "1060:52" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 54, + "src": "1213:53", + "src_char": "1213:53" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 61, + "src": "1366:21", + "src_char": "1366:21" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 68, + "src": "1530:17", + "src_char": "1530:17" + }, + { + "contract_path": "src/MisusedBoolean.sol", + "line_no": 75, + "src": "1691:18", + "src_char": "1691:18" + } + ] + }, + { + "title": "Sending native Eth is not protected from these functions.", + "description": "Introduce checks for `msg.sender` in the function", + "detector_name": "send-ether-no-checks", + "instances": [ + { + "contract_path": "src/CallGraphTests.sol", + "line_no": 38, + "src": "686:16", + "src_char": "686:16" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 53, + "src": "1060:5", + "src_char": "1060:5" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 77, + "src": "1405:5", + "src_char": "1405:5" + }, + { + "contract_path": "src/SendEtherNoChecks.sol", + "line_no": 99, + "src": "1795:5", + "src_char": "1795:5" + }, + { + "contract_path": "src/UninitializedStateVariable.sol", + "line_no": 17, + "src": "563:8", + "src_char": "563:8" + } + ] + }, + { + "title": "Delegatecall made by the function without checks on any adress.", + "description": "Introduce checks on the address", + "detector_name": "delegate-call-unchecked-address", + "instances": [ + { + "contract_path": "src/DelegateCallWithoutAddressCheck.sol", + "line_no": 15, + "src": "392:9", + "src_char": "392:9" + }, + { + "contract_path": "src/auditor_mode/ExternalCalls.sol", + "line_no": 38, + "src": "1253:28", + "src_char": "1253:28" + }, + { + "contract_path": "src/inheritance/ExtendedInheritance.sol", + "line_no": 14, + "src": "391:15", + "src_char": "391:15" + } + ] + }, { "title": "Tautological comparison.", "description": "The left hand side and the right hand side of the binary operation has the same value. This makes the condition always true or always false.", @@ -1632,6 +1783,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, @@ -1675,6 +1832,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/DeletionNestedMappingStructureContract.sol", "line_no": 2, @@ -1990,6 +2153,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, @@ -2136,6 +2317,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, @@ -2154,6 +2365,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, @@ -2265,6 +2494,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/DeletionNestedMappingStructureContract.sol", "line_no": 2, @@ -2398,6 +2633,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, @@ -2410,6 +2669,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, @@ -2435,6 +2718,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, @@ -2495,6 +2790,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, @@ -2695,12 +3008,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, @@ -2975,6 +3342,9 @@ "incorrect-caret-operator", "yul-return", "state-variable-shadowing", + "misused-boolean", + "send-ether-no-checks", + "delegate-call-unchecked-address", "tautological-compare", "rtlo", "unchecked-return", diff --git a/reports/report.md b/reports/report.md index aa89504e0..c4bf1036c 100644 --- a/reports/report.md +++ b/reports/report.md @@ -27,11 +27,14 @@ 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-24: Deletion from a nested mappping.](#h-24-deletion-from-a-nested-mappping) + - [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: 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) @@ -64,8 +67,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 61 | -| Total nSLOC | 1631 | +| .sol Files | 65 | +| Total nSLOC | 1836 | ## Files Details @@ -76,12 +79,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/DeletionNestedMappingStructureContract.sol | 11 | | src/DeprecatedOZFunctions.sol | 32 | | src/DivisionBeforeMultiplication.sol | 22 | @@ -95,10 +100,12 @@ 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 | | src/RevertsAndRequriesInLoops.sol | 27 | +| src/SendEtherNoChecks.sol | 58 | | src/StateShadowing.sol | 17 | | src/StateVariables.sol | 58 | | src/StorageConditionals.sol | 59 | @@ -133,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** | **1631** | +| **Total** | **1836** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 24 | +| High | 27 | | Low | 23 | @@ -1163,7 +1170,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) @@ -1172,6 +1179,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 @@ -1317,7 +1330,148 @@ Description of the high issue. -## H-20: Tautological comparison. +## H-20: Misused boolean with logical operators + +The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively. + +
10 Found Instances + + +- Found in src/MisusedBoolean.sol [Line: 12](../tests/contract-playground/src/MisusedBoolean.sol#L12) + + ```solidity + if (isEven(num) || true) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 19](../tests/contract-playground/src/MisusedBoolean.sol#L19) + + ```solidity + if (isEven(num) && false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 26](../tests/contract-playground/src/MisusedBoolean.sol#L26) + + ```solidity + if (false && isEven(num)) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 33](../tests/contract-playground/src/MisusedBoolean.sol#L33) + + ```solidity + if (true || isEven(num)) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 40](../tests/contract-playground/src/MisusedBoolean.sol#L40) + + ```solidity + if (true) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 47](../tests/contract-playground/src/MisusedBoolean.sol#L47) + + ```solidity + if (false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 54](../tests/contract-playground/src/MisusedBoolean.sol#L54) + + ```solidity + if (!false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 61](../tests/contract-playground/src/MisusedBoolean.sol#L61) + + ```solidity + if (isEven(num) && !false) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 68](../tests/contract-playground/src/MisusedBoolean.sol#L68) + + ```solidity + if (isEven(num) && NO) { + ``` + +- Found in src/MisusedBoolean.sol [Line: 75](../tests/contract-playground/src/MisusedBoolean.sol#L75) + + ```solidity + if (isEven(num) && !NO) { + ``` + +
+ + + +## H-21: Sending native Eth is not protected from these functions. + +Introduce checks for `msg.sender` in the function + +
5 Found Instances + + +- Found in src/CallGraphTests.sol [Line: 38](../tests/contract-playground/src/CallGraphTests.sol#L38) + + ```solidity + function enterTenthFloor2(address x) external passThroughNinthFloor2(x) { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 53](../tests/contract-playground/src/SendEtherNoChecks.sol#L53) + + ```solidity + function func1(address x) external mod1(x) { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 77](../tests/contract-playground/src/SendEtherNoChecks.sol#L77) + + ```solidity + function func1(address x) external mod1(x) { + ``` + +- Found in src/SendEtherNoChecks.sol [Line: 99](../tests/contract-playground/src/SendEtherNoChecks.sol#L99) + + ```solidity + function func1(address x) external mod1(x) { + ``` + +- Found in src/UninitializedStateVariable.sol [Line: 17](../tests/contract-playground/src/UninitializedStateVariable.sol#L17) + + ```solidity + function transfer() payable public { + ``` + +
+ + + +## H-22: Delegatecall made by the function without checks on any adress. + +Introduce checks on the address + +
3 Found Instances + + +- Found in src/DelegateCallWithoutAddressCheck.sol [Line: 15](../tests/contract-playground/src/DelegateCallWithoutAddressCheck.sol#L15) + + ```solidity + function delegate1(address to, bytes memory data) external { + ``` + +- Found in src/auditor_mode/ExternalCalls.sol [Line: 38](../tests/contract-playground/src/auditor_mode/ExternalCalls.sol#L38) + + ```solidity + function rawDelegateCallFromParameter(address myTarget, bytes calldata data) external returns (bytes memory) { + ``` + +- Found in src/inheritance/ExtendedInheritance.sol [Line: 14](../tests/contract-playground/src/inheritance/ExtendedInheritance.sol#L14) + + ```solidity + function doSomethingElse(address target) external { + ``` + +
+ + + +## H-23: Tautological comparison. The left hand side and the right hand side of the binary operation has the same value. This makes the condition always true or always false. @@ -1352,7 +1506,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-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! @@ -1369,7 +1523,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-25: Return value of the function call is not checked. Function returns a value but it is ignored. @@ -1392,7 +1546,7 @@ Function returns a value but it is ignored. -## H-23: 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. @@ -1415,7 +1569,7 @@ Potentially mistakened `=+` for `+=` or `=-` for `-=`. Please include a space in -## H-24: Deletion from a nested mappping. +## 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. @@ -1587,7 +1741,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) @@ -1638,6 +1792,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 @@ -1658,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) @@ -1685,6 +1845,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/DeletionNestedMappingStructureContract.sol [Line: 2](../tests/contract-playground/src/DeletionNestedMappingStructureContract.sol#L2) ```solidity @@ -1943,7 +2109,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) @@ -2012,6 +2178,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 @@ -2157,8 +2335,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) @@ -2178,6 +2386,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 @@ -2274,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) @@ -2301,6 +2527,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/DeletionNestedMappingStructureContract.sol [Line: 2](../tests/contract-playground/src/DeletionNestedMappingStructureContract.sol#L2) ```solidity @@ -2435,9 +2667,33 @@ 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) ```solidity @@ -2450,6 +2706,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 @@ -2470,7 +2750,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) @@ -2479,6 +2759,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 @@ -2539,6 +2831,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 @@ -2744,8 +3054,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) @@ -2753,6 +3087,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 e005ea3d0..d8bda7569 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,231 @@ }, "ruleId": "state-variable-shadowing" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 19, + "byteOffset": 257 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 419 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 582 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 19, + "byteOffset": 745 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 51, + "byteOffset": 908 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 52, + "byteOffset": 1060 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 53, + "byteOffset": 1213 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 1366 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 17, + "byteOffset": 1530 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/MisusedBoolean.sol" + }, + "region": { + "byteLength": 18, + "byteOffset": 1691 + } + } + } + ], + "message": { + "text": "The patterns `if (… || true)` and `if (.. && false)` will always evaluate to true and false respectively." + }, + "ruleId": "misused-boolean" + }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/CallGraphTests.sol" + }, + "region": { + "byteLength": 16, + "byteOffset": 686 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 5, + "byteOffset": 1060 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 5, + "byteOffset": 1405 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 5, + "byteOffset": 1795 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/UninitializedStateVariable.sol" + }, + "region": { + "byteLength": 8, + "byteOffset": 563 + } + } + } + ], + "message": { + "text": "Introduce checks for `msg.sender` in the function" + }, + "ruleId": "send-ether-no-checks" + }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 9, + "byteOffset": 392 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/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": [ @@ -2404,6 +2640,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 19, + "byteOffset": 1255 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2479,6 +2726,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3045,6 +3303,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": { @@ -3308,62 +3599,150 @@ { "physicalLocation": { "artifactLocation": { - "uri": "src/DeprecatedOZFunctions.sol" + "uri": "src/CallGraphTests.sol" }, "region": { "byteLength": 7, - "byteOffset": 1264 + "byteOffset": 128 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/DeprecatedOZFunctions.sol" + "uri": "src/CallGraphTests.sol" }, "region": { "byteLength": 6, - "byteOffset": 1389 + "byteOffset": 531 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/RevertsAndRequriesInLoops.sol" + "uri": "src/CallGraphTests.sol" }, "region": { "byteLength": 6, - "byteOffset": 503 + "byteOffset": 936 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/StateShadowing.sol" + "uri": "src/CallGraphTests.sol" }, "region": { "byteLength": 7, - "byteOffset": 135 + "byteOffset": 1246 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/ZeroAddressCheck.sol" + "uri": "src/DelegateCallWithoutAddressCheck.sol" }, "region": { - "byteLength": 6, - "byteOffset": 329 + "byteLength": 7, + "byteOffset": 948 } } }, { "physicalLocation": { "artifactLocation": { - "uri": "src/ZeroAddressCheck.sol" + "uri": "src/DeprecatedOZFunctions.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 1264 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DeprecatedOZFunctions.sol" + }, + "region": { + "byteLength": 6, + "byteOffset": 1389 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/RevertsAndRequriesInLoops.sol" + }, + "region": { + "byteLength": 6, + "byteOffset": 503 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 6, + "byteOffset": 268 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 513 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/SendEtherNoChecks.sol" + }, + "region": { + "byteLength": 6, + "byteOffset": 920 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateShadowing.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 135 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ZeroAddressCheck.sol" + }, + "region": { + "byteLength": 6, + "byteOffset": 329 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ZeroAddressCheck.sol" }, "region": { "byteLength": 6, @@ -3530,6 +3909,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/DelegateCallWithoutAddressCheck.sol" + }, + "region": { + "byteLength": 21, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3770,6 +4160,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": { @@ -3792,6 +4226,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": { @@ -3834,6 +4312,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": { @@ -3944,6 +4444,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": { @@ -4303,6 +4836,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": { @@ -4314,6 +4891,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/MisusedBoolean.sol b/tests/contract-playground/src/MisusedBoolean.sol new file mode 100644 index 000000000..591beee03 --- /dev/null +++ b/tests/contract-playground/src/MisusedBoolean.sol @@ -0,0 +1,82 @@ +pragma solidity 0.4.22; + +contract MisusedBoolean { + + bool public constant NO = false; + + function isEven(uint256 num) internal returns(bool) { + return num % 2 == 0; + } + + function misuse(uint256 num) external returns(uint256) { + if (isEven(num) || true) { + return num * num; + } + return 0; + } + + function misuse2(uint256 num) external returns(uint256) { + if (isEven(num) && false) { + return num * num; + } + return 0; + } + + function misuse3(uint256 num) external returns(uint256) { + if (false && isEven(num)) { + return num * num; + } + return 0; + } + + function misuse4(uint256 num) external returns(uint256) { + if (true || isEven(num)) { + return num * num; + } + return 0; + } + + function misuse5(uint256 num) external pure returns(uint256) { + if (true) { + return num * num; + } + return 0; + } + + function misuse6(uint256 num) external pure returns(uint256) { + if (false) { + return num * num; + } + return 0; + } + + function misuse7(uint256 num) external pure returns(uint256) { + if (!false) { + return num * num; + } + return 0; + } + + function misuse8(uint256 num) external returns(uint256) { + if (isEven(num) && !false) { + return num * num; + } + return 0; + } + + function misuse9(uint256 num) external returns(uint256) { + if (isEven(num) && NO) { + return num * num; + } + return 0; + } + + function misuse10(uint256 num) external returns(uint256) { + if (isEven(num) && !NO) { + return num * num; + } + return 0; + } + + +} \ No newline at end of file diff --git a/tests/contract-playground/src/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) { + + } + +}