Detect use of felt252 operations with tainted parameters

Cargo.toml

@@ -17,8 +17,8 @@ smol_str = "0.2"
 num-integer = "0.1"
 termcolor = "1.2"
 graphviz-rust = "0.6.2"
+cairo-felt = "0.8.7"
 thiserror = "1.0.47"
-cairo-felt = "0.8.6"
 
 cairo-lang-compiler = { git = "https://github.com/starkware-libs/cairo.git", tag = "v2.2.0" }
 cairo-lang-defs = { git = "https://github.com/starkware-libs/cairo.git", tag = "v2.2.0" }

README.md

@@ -15,7 +15,7 @@ Caracal is a static analyzer tool over the SIERRA representation for Starknet sm
 Precompiled binaries are available on our [releases page](https://github.com/crytic/caracal/releases). If you are using Cairo compiler 1.x.x uses the binary v0.1.x otherwise if you are using the Cairo compiler 2.x.x uses v0.2.x.
 
 ### Building from source
-You need the Rust compiler and Cargo.  
+You need the Rust compiler and Cargo.
 Building from git:
 ```bash
 cargo install --git https://github.com/crytic/caracal --profile release --force
@@ -37,7 +37,7 @@ List printers:
 caracal printers
 ```
 ### Standalone
-To use with a standalone cairo file you need to pass the path to the [corelib](https://github.com/starkware-libs/cairo/tree/main/corelib) library either with the `--corelib` cli option or by setting the `CORELIB_PATH` environment variable.  
+To use with a standalone cairo file you need to pass the path to the [corelib](https://github.com/starkware-libs/cairo/tree/main/corelib) library either with the `--corelib` cli option or by setting the `CORELIB_PATH` environment variable.
 Run detectors:
 ```bash
 caracal detect path/file/to/analyze --corelib path/to/corelib/src
@@ -55,7 +55,7 @@ sierra = true
 [cairo]
 sierra-replace-ids = true
 ```
-Then pass the path to the directory where Scarb.toml resides.  
+Then pass the path to the directory where Scarb.toml resides.
 Run detectors:
 ```bash
 caracal detect path/to/dir
@@ -70,20 +70,21 @@ Num | Detector | What it Detects | Impact | Confidence | Cairo
 --- | --- | --- | --- | --- | ---
 1 | `controlled-library-call` | Library calls with a user controlled class hash | High | Medium | 1 & 2
 2 | `unchecked-l1-handler-from` | Detect L1 handlers without from address check | High | Medium | 1 & 2
-3 | `reentrancy` | Detect when a storage variable is read before an external call and written after | Medium | Medium | 1 & 2
-4 | `read-only-reentrancy` | Detect when a view function read a storage variable written after an external call | Medium | Medium | 1 & 2
-5 | `unused-events` | Events defined but not emitted | Medium | Medium | 1 & 2
-6 | `unused-return` | Unused return values | Medium | Medium | 1 & 2
-7 | `unenforced-view` | Function has view decorator but modifies state | Medium | Medium | 1
-8 | `unused-arguments` | Unused arguments | Low | Medium | 1 & 2
-9 | `reentrancy-benign` | Detect when a storage variable is written after an external call but not read before | Low | Medium | 1 & 2
-10 | `reentrancy-events` | Detect when an event is emitted after an external call leading to out-of-order events | Low | Medium | 1 & 2
-11 | `dead-code` | Private functions never used | Low | Medium | 1 & 2
+3 | `felt-252-overflow` | Detect user controlled operations with felt252 type, which is not overflow safe | High | Medium | 1 & 2
+4 | `reentrancy` | Detect when a storage variable is read before an external call and written after | Medium | Medium | 1 & 2
+5 | `read-only-reentrancy` | Detect when a view function read a storage variable written after an external call | Medium | Medium | 1 & 2
+6 | `unused-events` | Events defined but not emitted | Medium | Medium | 1 & 2
+7 | `unused-return` | Unused return values | Medium | Medium | 1 & 2
+8 | `unenforced-view` | Function has view decorator but modifies state | Medium | Medium | 1
+9 | `unused-arguments` | Unused arguments | Low | Medium | 1 & 2
+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
 
-The Cairo column represent the compiler version for which the detector is valid.
+The Cairo column represent the compiler version(s) for which the detector is valid.
 
 ## Printers
-- `cfg`: Export the CFG of each function in a .dot file
+- `cfg`: Export the CFG of each function to a .dot file
 - `callgraph`: Export function call graph to a .dot file
 
 ## How to contribute

src/compilation/utils/felt252_serde.rs

@@ -1,5 +1,6 @@
 // Taken from https://github.com/starkware-libs/cairo/blob/0a3e9dec15c2a853559d233247a253871e7bb35a/crates/cairo-lang-starknet/src/felt252_serde.rs
 // Removed the serialization process
+
 use crate::compilation::utils::felt252_vec_compression::decompress;
 use cairo_lang_sierra::extensions::starknet::interoperability::ContractAddressTryFromFelt252Libfunc;
 use cairo_lang_sierra::extensions::starknet::secp256::Secp256GetPointFromXLibfunc;

src/core/cfg.rs

@@ -57,7 +57,7 @@ impl CfgRegular {
         for (i, statement) in statements.iter().enumerate() {
             let current_pc = base_pc + i;
             match statement {
-                // Statement with a single branch. It's a statemnt that doesn't change the flow (Fallthrough) or an unconditional jump
+                // Statement with a single branch. It's a statement that doesn't change the flow (Fallthrough) or an unconditional jump
                 SierraStatement::Invocation(function) if function.branches.len() == 1 => {
                     match function.branches[0].target {
                         BranchTarget::Fallthrough => {

src/core/compilation_unit.rs

@@ -91,7 +91,7 @@ impl CompilationUnit {
             .iter()
             .filter(|f| matches!(f.ty(), Type::External | Type::L1Handler | Type::View))
         {
-            for param in external_function.params() {
+            for param in external_function.params().skip(1) {
                 parameters.insert(WrapperVariable::new(
                     external_function.name(),
                     param.id.clone(),
@@ -350,7 +350,19 @@ impl CompilationUnit {
                                 .collect();
 
                             // Calling function's parameters
+
                             for param in calling_function.params() {
+                                // If this parameter is ContractState, we don't need to propogate taints
+                                if param
+                                    .ty
+                                    .debug_name
+                                    .as_ref()
+                                    .unwrap()
+                                    .to_string()
+                                    .contains("ContractState")
+                                {
+                                    continue;
+                                }
                                 // Check if the arguments used to call the private function are tainted by the calling function's parameters
                                 for sink in external_taint.taints_any_sinks_variable(
                                     &WrapperVariable::new(

src/detectors/felt252_overflow.rs

@@ -0,0 +1,196 @@
+use super::detector::{Confidence, Detector, Impact, Result};
+use crate::core::compilation_unit::CompilationUnit;
+use crate::core::core_unit::CoreUnit;
+use cairo_lang_sierra::extensions::felt252::Felt252BinaryOperationConcrete;
+use cairo_lang_sierra::extensions::felt252::Felt252BinaryOperator;
+use cairo_lang_sierra::extensions::{core::CoreConcreteLibfunc, felt252::Felt252Concrete};
+use cairo_lang_sierra::ids::VarId;
+use cairo_lang_sierra::program::GenInvocation;
+use cairo_lang_sierra::program::Statement as SierraStatement;
+use cairo_lang_sierra::program::StatementIdx;
+use std::collections::HashSet;
+
+#[derive(Default)]
+pub struct Felt252Overflow {}
+
+impl Detector for Felt252Overflow {
+    fn name(&self) -> &str {
+        "felt252-overflow"
+    }
+
+    fn description(&self) -> &str {
+        "Detect felt252 arithmetic overflow with user-controlled params"
+    }
+
+    fn confidence(&self) -> Confidence {
+        Confidence::Medium
+    }
+
+    fn impact(&self) -> Impact {
+        Impact::High
+    }
+
+    fn run(&self, core: &CoreUnit) -> Vec<Result> {
+        let mut results = Vec::new();
+
+        let compilation_units = core.get_compilation_units();
+        for compilation_unit in compilation_units {
+            let functions = compilation_unit.functions_user_defined();
+            // Iterate through the functions and find binary operations
+            for f in functions {
+                let name = f.name();
+                // Vector for looking up future instructions
+                let statements: &Vec<SierraStatement> = f.get_statements();
+                for (index, stmt) in statements.iter().enumerate() {
+                    if let SierraStatement::Invocation(invoc) = stmt {
+                        // Get the concrete libfunc called
+                        let libfunc = compilation_unit
+                            .registry()
+                            .get_libfunc(&invoc.libfunc_id)
+                            .expect("Library function not found in the registry");
+
+                        if let CoreConcreteLibfunc::Felt252(Felt252Concrete::BinaryOperation(op)) =
+                            libfunc
+                        {
+                            match op {
+                                Felt252BinaryOperationConcrete::WithConst(var) => {
+                                    let operation = var.operator;
+
+                                    self.handle_binops(
+                                        &mut results,
+                                        compilation_unit,
+                                        invoc,
+                                        statements,
+                                        index,
+                                        stmt,
+                                        &operation,
+                                        &name,
+                                    )
+                                }
+                                Felt252BinaryOperationConcrete::WithVar(var) => {
+                                    let operation = var.operator;
+
+                                    self.handle_binops(
+                                        &mut results,
+                                        compilation_unit,
+                                        invoc,
+                                        statements,
+                                        index,
+                                        stmt,
+                                        &operation,
+                                        &name,
+                                    )
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        }
+        results
+    }
+}
+impl Felt252Overflow {
+    fn check_felt252_tainted(
+        &self,
+        results: &mut Vec<Result>,
+        compilation_unit: &CompilationUnit,
+        libfunc: &SierraStatement,
+        args: &[VarId],
+        name: &String,
+    ) {
+        let mut tainted_by: HashSet<&VarId> = HashSet::new();
+        let mut taints = String::new();
+        for param in args.iter() {
+            // TODO: improve when we have source mapping,can add parameter's name instead of ID
+            if compilation_unit.is_tainted(name.to_string(), param.clone())
+                && !tainted_by.contains(&param)
+            {
+                let msg = format!("{},", &param);
+                taints.push_str(&msg);
+                tainted_by.insert(param);
+            }
+        }
+        // Get rid of trailing comma from formatting
+        taints.pop();
+        // Not tainted by any parameter, but still uses felt252 type
+        if tainted_by.is_empty() {
+            let msg = format!(
+                "The function {} uses the felt252 operation {}, which is not overflow safe",
+                &name, libfunc
+            );
+            results.push(Result {
+                name: self.name().to_string(),
+                impact: self.impact(),
+                confidence: self.confidence(),
+                message: msg,
+            });
+        } else {
+            let msg = format!(
+                    "The function {} uses the felt252 operation {} with the user-controlled parameters: {}, which is not overflow safe",
+                    &name,
+                    libfunc,
+                    taints
+                );
+            results.push(Result {
+                name: self.name().to_string(),
+                impact: self.impact(),
+                confidence: self.confidence(),
+                message: msg,
+            });
+        }
+    }
+    #[allow(clippy::too_many_arguments)]
+    fn handle_binops(
+        &self,
+        results: &mut Vec<Result>,
+        compilation_unit: &CompilationUnit,
+        invoc: &GenInvocation<StatementIdx>,
+        statements: &[SierraStatement],
+        idx: usize,
+        libfunc: &SierraStatement,
+        operation: &Felt252BinaryOperator,
+        name: &String,
+    ) {
+        // Get the return value of the sub statement
+        match operation {
+            Felt252BinaryOperator::Sub => {
+                let ret_value = &invoc.branches[0].results[0];
+
+                // Look two instructions in advance
+
+                if let Some(SierraStatement::Invocation(sub_statement)) = statements.get(idx + 2) {
+                    // Check if felt252_is_zero uses return param of sub instruction
+
+                    let libfunc_sub = compilation_unit
+                        .registry()
+                        .get_libfunc(&sub_statement.libfunc_id)
+                        .expect("Library function not found in the registry");
+                    if let CoreConcreteLibfunc::Felt252(Felt252Concrete::IsZero(_)) = libfunc_sub {
+                        let user_params = &sub_statement.args;
+                        if !user_params.contains(ret_value) {
+                            self.check_felt252_tainted(
+                                results,
+                                compilation_unit,
+                                libfunc,
+                                &invoc.args,
+                                name,
+                            );
+                        }
+                    } else {
+                        self.check_felt252_tainted(
+                            results,
+                            compilation_unit,
+                            libfunc,
+                            &invoc.args,
+                            name,
+                        )
+                    }
+                }
+            }
+            _ => {
+                self.check_felt252_tainted(results, compilation_unit, libfunc, &invoc.args, name);
+            }
+        }
+    }
+}

src/detectors/mod.rs

@@ -3,6 +3,7 @@ use self::detector::Detector;
 pub mod controlled_library_call;
 pub mod dead_code;
 pub mod detector;
+pub mod felt252_overflow;
 pub mod read_only_reentrancy;
 pub mod reentrancy;
 pub mod reentrancy_benign;
@@ -24,5 +25,6 @@ pub fn get_detectors() -> Vec<Box<dyn Detector>> {
         Box::<reentrancy_events::ReentrancyEvents>::default(),
         Box::<read_only_reentrancy::ReadOnlyReentrancy>::default(),
         Box::<unchecked_l1_handler_from::UncheckedL1HandlerFrom>::default(),
+        Box::<felt252_overflow::Felt252Overflow>::default(),
     ]
 }