From b8d6c4f3f425a402155598e63a6364847e338c7d Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Wed, 12 Jun 2024 14:11:31 -0300 Subject: [PATCH 1/4] Add vulnerable and remediated examples --- test-cases/front-running/Cargo.toml | 22 +++ .../remediated-example/Cargo.toml | 17 ++ .../remediated-example/src/lib.rs | 173 ++++++++++++++++++ .../vulnerable-example/Cargo.toml | 17 ++ .../vulnerable-example/src/lib.rs | 142 ++++++++++++++ 5 files changed, 371 insertions(+) create mode 100644 test-cases/front-running/Cargo.toml create mode 100644 test-cases/front-running/front-running-1/remediated-example/Cargo.toml create mode 100644 test-cases/front-running/front-running-1/remediated-example/src/lib.rs create mode 100644 test-cases/front-running/front-running-1/vulnerable-example/Cargo.toml create mode 100644 test-cases/front-running/front-running-1/vulnerable-example/src/lib.rs diff --git a/test-cases/front-running/Cargo.toml b/test-cases/front-running/Cargo.toml new file mode 100644 index 00000000..5a1bfab3 --- /dev/null +++ b/test-cases/front-running/Cargo.toml @@ -0,0 +1,22 @@ +[workspace] +exclude = [".cargo", "target"] +members = ["front-running-*/*"] +resolver = "2" + +[workspace.dependencies] +soroban-sdk = { version = "=20.0.0" } + +[profile.release] +codegen-units = 1 +debug = 0 +debug-assertions = false +lto = true +opt-level = "z" +overflow-checks = true +panic = "abort" +strip = "symbols" + +[profile.release-with-logs] +debug-assertions = true +inherits = "release" + diff --git a/test-cases/front-running/front-running-1/remediated-example/Cargo.toml b/test-cases/front-running/front-running-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..55f2be0b --- /dev/null +++ b/test-cases/front-running/front-running-1/remediated-example/Cargo.toml @@ -0,0 +1,17 @@ +[package] +edition = "2021" +name = "front-running-remediated-1" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "20.0.0" } +soroban-token-sdk = { version = "20.0.0" } + +[dev_dependencies] +soroban-sdk = { version = "20.0.0", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/front-running/front-running-1/remediated-example/src/lib.rs b/test-cases/front-running/front-running-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..6795865a --- /dev/null +++ b/test-cases/front-running/front-running-1/remediated-example/src/lib.rs @@ -0,0 +1,173 @@ +#![no_std] + +use soroban_sdk::{ + contract, contractimpl, contracttype, + token::{StellarAssetClient, TokenClient}, + Address, Env, +}; + +#[contracttype] +pub enum DataKey { + Token, +} + +#[contract] +pub struct FrontRunning; + +#[contractimpl] +impl FrontRunning { + pub fn init(e: Env, contract: Address) { + e.storage().persistent().set(&DataKey::Token, &contract); + } + + pub fn get_token(e: Env) -> Address { + get_token(&e) + } + + pub fn mint(e: Env, to: Address, amount: i128) { + StellarAssetClient::new(&e, &get_token(&e)).mint(&to, &amount); + } + + pub fn approve(e: Env, from: Address, spender: Address, amount: i128, expiration_ledger: u32) { + TokenClient::new(&e, &get_token(&e)).approve(&from, &spender, &amount, &expiration_ledger); + } + + pub fn transfer(e: Env, from: Address, to: Address, amount: i128, min_amount: i128) { + let transfer_amount = get_conversion_price(amount); + assert!(transfer_amount >= min_amount, "Insufficient amount"); + TokenClient::new(&e, &get_token(&e)).transfer(&from, &to, &transfer_amount); + } + + pub fn allowance(e: Env, from: Address, spender: Address) -> i128 { + TokenClient::new(&e, &get_token(&e)).allowance(&from, &spender) + } +} + +fn get_token(e: &Env) -> Address { + e.storage().persistent().get(&DataKey::Token).unwrap() +} + +fn get_conversion_price(amount: i128) -> i128 { + // This function symbolizes a change in the state of the blockchain + 100 * amount +} + +#[cfg(test)] +mod tests { + extern crate std; + + use soroban_sdk::{ + testutils::{AuthorizedFunction, AuthorizedInvocation}, + token::TokenClient, + Address, Env, IntoVal, Symbol, + }; + + use crate::{FrontRunning, FrontRunningClient}; + use soroban_sdk::testutils::Address as _; + + #[test] + fn test_approve() { + // Given + let env = Env::default(); + let admin = Address::generate(&env); + let token_contract_id = env.register_stellar_asset_contract(admin); + + let contract_id = env.register_contract(None, FrontRunning); + let client = FrontRunningClient::new(&env, &contract_id); + client.init(&token_contract_id); + + // When + let from = Address::generate(&env); + let spender = Address::generate(&env); + + // Then + client + .mock_all_auths_allowing_non_root_auth() + .approve(&from, &spender, &200, &200); + + let auths = env.auths(); + assert_eq!( + auths, + std::vec![( + from.clone(), + AuthorizedInvocation { + function: AuthorizedFunction::Contract(( + token_contract_id.clone(), + Symbol::new(&env, "approve"), + (&from, &spender, 200_i128, 200_u32).into_val(&env) + )), + sub_invocations: std::vec![] + } + )] + ); + + // Check allowance + let token_client = TokenClient::new(&env, &client.get_token()); + assert_eq!(token_client.allowance(&from, &spender), 200); + } + + #[test] + fn test_transfer() { + // Given + let env = Env::default(); + let admin = Address::generate(&env); + let token_contract_id = env.register_stellar_asset_contract(admin); + + let contract_id = env.register_contract(None, FrontRunning); + let client = FrontRunningClient::new(&env, &contract_id); + client.init(&token_contract_id); + + // When + let token_client = TokenClient::new(&env, &client.get_token()); + assert_eq!(token_client.decimals(), 7); + let from = Address::generate(&env); + let to = Address::generate(&env); + + // Mint tokens to the `from` address + client + .mock_all_auths_allowing_non_root_auth() + .mint(&from, &10000); + + // Then + client + .mock_all_auths_allowing_non_root_auth() + .transfer(&from, &to, &1, &0); + + // Check final balances + let from_balance = token_client.balance(&from); + let to_balance = token_client.balance(&to); + + assert_eq!(from_balance, 10000 - 100); + assert_eq!(to_balance, 100); + } + + #[test] + #[should_panic(expected = "Insufficient amount")] + fn test_front_running() { + // Given + let env = Env::default(); + let admin = Address::generate(&env); + let token_contract_id = env.register_stellar_asset_contract(admin); + + let contract_id = env.register_contract(None, FrontRunning); + let client = FrontRunningClient::new(&env, &contract_id); + client.init(&token_contract_id); + + // When + let token_client = TokenClient::new(&env, &client.get_token()); + assert_eq!(token_client.decimals(), 7); + let from = Address::generate(&env); + let to = Address::generate(&env); + + // Mint tokens to the `from` address + client + .mock_all_auths_allowing_non_root_auth() + .mint(&from, &10000); + + // Then + client + .mock_all_auths_allowing_non_root_auth() + .transfer(&from, &to, &100, &10001); + // The contract should now panic since the transfer amount is less than the minimum amount + } +} diff --git a/test-cases/front-running/front-running-1/vulnerable-example/Cargo.toml b/test-cases/front-running/front-running-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..4d7ccac0 --- /dev/null +++ b/test-cases/front-running/front-running-1/vulnerable-example/Cargo.toml @@ -0,0 +1,17 @@ +[package] +edition = "2021" +name = "front-running-vulnerable-1" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "20.0.0" } +soroban-token-sdk = { version = "20.0.0" } + +[dev_dependencies] +soroban-sdk = { version = "20.0.0", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/front-running/front-running-1/vulnerable-example/src/lib.rs b/test-cases/front-running/front-running-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..b7fbf131 --- /dev/null +++ b/test-cases/front-running/front-running-1/vulnerable-example/src/lib.rs @@ -0,0 +1,142 @@ +#![no_std] + +use soroban_sdk::{ + contract, contractimpl, contracttype, + token::{StellarAssetClient, TokenClient}, + Address, Env, +}; + +#[contracttype] +pub enum DataKey { + Token, +} + +#[contract] +pub struct FrontRunning; + +#[contractimpl] +impl FrontRunning { + pub fn init(e: Env, contract: Address) { + e.storage().persistent().set(&DataKey::Token, &contract); + } + + pub fn get_token(e: Env) -> Address { + get_token(&e) + } + + pub fn mint(e: Env, to: Address, amount: i128) { + StellarAssetClient::new(&e, &get_token(&e)).mint(&to, &amount); + } + + pub fn approve(e: Env, from: Address, spender: Address, amount: i128, expiration_ledger: u32) { + TokenClient::new(&e, &get_token(&e)).approve(&from, &spender, &amount, &expiration_ledger); + } + + pub fn transfer(e: Env, from: Address, to: Address, amount: i128) { + let transfer_amount = get_conversion_price(amount); + TokenClient::new(&e, &get_token(&e)).transfer(&from, &to, &transfer_amount); + } + + pub fn allowance(e: Env, from: Address, spender: Address) -> i128 { + TokenClient::new(&e, &get_token(&e)).allowance(&from, &spender) + } +} + +fn get_token(e: &Env) -> Address { + e.storage().persistent().get(&DataKey::Token).unwrap() +} + +fn get_conversion_price(amount: i128) -> i128 { + // This function symbolizes a change in the state of the blockchain + 100 * amount +} + +#[cfg(test)] +mod tests { + extern crate std; + + use soroban_sdk::{ + testutils::{AuthorizedFunction, AuthorizedInvocation}, + token::TokenClient, + Address, Env, IntoVal, Symbol, + }; + + use crate::{FrontRunning, FrontRunningClient}; + use soroban_sdk::testutils::Address as _; + + #[test] + fn test_approve() { + // Given + let env = Env::default(); + let admin = Address::generate(&env); + let token_contract_id = env.register_stellar_asset_contract(admin); + + let contract_id = env.register_contract(None, FrontRunning); + let client = FrontRunningClient::new(&env, &contract_id); + client.init(&token_contract_id); + + // When + let from = Address::generate(&env); + let spender = Address::generate(&env); + + // Then + client + .mock_all_auths_allowing_non_root_auth() + .approve(&from, &spender, &200, &200); + + let auths = env.auths(); + assert_eq!( + auths, + std::vec![( + from.clone(), + AuthorizedInvocation { + function: AuthorizedFunction::Contract(( + token_contract_id.clone(), + Symbol::new(&env, "approve"), + (&from, &spender, 200_i128, 200_u32).into_val(&env) + )), + sub_invocations: std::vec![] + } + )] + ); + + // Check allowance + let token_client = TokenClient::new(&env, &client.get_token()); + assert_eq!(token_client.allowance(&from, &spender), 200); + } + + #[test] + fn test_transfer() { + // Given + let env = Env::default(); + let admin = Address::generate(&env); + let token_contract_id = env.register_stellar_asset_contract(admin); + + let contract_id = env.register_contract(None, FrontRunning); + let client = FrontRunningClient::new(&env, &contract_id); + client.init(&token_contract_id); + + // When + let token_client = TokenClient::new(&env, &client.get_token()); + assert_eq!(token_client.decimals(), 7); + let from = Address::generate(&env); + let to = Address::generate(&env); + + // Mint tokens to the `from` address + client + .mock_all_auths_allowing_non_root_auth() + .mint(&from, &10000); + + // Then + client + .mock_all_auths_allowing_non_root_auth() + .transfer(&from, &to, &1); + + // Check final balances (optional) + let from_balance = token_client.balance(&from); + let to_balance = token_client.balance(&to); + + assert_eq!(from_balance, 10000 - 100); + assert_eq!(to_balance, 100); + } +} From d29585055af06aa0f5e07469fb8cc68a238b56ad Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Thu, 5 Sep 2024 11:53:28 -0300 Subject: [PATCH 2/4] Add front-running detector --- detectors/front-running/Cargo.toml | 17 ++ .../front-running/src/conditional_checker.rs | 64 +++++++ detectors/front-running/src/lib.rs | 175 ++++++++++++++++++ 3 files changed, 256 insertions(+) create mode 100644 detectors/front-running/Cargo.toml create mode 100644 detectors/front-running/src/conditional_checker.rs create mode 100644 detectors/front-running/src/lib.rs diff --git a/detectors/front-running/Cargo.toml b/detectors/front-running/Cargo.toml new file mode 100644 index 00000000..48cb9894 --- /dev/null +++ b/detectors/front-running/Cargo.toml @@ -0,0 +1,17 @@ +[package] +edition = "2021" +name = "front-running" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +clippy_utils = { workspace = true } +clippy_wrappers = { workspace = true } +dylint_linting = { workspace = true } +if_chain = { workspace = true } +utils = { workspace = true } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/detectors/front-running/src/conditional_checker.rs b/detectors/front-running/src/conditional_checker.rs new file mode 100644 index 00000000..5b07747d --- /dev/null +++ b/detectors/front-running/src/conditional_checker.rs @@ -0,0 +1,64 @@ +use rustc_hir::{def::Res, BinOpKind, Expr, ExprKind, HirId, QPath, UnOp}; + +const PANIC_INDUCING_FUNCTIONS: [&str; 2] = ["panic", "bail"]; + +#[derive(Clone, Copy, Hash, Eq, PartialEq, Default, Debug)] +pub struct ConditionalChecker { + pub greater_expr: Option, + pub lesser_expr: Option, +} + +impl ConditionalChecker { + fn get_res_hir_id(&self, expr: &Expr) -> Option { + if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind { + if let Res::Local(hir_id) = path.res { + Some(hir_id) + } else { + None + } + } else { + None + } + } + + fn handle_binary_op(&mut self, op: BinOpKind, left: &Expr, right: &Expr) -> bool { + match op { + BinOpKind::Ge | BinOpKind::Gt => { + self.greater_expr = self.get_res_hir_id(left); + self.lesser_expr = self.get_res_hir_id(right); + true + } + BinOpKind::Le | BinOpKind::Lt => { + self.lesser_expr = self.get_res_hir_id(left); + self.greater_expr = self.get_res_hir_id(right); + true + } + _ => false, + } + } + + pub fn handle_condition(&mut self, condition: &Expr) -> bool { + match &condition.kind { + ExprKind::Binary(op, left, right) => self.handle_binary_op(op.node, left, right), + ExprKind::Unary(UnOp::Not, unary_expr) => { + if let ExprKind::Binary(op, left, right) = &unary_expr.kind { + self.handle_binary_op(op.node, left, right) + } else { + false + } + } + _ => false, + } + } +} + +pub fn is_panic_inducing_call(func: &Expr<'_>) -> bool { + if let ExprKind::Path(QPath::Resolved(_, path)) = &func.kind { + return PANIC_INDUCING_FUNCTIONS.iter().any(|&func| { + path.segments + .iter() + .any(|segment| segment.ident.name.as_str().contains(func)) + }); + } + false +} diff --git a/detectors/front-running/src/lib.rs b/detectors/front-running/src/lib.rs new file mode 100644 index 00000000..219db235 --- /dev/null +++ b/detectors/front-running/src/lib.rs @@ -0,0 +1,175 @@ +#![feature(rustc_private)] + +extern crate rustc_hir; +extern crate rustc_span; + +mod conditional_checker; + +use clippy_utils::higher::If; +use clippy_wrappers::span_lint; +use conditional_checker::{is_panic_inducing_call, ConditionalChecker}; +use rustc_hir::{ + def::Res::Local, + intravisit::{walk_expr, FnKind, Visitor}, + Body, Expr, ExprKind, FnDecl, HirId, Path, QPath, +}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_span::{ + def_id::{DefId, LocalDefId}, + Span, +}; +use std::{ + collections::{HashMap, HashSet}, + vec, +}; +use utils::FunctionCallVisitor; + +const LINT_MESSAGE: &str = + "The transferred amount should be checked against a minimum to prevent front-running"; + +dylint_linting::impl_late_lint! { + pub FRONT_RUNNING, + Warn, + LINT_MESSAGE, + FrontRunning::default(), + { + name: "Front Running message", + long_message: "Front Running message", + severity: "", + help: "", + vulnerability_class: "", + } +} + +#[derive(Default)] +struct FrontRunning { + function_call_graph: HashMap>, + checked_functions: HashSet, +} + +impl<'tcx> LateLintPass<'tcx> for FrontRunning { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + _: FnKind<'tcx>, + _fn_decl: &'tcx FnDecl<'tcx>, + body: &'tcx Body<'tcx>, + span: Span, + local_def_id: LocalDefId, + ) { + let def_id = local_def_id.to_def_id(); + self.checked_functions.insert(cx.tcx.def_path_str(def_id)); + + if span.from_expansion() { + return; + } + + let mut function_call_visitor = + FunctionCallVisitor::new(cx, def_id, &mut self.function_call_graph); + function_call_visitor.visit_body(body); + + let mut front_running_visitor = FrontRunningVisitor { + cx, + local_variables: Vec::new(), + transfer_amount_id: Vec::new(), + conditional_checker: Vec::new(), + checked_hir_ids: Vec::new(), + }; + front_running_visitor.visit_body(body); + + for (transfer_amount_id, span) in front_running_visitor.transfer_amount_id.iter() { + if !front_running_visitor + .checked_hir_ids + .contains(transfer_amount_id) + && front_running_visitor + .local_variables + .contains(transfer_amount_id) + { + span_lint(cx, FRONT_RUNNING, *span, LINT_MESSAGE); + } + } + } +} + +struct FrontRunningVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + local_variables: Vec, + transfer_amount_id: Vec<(HirId, Span)>, + conditional_checker: Vec, + checked_hir_ids: Vec, +} + +impl<'a, 'tcx> Visitor<'tcx> for FrontRunningVisitor<'a, 'tcx> { + fn visit_local(&mut self, local: &'tcx rustc_hir::LetStmt<'tcx>) { + self.local_variables.push(local.pat.hir_id); + } + + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + // Check if the expression is a transfer method call, then store the HirId of the amount parameter + if let ExprKind::MethodCall(path_segment, _receiver, methodargs, ..) = expr.kind { + if path_segment.ident.name.to_string() == "transfer" + && self + .cx + .typeck_results() + .node_type(_receiver.hir_id) + .to_string() + == "soroban_sdk::token::TokenClient<'_>" + { + let amount_param = methodargs[2]; + if let ExprKind::AddrOf(_, _, new_exp, ..) = amount_param.kind { + if let ExprKind::Path(QPath::Resolved(_, Path { segments, .. }), ..) = + new_exp.kind + { + let transfer_amount_param = segments.first(); + if transfer_amount_param.is_some() { + if let Local(id) = transfer_amount_param.unwrap().res { + self.transfer_amount_id.push((id, expr.span)); + } + } + } + } + } + } + + // If we are inside an 'if' statement, check if the current expression is a return or a panic inducing call + if !self.conditional_checker.is_empty() { + let last_conditional_checker = self.conditional_checker.iter().last().unwrap(); + match &expr.kind { + ExprKind::Ret(..) => { + let hir_id = last_conditional_checker.greater_expr; + if let Some(hir_id) = hir_id { + self.checked_hir_ids.push(hir_id); + } + } + ExprKind::Call(func, _) if is_panic_inducing_call(func) => { + let hir_id = last_conditional_checker.greater_expr; + if let Some(hir_id) = hir_id { + self.checked_hir_ids.push(hir_id); + } + } + _ => {} + } + } + + // Check if the expression has an 'if' and if it does, check if it meets our condition + if let Some(If { + cond, + then: if_expr, + r#else: _, + }) = If::hir(expr) + { + let mut conditional_checker = ConditionalChecker { + greater_expr: None, + lesser_expr: None, + }; + if conditional_checker.handle_condition(cond) { + self.conditional_checker.push(conditional_checker); + walk_expr(self, if_expr); + } + self.conditional_checker.pop(); + return; + } + + walk_expr(self, expr); + } +} From 6e94600f0c148e0b5f4fda2702a745d6ab285850 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Thu, 5 Sep 2024 14:46:49 -0300 Subject: [PATCH 3/4] Update detector --- .../front-running/src/conditional_checker.rs | 32 +++---- detectors/front-running/src/lib.rs | 87 ++++++++++--------- .../remediated-example/Cargo.toml | 2 +- .../vulnerable-example/Cargo.toml | 2 +- 4 files changed, 64 insertions(+), 59 deletions(-) diff --git a/detectors/front-running/src/conditional_checker.rs b/detectors/front-running/src/conditional_checker.rs index 5b07747d..b6a56ab7 100644 --- a/detectors/front-running/src/conditional_checker.rs +++ b/detectors/front-running/src/conditional_checker.rs @@ -9,28 +9,16 @@ pub struct ConditionalChecker { } impl ConditionalChecker { - fn get_res_hir_id(&self, expr: &Expr) -> Option { - if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind { - if let Res::Local(hir_id) = path.res { - Some(hir_id) - } else { - None - } - } else { - None - } - } - fn handle_binary_op(&mut self, op: BinOpKind, left: &Expr, right: &Expr) -> bool { match op { BinOpKind::Ge | BinOpKind::Gt => { - self.greater_expr = self.get_res_hir_id(left); - self.lesser_expr = self.get_res_hir_id(right); + self.greater_expr = get_res_hir_id(left); + self.lesser_expr = get_res_hir_id(right); true } BinOpKind::Le | BinOpKind::Lt => { - self.lesser_expr = self.get_res_hir_id(left); - self.greater_expr = self.get_res_hir_id(right); + self.lesser_expr = get_res_hir_id(left); + self.greater_expr = get_res_hir_id(right); true } _ => false, @@ -52,6 +40,18 @@ impl ConditionalChecker { } } +pub fn get_res_hir_id(expr: &Expr) -> Option { + if let ExprKind::Path(QPath::Resolved(_, path)) = &expr.kind { + if let Res::Local(hir_id) = path.res { + Some(hir_id) + } else { + None + } + } else { + None + } +} + pub fn is_panic_inducing_call(func: &Expr<'_>) -> bool { if let ExprKind::Path(QPath::Resolved(_, path)) = &func.kind { return PANIC_INDUCING_FUNCTIONS.iter().any(|&func| { diff --git a/detectors/front-running/src/lib.rs b/detectors/front-running/src/lib.rs index 219db235..c458b863 100644 --- a/detectors/front-running/src/lib.rs +++ b/detectors/front-running/src/lib.rs @@ -8,21 +8,22 @@ mod conditional_checker; use clippy_utils::higher::If; use clippy_wrappers::span_lint; use conditional_checker::{is_panic_inducing_call, ConditionalChecker}; +use if_chain::if_chain; use rustc_hir::{ - def::Res::Local, + def::Res::{self}, intravisit::{walk_expr, FnKind, Visitor}, - Body, Expr, ExprKind, FnDecl, HirId, Path, QPath, + Body, Expr, ExprKind, FnDecl, HirId, LetStmt, Path, QPath, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_span::{ def_id::{DefId, LocalDefId}, - Span, + Span, Symbol, }; use std::{ collections::{HashMap, HashSet}, vec, }; -use utils::FunctionCallVisitor; +use utils::{get_node_type_opt, FunctionCallVisitor}; const LINT_MESSAGE: &str = "The transferred amount should be checked against a minimum to prevent front-running"; @@ -33,10 +34,10 @@ dylint_linting::impl_late_lint! { LINT_MESSAGE, FrontRunning::default(), { - name: "Front Running message", - long_message: "Front Running message", - severity: "", - help: "", + name: "Front Running Detection", + long_message: "This lint checks for potential front-running vulnerabilities in token transfers", + severity: "Warning", + help: "Consider implementing a minimum amount check before the transfer", vulnerability_class: "", } } @@ -52,7 +53,7 @@ impl<'tcx> LateLintPass<'tcx> for FrontRunning { &mut self, cx: &LateContext<'tcx>, _: FnKind<'tcx>, - _fn_decl: &'tcx FnDecl<'tcx>, + _: &'tcx FnDecl<'tcx>, body: &'tcx Body<'tcx>, span: Span, local_def_id: LocalDefId, @@ -68,9 +69,16 @@ impl<'tcx> LateLintPass<'tcx> for FrontRunning { FunctionCallVisitor::new(cx, def_id, &mut self.function_call_graph); function_call_visitor.visit_body(body); + let function_params = body + .params + .iter() + .map(|param| param.pat.hir_id) + .collect::>(); + let mut front_running_visitor = FrontRunningVisitor { cx, local_variables: Vec::new(), + function_params, transfer_amount_id: Vec::new(), conditional_checker: Vec::new(), checked_hir_ids: Vec::new(), @@ -94,58 +102,55 @@ impl<'tcx> LateLintPass<'tcx> for FrontRunning { struct FrontRunningVisitor<'a, 'tcx> { cx: &'a LateContext<'tcx>, local_variables: Vec, + function_params: Vec, transfer_amount_id: Vec<(HirId, Span)>, conditional_checker: Vec, checked_hir_ids: Vec, } +impl FrontRunningVisitor<'_, '_> { + fn add_to_checked_hir_ids(&mut self, last_conditional_checker: ConditionalChecker) { + if let (Some(lesser_hir_id), Some(greater_hir_id)) = ( + last_conditional_checker.lesser_expr, + last_conditional_checker.greater_expr, + ) { + if self.function_params.contains(&lesser_hir_id) { + self.checked_hir_ids.push(greater_hir_id); + } + } + } +} + impl<'a, 'tcx> Visitor<'tcx> for FrontRunningVisitor<'a, 'tcx> { - fn visit_local(&mut self, local: &'tcx rustc_hir::LetStmt<'tcx>) { + fn visit_local(&mut self, local: &'tcx LetStmt<'tcx>) { self.local_variables.push(local.pat.hir_id); } fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { // Check if the expression is a transfer method call, then store the HirId of the amount parameter - if let ExprKind::MethodCall(path_segment, _receiver, methodargs, ..) = expr.kind { - if path_segment.ident.name.to_string() == "transfer" - && self - .cx - .typeck_results() - .node_type(_receiver.hir_id) - .to_string() - == "soroban_sdk::token::TokenClient<'_>" - { - let amount_param = methodargs[2]; - if let ExprKind::AddrOf(_, _, new_exp, ..) = amount_param.kind { - if let ExprKind::Path(QPath::Resolved(_, Path { segments, .. }), ..) = - new_exp.kind - { - let transfer_amount_param = segments.first(); - if transfer_amount_param.is_some() { - if let Local(id) = transfer_amount_param.unwrap().res { - self.transfer_amount_id.push((id, expr.span)); - } - } - } - } + if_chain! { + if let ExprKind::MethodCall(path_segment, receiver, args, ..) = expr.kind; + if path_segment.ident.name == Symbol::intern("transfer"); + if let Some(receiver_type) = get_node_type_opt(self.cx, &receiver.hir_id); + if receiver_type.to_string() == "soroban_sdk::token::TokenClient<'_>"; + if let ExprKind::AddrOf(_, _, amount_expr, ..) = args[2].kind; + if let ExprKind::Path(QPath::Resolved(_, Path { segments, .. }), ..) = amount_expr.kind; + if let Some(segment) = segments.first(); + if let Res::Local(hir_id) = segment.res; + then { + self.transfer_amount_id.push((hir_id, expr.span)); } } // If we are inside an 'if' statement, check if the current expression is a return or a panic inducing call if !self.conditional_checker.is_empty() { - let last_conditional_checker = self.conditional_checker.iter().last().unwrap(); + let last_conditional_checker = *self.conditional_checker.last().unwrap(); match &expr.kind { ExprKind::Ret(..) => { - let hir_id = last_conditional_checker.greater_expr; - if let Some(hir_id) = hir_id { - self.checked_hir_ids.push(hir_id); - } + self.add_to_checked_hir_ids(last_conditional_checker); } ExprKind::Call(func, _) if is_panic_inducing_call(func) => { - let hir_id = last_conditional_checker.greater_expr; - if let Some(hir_id) = hir_id { - self.checked_hir_ids.push(hir_id); - } + self.add_to_checked_hir_ids(last_conditional_checker); } _ => {} } diff --git a/test-cases/front-running/front-running-1/remediated-example/Cargo.toml b/test-cases/front-running/front-running-1/remediated-example/Cargo.toml index 55f2be0b..a9947e6b 100644 --- a/test-cases/front-running/front-running-1/remediated-example/Cargo.toml +++ b/test-cases/front-running/front-running-1/remediated-example/Cargo.toml @@ -10,7 +10,7 @@ crate-type = ["cdylib"] soroban-sdk = { version = "20.0.0" } soroban-token-sdk = { version = "20.0.0" } -[dev_dependencies] +[dev-dependencies] soroban-sdk = { version = "20.0.0", features = ["testutils"] } [features] diff --git a/test-cases/front-running/front-running-1/vulnerable-example/Cargo.toml b/test-cases/front-running/front-running-1/vulnerable-example/Cargo.toml index 4d7ccac0..77555903 100644 --- a/test-cases/front-running/front-running-1/vulnerable-example/Cargo.toml +++ b/test-cases/front-running/front-running-1/vulnerable-example/Cargo.toml @@ -10,7 +10,7 @@ crate-type = ["cdylib"] soroban-sdk = { version = "20.0.0" } soroban-token-sdk = { version = "20.0.0" } -[dev_dependencies] +[dev-dependencies] soroban-sdk = { version = "20.0.0", features = ["testutils"] } [features] From 46a8e426f3ab2834e2a069b663df110e57fabdf8 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Thu, 5 Sep 2024 15:29:44 -0300 Subject: [PATCH 4/4] Update detector --- detectors/front-running/src/lib.rs | 59 +++++++++++++++++++----------- 1 file changed, 37 insertions(+), 22 deletions(-) diff --git a/detectors/front-running/src/lib.rs b/detectors/front-running/src/lib.rs index c458b863..20a6d267 100644 --- a/detectors/front-running/src/lib.rs +++ b/detectors/front-running/src/lib.rs @@ -7,7 +7,7 @@ mod conditional_checker; use clippy_utils::higher::If; use clippy_wrappers::span_lint; -use conditional_checker::{is_panic_inducing_call, ConditionalChecker}; +use conditional_checker::{get_res_hir_id, is_panic_inducing_call, ConditionalChecker}; use if_chain::if_chain; use rustc_hir::{ def::Res::{self}, @@ -38,7 +38,7 @@ dylint_linting::impl_late_lint! { long_message: "This lint checks for potential front-running vulnerabilities in token transfers", severity: "Warning", help: "Consider implementing a minimum amount check before the transfer", - vulnerability_class: "", + vulnerability_class: "MEV", } } @@ -69,28 +69,25 @@ impl<'tcx> LateLintPass<'tcx> for FrontRunning { FunctionCallVisitor::new(cx, def_id, &mut self.function_call_graph); function_call_visitor.visit_body(body); - let function_params = body - .params - .iter() - .map(|param| param.pat.hir_id) - .collect::>(); + let function_params: HashSet<_> = + body.params.iter().map(|param| param.pat.hir_id).collect(); let mut front_running_visitor = FrontRunningVisitor { + checked_hir_ids: HashSet::new(), + conditional_checker: Vec::new(), cx, - local_variables: Vec::new(), + filtered_local_variables: HashSet::new(), function_params, transfer_amount_id: Vec::new(), - conditional_checker: Vec::new(), - checked_hir_ids: Vec::new(), }; front_running_visitor.visit_body(body); - for (transfer_amount_id, span) in front_running_visitor.transfer_amount_id.iter() { + for (transfer_amount_id, span) in &front_running_visitor.transfer_amount_id { if !front_running_visitor .checked_hir_ids .contains(transfer_amount_id) && front_running_visitor - .local_variables + .filtered_local_variables .contains(transfer_amount_id) { span_lint(cx, FRONT_RUNNING, *span, LINT_MESSAGE); @@ -100,12 +97,12 @@ impl<'tcx> LateLintPass<'tcx> for FrontRunning { } struct FrontRunningVisitor<'a, 'tcx> { + checked_hir_ids: HashSet, + conditional_checker: Vec, cx: &'a LateContext<'tcx>, - local_variables: Vec, - function_params: Vec, + filtered_local_variables: HashSet, + function_params: HashSet, transfer_amount_id: Vec<(HirId, Span)>, - conditional_checker: Vec, - checked_hir_ids: Vec, } impl FrontRunningVisitor<'_, '_> { @@ -115,15 +112,34 @@ impl FrontRunningVisitor<'_, '_> { last_conditional_checker.greater_expr, ) { if self.function_params.contains(&lesser_hir_id) { - self.checked_hir_ids.push(greater_hir_id); + self.checked_hir_ids.insert(greater_hir_id); + } + } + } + + fn local_uses_parameter(&self, expr: &Expr) -> bool { + match &expr.kind { + ExprKind::MethodCall(_, _, args, _) | ExprKind::Call(_, args) => { + args.iter().any(|arg| { + get_res_hir_id(arg) + .map_or(false, |hir_id| self.function_params.contains(&hir_id)) + }) + } + ExprKind::Binary(_, left_expr, right_expr) => { + self.local_uses_parameter(left_expr) || self.local_uses_parameter(right_expr) } + _ => false, } } } impl<'a, 'tcx> Visitor<'tcx> for FrontRunningVisitor<'a, 'tcx> { fn visit_local(&mut self, local: &'tcx LetStmt<'tcx>) { - self.local_variables.push(local.pat.hir_id); + if let Some(init) = &local.init { + if self.local_uses_parameter(init) { + self.filtered_local_variables.insert(local.pat.hir_id); + } + } } fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { @@ -143,8 +159,7 @@ impl<'a, 'tcx> Visitor<'tcx> for FrontRunningVisitor<'a, 'tcx> { } // If we are inside an 'if' statement, check if the current expression is a return or a panic inducing call - if !self.conditional_checker.is_empty() { - let last_conditional_checker = *self.conditional_checker.last().unwrap(); + if let Some(last_conditional_checker) = self.conditional_checker.last().copied() { match &expr.kind { ExprKind::Ret(..) => { self.add_to_checked_hir_ids(last_conditional_checker); @@ -170,9 +185,9 @@ impl<'a, 'tcx> Visitor<'tcx> for FrontRunningVisitor<'a, 'tcx> { if conditional_checker.handle_condition(cond) { self.conditional_checker.push(conditional_checker); walk_expr(self, if_expr); + self.conditional_checker.pop(); + return; } - self.conditional_checker.pop(); - return; } walk_expr(self, expr);