Add support for Span in the user-after-pop-front detector

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.
 

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<Box<dyn Detector>> {
     vec![
-        Box::<array_use_after_pop_front::ArrayUseAfterPopFront>::default(),
+        Box::<use_after_pop_front::UseAfterPopFront>::default(),
         Box::<controlled_library_call::ControlledLibraryCall>::default(),
         Box::<unused_events::UnusedEvents>::default(),
         Box::<dead_code::DeadCode>::default(),

src/detectors/array_use_after_pop_front.rs → 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,31 +66,55 @@ 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,
                             }
                         }
                         _ => None,
                     })
                     .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::<Vec<u64>>();
                     let message = match array_ids.len() {
                         1 => format!(
@@ -96,15 +135,40 @@ impl Detector for ArrayUseAfterPopFront {
                         message,
                     });
                 }
+
+                if !bad_span_used.is_empty() {
+                    let span_ids = bad_span_used
+                        .iter()
+                        .map(|f| f.0.variable())
+                        .collect::<Vec<u64>>();
+                    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,
+                    });
+                }
             }
         }
 
         results
     }
 }
 
-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,18 +294,19 @@ 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<usize> = function
-            .params()
+            .params_all()
             .enumerate()
             .filter_map(|(i, param)| {
                 let param_type = compilation_unit
                     .registry()
                     .get_type(&param.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<WrapperVariable> = 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,
+        }
+    }
 }