diff --git a/README.md b/README.md index 8a96521..2e42b3e 100644 --- a/README.md +++ b/README.md @@ -96,7 +96,7 @@ Num | Detector | What it Detects | Impact | Confidence | Cairo 10 | `reentrancy-benign` | Detect when a storage variable is written after an external call but not read before | Low | Medium | 1 & 2 11 | `reentrancy-events` | Detect when an event is emitted after an external call leading to out-of-order events | Low | Medium | 1 & 2 12 | `dead-code` | Private functions never used | Low | Medium | 1 & 2 -13 | `array-use-after-pop-front` | Detect use of an array after removing element(s) | Low | Medium | 1 & 2 +13 | `use-after-pop-front` | Detect use of an array or a span after removing element(s) | Low | Medium | 1 & 2 The Cairo column represent the compiler version(s) for which the detector is valid. diff --git a/src/detectors/mod.rs b/src/detectors/mod.rs index ce0fa39..db3aabd 100644 --- a/src/detectors/mod.rs +++ b/src/detectors/mod.rs @@ -1,6 +1,5 @@ use self::detector::Detector; -pub mod array_use_after_pop_front; pub mod controlled_library_call; pub mod dead_code; pub mod detector; @@ -13,10 +12,11 @@ pub mod unchecked_l1_handler_from; pub mod unused_arguments; pub mod unused_events; pub mod unused_return; +pub mod use_after_pop_front; pub fn get_detectors() -> Vec> { vec![ - Box::::default(), + Box::::default(), Box::::default(), Box::::default(), Box::::default(), diff --git a/src/detectors/array_use_after_pop_front.rs b/src/detectors/use_after_pop_front.rs similarity index 68% rename from src/detectors/array_use_after_pop_front.rs rename to src/detectors/use_after_pop_front.rs index 52d213f..be7597e 100644 --- a/src/detectors/array_use_after_pop_front.rs +++ b/src/detectors/use_after_pop_front.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; + use super::detector::{Confidence, Detector, Impact, Result}; use crate::analysis::taint::WrapperVariable; use crate::core::compilation_unit::CompilationUnit; @@ -7,18 +9,31 @@ use cairo_lang_sierra::extensions::array::ArrayConcreteLibfunc; use cairo_lang_sierra::extensions::core::{CoreConcreteLibfunc, CoreTypeConcrete}; use cairo_lang_sierra::program::{GenStatement, Statement as SierraStatement}; use fxhash::FxHashSet; -use std::collections::HashSet; #[derive(Default)] -pub struct ArrayUseAfterPopFront {} +pub struct UseAfterPopFront {} -impl Detector for ArrayUseAfterPopFront { +enum CollectionType { + Array, + Span, +} + +impl CollectionType { + fn is_array(&self) -> bool { + match self { + CollectionType::Array => true, + CollectionType::Span => false, + } + } +} + +impl Detector for UseAfterPopFront { fn name(&self) -> &str { - "array-use-after-pop-front" + "use-after-pop-front" } fn description(&self) -> &str { - "Detect use of an array after removing element(s)" + "Detect use of an array or a span after removing element(s)" } fn confidence(&self) -> Confidence { @@ -35,7 +50,7 @@ impl Detector for ArrayUseAfterPopFront { for compilation_unit in compilation_units.iter() { for function in compilation_unit.functions_user_defined() { - let pop_fronts: Vec<(usize, WrapperVariable)> = function + let pop_fronts: Vec<(usize, WrapperVariable, CollectionType)> = function .get_statements() .iter() .enumerate() @@ -51,8 +66,23 @@ impl Detector for ArrayUseAfterPopFront { Some(( index, WrapperVariable::new(function.name(), invoc.args[0].id), + CollectionType::Array, )) } + CoreConcreteLibfunc::Array( + ArrayConcreteLibfunc::SnapshotPopFront(_), + ) => Some(( + index, + WrapperVariable::new(function.name(), invoc.args[0].id), + CollectionType::Span, + )), + CoreConcreteLibfunc::Array( + ArrayConcreteLibfunc::SnapshotPopBack(_), + ) => Some(( + index, + WrapperVariable::new(function.name(), invoc.args[0].id), + CollectionType::Span, + )), _ => None, } } @@ -60,22 +90,31 @@ impl Detector for ArrayUseAfterPopFront { }) .collect(); - let bad_array_used: Vec<&(usize, WrapperVariable)> = pop_fronts - .iter() - .filter(|(index, bad_array)| { - self.is_array_used_after_pop_front( - compilation_unit, - function, - bad_array, - *index, - ) - }) - .collect(); + // Required to silence clippy too-complex-type warning + type BadCollectionType<'a, 'b> = Vec<(&'a WrapperVariable, &'b CollectionType)>; + + let (bad_array_used, bad_span_used): (BadCollectionType, BadCollectionType) = + pop_fronts + .iter() + .filter_map(|(index, bad_array, collection_type)| { + let is_used = self.is_used_after_pop_front( + compilation_unit, + function, + bad_array, + *index, + ); + if is_used { + Some((bad_array, collection_type)) + } else { + None + } + }) + .partition(|(_, collection_type)| collection_type.is_array()); if !bad_array_used.is_empty() { let array_ids = bad_array_used .iter() - .map(|f| f.1.variable()) + .map(|f| f.0.variable()) .collect::>(); let message = match array_ids.len() { 1 => format!( @@ -96,6 +135,31 @@ impl Detector for ArrayUseAfterPopFront { message, }); } + + if !bad_span_used.is_empty() { + let span_ids = bad_span_used + .iter() + .map(|f| f.0.variable()) + .collect::>(); + let message = match span_ids.len() { + 1 => format!( + "The span {:?} is used after removing elements from it in the function {}", + span_ids, + &function.name() + ), + _ => format!( + "The spans {:?} are used after removing elements from them in the function {}", + span_ids, + &function.name() + ) + }; + results.insert(Result { + name: self.name().to_string(), + impact: self.impact(), + confidence: self.confidence(), + message, + }); + } } } @@ -103,8 +167,8 @@ impl Detector for ArrayUseAfterPopFront { } } -impl ArrayUseAfterPopFront { - fn is_array_used_after_pop_front( +impl UseAfterPopFront { + fn is_used_after_pop_front( &self, compilation_unit: &CompilationUnit, function: &Function, @@ -204,8 +268,8 @@ impl ArrayUseAfterPopFront { // check if the bad array is returned by the function // if yes then check if its a loop function - // if not then its clear usage of a bad array // if yes then we need to check its caller to see if it uses the bad array + // if not then its clear usage of a bad array fn check_returns( &self, compilation_unit: &CompilationUnit, @@ -230,7 +294,7 @@ impl ArrayUseAfterPopFront { // We can not find the array from the return types of the function // We assume that the array is returned at the same index as it was taken on let return_array_indices: Vec = function - .params() + .params_all() .enumerate() .filter_map(|(i, param)| { let param_type = compilation_unit @@ -238,10 +302,11 @@ impl ArrayUseAfterPopFront { .get_type(¶m.ty) .expect("Type not found in the registry"); - if let CoreTypeConcrete::Array(_) = param_type { - return Some(i); + match param_type { + CoreTypeConcrete::Array(_) => Some(i), + span if self.is_core_type_concrete_span(compilation_unit, span) => Some(i), + _ => None, } - None }) .collect(); @@ -310,10 +375,11 @@ impl ArrayUseAfterPopFront { .get_type(r) .expect("Type not found in the registry"); - if let CoreTypeConcrete::Array(_) = return_type { - return Some(i); + match return_type { + CoreTypeConcrete::Array(_) => Some(i), + span if self.is_core_type_concrete_span(compilation_unit, span) => Some(i), + _ => None, } - None }) .collect(); @@ -322,8 +388,9 @@ impl ArrayUseAfterPopFront { return false; } - // Not sure if it is required because taint analysis adds all the arguments as - // tainters of the all the return values. + // It is not required because taint analysis adds all the arugments as + // tainters of the all the return values. Added it in case the taint + // analysis is improved later on to be more granular. let returned_bad_arrays: Vec = function .get_statements() .iter() @@ -342,4 +409,37 @@ impl ArrayUseAfterPopFront { !returned_bad_arrays.is_empty() } + + // The Span is not a Core Sierra type, it is defined in the corelib as a Struct + // Therefore we can not match it against CoreTypeConcrete::Span directly + fn is_core_type_concrete_span( + &self, + compilation_unit: &CompilationUnit, + maybe_span: &CoreTypeConcrete, + ) -> bool { + match maybe_span { + CoreTypeConcrete::Struct(struct_type) => match &struct_type.members[..] { + [maybe_snapshot, ..] => { + let maybe_snapshot_type = compilation_unit + .registry() + .get_type(maybe_snapshot) + .expect("Type not found in the registry"); + + match maybe_snapshot_type { + CoreTypeConcrete::Snapshot(maybe_array) => { + let maybe_array_type = compilation_unit + .registry() + .get_type(&maybe_array.ty) + .expect("Type not found in the registry"); + + matches!(maybe_array_type, CoreTypeConcrete::Array(_)) + } + _ => false, + } + } + _ => false, + }, + _ => false, + } + } } diff --git a/tests/detectors/array_use_after_pop_front.cairo b/tests/detectors/array_use_after_pop_front.cairo deleted file mode 100644 index 7612a0f..0000000 --- a/tests/detectors/array_use_after_pop_front.cairo +++ /dev/null @@ -1,134 +0,0 @@ -#[starknet::interface] -trait IAnotherContract { - fn foo(ref self: T, a: Array) -> u128; -} - -#[starknet::contract] -mod ArrayUseAfterPopFront { - use super::{ - IAnotherContractDispatcherTrait, - IAnotherContractDispatcher, - IAnotherContractLibraryDispatcher - }; - use array::ArrayTrait; - use starknet::ContractAddress; - - #[storage] - struct Storage {} - - #[event] - #[derive(Drop, starknet::Event)] - enum Event { - ArrayEvent: ArrayEvent - } - - #[derive(Drop, starknet::Event)] - struct ArrayEvent { - arr: Array - } - - #[external(v0)] - fn bad(ref self: ContractState) { - let mut arr = ArrayTrait::::new(); - arr.append(1); - - let _b = arr.pop_front(); - self.emit(ArrayEvent{ arr}); - } - - #[external(v0)] - fn bad_one_branch(ref self: ContractState) { - let mut arr = ArrayTrait::::new(); - arr.append(1); - - match arr.pop_front() { - Option::Some(_val) => { - self.emit(ArrayEvent{ arr }); - () - }, - Option::None(_) => () - }; - } - - #[external(v0)] - fn bad_loop(ref self: ContractState) { - let mut arr = ArrayTrait::::new(); - arr.append(1); - - loop { - match arr.pop_front() { - Option::Some(_val) => (), - Option::None(_) => { - break (); - } - }; - }; - - self.emit(ArrayEvent{ arr }); - } - - #[external(v0)] - fn bad_return(ref self: ContractState) -> Array { - let mut arr = ArrayTrait::::new(); - arr.append(1); - - let _b = arr.pop_front(); - return arr; - } - - #[external(v0)] - fn bad_library_call(ref self: ContractState) -> u128 { - let mut arr = ArrayTrait::::new(); - arr.append(1); - - let _b = arr.pop_front(); - return IAnotherContractLibraryDispatcher { class_hash: starknet::class_hash_const::<0>() }.foo(arr); - } - - #[external(v0)] - fn bad_external_call(ref self: ContractState) -> u128 { - let mut arr = ArrayTrait::::new(); - arr.append(1); - - let _b = arr.pop_front(); - return IAnotherContractDispatcher { contract_address: starknet::contract_address_const::<0>() }.foo(arr); - } - - #[external(v0)] - fn bad_multiple_arrays(ref self: ContractState) { - let mut arr = ArrayTrait::::new(); - arr.append(1); - - let _b = arr.pop_front(); - self.emit(ArrayEvent{ arr }); - - let mut arr1 = ArrayTrait::::new(); - arr1.append(1); - - let _b1 = arr1.pop_front(); - self.emit(ArrayEvent{ arr: arr1 }); - } - - #[external(v0)] - fn good(self: @ContractState) { - let mut arr = ArrayTrait::::new(); - arr.append(1); - - let _a = arr.pop_front(); - } - - #[external(v0)] - fn good_loop(self: @ContractState) { - let mut arr = ArrayTrait::::new(); - arr.append(1); - - loop { - match arr.pop_front() { - Option::Some(_val) => (), - Option::None(_) => { - break (); - } - }; - }; - } -} \ No newline at end of file diff --git a/tests/detectors/use_after_pop_front.cairo b/tests/detectors/use_after_pop_front.cairo new file mode 100644 index 0000000..bf747b1 --- /dev/null +++ b/tests/detectors/use_after_pop_front.cairo @@ -0,0 +1,257 @@ +#[starknet::interface] +trait IAnotherContract { + fn foo(ref self: T, a: Array) -> u128; + fn bar(ref self: T, a: Span) -> u128; +} + +#[starknet::contract] +mod UseAfterPopFront { + use super::{ + IAnotherContractDispatcherTrait, + IAnotherContractDispatcher, + IAnotherContractLibraryDispatcher + }; + use array::ArrayTrait; + use starknet::ContractAddress; + + #[storage] + struct Storage {} + + #[event] + #[derive(Drop, starknet::Event)] + enum Event { + ArrayEvent: ArrayEvent, + SpanEvent: SpanEvent + } + + #[derive(Drop, starknet::Event)] + struct ArrayEvent { + arr: Array + } + + #[derive(Drop, starknet::Event)] + struct SpanEvent { + span: Span + } + + #[external(v0)] + fn bad(ref self: ContractState) { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let _b = arr.pop_front(); + self.emit(ArrayEvent{ arr }); + } + + #[external(v0)] + fn bad_one_branch(ref self: ContractState) { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + match arr.pop_front() { + Option::Some(_val) => { + self.emit(ArrayEvent{ arr }); + () + }, + Option::None(_) => () + }; + } + + #[external(v0)] + fn bad_loop(ref self: ContractState) { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + loop { + match arr.pop_front() { + Option::Some(_val) => (), + Option::None(_) => { + break (); + } + }; + }; + + self.emit(ArrayEvent{ arr }); + } + + #[external(v0)] + fn bad_return(ref self: ContractState) -> Array { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let _b = arr.pop_front(); + return arr; + } + + #[external(v0)] + fn bad_library_call(ref self: ContractState) -> u128 { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let _b = arr.pop_front(); + return IAnotherContractLibraryDispatcher { class_hash: starknet::class_hash_const::<0>() }.foo(arr); + } + + #[external(v0)] + fn bad_external_call(ref self: ContractState) -> u128 { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let _b = arr.pop_front(); + return IAnotherContractDispatcher { contract_address: starknet::contract_address_const::<0>() }.foo(arr); + } + + #[external(v0)] + fn bad_multiple_arrays(ref self: ContractState) { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let _b = arr.pop_front(); + self.emit(ArrayEvent{ arr }); + + let mut arr1 = ArrayTrait::::new(); + arr1.append(1); + + let _b1 = arr1.pop_front(); + self.emit(ArrayEvent{ arr: arr1 }); + } + + #[external(v0)] + fn good(self: @ContractState) { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let _a = arr.pop_front(); + } + + #[external(v0)] + fn good_loop(self: @ContractState) { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + loop { + match arr.pop_front() { + Option::Some(_val) => (), + Option::None(_) => { + break (); + } + }; + }; + } + + // Span test functions + #[external(v0)] + fn bad_span(ref self: ContractState) { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let mut span = arr.span(); + let _b = span.pop_front(); + self.emit(SpanEvent{ span }); + } + + #[external(v0)] + fn bad_one_branch_span(ref self: ContractState) { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let mut span = arr.span(); + match span.pop_front() { + Option::Some(_val) => { + self.emit(SpanEvent{ span }); + () + }, + Option::None(_) => () + }; + } + + #[external(v0)] + fn bad_loop_span(ref self: ContractState) { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let mut span = arr.span(); + loop { + match span.pop_front() { + Option::Some(_val) => (), + Option::None(_) => { + break (); + } + }; + }; + + self.emit(SpanEvent{ span }); + } + + #[external(v0)] + fn bad_return_span(ref self: ContractState) -> Span { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let mut span = arr.span(); + let _b = span.pop_front(); + return span; + } + + #[external(v0)] + fn bad_library_call_span(ref self: ContractState) -> u128 { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let mut span = arr.span(); + let _b = span.pop_front(); + return IAnotherContractLibraryDispatcher { class_hash: starknet::class_hash_const::<0>() }.bar(span); + } + + #[external(v0)] + fn bad_external_call_span(ref self: ContractState) -> u128 { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let mut span = arr.span(); + let _b = span.pop_front(); + return IAnotherContractDispatcher { contract_address: starknet::contract_address_const::<0>() }.bar(span); + } + + #[external(v0)] + fn bad_multiple_arrays_span(ref self: ContractState) { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let mut span = arr.span(); + let _b = span.pop_front(); + self.emit(SpanEvent{ span }); + + let mut arr1 = ArrayTrait::::new(); + arr1.append(1); + + let mut span1 = arr.span(); + let _b1 = span1.pop_front(); + self.emit(SpanEvent{ span: span1 }); + } + + #[external(v0)] + fn good_span(self: @ContractState) { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let mut span = arr.span(); + let _a = span.pop_front(); + } + + #[external(v0)] + fn good_loop_span(self: @ContractState) { + let mut arr = ArrayTrait::::new(); + arr.append(1); + + let mut span = arr.span(); + loop { + match span.pop_front() { + Option::Some(_val) => (), + Option::None(_) => { + break (); + } + }; + }; + } +} \ No newline at end of file diff --git a/tests/snapshots/integration_tests__detectors@array_use_after_pop_front.cairo.snap b/tests/snapshots/integration_tests__detectors@array_use_after_pop_front.cairo.snap deleted file mode 100644 index 156ca89..0000000 --- a/tests/snapshots/integration_tests__detectors@array_use_after_pop_front.cairo.snap +++ /dev/null @@ -1,49 +0,0 @@ ---- -source: tests/integration_tests.rs -expression: results -input_file: tests/detectors/array_use_after_pop_front.cairo ---- -[ - Result { - impact: Low, - name: "array-use-after-pop-front", - confidence: Medium, - message: "The array [2] is used after removing elements from it in the function array_use_after_pop_front::array_use_after_pop_front::ArrayUseAfterPopFront::bad_loop[expr11]", - }, - Result { - impact: Low, - name: "array-use-after-pop-front", - confidence: Medium, - message: "The array [3] is used after removing elements from it in the function array_use_after_pop_front::array_use_after_pop_front::ArrayUseAfterPopFront::bad_return", - }, - Result { - impact: Low, - name: "array-use-after-pop-front", - confidence: Medium, - message: "The array [6] is used after removing elements from it in the function array_use_after_pop_front::array_use_after_pop_front::ArrayUseAfterPopFront::bad", - }, - Result { - impact: Low, - name: "array-use-after-pop-front", - confidence: Medium, - message: "The array [6] is used after removing elements from it in the function array_use_after_pop_front::array_use_after_pop_front::ArrayUseAfterPopFront::bad_external_call", - }, - Result { - impact: Low, - name: "array-use-after-pop-front", - confidence: Medium, - message: "The array [6] is used after removing elements from it in the function array_use_after_pop_front::array_use_after_pop_front::ArrayUseAfterPopFront::bad_library_call", - }, - Result { - impact: Low, - name: "array-use-after-pop-front", - confidence: Medium, - message: "The array [6] is used after removing elements from it in the function array_use_after_pop_front::array_use_after_pop_front::ArrayUseAfterPopFront::bad_one_branch", - }, - Result { - impact: Low, - name: "array-use-after-pop-front", - confidence: Medium, - message: "The arrays [6, 21] are used after removing elements from them in the function array_use_after_pop_front::array_use_after_pop_front::ArrayUseAfterPopFront::bad_multiple_arrays", - }, -] diff --git a/tests/snapshots/integration_tests__detectors@use_after_pop_front.cairo.snap b/tests/snapshots/integration_tests__detectors@use_after_pop_front.cairo.snap new file mode 100644 index 0000000..ffb8bdf --- /dev/null +++ b/tests/snapshots/integration_tests__detectors@use_after_pop_front.cairo.snap @@ -0,0 +1,91 @@ +--- +source: tests/integration_tests.rs +expression: results +input_file: tests/detectors/use_after_pop_front.cairo +--- +[ + Result { + impact: Low, + name: "use-after-pop-front", + confidence: Medium, + message: "The array [2] is used after removing elements from it in the function use_after_pop_front::use_after_pop_front::UseAfterPopFront::bad_loop[expr11]", + }, + Result { + impact: Low, + name: "use-after-pop-front", + confidence: Medium, + message: "The array [3] is used after removing elements from it in the function use_after_pop_front::use_after_pop_front::UseAfterPopFront::bad_return", + }, + Result { + impact: Low, + name: "use-after-pop-front", + confidence: Medium, + message: "The array [6] is used after removing elements from it in the function use_after_pop_front::use_after_pop_front::UseAfterPopFront::bad", + }, + Result { + impact: Low, + name: "use-after-pop-front", + confidence: Medium, + message: "The array [6] is used after removing elements from it in the function use_after_pop_front::use_after_pop_front::UseAfterPopFront::bad_external_call", + }, + Result { + impact: Low, + name: "use-after-pop-front", + confidence: Medium, + message: "The array [6] is used after removing elements from it in the function use_after_pop_front::use_after_pop_front::UseAfterPopFront::bad_library_call", + }, + Result { + impact: Low, + name: "use-after-pop-front", + confidence: Medium, + message: "The array [6] is used after removing elements from it in the function use_after_pop_front::use_after_pop_front::UseAfterPopFront::bad_one_branch", + }, + Result { + impact: Low, + name: "use-after-pop-front", + confidence: Medium, + message: "The arrays [6, 21] are used after removing elements from them in the function use_after_pop_front::use_after_pop_front::UseAfterPopFront::bad_multiple_arrays", + }, + Result { + impact: Low, + name: "use-after-pop-front", + confidence: Medium, + message: "The span [10] is used after removing elements from it in the function use_after_pop_front::use_after_pop_front::UseAfterPopFront::bad_external_call_span", + }, + Result { + impact: Low, + name: "use-after-pop-front", + confidence: Medium, + message: "The span [10] is used after removing elements from it in the function use_after_pop_front::use_after_pop_front::UseAfterPopFront::bad_library_call_span", + }, + Result { + impact: Low, + name: "use-after-pop-front", + confidence: Medium, + message: "The span [10] is used after removing elements from it in the function use_after_pop_front::use_after_pop_front::UseAfterPopFront::bad_one_branch_span", + }, + Result { + impact: Low, + name: "use-after-pop-front", + confidence: Medium, + message: "The span [10] is used after removing elements from it in the function use_after_pop_front::use_after_pop_front::UseAfterPopFront::bad_span", + }, + Result { + impact: Low, + name: "use-after-pop-front", + confidence: Medium, + message: "The span [7] is used after removing elements from it in the function use_after_pop_front::use_after_pop_front::UseAfterPopFront::bad_return_span", + }, + Result { + impact: Low, + name: "use-after-pop-front", + confidence: Medium, + message: "The span [8] is used after removing elements from it in the function use_after_pop_front::use_after_pop_front::UseAfterPopFront::bad_loop_span[expr14]", + }, + Result { + impact: Low, + name: "use-after-pop-front", + confidence: Medium, + message: "The spans [11, 39] are used after removing elements from them in the function use_after_pop_front::use_after_pop_front::UseAfterPopFront::bad_multiple_arrays_span", + }, +]