Added: Slither[arbitrary from in transferFrom] rule | 0xKitsune#61

src/analyzer/vulnerabilities/arbitrary_from_in_transferfrom.rs

@@ -0,0 +1,123 @@
+use std::collections::HashSet;
+
+use solang_parser::pt::{self, Expression, Loc};
+use solang_parser::{self, pt::SourceUnit};
+
+use crate::analyzer::ast::{self, Target};
+
+pub fn arbitrary_from_in_transferfrom_vulnerability(source_unit: SourceUnit) -> HashSet<Loc> {
+    //Create a new hashset that stores the location of each vulnerability target identified
+    let mut vulnerability_locations: HashSet<Loc> = HashSet::new();
+
+    //Extract the target nodes from the source_unit
+    let target_nodes =
+        ast::extract_target_from_node(Target::FunctionDefinition, source_unit.into());
+
+    //For each target node that was extracted, check for the vulnerability patterns
+    for node in target_nodes {
+        let contract_part = node.contract_part().unwrap();
+
+        if let pt::ContractPart::FunctionDefinition(box_fn_definition) = contract_part {
+            // if function has no params or no body, it is probably not affected by this vulnerability.
+            if box_fn_definition.params.is_empty() || box_fn_definition.body.is_none() {
+                continue;
+            }
+
+            // We extract the function parameters to later verify if they are used as the first parameter in the `transferFrom/SafeTransferFrom`.
+            let fn_params = extract_fn_params_as_string(&box_fn_definition);
+
+            if let pt::Statement::Block {
+                loc: _,
+                unchecked: _,
+                statements,
+            } = box_fn_definition.body.unwrap()
+            {
+                // We loop through each body expression to determine if 'transferFrom/SafeTransferFrom' is used.
+                for statement in statements {
+                    if let pt::Statement::Expression(_, box_expression) = statement.clone() {
+                        if let pt::Expression::FunctionCall(
+                            loc,
+                            box_fn_expression,
+                            fn_call_params,
+                        ) = box_expression
+                        {
+                            // If a call exists and any of the function parameters is used as the first parameter in the call, we mark it as a vulnerability.
+                            if have_transfer_from_expression(box_fn_expression)
+                                && transfer_from_uses_fn_params_as_from_variable(
+                                    &fn_params,
+                                    &fn_call_params[0],
+                                )
+                            {
+                                vulnerability_locations.insert(loc);
+                            }
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+    //Return the identified vulnerability locations
+    vulnerability_locations
+}
+
+fn extract_fn_params_as_string(box_fn_definition: &Box<pt::FunctionDefinition>) -> Vec<String> {
+    return box_fn_definition
+        .params
+        .iter()
+        .filter(|v| v.clone().1.is_some())
+        .map(|v| v.clone().1.unwrap().name.unwrap().name)
+        .collect();
+}
+
+fn have_transfer_from_expression(box_fn_expression: Box<pt::Expression>) -> bool {
+    const TRANSFER_FROM: &str = "safetransferfrom";
+    const SAFE_TRANSFER_FROM: &str = "transferfrom";
+
+    if let pt::Expression::MemberAccess(_, _, member_identifier) = *box_fn_expression {
+        let name = member_identifier.name.to_lowercase();
+        return name == TRANSFER_FROM || name == SAFE_TRANSFER_FROM;
+    }
+
+    return false;
+}
+
+fn transfer_from_uses_fn_params_as_from_variable(
+    fn_params: &Vec<String>,
+    from_param: &Expression,
+) -> bool {
+    if let pt::Expression::Variable(var_identifier) = from_param {
+        // If any funtion parameter is used as `from` paramenter
+        return fn_params.contains(&var_identifier.name);
+    }
+    return false;
+}
+
+#[test]
+fn testarbitrary_from_in_transferfrom_vulnerability() {
+    let file_contents = r#"
+    
+    contract Contract0 {
+        function a(address from, address to, uint256 amount) public {
+            erc20.transferFrom(from, to, amount);
+        }
+
+        function b(address to, uint256 amount) public {
+            erc20.transferFrom(msg.sender, to, amount);
+        }
+
+        function c(address _from, uint256 amount) public {
+            erc20.transferFrom(_from, msg.sender, amount);
+        }
+
+        function d(address _from, address _to, uint256 amount) public {
+            erc20.safeTransferFrom(_from, _to, amount);
+        }
+    }
+    "#;
+
+    let source_unit = solang_parser::parse(file_contents, 0).unwrap().0;
+
+    let vulnerability_locations = arbitrary_from_in_transferfrom_vulnerability(source_unit);
+    assert_eq!(vulnerability_locations.len(), 3)
+}

src/analyzer/vulnerabilities/mod.rs

@@ -1,21 +1,19 @@
+pub mod arbitrary_from_in_transferfrom;
 pub mod divide_before_multiply;
 pub mod floating_pragma;
 pub mod template;
 pub mod unprotected_selfdestruct;
 pub mod unsafe_erc20_operation;
 
 use std::{
-    collections::{BTreeSet, HashMap, HashSet},
-    env, fs,
-    path::PathBuf,
-    str::FromStr,
+    collections::{BTreeSet, HashMap},
+    fs,
 };
 
-use solang_parser::pt::SourceUnit;
-
 use super::utils::{self, LineNumber};
 
 use self::{
+    arbitrary_from_in_transferfrom::arbitrary_from_in_transferfrom_vulnerability,
     divide_before_multiply::divide_before_multiply_vulnerability,
     floating_pragma::floating_pragma_vulnerability,
     unprotected_selfdestruct::unprotected_selfdestruct_vulnerability,
@@ -29,6 +27,7 @@ pub enum Vulnerability {
     UnsafeERC20Operation,
     UnprotectedSelfdestruct,
     DivideBeforeMultiply,
+    ArbitraryFromInTransferFrom,
 }
 
 pub fn get_all_vulnerabilities() -> Vec<Vulnerability> {
@@ -37,6 +36,7 @@ pub fn get_all_vulnerabilities() -> Vec<Vulnerability> {
         Vulnerability::UnprotectedSelfdestruct,
         Vulnerability::DivideBeforeMultiply,
         Vulnerability::FloatingPragma,
+        Vulnerability::ArbitraryFromInTransferFrom,
     ]
 }
 
@@ -46,6 +46,7 @@ pub fn str_to_vulnerability(vuln: &str) -> Vulnerability {
         "unsafe_erc20_operation" => Vulnerability::UnsafeERC20Operation,
         "unprotected_selfdestruct" => Vulnerability::UnprotectedSelfdestruct,
         "divide_before_multiply" => Vulnerability::DivideBeforeMultiply,
+        "arbitrary_from_in_transferfrom" => Vulnerability::ArbitraryFromInTransferFrom,
         other => {
             panic!("Unrecgonized vulnerability: {}", other)
         }
@@ -126,6 +127,9 @@ pub fn analyze_for_vulnerability(
             unprotected_selfdestruct_vulnerability(source_unit)
         }
         Vulnerability::DivideBeforeMultiply => divide_before_multiply_vulnerability(source_unit),
+        Vulnerability::ArbitraryFromInTransferFrom => {
+            arbitrary_from_in_transferfrom_vulnerability(source_unit)
+        }
     };
 
     for loc in locations {

src/report/report_sections/vulnerabilities/arbitrary_from_in_transferfrom.rs

@@ -0,0 +1,31 @@
+pub fn report_section_content() -> String {
+    String::from(
+        r##" 
+## Arbitrary `from` in transferFrom/safeTransferFrom.
+
+Use `msg.sender` as `from` in `transferFrom/safeTransferFrom`.
+
+### Exploit Scenario:
+
+```js
+/** 
+ * Alice approves this contract to spend her ERC20
+ * tokens. Bob can call a and specify Alice's address as
+ * the from parameter in transferFrom, allowing him to
+ * transfer Alice's tokens to himself.
+ * */
+
+function withdraw(address from, address to, uint256 amount) public {
+    erc20.transferFrom(from, to, am);
+}
+
+// or
+
+function emergencyWithdraw(address from, address to, uint256 amount) public {
+    erc20.safeTransferFrom(from, to, am);
+}
+
+```
+    "##,
+    )
+}

src/report/report_sections/vulnerabilities/mod.rs

@@ -1,3 +1,4 @@
+pub mod arbitrary_from_in_transferfrom;
 pub mod divide_before_multiply;
 pub mod floating_pragma;
 pub mod overview;

src/report/vulnerability_report.rs

@@ -9,6 +9,8 @@ use crate::report::report_sections::vulnerabilities::{
     unsafe_erc20_operation,
 };
 
+use super::report_sections::vulnerabilities::arbitrary_from_in_transferfrom;
+
 pub fn generate_vulnerability_report(
     vulnerabilities: HashMap<Vulnerability, Vec<(String, BTreeSet<LineNumber>)>>,
 ) -> String {
@@ -106,5 +108,9 @@ pub fn get_vulnerability_report_section(
             divide_before_multiply::report_section_content(),
             VulnerabilitySeverity::Medium,
         ),
+        Vulnerability::ArbitraryFromInTransferFrom => (
+            arbitrary_from_in_transferfrom::report_section_content(),
+            VulnerabilitySeverity::High,
+        ),
     }
 }