diff --git a/.github/workflows/python.yml b/.github/workflows/python.yml new file mode 100644 index 000000000..f8ea27a8c --- /dev/null +++ b/.github/workflows/python.yml @@ -0,0 +1,93 @@ +on: [push, pull_request, workflow_dispatch] + +name: Aderyn-py + +jobs: + check: + name: Check + runs-on: ubuntu-latest + steps: + - name: Checkout sources + uses: actions/checkout@v2 + + - name: Install stable toolchain + uses: actions-rs/toolchain@v1 + with: + profile: minimal + toolchain: stable + override: true + + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: "3.11" + + - name: Install git submodules + run: | + git submodule update --init --recursive + + - name: Run cargo check + uses: actions-rs/cargo@v1 + with: + command: check + + reports: + name: Check Reports + runs-on: ubuntu-latest + steps: + - name: foundry-toolchain + uses: foundry-rs/foundry-toolchain@v1.2.0 + + - name: Checkout sources + uses: actions/checkout@v2 + + - name: Install stable toolchain + uses: actions-rs/toolchain@v1 + with: + profile: minimal + toolchain: stable + override: true + + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: "3.11" + cache: "pip" + + - name: Submodule init + run: | + git submodule update --init --recursive + + - uses: actions/checkout@v3 + + - uses: actions/setup-node@v3 + with: + node-version: 20 + cache: "npm" + + - uses: pnpm/action-setup@v3 + with: + version: 8 + + - uses: bahmutov/npm-install@v1 + with: + useLockFile: false + working-directory: tests/2024-05-Sablier/v2-core + + - uses: bahmutov/npm-install@v1 + with: + useLockFile: false + working-directory: tests/prb-math/ + + - name: Setup virtual environment + run: | + python -m venv venv + source venv/bin/activate + pip install -r ./aderyn_py/requirements.txt + + - name: Run tests + run: | + source venv/bin/activate + cd aderyn_py + maturin develop + pytest tests diff --git a/Cargo.lock b/Cargo.lock index b7eb90584..c4005d063 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -70,6 +70,7 @@ dependencies = [ "criterion", "cyfrin-foundry-compilers", "cyfrin-foundry-config", + "field_access", "rayon", "serde", "serde_json", @@ -82,6 +83,7 @@ name = "aderyn_py" version = "0.1.10" dependencies = [ "aderyn_driver", + "field_access", "pyo3", ] @@ -1201,6 +1203,27 @@ dependencies = [ "subtle", ] +[[package]] +name = "field_access" +version = "0.1.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7e623ab785a56a622537034c7109d47898c41cb9219803155759dcca1f19655c" +dependencies = [ + "field_access_derive", + "paste", +] + +[[package]] +name = "field_access_derive" +version = "0.1.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "917d5dcdd27ef8b8bd66913702385e92fa2a2a9fd6f7b94ffa03e884d847a8d9" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.72", +] + [[package]] name = "figment" version = "0.10.19" @@ -1795,9 +1818,9 @@ dependencies = [ [[package]] name = "indoc" -version = "1.0.9" +version = "2.0.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bfa799dd5ed20a7e349f3b4639aa80d74549c81716d9ec4f994c9b5815598306" +checksum = "b248f5224d1d606005e02c97f5aa4e88eeb230488bcc03bc9ca4d7991399f2b5" [[package]] name = "inlinable_string" @@ -2483,6 +2506,12 @@ dependencies = [ "plotters-backend", ] +[[package]] +name = "portable-atomic" +version = "1.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "da544ee218f0d287a911e9c99a39a8c9bc8fcad3cb8db5959940044ecfc67265" + [[package]] name = "ppv-lite86" version = "0.2.18" @@ -2586,15 +2615,16 @@ dependencies = [ [[package]] name = "pyo3" -version = "0.19.2" +version = "0.22.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e681a6cfdc4adcc93b4d3cf993749a4552018ee0a9b65fc0ccfad74352c72a38" +checksum = "831e8e819a138c36e212f3af3fd9eeffed6bf1510a805af35b0edee5ffa59433" dependencies = [ "cfg-if", "indoc", "libc", "memoffset", - "parking_lot", + "once_cell", + "portable-atomic", "pyo3-build-config", "pyo3-ffi", "pyo3-macros", @@ -2603,9 +2633,9 @@ dependencies = [ [[package]] name = "pyo3-build-config" -version = "0.19.2" +version = "0.22.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "076c73d0bc438f7a4ef6fdd0c3bb4732149136abd952b110ac93e4edb13a6ba5" +checksum = "1e8730e591b14492a8945cdff32f089250b05f5accecf74aeddf9e8272ce1fa8" dependencies = [ "once_cell", "target-lexicon", @@ -2613,9 +2643,9 @@ dependencies = [ [[package]] name = "pyo3-ffi" -version = "0.19.2" +version = "0.22.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e53cee42e77ebe256066ba8aa77eff722b3bb91f3419177cf4cd0f304d3284d9" +checksum = "5e97e919d2df92eb88ca80a037969f44e5e70356559654962cbb3316d00300c6" dependencies = [ "libc", "pyo3-build-config", @@ -2623,25 +2653,27 @@ dependencies = [ [[package]] name = "pyo3-macros" -version = "0.19.2" +version = "0.22.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dfeb4c99597e136528c6dd7d5e3de5434d1ceaf487436a3f03b2d56b6fc9efd1" +checksum = "eb57983022ad41f9e683a599f2fd13c3664d7063a3ac5714cae4b7bee7d3f206" dependencies = [ "proc-macro2", "pyo3-macros-backend", "quote", - "syn 1.0.109", + "syn 2.0.72", ] [[package]] name = "pyo3-macros-backend" -version = "0.19.2" +version = "0.22.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "947dc12175c254889edc0c02e399476c2f652b4b9ebd123aa655c224de259536" +checksum = "ec480c0c51ddec81019531705acac51bcdbeae563557c982aa8263bb96880372" dependencies = [ + "heck 0.5.0", "proc-macro2", + "pyo3-build-config", "quote", - "syn 1.0.109", + "syn 2.0.72", ] [[package]] @@ -3867,9 +3899,9 @@ checksum = "f962df74c8c05a667b5ee8bcf162993134c104e96440b663c8daa176dc772d8c" [[package]] name = "unindent" -version = "0.1.11" +version = "0.2.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e1766d682d402817b5ac4490b3c3002d91dfa0d22812f341609f97b08757359c" +checksum = "c7de7d73e1754487cb58364ee906a499937a0dfabd86bcb980fa99ec8c8fa2ce" [[package]] name = "untrusted" diff --git a/aderyn_core/src/context/investigator/standard.rs b/aderyn_core/src/context/graph/callgraph.rs similarity index 60% rename from aderyn_core/src/context/investigator/standard.rs rename to aderyn_core/src/context/graph/callgraph.rs index 70329a925..7e8ae518d 100644 --- a/aderyn_core/src/context/investigator/standard.rs +++ b/aderyn_core/src/context/graph/callgraph.rs @@ -1,6 +1,6 @@ //! 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 +//! Our first kind of investigator is [`CallGraph`] it comes bundled with actions to help //! application modules "hook in" and consume the graphs. //! //! @@ -16,53 +16,53 @@ use crate::{ }, }; -use super::StandardInvestigatorVisitor; +use super::traits::CallGraphVisitor; #[derive(PartialEq)] -pub enum StandardInvestigationStyle { - /// Picks the regular call graph (forward) - Downstream, +pub enum CallGraphDirection { + /// Depper into the callgraph + Inward, - /// Picks the reverse call graph - Upstream, + /// Opposite of Inward + Outward, - /// Picks both the call graphs (choose this if upstream side effects also need to be tracked) + /// Both inward and outward (If outward side effects also need to be tracked) BothWays, } -pub struct StandardInvestigator { - /// Ad-hoc Nodes that we would like to explore downstream from. +pub struct CallGraph { + /// Ad-hoc Nodes that we would like to explore inward 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, + pub inward_surface_points: Vec, - /// Same as the forward one, but acts on reverse graph. - pub backward_surface_points: Vec, + /// Same as the inward one, but acts on reverse graph. + pub outward_surface_points: Vec, /// Decides what graph type to chose from [`WorkspaceContext`] - pub investigation_style: StandardInvestigationStyle, + pub direction: CallGraphDirection, } #[derive(PartialEq, Clone, Copy)] enum CurrentDFSVector { - Forward, // Going downstream - Backward, // Going upstream - UpstreamSideEffect, // Going downstream from upstream nodes + Inward, + Outward, + OutwardSideEffect, } -impl StandardInvestigator { - /// Creates a [`StandardInvestigator`] by exploring paths from given nodes. This is the starting point. - pub fn for_specific_nodes( +impl CallGraph { + /// Creates a [`CallGraph`] by exploring paths from given nodes. This is the starting point. + pub fn from_nodes( context: &WorkspaceContext, nodes: &[&ASTNode], - investigation_style: StandardInvestigationStyle, - ) -> super::Result { + direction: CallGraphDirection, + ) -> super::Result { let mut entry_points = vec![]; - let mut forward_surface_points = vec![]; - let mut backward_surface_points = vec![]; + let mut inward_surface_points = vec![]; + let mut outward_surface_points = vec![]; // Construct entry points for &node in nodes { @@ -72,30 +72,30 @@ impl StandardInvestigator { entry_points.push(node_id); } - // Construct forward surface points + // Construct inward 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); + inward_surface_points.push(declared_id); } else if let ASTNode::FunctionDefinition(function_definition) = node { if function_definition.implemented { - forward_surface_points.push(declared_id); + inward_surface_points.push(declared_id); } } } } } - // Construct backward surface points + // Construct outward 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); + outward_surface_points.push(id); } } else { let parent_surface_point = node @@ -105,44 +105,44 @@ impl StandardInvestigator { }); 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); + outward_surface_points.push(parent_surface_point_id); } } } } - Ok(StandardInvestigator { + Ok(CallGraph { entry_points, - forward_surface_points, - backward_surface_points, - investigation_style, + inward_surface_points, + outward_surface_points, + direction, }) } pub fn new( context: &WorkspaceContext, nodes: &[&ASTNode], - investigation_style: StandardInvestigationStyle, - ) -> super::Result { - Self::for_specific_nodes(context, nodes, investigation_style) + direction: CallGraphDirection, + ) -> super::Result { + Self::from_nodes(context, nodes, direction) } /// 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<()> + pub fn accept(&self, context: &WorkspaceContext, visitor: &mut T) -> super::Result<()> where - T: StandardInvestigatorVisitor, + T: CallGraphVisitor, { - self._investigate( + self._accept( context, context - .forward_callgraph + .inward_callgraph .as_ref() - .ok_or(super::Error::ForwardCallgraphNotAvailable)?, + .ok_or(super::Error::InwardCallgraphNotAvailable)?, context - .reverse_callgraph + .outward_callgraph .as_ref() - .ok_or(super::Error::BackwardCallgraphNotAvailable)?, + .ok_or(super::Error::OutwardCallgraphNotAvailable)?, visitor, ) } @@ -151,15 +151,15 @@ impl StandardInvestigator { /// 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( + fn _accept( &self, context: &WorkspaceContext, - forward_callgraph: &WorkspaceCallGraph, - reverse_callgraph: &WorkspaceCallGraph, + inward_callgraph: &WorkspaceCallGraph, + outward_callgraph: &WorkspaceCallGraph, visitor: &mut T, ) -> super::Result<()> where - T: StandardInvestigatorVisitor, + T: CallGraphVisitor, { // 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 { @@ -167,40 +167,40 @@ impl StandardInvestigator { } // 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(); + let mut visited_inward = HashSet::new(); + let mut visited_outward = HashSet::new(); + let mut visited_outward_side_effects = HashSet::new(); - // Now decide, which points to visit upstream or downstream - if self.investigation_style == StandardInvestigationStyle::BothWays - || self.investigation_style == StandardInvestigationStyle::Downstream + // Now decide, which points to visit outward or inward + if self.direction == CallGraphDirection::BothWays + || self.direction == CallGraphDirection::Inward { // Visit the subgraph starting from surface points - for surface_point_id in &self.forward_surface_points { + for surface_point_id in &self.inward_surface_points { self.dfs_and_visit_subgraph( *surface_point_id, - &mut visited_downstream, + &mut visited_inward, context, - forward_callgraph, + inward_callgraph, visitor, - CurrentDFSVector::Forward, + CurrentDFSVector::Inward, None, )?; } } - if self.investigation_style == StandardInvestigationStyle::BothWays - || self.investigation_style == StandardInvestigationStyle::Upstream + if self.direction == CallGraphDirection::BothWays + || self.direction == CallGraphDirection::Outward { // Visit the subgraph starting from surface points - for surface_point_id in &self.backward_surface_points { + for surface_point_id in &self.outward_surface_points { self.dfs_and_visit_subgraph( *surface_point_id, - &mut visited_upstream, + &mut visited_outward, context, - reverse_callgraph, + outward_callgraph, visitor, - CurrentDFSVector::Backward, + CurrentDFSVector::Outward, None, )?; } @@ -209,22 +209,22 @@ impl StandardInvestigator { // 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(visited_inward.iter()); + blacklisted.extend(visited_outward.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 + if self.direction == CallGraphDirection::BothWays { + // Visit the subgraph from the outward points (go inward in inward graph) + // but do not re-visit the outward nodes or the inward nodes again - for surface_point_id in &visited_upstream { + for surface_point_id in &visited_outward { self.dfs_and_visit_subgraph( *surface_point_id, - &mut visited_upstream_side_effects, + &mut visited_outward_side_effects, context, - forward_callgraph, + inward_callgraph, visitor, - CurrentDFSVector::UpstreamSideEffect, + CurrentDFSVector::OutwardSideEffect, Some(&blacklisted), )?; } @@ -245,7 +245,7 @@ impl StandardInvestigator { blacklist: Option<&HashSet>, ) -> super::Result<()> where - T: StandardInvestigatorVisitor, + T: CallGraphVisitor, { if visited.contains(&node_id) { return Ok(()); @@ -271,7 +271,7 @@ impl StandardInvestigator { )?; } - if let Some(pointing_to) = callgraph.graph.get(&node_id) { + if let Some(pointing_to) = callgraph.raw_callgraph.get(&node_id) { for destination in pointing_to { self.dfs_and_visit_subgraph( *destination, @@ -295,7 +295,7 @@ impl StandardInvestigator { current_investigation_direction: CurrentDFSVector, ) -> super::Result<()> where - T: StandardInvestigatorVisitor, + T: CallGraphVisitor, { if let Some(node) = context.nodes.get(&node_id) { if node.node_type() != NodeType::FunctionDefinition @@ -305,43 +305,43 @@ impl StandardInvestigator { } match current_investigation_direction { - CurrentDFSVector::Forward => { + CurrentDFSVector::Inward => { if let ASTNode::FunctionDefinition(function) = node { visitor - .visit_downstream_function_definition(function) - .map_err(|_| super::Error::DownstreamFunctionDefinitionVisitError)?; + .visit_inward_function_definition(function) + .map_err(|_| super::Error::InwardFunctionDefinitionVisitError)?; } if let ASTNode::ModifierDefinition(modifier) = node { visitor - .visit_downstream_modifier_definition(modifier) - .map_err(|_| super::Error::DownstreamModifierDefinitionVisitError)?; + .visit_inward_modifier_definition(modifier) + .map_err(|_| super::Error::InwardModifierDefinitionVisitError)?; } } - CurrentDFSVector::Backward => { + CurrentDFSVector::Outward => { if let ASTNode::FunctionDefinition(function) = node { visitor - .visit_upstream_function_definition(function) - .map_err(|_| super::Error::UpstreamFunctionDefinitionVisitError)?; + .visit_outward_function_definition(function) + .map_err(|_| super::Error::OutwardFunctionDefinitionVisitError)?; } if let ASTNode::ModifierDefinition(modifier) = node { visitor - .visit_upstream_modifier_definition(modifier) - .map_err(|_| super::Error::UpstreamModifierDefinitionVisitError)?; + .visit_outward_modifier_definition(modifier) + .map_err(|_| super::Error::OutwardModifierDefinitionVisitError)?; } } - CurrentDFSVector::UpstreamSideEffect => { + CurrentDFSVector::OutwardSideEffect => { if let ASTNode::FunctionDefinition(function) = node { visitor - .visit_upstream_side_effect_function_definition(function) + .visit_outward_side_effect_function_definition(function) .map_err(|_| { - super::Error::UpstreamSideEffectFunctionDefinitionVisitError + super::Error::OutwardSideEffectFunctionDefinitionVisitError })?; } if let ASTNode::ModifierDefinition(modifier) = node { visitor - .visit_upstream_side_effect_modifier_definition(modifier) + .visit_outward_side_effect_modifier_definition(modifier) .map_err(|_| { - super::Error::UpstreamSideEffectModifierDefinitionVisitError + super::Error::OutwardSideEffectModifierDefinitionVisitError })?; } } @@ -358,7 +358,7 @@ impl StandardInvestigator { visitor: &mut T, ) -> super::Result<()> where - T: StandardInvestigatorVisitor, + T: CallGraphVisitor, { let node = context .nodes diff --git a/aderyn_core/src/context/graph/callgraph_tests.rs b/aderyn_core/src/context/graph/callgraph_tests.rs new file mode 100644 index 000000000..49be47ea3 --- /dev/null +++ b/aderyn_core/src/context/graph/callgraph_tests.rs @@ -0,0 +1,272 @@ +#![allow(clippy::collapsible_match)] + +#[cfg(test)] +mod callgraph_tests { + use crate::{ + ast::{FunctionDefinition, ModifierDefinition}, + context::{ + graph::{callgraph::CallGraph, traits::CallGraphVisitor}, + workspace_context::{ASTNode, WorkspaceContext}, + }, + }; + + use crate::context::graph::callgraph::CallGraphDirection::{BothWays, Inward, Outward}; + use serial_test::serial; + + 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] + #[serial] + fn test_callgraph_is_not_none() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + assert!(context.inward_callgraph.is_some()); + assert!(context.outward_callgraph.is_some()); + } + + #[test] + #[serial] + fn test_tower1_modifier_has_no_inward() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let visit_eighth_floor1 = get_function_by_name(&context, "visitEighthFloor1"); + + let callgraph = CallGraph::new(&context, &[&visit_eighth_floor1], Inward).unwrap(); + + let mut tracker = Tracker::new(&context); + callgraph.accept(&context, &mut tracker).unwrap(); + + assert!(tracker.inward_func_definitions_names.is_empty()); + assert!(tracker.inward_modifier_definitions_names.is_empty()); + } + + #[test] + #[serial] + fn test_tower1_modifier_has_outward() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let visit_eighth_floor1 = get_function_by_name(&context, "visitEighthFloor1"); + + let callgraph = CallGraph::new(&context, &[&visit_eighth_floor1], Outward).unwrap(); + + let mut tracker = Tracker::new(&context); + callgraph.accept(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_outward_modifiers_with_names(&["passThroughNinthFloor1"])); + assert!(tracker.has_found_outward_functions_with_names(&["enterTenthFloor1"])); + } + + #[test] + #[serial] + fn test_tower2_modifier_has_both_outward_and_inward() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let pass_through_ninth_floor2 = + get_modifier_definition_by_name(&context, "passThroughNinthFloor2"); + + let callgraph = CallGraph::new(&context, &[&pass_through_ninth_floor2], BothWays).unwrap(); + + let mut tracker = Tracker::new(&context); + callgraph.accept(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_inward_functions_with_names(&["visitEighthFloor2"])); + assert!(tracker.has_found_outward_functions_with_names(&["enterTenthFloor2"])); + } + + #[test] + #[serial] + fn test_tower3_modifier_has_both_outward_and_inward() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let pass_through_ninth_floor3 = + get_modifier_definition_by_name(&context, "passThroughNinthFloor3"); + + let callgraph = CallGraph::new(&context, &[&pass_through_ninth_floor3], BothWays).unwrap(); + + let mut tracker = Tracker::new(&context); + callgraph.accept(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_outward_functions_with_names(&["enterTenthFloor3"])); + assert!(tracker.has_found_inward_functions_with_names(&["visitEighthFloor3"])); + assert!(tracker.has_not_found_any_outward_functions_with_name("visitSeventhFloor3")); + assert!(tracker.has_found_outward_side_effect_functions_with_name(&["visitSeventhFloor3"])); + } + + #[test] + #[serial] + fn test_tower3_functions_has_outward() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let visit_eighth_floor3 = get_function_by_name(&context, "visitSeventhFloor3"); + + let callgraph = CallGraph::new(&context, &[&visit_eighth_floor3], Outward).unwrap(); + + let mut tracker = Tracker::new(&context); + callgraph.accept(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_outward_functions_with_names(&["enterTenthFloor3"])); + } + + #[test] + #[serial] + fn test_tower4_functions_has_outward_and_inward() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/CallGraphTests.sol", + ); + + let recurse = get_function_by_name(&context, "recurse"); + + let callgraph = CallGraph::new(&context, &[&recurse], BothWays).unwrap(); + + let mut tracker = Tracker::new(&context); + callgraph.accept(&context, &mut tracker).unwrap(); + + assert!(tracker.has_found_outward_functions_with_names(&["recurse"])); + assert!(tracker.has_found_inward_functions_with_names(&["recurse"])); + } + + struct Tracker<'a> { + context: &'a WorkspaceContext, + entry_points: Vec<(String, usize, String)>, + inward_func_definitions_names: Vec, + outward_func_definitions_names: Vec, + inward_modifier_definitions_names: Vec, + outward_modifier_definitions_names: Vec, + outward_side_effects_func_definitions_names: Vec, + outward_side_effects_modifier_definitions_names: Vec, + } + + impl<'a> Tracker<'a> { + fn new(context: &WorkspaceContext) -> Tracker { + Tracker { + context, + entry_points: vec![], + inward_func_definitions_names: vec![], + inward_modifier_definitions_names: vec![], + outward_func_definitions_names: vec![], + outward_modifier_definitions_names: vec![], + outward_side_effects_func_definitions_names: vec![], + outward_side_effects_modifier_definitions_names: vec![], + } + } + + // inward functions + fn has_found_inward_functions_with_names(&self, name: &[&str]) -> bool { + name.iter() + .all(|&n| self.inward_func_definitions_names.contains(&n.to_string())) + } + + // outward functions + fn has_found_outward_functions_with_names(&self, name: &[&str]) -> bool { + name.iter() + .all(|&n| self.outward_func_definitions_names.contains(&n.to_string())) + } + + fn has_not_found_any_outward_functions_with_name(&self, name: &str) -> bool { + !self + .outward_func_definitions_names + .contains(&name.to_string()) + } + + // outward modifiers + fn has_found_outward_modifiers_with_names(&self, name: &[&str]) -> bool { + name.iter().all(|&n| { + self.outward_modifier_definitions_names + .contains(&n.to_string()) + }) + } + + // outward side effects + fn has_found_outward_side_effect_functions_with_name(&self, name: &[&str]) -> bool { + name.iter().all(|&n| { + self.outward_side_effects_func_definitions_names + .contains(&n.to_string()) + }) + } + } + + impl CallGraphVisitor 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_inward_function_definition( + &mut self, + node: &crate::ast::FunctionDefinition, + ) -> eyre::Result<()> { + self.inward_func_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_inward_modifier_definition( + &mut self, + node: &crate::ast::ModifierDefinition, + ) -> eyre::Result<()> { + self.inward_modifier_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_outward_function_definition( + &mut self, + node: &crate::ast::FunctionDefinition, + ) -> eyre::Result<()> { + self.outward_func_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_outward_modifier_definition( + &mut self, + node: &crate::ast::ModifierDefinition, + ) -> eyre::Result<()> { + self.outward_modifier_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_outward_side_effect_function_definition( + &mut self, + node: &FunctionDefinition, + ) -> eyre::Result<()> { + self.outward_side_effects_func_definitions_names + .push(node.name.to_string()); + Ok(()) + } + fn visit_outward_side_effect_modifier_definition( + &mut self, + node: &ModifierDefinition, + ) -> eyre::Result<()> { + self.outward_side_effects_modifier_definitions_names + .push(node.name.to_string()); + Ok(()) + } + } +} diff --git a/aderyn_core/src/context/graph/mod.rs b/aderyn_core/src/context/graph/mod.rs index 3d688c447..e6ca14e65 100644 --- a/aderyn_core/src/context/graph/mod.rs +++ b/aderyn_core/src/context/graph/mod.rs @@ -1,10 +1,16 @@ -pub mod traits; +mod callgraph; +mod callgraph_tests; +mod traits; mod workspace_callgraph; +pub use callgraph::*; +pub use traits::*; pub use workspace_callgraph::*; use derive_more::From; +use crate::ast::{ASTNode, NodeID}; + pub type Result = core::result::Result; #[derive(Debug, From)] @@ -14,6 +20,17 @@ pub enum Error { // region: -- standard::* errors WorkspaceCallGraphDFSError, + InwardCallgraphNotAvailable, + OutwardCallgraphNotAvailable, + UnidentifiedEntryPointNode(ASTNode), + InvalidEntryPointId(NodeID), + EntryPointVisitError, + OutwardFunctionDefinitionVisitError, + OutwardModifierDefinitionVisitError, + InwardFunctionDefinitionVisitError, + InwardModifierDefinitionVisitError, + OutwardSideEffectFunctionDefinitionVisitError, + OutwardSideEffectModifierDefinitionVisitError, // endregion } diff --git a/aderyn_core/src/context/graph/traits.rs b/aderyn_core/src/context/graph/traits.rs index 661bbf571..f6476f49b 100644 --- a/aderyn_core/src/context/graph/traits.rs +++ b/aderyn_core/src/context/graph/traits.rs @@ -1,4 +1,62 @@ +use crate::ast::{ASTNode, FunctionDefinition, ModifierDefinition}; + /// Trait to support reversing of callgraph. (Because, direct impl is not allowed on Foreign Types) pub trait Transpose { fn reverse(&self) -> Self; } + +/// Use with [`super::CallGraph`] +pub trait CallGraphVisitor { + /// 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 + /// inward 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::inward_callgraph`] + fn visit_inward_function_definition(&mut self, node: &FunctionDefinition) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Meant to be invoked while traversing [`crate::context::workspace_context::WorkspaceContext::outward_callgraph`] + fn visit_outward_function_definition(&mut self, node: &FunctionDefinition) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Meant to be invoked while traversing [`crate::context::workspace_context::WorkspaceContext::inward_callgraph`] + fn visit_inward_modifier_definition(&mut self, node: &ModifierDefinition) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Meant to be invoked while traversing [`crate::context::workspace_context::WorkspaceContext::outward_callgraph`] + fn visit_outward_modifier_definition(&mut self, node: &ModifierDefinition) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Read as "outward's inward-side-effect" function definition + /// These are function definitions that are inward from the outward nodes + /// but are themselves neither outward nor inward to the entry points + fn visit_outward_side_effect_function_definition( + &mut self, + node: &FunctionDefinition, + ) -> eyre::Result<()> { + self.visit_any(&(node.into())) + } + + /// Read as "outward's inward-side-effect" modifier definition + /// These are modifier definitions that are inward from the outward nodes + /// but are themselves neither outward nor inward to the entry points + fn visit_outward_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/graph/workspace_callgraph.rs b/aderyn_core/src/context/graph/workspace_callgraph.rs index 745175474..95f12fe51 100644 --- a/aderyn_core/src/context/graph/workspace_callgraph.rs +++ b/aderyn_core/src/context/graph/workspace_callgraph.rs @@ -12,18 +12,18 @@ use super::traits::Transpose; #[derive(Debug)] pub struct WorkspaceCallGraph { - pub graph: CallGraph, + pub raw_callgraph: RawCallGraph, } /** -* Every NodeID in CallGraph should corresponds to [`crate::ast::FunctionDefinition`] or [`crate::ast::ModifierDefinition`] +* Every NodeID in RawCallGraph should corresponds to [`crate::ast::FunctionDefinition`] or [`crate::ast::ModifierDefinition`] */ -pub type CallGraph = HashMap>; +pub type RawCallGraph = 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 raw_callgraph: RawCallGraph = HashMap::new(); let mut visited: HashSet = HashSet::new(); let funcs = context @@ -35,16 +35,16 @@ impl WorkspaceCallGraph { let modifier_definitions = context.modifier_definitions(); for func in funcs { - dfs_to_create_graph(func.id, &mut graph, &mut visited, context) + dfs_to_create_graph(func.id, &mut raw_callgraph, &mut visited, context) .map_err(|_| super::Error::WorkspaceCallGraphDFSError)?; } for modifier in modifier_definitions { - dfs_to_create_graph(modifier.id, &mut graph, &mut visited, context) + dfs_to_create_graph(modifier.id, &mut raw_callgraph, &mut visited, context) .map_err(|_| super::Error::WorkspaceCallGraphDFSError)?; } - Ok(WorkspaceCallGraph { graph }) + Ok(WorkspaceCallGraph { raw_callgraph }) } } @@ -52,7 +52,7 @@ impl WorkspaceCallGraph { /// with their connected counterparts. fn dfs_to_create_graph( id: NodeID, - graph: &mut CallGraph, + raw_callgraph: &mut RawCallGraph, visited: &mut HashSet, context: &WorkspaceContext, ) -> super::Result<()> { @@ -76,8 +76,8 @@ fn dfs_to_create_graph( 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)?; + create_connection_if_not_exsits(id, referenced_function_id, raw_callgraph); + dfs_to_create_graph(referenced_function_id, raw_callgraph, visited, context)?; } } } @@ -88,14 +88,28 @@ fn dfs_to_create_graph( 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)?; + create_connection_if_not_exsits(id, reference_modifier_id, raw_callgraph); + dfs_to_create_graph( + reference_modifier_id, + raw_callgraph, + 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)?; + create_connection_if_not_exsits( + id, + referenced_modifier_id as i64, + raw_callgraph, + ); + dfs_to_create_graph( + referenced_modifier_id as i64, + raw_callgraph, + visited, + context, + )?; } } } @@ -107,8 +121,12 @@ fn dfs_to_create_graph( Ok(()) } -fn create_connection_if_not_exsits(from_id: NodeID, to_id: NodeID, graph: &mut CallGraph) { - match graph.entry(from_id) { +fn create_connection_if_not_exsits( + from_id: NodeID, + to_id: NodeID, + raw_callgraph: &mut RawCallGraph, +) { + match raw_callgraph.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) { @@ -121,9 +139,9 @@ fn create_connection_if_not_exsits(from_id: NodeID, to_id: NodeID, graph: &mut C } } -impl Transpose for CallGraph { +impl Transpose for RawCallGraph { fn reverse(&self) -> Self { - let mut reversed_callgraph = CallGraph::default(); + let mut reversed_callgraph = RawCallGraph::default(); for (from_id, tos) in self { for to_id in tos { create_connection_if_not_exsits(*to_id, *from_id, &mut reversed_callgraph); diff --git a/aderyn_core/src/context/investigator/callgraph_tests.rs b/aderyn_core/src/context/investigator/callgraph_tests.rs deleted file mode 100644 index 3f5c1b268..000000000 --- a/aderyn_core/src/context/investigator/callgraph_tests.rs +++ /dev/null @@ -1,283 +0,0 @@ -#![allow(clippy::collapsible_match)] - -#[cfg(test)] -mod callgraph_tests { - use crate::{ - ast::{FunctionDefinition, ModifierDefinition}, - context::{ - investigator::{ - StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, - }, - workspace_context::{ASTNode, WorkspaceContext}, - }, - }; - - use serial_test::serial; - 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] - #[serial] - fn test_callgraph_is_not_none() { - let context = crate::detect::test_utils::load_solidity_source_unit( - "../tests/contract-playground/src/CallGraphTests.sol", - ); - assert!(context.forward_callgraph.is_some()); - assert!(context.reverse_callgraph.is_some()); - } - - #[test] - #[serial] - fn test_tower1_modifier_has_no_downstream() { - let context = crate::detect::test_utils::load_solidity_source_unit( - "../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] - #[serial] - fn test_tower1_modifier_has_upstream() { - let context = crate::detect::test_utils::load_solidity_source_unit( - "../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] - #[serial] - fn test_tower2_modifier_has_both_upstream_and_downstream() { - let context = crate::detect::test_utils::load_solidity_source_unit( - "../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] - #[serial] - fn test_tower3_modifier_has_both_upstream_and_downstream() { - let context = crate::detect::test_utils::load_solidity_source_unit( - "../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] - #[serial] - fn test_tower3_functions_has_upstream() { - let context = crate::detect::test_utils::load_solidity_source_unit( - "../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] - #[serial] - fn test_tower4_functions_has_upstream_and_downstream() { - let context = crate::detect::test_utils::load_solidity_source_unit( - "../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 deleted file mode 100644 index 79f660156..000000000 --- a/aderyn_core/src/context/investigator/mod.rs +++ /dev/null @@ -1,46 +0,0 @@ -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/traits.rs b/aderyn_core/src/context/investigator/traits.rs deleted file mode 100644 index c0c9a9b6d..000000000 --- a/aderyn_core/src/context/investigator/traits.rs +++ /dev/null @@ -1,78 +0,0 @@ -//! 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 ca73f66c9..6ff7b956c 100644 --- a/aderyn_core/src/context/mod.rs +++ b/aderyn_core/src/context/mod.rs @@ -1,7 +1,6 @@ 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 5cd2cdb47..df569c3bd 100644 --- a/aderyn_core/src/context/workspace_context.rs +++ b/aderyn_core/src/context/workspace_context.rs @@ -28,8 +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 inward_callgraph: Option, + pub outward_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 c10ff65db..b79ae2cb5 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -81,11 +81,13 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), Box::::default(), Box::::default(), Box::::default(), Box::::default(), + Box::::default(), ] } @@ -98,6 +100,8 @@ pub fn get_all_detectors_names() -> Vec { #[strum(serialize_all = "kebab-case")] pub(crate) enum IssueDetectorNamePool { CacheArrayLength, + BuiltinSymbolShadow, + IncorrectERC721Interface, FunctionInitializingState, DelegateCallInLoop, CentralizationRisk, @@ -177,6 +181,12 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option Some(Box::::default()), + IssueDetectorNamePool::BuiltinSymbolShadow => { + Some(Box::::default()) + } + IssueDetectorNamePool::IncorrectERC721Interface => { + Some(Box::::default()) + } IssueDetectorNamePool::OutOfOrderRetryable => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/high/contract_locks_ether.rs b/aderyn_core/src/detect/high/contract_locks_ether.rs index 808e0df02..dbf2eef0c 100644 --- a/aderyn_core/src/detect/high/contract_locks_ether.rs +++ b/aderyn_core/src/detect/high/contract_locks_ether.rs @@ -66,7 +66,8 @@ mod contract_eth_helper { use crate::{ ast::{ASTNode, ContractDefinition, StateMutability, Visibility}, context::{ - browser::ExtractFunctionDefinitions, investigator::*, + browser::ExtractFunctionDefinitions, + graph::{CallGraph, CallGraphDirection, CallGraphVisitor}, workspace_context::WorkspaceContext, }, detect::helpers, @@ -111,14 +112,14 @@ mod contract_eth_helper { let mut tracker = EthWithdrawalAllowerTracker::default(); - let investigator = StandardInvestigator::new( + let callgraph = CallGraph::new( context, funcs.iter().collect::>().as_slice(), - StandardInvestigationStyle::Downstream, + CallGraphDirection::Inward, ) .ok()?; - investigator.investigate(context, &mut tracker).ok()?; + callgraph.accept(context, &mut tracker).ok()?; if tracker.has_calls_that_sends_native_eth { return Some(true); @@ -137,7 +138,7 @@ mod contract_eth_helper { has_calls_that_sends_native_eth: bool, } - impl StandardInvestigatorVisitor for EthWithdrawalAllowerTracker { + impl CallGraphVisitor for EthWithdrawalAllowerTracker { fn visit_any(&mut self, ast_node: &ASTNode) -> eyre::Result<()> { if !self.has_calls_that_sends_native_eth && helpers::has_calls_that_sends_native_eth(ast_node) diff --git a/aderyn_core/src/detect/high/delegate_call_no_address_check.rs b/aderyn_core/src/detect/high/delegate_call_no_address_check.rs index 19a698cc0..77d9ef8f7 100644 --- a/aderyn_core/src/detect/high/delegate_call_no_address_check.rs +++ b/aderyn_core/src/detect/high/delegate_call_no_address_check.rs @@ -4,9 +4,8 @@ use std::error::Error; use crate::ast::NodeID; use crate::capture; -use crate::context::investigator::{ - StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, -}; + +use crate::context::graph::{CallGraph, CallGraphDirection, CallGraphVisitor}; use crate::detect::detector::IssueDetectorNamePool; use crate::detect::helpers; use crate::{ @@ -30,12 +29,8 @@ impl IssueDetector for DelegateCallOnUncheckedAddressDetector { has_delegate_call_on_non_state_variable_address: false, context, }; - let investigator = StandardInvestigator::new( - context, - &[&(func.into())], - StandardInvestigationStyle::Downstream, - )?; - investigator.investigate(context, &mut tracker)?; + let callgraph = CallGraph::new(context, &[&(func.into())], CallGraphDirection::Inward)?; + callgraph.accept(context, &mut tracker)?; if tracker.has_delegate_call_on_non_state_variable_address && !tracker.has_address_checks @@ -74,7 +69,7 @@ struct DelegateCallNoAddressChecksTracker<'a> { context: &'a WorkspaceContext, } -impl<'a> StandardInvestigatorVisitor for DelegateCallNoAddressChecksTracker<'a> { +impl<'a> CallGraphVisitor 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; diff --git a/aderyn_core/src/detect/high/incorrect_erc20_interface.rs b/aderyn_core/src/detect/high/incorrect_erc20_interface.rs index 0919f59ae..abdad7151 100644 --- a/aderyn_core/src/detect/high/incorrect_erc20_interface.rs +++ b/aderyn_core/src/detect/high/incorrect_erc20_interface.rs @@ -197,29 +197,6 @@ mod erc_matching_function_signature_helper { satisifer.satisfies(self) } } - - // Helpers to match return types (bool & uint256) - impl FunctionDefinition { - pub fn returns_bool(&self) -> bool { - let params = &self.return_parameters.parameters; - params.len() == 1 - && params[0] - .type_descriptions - .type_string - .as_ref() - .is_some_and(|type_string| type_string == "bool") - } - - pub fn returns_uint256(&self) -> bool { - let params = &self.return_parameters.parameters; - params.len() == 1 - && params[0] - .type_descriptions - .type_string - .as_ref() - .is_some_and(|type_string| type_string == "uint256") - } - } } #[cfg(test)] diff --git a/aderyn_core/src/detect/high/incorrect_erc721_interface.rs b/aderyn_core/src/detect/high/incorrect_erc721_interface.rs new file mode 100644 index 000000000..39d20da7d --- /dev/null +++ b/aderyn_core/src/detect/high/incorrect_erc721_interface.rs @@ -0,0 +1,309 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::{ASTNode, NodeID, Visibility}; + +use crate::capture; +use crate::context::browser::ExtractFunctionDefinitions; +use crate::detect::detector::IssueDetectorNamePool; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct IncorrectERC721InterfaceDetector { + // 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 IncorrectERC721InterfaceDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + // Analyze each contract in context + for current_contract in context.contract_definitions() { + // Look through it's inheritance heirarchy to determine if it's an ERC721 + if let Some(contract_ids) = current_contract.linearized_base_contracts.as_ref() { + let current_contract_is_erc721 = contract_ids.iter().any(|i| { + context.nodes.get(i).is_some_and(|c| { + if let ASTNode::ContractDefinition(contract) = c { + if contract.name.contains("ERC721") { + return true; + } + } + false + }) + }); + + if !current_contract_is_erc721 { + continue; + } + + // Now we know that current contract is an ERC721 + + for contract_id in contract_ids { + if let Some(ASTNode::ContractDefinition(contract)) = + context.nodes.get(contract_id) + { + let functions = ExtractFunctionDefinitions::from(contract).extracted; + + for func in functions { + if (func.visibility != Visibility::Public + && func.visibility != Visibility::External) + || !func.implemented + { + continue; + } + + if func.represents_erc721_balance_of() && !func.returns_uint256() { + capture!(self, context, func); + } + + if (func.represents_erc721_get_approved() + || func.represents_erc721_owner_of()) + && !func.returns_address() + { + capture!(self, context, func); + } + + if (func.represents_erc721_safe_transfer_from() + || func.represents_erc721_transfer_from()) + && !func.returns_nothing() + { + capture!(self, context, func); + } + + if (func.represents_erc721_approve() + || func.represents_erc721_set_approval_for_all()) + && !func.returns_nothing() + { + capture!(self, context, func); + } + + if func.represents_erc721_is_approved_for_all() && !func.returns_bool() + { + capture!(self, context, func); + } + } + } + } + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Incorrect ERC721 interface.") + } + + fn description(&self) -> String { + String::from("Incorrect return values for ERC721 functions. A contract compiled with Solidity > 0.4.22 \ + interacting with these functions will fail to execute them, as the return value is missing. Set the \ + appropriate return values and types for the defined ERC721 functions." + ) + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + format!("{}", IssueDetectorNamePool::IncorrectERC721Interface) + } +} + +mod erc721_matching_function_signature_helper { + //! This module matches function signature only (name + parameters) + //! This means, that the return value could be different. + + use crate::ast::FunctionDefinition; + + struct SignatureMatcher<'a> { + name: &'a str, + paramter_types: Vec<&'a str>, + } + + // Helps with checking if a function definition satisifed a signature matcher + impl<'a> SignatureMatcher<'a> { + fn satisfies(&self, func: &FunctionDefinition) -> bool { + if func.name != self.name { + return false; + } + let params = &func.parameters.parameters; + if params.len() != self.paramter_types.len() { + return false; + } + #[allow(clippy::needless_range_loop)] + for idx in 0..params.len() { + if let Some(func_param_type) = params[idx].type_descriptions.type_string.as_ref() { + let target = &self.paramter_types[idx]; + if *target == "address" { + if func_param_type == "address" || func_param_type == "address payable" { + continue; + } else { + return false; + } + } else if func_param_type != target { + return false; + } + } else { + return false; + } + } + true + } + } + + // ERC721 function signature matching + impl FunctionDefinition { + pub fn represents_erc721_get_approved(&self) -> bool { + let satisifer = SignatureMatcher { + name: "getApproved", + paramter_types: vec!["uint256"], + }; + satisifer.satisfies(self) + } + + pub fn represents_erc721_transfer_from(&self) -> bool { + let satisifer = SignatureMatcher { + name: "transferFrom", + paramter_types: vec!["address", "address", "uint256"], + }; + satisifer.satisfies(self) + } + + pub fn represents_erc721_approve(&self) -> bool { + let satisifer = SignatureMatcher { + name: "approve", + paramter_types: vec!["address", "uint256"], + }; + satisifer.satisfies(self) + } + + pub fn represents_erc721_is_approved_for_all(&self) -> bool { + let satisifer = SignatureMatcher { + name: "isApprovedForAll", + paramter_types: vec!["address", "address"], + }; + satisifer.satisfies(self) + } + + pub fn represents_erc721_balance_of(&self) -> bool { + let satisifer = SignatureMatcher { + name: "balanceOf", + paramter_types: vec!["address"], + }; + satisifer.satisfies(self) + } + + pub fn represents_erc721_owner_of(&self) -> bool { + let satisifer = SignatureMatcher { + name: "ownerOf", + paramter_types: vec!["uint256"], + }; + satisifer.satisfies(self) + } + + pub fn represents_erc721_safe_transfer_from(&self) -> bool { + let type_1_staisfier = SignatureMatcher { + name: "safeTransferFrom", + paramter_types: vec!["address", "address", "uint256", "bytes"], + }; + + let type_2_satisifer = SignatureMatcher { + name: "safeTransferFrom", + paramter_types: vec!["address", "address", "uint256"], + }; + + type_1_staisfier.satisfies(self) || type_2_satisifer.satisfies(self) + } + + pub fn represents_erc721_set_approval_for_all(&self) -> bool { + let satisfier = SignatureMatcher { + name: "setApprovalForAll", + paramter_types: vec!["address", "bool"], + }; + satisfier.satisfies(self) + } + } + + // Helpers to match return types (bool & uint256) + impl FunctionDefinition { + pub fn returns_nothing(&self) -> bool { + self.return_parameters.parameters.is_empty() + } + + pub fn returns_bool(&self) -> bool { + let params = &self.return_parameters.parameters; + params.len() == 1 + && params[0] + .type_descriptions + .type_string + .as_ref() + .is_some_and(|type_string| type_string == "bool") + } + + pub fn returns_uint256(&self) -> bool { + let params = &self.return_parameters.parameters; + params.len() == 1 + && params[0] + .type_descriptions + .type_string + .as_ref() + .is_some_and(|type_string| type_string == "uint256") + } + + pub fn returns_address(&self) -> bool { + let params = &self.return_parameters.parameters; + params.len() == 1 + && params[0] + .type_descriptions + .type_string + .as_ref() + .is_some_and(|type_string| { + type_string == "address" || type_string == "address payable" + }) + } + } +} + +#[cfg(test)] +mod incorrect_erc721_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, high::incorrect_erc721_interface::IncorrectERC721InterfaceDetector, + }; + + #[test] + #[serial] + fn test_incorrect_erc721_functions() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/IncorrectERC721.sol", + ); + + let mut detector = IncorrectERC721InterfaceDetector::default(); + let found = detector.detect(&context).unwrap(); + + // We capture every faulty method in the IncorrectERC721 contract that has the wrong return type + println!("{:#?}", detector.instances()); + + // assert that the detector found an issue + assert!(found); + // assert that the detector found the correct number of instances + + assert_eq!(detector.instances().len(), 8); + + // assert the severity is high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + } +} diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index b19236ac4..27f9cfd86 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -12,6 +12,7 @@ pub(crate) mod enumerable_loop_removal; pub(crate) mod experimental_encoder; pub(crate) mod incorrect_caret_operator; pub(crate) mod incorrect_erc20_interface; +pub(crate) mod incorrect_erc721_interface; pub(crate) mod incorrect_shift_order; pub(crate) mod misused_boolean; pub(crate) mod msg_value_in_loops; @@ -51,6 +52,7 @@ pub use enumerable_loop_removal::EnumerableLoopRemovalDetector; pub use experimental_encoder::ExperimentalEncoderDetector; pub use incorrect_caret_operator::IncorrectUseOfCaretOperatorDetector; pub use incorrect_erc20_interface::IncorrectERC20InterfaceDetector; +pub use incorrect_erc721_interface::IncorrectERC721InterfaceDetector; pub use incorrect_shift_order::IncorrectShiftOrderDetector; pub use misused_boolean::MisusedBooleanDetector; pub use msg_value_in_loops::MsgValueUsedInLoopDetector; diff --git a/aderyn_core/src/detect/high/msg_value_in_loops.rs b/aderyn_core/src/detect/high/msg_value_in_loops.rs index b95ed64db..d8644961a 100644 --- a/aderyn_core/src/detect/high/msg_value_in_loops.rs +++ b/aderyn_core/src/detect/high/msg_value_in_loops.rs @@ -6,9 +6,7 @@ use crate::ast::{ASTNode, Expression, NodeID}; use crate::capture; use crate::context::browser::ExtractMemberAccesses; -use crate::context::investigator::{ - StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, -}; +use crate::context::graph::{CallGraph, CallGraphDirection, CallGraphVisitor}; use crate::detect::detector::IssueDetectorNamePool; use crate::{ context::workspace_context::WorkspaceContext, @@ -72,11 +70,9 @@ impl IssueDetector for MsgValueUsedInLoopDetector { fn uses_msg_value(context: &WorkspaceContext, ast_node: &ASTNode) -> Option { let mut tracker = MsgValueTracker::default(); - let investigator = - StandardInvestigator::new(context, &[ast_node], StandardInvestigationStyle::Downstream) - .ok()?; + let callgraph = CallGraph::new(context, &[ast_node], CallGraphDirection::Inward).ok()?; - investigator.investigate(context, &mut tracker).ok()?; + callgraph.accept(context, &mut tracker).ok()?; Some(tracker.has_msg_value) } @@ -85,7 +81,7 @@ struct MsgValueTracker { has_msg_value: bool, } -impl StandardInvestigatorVisitor for MsgValueTracker { +impl CallGraphVisitor for MsgValueTracker { fn visit_any(&mut self, node: &crate::ast::ASTNode) -> eyre::Result<()> { if !self.has_msg_value && ExtractMemberAccesses::from(node) diff --git a/aderyn_core/src/detect/high/out_of_order_retryable.rs b/aderyn_core/src/detect/high/out_of_order_retryable.rs index a39102268..433a70cd1 100644 --- a/aderyn_core/src/detect/high/out_of_order_retryable.rs +++ b/aderyn_core/src/detect/high/out_of_order_retryable.rs @@ -5,9 +5,7 @@ use crate::ast::{Expression, MemberAccess, NodeID}; use crate::capture; use crate::context::browser::ExtractFunctionCalls; -use crate::context::investigator::{ - StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, -}; +use crate::context::graph::{CallGraph, CallGraphDirection, CallGraphVisitor}; use crate::detect::detector::IssueDetectorNamePool; use crate::detect::helpers; use crate::{ @@ -29,12 +27,8 @@ impl IssueDetector for OutOfOrderRetryableDetector { let mut tracker = OutOfOrderRetryableTracker { number_of_retry_calls: 0, }; - let investigator = StandardInvestigator::new( - context, - &[&(func.into())], - StandardInvestigationStyle::Downstream, - )?; - investigator.investigate(context, &mut tracker)?; + let callgraph = CallGraph::new(context, &[&(func.into())], CallGraphDirection::Inward)?; + callgraph.accept(context, &mut tracker)?; if tracker.number_of_retry_calls >= 2 { capture!(self, context, func); } @@ -77,7 +71,7 @@ const SEQUENCER_FUNCTIONS: [&str; 3] = [ "unsafeCreateRetryableTicket", ]; -impl StandardInvestigatorVisitor for OutOfOrderRetryableTracker { +impl CallGraphVisitor for OutOfOrderRetryableTracker { fn visit_any(&mut self, node: &crate::ast::ASTNode) -> eyre::Result<()> { if self.number_of_retry_calls >= 2 { return Ok(()); diff --git a/aderyn_core/src/detect/high/send_ether_no_checks.rs b/aderyn_core/src/detect/high/send_ether_no_checks.rs index 964c694a8..5358e7b1d 100644 --- a/aderyn_core/src/detect/high/send_ether_no_checks.rs +++ b/aderyn_core/src/detect/high/send_ether_no_checks.rs @@ -4,9 +4,7 @@ use std::error::Error; use crate::ast::NodeID; use crate::capture; -use crate::context::investigator::{ - StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, -}; +use crate::context::graph::{CallGraph, CallGraphDirection, CallGraphVisitor}; use crate::context::workspace_context::ASTNode; use crate::detect::detector::IssueDetectorNamePool; use crate::detect::helpers; @@ -27,12 +25,9 @@ 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)?; + let investigator = + CallGraph::new(context, &[&(func.into())], CallGraphDirection::Inward)?; + investigator.accept(context, &mut tracker)?; if tracker.sends_native_eth && !tracker.has_msg_sender_checks { capture!(self, context, func); @@ -108,7 +103,7 @@ pub struct MsgSenderAndCallWithValueTracker { pub sends_native_eth: bool, } -impl StandardInvestigatorVisitor for MsgSenderAndCallWithValueTracker { +impl CallGraphVisitor 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; diff --git a/aderyn_core/src/detect/high/tx_origin_used_for_auth.rs b/aderyn_core/src/detect/high/tx_origin_used_for_auth.rs index f733de663..11fd5ab99 100644 --- a/aderyn_core/src/detect/high/tx_origin_used_for_auth.rs +++ b/aderyn_core/src/detect/high/tx_origin_used_for_auth.rs @@ -5,9 +5,7 @@ use crate::ast::{ASTNode, Expression, Identifier, NodeID}; use crate::capture; use crate::context::browser::ExtractMemberAccesses; -use crate::context::investigator::{ - StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, -}; +use crate::context::graph::{CallGraph, CallGraphDirection, CallGraphVisitor}; use crate::detect::detector::IssueDetectorNamePool; use crate::{ context::workspace_context::WorkspaceContext, @@ -85,12 +83,8 @@ impl TxOriginUsedForAuthDetector { ) -> Result<(), Box> { // Boilerplate let mut tracker = MsgSenderAndTxOriginTracker::default(); - let investigator = StandardInvestigator::new( - context, - check_nodes, - StandardInvestigationStyle::Downstream, - )?; - investigator.investigate(context, &mut tracker)?; + let callgraph = CallGraph::new(context, check_nodes, CallGraphDirection::Inward)?; + callgraph.accept(context, &mut tracker)?; if tracker.satisifed() { capture!(self, context, capture_node); @@ -113,7 +107,7 @@ impl MsgSenderAndTxOriginTracker { } } -impl StandardInvestigatorVisitor for MsgSenderAndTxOriginTracker { +impl CallGraphVisitor for MsgSenderAndTxOriginTracker { fn visit_any(&mut self, node: &crate::ast::ASTNode) -> eyre::Result<()> { let member_accesses = ExtractMemberAccesses::from(node).extracted; diff --git a/aderyn_core/src/detect/low/builtin_symbol_shadowing.rs b/aderyn_core/src/detect/low/builtin_symbol_shadowing.rs new file mode 100644 index 000000000..75bd2e1ec --- /dev/null +++ b/aderyn_core/src/detect/low/builtin_symbol_shadowing.rs @@ -0,0 +1,166 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::NodeID; + +use crate::capture; +use crate::detect::detector::IssueDetectorNamePool; +use phf::phf_set; + +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct BuiltinSymbolShadowDetector { + // 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 BuiltinSymbolShadowDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + // Variable Declaration names + for variable_declaration in context.variable_declarations() { + if DENY_LIST.contains(&variable_declaration.name) { + capture!(self, context, variable_declaration); + } + } + + // Function Definition names + for function in context.function_definitions() { + if DENY_LIST.contains(&function.name) { + capture!(self, context, function); + } + } + + // Modifier definition names + for modifier in context.modifier_definitions() { + if DENY_LIST.contains(&modifier.name) { + capture!(self, context, modifier); + } + } + + // Event definition names + for event in context.event_definitions() { + if DENY_LIST.contains(&event.name) { + capture!(self, context, event); + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::Low + } + + fn title(&self) -> String { + String::from("Builtin Symbol Shadowing") + } + + fn description(&self) -> String { + String::from("Name clashes with a built-in-symbol. Consider renaming it.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + format!("{}", IssueDetectorNamePool::BuiltinSymbolShadow) + } +} + +// Copied from SLITHER +static DENY_LIST: phf::Set<&'static str> = phf_set! { + "assert", + "require", + "revert", + "block", + "blockhash", + "gasleft", + "msg", + "now", + "tx", + "this", + "addmod", + "mulmod", + "keccak256", + "sha256", + "sha3", + "ripemd160", + "ecrecover", + "selfdestruct", + "suicide", + "abi", + "fallback", + "receive", + "abstract", + "after", + "alias", + "apply", + "auto", + "case", + "catch", + "copyof", + "default", + "define", + "final", + "immutable", + "implements", + "in", + "inline", + "let", + "macro", + "match", + "mutable", + "null", + "of", + "override", + "partial", + "promise", + "reference", + "relocatable", + "sealed", + "sizeof", + "static", + "supports", + "switch", + "try", + "type", + "typedef", + "typeof", + "unchecked", +}; + +#[cfg(test)] +mod builtin_symbol_shadowing_tests { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, low::builtin_symbol_shadowing::BuiltinSymbolShadowDetector, + }; + + #[test] + #[serial] + fn test_builtin_symbol_shadow() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/BuiltinSymbolShadow.sol", + ); + + let mut detector = BuiltinSymbolShadowDetector::default(); + let found = detector.detect(&context).unwrap(); + // assert that the detector found an issue + assert!(found); + // assert that the detector found the correct number of instances + assert_eq!(detector.instances().len(), 4); + // assert the severity is low + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::Low + ); + } +} diff --git a/aderyn_core/src/detect/low/cache_array_length.rs b/aderyn_core/src/detect/low/cache_array_length.rs index 46f8a2e73..027138ce0 100644 --- a/aderyn_core/src/detect/low/cache_array_length.rs +++ b/aderyn_core/src/detect/low/cache_array_length.rs @@ -98,9 +98,7 @@ mod loop_investigation_helper { ast::{ASTNode, Expression, ForStatement, Identifier, NodeID, TypeDescriptions}, context::{ browser::{ApproximateStorageChangeFinder, ExtractMemberAccesses}, - investigator::{ - StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, - }, + graph::{CallGraph, CallGraphDirection, CallGraphVisitor}, workspace_context::WorkspaceContext, }, }; @@ -155,14 +153,10 @@ mod loop_investigation_helper { context, }; - let investigator = StandardInvestigator::new( - context, - &[&(self.into())], - StandardInvestigationStyle::Downstream, - ) - .ok()?; + let investigator = + CallGraph::new(context, &[&(self.into())], CallGraphDirection::Inward).ok()?; - investigator.investigate(context, &mut tracker).ok()?; + investigator.accept(context, &mut tracker).ok()?; tracker.changes.take() } @@ -173,7 +167,7 @@ mod loop_investigation_helper { changes: Option>, } - impl<'a> StandardInvestigatorVisitor for StateVariableChangeTracker<'a> { + impl<'a> CallGraphVisitor for StateVariableChangeTracker<'a> { fn visit_any(&mut self, node: &ASTNode) -> eyre::Result<()> { let changes = ApproximateStorageChangeFinder::from(self.context, node); if self.changes.is_none() { diff --git a/aderyn_core/src/detect/low/constant_funcs_assembly.rs b/aderyn_core/src/detect/low/constant_funcs_assembly.rs index 3608ba2bb..a2a9aefa5 100644 --- a/aderyn_core/src/detect/low/constant_funcs_assembly.rs +++ b/aderyn_core/src/detect/low/constant_funcs_assembly.rs @@ -8,9 +8,8 @@ use crate::capture; use crate::context::browser::{ ExtractInlineAssemblys, ExtractPragmaDirectives, GetClosestAncestorOfTypeX, }; -use crate::context::investigator::{ - StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, -}; + +use crate::context::graph::{CallGraph, CallGraphDirection, CallGraphVisitor}; use crate::detect::detector::IssueDetectorNamePool; use crate::detect::helpers::{self, pragma_directive_to_semver}; use crate::{ @@ -50,12 +49,12 @@ impl IssueDetector for ConstantFunctionContainsAssemblyDetector { let mut tracker = AssemblyTracker { has_assembly: false, }; - let investigator = StandardInvestigator::new( + let callgraph = CallGraph::new( context, &[&(function.into())], - StandardInvestigationStyle::Downstream, + CallGraphDirection::Inward, )?; - investigator.investigate(context, &mut tracker)?; + callgraph.accept(context, &mut tracker)?; if tracker.has_assembly { capture!(self, context, function); @@ -110,7 +109,7 @@ struct AssemblyTracker { has_assembly: bool, } -impl StandardInvestigatorVisitor for AssemblyTracker { +impl CallGraphVisitor for AssemblyTracker { fn visit_any(&mut self, node: &crate::ast::ASTNode) -> eyre::Result<()> { // If we are already satisifed, do not bother checking if self.has_assembly { diff --git a/aderyn_core/src/detect/low/function_init_state_vars.rs b/aderyn_core/src/detect/low/function_init_state_vars.rs index 67fc87189..f40dffa47 100644 --- a/aderyn_core/src/detect/low/function_init_state_vars.rs +++ b/aderyn_core/src/detect/low/function_init_state_vars.rs @@ -5,9 +5,7 @@ use crate::ast::{ASTNode, Expression, FunctionCall, Identifier, NodeID}; use crate::capture; use crate::context::browser::ExtractReferencedDeclarations; -use crate::context::investigator::{ - StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, -}; +use crate::context::graph::{CallGraph, CallGraphDirection, CallGraphVisitor}; use crate::detect::detector::IssueDetectorNamePool; use crate::{ context::workspace_context::WorkspaceContext, @@ -46,13 +44,10 @@ impl IssueDetector for FunctionInitializingStateDetector { let mut tracker = NonConstantStateVariableReferenceDeclarationTracker::new(context); - let investigator = StandardInvestigator::new( - context, - &[&(func.into())], - StandardInvestigationStyle::Downstream, - )?; + let investigator = + CallGraph::new(context, &[&(func.into())], CallGraphDirection::Inward)?; - investigator.investigate(context, &mut tracker)?; + investigator.accept(context, &mut tracker)?; if tracker.makes_a_reference { capture!(self, context, variable_declaration); @@ -102,7 +97,7 @@ impl<'a> NonConstantStateVariableReferenceDeclarationTracker<'a> { } } -impl<'a> StandardInvestigatorVisitor for NonConstantStateVariableReferenceDeclarationTracker<'a> { +impl<'a> CallGraphVisitor for NonConstantStateVariableReferenceDeclarationTracker<'a> { fn visit_any(&mut self, node: &ASTNode) -> eyre::Result<()> { // We already know the condition is satisifed if self.makes_a_reference { diff --git a/aderyn_core/src/detect/low/mod.rs b/aderyn_core/src/detect/low/mod.rs index 9be472abf..00edfb5fd 100644 --- a/aderyn_core/src/detect/low/mod.rs +++ b/aderyn_core/src/detect/low/mod.rs @@ -1,4 +1,5 @@ pub(crate) mod boolean_equality; +pub(crate) mod builtin_symbol_shadowing; pub(crate) mod cache_array_length; pub(crate) mod centralization_risk; pub(crate) mod constant_funcs_assembly; @@ -31,6 +32,7 @@ pub(crate) mod useless_public_function; pub(crate) mod zero_address_check; pub use boolean_equality::BooleanEqualityDetector; +pub use builtin_symbol_shadowing::BuiltinSymbolShadowDetector; pub use cache_array_length::CacheArrayLengthDetector; pub use centralization_risk::CentralizationRiskDetector; pub use constant_funcs_assembly::ConstantFunctionContainsAssemblyDetector; diff --git a/aderyn_core/src/detect/low/return_bomb.rs b/aderyn_core/src/detect/low/return_bomb.rs index aa32ca6eb..c49ef4c91 100644 --- a/aderyn_core/src/detect/low/return_bomb.rs +++ b/aderyn_core/src/detect/low/return_bomb.rs @@ -6,9 +6,7 @@ use crate::ast::{ASTNode, MemberAccess, NodeID}; use crate::ast::NodeType; use crate::capture; use crate::context::browser::GetClosestAncestorOfTypeX; -use crate::context::investigator::{ - StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor, -}; +use crate::context::graph::{CallGraph, CallGraphDirection, CallGraphVisitor}; use crate::detect::detector::IssueDetectorNamePool; use crate::detect::helpers; use crate::{ @@ -39,12 +37,8 @@ impl IssueDetector for ReturnBombDetector { calls_on_non_state_variable_addresses: vec![], // collection of all `address.call` Member Accesses where address is not a state variable context, }; - let investigator = StandardInvestigator::new( - context, - &[&(func.into())], - StandardInvestigationStyle::Downstream, - )?; - investigator.investigate(context, &mut tracker)?; + let callgraph = CallGraph::new(context, &[&(func.into())], CallGraphDirection::Inward)?; + callgraph.accept(context, &mut tracker)?; if !tracker.has_address_checks { // Now we assume that in this region all addresses are unprotected (because they are not involved in any binary ops/checks) @@ -125,7 +119,7 @@ struct CallNoAddressChecksTracker<'a> { context: &'a WorkspaceContext, } -impl<'a> StandardInvestigatorVisitor for CallNoAddressChecksTracker<'a> { +impl<'a> CallGraphVisitor for CallNoAddressChecksTracker<'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; 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 c52a15f29..62daf78da 100644 --- a/aderyn_core/src/detect/test_utils/load_source_unit.rs +++ b/aderyn_core/src/detect/test_utils/load_source_unit.rs @@ -7,7 +7,7 @@ use std::{ use crate::{ ast::SourceUnit, - context::{graph::traits::Transpose, workspace_context::WorkspaceContext}, + context::{graph::Transpose, workspace_context::WorkspaceContext}, }; use crate::{context::graph::WorkspaceCallGraph, visitor::ast_visitor::Node}; @@ -92,12 +92,12 @@ 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(), + let inward_callgraph = WorkspaceCallGraph::from_context(context).unwrap(); + let outward_callgraph = WorkspaceCallGraph { + raw_callgraph: inward_callgraph.raw_callgraph.reverse(), }; - context.forward_callgraph = Some(forward_callgraph); - context.reverse_callgraph = Some(reverse_callgraph); + context.inward_callgraph = Some(inward_callgraph); + context.outward_callgraph = Some(outward_callgraph); } fn absorb_ast_content_into_context( diff --git a/aderyn_driver/Cargo.toml b/aderyn_driver/Cargo.toml index 16be8294e..dd6669b01 100644 --- a/aderyn_driver/Cargo.toml +++ b/aderyn_driver/Cargo.toml @@ -17,6 +17,7 @@ serde = { version = "1.0.160", features = ["derive"] } serde_repr = "0.1.12" cyfrin-foundry-config = { version = "0.2.1" } toml = "0.8.13" +field_access = "0.1.8" [dev-dependencies] criterion = "0.5.1" diff --git a/aderyn_driver/src/driver.rs b/aderyn_driver/src/driver.rs index 3abef9b39..748eef99d 100644 --- a/aderyn_driver/src/driver.rs +++ b/aderyn_driver/src/driver.rs @@ -2,9 +2,11 @@ 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::{graph::WorkspaceCallGraph, workspace_context::WorkspaceContext}, + context::{ + graph::{Transpose, WorkspaceCallGraph}, + workspace_context::WorkspaceContext, + }, detect::detector::{get_all_issue_detectors, IssueDetector, IssueSeverity}, fscloc, report::{ @@ -13,9 +15,10 @@ use aderyn_core::{ }, run, }; +use field_access::FieldAccess; use std::{collections::HashMap, error::Error, path::PathBuf}; -#[derive(Clone)] +#[derive(Clone, FieldAccess)] pub struct Args { pub root: String, pub output: String, @@ -154,12 +157,12 @@ fn make_context(args: &Args) -> WorkspaceContextWrapper { 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(), + let inward_callgraph = WorkspaceCallGraph::from_context(context).unwrap(); + let outward_callgraph = WorkspaceCallGraph { + raw_callgraph: inward_callgraph.raw_callgraph.reverse(), }; - context.forward_callgraph = Some(forward_callgraph); - context.reverse_callgraph = Some(reverse_callgraph); + context.inward_callgraph = Some(inward_callgraph); + context.outward_callgraph = Some(outward_callgraph); } // Using the source path, calculate the sloc diff --git a/aderyn_py/Cargo.toml b/aderyn_py/Cargo.toml index f40308ac5..04ed51e0b 100644 --- a/aderyn_py/Cargo.toml +++ b/aderyn_py/Cargo.toml @@ -15,8 +15,9 @@ crate-type = ["cdylib"] [dependencies] aderyn_driver = { path = "../aderyn_driver", version = "0.1.10" } +field_access = "0.1.8" [dependencies.pyo3] -version = "0.19.0" +version = "0.22.2" # "abi3-py38" tells pyo3 (and maturin) to build using the stable ABI with minimum Python version 3.8 -features = ["abi3-py37"] \ No newline at end of file +features = ["abi3-py38"] \ No newline at end of file diff --git a/aderyn_py/requirements.txt b/aderyn_py/requirements.txt new file mode 100644 index 000000000..cb6f7b95b --- /dev/null +++ b/aderyn_py/requirements.txt @@ -0,0 +1,2 @@ +pytest==8.3.2 +maturin==1.7.0 \ No newline at end of file diff --git a/aderyn_py/src/lib.rs b/aderyn_py/src/lib.rs index 70a664733..f75501787 100644 --- a/aderyn_py/src/lib.rs +++ b/aderyn_py/src/lib.rs @@ -1,26 +1,43 @@ #![allow(unused)] use aderyn_driver::driver; +use field_access::{FieldAccess, FieldMut}; fn main() { use pyo3::prelude::*; + use pyo3::types::{PyBool, PyDict}; #[pyfunction] - fn generate_report(root: String, output: String) { - let args = driver::Args { + #[pyo3(signature = (root, output, **py_kwargs))] + fn generate_report(root: String, output: String, py_kwargs: Option<&Bound<'_, PyDict>>) { + let mut args = driver::Args { root, output, - src: None, // TODO support this later - no_snippets: false, // TODO support this later - skip_build: false, // TODO support this later - skip_cloc: false, // TODO support this later - path_includes: None, // TODO support this later - path_excludes: None, // TODO support this later - stdout: false, // TODO support this later - skip_update_check: false, // TODO support this later - auditor_mode: false, // TODO support this later - highs_only: false, // TODO support this later + src: None, + no_snippets: false, + skip_build: false, + skip_cloc: false, + path_includes: None, + path_excludes: None, + stdout: false, + skip_update_check: false, + auditor_mode: false, + highs_only: false, }; + + if let Some(kwargs) = py_kwargs { + kwargs.iter().for_each(|(py_key, py_value)| { + let rust_key: String = py_key.extract().unwrap(); + if py_value.is_instance_of::() { + let rust_value: bool = py_value.extract().unwrap(); + args.field_mut(&rust_key).unwrap().replace(rust_value); + } else { + let rust_value: Vec = py_value.extract().unwrap_or_default(); + args.field_mut(&rust_key).unwrap().replace(Some(rust_value)); + } + }) + } + driver::drive(args); } @@ -28,7 +45,7 @@ fn main() { /// the `lib.name` setting in the `Cargo.toml`, else Python will not be able to /// import the module. #[pymodule] - fn aderynpy(_py: Python, m: &PyModule) -> PyResult<()> { + fn aderynpy(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_function(wrap_pyfunction!(generate_report, m)?)?; Ok(()) diff --git a/aderyn_py/tests/test_generate_report.py b/aderyn_py/tests/test_generate_report.py new file mode 100644 index 000000000..8d932c9f6 --- /dev/null +++ b/aderyn_py/tests/test_generate_report.py @@ -0,0 +1,27 @@ +import subprocess +import pytest +from aderynpy import generate_report + +@pytest.mark.parametrize("root, report", [ + ("../tests/contract-playground", "../reports/report.md"), + ("../tests/2024-05-Sablier", "../reports/sablier-aderyn-toml-nested-root.md"), + ("../tests/adhoc-sol-files", "../reports/adhoc-sol-files-report.md"), + ("../tests/foundry-nft-f23", "../reports/nft-report.md"), + ("../tests/prb-math", "../reports/prb-math-report.md"), +]) +def test_generate_report(root, report): + # Define output file path + out_file = f"./{root.split('../')[-1]}-workflow.md" + + # Call the generate_report function + generate_report(root, out_file) + + # Run the diff command to compare the generated report with the original report + result = subprocess.run( + ["diff", str(out_file), report], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + + # Check if diff command found any differences (result.returncode == 0 means no differences) + assert result.returncode == 1 diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index 10b67692e..fe22ffccf 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -198,6 +198,7 @@ "tx-origin-used-for-auth", "msg-value-in-loop", "contract-locks-ether", + "incorrect-erc721-interface", "incorrect-erc20-interface", "out-of-order-retryable" ] diff --git a/reports/report.json b/reports/report.json index bd78c0024..beb73b342 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 86, - "total_sloc": 2933 + "total_source_units": 88, + "total_sloc": 3178 }, "files_details": { "files_details": [ @@ -25,6 +25,10 @@ "file_path": "src/BooleanEquality.sol", "n_sloc": 27 }, + { + "file_path": "src/BuiltinSymbolShadow.sol", + "n_sloc": 14 + }, { "file_path": "src/CacheArrayLength.sol", "n_sloc": 38 @@ -129,6 +133,10 @@ "file_path": "src/IncorrectERC20.sol", "n_sloc": 97 }, + { + "file_path": "src/IncorrectERC721.sol", + "n_sloc": 231 + }, { "file_path": "src/IncorrectShift.sol", "n_sloc": 17 @@ -352,8 +360,8 @@ ] }, "issue_count": { - "high": 38, - "low": 31 + "high": 39, + "low": 32 }, "high_issues": { "issues": [ @@ -1319,6 +1327,12 @@ "src": "97:1", "src_char": "97:1" }, + { + "contract_path": "src/BuiltinSymbolShadow.sol", + "line_no": 5, + "src": "92:8", + "src_char": "92:8" + }, { "contract_path": "src/ConstantFuncsAssembly.sol", "line_no": 6, @@ -1349,6 +1363,12 @@ "src": "355:7", "src_char": "355:7" }, + { + "contract_path": "src/IncorrectERC721.sol", + "line_no": 147, + "src": "4076:11", + "src_char": "4076:11" + }, { "contract_path": "src/PublicVariableReadInExternalContext.sol", "line_no": 6, @@ -2169,6 +2189,61 @@ } ] }, + { + "title": "Incorrect ERC721 interface.", + "description": "Incorrect return values for ERC721 functions. A contract compiled with Solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing. Set the appropriate return values and types for the defined ERC721 functions.", + "detector_name": "incorrect-erc721-interface", + "instances": [ + { + "contract_path": "src/IncorrectERC721.sol", + "line_no": 14, + "src": "433:9", + "src_char": "433:9" + }, + { + "contract_path": "src/IncorrectERC721.sol", + "line_no": 18, + "src": "551:7", + "src_char": "551:7" + }, + { + "contract_path": "src/IncorrectERC721.sol", + "line_no": 22, + "src": "696:7", + "src_char": "696:7" + }, + { + "contract_path": "src/IncorrectERC721.sol", + "line_no": 37, + "src": "1092:11", + "src_char": "1092:11" + }, + { + "contract_path": "src/IncorrectERC721.sol", + "line_no": 41, + "src": "1231:17", + "src_char": "1231:17" + }, + { + "contract_path": "src/IncorrectERC721.sol", + "line_no": 49, + "src": "1484:16", + "src_char": "1484:16" + }, + { + "contract_path": "src/IncorrectERC721.sol", + "line_no": 56, + "src": "1684:12", + "src_char": "1684:12" + }, + { + "contract_path": "src/IncorrectERC721.sol", + "line_no": 71, + "src": "2086:16", + "src_char": "2086:16" + } + ] + }, { "title": "Incorrect ERC20 interface.", "description": "Incorrect return values for ERC20 functions. A contract compiled with Solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing. Set the appropriate return values and types for the defined ERC20 functions.", @@ -2455,6 +2530,12 @@ "description": "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;`", "detector_name": "unspecific-solidity-pragma", "instances": [ + { + "contract_path": "src/BuiltinSymbolShadow.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/CompilerBugStorageSignedIntegerArray.sol", "line_no": 2, @@ -2533,6 +2614,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/IncorrectERC721.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/MsgValueInLoop.sol", "line_no": 2, @@ -2709,6 +2796,12 @@ "src": "113:1", "src_char": "113:1" }, + { + "contract_path": "src/BuiltinSymbolShadow.sol", + "line_no": 8, + "src": "125:41", + "src_char": "125:41" + }, { "contract_path": "src/ContractLocksEther.sol", "line_no": 20, @@ -3425,6 +3518,12 @@ "src": "5655:54", "src_char": "5655:54" }, + { + "contract_path": "src/IncorrectERC721.sol", + "line_no": 112, + "src": "3122:115", + "src_char": "3122:115" + }, { "contract_path": "src/TestERC20.sol", "line_no": 14, @@ -3699,6 +3798,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/IncorrectERC721.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/KeccakContract.sol", "line_no": 2, @@ -3856,6 +3961,12 @@ "description": "", "detector_name": "useless-modifier", "instances": [ + { + "contract_path": "src/BuiltinSymbolShadow.sol", + "line_no": 17, + "src": "358:39", + "src_char": "358:39" + }, { "contract_path": "src/CallGraphTests.sol", "line_no": 10, @@ -3941,6 +4052,12 @@ "src": "457:23", "src_char": "457:23" }, + { + "contract_path": "src/BuiltinSymbolShadow.sol", + "line_no": 8, + "src": "125:41", + "src_char": "125:41" + }, { "contract_path": "src/CacheArrayLength.sol", "line_no": 17, @@ -4947,6 +5064,37 @@ "src_char": "1325:183" } ] + }, + { + "title": "Builtin Symbol Shadowing", + "description": "Name clashes with a built-in-symbol. Consider renaming it.", + "detector_name": "builtin-symbol-shadow", + "instances": [ + { + "contract_path": "src/BuiltinSymbolShadow.sol", + "line_no": 5, + "src": "92:8", + "src_char": "92:8" + }, + { + "contract_path": "src/BuiltinSymbolShadow.sol", + "line_no": 8, + "src": "125:41", + "src_char": "125:41" + }, + { + "contract_path": "src/BuiltinSymbolShadow.sol", + "line_no": 17, + "src": "358:39", + "src_char": "358:39" + }, + { + "contract_path": "src/BuiltinSymbolShadow.sol", + "line_no": 22, + "src": "414:15", + "src_char": "414:15" + } + ] } ] }, @@ -5015,10 +5163,12 @@ "tx-origin-used-for-auth", "msg-value-in-loop", "contract-locks-ether", + "incorrect-erc721-interface", "incorrect-erc20-interface", "return-bomb", "out-of-order-retryable", "function-initializing-state", - "cache-array-length" + "cache-array-length", + "builtin-symbol-shadow" ] } \ No newline at end of file diff --git a/reports/report.md b/reports/report.md index f7a9a79b7..304393a95 100644 --- a/reports/report.md +++ b/reports/report.md @@ -44,8 +44,9 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [H-34: Potential use of `tx.origin` for authentication.](#h-34-potential-use-of-txorigin-for-authentication) - [H-35: Loop contains `msg.value`.](#h-35-loop-contains-msgvalue) - [H-36: Contract locks Ether without a withdraw function.](#h-36-contract-locks-ether-without-a-withdraw-function) - - [H-37: Incorrect ERC20 interface.](#h-37-incorrect-erc20-interface) - - [H-38: Out of order retryable transactions.](#h-38-out-of-order-retryable-transactions) + - [H-37: Incorrect ERC721 interface.](#h-37-incorrect-erc721-interface) + - [H-38: Incorrect ERC20 interface.](#h-38-incorrect-erc20-interface) + - [H-39: Out of order retryable transactions.](#h-39-out-of-order-retryable-transactions) - [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) @@ -78,6 +79,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [L-29: Return Bomb](#l-29-return-bomb) - [L-30: Function initializing state.](#l-30-function-initializing-state) - [L-31: Loop condition contains `state_variable.length` that could be cached outside.](#l-31-loop-condition-contains-statevariablelength-that-could-be-cached-outside) + - [L-32: Builtin Symbol Shadowing](#l-32-builtin-symbol-shadowing) # Summary @@ -86,8 +88,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 86 | -| Total nSLOC | 2933 | +| .sol Files | 88 | +| Total nSLOC | 3178 | ## Files Details @@ -99,6 +101,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/ArbitraryTransferFrom.sol | 37 | | src/AssemblyExample.sol | 9 | | src/BooleanEquality.sol | 27 | +| src/BuiltinSymbolShadow.sol | 14 | | src/CacheArrayLength.sol | 38 | | src/CallGraphTests.sol | 49 | | src/Casting.sol | 126 | @@ -125,6 +128,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/InconsistentUints.sol | 17 | | src/IncorrectCaretOperator.sol | 16 | | src/IncorrectERC20.sol | 97 | +| src/IncorrectERC721.sol | 231 | | src/IncorrectShift.sol | 17 | | src/InternalFunctions.sol | 22 | | src/KeccakContract.sol | 21 | @@ -180,15 +184,15 @@ 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** | **2933** | +| **Total** | **3178** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 38 | -| Low | 31 | +| High | 39 | +| Low | 32 | # High Issues @@ -1210,7 +1214,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. -
30 Found Instances +
32 Found Instances - Found in src/AssemblyExample.sol [Line: 5](../tests/contract-playground/src/AssemblyExample.sol#L5) @@ -1219,6 +1223,12 @@ Solidity does initialize variables by default when you declare them, however it' uint b; ``` +- Found in src/BuiltinSymbolShadow.sol [Line: 5](../tests/contract-playground/src/BuiltinSymbolShadow.sol#L5) + + ```solidity + uint now; // BAD + ``` + - Found in src/ConstantFuncsAssembly.sol [Line: 6](../tests/contract-playground/src/ConstantFuncsAssembly.sol#L6) ```solidity @@ -1249,6 +1259,12 @@ Solidity does initialize variables by default when you declare them, however it' uint256 private s_first; ``` +- Found in src/IncorrectERC721.sol [Line: 147](../tests/contract-playground/src/IncorrectERC721.sol#L147) + + ```solidity + uint256 public totalSupply; + ``` + - Found in src/PublicVariableReadInExternalContext.sol [Line: 6](../tests/contract-playground/src/PublicVariableReadInExternalContext.sol#L6) ```solidity @@ -2151,7 +2167,66 @@ It appears that the contract includes a payable function to accept Ether but lac -## H-37: Incorrect ERC20 interface. +## H-37: Incorrect ERC721 interface. + +Incorrect return values for ERC721 functions. A contract compiled with Solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing. Set the appropriate return values and types for the defined ERC721 functions. + +
8 Found Instances + + +- Found in src/IncorrectERC721.sol [Line: 14](../tests/contract-playground/src/IncorrectERC721.sol#L14) + + ```solidity + function balanceOf(address owner) external view returns (uint72) { + ``` + +- Found in src/IncorrectERC721.sol [Line: 18](../tests/contract-playground/src/IncorrectERC721.sol#L18) + + ```solidity + function ownerOf(uint256 tokenId) public view returns (bytes4) { + ``` + +- Found in src/IncorrectERC721.sol [Line: 22](../tests/contract-playground/src/IncorrectERC721.sol#L22) + + ```solidity + function approve( + ``` + +- Found in src/IncorrectERC721.sol [Line: 37](../tests/contract-playground/src/IncorrectERC721.sol#L37) + + ```solidity + function getApproved(uint256 tokenId) external view returns (uint72) { + ``` + +- Found in src/IncorrectERC721.sol [Line: 41](../tests/contract-playground/src/IncorrectERC721.sol#L41) + + ```solidity + function setApprovalForAll( + ``` + +- Found in src/IncorrectERC721.sol [Line: 49](../tests/contract-playground/src/IncorrectERC721.sol#L49) + + ```solidity + function isApprovedForAll( + ``` + +- Found in src/IncorrectERC721.sol [Line: 56](../tests/contract-playground/src/IncorrectERC721.sol#L56) + + ```solidity + function transferFrom( + ``` + +- Found in src/IncorrectERC721.sol [Line: 71](../tests/contract-playground/src/IncorrectERC721.sol#L71) + + ```solidity + function safeTransferFrom( + ``` + +
+ + + +## H-38: Incorrect ERC20 interface. Incorrect return values for ERC20 functions. A contract compiled with Solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing. Set the appropriate return values and types for the defined ERC20 functions. @@ -2192,7 +2267,7 @@ Incorrect return values for ERC20 functions. A contract compiled with Solidity > -## H-38: Out of order retryable transactions. +## H-39: Out of order retryable transactions. Do not rely on the order or successful execution of retryable tickets. Functions like createRetryableTicket, outboundTransferCustomRefund, unsafeCreateRetryableTicket are free to be re-tried in any order if they fail in the first go. Since this operation happens off chain, the sequencer is in control of the @@ -2467,9 +2542,15 @@ 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;` -
29 Found Instances +
31 Found Instances +- Found in src/BuiltinSymbolShadow.sol [Line: 2](../tests/contract-playground/src/BuiltinSymbolShadow.sol#L2) + + ```solidity + pragma solidity ^0.4.0; + ``` + - Found in src/CompilerBugStorageSignedIntegerArray.sol [Line: 2](../tests/contract-playground/src/CompilerBugStorageSignedIntegerArray.sol#L2) ```solidity @@ -2548,6 +2629,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity ^0.8.0; ``` +- Found in src/IncorrectERC721.sol [Line: 2](../tests/contract-playground/src/IncorrectERC721.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/MsgValueInLoop.sol [Line: 2](../tests/contract-playground/src/MsgValueInLoop.sol#L2) ```solidity @@ -2717,7 +2804,7 @@ Check for `address(0)` when assigning values to address state variables. Instead of marking a function as `public`, consider marking it as `external` if it is not used internally. -
40 Found Instances +
41 Found Instances - Found in src/ArbitraryTransferFrom.sol [Line: 28](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L28) @@ -2732,6 +2819,12 @@ Instead of marking a function as `public`, consider marking it as `external` if function f(uint x) public view returns (uint r) { ``` +- Found in src/BuiltinSymbolShadow.sol [Line: 8](../tests/contract-playground/src/BuiltinSymbolShadow.sol#L8) + + ```solidity + function assert(bool condition) public {} + ``` + - Found in src/ContractLocksEther.sol [Line: 20](../tests/contract-playground/src/ContractLocksEther.sol#L20) ```solidity @@ -3363,7 +3456,7 @@ If the same constant literal value is used multiple times, create a constant sta Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed. -
18 Found Instances +
19 Found Instances - Found in src/ContractLocksEther.sol [Line: 7](../tests/contract-playground/src/ContractLocksEther.sol#L7) @@ -3432,6 +3525,12 @@ Index event fields make the field more quickly accessible to off-chain tools tha event Transferred(address indexed to, uint256 amount); ``` +- Found in src/IncorrectERC721.sol [Line: 112](../tests/contract-playground/src/IncorrectERC721.sol#L112) + + ```solidity + event ApprovalForAll( + ``` + - Found in src/TestERC20.sol [Line: 14](../tests/contract-playground/src/TestERC20.sol#L14) ```solidity @@ -3647,7 +3746,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. -
37 Found Instances +
38 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -3722,6 +3821,12 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai pragma solidity ^0.8.0; ``` +- Found in src/IncorrectERC721.sol [Line: 2](../tests/contract-playground/src/IncorrectERC721.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/KeccakContract.sol [Line: 2](../tests/contract-playground/src/KeccakContract.sol#L2) ```solidity @@ -3880,8 +3985,14 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai -
12 Found Instances +
13 Found Instances + +- Found in src/BuiltinSymbolShadow.sol [Line: 17](../tests/contract-playground/src/BuiltinSymbolShadow.sol#L17) + + ```solidity + modifier blockhash() { + ``` - Found in src/CallGraphTests.sol [Line: 10](../tests/contract-playground/src/CallGraphTests.sol#L10) @@ -3963,7 +4074,7 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai Consider removing empty blocks. -
30 Found Instances +
31 Found Instances - Found in src/AdminContract.sol [Line: 14](../tests/contract-playground/src/AdminContract.sol#L14) @@ -3972,6 +4083,12 @@ Consider removing empty blocks. function someOtherImportantThing() external nonReentrant onlyOwner { ``` +- Found in src/BuiltinSymbolShadow.sol [Line: 8](../tests/contract-playground/src/BuiltinSymbolShadow.sol#L8) + + ```solidity + function assert(bool condition) public {} + ``` + - Found in src/CacheArrayLength.sol [Line: 17](../tests/contract-playground/src/CacheArrayLength.sol#L17) ```solidity @@ -5042,3 +5159,38 @@ Cache the lengths of storage arrays if they are used and not modified in for loo +## L-32: Builtin Symbol Shadowing + +Name clashes with a built-in-symbol. Consider renaming it. + +
4 Found Instances + + +- Found in src/BuiltinSymbolShadow.sol [Line: 5](../tests/contract-playground/src/BuiltinSymbolShadow.sol#L5) + + ```solidity + uint now; // BAD + ``` + +- Found in src/BuiltinSymbolShadow.sol [Line: 8](../tests/contract-playground/src/BuiltinSymbolShadow.sol#L8) + + ```solidity + function assert(bool condition) public {} + ``` + +- Found in src/BuiltinSymbolShadow.sol [Line: 17](../tests/contract-playground/src/BuiltinSymbolShadow.sol#L17) + + ```solidity + modifier blockhash() { + ``` + +- Found in src/BuiltinSymbolShadow.sol [Line: 22](../tests/contract-playground/src/BuiltinSymbolShadow.sol#L22) + + ```solidity + event sha256(); + ``` + +
+ + + diff --git a/reports/report.sarif b/reports/report.sarif index a8cf04280..5e3d253ca 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -1704,6 +1704,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BuiltinSymbolShadow.sol" + }, + "region": { + "byteLength": 8, + "byteOffset": 92 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -1759,6 +1770,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectERC721.sol" + }, + "region": { + "byteLength": 11, + "byteOffset": 4076 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3188,6 +3210,103 @@ }, "ruleId": "contract-locks-ether" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectERC721.sol" + }, + "region": { + "byteLength": 9, + "byteOffset": 433 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectERC721.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 551 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectERC721.sol" + }, + "region": { + "byteLength": 7, + "byteOffset": 696 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectERC721.sol" + }, + "region": { + "byteLength": 11, + "byteOffset": 1092 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectERC721.sol" + }, + "region": { + "byteLength": 17, + "byteOffset": 1231 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectERC721.sol" + }, + "region": { + "byteLength": 16, + "byteOffset": 1484 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectERC721.sol" + }, + "region": { + "byteLength": 12, + "byteOffset": 1684 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectERC721.sol" + }, + "region": { + "byteLength": 16, + "byteOffset": 2086 + } + } + } + ], + "message": { + "text": "Incorrect return values for ERC721 functions. A contract compiled with Solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing. Set the appropriate return values and types for the defined ERC721 functions." + }, + "ruleId": "incorrect-erc721-interface" + }, { "level": "warning", "locations": [ @@ -3672,6 +3791,17 @@ { "level": "note", "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BuiltinSymbolShadow.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3815,6 +3945,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectERC721.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -4130,6 +4271,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BuiltinSymbolShadow.sol" + }, + "region": { + "byteLength": 41, + "byteOffset": 125 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -5435,6 +5587,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectERC721.sol" + }, + "region": { + "byteLength": 115, + "byteOffset": 3122 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -5922,6 +6085,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/IncorrectERC721.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -6206,6 +6380,17 @@ { "level": "note", "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BuiltinSymbolShadow.sol" + }, + "region": { + "byteLength": 39, + "byteOffset": 358 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -6358,6 +6543,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BuiltinSymbolShadow.sol" + }, + "region": { + "byteLength": 41, + "byteOffset": 125 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -8148,6 +8344,59 @@ "text": "Cache the lengths of storage arrays if they are used and not modified in for loops." }, "ruleId": "cache-array-length" + }, + { + "level": "note", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BuiltinSymbolShadow.sol" + }, + "region": { + "byteLength": 8, + "byteOffset": 92 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BuiltinSymbolShadow.sol" + }, + "region": { + "byteLength": 41, + "byteOffset": 125 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BuiltinSymbolShadow.sol" + }, + "region": { + "byteLength": 39, + "byteOffset": 358 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/BuiltinSymbolShadow.sol" + }, + "region": { + "byteLength": 15, + "byteOffset": 414 + } + } + } + ], + "message": { + "text": "Name clashes with a built-in-symbol. Consider renaming it." + }, + "ruleId": "builtin-symbol-shadow" } ], "tool": { diff --git a/tests/contract-playground/src/BuiltinSymbolShadow.sol b/tests/contract-playground/src/BuiltinSymbolShadow.sol new file mode 100644 index 000000000..c07d261ea --- /dev/null +++ b/tests/contract-playground/src/BuiltinSymbolShadow.sol @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.4.0; + +contract BuiltinSymbolShadow { + uint now; // BAD + + // BAD + function assert(bool condition) public {} + + function get_next_expiration( + uint earlier_time + ) private blockhash returns (uint) { + return now + 259200; // References overshadowed timestamp. + } + + // BAD + modifier blockhash() { + _; + } + + // BAD + event sha256(); +} diff --git a/tests/contract-playground/src/IncorrectERC721.sol b/tests/contract-playground/src/IncorrectERC721.sol new file mode 100644 index 000000000..8a5c0edec --- /dev/null +++ b/tests/contract-playground/src/IncorrectERC721.sol @@ -0,0 +1,285 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +contract IncorrectERC721 { + string public name = "IncorrectNFT"; + string public symbol = "INFT"; + uint256 public totalSupply; + + mapping(uint256 => address) public owners; + mapping(address => uint256) public balances; + mapping(uint256 => address) public tokenApprovals; + mapping(address => mapping(address => bool)) public operatorApprovals; + + function balanceOf(address owner) external view returns (uint72) { + return uint72(balances[owner]); + } + + function ownerOf(uint256 tokenId) public view returns (bytes4) { + return bytes4(keccak256(abi.encodePacked(owners[tokenId]))); + } + + function approve( + address to, + uint256 tokenId + ) external returns (bytes memory) { + address owner = msg.sender; + require( + to != address(uint160(bytes20(owner))), + "Approval to current owner" + ); + require(msg.sender == owner, "Approve caller is not owner"); + + tokenApprovals[tokenId] = to; + return ""; + } + + function getApproved(uint256 tokenId) external view returns (uint72) { + return uint72(uint160(tokenApprovals[tokenId])); + } + + function setApprovalForAll( + address operator, + bool approved + ) external returns (bytes4) { + operatorApprovals[msg.sender][operator] = approved; + return bytes4(keccak256(abi.encodePacked(operator, approved))); + } + + function isApprovedForAll( + address owner, + address operator + ) external view returns (uint72) { + return operatorApprovals[owner][operator] ? uint72(1) : uint72(0); + } + + function transferFrom( + address from, + address to, + uint256 tokenId + ) external returns (bytes memory) { + require( + address(uint160(bytes20(ownerOf(tokenId)))) == from, + "Transfer from incorrect owner" + ); + require(to != address(0), "Transfer to the zero address"); + + _transfer(from, to, tokenId); + return ""; + } + + function safeTransferFrom( + address from, + address to, + uint256 tokenId, + bytes memory _data + ) external returns (uint8) { + return 1; + } + + function _transfer(address from, address to, uint256 tokenId) internal { + owners[tokenId] = to; + balances[from] -= 1; + balances[to] += 1; + } + + function mint(address to, uint256 tokenId) external returns (uint72) { + require(to != address(0), "Mint to the zero address"); + require(owners[tokenId] == address(0), "Token already minted"); + + _mint(to, tokenId); + return uint72(tokenId); + } + + function _mint(address to, uint256 tokenId) internal { + owners[tokenId] = to; + balances[to] += 1; + totalSupply += 1; + } +} + +interface MyIERC721 { + event Transfer( + address indexed from, + address indexed to, + uint256 indexed tokenId + ); + event Approval( + address indexed owner, + address indexed approved, + uint256 indexed tokenId + ); + event ApprovalForAll( + address indexed owner, + address indexed operator, + bool approved + ); + + function balanceOf(address owner) external view returns (uint256 balance); + + function ownerOf(uint256 tokenId) external view returns (address owner); + + function approve(address to, uint256 tokenId) external; + + function getApproved( + uint256 tokenId + ) external view returns (address operator); + + function setApprovalForAll(address operator, bool _approved) external; + + function isApprovedForAll( + address owner, + address operator + ) external view returns (bool); + + function transferFrom(address from, address to, uint256 tokenId) external; + + function safeTransferFrom( + address from, + address to, + uint256 tokenId + ) external; +} + +contract CorrectERC721 is MyIERC721 { + string public name = "CorrectNFT"; + string public symbol = "CNFT"; + uint256 public totalSupply; + + mapping(uint256 => address) private _owners; + mapping(address => uint256) private _balances; + mapping(uint256 => address) private _tokenApprovals; + mapping(address => mapping(address => bool)) private _operatorApprovals; + + function balanceOf(address owner) external view override returns (uint256) { + require(owner != address(0), "Balance query for the zero address"); + return _balances[owner]; + } + + function ownerOf(uint256 tokenId) public view override returns (address) { + address owner = _owners[tokenId]; + require(owner != address(0), "Owner query for nonexistent token"); + return owner; + } + + function approve(address to, uint256 tokenId) external override { + address owner = ownerOf(tokenId); + require(to != owner, "Approval to current owner"); + require( + msg.sender == owner || isApprovedForAll(owner, msg.sender), + "Approve caller is not owner nor approved for all" + ); + + _approve(to, tokenId); + } + + function getApproved( + uint256 tokenId + ) public view override returns (address) { + require( + _owners[tokenId] != address(0), + "Approved query for nonexistent token" + ); + return _tokenApprovals[tokenId]; + } + + function setApprovalForAll( + address operator, + bool approved + ) external override { + require(operator != msg.sender, "Approve to caller"); + + _operatorApprovals[msg.sender][operator] = approved; + emit ApprovalForAll(msg.sender, operator, approved); + } + + function isApprovedForAll( + address owner, + address operator + ) public view override returns (bool) { + return _operatorApprovals[owner][operator]; + } + + function transferFrom( + address from, + address to, + uint256 tokenId + ) external override { + require( + _isApprovedOrOwner(msg.sender, tokenId), + "Transfer caller is not owner nor approved" + ); + _transfer(from, to, tokenId); + } + + function safeTransferFrom( + address from, + address to, + uint256 tokenId + ) external override { + safeTransferFrom(from, to, tokenId, ""); + } + + function safeTransferFrom( + address from, + address to, + uint256 tokenId, + bytes memory _data + ) public { + require( + _isApprovedOrOwner(msg.sender, tokenId), + "Transfer caller is not owner nor approved" + ); + _safeTransfer(from, to, tokenId, _data); + } + + function _safeTransfer( + address from, + address to, + uint256 tokenId, + bytes memory _data + ) internal { + _transfer(from, to, tokenId); + } + + function _transfer(address from, address to, uint256 tokenId) internal { + require(ownerOf(tokenId) == from, "Transfer from incorrect owner"); + require(to != address(0), "Transfer to the zero address"); + + _approve(address(0), tokenId); + + _balances[from] -= 1; + _balances[to] += 1; + _owners[tokenId] = to; + + emit Transfer(from, to, tokenId); + } + + function _approve(address to, uint256 tokenId) internal { + _tokenApprovals[tokenId] = to; + emit Approval(ownerOf(tokenId), to, tokenId); + } + + function _isApprovedOrOwner( + address spender, + uint256 tokenId + ) internal view returns (bool) { + require( + _owners[tokenId] != address(0), + "Operator query for nonexistent token" + ); + address owner = ownerOf(tokenId); + return (spender == owner || + getApproved(tokenId) == spender || + isApprovedForAll(owner, spender)); + } +} + +interface IERC721Receiver { + function onERC721Received( + address operator, + address from, + uint256 tokenId, + bytes calldata data + ) external returns (bytes4); +}