From 4909dad5318319e923e9ea7db64db9e4dd47815a Mon Sep 17 00:00:00 2001 From: Tilak Madichetti Date: Mon, 19 Aug 2024 14:32:15 +0530 Subject: [PATCH] Detector: Constant Function changing state (#661) --- aderyn_core/src/context/browser/mod.rs | 2 + .../src/context/browser/storage_vars.rs | 1136 +++++++++++++++++ aderyn_core/src/detect/detector.rs | 5 + .../detect/high/const_func_change_state.rs | 168 +++ aderyn_core/src/detect/high/mod.rs | 2 + .../adhoc-sol-files-highs-only-report.json | 3 +- reports/report.json | 190 ++- reports/report.md | 204 ++- reports/report.sarif | 317 +++++ .../src/ConstFuncChangeState.sol | 23 + .../src/StateVariablesManipulation.sol | 356 ++++++ 11 files changed, 2391 insertions(+), 15 deletions(-) create mode 100644 aderyn_core/src/context/browser/storage_vars.rs create mode 100644 aderyn_core/src/detect/high/const_func_change_state.rs create mode 100644 tests/contract-playground/src/ConstFuncChangeState.sol create mode 100644 tests/contract-playground/src/StateVariablesManipulation.sol diff --git a/aderyn_core/src/context/browser/mod.rs b/aderyn_core/src/context/browser/mod.rs index e456c2c2d..e49ad8385 100644 --- a/aderyn_core/src/context/browser/mod.rs +++ b/aderyn_core/src/context/browser/mod.rs @@ -10,6 +10,7 @@ mod peek_over; mod peek_under; mod siblings; mod sort_nodes; +mod storage_vars; pub use ancestral_line::*; pub use closest_ancestor::*; pub use extractor::*; @@ -21,3 +22,4 @@ pub use peek_over::*; pub use peek_under::*; pub use siblings::*; pub use sort_nodes::*; +pub use storage_vars::*; diff --git a/aderyn_core/src/context/browser/storage_vars.rs b/aderyn_core/src/context/browser/storage_vars.rs new file mode 100644 index 000000000..873bb0efb --- /dev/null +++ b/aderyn_core/src/context/browser/storage_vars.rs @@ -0,0 +1,1136 @@ +use crate::{ + ast::*, + context::workspace_context::WorkspaceContext, + visitor::ast_visitor::{ASTConstVisitor, Node}, +}; +use eyre::*; +use std::{ + collections::{BTreeMap, BTreeSet}, + fmt::Debug, + iter::zip, + ops::Add, +}; + +/// Given an AST Block, it tries to detect any state variable inside it that may have been manipulated. +/// Now it's important to know that manipulations can occur either directly by assigning to a state variable +/// or, it may occur by assigning to a storage pointer that points to some state variable. +/// This light weight state variable manipulation finder captures both of the above kinds of assignments. +/// However, it's not smart enough to use a data dependency graph to determine the exact state variables +/// these storage pointers would be pointing to, in the context of the block-flow. +/// +/// NOTE - Asignment is not the only avenue for manipulating state variables, but also operations like +/// `push()` and `pop()` on arrays, `M[i] = x` on mappings, `delete X` imply the same. +/// +/// Here, the term manipulation covers all kinds of changes discussed above. +/// +/// IMPORTANT: DO NOT MAKE THESE MEMBERS PUBLIC. Use the public methods implemented on this structure only. +pub struct ApproximateStorageChangeFinder<'a> { + directly_manipulated_state_variables: BTreeSet, + manipulated_storage_pointers: BTreeSet, + /// Key => State Variable ID, Value => Storage Pointer ID (Heuristics based, this map is NOT exhaustive) + /// It leaves out a lot of links especially in cases where storage pointers are passed to and fro internal functions. + /// But on average, for most cases the way code is generally written, this should contain a decent chunk of links to lookup. + state_variables_to_storage_pointers: BTreeMap>, + context: &'a WorkspaceContext, +} + +/// This trait implementation will be useful when we run it through our callgraph and try to aggregate state variable changes. +impl<'a> Add<&'a ApproximateStorageChangeFinder<'_>> for ApproximateStorageChangeFinder<'a> { + type Output = ApproximateStorageChangeFinder<'a>; + + fn add(mut self, rhs: &ApproximateStorageChangeFinder) -> Self::Output { + self.directly_manipulated_state_variables + .extend(rhs.directly_manipulated_state_variables.iter()); + self.manipulated_storage_pointers + .extend(rhs.manipulated_storage_pointers.iter()); + // For state_variables_to_storage_pointers, we have to "add" the storage point entry vectors + for (state_var_id, storage_pointer_ids) in &rhs.state_variables_to_storage_pointers { + match self + .state_variables_to_storage_pointers + .entry(*state_var_id) + { + std::collections::btree_map::Entry::Vacant(v) => { + v.insert(BTreeSet::from_iter(storage_pointer_ids.iter().copied())); + } + std::collections::btree_map::Entry::Occupied(mut o) => { + (*o.get_mut()).extend(storage_pointer_ids); + } + }; + } + self + } +} + +impl<'a> Debug for ApproximateStorageChangeFinder<'a> { + // Do not print context. Hence, debug is custom derived for this struct + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + writeln!( + f, + "Manipulated directly: {:?}", + self.directly_manipulated_state_variables + )?; + + if !self.directly_manipulated_state_variables.is_empty() { + writeln!(f, "↓↓")?; + for id in &self.directly_manipulated_state_variables { + if let Some(node) = self.context.nodes.get(id) { + let loc_info = self.context.get_node_sort_key(node); + writeln!(f, "Line {:?}", (loc_info.1, loc_info.2))?; + } else { + writeln!(f, "\n", id)?; + } + } + writeln!(f)?; + } + + writeln!( + f, + "Manipulated through storage pointers: {:?}", + self.manipulated_storage_pointers + )?; + + if !self.manipulated_storage_pointers.is_empty() { + writeln!(f, "↓↓")?; + for id in &self.manipulated_storage_pointers { + if let Some(node) = self.context.nodes.get(id) { + let loc_info = self.context.get_node_sort_key(node); + writeln!(f, "Line {:?}", (loc_info.1, loc_info.2))?; + } else { + writeln!(f, "", id)?; + } + } + writeln!(f)?; + } + + writeln!( + f, + "Links heuristics: {:?}", + self.state_variables_to_storage_pointers + )?; + + if !self.state_variables_to_storage_pointers.is_empty() { + writeln!(f, "↓↓")?; + for (state_variable_id, storage_pointer_references) in + &self.state_variables_to_storage_pointers + { + if let Some(node) = self.context.nodes.get(state_variable_id) { + let loc_info = self.context.get_node_sort_key(node); + writeln!(f, "Links to {:?}", (loc_info.1, loc_info.2))?; + for node_id in storage_pointer_references { + if let Some(node) = self.context.nodes.get(node_id) { + let loc_info = self.context.get_node_sort_key(node); + writeln!(f, "\t <> {:?}", (loc_info.1, loc_info.2))?; + } + } + } else { + writeln!(f, "", state_variable_id)?; + } + } + writeln!(f)?; + } + + std::fmt::Result::Ok(()) + } +} + +/// Interface to be used by other modules defined here. +impl<'a> ApproximateStorageChangeFinder<'a> { + /// Initialize + pub fn from(context: &'a WorkspaceContext, node: &T) -> Self { + let mut extractor = ApproximateStorageChangeFinder { + directly_manipulated_state_variables: BTreeSet::new(), + manipulated_storage_pointers: BTreeSet::new(), + state_variables_to_storage_pointers: BTreeMap::new(), + context, + }; + node.accept(&mut extractor).unwrap_or_default(); + extractor + } + + pub fn state_variables_have_been_manipulated(&self) -> bool { + !self.directly_manipulated_state_variables.is_empty() + || !self.manipulated_storage_pointers.is_empty() + } + + pub fn no_state_variable_has_been_manipulated(&self) -> bool { + self.directly_manipulated_state_variables.is_empty() + && self.manipulated_storage_pointers.is_empty() + } + + pub fn fetch_non_exhaustive_manipulated_state_variables(&self) -> Vec<&VariableDeclaration> { + let mut manipulated_state_vars: BTreeSet = BTreeSet::new(); + manipulated_state_vars.extend(self.directly_manipulated_state_variables.iter()); + for (state_variable_id, storage_pointers) in self.state_variables_to_storage_pointers.iter() + { + if storage_pointers + .iter() + .any(|ptr| self.manipulated_storage_pointers.contains(ptr)) + { + manipulated_state_vars.insert(*state_variable_id); + } + } + manipulated_state_vars + .into_iter() + .flat_map(|v| self.context.nodes.get(&v)) + .flat_map(|n| { + if let ASTNode::VariableDeclaration(variable_declaration) = n { + assert!(variable_declaration.state_variable); + return Some(variable_declaration); + } + None + }) + .collect() + } + + pub fn state_variable_has_been_manipulated(&self, var: &VariableDeclaration) -> Option { + if self.directly_manipulated_state_variables.contains(&var.id) { + return Some(true); + } + if self.manipulated_storage_pointers.is_empty() { + return Some(false); + } + // Now use our heuristics + if self + .state_variables_to_storage_pointers + .get(&var.id) + .is_some_and(|entry| { + entry + .iter() + .any(|e| self.manipulated_storage_pointers.contains(e)) + }) + { + return Some(true); + } + + // At this point, we don't know if any of the storage pointers refer to [`var`], so we cannot say for + // sure, if it has been manipulated or not. + None + } + + pub fn state_variable_has_not_been_manipulated( + &self, + var: &VariableDeclaration, + ) -> Option { + if self.directly_manipulated_state_variables.contains(&var.id) { + return Some(false); + } + if self.manipulated_storage_pointers.is_empty() { + return Some(true); + } + // Now use our heuristics + if self + .state_variables_to_storage_pointers + .get(&var.id) + .is_some_and(|entry| { + entry + .iter() + .any(|e| self.manipulated_storage_pointers.contains(e)) + }) + { + return Some(false); + } + + // At this point, we don't know if any of the storage pointers refer to [`var`], so we cannot say for + // sure, if it has been manipulated or not. + None + } +} + +impl<'a> ASTConstVisitor for ApproximateStorageChangeFinder<'a> { + fn visit_unary_operation(&mut self, node: &UnaryOperation) -> Result { + // WRITE HEURISTICS + // Catch unary operations that manipulate variables + if node.operator == "delete" || node.operator == "++" || node.operator == "--" { + for id in find_base(node.sub_expression.as_ref()).0 { + match is_storage_variable_or_storage_pointer(self.context, id) { + Some(AssigneeType::StateVariable) => { + self.directly_manipulated_state_variables.insert(id); + } + Some(AssigneeType::StorageLocationVariable) => { + self.manipulated_storage_pointers.insert(id); + } + None => {} + }; + } + } + + Ok(true) + } + + fn visit_member_access(&mut self, member: &MemberAccess) -> Result { + if !member + .expression + .type_descriptions() + .is_some_and(|type_desc| { + type_desc.type_string.as_ref().is_some_and(|type_string| { + type_string.ends_with("[] storage ref") + || type_string.ends_with("[] storage pointer") + }) + }) + { + return Ok(true); + } + + let (base_variable_ids, _) = find_base(member.expression.as_ref()); + // assert!(base_variable_ids.len() == 1); + + // WRITE HEURISTICS + if member.member_name == "push" || member.member_name == "pop" { + for id in base_variable_ids.iter() { + match is_storage_variable_or_storage_pointer(self.context, *id) { + Some(AssigneeType::StateVariable) => { + self.directly_manipulated_state_variables.insert(*id); + } + Some(AssigneeType::StorageLocationVariable) => { + self.manipulated_storage_pointers.insert(*id); + } + None => {} + }; + } + } + + Ok(true) + } + + fn visit_assignment(&mut self, assignment: &Assignment) -> Result { + let (base_variable_lhs_ids, type_strings) = find_base(assignment.left_hand_side.as_ref()); + let (base_variable_rhs_ids, _) = find_base(assignment.right_hand_side.as_ref()); + + for (id, type_string) in zip(base_variable_lhs_ids.iter(), type_strings.iter()) { + // When something is assigned to an expression of type "storage pointer", no state variable's value changes. + // The only value changed is the thing which the storage pointer points to. + // The value of a storage variable changes if the expression's type string contains "storage ref" in case of structs, arrays, etc + + if type_string.ends_with("storage pointer") { + continue; + } + + match is_storage_variable_or_storage_pointer(self.context, *id) { + Some(AssigneeType::StateVariable) => { + self.directly_manipulated_state_variables.insert(*id); + } + Some(AssigneeType::StorageLocationVariable) => { + self.manipulated_storage_pointers.insert(*id); + } + None => {} + }; + } + + // Now, on a separate note, let's look for a heuristic to link up state variables with storage pointers. + // But here, we only handle the cases when there are equal number of elements on either side of `=` . + // This allows us to assume 1:1 relationship. + if base_variable_lhs_ids.len() == base_variable_rhs_ids.len() { + for (lhs_id, rhs_id) in zip(base_variable_lhs_ids, base_variable_rhs_ids) { + if let ( + Some(AssigneeType::StorageLocationVariable), + Some(AssigneeType::StateVariable), + ) = ( + is_storage_variable_or_storage_pointer(self.context, lhs_id), + is_storage_variable_or_storage_pointer(self.context, rhs_id), + ) { + match self.state_variables_to_storage_pointers.entry(rhs_id) { + std::collections::btree_map::Entry::Vacant(v) => { + v.insert(BTreeSet::from_iter([lhs_id])); + } + std::collections::btree_map::Entry::Occupied(mut o) => { + (*o.get_mut()).insert(lhs_id); + } + }; + } + } + } + + // READ HEURISTICS + // Pretty much the same logic as described in `visit_variable_declaration_statement` + // + + // let (left_node_ids, _) = find_base(assignment.left_hand_side.as_ref()); + // let right_node_ids = flatten_expression(assignment.right_hand_side.as_ref()); + + // See if it's a 1:1 relationship. Only then, proceed. Later, we can think of heuristics to handle x:y + // (but likely we will have to rely on a dependency graph for that. Case: `(x, y) = func()` is out of scope for this). + // + // When it comes to tracking WRITEs, it doesn't matter what's on RHS of `=` + // When it comes to tracking READs, it does! If the LHS type is storage then you are simply carrying a reference + // at compile time, you are not actually reading. Whereas, if the LHS is memory, you are performing "sload"! + // But again, this logic changes based on type of value in RHS. If it's a function call you should look at + // return values and nature of the corresponding variable where that value will be stored. Likewise, differernt + // for different nodes albeit identifier one is probably the most straightforward + // if left_node_ids.len() == right_node_ids.len() {} + + Ok(true) + } + + // For heurstics: Try to link up as much storage pointers and state variables as possible + fn visit_variable_declaration_statement( + &mut self, + node: &VariableDeclarationStatement, + ) -> Result { + if let Some(initial_value) = node.initial_value.as_ref() { + let corresponding_variable_declaration_ids = node + .declarations + .iter() + .map(|v| { + if let Some(variable_declaration) = v { + variable_declaration.id + } else { + i64::MIN + } + }) + .collect::>(); + let initial_value_node_ids = flatten_expression(initial_value); // For READ heuristics + let (initial_value_bases, _) = find_base(initial_value); // For LINK heuristics + + // Let's first support 1:1 relationships only + if corresponding_variable_declaration_ids.len() == initial_value_node_ids.len() { + let common_len = corresponding_variable_declaration_ids.len(); + + // This for loop takes care of recording instances in VDS (Var Decl Stmnt) where there is a read + // from the storage. + // + // READ HEURISTICS + // + // TODO: Write tests for these + // Then creates `passes_read_check()` before matching the var id on extracting refernce declarations + // Then replicate the logic for assignments + // use lvaluerequested = false and islvalue = true to check if it's being read + // in case of visiting indexaccess, memberaccess, etc (expr_node! variants) + // So that it can detect stuff outside of just assignments. Example - functionCall(a.b) where a is state var + // (Technically you are reading a.b's value to make that function call) + // + // for i in 0..common_len { + // let variable_declaration_id = corresponding_variable_declaration_ids[i]; + // let corresponding_initial_value_id = initial_value_node_ids[i]; + + // if is_storage_variable_or_storage_pointer( + // &self.context, + // variable_declaration_id, + // ) + // .is_some() + // { + // // If we are not assigning something to a storage pointer or a storage reference, that means + // // we're storing it in memory. Therefore, we can consider that the corresponding initialValue + // // is being "read". Otherwise we are just creating pointers, not "sload"ing. + // continue; + // } + + // if let Some(node) = self.context.nodes.get(&corresponding_initial_value_id) { + // // In case of internal function call, it's complex to analyze + // if node.node_type() != NodeType::FunctionCall || it is{ + // let referenced_declarations = + // ExtractReferencedDeclarations::from(node).extracted; + + // for variable_id in referenced_declarations { + // match is_storage_variable_or_storage_pointer( + // self.context, + // variable_id, + // ) { + // // Assumption: At this point in code we know that it could be a storage pointer/variable that represents + // // uint, bool, array element, etc. so it's technically being read. + // Some(AssigneeType::StateVariable) => { + // self.directly_read_state_variables.insert(variable_id); + // } + // Some(AssigneeType::StorageLocationVariable) => { + // self.read_storage_pointers.insert(variable_id); + // } + // None => {} + // } + // } + // } + // } + // } + + assert!(initial_value_bases.len() == common_len); + + // LINK heuristics + for i in 0..common_len { + let variable_declaration_id = corresponding_variable_declaration_ids[i]; + let corresponding_initial_value_base_id = initial_value_bases[i]; + + if let ( + Some(AssigneeType::StorageLocationVariable), + Some(AssigneeType::StateVariable), + ) = ( + is_storage_variable_or_storage_pointer( + self.context, + variable_declaration_id, + ), + is_storage_variable_or_storage_pointer( + self.context, + corresponding_initial_value_base_id, + ), + ) { + match self + .state_variables_to_storage_pointers + .entry(corresponding_initial_value_base_id) + { + std::collections::btree_map::Entry::Vacant(v) => { + v.insert(BTreeSet::from_iter([variable_declaration_id])); + } + std::collections::btree_map::Entry::Occupied(mut o) => { + (*o.get_mut()).insert(variable_declaration_id); + } + }; + } + } + } + } + + Ok(true) + } +} + +fn flatten_expression(expr: &Expression) -> Vec { + let mut node_ids = vec![]; + match expr { + Expression::TupleExpression(TupleExpression { components, .. }) => { + for component in components.iter().flatten() { + let component_node_ids = flatten_expression(component); + node_ids.extend(component_node_ids); + } + } + _ => { + node_ids.push(expr.get_node_id().unwrap_or(i64::MIN)); + } + } + node_ids +} + +fn find_base(expr: &Expression) -> (Vec, Vec) { + let mut node_ids = vec![]; + let mut type_strings = vec![]; + match expr { + Expression::Identifier(Identifier { + referenced_declaration: Some(id), + type_descriptions: + TypeDescriptions { + type_string: Some(type_string), + .. + }, + .. + }) => { + node_ids.push(*id); + type_strings.push(type_string.clone()); + } + // Handle mappings assignment + Expression::IndexAccess(IndexAccess { + base_expression, + type_descriptions: + TypeDescriptions { + type_string: Some(type_string), + .. + }, + .. + }) => { + node_ids.extend(find_base(base_expression.as_ref()).0); + type_strings.push(type_string.clone()); + } + // Handle struct member assignment + Expression::MemberAccess(MemberAccess { + expression, + type_descriptions: + TypeDescriptions { + type_string: Some(type_string), + .. + }, + .. + }) => { + node_ids.extend(find_base(expression.as_ref()).0); + type_strings.push(type_string.clone()); + } + // Handle tuple form lhs while assigning + Expression::TupleExpression(TupleExpression { components, .. }) => { + for component in components.iter() { + if let Some(component) = component { + let (component_node_ids, component_type_strings) = find_base(component); + node_ids.extend(component_node_ids); + type_strings.extend(component_type_strings); + } else { + node_ids.push(i64::MIN); + type_strings.push(String::from("irrelevant")); + } + } + } + // Handle assignment with values like ++i, --j, i++, etc + Expression::UnaryOperation(UnaryOperation { + sub_expression, + type_descriptions: + TypeDescriptions { + type_string: Some(type_string), + .. + }, + .. + }) => { + node_ids.extend(find_base(sub_expression.as_ref()).0); + type_strings.push(type_string.clone()); + } + _ => { + node_ids.push(i64::MIN); + type_strings.push(String::from("irrelevant")); + } + }; + assert_eq!(node_ids.len(), type_strings.len()); + (node_ids, type_strings) +} + +#[derive(PartialEq)] +enum AssigneeType { + StateVariable, + StorageLocationVariable, +} + +fn is_storage_variable_or_storage_pointer( + context: &WorkspaceContext, + node_id: NodeID, +) -> Option { + let node = context.nodes.get(&node_id)?; + if let ASTNode::VariableDeclaration(variable) = node { + // Assumption + // variable.state_variable is true when it's an actual state variable + // variable.storage_location is StorageLocation::Storage when it's a storage reference pointer + if variable.state_variable { + return Some(AssigneeType::StateVariable); + } else if variable.storage_location == StorageLocation::Storage { + return Some(AssigneeType::StorageLocationVariable); + } + } + None +} + +#[cfg(test)] +mod approximate_storage_change_finder_tests { + use crate::detect::test_utils::load_solidity_source_unit; + + use super::ApproximateStorageChangeFinder; + use serial_test::serial; + + #[test] + #[serial] + fn has_variable_declarations() { + let context = load_solidity_source_unit( + "../tests/contract-playground/src/StateVariablesManipulation.sol", + ); + + assert!(!context.variable_declarations().is_empty()); + } + + #[test] + #[serial] + fn test_no_state_variable_manipulations_found() { + let context = load_solidity_source_unit( + "../tests/contract-playground/src/StateVariablesManipulation.sol", + ); + + let contract = context.find_contract_by_name("NoStateVarManipulationExample"); + let func = contract.find_function_by_name("dontManipulateStateVar"); + + let finder = ApproximateStorageChangeFinder::from(&context, func.into()); + let no_changes_found = !finder.state_variables_have_been_manipulated(); + println!( + "NoStateVarManipulationExample::dontManipulateStateVar()\n{:?}", + finder + ); + println!("{:?}", finder); + assert!(no_changes_found); + } + + #[test] + #[serial] + fn test_simple_state_variable_manipulations_found() { + let context = load_solidity_source_unit( + "../tests/contract-playground/src/StateVariablesManipulation.sol", + ); + + let contract = context.find_contract_by_name("SimpleStateVarManipulationExample"); + let func = contract.find_function_by_name("manipulateStateVarDirectly"); + let func2 = contract.find_function_by_name("readSimpleStateVars"); + + let finder = ApproximateStorageChangeFinder::from(&context, func.into()); + let changes_found = finder.state_variables_have_been_manipulated(); + println!( + "SimpleStateVarManipulationExample::manipulateStateVarDirectly()\n{:?}", + finder + ); + assert!(changes_found); + assert_eq!(finder.directly_manipulated_state_variables.len(), 5); + assert!(finder.manipulated_storage_pointers.is_empty()); + + let finder = ApproximateStorageChangeFinder::from(&context, func2.into()); + let changes_found = finder.state_variables_have_been_manipulated(); + println!( + "SimpleStateVarManipulationExample::readSimpleStateVars()\n{:?}", + finder + ); + assert!(!changes_found); + assert!(finder.directly_manipulated_state_variables.is_empty()); + assert!(finder.manipulated_storage_pointers.is_empty()); + } + + #[test] + #[serial] + fn test_fixed_size_array_assignments() { + let context = load_solidity_source_unit( + "../tests/contract-playground/src/StateVariablesManipulation.sol", + ); + + let contract = context.find_contract_by_name("FixedSizeArraysAssignmentExample"); + + let func1 = contract.find_function_by_name("manipulateDirectly"); + let func2 = contract.find_function_by_name("manipulateViaIndexAccess"); + + // Test manipulateDirectly() function + + let finder = ApproximateStorageChangeFinder::from(&context, func1.into()); + println!( + "FixedSizeArraysAssignmentExample::manipulateDirectly()\n{:?}", + finder + ); + + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert_eq!(finder.directly_manipulated_state_variables.len(), 1); + assert!(finder.manipulated_storage_pointers.is_empty()); + + // Test manipulateViaIndexAccess() function + + let finder = ApproximateStorageChangeFinder::from(&context, func2.into()); + println!( + "FixedSizeArraysAssignmentExample::manipulateViaIndexAccess()\n{:?}", + finder + ); + + let changes_found2 = finder.state_variables_have_been_manipulated(); + assert!(changes_found2); + assert_eq!(finder.directly_manipulated_state_variables.len(), 2); + assert!(finder.manipulated_storage_pointers.is_empty()); + } + + #[test] + #[serial] + fn test_struct_plus_fixed_array_assignment_example() { + let context = load_solidity_source_unit( + "../tests/contract-playground/src/StateVariablesManipulation.sol", + ); + + let contract = context.find_contract_by_name("StructPlusFixedArrayAssignmentExample"); + + let func = contract.find_function_by_name("manipulateStateVariables"); + let func2 = contract.find_function_by_name("manipulateStateVariables2"); + let func3 = contract.find_function_by_name("manipulateStateVariables3"); + let func4 = contract.find_function_by_name("manipulateStateVariables4"); + let func5 = contract.find_function_by_name("manipulateStateVariables5"); + let func6 = contract.find_function_by_name("manipulateStateVariables6"); + let func7 = contract.find_function_by_name("manipulateStateVariables7"); + let func8 = contract.find_function_by_name("manipulateStateVariables8"); + let func_helper = contract.find_function_by_name("manipulateHelper"); + + // Test manipulateStateVariables + let finder = ApproximateStorageChangeFinder::from(&context, func.into()); + println!( + "StructPlusFixedArrayAssignmentExample::manipulateStateVariables()\n{:?}", + finder + ); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert_eq!(finder.directly_manipulated_state_variables.len(), 3); + assert!(finder.manipulated_storage_pointers.is_empty()); + + // Test manipulateStateVariables2 + let finder = ApproximateStorageChangeFinder::from(&context, func2.into()); + println!( + "StructPlusFixedArrayAssignmentExample::manipulateStateVariables2()\n{:?}", + finder + ); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert_eq!(finder.manipulated_storage_pointers.len(), 1); + assert_eq!( + finder + .state_variables_to_storage_pointers + .get(&contract.find_state_variable_node_id_by_name("person3")) + .unwrap() + .len(), + 1 + ); + assert_eq!( + finder + .state_variables_to_storage_pointers + .get(&contract.find_state_variable_node_id_by_name("persons")) + .unwrap() + .len(), + 1 + ); + assert!(finder.directly_manipulated_state_variables.is_empty()); + + // Test manipulateStateVariables3 + let finder = ApproximateStorageChangeFinder::from(&context, func3.into()); + println!( + "StructPlusFixedArrayAssignmentExample::manipulateStateVariables3()\n{:?}", + finder + ); + let no_changes_found = !finder.state_variables_have_been_manipulated(); + assert!(no_changes_found); + assert!(finder.manipulated_storage_pointers.is_empty()); + assert!(finder.directly_manipulated_state_variables.is_empty()); + assert_eq!(finder.state_variables_to_storage_pointers.len(), 1); // person3 links to ptr_person3 + assert_eq!( + finder + .state_variables_to_storage_pointers + .get(&contract.find_state_variable_node_id_by_name("person3")) + .unwrap() + .len(), + 1 + ); + + // Test manipulateStateVariables4 + let finder = ApproximateStorageChangeFinder::from(&context, func4.into()); + println!( + "StructPlusFixedArrayAssignmentExample::manipulateStateVariables4()\n{:?}", + finder + ); + let no_changes_found = !finder.state_variables_have_been_manipulated(); + assert!(no_changes_found); + assert!(finder.manipulated_storage_pointers.is_empty()); + assert!(finder.directly_manipulated_state_variables.is_empty()); + + // Test manipulateStateVariables5 + let finder = ApproximateStorageChangeFinder::from(&context, func5.into()); + println!( + "StructPlusFixedArrayAssignmentExample::manipulateStateVariables5()\n{:?}", + finder + ); + let no_changes_found = !finder.state_variables_have_been_manipulated(); + assert!(no_changes_found); + assert!(finder.manipulated_storage_pointers.is_empty()); + assert!(finder.directly_manipulated_state_variables.is_empty()); + assert_eq!(finder.state_variables_to_storage_pointers.len(), 1); + assert_eq!( + finder + .state_variables_to_storage_pointers + .get(&contract.find_state_variable_node_id_by_name("person3")) + .unwrap() + .len(), + 1 + ); + + // Test funcHelper + let finder = ApproximateStorageChangeFinder::from(&context, func_helper.into()); + println!( + "StructPlusFixedArrayAssignmentExample::manipulateHelper()\n{:?}", + finder + ); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert_eq!(finder.manipulated_storage_pointers.len(), 2); + assert!(finder.directly_manipulated_state_variables.is_empty()); + assert_eq!( + finder + .state_variables_to_storage_pointers + .get(&contract.find_state_variable_node_id_by_name("person3")) + .unwrap() + .len(), + 1 + ); + + // Test manipulateStateVariables6 + let finder = ApproximateStorageChangeFinder::from(&context, func6.into()); + println!( + "StructPlusFixedArrayAssignmentExample::manipulateStateVariables6()\n{:?}", + finder + ); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert_eq!(finder.manipulated_storage_pointers.len(), 2); + assert!(finder.directly_manipulated_state_variables.is_empty()); + assert_eq!( + finder + .state_variables_to_storage_pointers + .get(&contract.find_state_variable_node_id_by_name("person3")) + .unwrap() + .len(), + 4 + ); + + // Test manipulateStateVariables7 + let finder = ApproximateStorageChangeFinder::from(&context, func7.into()); + println!( + "StructPlusFixedArrayAssignmentExample::manipulateStateVariables7()\n{:?}", + finder + ); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert!(finder.manipulated_storage_pointers.is_empty()); + assert_eq!(finder.directly_manipulated_state_variables.len(), 3); + + // Test manipulateStateVariables8 + let finder = ApproximateStorageChangeFinder::from(&context, func8.into()); + println!( + "StructPlusFixedArrayAssignmentExample::manipulateStateVariables8()\n{:?}", + finder + ); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert!(finder.manipulated_storage_pointers.is_empty()); + assert_eq!(finder.directly_manipulated_state_variables.len(), 2); + } + + #[test] + #[serial] + fn test_sv_manipulation_library() { + let context = load_solidity_source_unit( + "../tests/contract-playground/src/StateVariablesManipulation.sol", + ); + + let contract = context.find_contract_by_name("SVManipulationLibrary"); + + let func = contract.find_function_by_name("manipulateLib"); + let func2 = contract.find_function_by_name("manipulateLib2"); + let func3 = contract.find_function_by_name("manipulateLib3"); + + // Test manipulateLib() + let finder = ApproximateStorageChangeFinder::from(&context, func.into()); + println!("SVManipulationLibrary::manipulateLib()\n{:?}", finder); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert_eq!(finder.manipulated_storage_pointers.len(), 1); + assert!(finder.directly_manipulated_state_variables.is_empty()); + + // Test manipulateLib2() + let finder = ApproximateStorageChangeFinder::from(&context, func2.into()); + println!("SVManipulationLibrary::manipulateLib2()\n{:?}", finder); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert_eq!(finder.manipulated_storage_pointers.len(), 1); + assert!(finder.directly_manipulated_state_variables.is_empty()); + + // Test manipulateLib3() + let finder = ApproximateStorageChangeFinder::from(&context, func3.into()); + println!("SVManipulationLibrary::manipulateLib3()\n{:?}", finder); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert_eq!(finder.manipulated_storage_pointers.len(), 2); + assert!(finder.directly_manipulated_state_variables.is_empty()); + } + + #[test] + #[serial] + fn test_no_struct_plus_fixed_array_assignment_example() { + let context = load_solidity_source_unit( + "../tests/contract-playground/src/StateVariablesManipulation.sol", + ); + + let contract = context.find_contract_by_name("NoStructPlusFixedArrayAssignmentExample"); + + let func = contract.find_function_by_name("dontManipulateStateVariables"); + let func2 = contract.find_function_by_name("dontManipulateStateVariablesPart2"); + let func3 = contract.find_function_by_name("dontManipulateStateVariablesPart3"); + let func4 = contract.find_function_by_name("dontManipulateStateVariablesPart4"); + let func5 = contract.find_function_by_name("dontManipulateStateVariablesPart5"); + + // Test dontManipulateStateVariables() + let finder = ApproximateStorageChangeFinder::from(&context, func.into()); + println!( + "NoStructPlusFixedArrayAssignmentExample::dontManipulateStateVariables()\n{:?}", + finder + ); + let no_changes_found = !finder.state_variables_have_been_manipulated(); + assert!(no_changes_found); + assert!(finder.manipulated_storage_pointers.is_empty()); + assert!(finder.directly_manipulated_state_variables.is_empty()); + + // Test dontManipulateStateVariablesPart2() + let finder = ApproximateStorageChangeFinder::from(&context, func2.into()); + println!( + "NoStructPlusFixedArrayAssignmentExample::dontManipulateStateVariablesPart2()\n{:?}", + finder + ); + let no_changes_found = !finder.state_variables_have_been_manipulated(); + assert!(no_changes_found); + assert!(finder.manipulated_storage_pointers.is_empty()); + assert!(finder.directly_manipulated_state_variables.is_empty()); + + // Test dontManipulateStateVariablesPart3() + let finder = ApproximateStorageChangeFinder::from(&context, func3.into()); + println!( + "NoStructPlusFixedArrayAssignmentExample::dontManipulateStateVariablesPart3()\n{:?}", + finder + ); + let no_changes_found = !finder.state_variables_have_been_manipulated(); + assert!(no_changes_found); + assert!(finder.manipulated_storage_pointers.is_empty()); + assert!(finder.directly_manipulated_state_variables.is_empty()); + + // Test dontManipulateStateVariablesPart4() + let finder = ApproximateStorageChangeFinder::from(&context, func4.into()); + println!( + "NoStructPlusFixedArrayAssignmentExample::dontManipulateStateVariablesPart4()\n{:?}", + finder + ); + let no_changes_found = !finder.state_variables_have_been_manipulated(); + assert!(no_changes_found); + assert!(finder.manipulated_storage_pointers.is_empty()); + assert!(finder.directly_manipulated_state_variables.is_empty()); + + // Test dontManipulateStateVariablesPart4() + let finder = ApproximateStorageChangeFinder::from(&context, func5.into()); + println!( + "NoStructPlusFixedArrayAssignmentExample::dontManipulateStateVariablesPart5()\n{:?}", + finder + ); + let no_changes_found = !finder.state_variables_have_been_manipulated(); + assert!(no_changes_found); + assert!(finder.manipulated_storage_pointers.is_empty()); + assert!(finder.directly_manipulated_state_variables.is_empty()); + } + + #[test] + #[serial] + fn test_dynamic_array_push_changes() { + let context = load_solidity_source_unit( + "../tests/contract-playground/src/StateVariablesManipulation.sol", + ); + + let contract = context.find_contract_by_name("DynamicArraysPushExample"); + + let func = contract.find_function_by_name("manipulateDirectly"); + let func2 = contract.find_function_by_name("manipulateViaIndexAccess"); + let func3 = contract.find_function_by_name("manipulateViaMemberAccess"); + let func4 = contract.find_function_by_name("manipulateViaMemberAccess2"); + + // Test manipulateDirectly() + let finder = ApproximateStorageChangeFinder::from(&context, func.into()); + println!( + "DynamicArraysPushExample::manipulateDirectly()\n{:?}", + finder + ); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert!(finder.manipulated_storage_pointers.is_empty()); + assert_eq!(finder.directly_manipulated_state_variables.len(), 1); + + // Test manipulateViaIndexAccess() + let finder = ApproximateStorageChangeFinder::from(&context, func2.into()); + println!( + "DynamicArraysPushExample::manipulateViaIndexAccess()\n{:?}", + finder + ); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert!(finder.manipulated_storage_pointers.is_empty()); + assert_eq!(finder.directly_manipulated_state_variables.len(), 3); + + // Test manipulateViaMemberAccess() + let finder = ApproximateStorageChangeFinder::from(&context, func3.into()); + println!( + "DynamicArraysPushExample::manipulateViaMemberAccess()\n{:?}", + finder + ); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert!(finder.manipulated_storage_pointers.is_empty()); + assert_eq!(finder.directly_manipulated_state_variables.len(), 1); + + // Test manipulateViaMemberAccess2() + let finder = ApproximateStorageChangeFinder::from(&context, func4.into()); + println!( + "DynamicArraysPushExample::manipulateViaMemberAccess2()\n{:?}", + finder + ); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert_eq!(finder.manipulated_storage_pointers.len(), 1); // we only want to capture p1 + assert!(finder.directly_manipulated_state_variables.is_empty()); + } + + #[test] + #[serial] + fn test_dynamic_mappings_array_push_changes() { + let context = load_solidity_source_unit( + "../tests/contract-playground/src/StateVariablesManipulation.sol", + ); + + let contract = context.find_contract_by_name("DynamicMappingsArrayPushExample"); + let func = contract.find_function_by_name("add"); + + // Test add() + let finder = ApproximateStorageChangeFinder::from(&context, func.into()); + println!("DynamicMappingsArrayPushExample::add()\n{:?}", finder); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert_eq!(finder.directly_manipulated_state_variables.len(), 1); + assert!(finder.manipulated_storage_pointers.is_empty()); + } + + #[test] + #[serial] + fn test_fixed_size_arrays_deletion_example() { + let context = load_solidity_source_unit( + "../tests/contract-playground/src/StateVariablesManipulation.sol", + ); + + let contract = context.find_contract_by_name("FixedSizeArraysDeletionExample"); + let func = contract.find_function_by_name("manipulateDirectly"); + let func2 = contract.find_function_by_name("manipulateViaIndexAccess"); + + // Test func() + let finder = ApproximateStorageChangeFinder::from(&context, func.into()); + println!( + "FixedSizeArraysDeletionExample::manipulateDirectly()\n{:?}", + finder + ); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert_eq!(finder.directly_manipulated_state_variables.len(), 1); + assert!(finder.manipulated_storage_pointers.is_empty()); + + // Test func2() + let finder = ApproximateStorageChangeFinder::from(&context, func2.into()); + println!( + "FixedSizeArraysDeletionExample::manipulateViaIndexAccess()\n{:?}", + finder + ); + let changes_found = finder.state_variables_have_been_manipulated(); + assert!(changes_found); + assert_eq!(finder.directly_manipulated_state_variables.len(), 2); + assert!(finder.manipulated_storage_pointers.is_empty()); + } +} + +#[cfg(test)] +mod storage_vars_tests_helper { + + // Using unwraps here are OK *only* becuase it's compiled with #[cfg(test)] + + use crate::context::{ + browser::ExtractVariableDeclarations, workspace_context::WorkspaceContext, + }; + + use super::{ContractDefinition, FunctionDefinition, NodeID}; + + impl WorkspaceContext { + pub fn find_contract_by_name(&self, name: &str) -> &ContractDefinition { + self.contract_definitions() + .into_iter() + .find(|c| c.name.as_str() == name) + .unwrap() + } + } + + impl ContractDefinition { + pub fn find_function_by_name(&self, name: &str) -> &FunctionDefinition { + self.function_definitions() + .iter() + .find(|func| func.name == name) + .unwrap() + } + + pub fn find_state_variable_node_id_by_name(&self, name: &str) -> NodeID { + let variable_declarations = ExtractVariableDeclarations::from(self).extracted; + let variable = variable_declarations + .into_iter() + .filter(|v| v.state_variable && v.name == name) + .collect::>(); + variable.first().unwrap().id + } + } +} diff --git a/aderyn_core/src/detect/detector.rs b/aderyn_core/src/detect/detector.rs index ea299e1e4..1b63e6c1e 100644 --- a/aderyn_core/src/detect/detector.rs +++ b/aderyn_core/src/detect/detector.rs @@ -86,6 +86,7 @@ pub fn get_all_issue_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), ] } @@ -98,6 +99,7 @@ pub fn get_all_detectors_names() -> Vec { #[derive(Debug, PartialEq, EnumString, Display)] #[strum(serialize_all = "kebab-case")] pub(crate) enum IssueDetectorNamePool { + ConstantFunctionChangingState, BuiltinSymbolShadow, IncorrectERC721Interface, FunctionInitializingState, @@ -178,6 +180,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option { + Some(Box::::default()) + } IssueDetectorNamePool::BuiltinSymbolShadow => { Some(Box::::default()) } diff --git a/aderyn_core/src/detect/high/const_func_change_state.rs b/aderyn_core/src/detect/high/const_func_change_state.rs new file mode 100644 index 000000000..cbeb77dd9 --- /dev/null +++ b/aderyn_core/src/detect/high/const_func_change_state.rs @@ -0,0 +1,168 @@ +use std::collections::BTreeMap; +use std::error::Error; + +use crate::ast::{NodeID, StateMutability}; + +use crate::capture; +use crate::context::browser::ApproximateStorageChangeFinder; +use crate::context::graph::{CallGraph, CallGraphDirection, CallGraphVisitor}; +use crate::detect::detector::IssueDetectorNamePool; +use crate::detect::helpers; +use crate::{ + context::workspace_context::WorkspaceContext, + detect::detector::{IssueDetector, IssueSeverity}, +}; +use eyre::Result; + +#[derive(Default)] +pub struct ConstantFunctionChangingStateDetector { + // 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 ConstantFunctionChangingStateDetector { + fn detect(&mut self, context: &WorkspaceContext) -> Result> { + for func in helpers::get_implemented_external_and_public_functions(context) { + // Rule applies to only view functions, so ignore the rest + if func.state_mutability() != &StateMutability::View { + continue; + } + // Check if this func is compilable for solc < 0.5.0. If not, move on to the next + if !func.compiles_for_solc_below_0_5_0(context) { + continue; + } + // Now, investigate the function to see if there is scope for any state variable changes + let mut tracker = StateVariableChangeTracker { + state_var_has_changed: false, + context, + }; + + let investigator = + CallGraph::new(context, &[&(func.into())], CallGraphDirection::Inward)?; + investigator.accept(context, &mut tracker)?; + + if tracker.state_var_has_changed { + capture!(self, context, func); + } + } + + Ok(!self.found_instances.is_empty()) + } + + fn severity(&self) -> IssueSeverity { + IssueSeverity::High + } + + fn title(&self) -> String { + String::from("Constant functions changing state") + } + + fn description(&self) -> String { + String::from("Function is declared constant/view but it changes state. Ensure that the attributes of contract compiled prior to 0.5 are correct.") + } + + fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> { + self.found_instances.clone() + } + + fn name(&self) -> String { + IssueDetectorNamePool::ConstantFunctionChangingState.to_string() + } +} + +struct StateVariableChangeTracker<'a> { + state_var_has_changed: bool, + context: &'a WorkspaceContext, +} + +impl<'a> CallGraphVisitor for StateVariableChangeTracker<'a> { + fn visit_any(&mut self, node: &crate::ast::ASTNode) -> eyre::Result<()> { + if self.state_var_has_changed { + return Ok(()); + } + // Check for state variable changes + let finder = ApproximateStorageChangeFinder::from(self.context, node); + if finder.state_variables_have_been_manipulated() { + self.state_var_has_changed = true; + } + Ok(()) + } +} + +mod func_compilation_solc_pragma_helper { + use std::str::FromStr; + + use semver::{Version, VersionReq}; + + use crate::{ + ast::{FunctionDefinition, NodeType}, + context::{ + browser::{ExtractPragmaDirectives, GetClosestAncestorOfTypeX}, + workspace_context::WorkspaceContext, + }, + detect::helpers, + }; + + impl FunctionDefinition { + pub fn compiles_for_solc_below_0_5_0(&self, context: &WorkspaceContext) -> bool { + if let Some(source_unit) = self.closest_ancestor_of_type(context, NodeType::SourceUnit) + { + let pragma_directives = ExtractPragmaDirectives::from(source_unit).extracted; + + if let Some(pragma_directive) = pragma_directives.first() { + if let Ok(pragma_semver) = helpers::pragma_directive_to_semver(pragma_directive) + { + if version_req_allows_below_0_5_0(&pragma_semver) { + return true; + } + } + } + } + false + } + } + + fn version_req_allows_below_0_5_0(version_req: &VersionReq) -> bool { + // If it matches any 0.4.0 to 0.4.26, return true + for i in 0..=26 { + let version = Version::from_str(&format!("0.4.{}", i)).unwrap(); + if version_req.matches(&version) { + return true; + } + } + + // Else, return false + false + } +} + +#[cfg(test)] +mod constant_func_changing_state { + use serial_test::serial; + + use crate::detect::{ + detector::IssueDetector, + high::const_func_change_state::ConstantFunctionChangingStateDetector, + }; + + #[test] + #[serial] + fn test_constant_function_changing_state() { + let context = crate::detect::test_utils::load_solidity_source_unit( + "../tests/contract-playground/src/ConstFuncChangeState.sol", + ); + + let mut detector = ConstantFunctionChangingStateDetector::default(); + let found = detector.detect(&context).unwrap(); + // assert that the detector found an issue + assert!(found); + // assert that the detector found the correct number of instances + assert_eq!(detector.instances().len(), 1); + // assert the severity is high + assert_eq!( + detector.severity(), + crate::detect::detector::IssueSeverity::High + ); + } +} diff --git a/aderyn_core/src/detect/high/mod.rs b/aderyn_core/src/detect/high/mod.rs index 27f9cfd86..5267c7a8d 100644 --- a/aderyn_core/src/detect/high/mod.rs +++ b/aderyn_core/src/detect/high/mod.rs @@ -1,6 +1,7 @@ pub(crate) mod arbitrary_transfer_from; pub(crate) mod avoid_abi_encode_packed; pub(crate) mod block_timestamp_deadline; +pub(crate) mod const_func_change_state; pub(crate) mod contract_locks_ether; pub(crate) mod dangerous_strict_equality_balance; pub(crate) mod dangerous_unary_operator; @@ -41,6 +42,7 @@ pub(crate) mod yul_return; pub use arbitrary_transfer_from::ArbitraryTransferFromDetector; pub use avoid_abi_encode_packed::AvoidAbiEncodePackedDetector; pub use block_timestamp_deadline::BlockTimestampDeadlineDetector; +pub use const_func_change_state::ConstantFunctionChangingStateDetector; pub use contract_locks_ether::ContractLocksEtherDetector; pub use dangerous_strict_equality_balance::DangerousStrictEqualityOnBalanceDetector; pub use dangerous_unary_operator::DangerousUnaryOperatorDetector; diff --git a/reports/adhoc-sol-files-highs-only-report.json b/reports/adhoc-sol-files-highs-only-report.json index fe22ffccf..a69540c76 100644 --- a/reports/adhoc-sol-files-highs-only-report.json +++ b/reports/adhoc-sol-files-highs-only-report.json @@ -200,6 +200,7 @@ "contract-locks-ether", "incorrect-erc721-interface", "incorrect-erc20-interface", - "out-of-order-retryable" + "out-of-order-retryable", + "constant-function-changing-state" ] } \ No newline at end of file diff --git a/reports/report.json b/reports/report.json index 0e6baff25..45cb8e406 100644 --- a/reports/report.json +++ b/reports/report.json @@ -1,7 +1,7 @@ { "files_summary": { - "total_source_units": 86, - "total_sloc": 2890 + "total_source_units": 88, + "total_sloc": 3155 }, "files_details": { "files_details": [ @@ -41,6 +41,10 @@ "file_path": "src/CompilerBugStorageSignedIntegerArray.sol", "n_sloc": 13 }, + { + "file_path": "src/ConstFuncChangeState.sol", + "n_sloc": 15 + }, { "file_path": "src/ConstantFuncsAssembly.sol", "n_sloc": 26 @@ -201,6 +205,10 @@ "file_path": "src/StateVariables.sol", "n_sloc": 58 }, + { + "file_path": "src/StateVariablesManipulation.sol", + "n_sloc": 250 + }, { "file_path": "src/StorageConditionals.sol", "n_sloc": 59 @@ -352,7 +360,7 @@ ] }, "issue_count": { - "high": 39, + "high": 40, "low": 31 }, "high_issues": { @@ -1397,6 +1405,48 @@ "src": "282:18", "src_char": "282:18" }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 8, + "src": "184:10", + "src_char": "184:10" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 9, + "src": "214:9", + "src_char": "214:9" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 10, + "src": "241:10", + "src_char": "241:10" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 11, + "src": "272:13", + "src_char": "272:13" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 12, + "src": "314:20", + "src_char": "314:20" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 13, + "src": "354:12", + "src_char": "354:12" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 14, + "src": "385:11", + "src_char": "385:11" + }, { "contract_path": "src/TautologyOrContradiction.sol", "line_no": 6, @@ -1833,6 +1883,12 @@ "src": "4337:261", "src_char": "4337:261" }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 142, + "src": "4146:20", + "src_char": "4146:20" + }, { "contract_path": "src/UncheckedReturn.sol", "line_no": 14, @@ -2243,6 +2299,19 @@ "src_char": "2557:22" } ] + }, + { + "title": "Constant functions changing state", + "description": "Function is declared constant/view but it changes state. Ensure that the attributes of contract compiled prior to 0.5 are correct.", + "detector_name": "constant-function-changing-state", + "instances": [ + { + "contract_path": "src/ConstFuncChangeState.sol", + "line_no": 8, + "src": "173:112", + "src_char": "173:112" + } + ] } ] }, @@ -2486,6 +2555,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/ConstFuncChangeState.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/ConstantFuncsAssembly.sol", "line_no": 2, @@ -2588,6 +2663,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/TautologyOrContradiction.sol", "line_no": 2, @@ -2685,6 +2766,12 @@ "src": "2121:14", "src_char": "2121:14" }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 56, + "src": "1657:17", + "src_char": "1657:17" + }, { "contract_path": "src/ZeroAddressCheck.sol", "line_no": 43, @@ -2734,6 +2821,24 @@ "src": "125:41", "src_char": "125:41" }, + { + "contract_path": "src/ConstFuncChangeState.sol", + "line_no": 8, + "src": "173:112", + "src_char": "173:112" + }, + { + "contract_path": "src/ConstFuncChangeState.sol", + "line_no": 14, + "src": "339:108", + "src_char": "339:108" + }, + { + "contract_path": "src/ConstFuncChangeState.sol", + "line_no": 20, + "src": "517:89", + "src_char": "517:89" + }, { "contract_path": "src/ContractLocksEther.sol", "line_no": 20, @@ -3227,6 +3332,78 @@ "src": "745:2", "src_char": "745:2" }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 121, + "src": "3471:2", + "src_char": "3471:2" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 123, + "src": "3542:2", + "src_char": "3542:2" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 192, + "src": "5408:3", + "src_char": "5408:3" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 202, + "src": "5621:3", + "src_char": "5621:3" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 233, + "src": "6372:1", + "src_char": "6372:1" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 237, + "src": "6468:2", + "src_char": "6468:2" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 239, + "src": "6543:2", + "src_char": "6543:2" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 244, + "src": "6704:2", + "src_char": "6704:2" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 258, + "src": "7236:1", + "src_char": "7236:1" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 264, + "src": "7497:1", + "src_char": "7497:1" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 333, + "src": "9506:3", + "src_char": "9506:3" + }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 339, + "src": "9671:3", + "src_char": "9671:3" + }, { "contract_path": "src/UncheckedReturn.sol", "line_no": 27, @@ -3664,6 +3841,12 @@ "src": "32:23", "src_char": "32:23" }, + { + "contract_path": "src/StateVariablesManipulation.sol", + "line_no": 2, + "src": "32:23", + "src_char": "32:23" + }, { "contract_path": "src/StorageConditionals.sol", "line_no": 2, @@ -4955,6 +5138,7 @@ "return-bomb", "out-of-order-retryable", "function-initializing-state", + "constant-function-changing-state", "builtin-symbol-shadow" ] } \ No newline at end of file diff --git a/reports/report.md b/reports/report.md index b4902741b..e5e513467 100644 --- a/reports/report.md +++ b/reports/report.md @@ -47,6 +47,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati - [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) + - [H-40: Constant functions changing state](#h-40-constant-functions-changing-state) - [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) @@ -87,8 +88,8 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | Key | Value | | --- | --- | -| .sol Files | 86 | -| Total nSLOC | 2890 | +| .sol Files | 88 | +| Total nSLOC | 3155 | ## Files Details @@ -104,6 +105,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/CallGraphTests.sol | 49 | | src/Casting.sol | 126 | | src/CompilerBugStorageSignedIntegerArray.sol | 13 | +| src/ConstFuncChangeState.sol | 15 | | src/ConstantFuncsAssembly.sol | 26 | | src/ConstantsLiterals.sol | 28 | | src/ContractLocksEther.sol | 121 | @@ -144,6 +146,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/SendEtherNoChecks.sol | 58 | | src/StateShadowing.sol | 17 | | src/StateVariables.sol | 58 | +| src/StateVariablesManipulation.sol | 250 | | src/StorageConditionals.sol | 59 | | src/StorageParameters.sol | 16 | | src/T11sTranferer.sol | 8 | @@ -181,14 +184,14 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati | src/reused_contract_name/ContractB.sol | 7 | | src/uniswap/UniswapV2Swapper.sol | 50 | | src/uniswap/UniswapV3Swapper.sol | 150 | -| **Total** | **2890** | +| **Total** | **3155** | ## Issue Summary | Category | No. of Issues | | --- | --- | -| High | 39 | +| High | 40 | | Low | 31 | @@ -1211,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. -
25 Found Instances +
32 Found Instances - Found in src/AssemblyExample.sol [Line: 5](../tests/contract-playground/src/AssemblyExample.sol#L5) @@ -1298,6 +1301,48 @@ Solidity does initialize variables by default when you declare them, however it' uint256 public staticPublicNumber; ``` +- Found in src/StateVariablesManipulation.sol [Line: 8](../tests/contract-playground/src/StateVariablesManipulation.sol#L8) + + ```solidity + uint256 public simpleUint; + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 9](../tests/contract-playground/src/StateVariablesManipulation.sol#L9) + + ```solidity + int256 public simpleInt; + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 10](../tests/contract-playground/src/StateVariablesManipulation.sol#L10) + + ```solidity + bool public simpleBool; + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 11](../tests/contract-playground/src/StateVariablesManipulation.sol#L11) + + ```solidity + address public simpleAddress; + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 12](../tests/contract-playground/src/StateVariablesManipulation.sol#L12) + + ```solidity + address payable public simplePayableAddress; + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 13](../tests/contract-playground/src/StateVariablesManipulation.sol#L13) + + ```solidity + string public simpleString; + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 14](../tests/contract-playground/src/StateVariablesManipulation.sol#L14) + + ```solidity + bytes public simpleBytes; + ``` + - Found in src/TautologyOrContradiction.sol [Line: 6](../tests/contract-playground/src/TautologyOrContradiction.sol#L6) ```solidity @@ -1735,7 +1780,7 @@ Right to left override character may be misledaing and cause potential attacks b Function returns a value but it is ignored. -
8 Found Instances +
9 Found Instances - Found in src/OutOfOrderRetryable.sol [Line: 65](../tests/contract-playground/src/OutOfOrderRetryable.sol#L65) @@ -1774,6 +1819,12 @@ Function returns a value but it is ignored. IInbox(inbox).unsafeCreateRetryableTicket( ``` +- Found in src/StateVariablesManipulation.sol [Line: 142](../tests/contract-playground/src/StateVariablesManipulation.sol#L142) + + ```solidity + manipulateHelper(p1); + ``` + - Found in src/UncheckedReturn.sol [Line: 14](../tests/contract-playground/src/UncheckedReturn.sol#L14) ```solidity @@ -2241,6 +2292,23 @@ Do not rely on the order or successful execution of retryable tickets. Functions +## H-40: Constant functions changing state + +Function is declared constant/view but it changes state. Ensure that the attributes of contract compiled prior to 0.5 are correct. + +
1 Found Instances + + +- Found in src/ConstFuncChangeState.sol [Line: 8](../tests/contract-playground/src/ConstFuncChangeState.sol#L8) + + ```solidity + function changeState() public view returns (uint) { + ``` + +
+ + + # Low Issues ## L-1: Centralization Risk for trusted owners @@ -2491,7 +2559,7 @@ ERC20 functions may not behave as expected. For example: return values are not a Consider using a specific version of Solidity in your contracts instead of a wide version. For example, instead of `pragma solidity ^0.8.0;`, use `pragma solidity 0.8.0;` -
30 Found Instances +
32 Found Instances - Found in src/BuiltinSymbolShadow.sol [Line: 2](../tests/contract-playground/src/BuiltinSymbolShadow.sol#L2) @@ -2506,6 +2574,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity ^0.4.0; ``` +- Found in src/ConstFuncChangeState.sol [Line: 2](../tests/contract-playground/src/ConstFuncChangeState.sol#L2) + + ```solidity + pragma solidity ^0.4.0; + ``` + - Found in src/ConstantFuncsAssembly.sol [Line: 2](../tests/contract-playground/src/ConstantFuncsAssembly.sol#L2) ```solidity @@ -2608,6 +2682,12 @@ Consider using a specific version of Solidity in your contracts instead of a wid pragma solidity ^0.4.0; ``` +- Found in src/StateVariablesManipulation.sol [Line: 2](../tests/contract-playground/src/StateVariablesManipulation.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/TautologyOrContradiction.sol [Line: 2](../tests/contract-playground/src/TautologyOrContradiction.sol#L2) ```solidity @@ -2682,7 +2762,7 @@ Consider using a specific version of Solidity in your contracts instead of a wid Check for `address(0)` when assigning values to address state variables. -
8 Found Instances +
9 Found Instances - Found in src/ArbitraryTransferFrom.sol [Line: 12](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L12) @@ -2709,6 +2789,12 @@ Check for `address(0)` when assigning values to address state variables. addr = newAddr; ``` +- Found in src/StateVariablesManipulation.sol [Line: 56](../tests/contract-playground/src/StateVariablesManipulation.sol#L56) + + ```solidity + simpleAddress = d; + ``` + - Found in src/ZeroAddressCheck.sol [Line: 43](../tests/contract-playground/src/ZeroAddressCheck.sol#L43) ```solidity @@ -2741,7 +2827,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. -
41 Found Instances +
44 Found Instances - Found in src/ArbitraryTransferFrom.sol [Line: 28](../tests/contract-playground/src/ArbitraryTransferFrom.sol#L28) @@ -2762,6 +2848,24 @@ Instead of marking a function as `public`, consider marking it as `external` if function assert(bool condition) public {} ``` +- Found in src/ConstFuncChangeState.sol [Line: 8](../tests/contract-playground/src/ConstFuncChangeState.sol#L8) + + ```solidity + function changeState() public view returns (uint) { + ``` + +- Found in src/ConstFuncChangeState.sol [Line: 14](../tests/contract-playground/src/ConstFuncChangeState.sol#L14) + + ```solidity + function changeState2() public returns (uint) { + ``` + +- Found in src/ConstFuncChangeState.sol [Line: 20](../tests/contract-playground/src/ConstFuncChangeState.sol#L20) + + ```solidity + function dontChangeState() public view returns (uint) { + ``` + - Found in src/ContractLocksEther.sol [Line: 20](../tests/contract-playground/src/ContractLocksEther.sol#L20) ```solidity @@ -2998,7 +3102,7 @@ Instead of marking a function as `public`, consider marking it as `external` if If the same constant literal value is used multiple times, create a constant state variable and reference it throughout the contract. -
52 Found Instances +
64 Found Instances - Found in src/BooleanEquality.sol [Line: 6](../tests/contract-playground/src/BooleanEquality.sol#L6) @@ -3241,6 +3345,78 @@ If the same constant literal value is used multiple times, create a constant sta for (uint256 id = 0; id < 10; ++id) { ``` +- Found in src/StateVariablesManipulation.sol [Line: 121](../tests/contract-playground/src/StateVariablesManipulation.sol#L121) + + ```solidity + person = Person("Spiderman", 21); + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 123](../tests/contract-playground/src/StateVariablesManipulation.sol#L123) + + ```solidity + person2.age = 21; + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 192](../tests/contract-playground/src/StateVariablesManipulation.sol#L192) + + ```solidity + p.age = 200; + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 202](../tests/contract-playground/src/StateVariablesManipulation.sol#L202) + + ```solidity + p2.age = 200; + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 233](../tests/contract-playground/src/StateVariablesManipulation.sol#L233) + + ```solidity + uint256[5][1] memory m_allAges; + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 237](../tests/contract-playground/src/StateVariablesManipulation.sol#L237) + + ```solidity + m_person = Person("Spiderman", 21); + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 239](../tests/contract-playground/src/StateVariablesManipulation.sol#L239) + + ```solidity + m_person2.age = 21; + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 244](../tests/contract-playground/src/StateVariablesManipulation.sol#L244) + + ```solidity + Person memory m_dummy = Person("Spiderman", 21); + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 258](../tests/contract-playground/src/StateVariablesManipulation.sol#L258) + + ```solidity + uint256[5] storage ageRef1 = allAges[0]; + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 264](../tests/contract-playground/src/StateVariablesManipulation.sol#L264) + + ```solidity + uint256[5] storage ageRef2 = allAges[0]; + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 333](../tests/contract-playground/src/StateVariablesManipulation.sol#L333) + + ```solidity + p.some.push(102); + ``` + +- Found in src/StateVariablesManipulation.sol [Line: 339](../tests/contract-playground/src/StateVariablesManipulation.sol#L339) + + ```solidity + that.push(102); + ``` + - Found in src/UncheckedReturn.sol [Line: 27](../tests/contract-playground/src/UncheckedReturn.sol#L27) ```solidity @@ -3587,7 +3763,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. -
36 Found Instances +
37 Found Instances - Found in src/AdminContract.sol [Line: 2](../tests/contract-playground/src/AdminContract.sol#L2) @@ -3692,6 +3868,12 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai pragma solidity 0.8.20; ``` +- Found in src/StateVariablesManipulation.sol [Line: 2](../tests/contract-playground/src/StateVariablesManipulation.sol#L2) + + ```solidity + pragma solidity ^0.8.0; + ``` + - Found in src/StorageConditionals.sol [Line: 2](../tests/contract-playground/src/StorageConditionals.sol#L2) ```solidity diff --git a/reports/report.sarif b/reports/report.sarif index 348641599..5319d9cb9 100644 --- a/reports/report.sarif +++ b/reports/report.sarif @@ -1847,6 +1847,83 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 10, + "byteOffset": 184 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 9, + "byteOffset": 214 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 10, + "byteOffset": 241 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 13, + "byteOffset": 272 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 314 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 12, + "byteOffset": 354 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 11, + "byteOffset": 385 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -2608,6 +2685,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 20, + "byteOffset": 4146 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3314,6 +3402,26 @@ }, "ruleId": "out-of-order-retryable" }, + { + "level": "warning", + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ConstFuncChangeState.sol" + }, + "region": { + "byteLength": 112, + "byteOffset": 173 + } + } + } + ], + "message": { + "text": "Function is declared constant/view but it changes state. Ensure that the attributes of contract compiled prior to 0.5 are correct." + }, + "ruleId": "constant-function-changing-state" + }, { "level": "note", "locations": [ @@ -3725,6 +3833,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ConstFuncChangeState.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -3912,6 +4031,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -4086,6 +4216,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 17, + "byteOffset": 1657 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -4172,6 +4313,39 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ConstFuncChangeState.sol" + }, + "region": { + "byteLength": 112, + "byteOffset": 173 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ConstFuncChangeState.sol" + }, + "region": { + "byteLength": 108, + "byteOffset": 339 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/ConstFuncChangeState.sol" + }, + "region": { + "byteLength": 89, + "byteOffset": 517 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -5072,6 +5246,138 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 3471 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 3542 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 5408 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 5621 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 1, + "byteOffset": 6372 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 6468 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 6543 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 2, + "byteOffset": 6704 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 1, + "byteOffset": 7236 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 1, + "byteOffset": 7497 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 9506 + } + } + }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 3, + "byteOffset": 9671 + } + } + }, { "physicalLocation": { "artifactLocation": { @@ -5854,6 +6160,17 @@ } } }, + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/StateVariablesManipulation.sol" + }, + "region": { + "byteLength": 23, + "byteOffset": 32 + } + } + }, { "physicalLocation": { "artifactLocation": { diff --git a/tests/contract-playground/src/ConstFuncChangeState.sol b/tests/contract-playground/src/ConstFuncChangeState.sol new file mode 100644 index 000000000..3fa0283bb --- /dev/null +++ b/tests/contract-playground/src/ConstFuncChangeState.sol @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.4.0; + +contract ConstantFunctionChangeState { + uint counter; + + // BAD (it is declared as view but changes state) + function changeState() public view returns (uint) { + counter = counter + 1; + return counter; + } + + // GOOD (because it's not declared as view) + function changeState2() public returns (uint) { + counter = counter + 1; + return counter; + } + + // GOOD (it's declared as view and it doesn't change state) + function dontChangeState() public view returns (uint) { + return counter + 1; + } +} diff --git a/tests/contract-playground/src/StateVariablesManipulation.sol b/tests/contract-playground/src/StateVariablesManipulation.sol new file mode 100644 index 000000000..743bffb79 --- /dev/null +++ b/tests/contract-playground/src/StateVariablesManipulation.sol @@ -0,0 +1,356 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +/***** ASSIGNMENT OPERATIONS *****/ + +contract NoStateVarManipulationExample { + // Simple state variables + uint256 public simpleUint; + int256 public simpleInt; + bool public simpleBool; + address public simpleAddress; + address payable public simplePayableAddress; + string public simpleString; + bytes public simpleBytes; + + // There are NO state variables assigned in this function + function dontManipulateStateVar() external view { + // VariableDeclaration (initialize with values) + uint256 a = simpleUint * 100; + int256 b = simpleInt * int256(a); + bool c = simpleBool; + bytes memory g = simpleBytes; + + // VariableDeclarationStatements (initialize) + address d; + address payable e; + string memory f; + + // Assignments + d = simpleAddress; + e = simplePayableAddress; + f = simpleString; + } +} + +// This contract should catch a total of 7 state variables manipulated +contract SimpleStateVarManipulationExample { + // Simple state variables (2 of them assigned to here) + uint256 public simpleUint; + int256 public simpleInt = 100; + bool public simpleBool = true; + address public simpleAddress; + address payable public simplePayableAddress; + string public simpleString; + bytes public simpleBytes; + + // Remaining 5 state variables manipulated in this function + function manipulateStateVarDirectly( + uint256 a, + address d, + address payable e, + string calldata f, + bytes calldata g + ) external { + simpleUint = a; + simpleAddress = d; + simplePayableAddress = e; + simpleString = f; + simpleBytes = g; + } + + function readSimpleStateVars() external { + uint256 _a = simpleUint; + int256 _b; + _b = simpleInt; + } +} + +// This contract should catch 4 state variables being assigned +contract FixedSizeArraysAssignmentExample { + uint256[5] public directlyAssignMe; + uint256[5] public assignToMeNow = [1, 4, 5, 8, 9]; // 1 state var assigned here + uint256[5] public indexMeToAssign; + uint256[5] public indexMeTwiceToAssign; + + // This shouldn't be caught! (it's not being manipulated) + uint256[5] public dummy; + + // 1 State variable is assigned here + function manipulateDirectly(uint256[5] calldata _values) external { + directlyAssignMe = _values; + } + + // 2 more state variables assigned here + function manipulateViaIndexAccess() external { + // 1st - indexMeToAssign + indexMeToAssign[0] = directlyAssignMe[0] + directlyAssignMe[1]; + indexMeToAssign[1] = dummy[0] + dummy[1]; + + // 2nd - indexMeTwiceToAssign + indexMeTwiceToAssign[dummy[dummy[0]]] = directlyAssignMe[3]; + } +} + +contract StructPlusFixedArrayAssignmentExample { + // Struct example + struct Person { + string name; + uint256 age; + } + + using SVManipulationLibrary for Person; + + // Simple Assignments + Person public person; + Person public person2; + uint256[5][1] public allAges; + + // Complex assignments + Person public person3; + Person[5][1] public persons; + + // More Complex Assignment + Person[5][1] public personsUltimate; + + // This is not manipulated + Person public dummy; + + // 3 state vars manipulated here + function manipulateStateVariables() external { + person = Person("Spiderman", 21); + allAges[person.age][0] = dummy.age; + person2.age = 21; + } + + function manipulateStateVariables2() external { + Person storage ptr_person = person3; + ptr_person.name = "Changed"; + + ptr_person = persons[dummy.age][0]; + ptr_person.name = "Changed"; + } + + function manipulateStateVariables3() external { + // This is VariableDeclarationStatement, not an Assigment + Person storage ptr_person3 = person3; + } + + function manipulateStateVariables4() external { + // This is VariableDeclarationStatement, not an Assigment + (Person storage p1, ) = manipulateHelper(personsUltimate[0][0]); + manipulateHelper(p1); + p1.manipulateLib(); + } + + function manipulateHelper( + Person storage h + ) internal returns (Person storage, Person storage) { + h.name = "H"; + Person storage p1 = person3; + p1.age = 130; + return (p1, p1); + } + + function manipulateStateVariables5() external view { + Person storage p1; + p1 = person3; + } + + function manipulateStateVariables6() external { + // here, p1 and p2 are manipulated but not p3 and p4 + Person storage p1; + Person storage p2; + Person storage p3; + Person storage p4; + // These are assigments with tuple expression on lhs and rhs + (p3, p4, p1, p2, (p1.age, p2.age)) = ( + person3, + person3, + person3, + person3, + (1398, 1399) + ); + } + + function manipulateStateVariables7() external { + person.age += 1; + person2.age -= 10; + person3.age *= 100; + } + + function manipulateStateVariables8() external { + person.age++; + person2.age--; + } +} + +library SVManipulationLibrary { + function manipulateLib( + StructPlusFixedArrayAssignmentExample.Person storage p + ) internal { + p.age = 200; + } + + function manipulateLib2() + internal + returns (StructPlusFixedArrayAssignmentExample.Person storage p2) + { + assembly { + p2.slot := 0x00 + } + p2.age = 200; + } + + function manipulateLib3() internal { + StructPlusFixedArrayAssignmentExample.Person storage p1; + StructPlusFixedArrayAssignmentExample.Person storage p2; + assembly { + p1.slot := 0x00 + p2.slot := 0x01 + } + (p1.name, p2.age) = ("Hello", 400); + } +} + +contract NoStructPlusFixedArrayAssignmentExample { + // Struct example + struct Person { + string name; + uint256 age; + } + + Person public person; + Person public person2; + uint256[5][1] public allAges; + + Person public dummy; + + // NO state vars manipulated here + function dontManipulateStateVariables() external { + Person memory m_person; + Person memory m_person2; + uint256[5][1] memory m_allAges; + + Person memory m_dummy; + + m_person = Person("Spiderman", 21); + m_allAges[person.age][0] = dummy.age; + m_person2.age = 21; + } + + // NO state vars manipulated here + function dontManipulateStateVariablesPart2() external { + Person memory m_dummy = Person("Spiderman", 21); + } + + // NO state vars manipulated here + function dontManipulateStateVariablesPart3() external { + uint256 theAge = person.age; + uint256 theAge2; + theAge2 = person.age; + } + + // Only person2 state variable is read here. Also it's "directly read" (aka not through storage pointers) + // Other initializations are merely just references. + function dontManipulateStateVariablesPart4() external { + (uint256 theAge3, uint256 theAge4) = (340, bytes(person2.name).length); + uint256[5] storage ageRef1 = allAges[0]; + Person storage dr = dummy; + } + + // Only `v` is the one that's causing a read. Also, this is "indirectly" read (through a storage pointer) + function dontManipulateStateVariablesPart5() external { + uint256[5] storage ageRef2 = allAges[0]; + uint256 v = ageRef2[3]; + } +} + +/***** DELETE OPERATIONS *****/ + +// This contract should catch 3 state variables undergoing a delete operation +contract FixedSizeArraysDeletionExample { + uint256[5] public directlyDeleteMe; + uint256[5] public indexMeToDelete; + uint256[5] public indexMeTwiceToDelete; + + // This shouldn't be caught! (it's not being manipulated) + uint256[5] public dummy; + + // 1 State variable is deleted here + function manipulateDirectly() external { + delete directlyDeleteMe; + } + + // 2 more state variables deleted here + function manipulateViaIndexAccess() external { + delete indexMeToDelete[0]; + delete indexMeTwiceToDelete[dummy[dummy[0]]]; + } +} + +/***** PUSH OPERATIONS *****/ + +// This contract should catch 3 state variables undergoing a delete operation +contract DynamicArraysPushExample { + // Struct example + struct Person { + string name; + uint256 age; + uint256[] some; + } + + Person[] public directlyPushMe; + Person[][5] public indexMeToPush; + Person[][] public indexMeTwiceToPush; + + uint256[] public indexMeToPushDArray; + Person public p; + + // This shouldn't be caught! (it's not being manipulated) + uint256[5] public dummy; + + // 1 State variable is pushed to here + function manipulateDirectly() external { + directlyPushMe.push( + Person({name: "dfsf", age: 610, some: indexMeToPushDArray}) + ); + } + + // 3 more state variables pushed to here + function manipulateViaIndexAccess() external { + indexMeToPush[dummy.length].push( + Person({name: "dfsf", age: 710, some: indexMeToPushDArray}) + ); + indexMeTwiceToPush[dummy.length].push( + Person({name: "dfsf", age: 810, some: indexMeToPushDArray}) + ); + indexMeToPushDArray.push(12309); + } + + // 1 state variable pushed to here + function manipulateViaMemberAccess() external { + p.some.push(102); + } + + // 1 storage pointer pushed to here + function manipulateViaMemberAccess2() external { + uint256[] storage that = p.some; + that.push(102); + } +} + +contract DynamicMappingsArrayPushExample { + // Struct example + struct Person { + string name; + uint256 age; + uint256[] some; + } + + mapping(uint256 => Person) myMap; + + function add(uint256 i) external { + myMap[i].some.push(304); + } +}