From c4b01e5ceaee69cf550674c2b8e95788d8113ff3 Mon Sep 17 00:00:00 2001 From: Maxi Date: Fri, 10 Feb 2023 23:35:50 -0300 Subject: [PATCH] Added: Slither[arbitrary from in transferFrom] rule | #61 --- .../arbitrary_from_in_transferfrom.rs | 133 ++++++++++++++++++ src/analyzer/vulnerabilities/mod.rs | 16 ++- .../arbitrary_from_in_transferfrom.rs | 31 ++++ .../report_sections/vulnerabilities/mod.rs | 1 + src/report/vulnerability_report.rs | 6 + 5 files changed, 181 insertions(+), 6 deletions(-) create mode 100644 src/analyzer/vulnerabilities/arbitrary_from_in_transferfrom.rs create mode 100644 src/report/report_sections/vulnerabilities/arbitrary_from_in_transferfrom.rs diff --git a/src/analyzer/vulnerabilities/arbitrary_from_in_transferfrom.rs b/src/analyzer/vulnerabilities/arbitrary_from_in_transferfrom.rs new file mode 100644 index 0000000..d6bd569 --- /dev/null +++ b/src/analyzer/vulnerabilities/arbitrary_from_in_transferfrom.rs @@ -0,0 +1,133 @@ +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 { + //Create a new hashset that stores the location of each vulnerability target identified + let mut vulnerability_locations: HashSet = 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) { + if 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) -> Vec { + return box_fn_definition + .params + .iter() + .filter(|v| v.clone().1.is_some()) + .map(|v| v.clone().1.unwrap()) + .filter(|v| v.name.is_some()) + .map(|v| v.name.unwrap().name) + .collect(); +} + +fn have_transfer_from_expression(box_fn_expression: Box) -> 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, + 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); + } + + function e() public { + erc20.safeTransferFrom(msg.sender, address(this), amount); + } + + function f(address _token, address _sender, uint256 amount) public { + _token.transferFrom(_sender, address(this), 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(), 4) +} diff --git a/src/analyzer/vulnerabilities/mod.rs b/src/analyzer/vulnerabilities/mod.rs index 2eb3cb4..e88119e 100644 --- a/src/analyzer/vulnerabilities/mod.rs +++ b/src/analyzer/vulnerabilities/mod.rs @@ -1,3 +1,4 @@ +pub mod arbitrary_from_in_transferfrom; pub mod divide_before_multiply; pub mod floating_pragma; pub mod template; @@ -5,17 +6,14 @@ 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 { @@ -37,6 +36,7 @@ pub fn get_all_vulnerabilities() -> Vec { 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 { diff --git a/src/report/report_sections/vulnerabilities/arbitrary_from_in_transferfrom.rs b/src/report/report_sections/vulnerabilities/arbitrary_from_in_transferfrom.rs new file mode 100644 index 0000000..23e6f23 --- /dev/null +++ b/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); +} + +``` + "##, + ) +} diff --git a/src/report/report_sections/vulnerabilities/mod.rs b/src/report/report_sections/vulnerabilities/mod.rs index f9e609a..b68902b 100644 --- a/src/report/report_sections/vulnerabilities/mod.rs +++ b/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; diff --git a/src/report/vulnerability_report.rs b/src/report/vulnerability_report.rs index 277147d..9b44980 100644 --- a/src/report/vulnerability_report.rs +++ b/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)>>, ) -> 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, + ), } }