From 25c9c6d1075bb90630f7d0c919074eff078c3473 Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Fri, 18 Aug 2023 17:18:33 -0700 Subject: [PATCH 01/12] added mvp --- src/detectors/felt252_overflow.rs | 80 +++++++++++++++++++ src/detectors/mod.rs | 2 + tests/detectors/felt252_overflow.cairo | 42 ++++++++++ tests/detectors/test.cairo | 6 ++ ..._detectors@felt252_overflow.cairo.snap.new | 7 ++ 5 files changed, 137 insertions(+) create mode 100644 src/detectors/felt252_overflow.rs create mode 100644 tests/detectors/felt252_overflow.cairo create mode 100644 tests/detectors/test.cairo create mode 100644 tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap.new diff --git a/src/detectors/felt252_overflow.rs b/src/detectors/felt252_overflow.rs new file mode 100644 index 0000000..fbe5e2b --- /dev/null +++ b/src/detectors/felt252_overflow.rs @@ -0,0 +1,80 @@ +use std::collections::HashSet; + +use super::detector::{Confidence, Detector, Impact, Result}; +use crate::analysis::taint::WrapperVariable; +use crate::core::compilation_unit::CompilationUnit; +use crate::utils::filter_builtins_from_arguments; +use cairo_lang_sierra::extensions::felt252::Felt252Libfunc::BinaryOperation; +use cairo_lang_sierra::extensions::lib_func::ParamSignature; +use crate::core::core_unit::CoreUnit; +use crate::core::function::{Function, Type}; +use cairo_lang_sierra::extensions::{core::CoreConcreteLibfunc, felt252::Felt252Concrete}; +use cairo_lang_sierra::ids::VarId; +use cairo_lang_sierra::extensions::ConcreteLibfunc; +use cairo_lang_sierra::program::{GenStatement, Statement as SierraStatement}; + +#[derive(Default)] +pub struct Felt252Overflow {} + +impl Detector for Felt252Overflow { + fn name(&self) -> &str { + "felt252-overflow" + } + + fn description(&self) -> &str { + "Detect arithmetic overflow with felt252" + } + + fn confidence(&self) -> Confidence { + Confidence::Medium + } + + fn impact(&self) -> Impact { + Impact::Medium + } + + fn run(&self, core: &CoreUnit) -> Vec { + println!("test"); + 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(); + for f in functions { + for stmt in f.get_statements().iter() { + 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 { + self.check_felt252_tainted(&mut results, &compilation_unit,op.param_signatures(),invoc.args.clone(),&f.name()); + + } + + + } + } + } + } + return results; + } +} +impl Felt252Overflow { + fn check_felt252_tainted(&self,results:&mut Vec, compilation_unit: &CompilationUnit, params:&[ParamSignature],args:Vec,name:&String) { + let user_params = filter_builtins_from_arguments(params, args); + //do the taint + for param in user_params { + if compilation_unit.is_tainted(name.to_string(), param) { + let msg = format!("Function {} uses the felt252 type which is not overflow safe",&name); + results.push(Result { + name: self.name().to_string(), + impact: self.impact(), + confidence: self.confidence(), + message: msg, + }); + } + } + } +} diff --git a/src/detectors/mod.rs b/src/detectors/mod.rs index f299a0f..50b2755 100644 --- a/src/detectors/mod.rs +++ b/src/detectors/mod.rs @@ -11,6 +11,7 @@ pub mod unchecked_l1_handler_from; pub mod unused_arguments; pub mod unused_events; pub mod unused_return; +pub mod felt252_overflow; pub fn get_detectors() -> Vec> { vec![ @@ -24,5 +25,6 @@ pub fn get_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), ] } diff --git a/tests/detectors/felt252_overflow.cairo b/tests/detectors/felt252_overflow.cairo new file mode 100644 index 0000000..be5c801 --- /dev/null +++ b/tests/detectors/felt252_overflow.cairo @@ -0,0 +1,42 @@ +use debug::PrintTrait; +#[starknet::contract] +mod Felt252Overflow { + #[storage] + struct Storage { + a: felt252, + b: felt252 + } + + #[external(v0)] + fn bad(ref self:ContractState) -> felt252 { + let max: felt252 = 0x800000000000011000000000000000000000000000000000000000000000000; + self.a.write(max +1); + return self.a.read(); + } + + #[external(v0)] + fn bad_user_controlled(ref self:ContractState, user_param:felt252) { + let a = self.a.read(); + self.b.write(a + user_param); + } + + #[external(v0)] + fn bad_sub(ref self:ContractState) { + let min: felt252 =0; + self.a.write(min-1); + } + + #[external(v0)] + fn bad_sub_user_controlled(ref self:ContractState, user_param:felt252) { + let a = self.a.read(); + self.b.write(a - user_param); + } + + // #[external(v0)] + // fn bad_test_div(self:@ContractState) { + // let div:felt252 = 7/3; + // div.print(); + + // } + +} \ No newline at end of file diff --git a/tests/detectors/test.cairo b/tests/detectors/test.cairo new file mode 100644 index 0000000..dfe0eeb --- /dev/null +++ b/tests/detectors/test.cairo @@ -0,0 +1,6 @@ +use debug::PrintTrait; + +fn main() { + let a:felt252 = 0x80000000000001100000000000000000000000000000000000000000000000 + 1; + a.print(); +} \ No newline at end of file diff --git a/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap.new b/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap.new new file mode 100644 index 0000000..f9f7aef --- /dev/null +++ b/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap.new @@ -0,0 +1,7 @@ +--- +source: tests/integration_tests.rs +assertion_line: 21 +expression: results +input_file: tests/detectors/felt252_overflow.cairo +--- +[] From e3481484b3efd7606f504a9a2e362aa9b293b44b Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Mon, 21 Aug 2023 09:26:02 -0700 Subject: [PATCH 02/12] fixed ContractState being tainted, added formatting --- src/core/cfg.rs | 2 +- src/core/compilation_unit.rs | 4 +- src/detectors/felt252_overflow.rs | 96 +++++++++++++------ src/detectors/mod.rs | 2 +- tests/detectors/controlled_library_call.cairo | 8 +- tests/detectors/felt252_overflow.cairo | 33 ++++--- tests/detectors/test.cairo | 5 +- 7 files changed, 100 insertions(+), 50 deletions(-) diff --git a/src/core/cfg.rs b/src/core/cfg.rs index 0596f4a..22fb28d 100644 --- a/src/core/cfg.rs +++ b/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 => { diff --git a/src/core/compilation_unit.rs b/src/core/compilation_unit.rs index 33c3750..13c8627 100644 --- a/src/core/compilation_unit.rs +++ b/src/core/compilation_unit.rs @@ -89,7 +89,7 @@ impl CompilationUnit { .iter() .filter(|f| matches!(f.ty(), Type::External | Type::L1Handler)) { - for param in external_function.params() { + for param in external_function.params().skip(1) { parameters.insert(WrapperVariable::new( external_function.name(), param.id.clone(), @@ -328,7 +328,7 @@ impl CompilationUnit { .collect(); // Calling function's parameters - for param in calling_function.params() { + for param in calling_function.params().skip(1) { // 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( diff --git a/src/detectors/felt252_overflow.rs b/src/detectors/felt252_overflow.rs index fbe5e2b..cac6622 100644 --- a/src/detectors/felt252_overflow.rs +++ b/src/detectors/felt252_overflow.rs @@ -1,17 +1,14 @@ -use std::collections::HashSet; - use super::detector::{Confidence, Detector, Impact, Result}; -use crate::analysis::taint::WrapperVariable; use crate::core::compilation_unit::CompilationUnit; +use crate::core::core_unit::CoreUnit; use crate::utils::filter_builtins_from_arguments; -use cairo_lang_sierra::extensions::felt252::Felt252Libfunc::BinaryOperation; + use cairo_lang_sierra::extensions::lib_func::ParamSignature; -use crate::core::core_unit::CoreUnit; -use crate::core::function::{Function, Type}; +use cairo_lang_sierra::extensions::ConcreteLibfunc; use cairo_lang_sierra::extensions::{core::CoreConcreteLibfunc, felt252::Felt252Concrete}; use cairo_lang_sierra::ids::VarId; -use cairo_lang_sierra::extensions::ConcreteLibfunc; -use cairo_lang_sierra::program::{GenStatement, Statement as SierraStatement}; +use cairo_lang_sierra::program::Statement as SierraStatement; +use std::collections::HashSet; #[derive(Default)] pub struct Felt252Overflow {} @@ -22,7 +19,7 @@ impl Detector for Felt252Overflow { } fn description(&self) -> &str { - "Detect arithmetic overflow with felt252" + "Detect felt252 arithmetic overflow with user-controlled params" } fn confidence(&self) -> Confidence { @@ -30,16 +27,17 @@ impl Detector for Felt252Overflow { } fn impact(&self) -> Impact { - Impact::Medium + Impact::High } fn run(&self, core: &CoreUnit) -> Vec { - println!("test"); 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(); for f in functions { + let name = f.name(); for stmt in f.get_statements().iter() { if let SierraStatement::Invocation(invoc) = stmt { // Get the concrete libfunc called @@ -48,33 +46,75 @@ impl Detector for Felt252Overflow { .get_libfunc(&invoc.libfunc_id) .expect("Library function not found in the registry"); - if let CoreConcreteLibfunc::Felt252(Felt252Concrete::BinaryOperation(op)) = libfunc { - self.check_felt252_tainted(&mut results, &compilation_unit,op.param_signatures(),invoc.args.clone(),&f.name()); - + if let CoreConcreteLibfunc::Felt252(Felt252Concrete::BinaryOperation(op)) = + libfunc + { + self.check_felt252_tainted( + &mut results, + compilation_unit, + op.param_signatures(), + stmt, + invoc.args.clone(), + &name, + ); } - - } } } } - return results; + results } } impl Felt252Overflow { - fn check_felt252_tainted(&self,results:&mut Vec, compilation_unit: &CompilationUnit, params:&[ParamSignature],args:Vec,name:&String) { + fn check_felt252_tainted( + &self, + results: &mut Vec, + compilation_unit: &CompilationUnit, + params: &[ParamSignature], + libfunc: &SierraStatement, + args: Vec, + name: &String, + ) { let user_params = filter_builtins_from_arguments(params, args); - //do the taint - for param in user_params { - if compilation_unit.is_tainted(name.to_string(), param) { - let msg = format!("Function {} uses the felt252 type which is not overflow safe",&name); - results.push(Result { - name: self.name().to_string(), - impact: self.impact(), - confidence: self.confidence(), - message: msg, - }); + let mut tainted_by: HashSet<&VarId> = HashSet::new(); + let mut taints = String::new(); + for param in user_params.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(¶m) + { + let msg = format!("{},", ¶m); + taints.push_str(&msg); + tainted_by.insert(¶m); } } + // Get rid of trailing comma from formatting + taints.pop(); + // Not tainted by any parameter, but still uses felt252 type + if tainted_by.len() == 0 { + 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 felt 252 operation {} with the user-controlled parameters: {}", + &name, + libfunc, + taints + ); + results.push(Result { + name: self.name().to_string(), + impact: self.impact(), + confidence: self.confidence(), + message: msg, + }); + } } } diff --git a/src/detectors/mod.rs b/src/detectors/mod.rs index 50b2755..189f057 100644 --- a/src/detectors/mod.rs +++ b/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; @@ -11,7 +12,6 @@ pub mod unchecked_l1_handler_from; pub mod unused_arguments; pub mod unused_events; pub mod unused_return; -pub mod felt252_overflow; pub fn get_detectors() -> Vec> { vec![ diff --git a/tests/detectors/controlled_library_call.cairo b/tests/detectors/controlled_library_call.cairo index ebd6329..e31e9f8 100644 --- a/tests/detectors/controlled_library_call.cairo +++ b/tests/detectors/controlled_library_call.cairo @@ -4,16 +4,18 @@ trait IAnotherContract { } #[starknet::contract] -mod TestContract { +mod ArbitraryLibraryCall { use super::IAnotherContractDispatcherTrait; use super::IAnotherContractLibraryDispatcher; use starknet::class_hash::ClassHash; #[storage] - struct Storage {} + struct Storage { + important_param: u128, + } #[external(v0)] - fn bad1(ref self: ContractState, class_hash: ClassHash) -> u128 { + fn controlled_library_call(ref self: ContractState, class_hash: ClassHash) -> u128 { IAnotherContractLibraryDispatcher { class_hash: class_hash }.foo(2_u128) } diff --git a/tests/detectors/felt252_overflow.cairo b/tests/detectors/felt252_overflow.cairo index be5c801..e013e02 100644 --- a/tests/detectors/felt252_overflow.cairo +++ b/tests/detectors/felt252_overflow.cairo @@ -8,35 +8,40 @@ mod Felt252Overflow { } #[external(v0)] - fn bad(ref self:ContractState) -> felt252 { - let max: felt252 = 0x800000000000011000000000000000000000000000000000000000000000000; - self.a.write(max +1); + fn bad_mul_controlled(ref self:ContractState,param1:felt252, param2:felt252, param3:felt252) -> felt252 { + let max: felt252 = param1 * param2; + let my: felt252 = param1 * param1; + self.b.write(max * param3); return self.a.read(); } #[external(v0)] - fn bad_user_controlled(ref self:ContractState, user_param:felt252) { + fn bad_add_controlled(ref self:ContractState, user_param:felt252, user_param2: felt252) { let a = self.a.read(); - self.b.write(a + user_param); + let controlled = bad_add(user_param, user_param2); + self.b.write(a + user_param+ user_param2); } #[external(v0)] - fn bad_sub(ref self:ContractState) { + fn bad_sub_uncontrolled(ref self:ContractState) { let min: felt252 =0; self.a.write(min-1); } #[external(v0)] - fn bad_sub_user_controlled(ref self:ContractState, user_param:felt252) { - let a = self.a.read(); - self.b.write(a - user_param); + fn bad_add_uncontrolled(self:@ContractState) -> felt252 { + let max: felt252 = 0 - 1; + bad_add(max,2) } - // #[external(v0)] - // fn bad_test_div(self:@ContractState) { - // let div:felt252 = 7/3; - // div.print(); + fn bad_add(param1:felt252, param2:felt252) -> felt252{ + param1 + param2 + } - // } + #[external(v0)] + fn bad_sub_controlled(ref self:ContractState, user_param:felt252) { + let a = self.a.read(); + self.b.write(a - user_param); + } } \ No newline at end of file diff --git a/tests/detectors/test.cairo b/tests/detectors/test.cairo index dfe0eeb..45e2fb1 100644 --- a/tests/detectors/test.cairo +++ b/tests/detectors/test.cairo @@ -1,6 +1,9 @@ use debug::PrintTrait; fn main() { - let a:felt252 = 0x80000000000001100000000000000000000000000000000000000000000000 + 1; + let a:felt252 = 0x800000000000011000000000000000000000000000000000000000000000000 + 1; + let b:felt252 = 0-1; a.print(); + b.print(); + } \ No newline at end of file From 68a519cf11ee5d8112e2aa4afc5b3c0848010699 Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Mon, 21 Aug 2023 09:49:42 -0700 Subject: [PATCH 03/12] added insta snapshots --- src/detectors/felt252_overflow.rs | 4 +- tests/detectors/controlled_library_call.cairo | 3 +- ...tectors@controlled_library_call.cairo.snap | 4 +- ...tion_tests__detectors@dead_code.cairo.snap | 13 +++ ...sts__detectors@felt252_overflow.cairo.snap | 79 +++++++++++++++++++ ..._detectors@felt252_overflow.cairo.snap.new | 7 -- ...ion_tests__detectors@reentrancy.cairo.snap | 6 ++ ...ts__detectors@reentrancy_benign.cairo.snap | 6 ++ 8 files changed, 109 insertions(+), 13 deletions(-) create mode 100644 tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap delete mode 100644 tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap.new diff --git a/src/detectors/felt252_overflow.rs b/src/detectors/felt252_overflow.rs index cac6622..c2e68ac 100644 --- a/src/detectors/felt252_overflow.rs +++ b/src/detectors/felt252_overflow.rs @@ -85,13 +85,13 @@ impl Felt252Overflow { { let msg = format!("{},", ¶m); taints.push_str(&msg); - tainted_by.insert(¶m); + 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.len() == 0 { + if tainted_by.is_empty() { let msg = format!( "The function {} uses the felt252 operation {}, which is not overflow safe", &name, libfunc diff --git a/tests/detectors/controlled_library_call.cairo b/tests/detectors/controlled_library_call.cairo index e31e9f8..23e82a8 100644 --- a/tests/detectors/controlled_library_call.cairo +++ b/tests/detectors/controlled_library_call.cairo @@ -11,11 +11,10 @@ mod ArbitraryLibraryCall { #[storage] struct Storage { - important_param: u128, } #[external(v0)] - fn controlled_library_call(ref self: ContractState, class_hash: ClassHash) -> u128 { + fn bad1(ref self: ContractState, class_hash: ClassHash) -> u128 { IAnotherContractLibraryDispatcher { class_hash: class_hash }.foo(2_u128) } diff --git a/tests/snapshots/integration_tests__detectors@controlled_library_call.cairo.snap b/tests/snapshots/integration_tests__detectors@controlled_library_call.cairo.snap index 1221adc..e209382 100644 --- a/tests/snapshots/integration_tests__detectors@controlled_library_call.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@controlled_library_call.cairo.snap @@ -8,12 +8,12 @@ input_file: tests/detectors/controlled_library_call.cairo impact: High, name: "controlled-library-call", confidence: Medium, - message: "Library call to user controlled class hash in controlled_library_call::controlled_library_call::TestContract::bad1\n function_call([11], [12], [13], [14], [15]) -> ([7], [8], [9], [10])", + message: "Library call to user controlled class hash in controlled_library_call::controlled_library_call::ArbitraryLibraryCall::bad1\n function_call([11], [12], [13], [14], [15]) -> ([7], [8], [9], [10])", }, Result { impact: High, name: "controlled-library-call", confidence: Medium, - message: "Library call to user controlled class hash in controlled_library_call::controlled_library_call::TestContract::internal_lib_call\n function_call([10], [11], [12], [13], [14]) -> ([6], [7], [8], [9])", + message: "Library call to user controlled class hash in controlled_library_call::controlled_library_call::ArbitraryLibraryCall::internal_lib_call\n function_call([10], [11], [12], [13], [14]) -> ([6], [7], [8], [9])", }, ] diff --git a/tests/snapshots/integration_tests__detectors@dead_code.cairo.snap b/tests/snapshots/integration_tests__detectors@dead_code.cairo.snap index a95fb60..06e90ef 100644 --- a/tests/snapshots/integration_tests__detectors@dead_code.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@dead_code.cairo.snap @@ -1,8 +1,21 @@ --- source: tests/integration_tests.rs expression: results +input_file: tests/detectors/dead_code.cairo --- [ + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function dead_code::dead_code::DeadCode::add_1 uses the felt252 operation felt252_add([0], [1]) -> ([2]), which is not overflow safe", + }, + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function dead_code::dead_code::DeadCode::add_2 uses the felt252 operation felt252_add([0], [1]) -> ([2]), which is not overflow safe", + }, Result { impact: Low, name: "dead-code", diff --git a/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap b/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap new file mode 100644 index 0000000..c5988d0 --- /dev/null +++ b/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap @@ -0,0 +1,79 @@ +--- +source: tests/integration_tests.rs +expression: results +input_file: tests/detectors/felt252_overflow.cairo +--- +[ + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add uses the felt252 operation felt252_add([0], [1]) -> ([2]), which is not overflow safe", + }, + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt 252 operation felt252_add([17], [3]) -> ([21]) with the user-controlled parameters: [3]", + }, + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt 252 operation felt252_add([19], [20]) -> ([18]) with the user-controlled parameters: [19],[20]", + }, + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt 252 operation felt252_add([21], [4]) -> ([22]) with the user-controlled parameters: [21],[4]", + }, + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_uncontrolled uses the felt252 operation felt252_add([3], [4]) -> ([5]), which is not overflow safe", + }, + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_uncontrolled uses the felt252 operation felt252_sub([1], [2]) -> ([3]), which is not overflow safe", + }, + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt 252 operation felt252_mul([6], [5]) -> ([10]) with the user-controlled parameters: [6],[5]", + }, + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt 252 operation felt252_mul([7], [4]) -> ([6]) with the user-controlled parameters: [7],[4]", + }, + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt 252 operation felt252_mul([9], [3]) -> ([8]) with the user-controlled parameters: [9],[3]", + }, + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_sub_controlled uses the felt 252 operation felt252_sub([16], [3]) -> ([17]) with the user-controlled parameters: [3]", + }, + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_sub_uncontrolled uses the felt252 operation felt252_sub([3], [4]) -> ([5]), which is not overflow safe", + }, + Result { + impact: Low, + name: "dead-code", + confidence: Medium, + message: "Function bad_add defined in felt252_overflow::felt252_overflow::Felt252Overflow is never used", + }, +] diff --git a/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap.new b/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap.new deleted file mode 100644 index f9f7aef..0000000 --- a/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap.new +++ /dev/null @@ -1,7 +0,0 @@ ---- -source: tests/integration_tests.rs -assertion_line: 21 -expression: results -input_file: tests/detectors/felt252_overflow.cairo ---- -[] diff --git a/tests/snapshots/integration_tests__detectors@reentrancy.cairo.snap b/tests/snapshots/integration_tests__detectors@reentrancy.cairo.snap index fbe5072..2ac3683 100644 --- a/tests/snapshots/integration_tests__detectors@reentrancy.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@reentrancy.cairo.snap @@ -4,6 +4,12 @@ expression: results input_file: tests/detectors/reentrancy.cairo --- [ + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function reentrancy::reentrancy::TestContract::bad2 uses the felt252 operation felt252_sub([4], [5]) -> ([6]), which is not overflow safe", + }, Result { impact: Medium, name: "reentrancy", diff --git a/tests/snapshots/integration_tests__detectors@reentrancy_benign.cairo.snap b/tests/snapshots/integration_tests__detectors@reentrancy_benign.cairo.snap index 2270b91..e27c85b 100644 --- a/tests/snapshots/integration_tests__detectors@reentrancy_benign.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@reentrancy_benign.cairo.snap @@ -4,6 +4,12 @@ expression: results input_file: tests/detectors/reentrancy_benign.cairo --- [ + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function reentrancy_benign::reentrancy_benign::TestContract::bad2 uses the felt252 operation felt252_sub([4], [5]) -> ([6]), which is not overflow safe", + }, Result { impact: Low, name: "reentrancy-benign", From eac71fed76542e07dbf33c4c9425575386456082 Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Tue, 29 Aug 2023 10:06:44 -0700 Subject: [PATCH 04/12] added logic to filter if/assert --- ...lt252_overflow_Felt252Overflow_bad_add.dot | 7 ++ ...low_Felt252Overflow_bad_add_controlled.dot | 66 +++++++++++++ ...w_Felt252Overflow_bad_add_uncontrolled.dot | 14 +++ ...low_Felt252Overflow_bad_mul_controlled.dot | 63 ++++++++++++ ...low_Felt252Overflow_bad_sub_controlled.dot | 59 +++++++++++ ...w_Felt252Overflow_bad_sub_uncontrolled.dot | 39 ++++++++ ..._felt252_overflow_Felt252Overflow_test.dot | 48 +++++++++ ...2_overflow_Felt252Overflow_test_assert.dot | 98 +++++++++++++++++++ src/detectors/felt252_overflow.rs | 65 ++++++++++-- tests/detectors/felt252_overflow.cairo | 15 +++ 10 files changed, 467 insertions(+), 7 deletions(-) create mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_bad_add.dot create mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_controlled.dot create mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_uncontrolled.dot create mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_bad_mul_controlled.dot create mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_controlled.dot create mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_uncontrolled.dot create mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_test.dot create mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_test_assert.dot diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add.dot new file mode 100644 index 0000000..eb8effb --- /dev/null +++ b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add.dot @@ -0,0 +1,7 @@ +digraph "felt252_overflow_felt252_overflow_Felt252Overflow_bad_add.dot" { + 0[label="BB 0 +136 felt252_add([0], [1]) -> ([2]) +137 store_temp([2]) -> ([3]) +138 return([3]) +"] +} \ No newline at end of file diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_controlled.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_controlled.dot new file mode 100644 index 0000000..9f507cd --- /dev/null +++ b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_controlled.dot @@ -0,0 +1,66 @@ +digraph "felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_controlled.dot" { + 0[label="BB 0 +47 struct_deconstruct([2]) -> ([5], [6]) +48 snapshot_take([5]) -> ([7], [8]) +49 store_temp([0]) -> ([12]) +50 store_temp([1]) -> ([13]) +51 store_temp([8]) -> ([14]) +52 function_call([12], [13], [14]) -> ([9], [10], [11]) +53 enum_match>([11]) { fallthrough([15]) 87([16]) } +"] + 0 -> 1 + 0 -> 4 + 1[label="BB 1 +54 branch_align() -> () +55 struct_deconstruct>([15]) -> ([17]) +56 dup([3]) -> ([3], [19]) +57 dup([4]) -> ([4], [20]) +58 felt252_add([19], [20]) -> ([18]) +59 drop([18]) -> () +60 felt252_add([17], [3]) -> ([21]) +61 store_temp([21]) -> ([21]) +62 felt252_add([21], [4]) -> ([22]) +63 store_temp([9]) -> ([26]) +64 store_temp([10]) -> ([27]) +65 store_temp([6]) -> ([28]) +66 store_temp([22]) -> ([29]) +67 function_call([26], [27], [28], [29]) -> ([23], [24], [25]) +68 enum_match>([25]) { fallthrough([30]) 80([31]) } +"] + 1 -> 2 + 1 -> 3 + 2[label="BB 2 +69 branch_align() -> () +70 struct_deconstruct>([30]) -> ([32], [33]) +71 drop([33]) -> () +72 struct_construct([7], [32]) -> ([34]) +73 struct_construct() -> ([35]) +74 struct_construct>([34], [35]) -> ([36]) +75 enum_init, 0>([36]) -> ([37]) +76 store_temp([23]) -> ([38]) +77 store_temp([24]) -> ([39]) +78 store_temp>([37]) -> ([40]) +79 return([38], [39], [40]) +"] + 3[label="BB 3 +80 branch_align() -> () +81 drop([7]) -> () +82 enum_init, 1>([31]) -> ([41]) +83 store_temp([23]) -> ([42]) +84 store_temp([24]) -> ([43]) +85 store_temp>([41]) -> ([44]) +86 return([42], [43], [44]) +"] + 4[label="BB 4 +87 branch_align() -> () +88 drop([7]) -> () +89 drop([6]) -> () +90 drop([4]) -> () +91 drop([3]) -> () +92 enum_init, 1>([16]) -> ([45]) +93 store_temp([9]) -> ([46]) +94 store_temp([10]) -> ([47]) +95 store_temp>([45]) -> ([48]) +96 return([46], [47], [48]) +"] +} \ No newline at end of file diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_uncontrolled.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_uncontrolled.dot new file mode 100644 index 0000000..c740060 --- /dev/null +++ b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_uncontrolled.dot @@ -0,0 +1,14 @@ +digraph "felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_uncontrolled.dot" { + 0[label="BB 0 +126 drop([0]) -> () +127 felt252_const<0>() -> ([1]) +128 felt252_const<1>() -> ([2]) +129 store_temp([1]) -> ([1]) +130 felt252_sub([1], [2]) -> ([3]) +131 felt252_const<2>() -> ([4]) +132 store_temp([3]) -> ([3]) +133 felt252_add([3], [4]) -> ([5]) +134 store_temp([5]) -> ([6]) +135 return([6]) +"] +} \ No newline at end of file diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_mul_controlled.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_mul_controlled.dot new file mode 100644 index 0000000..c8ab7a7 --- /dev/null +++ b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_mul_controlled.dot @@ -0,0 +1,63 @@ +digraph "felt252_overflow_felt252_overflow_Felt252Overflow_bad_mul_controlled.dot" { + 0[label="BB 0 +0 dup([3]) -> ([3], [7]) +1 felt252_mul([7], [4]) -> ([6]) +2 dup([3]) -> ([3], [9]) +3 felt252_mul([9], [3]) -> ([8]) +4 drop([8]) -> () +5 store_temp([6]) -> ([6]) +6 felt252_mul([6], [5]) -> ([10]) +7 struct_deconstruct([2]) -> ([11], [12]) +8 store_temp([0]) -> ([16]) +9 store_temp([1]) -> ([17]) +10 store_temp([12]) -> ([18]) +11 store_temp([10]) -> ([19]) +12 function_call([16], [17], [18], [19]) -> ([13], [14], [15]) +13 enum_match>([15]) { fallthrough([20]) 40([21]) } +"] + 0 -> 1 + 0 -> 4 + 1[label="BB 1 +14 branch_align() -> () +15 struct_deconstruct>([20]) -> ([22], [23]) +16 drop([23]) -> () +17 snapshot_take([11]) -> ([24], [25]) +18 store_temp([13]) -> ([29]) +19 store_temp([14]) -> ([30]) +20 store_temp([25]) -> ([31]) +21 function_call([29], [30], [31]) -> ([26], [27], [28]) +22 enum_match>([28]) { fallthrough([32]) 32([33]) } +"] + 1 -> 2 + 1 -> 3 + 2[label="BB 2 +23 branch_align() -> () +24 struct_deconstruct>([32]) -> ([34]) +25 struct_construct([24], [22]) -> ([35]) +26 struct_construct>([35], [34]) -> ([36]) +27 enum_init, 0>([36]) -> ([37]) +28 store_temp([26]) -> ([38]) +29 store_temp([27]) -> ([39]) +30 store_temp>([37]) -> ([40]) +31 return([38], [39], [40]) +"] + 3[label="BB 3 +32 branch_align() -> () +33 drop([24]) -> () +34 drop([22]) -> () +35 enum_init, 1>([33]) -> ([41]) +36 store_temp([26]) -> ([42]) +37 store_temp([27]) -> ([43]) +38 store_temp>([41]) -> ([44]) +39 return([42], [43], [44]) +"] + 4[label="BB 4 +40 branch_align() -> () +41 drop([11]) -> () +42 enum_init, 1>([21]) -> ([45]) +43 store_temp([13]) -> ([46]) +44 store_temp([14]) -> ([47]) +45 store_temp>([45]) -> ([48]) +46 return([46], [47], [48]) +"] +} \ No newline at end of file diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_controlled.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_controlled.dot new file mode 100644 index 0000000..9e7714e --- /dev/null +++ b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_controlled.dot @@ -0,0 +1,59 @@ +digraph "felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_controlled.dot" { + 0[label="BB 0 +139 struct_deconstruct([2]) -> ([4], [5]) +140 snapshot_take([4]) -> ([6], [7]) +141 store_temp([0]) -> ([11]) +142 store_temp([1]) -> ([12]) +143 store_temp([7]) -> ([13]) +144 function_call([11], [12], [13]) -> ([8], [9], [10]) +145 enum_match>([10]) { fallthrough([14]) 173([15]) } +"] + 0 -> 1 + 0 -> 4 + 1[label="BB 1 +146 branch_align() -> () +147 struct_deconstruct>([14]) -> ([16]) +148 felt252_sub([16], [3]) -> ([17]) +149 store_temp([8]) -> ([21]) +150 store_temp([9]) -> ([22]) +151 store_temp([5]) -> ([23]) +152 store_temp([17]) -> ([24]) +153 function_call([21], [22], [23], [24]) -> ([18], [19], [20]) +154 enum_match>([20]) { fallthrough([25]) 166([26]) } +"] + 1 -> 2 + 1 -> 3 + 2[label="BB 2 +155 branch_align() -> () +156 struct_deconstruct>([25]) -> ([27], [28]) +157 drop([28]) -> () +158 struct_construct([6], [27]) -> ([29]) +159 struct_construct() -> ([30]) +160 struct_construct>([29], [30]) -> ([31]) +161 enum_init, 0>([31]) -> ([32]) +162 store_temp([18]) -> ([33]) +163 store_temp([19]) -> ([34]) +164 store_temp>([32]) -> ([35]) +165 return([33], [34], [35]) +"] + 3[label="BB 3 +166 branch_align() -> () +167 drop([6]) -> () +168 enum_init, 1>([26]) -> ([36]) +169 store_temp([18]) -> ([37]) +170 store_temp([19]) -> ([38]) +171 store_temp>([36]) -> ([39]) +172 return([37], [38], [39]) +"] + 4[label="BB 4 +173 branch_align() -> () +174 drop([6]) -> () +175 drop([5]) -> () +176 drop([3]) -> () +177 enum_init, 1>([15]) -> ([40]) +178 store_temp([8]) -> ([41]) +179 store_temp([9]) -> ([42]) +180 store_temp>([40]) -> ([43]) +181 return([41], [42], [43]) +"] +} \ No newline at end of file diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_uncontrolled.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_uncontrolled.dot new file mode 100644 index 0000000..112e664 --- /dev/null +++ b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_uncontrolled.dot @@ -0,0 +1,39 @@ +digraph "felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_uncontrolled.dot" { + 0[label="BB 0 +97 felt252_const<0>() -> ([3]) +98 felt252_const<1>() -> ([4]) +99 store_temp([3]) -> ([3]) +100 felt252_sub([3], [4]) -> ([5]) +101 struct_deconstruct([2]) -> ([6], [7]) +102 store_temp([0]) -> ([11]) +103 store_temp([1]) -> ([12]) +104 store_temp([6]) -> ([13]) +105 store_temp([5]) -> ([14]) +106 function_call([11], [12], [13], [14]) -> ([8], [9], [10]) +107 enum_match>([10]) { fallthrough([15]) 119([16]) } +"] + 0 -> 1 + 0 -> 2 + 1[label="BB 1 +108 branch_align() -> () +109 struct_deconstruct>([15]) -> ([17], [18]) +110 drop([18]) -> () +111 struct_construct([17], [7]) -> ([19]) +112 struct_construct() -> ([20]) +113 struct_construct>([19], [20]) -> ([21]) +114 enum_init, 0>([21]) -> ([22]) +115 store_temp([8]) -> ([23]) +116 store_temp([9]) -> ([24]) +117 store_temp>([22]) -> ([25]) +118 return([23], [24], [25]) +"] + 2[label="BB 2 +119 branch_align() -> () +120 drop([7]) -> () +121 enum_init, 1>([16]) -> ([26]) +122 store_temp([8]) -> ([27]) +123 store_temp([9]) -> ([28]) +124 store_temp>([26]) -> ([29]) +125 return([27], [28], [29]) +"] +} \ No newline at end of file diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_test.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_test.dot new file mode 100644 index 0000000..f890e7e --- /dev/null +++ b/felt252_overflow_felt252_overflow_Felt252Overflow_test.dot @@ -0,0 +1,48 @@ +digraph "felt252_overflow_felt252_overflow_Felt252Overflow_test.dot" { + 0[label="BB 0 +182 drop([0]) -> () +183 dup([1]) -> ([1], [3]) +184 store_temp([3]) -> ([3]) +185 function_call([3]) -> ([2]) +186 enum_match>([2]) { fallthrough([4]) 205([5]) } +"] + 0 -> 1 + 0 -> 5 + 1[label="BB 1 +187 branch_align() -> () +188 drop>([4]) -> () +189 felt252_const<10>() -> ([6]) +190 felt252_sub([1], [6]) -> ([7]) +191 store_temp([7]) -> ([7]) +192 felt252_is_zero([7]) { fallthrough() 197([8]) } +"] + 1 -> 2 + 1 -> 3 + 2[label="BB 2 +193 branch_align() -> () +194 felt252_const<3>() -> ([9]) +195 store_temp([9]) -> ([10]) +196 jump() { 201() } +"] + 2 -> 4 + 3[label="BB 3 +197 branch_align() -> () +198 drop>([8]) -> () +199 felt252_const<4>() -> ([11]) +200 store_temp([11]) -> ([10]) +"] + 3 -> 4 + 4[label="BB 4 +201 struct_construct>([10]) -> ([12]) +202 enum_init, 0>([12]) -> ([13]) +203 store_temp>([13]) -> ([14]) +204 return([14]) +"] + 5[label="BB 5 +205 branch_align() -> () +206 drop([1]) -> () +207 enum_init, 1>([5]) -> ([15]) +208 store_temp>([15]) -> ([16]) +209 return([16]) +"] +} \ No newline at end of file diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_test_assert.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_test_assert.dot new file mode 100644 index 0000000..bf38e94 --- /dev/null +++ b/felt252_overflow_felt252_overflow_Felt252Overflow_test_assert.dot @@ -0,0 +1,98 @@ +digraph "felt252_overflow_felt252_overflow_Felt252Overflow_test_assert.dot" { + 0[label="BB 0 +210 felt252_const<4>() -> ([1]) +211 snapshot_take([1]) -> ([2], [3]) +212 drop([2]) -> () +213 felt252_const<0>() -> ([4]) +214 snapshot_take([4]) -> ([5], [6]) +215 drop([5]) -> () +216 rename([3]) -> ([7]) +217 snapshot_take([7]) -> ([8], [9]) +218 drop([8]) -> () +219 rename([6]) -> ([10]) +220 snapshot_take([10]) -> ([11], [12]) +221 drop([11]) -> () +222 rename([9]) -> ([13]) +223 rename([12]) -> ([14]) +224 store_temp([13]) -> ([13]) +225 felt252_sub([13], [14]) -> ([15]) +226 store_temp([15]) -> ([15]) +227 felt252_is_zero([15]) { fallthrough() 233([16]) } +"] + 0 -> 1 + 0 -> 2 + 1[label="BB 1 +228 branch_align() -> () +229 struct_construct() -> ([17]) +230 enum_init([17]) -> ([18]) +231 store_temp([18]) -> ([19]) +232 jump() { 238() } +"] + 1 -> 3 + 2[label="BB 2 +233 branch_align() -> () +234 drop>([16]) -> () +235 struct_construct() -> ([20]) +236 enum_init([20]) -> ([21]) +237 store_temp([21]) -> ([19]) +"] + 2 -> 3 + 3[label="BB 3 +238 bool_not_impl([19]) -> ([22]) +239 store_temp([22]) -> ([22]) +240 enum_match([22]) { fallthrough([23]) 253([24]) } +"] + 3 -> 4 + 3 -> 5 + 4[label="BB 4 +241 branch_align() -> () +242 drop([23]) -> () +243 drop([0]) -> () +244 array_new() -> ([25]) +245 felt252_const<6447460>() -> ([26]) +246 store_temp([26]) -> ([26]) +247 array_append([25], [26]) -> ([27]) +248 struct_construct() -> ([28]) +249 struct_construct>>([28], [27]) -> ([29]) +250 enum_init, 1>([29]) -> ([30]) +251 store_temp>([30]) -> ([31]) +252 return([31]) +"] + 5[label="BB 5 +253 branch_align() -> () +254 drop([24]) -> () +255 snapshot_take([0]) -> ([32], [33]) +256 drop([32]) -> () +257 felt252_const<3>() -> ([34]) +258 snapshot_take([34]) -> ([35], [36]) +259 drop([35]) -> () +260 rename([33]) -> ([37]) +261 rename([36]) -> ([38]) +262 felt252_sub([37], [38]) -> ([39]) +263 store_temp([39]) -> ([39]) +264 felt252_is_zero([39]) { fallthrough() 271([40]) } +"] + 5 -> 6 + 5 -> 7 + 6[label="BB 6 +265 branch_align() -> () +266 struct_construct() -> ([41]) +267 struct_construct>([41]) -> ([42]) +268 enum_init, 0>([42]) -> ([43]) +269 store_temp>([43]) -> ([44]) +270 return([44]) +"] + 7[label="BB 7 +271 branch_align() -> () +272 drop>([40]) -> () +273 array_new() -> ([45]) +274 felt252_const<28523>() -> ([46]) +275 store_temp([46]) -> ([46]) +276 array_append([45], [46]) -> ([47]) +277 struct_construct() -> ([48]) +278 struct_construct>>([48], [47]) -> ([49]) +279 enum_init, 1>([49]) -> ([50]) +280 store_temp>([50]) -> ([51]) +281 return([51]) +"] +} \ No newline at end of file diff --git a/src/detectors/felt252_overflow.rs b/src/detectors/felt252_overflow.rs index c2e68ac..6d370b7 100644 --- a/src/detectors/felt252_overflow.rs +++ b/src/detectors/felt252_overflow.rs @@ -2,14 +2,23 @@ use super::detector::{Confidence, Detector, Impact, Result}; use crate::core::compilation_unit::CompilationUnit; use crate::core::core_unit::CoreUnit; use crate::utils::filter_builtins_from_arguments; - +use cairo_lang_sierra::extensions::felt252::Felt252BinaryOperationConcrete; +use cairo_lang_sierra::extensions::felt252::Felt252BinaryOperator; use cairo_lang_sierra::extensions::lib_func::ParamSignature; use cairo_lang_sierra::extensions::ConcreteLibfunc; use cairo_lang_sierra::extensions::{core::CoreConcreteLibfunc, felt252::Felt252Concrete}; use cairo_lang_sierra::ids::VarId; use cairo_lang_sierra::program::Statement as SierraStatement; -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; +pub struct SubVarInfo<'a> { + /// The libfunc used + libfunc: &'a SierraStatement, + /// The parameters used + params: &'a [ParamSignature], + /// Arguments to the sub + args: &'a Vec, +} #[derive(Default)] pub struct Felt252Overflow {} @@ -36,7 +45,9 @@ impl Detector for Felt252Overflow { 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 mut sub_vars = HashMap::new(); let name = f.name(); for stmt in f.get_statements().iter() { if let SierraStatement::Invocation(invoc) = stmt { @@ -49,14 +60,54 @@ impl Detector for Felt252Overflow { if let CoreConcreteLibfunc::Felt252(Felt252Concrete::BinaryOperation(op)) = libfunc { - self.check_felt252_tainted( - &mut results, - compilation_unit, + if let Felt252BinaryOperationConcrete::WithVar(var) = op { + match var.operator { + // We need to see if this is a geniune sub or an if/assert + Felt252BinaryOperator::Sub => { + // Get the return value of the sub statement + let ret_value = &invoc.branches[0].results[0]; + let sub_struct = SubVarInfo { + libfunc: stmt, + params: op.param_signatures(), + args: &invoc.args, + }; + // Add to HashMap to track return values for later + sub_vars.insert(ret_value, sub_struct); + // Continue the loop, we'll analyze this after we check felt252_is_zero + continue; + } + _ => { + self.check_felt252_tainted( + &mut results, + compilation_unit, + op.param_signatures(), + stmt, + invoc.args.clone(), + &name, + ); + } + } + } + } + // Check if felt252_is_zero uses return param of sub instruction + if let CoreConcreteLibfunc::Felt252(Felt252Concrete::IsZero(op)) = libfunc { + let user_params = filter_builtins_from_arguments( op.param_signatures(), - stmt, invoc.args.clone(), - &name, ); + for (k, v) in &sub_vars { + if !user_params.contains(k) { + // This is a geniuine sub instruction since it isn't used by felt252_is_zero + self.check_felt252_tainted( + &mut results, + compilation_unit, + v.params, + v.libfunc, + v.args.clone(), + &name, + ); + } + } } } } diff --git a/tests/detectors/felt252_overflow.cairo b/tests/detectors/felt252_overflow.cairo index e013e02..204118c 100644 --- a/tests/detectors/felt252_overflow.cairo +++ b/tests/detectors/felt252_overflow.cairo @@ -43,5 +43,20 @@ mod Felt252Overflow { let a = self.a.read(); self.b.write(a - user_param); } + #[external(v0)] + fn test_sub_assert(self: @ContractState, p: felt252) -> felt252 { + test_assert(p); + if p == 10 { + 3 + } + else { + 4 - 5 + } + } + fn test_assert(p: felt252) { + assert(4 != 0,'bad'); + assert(p == 3, 'ok'); + + } } \ No newline at end of file From 769cb489eda9d6db3f58c66bc7800f2454315645 Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Wed, 30 Aug 2023 16:19:25 -0700 Subject: [PATCH 05/12] updated if/assert logic to look 2 stmts in advance --- README.md | 12 +- ...lt252_overflow_Felt252Overflow_bad_add.dot | 7 - ...low_Felt252Overflow_bad_add_controlled.dot | 66 -------- ...w_Felt252Overflow_bad_add_uncontrolled.dot | 14 -- ...low_Felt252Overflow_bad_mul_controlled.dot | 63 -------- ...low_Felt252Overflow_bad_sub_controlled.dot | 59 ------- ...w_Felt252Overflow_bad_sub_uncontrolled.dot | 39 ----- ..._felt252_overflow_Felt252Overflow_test.dot | 48 ------ ...2_overflow_Felt252Overflow_test_assert.dot | 98 ------------ src/core/compilation_unit.rs | 14 +- src/detectors/felt252_overflow.rs | 147 +++++++++++++----- tests/detectors/felt252_overflow.cairo | 2 +- ...sts__detectors@felt252_overflow.cairo.snap | 30 ++-- ...ion_tests__detectors@reentrancy.cairo.snap | 6 - ...ts__detectors@reentrancy_benign.cairo.snap | 6 - 15 files changed, 139 insertions(+), 472 deletions(-) delete mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_bad_add.dot delete mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_controlled.dot delete mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_uncontrolled.dot delete mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_bad_mul_controlled.dot delete mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_controlled.dot delete mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_uncontrolled.dot delete mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_test.dot delete mode 100644 felt252_overflow_felt252_overflow_Felt252Overflow_test_assert.dot diff --git a/README.md b/README.md index a07441a..c656a79 100644 --- a/README.md +++ b/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 @@ -79,11 +79,11 @@ Num | Detector | What it Detects | Impact | Confidence | Cairo 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 - -The Cairo column represent the compiler version for which the detector is valid. +12 | `felt-252-overflow` | Detect user controlled operations with felt252 type, which is not overflow safe | High | Medium | 1 & 2 +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 diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add.dot deleted file mode 100644 index eb8effb..0000000 --- a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add.dot +++ /dev/null @@ -1,7 +0,0 @@ -digraph "felt252_overflow_felt252_overflow_Felt252Overflow_bad_add.dot" { - 0[label="BB 0 -136 felt252_add([0], [1]) -> ([2]) -137 store_temp([2]) -> ([3]) -138 return([3]) -"] -} \ No newline at end of file diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_controlled.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_controlled.dot deleted file mode 100644 index 9f507cd..0000000 --- a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_controlled.dot +++ /dev/null @@ -1,66 +0,0 @@ -digraph "felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_controlled.dot" { - 0[label="BB 0 -47 struct_deconstruct([2]) -> ([5], [6]) -48 snapshot_take([5]) -> ([7], [8]) -49 store_temp([0]) -> ([12]) -50 store_temp([1]) -> ([13]) -51 store_temp([8]) -> ([14]) -52 function_call([12], [13], [14]) -> ([9], [10], [11]) -53 enum_match>([11]) { fallthrough([15]) 87([16]) } -"] - 0 -> 1 - 0 -> 4 - 1[label="BB 1 -54 branch_align() -> () -55 struct_deconstruct>([15]) -> ([17]) -56 dup([3]) -> ([3], [19]) -57 dup([4]) -> ([4], [20]) -58 felt252_add([19], [20]) -> ([18]) -59 drop([18]) -> () -60 felt252_add([17], [3]) -> ([21]) -61 store_temp([21]) -> ([21]) -62 felt252_add([21], [4]) -> ([22]) -63 store_temp([9]) -> ([26]) -64 store_temp([10]) -> ([27]) -65 store_temp([6]) -> ([28]) -66 store_temp([22]) -> ([29]) -67 function_call([26], [27], [28], [29]) -> ([23], [24], [25]) -68 enum_match>([25]) { fallthrough([30]) 80([31]) } -"] - 1 -> 2 - 1 -> 3 - 2[label="BB 2 -69 branch_align() -> () -70 struct_deconstruct>([30]) -> ([32], [33]) -71 drop([33]) -> () -72 struct_construct([7], [32]) -> ([34]) -73 struct_construct() -> ([35]) -74 struct_construct>([34], [35]) -> ([36]) -75 enum_init, 0>([36]) -> ([37]) -76 store_temp([23]) -> ([38]) -77 store_temp([24]) -> ([39]) -78 store_temp>([37]) -> ([40]) -79 return([38], [39], [40]) -"] - 3[label="BB 3 -80 branch_align() -> () -81 drop([7]) -> () -82 enum_init, 1>([31]) -> ([41]) -83 store_temp([23]) -> ([42]) -84 store_temp([24]) -> ([43]) -85 store_temp>([41]) -> ([44]) -86 return([42], [43], [44]) -"] - 4[label="BB 4 -87 branch_align() -> () -88 drop([7]) -> () -89 drop([6]) -> () -90 drop([4]) -> () -91 drop([3]) -> () -92 enum_init, 1>([16]) -> ([45]) -93 store_temp([9]) -> ([46]) -94 store_temp([10]) -> ([47]) -95 store_temp>([45]) -> ([48]) -96 return([46], [47], [48]) -"] -} \ No newline at end of file diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_uncontrolled.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_uncontrolled.dot deleted file mode 100644 index c740060..0000000 --- a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_uncontrolled.dot +++ /dev/null @@ -1,14 +0,0 @@ -digraph "felt252_overflow_felt252_overflow_Felt252Overflow_bad_add_uncontrolled.dot" { - 0[label="BB 0 -126 drop([0]) -> () -127 felt252_const<0>() -> ([1]) -128 felt252_const<1>() -> ([2]) -129 store_temp([1]) -> ([1]) -130 felt252_sub([1], [2]) -> ([3]) -131 felt252_const<2>() -> ([4]) -132 store_temp([3]) -> ([3]) -133 felt252_add([3], [4]) -> ([5]) -134 store_temp([5]) -> ([6]) -135 return([6]) -"] -} \ No newline at end of file diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_mul_controlled.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_mul_controlled.dot deleted file mode 100644 index c8ab7a7..0000000 --- a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_mul_controlled.dot +++ /dev/null @@ -1,63 +0,0 @@ -digraph "felt252_overflow_felt252_overflow_Felt252Overflow_bad_mul_controlled.dot" { - 0[label="BB 0 -0 dup([3]) -> ([3], [7]) -1 felt252_mul([7], [4]) -> ([6]) -2 dup([3]) -> ([3], [9]) -3 felt252_mul([9], [3]) -> ([8]) -4 drop([8]) -> () -5 store_temp([6]) -> ([6]) -6 felt252_mul([6], [5]) -> ([10]) -7 struct_deconstruct([2]) -> ([11], [12]) -8 store_temp([0]) -> ([16]) -9 store_temp([1]) -> ([17]) -10 store_temp([12]) -> ([18]) -11 store_temp([10]) -> ([19]) -12 function_call([16], [17], [18], [19]) -> ([13], [14], [15]) -13 enum_match>([15]) { fallthrough([20]) 40([21]) } -"] - 0 -> 1 - 0 -> 4 - 1[label="BB 1 -14 branch_align() -> () -15 struct_deconstruct>([20]) -> ([22], [23]) -16 drop([23]) -> () -17 snapshot_take([11]) -> ([24], [25]) -18 store_temp([13]) -> ([29]) -19 store_temp([14]) -> ([30]) -20 store_temp([25]) -> ([31]) -21 function_call([29], [30], [31]) -> ([26], [27], [28]) -22 enum_match>([28]) { fallthrough([32]) 32([33]) } -"] - 1 -> 2 - 1 -> 3 - 2[label="BB 2 -23 branch_align() -> () -24 struct_deconstruct>([32]) -> ([34]) -25 struct_construct([24], [22]) -> ([35]) -26 struct_construct>([35], [34]) -> ([36]) -27 enum_init, 0>([36]) -> ([37]) -28 store_temp([26]) -> ([38]) -29 store_temp([27]) -> ([39]) -30 store_temp>([37]) -> ([40]) -31 return([38], [39], [40]) -"] - 3[label="BB 3 -32 branch_align() -> () -33 drop([24]) -> () -34 drop([22]) -> () -35 enum_init, 1>([33]) -> ([41]) -36 store_temp([26]) -> ([42]) -37 store_temp([27]) -> ([43]) -38 store_temp>([41]) -> ([44]) -39 return([42], [43], [44]) -"] - 4[label="BB 4 -40 branch_align() -> () -41 drop([11]) -> () -42 enum_init, 1>([21]) -> ([45]) -43 store_temp([13]) -> ([46]) -44 store_temp([14]) -> ([47]) -45 store_temp>([45]) -> ([48]) -46 return([46], [47], [48]) -"] -} \ No newline at end of file diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_controlled.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_controlled.dot deleted file mode 100644 index 9e7714e..0000000 --- a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_controlled.dot +++ /dev/null @@ -1,59 +0,0 @@ -digraph "felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_controlled.dot" { - 0[label="BB 0 -139 struct_deconstruct([2]) -> ([4], [5]) -140 snapshot_take([4]) -> ([6], [7]) -141 store_temp([0]) -> ([11]) -142 store_temp([1]) -> ([12]) -143 store_temp([7]) -> ([13]) -144 function_call([11], [12], [13]) -> ([8], [9], [10]) -145 enum_match>([10]) { fallthrough([14]) 173([15]) } -"] - 0 -> 1 - 0 -> 4 - 1[label="BB 1 -146 branch_align() -> () -147 struct_deconstruct>([14]) -> ([16]) -148 felt252_sub([16], [3]) -> ([17]) -149 store_temp([8]) -> ([21]) -150 store_temp([9]) -> ([22]) -151 store_temp([5]) -> ([23]) -152 store_temp([17]) -> ([24]) -153 function_call([21], [22], [23], [24]) -> ([18], [19], [20]) -154 enum_match>([20]) { fallthrough([25]) 166([26]) } -"] - 1 -> 2 - 1 -> 3 - 2[label="BB 2 -155 branch_align() -> () -156 struct_deconstruct>([25]) -> ([27], [28]) -157 drop([28]) -> () -158 struct_construct([6], [27]) -> ([29]) -159 struct_construct() -> ([30]) -160 struct_construct>([29], [30]) -> ([31]) -161 enum_init, 0>([31]) -> ([32]) -162 store_temp([18]) -> ([33]) -163 store_temp([19]) -> ([34]) -164 store_temp>([32]) -> ([35]) -165 return([33], [34], [35]) -"] - 3[label="BB 3 -166 branch_align() -> () -167 drop([6]) -> () -168 enum_init, 1>([26]) -> ([36]) -169 store_temp([18]) -> ([37]) -170 store_temp([19]) -> ([38]) -171 store_temp>([36]) -> ([39]) -172 return([37], [38], [39]) -"] - 4[label="BB 4 -173 branch_align() -> () -174 drop([6]) -> () -175 drop([5]) -> () -176 drop([3]) -> () -177 enum_init, 1>([15]) -> ([40]) -178 store_temp([8]) -> ([41]) -179 store_temp([9]) -> ([42]) -180 store_temp>([40]) -> ([43]) -181 return([41], [42], [43]) -"] -} \ No newline at end of file diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_uncontrolled.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_uncontrolled.dot deleted file mode 100644 index 112e664..0000000 --- a/felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_uncontrolled.dot +++ /dev/null @@ -1,39 +0,0 @@ -digraph "felt252_overflow_felt252_overflow_Felt252Overflow_bad_sub_uncontrolled.dot" { - 0[label="BB 0 -97 felt252_const<0>() -> ([3]) -98 felt252_const<1>() -> ([4]) -99 store_temp([3]) -> ([3]) -100 felt252_sub([3], [4]) -> ([5]) -101 struct_deconstruct([2]) -> ([6], [7]) -102 store_temp([0]) -> ([11]) -103 store_temp([1]) -> ([12]) -104 store_temp([6]) -> ([13]) -105 store_temp([5]) -> ([14]) -106 function_call([11], [12], [13], [14]) -> ([8], [9], [10]) -107 enum_match>([10]) { fallthrough([15]) 119([16]) } -"] - 0 -> 1 - 0 -> 2 - 1[label="BB 1 -108 branch_align() -> () -109 struct_deconstruct>([15]) -> ([17], [18]) -110 drop([18]) -> () -111 struct_construct([17], [7]) -> ([19]) -112 struct_construct() -> ([20]) -113 struct_construct>([19], [20]) -> ([21]) -114 enum_init, 0>([21]) -> ([22]) -115 store_temp([8]) -> ([23]) -116 store_temp([9]) -> ([24]) -117 store_temp>([22]) -> ([25]) -118 return([23], [24], [25]) -"] - 2[label="BB 2 -119 branch_align() -> () -120 drop([7]) -> () -121 enum_init, 1>([16]) -> ([26]) -122 store_temp([8]) -> ([27]) -123 store_temp([9]) -> ([28]) -124 store_temp>([26]) -> ([29]) -125 return([27], [28], [29]) -"] -} \ No newline at end of file diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_test.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_test.dot deleted file mode 100644 index f890e7e..0000000 --- a/felt252_overflow_felt252_overflow_Felt252Overflow_test.dot +++ /dev/null @@ -1,48 +0,0 @@ -digraph "felt252_overflow_felt252_overflow_Felt252Overflow_test.dot" { - 0[label="BB 0 -182 drop([0]) -> () -183 dup([1]) -> ([1], [3]) -184 store_temp([3]) -> ([3]) -185 function_call([3]) -> ([2]) -186 enum_match>([2]) { fallthrough([4]) 205([5]) } -"] - 0 -> 1 - 0 -> 5 - 1[label="BB 1 -187 branch_align() -> () -188 drop>([4]) -> () -189 felt252_const<10>() -> ([6]) -190 felt252_sub([1], [6]) -> ([7]) -191 store_temp([7]) -> ([7]) -192 felt252_is_zero([7]) { fallthrough() 197([8]) } -"] - 1 -> 2 - 1 -> 3 - 2[label="BB 2 -193 branch_align() -> () -194 felt252_const<3>() -> ([9]) -195 store_temp([9]) -> ([10]) -196 jump() { 201() } -"] - 2 -> 4 - 3[label="BB 3 -197 branch_align() -> () -198 drop>([8]) -> () -199 felt252_const<4>() -> ([11]) -200 store_temp([11]) -> ([10]) -"] - 3 -> 4 - 4[label="BB 4 -201 struct_construct>([10]) -> ([12]) -202 enum_init, 0>([12]) -> ([13]) -203 store_temp>([13]) -> ([14]) -204 return([14]) -"] - 5[label="BB 5 -205 branch_align() -> () -206 drop([1]) -> () -207 enum_init, 1>([5]) -> ([15]) -208 store_temp>([15]) -> ([16]) -209 return([16]) -"] -} \ No newline at end of file diff --git a/felt252_overflow_felt252_overflow_Felt252Overflow_test_assert.dot b/felt252_overflow_felt252_overflow_Felt252Overflow_test_assert.dot deleted file mode 100644 index bf38e94..0000000 --- a/felt252_overflow_felt252_overflow_Felt252Overflow_test_assert.dot +++ /dev/null @@ -1,98 +0,0 @@ -digraph "felt252_overflow_felt252_overflow_Felt252Overflow_test_assert.dot" { - 0[label="BB 0 -210 felt252_const<4>() -> ([1]) -211 snapshot_take([1]) -> ([2], [3]) -212 drop([2]) -> () -213 felt252_const<0>() -> ([4]) -214 snapshot_take([4]) -> ([5], [6]) -215 drop([5]) -> () -216 rename([3]) -> ([7]) -217 snapshot_take([7]) -> ([8], [9]) -218 drop([8]) -> () -219 rename([6]) -> ([10]) -220 snapshot_take([10]) -> ([11], [12]) -221 drop([11]) -> () -222 rename([9]) -> ([13]) -223 rename([12]) -> ([14]) -224 store_temp([13]) -> ([13]) -225 felt252_sub([13], [14]) -> ([15]) -226 store_temp([15]) -> ([15]) -227 felt252_is_zero([15]) { fallthrough() 233([16]) } -"] - 0 -> 1 - 0 -> 2 - 1[label="BB 1 -228 branch_align() -> () -229 struct_construct() -> ([17]) -230 enum_init([17]) -> ([18]) -231 store_temp([18]) -> ([19]) -232 jump() { 238() } -"] - 1 -> 3 - 2[label="BB 2 -233 branch_align() -> () -234 drop>([16]) -> () -235 struct_construct() -> ([20]) -236 enum_init([20]) -> ([21]) -237 store_temp([21]) -> ([19]) -"] - 2 -> 3 - 3[label="BB 3 -238 bool_not_impl([19]) -> ([22]) -239 store_temp([22]) -> ([22]) -240 enum_match([22]) { fallthrough([23]) 253([24]) } -"] - 3 -> 4 - 3 -> 5 - 4[label="BB 4 -241 branch_align() -> () -242 drop([23]) -> () -243 drop([0]) -> () -244 array_new() -> ([25]) -245 felt252_const<6447460>() -> ([26]) -246 store_temp([26]) -> ([26]) -247 array_append([25], [26]) -> ([27]) -248 struct_construct() -> ([28]) -249 struct_construct>>([28], [27]) -> ([29]) -250 enum_init, 1>([29]) -> ([30]) -251 store_temp>([30]) -> ([31]) -252 return([31]) -"] - 5[label="BB 5 -253 branch_align() -> () -254 drop([24]) -> () -255 snapshot_take([0]) -> ([32], [33]) -256 drop([32]) -> () -257 felt252_const<3>() -> ([34]) -258 snapshot_take([34]) -> ([35], [36]) -259 drop([35]) -> () -260 rename([33]) -> ([37]) -261 rename([36]) -> ([38]) -262 felt252_sub([37], [38]) -> ([39]) -263 store_temp([39]) -> ([39]) -264 felt252_is_zero([39]) { fallthrough() 271([40]) } -"] - 5 -> 6 - 5 -> 7 - 6[label="BB 6 -265 branch_align() -> () -266 struct_construct() -> ([41]) -267 struct_construct>([41]) -> ([42]) -268 enum_init, 0>([42]) -> ([43]) -269 store_temp>([43]) -> ([44]) -270 return([44]) -"] - 7[label="BB 7 -271 branch_align() -> () -272 drop>([40]) -> () -273 array_new() -> ([45]) -274 felt252_const<28523>() -> ([46]) -275 store_temp([46]) -> ([46]) -276 array_append([45], [46]) -> ([47]) -277 struct_construct() -> ([48]) -278 struct_construct>>([48], [47]) -> ([49]) -279 enum_init, 1>([49]) -> ([50]) -280 store_temp>([50]) -> ([51]) -281 return([51]) -"] -} \ No newline at end of file diff --git a/src/core/compilation_unit.rs b/src/core/compilation_unit.rs index 13c8627..f790f3a 100644 --- a/src/core/compilation_unit.rs +++ b/src/core/compilation_unit.rs @@ -328,7 +328,19 @@ impl CompilationUnit { .collect(); // Calling function's parameters - for param in calling_function.params().skip(1) { + + 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( diff --git a/src/detectors/felt252_overflow.rs b/src/detectors/felt252_overflow.rs index 6d370b7..22a5728 100644 --- a/src/detectors/felt252_overflow.rs +++ b/src/detectors/felt252_overflow.rs @@ -9,16 +9,8 @@ use cairo_lang_sierra::extensions::ConcreteLibfunc; use cairo_lang_sierra::extensions::{core::CoreConcreteLibfunc, felt252::Felt252Concrete}; use cairo_lang_sierra::ids::VarId; use cairo_lang_sierra::program::Statement as SierraStatement; -use std::collections::{HashMap, HashSet}; - -pub struct SubVarInfo<'a> { - /// The libfunc used - libfunc: &'a SierraStatement, - /// The parameters used - params: &'a [ParamSignature], - /// Arguments to the sub - args: &'a Vec, -} +use std::collections::HashSet; + #[derive(Default)] pub struct Felt252Overflow {} @@ -47,9 +39,10 @@ impl Detector for Felt252Overflow { let functions = compilation_unit.functions_user_defined(); // Iterate through the functions and find binary operations for f in functions { - let mut sub_vars = HashMap::new(); let name = f.name(); - for stmt in f.get_statements().iter() { + // Vector for looking up future instructions + let statements: Vec = f.get_statements().to_owned(); + for (index, stmt) in statements.iter().enumerate() { if let SierraStatement::Invocation(invoc) = stmt { // Get the concrete libfunc called let libfunc = compilation_unit @@ -66,16 +59,50 @@ impl Detector for Felt252Overflow { Felt252BinaryOperator::Sub => { // Get the return value of the sub statement let ret_value = &invoc.branches[0].results[0]; - let sub_struct = SubVarInfo { - libfunc: stmt, - params: op.param_signatures(), - args: &invoc.args, - }; - // Add to HashMap to track return values for later - sub_vars.insert(ret_value, sub_struct); - // Continue the loop, we'll analyze this after we check felt252_is_zero - continue; + + // Look two instructions in advance, since pattern will always be sub -> store_temp -> is_zero + + if let Some(SierraStatement::Invocation(sub_statement)) = + statements.get(index + 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(_z), + ) = libfunc_sub + { + let user_params = &sub_statement.args; + if !user_params.contains(ret_value) { + // This is a geniuine sub instruction since it isn't used by felt252_is_zero + // Maybe we can just continue here since is_zero is only for checking branches? + self.check_felt252_tainted( + &mut results, + compilation_unit, + op.param_signatures(), + stmt, + invoc.args.clone(), + &name, + ); + } + } else { + self.check_felt252_tainted( + &mut results, + compilation_unit, + op.param_signatures(), + stmt, + invoc.args.clone(), + &name, + ) + } + } } + _ => { self.check_felt252_tainted( &mut results, @@ -88,24 +115,64 @@ impl Detector for Felt252Overflow { } } } - } - // Check if felt252_is_zero uses return param of sub instruction - if let CoreConcreteLibfunc::Felt252(Felt252Concrete::IsZero(op)) = libfunc { - let user_params = filter_builtins_from_arguments( - op.param_signatures(), - invoc.args.clone(), - ); - for (k, v) in &sub_vars { - if !user_params.contains(k) { - // This is a geniuine sub instruction since it isn't used by felt252_is_zero - self.check_felt252_tainted( - &mut results, - compilation_unit, - v.params, - v.libfunc, - v.args.clone(), - &name, - ); + if let Felt252BinaryOperationConcrete::WithConst(var) = op { + match var.operator { + // Do the same as above but for constant case + Felt252BinaryOperator::Sub => { + // Get the return value of the sub statement + let ret_value = &invoc.branches[0].results[0]; + + // Look two instructions in advance + + if let Some(SierraStatement::Invocation(sub_statement)) = + statements.get(index + 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(_z), + ) = libfunc_sub + { + let user_params = &sub_statement.args; + if !user_params.contains(ret_value) { + self.check_felt252_tainted( + &mut results, + compilation_unit, + op.param_signatures(), + stmt, + invoc.args.clone(), + &name, + ); + } + } else { + self.check_felt252_tainted( + &mut results, + compilation_unit, + op.param_signatures(), + stmt, + invoc.args.clone(), + &name, + ) + } + } + } + + _ => { + self.check_felt252_tainted( + &mut results, + compilation_unit, + op.param_signatures(), + stmt, + invoc.args.clone(), + &name, + ); + } } } } @@ -155,7 +222,7 @@ impl Felt252Overflow { }); } else { let msg = format!( - "The function {} uses the felt 252 operation {} with the user-controlled parameters: {}", + "The function {} uses the felt252 operation {} with the user-controlled parameters: {}", &name, libfunc, taints diff --git a/tests/detectors/felt252_overflow.cairo b/tests/detectors/felt252_overflow.cairo index 204118c..c721b12 100644 --- a/tests/detectors/felt252_overflow.cairo +++ b/tests/detectors/felt252_overflow.cairo @@ -35,6 +35,7 @@ mod Felt252Overflow { } fn bad_add(param1:felt252, param2:felt252) -> felt252{ + test_assert(param1); param1 + param2 } @@ -56,7 +57,6 @@ mod Felt252Overflow { fn test_assert(p: felt252) { assert(4 != 0,'bad'); assert(p == 3, 'ok'); - } } \ No newline at end of file diff --git a/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap b/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap index c5988d0..5da225a 100644 --- a/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap @@ -8,31 +8,19 @@ input_file: tests/detectors/felt252_overflow.cairo impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add uses the felt252 operation felt252_add([0], [1]) -> ([2]), which is not overflow safe", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add uses the felt 252 operation felt252_add([0], [1]) -> ([6]) with the user-controlled parameters: [0],[1]", }, Result { impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt 252 operation felt252_add([17], [3]) -> ([21]) with the user-controlled parameters: [3]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt 252 operation felt252_add([17], [3]) -> ([23]) with the user-controlled parameters: [3]", }, Result { impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt 252 operation felt252_add([19], [20]) -> ([18]) with the user-controlled parameters: [19],[20]", - }, - Result { - impact: High, - name: "felt252-overflow", - confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt 252 operation felt252_add([21], [4]) -> ([22]) with the user-controlled parameters: [21],[4]", - }, - Result { - impact: High, - name: "felt252-overflow", - confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_uncontrolled uses the felt252 operation felt252_add([3], [4]) -> ([5]), which is not overflow safe", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt 252 operation felt252_add([23], [4]) -> ([24]) with the user-controlled parameters: [23],[4]", }, Result { impact: High, @@ -71,9 +59,15 @@ input_file: tests/detectors/felt252_overflow.cairo message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_sub_uncontrolled uses the felt252 operation felt252_sub([3], [4]) -> ([5]), which is not overflow safe", }, Result { - impact: Low, - name: "dead-code", + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::test_sub_assert uses the felt252 operation felt252_sub([11], [12]) -> ([13]), which is not overflow safe", + }, + Result { + impact: Medium, + name: "unused-return", confidence: Medium, - message: "Function bad_add defined in felt252_overflow::felt252_overflow::Felt252Overflow is never used", + message: "Return value unused for the function call function_call([19], [20]) -> ([18]) in felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled", }, ] diff --git a/tests/snapshots/integration_tests__detectors@reentrancy.cairo.snap b/tests/snapshots/integration_tests__detectors@reentrancy.cairo.snap index 2ac3683..fbe5072 100644 --- a/tests/snapshots/integration_tests__detectors@reentrancy.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@reentrancy.cairo.snap @@ -4,12 +4,6 @@ expression: results input_file: tests/detectors/reentrancy.cairo --- [ - Result { - impact: High, - name: "felt252-overflow", - confidence: Medium, - message: "The function reentrancy::reentrancy::TestContract::bad2 uses the felt252 operation felt252_sub([4], [5]) -> ([6]), which is not overflow safe", - }, Result { impact: Medium, name: "reentrancy", diff --git a/tests/snapshots/integration_tests__detectors@reentrancy_benign.cairo.snap b/tests/snapshots/integration_tests__detectors@reentrancy_benign.cairo.snap index e27c85b..2270b91 100644 --- a/tests/snapshots/integration_tests__detectors@reentrancy_benign.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@reentrancy_benign.cairo.snap @@ -4,12 +4,6 @@ expression: results input_file: tests/detectors/reentrancy_benign.cairo --- [ - Result { - impact: High, - name: "felt252-overflow", - confidence: Medium, - message: "The function reentrancy_benign::reentrancy_benign::TestContract::bad2 uses the felt252 operation felt252_sub([4], [5]) -> ([6]), which is not overflow safe", - }, Result { impact: Low, name: "reentrancy-benign", From 593d07c25b9f4fdc23b1111f673c3a9c96337eac Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Wed, 30 Aug 2023 16:29:05 -0700 Subject: [PATCH 06/12] updated to master + updated snapshots --- Cargo.lock | 14 +- Cargo.toml | 2 + src/compilation/utils/felt252_serde.rs | 121 ++++++++++++++---- .../utils/felt252_vec_compression.rs | 10 +- ...sts__detectors@felt252_overflow.cairo.snap | 14 +- 5 files changed, 116 insertions(+), 45 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d0415f8..99eb228 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -159,9 +159,9 @@ checksum = "c3ac9f8b63eca6fd385229b3675f6cc0dc5c8a5c8a54a59d4f52ffd670d87b0c" [[package]] name = "cairo-felt" -version = "0.8.5" +version = "0.8.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ed22664386f178bf9ca7b9ae7235727d92fa37b731a9063b5122488a1f699834" +checksum = "5972097b8800ca5dffb458040e74c724a2ac4fa4b5b480b50f5b96c7e67d6427" dependencies = [ "lazy_static", "num-bigint", @@ -567,6 +567,7 @@ name = "caracal" version = "0.2.0" dependencies = [ "anyhow", + "cairo-felt", "cairo-lang-compiler", "cairo-lang-defs", "cairo-lang-filesystem", @@ -588,6 +589,7 @@ dependencies = [ "serde_json", "smol_str", "termcolor", + "thiserror", ] [[package]] @@ -1891,18 +1893,18 @@ dependencies = [ [[package]] name = "thiserror" -version = "1.0.44" +version = "1.0.47" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "611040a08a0439f8248d1990b111c95baa9c704c805fa1f62104b39655fd7f90" +checksum = "97a802ec30afc17eee47b2855fc72e0c4cd62be9b4efe6591edde0ec5bd68d8f" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "1.0.44" +version = "1.0.47" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "090198534930841fab3a5d1bb637cde49e339654e606195f8d9c76eeb081dc96" +checksum = "6bb623b56e39ab7dcd4b1b98bb6c8f8d907ed255b18de254088016b27a8ee19b" dependencies = [ "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index eb423db..835e9e8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,6 +29,8 @@ cairo-lang-semantic = { git = "https://github.com/starkware-libs/cairo.git", tag cairo-lang-utils = { git = "https://github.com/starkware-libs/cairo.git", tag = "v2.1.0" } cairo-lang-sierra-generator = { git = "https://github.com/starkware-libs/cairo.git", tag = "v2.1.0" } cairo-lang-sierra = { git = "https://github.com/starkware-libs/cairo.git", tag = "v2.1.0" } +cairo-felt = "0.8.7" +thiserror = "1.0.47" [dev-dependencies] insta = { version = "1.30", features = ["glob"] } diff --git a/src/compilation/utils/felt252_serde.rs b/src/compilation/utils/felt252_serde.rs index e737b89..7fb888e 100644 --- a/src/compilation/utils/felt252_serde.rs +++ b/src/compilation/utils/felt252_serde.rs @@ -1,7 +1,11 @@ -// Taken from https://github.com/starkware-libs/cairo/blob/77ae28f996c0960ce5cfc926703f60bae8d5db5a/crates/cairo-lang-starknet/src/felt252_serde.rs +// 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; +use cairo_lang_sierra::extensions::starknet::secp256k1::Secp256k1; +use cairo_lang_sierra::extensions::starknet::secp256r1::Secp256r1; use cairo_lang_sierra::extensions::starknet::storage::{ StorageAddressFromBaseAndOffsetLibfunc, StorageAddressTryFromFelt252Trait, StorageBaseAddressFromFelt252Libfunc, @@ -13,10 +17,11 @@ use cairo_lang_sierra::ids::{ VarId, }; use cairo_lang_sierra::program::{ - BranchInfo, BranchTarget, ConcreteLibfuncLongId, ConcreteTypeLongId, Function, - FunctionSignature, GenericArg, Invocation, LibfuncDeclaration, Param, Program, Statement, - StatementIdx, TypeDeclaration, + BranchInfo, BranchTarget, ConcreteLibfuncLongId, ConcreteTypeLongId, DeclaredTypeInfo, + Function, FunctionSignature, GenericArg, Invocation, LibfuncDeclaration, Param, Program, + Statement, StatementIdx, TypeDeclaration, }; +use cairo_lang_starknet::contract::starknet_keccak; use cairo_lang_utils::bigint::BigUintAsHex; use cairo_lang_utils::ordered_hash_set::OrderedHashSet; use cairo_lang_utils::unordered_hash_map::UnorderedHashMap; @@ -24,9 +29,8 @@ use num_bigint::{BigInt, BigUint, ToBigInt}; use num_traits::ToPrimitive; use once_cell::sync::Lazy; use smol_str::SmolStr; - -use crate::compilation::utils::felt252_vec_compression::decompress; -use cairo_lang_starknet::contract::starknet_keccak; +use std::ops::Shr; +use thiserror::Error; #[derive(Debug, PartialEq, Eq, Clone)] pub struct VersionId { @@ -34,9 +38,9 @@ pub struct VersionId { pub minor: usize, pub patch: usize, } - -#[derive(Debug, Eq, PartialEq)] +#[derive(Error, Debug, Eq, PartialEq)] pub enum Felt252SerdeError { + #[error("Invalid input for deserialization.")] InvalidInputForDeserialization, } @@ -129,6 +133,8 @@ static SERDE_SUPPORTED_LONG_IDS: Lazy> = Lazy::new( ContractAddressTryFromFelt252Libfunc::STR_ID, StorageBaseAddressFromFelt252Libfunc::STR_ID, StorageAddressTryFromFelt252Trait::STR_ID, + Secp256GetPointFromXLibfunc::::STR_ID, + Secp256GetPointFromXLibfunc::::STR_ID, ] .into_iter(), ) @@ -206,7 +212,6 @@ id_serde!(VarId); id_serde!(FunctionId); // Impls for structs. - macro_rules! struct_deserialize_impl { ($input:ident, { $($field_name:ident : $field_type:ty),* }) => { let __input = $input; @@ -242,11 +247,11 @@ impl Felt252Serde for Program { let (size, mut input) = usize::deserialize(input)?; let mut type_declarations = Vec::with_capacity(size); for i in 0..size { - let (long_id, next) = ConcreteTypeLongId::deserialize(input)?; + let (info, next) = ConcreteTypeInfo::deserialize(input)?; type_declarations.push(TypeDeclaration { id: ConcreteTypeId::new(i as u64), - long_id, - declared_type_info: None, + long_id: info.long_id, + declared_type_info: info.declared_type_info, }); input = next; } @@ -300,10 +305,51 @@ impl Felt252Serde for Program { } } -struct_serde! { - ConcreteTypeLongId { - generic_id: GenericTypeId, - generic_args: Vec, +/// Helper struct to serialize and deserialize a `ConcreteTypeLongId` and its optional +/// `DeclaredTypeInfo`. +struct ConcreteTypeInfo { + long_id: ConcreteTypeLongId, + declared_type_info: Option, +} + +const TYPE_STORABLE: u64 = 0b0001; +const TYPE_DROPPABLE: u64 = 0b0010; +const TYPE_DUPLICATABLE: u64 = 0b0100; +const TYPE_ZERO_SIZED: u64 = 0b1000; + +impl Felt252Serde for ConcreteTypeInfo { + fn deserialize(input: &[BigUintAsHex]) -> Result<(Self, &[BigUintAsHex]), Felt252SerdeError> { + let (generic_id, input) = GenericTypeId::deserialize(input)?; + let (len_and_decl_ti_value, mut input) = BigInt::deserialize(input)?; + let len = (len_and_decl_ti_value.clone() & BigInt::from(u128::MAX)) + .to_usize() + .unwrap(); + let decl_ti_value = (len_and_decl_ti_value.shr(128) as BigInt).to_u64().unwrap(); + let mut generic_args = Vec::with_capacity(len); + for _ in 0..len { + let (arg, next) = GenericArg::deserialize(input)?; + generic_args.push(arg); + input = next; + } + Ok(( + Self { + long_id: ConcreteTypeLongId { + generic_id, + generic_args, + }, + declared_type_info: if decl_ti_value == 0 { + None + } else { + Some(DeclaredTypeInfo { + storable: (decl_ti_value & TYPE_STORABLE) != 0, + droppable: (decl_ti_value & TYPE_DROPPABLE) != 0, + duplicatable: (decl_ti_value & TYPE_DUPLICATABLE) != 0, + zero_sized: (decl_ti_value & TYPE_ZERO_SIZED) != 0, + }) + }, + }, + input, + )) } } @@ -343,7 +389,6 @@ struct_serde!(VersionId { }); // Impls for enums. - macro_rules! enum_deserialize { ($($variant_name:ident ( $variant_type:ty ) = $variant_id:literal),*) => { fn deserialize( @@ -376,13 +421,37 @@ enum_serde! { } } -enum_serde! { - GenericArg { - UserType(UserTypeId) = 0, - Type(ConcreteTypeId) = 1, - Value(BigInt) = 2, - UserFunc(FunctionId) = 3, - Libfunc(ConcreteLibfuncId) = 4, +/// Custom serialization for `GenericArg` to support negatives in `GenericArg::Value`. +impl Felt252Serde for GenericArg { + fn deserialize(input: &[BigUintAsHex]) -> Result<(Self, &[BigUintAsHex]), Felt252SerdeError> { + let (idx, input) = usize::deserialize(input)?; + Ok(match idx { + 0 => { + let (id, input) = UserTypeId::deserialize(input)?; + (Self::UserType(id), input) + } + 1 => { + let (id, input) = ConcreteTypeId::deserialize(input)?; + (Self::Type(id), input) + } + 2 => { + let (value, input) = BigInt::deserialize(input)?; + (Self::Value(value), input) + } + 3 => { + let (id, input) = FunctionId::deserialize(input)?; + (Self::UserFunc(id), input) + } + 4 => { + let (id, input) = ConcreteLibfuncId::deserialize(input)?; + (Self::Libfunc(id), input) + } + 5 => { + let (value, input) = BigInt::deserialize(input)?; + (Self::Value(-value), input) + } + _ => return Err(Felt252SerdeError::InvalidInputForDeserialization), + }) } } @@ -398,4 +467,4 @@ impl Felt252Serde for BranchTarget { input, )) } -} +} \ No newline at end of file diff --git a/src/compilation/utils/felt252_vec_compression.rs b/src/compilation/utils/felt252_vec_compression.rs index 23bbcd9..64c7d33 100644 --- a/src/compilation/utils/felt252_vec_compression.rs +++ b/src/compilation/utils/felt252_vec_compression.rs @@ -1,5 +1,6 @@ -// Taken from https://github.com/starkware-libs/cairo/blob/77ae28f996c0960ce5cfc926703f60bae8d5db5a/crates/cairo-lang-starknet/src/felt252_vec_compression.rs +// Taken from https://github.com/starkware-libs/cairo/blob/0a3e9dec15c2a853559d233247a253871e7bb35a/crates/cairo-lang-starknet/src/felt252_vec_compression.rs +use cairo_felt::Felt252; use cairo_lang_utils::bigint::BigUintAsHex; use num_bigint::BigUint; use num_integer::Integer; @@ -48,17 +49,14 @@ fn pop_usize(values: &[BigUintAsHex]) -> Option<(&[BigUintAsHex], usize)> { Some((values, size.value.to_usize()?)) } -const FIELD_HIGH: u128 = (1 << 123) + (17 << 64); // this is equal to 10633823966279327296825105735305134080 -const FIELD_LOW: u128 = 1; - /// Given the size of the code book, returns the number of code words that can be encoded in a felt. fn words_per_felt(padded_code_size: usize) -> usize { let mut count = 0; - let prime = (Into::::into(FIELD_HIGH) << 128) + Into::::into(FIELD_LOW); + let prime = Felt252::prime(); let mut max_encoded = BigUint::from(padded_code_size); while max_encoded < prime { max_encoded *= padded_code_size; count += 1; } count -} +} \ No newline at end of file diff --git a/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap b/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap index 5da225a..bf1f3b3 100644 --- a/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap @@ -8,19 +8,19 @@ input_file: tests/detectors/felt252_overflow.cairo impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add uses the felt 252 operation felt252_add([0], [1]) -> ([6]) with the user-controlled parameters: [0],[1]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add uses the felt252 operation felt252_add([0], [1]) -> ([6]) with the user-controlled parameters: [0],[1]", }, Result { impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt 252 operation felt252_add([17], [3]) -> ([23]) with the user-controlled parameters: [3]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt252 operation felt252_add([17], [3]) -> ([23]) with the user-controlled parameters: [3]", }, Result { impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt 252 operation felt252_add([23], [4]) -> ([24]) with the user-controlled parameters: [23],[4]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt252 operation felt252_add([23], [4]) -> ([24]) with the user-controlled parameters: [23],[4]", }, Result { impact: High, @@ -32,25 +32,25 @@ input_file: tests/detectors/felt252_overflow.cairo impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt 252 operation felt252_mul([6], [5]) -> ([10]) with the user-controlled parameters: [6],[5]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([6], [5]) -> ([10]) with the user-controlled parameters: [6],[5]", }, Result { impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt 252 operation felt252_mul([7], [4]) -> ([6]) with the user-controlled parameters: [7],[4]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([7], [4]) -> ([6]) with the user-controlled parameters: [7],[4]", }, Result { impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt 252 operation felt252_mul([9], [3]) -> ([8]) with the user-controlled parameters: [9],[3]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([9], [3]) -> ([8]) with the user-controlled parameters: [9],[3]", }, Result { impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_sub_controlled uses the felt 252 operation felt252_sub([16], [3]) -> ([17]) with the user-controlled parameters: [3]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_sub_controlled uses the felt252 operation felt252_sub([16], [3]) -> ([17]) with the user-controlled parameters: [3]", }, Result { impact: High, From 8a078cd3fb854dfc1972707da82117c844a79f2d Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Thu, 31 Aug 2023 00:37:46 -0700 Subject: [PATCH 07/12] various fixes --- README.md | 2 +- src/compilation/utils/felt252_serde.rs | 2 +- src/compilation/utils/felt252_vec_compression.rs | 2 +- src/detectors/felt252_overflow.rs | 1 + tests/detectors/felt252_overflow.cairo | 2 +- 5 files changed, 5 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index c656a79..6ec50a0 100644 --- a/README.md +++ b/README.md @@ -79,7 +79,7 @@ Num | Detector | What it Detects | Impact | Confidence | Cairo 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 -12 | `felt-252-overflow` | Detect user controlled operations with felt252 type, which is not overflow safe | High | Medium | 1 & 2 +12 | `felt-252-overflow` | Detect user controlled operations with felt252 type, which is not overflow safe | Medium | High | 1 & 2 The Cairo column represent the compiler version(s) for which the detector is valid. ## Printers diff --git a/src/compilation/utils/felt252_serde.rs b/src/compilation/utils/felt252_serde.rs index 7fb888e..632e595 100644 --- a/src/compilation/utils/felt252_serde.rs +++ b/src/compilation/utils/felt252_serde.rs @@ -467,4 +467,4 @@ impl Felt252Serde for BranchTarget { input, )) } -} \ No newline at end of file +} diff --git a/src/compilation/utils/felt252_vec_compression.rs b/src/compilation/utils/felt252_vec_compression.rs index 64c7d33..a9abf76 100644 --- a/src/compilation/utils/felt252_vec_compression.rs +++ b/src/compilation/utils/felt252_vec_compression.rs @@ -59,4 +59,4 @@ fn words_per_felt(padded_code_size: usize) -> usize { count += 1; } count -} \ No newline at end of file +} diff --git a/src/detectors/felt252_overflow.rs b/src/detectors/felt252_overflow.rs index 22a5728..0d48606 100644 --- a/src/detectors/felt252_overflow.rs +++ b/src/detectors/felt252_overflow.rs @@ -194,6 +194,7 @@ impl Felt252Overflow { name: &String, ) { let user_params = filter_builtins_from_arguments(params, args); + println!("{:?}", user_params); let mut tainted_by: HashSet<&VarId> = HashSet::new(); let mut taints = String::new(); for param in user_params.iter() { diff --git a/tests/detectors/felt252_overflow.cairo b/tests/detectors/felt252_overflow.cairo index c721b12..c4010be 100644 --- a/tests/detectors/felt252_overflow.cairo +++ b/tests/detectors/felt252_overflow.cairo @@ -51,7 +51,7 @@ mod Felt252Overflow { 3 } else { - 4 - 5 + p - 5 } } fn test_assert(p: felt252) { From 17d056083a745ac70e917661b667a7c6f872c60d Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Thu, 31 Aug 2023 00:41:51 -0700 Subject: [PATCH 08/12] update readme --- README.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 6ec50a0..f3532c7 100644 --- a/README.md +++ b/README.md @@ -70,16 +70,17 @@ 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 +4 | `felt-252-overflow` | Detect user controlled operations with felt252 type, which is not overflow safe | High | Medium | 1 & 2 +5 | `reentrancy` | Detect when a storage variable is read before an external call and written after | Medium | Medium | 1 & 2 +6 | `read-only-reentrancy` | Detect when a view function read a storage variable written after an external call | Medium | Medium | 1 & 2 +7 | `unused-events` | Events defined but not emitted | Medium | Medium | 1 & 2 +8 | `unused-return` | Unused return values | Medium | Medium | 1 & 2 +9 | `unenforced-view` | Function has view decorator but modifies state | Medium | Medium | 1 +10 | `unused-arguments` | Unused arguments | Low | Medium | 1 & 2 +11 | `reentrancy-benign` | Detect when a storage variable is written after an external call but not read before | Low | Medium | 1 & 2 +12 | `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 -12 | `felt-252-overflow` | Detect user controlled operations with felt252 type, which is not overflow safe | Medium | High | 1 & 2 + The Cairo column represent the compiler version(s) for which the detector is valid. ## Printers From 72ae9532341416bb16cfc64ab7c3514d0aac31e6 Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Thu, 31 Aug 2023 00:43:27 -0700 Subject: [PATCH 09/12] update readme --- README.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index f3532c7..0c099cf 100644 --- a/README.md +++ b/README.md @@ -70,16 +70,16 @@ 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 -4 | `felt-252-overflow` | Detect user controlled operations with felt252 type, which is not overflow safe | High | Medium | 1 & 2 -5 | `reentrancy` | Detect when a storage variable is read before an external call and written after | Medium | Medium | 1 & 2 -6 | `read-only-reentrancy` | Detect when a view function read a storage variable written after an external call | Medium | Medium | 1 & 2 -7 | `unused-events` | Events defined but not emitted | Medium | Medium | 1 & 2 -8 | `unused-return` | Unused return values | Medium | Medium | 1 & 2 -9 | `unenforced-view` | Function has view decorator but modifies state | Medium | Medium | 1 -10 | `unused-arguments` | Unused arguments | Low | Medium | 1 & 2 -11 | `reentrancy-benign` | Detect when a storage variable is written after an external call but not read before | Low | Medium | 1 & 2 -12 | `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(s) for which the detector is valid. From 8c03b17c27353ee27d8721673653c6297e20fca1 Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Thu, 31 Aug 2023 00:47:09 -0700 Subject: [PATCH 10/12] misc cleanup --- tests/detectors/controlled_library_call.cairo | 5 ++--- tests/detectors/test.cairo | 9 --------- 2 files changed, 2 insertions(+), 12 deletions(-) delete mode 100644 tests/detectors/test.cairo diff --git a/tests/detectors/controlled_library_call.cairo b/tests/detectors/controlled_library_call.cairo index 23e82a8..ebd6329 100644 --- a/tests/detectors/controlled_library_call.cairo +++ b/tests/detectors/controlled_library_call.cairo @@ -4,14 +4,13 @@ trait IAnotherContract { } #[starknet::contract] -mod ArbitraryLibraryCall { +mod TestContract { use super::IAnotherContractDispatcherTrait; use super::IAnotherContractLibraryDispatcher; use starknet::class_hash::ClassHash; #[storage] - struct Storage { - } + struct Storage {} #[external(v0)] fn bad1(ref self: ContractState, class_hash: ClassHash) -> u128 { diff --git a/tests/detectors/test.cairo b/tests/detectors/test.cairo deleted file mode 100644 index 45e2fb1..0000000 --- a/tests/detectors/test.cairo +++ /dev/null @@ -1,9 +0,0 @@ -use debug::PrintTrait; - -fn main() { - let a:felt252 = 0x800000000000011000000000000000000000000000000000000000000000000 + 1; - let b:felt252 = 0-1; - a.print(); - b.print(); - -} \ No newline at end of file From 2afb65cde7aabff302333a5f72698c5d99afd8f3 Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Thu, 31 Aug 2023 00:54:33 -0700 Subject: [PATCH 11/12] misc cleanup --- Cargo.lock | 3 --- 1 file changed, 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index efffa25..20b273b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -161,9 +161,6 @@ name = "cairo-felt" version = "0.8.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5972097b8800ca5dffb458040e74c724a2ac4fa4b5b480b50f5b96c7e67d6427" -version = "0.8.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c495417de017d516c679f07f63c76a037521b5a80cbbf0928389c70987f6db3a" dependencies = [ "lazy_static", "num-bigint", From 54c29ec4af4c5d9e076eda7dfe4e05e6d50502d8 Mon Sep 17 00:00:00 2001 From: technovision99 <25871300+technovision99@users.noreply.github.com> Date: Thu, 31 Aug 2023 07:55:46 -0700 Subject: [PATCH 12/12] remove duplication and cleaned up stuff --- src/detectors/felt252_overflow.rs | 215 +++++++----------- ...tectors@controlled_library_call.cairo.snap | 4 +- ...sts__detectors@felt252_overflow.cairo.snap | 16 +- ...ctors@unchecked_l1_handler_from.cairo.snap | 25 ++ ..._tests__detectors@unused_return.cairo.snap | 6 + 5 files changed, 127 insertions(+), 139 deletions(-) diff --git a/src/detectors/felt252_overflow.rs b/src/detectors/felt252_overflow.rs index 0d48606..666b353 100644 --- a/src/detectors/felt252_overflow.rs +++ b/src/detectors/felt252_overflow.rs @@ -1,14 +1,13 @@ use super::detector::{Confidence, Detector, Impact, Result}; use crate::core::compilation_unit::CompilationUnit; use crate::core::core_unit::CoreUnit; -use crate::utils::filter_builtins_from_arguments; use cairo_lang_sierra::extensions::felt252::Felt252BinaryOperationConcrete; use cairo_lang_sierra::extensions::felt252::Felt252BinaryOperator; -use cairo_lang_sierra::extensions::lib_func::ParamSignature; -use cairo_lang_sierra::extensions::ConcreteLibfunc; 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)] @@ -41,7 +40,7 @@ impl Detector for Felt252Overflow { for f in functions { let name = f.name(); // Vector for looking up future instructions - let statements: Vec = f.get_statements().to_owned(); + let statements: &Vec = f.get_statements(); for (index, stmt) in statements.iter().enumerate() { if let SierraStatement::Invocation(invoc) = stmt { // Get the concrete libfunc called @@ -53,126 +52,34 @@ impl Detector for Felt252Overflow { if let CoreConcreteLibfunc::Felt252(Felt252Concrete::BinaryOperation(op)) = libfunc { - if let Felt252BinaryOperationConcrete::WithVar(var) = op { - match var.operator { - // We need to see if this is a geniune sub or an if/assert - Felt252BinaryOperator::Sub => { - // Get the return value of the sub statement - let ret_value = &invoc.branches[0].results[0]; - - // Look two instructions in advance, since pattern will always be sub -> store_temp -> is_zero - - if let Some(SierraStatement::Invocation(sub_statement)) = - statements.get(index + 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(_z), - ) = libfunc_sub - { - let user_params = &sub_statement.args; - if !user_params.contains(ret_value) { - // This is a geniuine sub instruction since it isn't used by felt252_is_zero - // Maybe we can just continue here since is_zero is only for checking branches? - self.check_felt252_tainted( - &mut results, - compilation_unit, - op.param_signatures(), - stmt, - invoc.args.clone(), - &name, - ); - } - } else { - self.check_felt252_tainted( - &mut results, - compilation_unit, - op.param_signatures(), - stmt, - invoc.args.clone(), - &name, - ) - } - } - } - - _ => { - self.check_felt252_tainted( - &mut results, - compilation_unit, - op.param_signatures(), - stmt, - invoc.args.clone(), - &name, - ); - } + match op { + Felt252BinaryOperationConcrete::WithConst(var) => { + let operation = var.operator; + + self.handle_binops( + &mut results, + compilation_unit, + invoc, + statements, + index, + stmt, + &operation, + &name, + ) } - } - if let Felt252BinaryOperationConcrete::WithConst(var) = op { - match var.operator { - // Do the same as above but for constant case - Felt252BinaryOperator::Sub => { - // Get the return value of the sub statement - let ret_value = &invoc.branches[0].results[0]; - - // Look two instructions in advance - - if let Some(SierraStatement::Invocation(sub_statement)) = - statements.get(index + 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(_z), - ) = libfunc_sub - { - let user_params = &sub_statement.args; - if !user_params.contains(ret_value) { - self.check_felt252_tainted( - &mut results, - compilation_unit, - op.param_signatures(), - stmt, - invoc.args.clone(), - &name, - ); - } - } else { - self.check_felt252_tainted( - &mut results, - compilation_unit, - op.param_signatures(), - stmt, - invoc.args.clone(), - &name, - ) - } - } - } - - _ => { - self.check_felt252_tainted( - &mut results, - compilation_unit, - op.param_signatures(), - stmt, - invoc.args.clone(), - &name, - ); - } + Felt252BinaryOperationConcrete::WithVar(var) => { + let operation = var.operator; + + self.handle_binops( + &mut results, + compilation_unit, + invoc, + statements, + index, + stmt, + &operation, + &name, + ) } } } @@ -188,16 +95,13 @@ impl Felt252Overflow { &self, results: &mut Vec, compilation_unit: &CompilationUnit, - params: &[ParamSignature], libfunc: &SierraStatement, - args: Vec, + args: &[VarId], name: &String, ) { - let user_params = filter_builtins_from_arguments(params, args); - println!("{:?}", user_params); let mut tainted_by: HashSet<&VarId> = HashSet::new(); let mut taints = String::new(); - for param in user_params.iter() { + 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(¶m) @@ -223,7 +127,7 @@ impl Felt252Overflow { }); } else { let msg = format!( - "The function {} uses the felt252 operation {} with the user-controlled parameters: {}", + "The function {} uses the felt252 operation {} with the user-controlled parameters: {}, which is not overflow safe", &name, libfunc, taints @@ -236,4 +140,57 @@ impl Felt252Overflow { }); } } + #[allow(clippy::too_many_arguments)] + fn handle_binops( + &self, + results: &mut Vec, + compilation_unit: &CompilationUnit, + invoc: &GenInvocation, + 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); + } + } + } } diff --git a/tests/snapshots/integration_tests__detectors@controlled_library_call.cairo.snap b/tests/snapshots/integration_tests__detectors@controlled_library_call.cairo.snap index e209382..1221adc 100644 --- a/tests/snapshots/integration_tests__detectors@controlled_library_call.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@controlled_library_call.cairo.snap @@ -8,12 +8,12 @@ input_file: tests/detectors/controlled_library_call.cairo impact: High, name: "controlled-library-call", confidence: Medium, - message: "Library call to user controlled class hash in controlled_library_call::controlled_library_call::ArbitraryLibraryCall::bad1\n function_call([11], [12], [13], [14], [15]) -> ([7], [8], [9], [10])", + message: "Library call to user controlled class hash in controlled_library_call::controlled_library_call::TestContract::bad1\n function_call([11], [12], [13], [14], [15]) -> ([7], [8], [9], [10])", }, Result { impact: High, name: "controlled-library-call", confidence: Medium, - message: "Library call to user controlled class hash in controlled_library_call::controlled_library_call::ArbitraryLibraryCall::internal_lib_call\n function_call([10], [11], [12], [13], [14]) -> ([6], [7], [8], [9])", + message: "Library call to user controlled class hash in controlled_library_call::controlled_library_call::TestContract::internal_lib_call\n function_call([10], [11], [12], [13], [14]) -> ([6], [7], [8], [9])", }, ] diff --git a/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap b/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap index bf1f3b3..91b72a8 100644 --- a/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@felt252_overflow.cairo.snap @@ -8,19 +8,19 @@ input_file: tests/detectors/felt252_overflow.cairo impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add uses the felt252 operation felt252_add([0], [1]) -> ([6]) with the user-controlled parameters: [0],[1]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add uses the felt252 operation felt252_add([0], [1]) -> ([6]) with the user-controlled parameters: [0],[1], which is not overflow safe", }, Result { impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt252 operation felt252_add([17], [3]) -> ([23]) with the user-controlled parameters: [3]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt252 operation felt252_add([17], [3]) -> ([23]) with the user-controlled parameters: [3], which is not overflow safe", }, Result { impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt252 operation felt252_add([23], [4]) -> ([24]) with the user-controlled parameters: [23],[4]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_add_controlled uses the felt252 operation felt252_add([23], [4]) -> ([24]) with the user-controlled parameters: [23],[4], which is not overflow safe", }, Result { impact: High, @@ -32,25 +32,25 @@ input_file: tests/detectors/felt252_overflow.cairo impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([6], [5]) -> ([10]) with the user-controlled parameters: [6],[5]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([6], [5]) -> ([10]) with the user-controlled parameters: [6],[5], which is not overflow safe", }, Result { impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([7], [4]) -> ([6]) with the user-controlled parameters: [7],[4]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([7], [4]) -> ([6]) with the user-controlled parameters: [7],[4], which is not overflow safe", }, Result { impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([9], [3]) -> ([8]) with the user-controlled parameters: [9],[3]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_mul_controlled uses the felt252 operation felt252_mul([9], [3]) -> ([8]) with the user-controlled parameters: [9],[3], which is not overflow safe", }, Result { impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_sub_controlled uses the felt252 operation felt252_sub([16], [3]) -> ([17]) with the user-controlled parameters: [3]", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::bad_sub_controlled uses the felt252 operation felt252_sub([16], [3]) -> ([17]) with the user-controlled parameters: [3], which is not overflow safe", }, Result { impact: High, @@ -62,7 +62,7 @@ input_file: tests/detectors/felt252_overflow.cairo impact: High, name: "felt252-overflow", confidence: Medium, - message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::test_sub_assert uses the felt252 operation felt252_sub([11], [12]) -> ([13]), which is not overflow safe", + message: "The function felt252_overflow::felt252_overflow::Felt252Overflow::test_sub_assert uses the felt252 operation felt252_sub([1], [12]) -> ([13]), which is not overflow safe", }, Result { impact: Medium, diff --git a/tests/snapshots/integration_tests__detectors@unchecked_l1_handler_from.cairo.snap b/tests/snapshots/integration_tests__detectors@unchecked_l1_handler_from.cairo.snap index a3d9ede..2506fd2 100644 --- a/tests/snapshots/integration_tests__detectors@unchecked_l1_handler_from.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@unchecked_l1_handler_from.cairo.snap @@ -1,8 +1,33 @@ --- source: tests/integration_tests.rs expression: results +input_file: tests/detectors/unchecked_l1_handler_from.cairo --- [ + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function unchecked_l1_handler_from::unchecked_l1_handler_from::UncheckedL1HandlerFrom::bad uses the felt252 operation felt252_add([1], [2]) -> ([3]) with the user-controlled parameters: [1], which is not overflow safe", + }, + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function unchecked_l1_handler_from::unchecked_l1_handler_from::UncheckedL1HandlerFrom::check_recursive uses the felt252 operation felt252_add([3], [50]) -> ([51]), which is not overflow safe", + }, + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function unchecked_l1_handler_from::unchecked_l1_handler_from::UncheckedL1HandlerFrom::check_recursive uses the felt252 operation felt252_sub([11], [9]) -> ([10]), which is not overflow safe", + }, + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function unchecked_l1_handler_from::unchecked_l1_handler_from::UncheckedL1HandlerFrom::good3 uses the felt252 operation felt252_add([14], [15]) -> ([16]) with the user-controlled parameters: [14], which is not overflow safe", + }, Result { impact: High, name: "unchecked-l1-handler-from", diff --git a/tests/snapshots/integration_tests__detectors@unused_return.cairo.snap b/tests/snapshots/integration_tests__detectors@unused_return.cairo.snap index 7fe636d..7f41c0c 100644 --- a/tests/snapshots/integration_tests__detectors@unused_return.cairo.snap +++ b/tests/snapshots/integration_tests__detectors@unused_return.cairo.snap @@ -4,6 +4,12 @@ expression: results input_file: tests/detectors/unused_return.cairo --- [ + Result { + impact: High, + name: "felt252-overflow", + confidence: Medium, + message: "The function unused_return::unused_return::UnusedReturn::f_5 uses the felt252 operation felt252_mul([14], [15]) -> ([16]), which is not overflow safe", + }, Result { impact: Medium, name: "unused-return",