Add safe external calls option (#59)

crytic · Jan 25, 2024 · ff0e710 · ff0e710
1 parent 7b4d571
commit ff0e710
Show file tree

Hide file tree

Showing 12 changed files with 112 additions and 18 deletions.
diff --git a/src/cli/commands/detect/mod.rs b/src/cli/commands/detect/mod.rs
@@ -22,6 +22,10 @@ pub struct DetectArgs {
     #[arg(long, num_args(0..))]
     contract_path: Option<Vec<String>>,
 
+    /// Functions name that are safe when called (e.g. they don't cause a reentrancy)
+    #[arg(long, num_args(0..))]
+    safe_external_calls: Option<Vec<String>>,
+
     /// Detectors to run
     #[arg(long, num_args(0..), conflicts_with_all(["exclude", "exclude_informational", "exclude_low", "exclude_medium", "exclude_high"]))]
     detect: Option<Vec<String>>,
@@ -53,6 +57,7 @@ impl From<&DetectArgs> for CoreOpts {
             target: args.target.clone(),
             corelib: args.corelib.clone(),
             contract_path: args.contract_path.clone(),
+            safe_external_calls: args.safe_external_calls.clone(),
         }
     }
 }

diff --git a/src/cli/commands/print/mod.rs b/src/cli/commands/print/mod.rs
@@ -18,6 +18,10 @@ pub struct PrintArgs {
     #[arg(long, num_args(0..))]
     contract_path: Option<Vec<String>>,
 
+    /// Functions name that are safe when called (e.g. they don't cause a reentrancy)
+    #[arg(long, num_args(0..))]
+    safe_external_calls: Option<Vec<String>>,
+
     /// Which functions to run the printer (all, user-functions)   
     #[arg(short, long, default_value_t = Filter::UserFunctions)]
     filter: Filter,
@@ -33,6 +37,7 @@ impl From<&PrintArgs> for CoreOpts {
             target: args.target.clone(),
             corelib: args.corelib.clone(),
             contract_path: args.contract_path.clone(),
+            safe_external_calls: args.safe_external_calls.clone(),
         }
     }
 }

diff --git a/src/core/core_unit.rs b/src/core/core_unit.rs
@@ -10,14 +10,17 @@ pub struct CoreOpts {
     pub target: PathBuf,
     pub corelib: Option<PathBuf>,
     pub contract_path: Option<Vec<String>>,
+    pub safe_external_calls: Option<Vec<String>>,
 }
 
 pub struct CoreUnit {
     compilation_units: Vec<CompilationUnit>,
+    safe_external_calls: Option<Vec<String>>,
 }
 
 impl CoreUnit {
     pub fn new(opts: CoreOpts) -> Result<Self> {
+        let safe_external_calls = opts.safe_external_calls.clone();
         let program_compiled = compile(opts)?;
         let compilation_units = program_compiled
             .par_iter()
@@ -31,10 +34,17 @@ impl CoreUnit {
                 compilation_unit
             })
             .collect();
-        Ok(CoreUnit { compilation_units })
+        Ok(CoreUnit {
+            compilation_units,
+            safe_external_calls,
+        })
     }
 
     pub fn get_compilation_units(&self) -> &Vec<CompilationUnit> {
         &self.compilation_units
     }
+
+    pub fn get_safe_external_calls(&self) -> &Option<Vec<String>> {
+        &self.safe_external_calls
+    }
 }
diff --git a/src/detectors/read_only_reentrancy.rs b/src/detectors/read_only_reentrancy.rs
@@ -58,6 +58,20 @@ impl Detector for ReadOnlyReentrancy {
                     } = bb_info.1
                     {
                         for call in reentrancy_info.external_calls.iter() {
+                            let external_function_call = format!(
+                                "{}",
+                                call.get_external_call().as_ref().unwrap().get_statement()
+                            );
+
+                            if let Some(safe_external_calls) = core.get_safe_external_calls() {
+                                if safe_external_calls
+                                    .iter()
+                                    .any(|f_name| external_function_call.contains(f_name))
+                                {
+                                    continue;
+                                }
+                            }
+
                             for written_variable in reentrancy_info.storage_variables_written.iter()
                             {
                                 let written_variable_name = written_variable
@@ -82,10 +96,7 @@ impl Detector for ReadOnlyReentrancy {
                                             message: format!(
                                                 "Read only reentrancy in {}\n\tExternal call {} done in {}\n\tVariable written after {} in {}",
                                                 view_function,
-                                                call.get_external_call()
-                                                    .as_ref()
-                                                    .unwrap()
-                                                    .get_statement(),
+                                                external_function_call,
                                                 call.get_function(),
                                                 written_variable
                                                     .get_storage_variable_written()

diff --git a/src/detectors/reentrancy.rs b/src/detectors/reentrancy.rs
@@ -38,6 +38,20 @@ impl Detector for Reentrancy {
                     } = bb_info.1
                     {
                         for call in reentrancy_info.external_calls.iter() {
+                            let external_function_call = format!(
+                                "{}",
+                                call.get_external_call().as_ref().unwrap().get_statement()
+                            );
+
+                            if let Some(safe_external_calls) = core.get_safe_external_calls() {
+                                if safe_external_calls
+                                    .iter()
+                                    .any(|f_name| external_function_call.contains(f_name))
+                                {
+                                    continue;
+                                }
+                            }
+
                             if let Some(current_vars_read_before_call) = reentrancy_info
                                 .variables_read_before_calls
                                 .iter()
@@ -79,10 +93,7 @@ impl Detector for Reentrancy {
                                             message: format!(
                                                 "Reentrancy in {}\n\tExternal call {} done in {}\n\tVariable written after {} in {}.",
                                                 f.name(),
-                                                call.get_external_call()
-                                                    .as_ref()
-                                                    .unwrap()
-                                                    .get_statement(),
+                                                external_function_call,
                                                 call.get_function(),
                                                 written_variable
                                                     .get_storage_variable_written()

diff --git a/src/detectors/reentrancy_benign.rs b/src/detectors/reentrancy_benign.rs
@@ -38,6 +38,20 @@ impl Detector for ReentrancyBenign {
                     } = bb_info.1
                     {
                         for call in reentrancy_info.external_calls.iter() {
+                            let external_function_call = format!(
+                                "{}",
+                                call.get_external_call().as_ref().unwrap().get_statement()
+                            );
+
+                            if let Some(safe_external_calls) = core.get_safe_external_calls() {
+                                if safe_external_calls
+                                    .iter()
+                                    .any(|f_name| external_function_call.contains(f_name))
+                                {
+                                    continue;
+                                }
+                            }
+
                             if let Some(current_vars_read_before_call) = reentrancy_info
                                 .variables_read_before_calls
                                 .iter()
@@ -79,10 +93,7 @@ impl Detector for ReentrancyBenign {
                                             message: format!(
                                                 "Reentrancy in {}\n\tExternal call {} done in {}\n\tVariable written after {} in {}.",
                                                 f.name(),
-                                                call.get_external_call()
-                                                    .as_ref()
-                                                    .unwrap()
-                                                    .get_statement(),
+                                                external_function_call,
                                                 call.get_function(),
                                                 written_variable
                                                     .get_storage_variable_written()

diff --git a/src/detectors/reentrancy_events.rs b/src/detectors/reentrancy_events.rs
@@ -39,17 +39,28 @@ impl Detector for ReentrancyEvents {
                     {
                         for event in reentrancy_info.events.iter() {
                             for call in reentrancy_info.external_calls.iter() {
+                                let external_function_call = format!(
+                                    "{}",
+                                    call.get_external_call().as_ref().unwrap().get_statement()
+                                );
+
+                                if let Some(safe_external_calls) = core.get_safe_external_calls() {
+                                    if safe_external_calls
+                                        .iter()
+                                        .any(|f_name| external_function_call.contains(f_name))
+                                    {
+                                        continue;
+                                    }
+                                }
+
                                 results.insert(Result {
                                     name: self.name().to_string(),
                                     impact: self.impact(),
                                     confidence: self.confidence(),
                                     message: format!(
                                         "Reentrancy in {}\n\tExternal call {} done in {}\n\tEvent emitted after {} in {}.",
                                         f.name(),
-                                        call.get_external_call()
-                                            .as_ref()
-                                            .unwrap()
-                                            .get_statement(),
+                                        external_function_call,
                                         call.get_function(),
                                         event.get_event_emitted().as_ref().unwrap().get_statement(),
                                         event.get_function()

diff --git a/tests/detectors/read_only_reentrancy.cairo b/tests/detectors/read_only_reentrancy.cairo
@@ -1,6 +1,7 @@
 #[starknet::interface]
 trait IAnotherContract<T> {
     fn foo(self: @T, a: felt252);
+    fn safe_foo(self: @T, a: felt252);
 }
 
 #[starknet::contract]
@@ -32,8 +33,14 @@ mod TestContract {
     }
 
     #[external(v0)]
-    fn ok(ref self: ContractState, address: ContractAddress) {
+    fn good1(ref self: ContractState, address: ContractAddress) {
         IAnotherContractDispatcher { contract_address: address }.foo(4);
     }
 
+    #[external(v0)]
+    fn good2(ref self: ContractState, address: ContractAddress) {
+        IAnotherContractDispatcher { contract_address: address }.safe_foo(4);
+        self.a.write(4);
+    }
+
 }
diff --git a/tests/detectors/reentrancy.cairo b/tests/detectors/reentrancy.cairo
@@ -1,6 +1,7 @@
 #[starknet::interface]
 trait IAnotherContract<T> {
     fn foo(self: @T, a: felt252);
+    fn safe_foo(self: @T, a: felt252);
 }
 
 #[starknet::contract]
@@ -22,6 +23,13 @@ mod TestContract {
         IAnotherContractDispatcher { contract_address: address }.foo(a);
     }
 
+    #[external(v0)]
+    fn good2(ref self: ContractState, address: ContractAddress) {
+        let a = self.a.read();
+        IAnotherContractDispatcher { contract_address: address }.safe_foo(a);
+        self.a.write(4);
+    }
+
     #[external(v0)]
     fn bad1(ref self: ContractState, address: ContractAddress) {
         let a = self.a.read();

diff --git a/tests/detectors/reentrancy_benign.cairo b/tests/detectors/reentrancy_benign.cairo
@@ -1,6 +1,7 @@
 #[starknet::interface]
 trait IAnotherContract<T> {
     fn foo(self: @T, a: felt252);
+    fn safe_foo(self: @T, a: felt252);
 }
 
 #[starknet::contract]
@@ -22,6 +23,12 @@ mod TestContract {
         IAnotherContractDispatcher { contract_address: address }.foo(a);
     }
 
+    #[external(v0)]
+    fn good2(ref self: ContractState, address: ContractAddress) {
+        IAnotherContractDispatcher { contract_address: address }.safe_foo(4);
+        self.a.write(4);
+    }
+
     #[external(v0)]
     fn bad1(ref self: ContractState, address: ContractAddress) {
         IAnotherContractDispatcher { contract_address: address }.foo(4);

diff --git a/tests/detectors/reentrancy_events.cairo b/tests/detectors/reentrancy_events.cairo
@@ -1,6 +1,7 @@
 #[starknet::interface]
 trait IAnotherContract<T> {
     fn foo(self: @T, a: felt252);
+    fn safe_foo(self: @T, a: felt252);
 }
 
 #[starknet::contract]
@@ -27,6 +28,12 @@ mod TestContract {
         IAnotherContractDispatcher { contract_address: address }.foo(4);
     }
 
+    #[external(v0)]
+    fn good2(ref self: ContractState, address: ContractAddress) {
+        IAnotherContractDispatcher { contract_address: address }.safe_foo(4);
+        self.emit(MyEvent { });
+    }
+
     #[external(v0)]
     fn bad1(ref self: ContractState, address: ContractAddress) {
         IAnotherContractDispatcher { contract_address: address }.foo(4);

diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs
@@ -12,6 +12,7 @@ fn test_detectors() {
                 env::var("CARGO_MANIFEST_DIR").unwrap() + "/corelib/src",
             )),
             contract_path: None,
+            safe_external_calls: Some(vec!["::safe_foo".to_string()]),
         };
         let core = CoreUnit::new(opts).unwrap();
         let mut results = get_detectors()