From 52baccc5b73ebbd51cf25ed19581cda2b76a043e Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Wed, 10 Apr 2024 10:57:36 -0300 Subject: [PATCH 01/16] dos unexpected revert with vector --- .../Cargo.toml | 20 + .../src/lib.rs | 387 ++++++++++++++++++ .../remediated-example/Cargo.toml | 37 ++ .../remediated-example/lib.rs | 157 +++++++ .../vulnerable-example/Cargo.toml | 37 ++ .../vulnerable-example/lib.rs | 157 +++++++ 6 files changed, 795 insertions(+) create mode 100644 detectors/dos-unexpected-revert-with-vector/Cargo.toml create mode 100644 detectors/dos-unexpected-revert-with-vector/src/lib.rs create mode 100644 test-cases/dos-unexpected-revert-with-vector/remediated-example/Cargo.toml create mode 100644 test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs create mode 100644 test-cases/dos-unexpected-revert-with-vector/vulnerable-example/Cargo.toml create mode 100644 test-cases/dos-unexpected-revert-with-vector/vulnerable-example/lib.rs diff --git a/detectors/dos-unexpected-revert-with-vector/Cargo.toml b/detectors/dos-unexpected-revert-with-vector/Cargo.toml new file mode 100644 index 00000000..fd462f96 --- /dev/null +++ b/detectors/dos-unexpected-revert-with-vector/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "dos-unexpected-revert-with-vector" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +scout-audit-clippy-utils = { workspace = true } +dylint_linting = { workspace = true } +if_chain = { workspace = true } + +scout-audit-internal = { workspace = true } + +[dev-dependencies] +dylint_testing = { workspace = true } + +[package.metadata.rust-analyzer] +rustc_private = true \ No newline at end of file diff --git a/detectors/dos-unexpected-revert-with-vector/src/lib.rs b/detectors/dos-unexpected-revert-with-vector/src/lib.rs new file mode 100644 index 00000000..4592f1da --- /dev/null +++ b/detectors/dos-unexpected-revert-with-vector/src/lib.rs @@ -0,0 +1,387 @@ +#![feature(rustc_private)] +#![warn(unused_extern_crates)] +#![feature(let_chains)] + +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use std::collections::HashSet; + +use rustc_hir::intravisit::walk_expr; +use rustc_hir::intravisit::Visitor; +use rustc_hir::QPath; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::mir::Const; +use rustc_middle::mir::{ + BasicBlock, BasicBlockData, BasicBlocks, Operand, Place, StatementKind, TerminatorKind, +}; +use rustc_middle::ty::{Ty, TyKind}; +use rustc_span::def_id::DefId; +use rustc_span::Span; +use scout_audit_internal::{DetectorImpl, InkDetector as Detector}; + +dylint_linting::impl_late_lint! { + /// ### What it does + /// Checks for array pushes without access control. + /// ### Why is this bad? + /// Arrays have a maximum size according to the storage cell. If the array is full, the push will revert. This can be used to prevent the execution of a function. + /// ### Known problems + /// If the owner validation is performed in an auxiliary function, the warning will be shown, resulting in a false positive. + /// ### Example + /// ```rust + /// if self.votes.contains(candidate) { + /// Err(Errors::CandidateAlreadyAdded) + /// } else { + /// self.candidates.push(candidate); + /// self.votes.insert(candidate, 0); + /// Ok(()) + /// } + /// ``` + /// Use instead: + /// ```rust + /// if self.votes.contains(candidate) { + /// Err(Errors::CandidateAlreadyAdded) + /// } else { + /// self.candidates.set(self.total_candidates, candidate); + /// self.total_candidates += 1; + /// self.votes.set(candidate, 0); + /// Ok(()) + /// } + /// ``` + pub UNEXPECTED_REVERT_WARN, + Warn, + "", + UnexpectedRevertWarn::default() +} + +#[derive(Default)] +pub struct UnexpectedRevertWarn {} +impl UnexpectedRevertWarn { + pub fn new() -> Self { + Self {} + } +} + +impl<'tcx> LateLintPass<'tcx> for UnexpectedRevertWarn { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + _: rustc_hir::intravisit::FnKind<'tcx>, + _: &'tcx rustc_hir::FnDecl<'tcx>, + body: &'tcx rustc_hir::Body<'tcx>, + _: Span, + localdef: rustc_span::def_id::LocalDefId, + ) { + struct UnprotectedVectorFinder<'tcx, 'tcx_ref> { + cx: &'tcx_ref LateContext<'tcx>, + callers_def_id: HashSet, + push_def_id: Option, + } + impl<'tcx> Visitor<'tcx> for UnprotectedVectorFinder<'tcx, '_> { + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + if let ExprKind::MethodCall(path, receiver, ..) = expr.kind { + let defid = self.cx.typeck_results().type_dependent_def_id(expr.hir_id); + let ty = Ty::new_foreign(self.cx.tcx, defid.unwrap()); + if ty.to_string().contains("soroban_sdk::Vec") { + if path.ident.name.to_string() == "push_back" || path.ident.name.to_string() == "push_front" { + self.push_def_id = defid; + } + } else if let ExprKind::MethodCall(rec_path, receiver2, ..) = receiver.kind + && rec_path.ident.name.to_string() == "env" + && let ExprKind::Path(rec2_qpath) = &receiver2.kind + && let QPath::Resolved(qualifier, rec2_path) = rec2_qpath + && rec2_path.segments.first().map_or(false, |seg| { + seg.ident.to_string() == "self" && qualifier.is_none() + }) + && path.ident.name.to_string() == "caller" + { + if self + .cx + .typeck_results() + .type_dependent_def_id(expr.hir_id) + .is_some() + { + self.callers_def_id.insert( + self.cx + .typeck_results() + .type_dependent_def_id(expr.hir_id) + .unwrap(), + ); + } + } else if let ExprKind::Call(receiver2, ..) = receiver.kind + && let ExprKind::Path(rec2_qpath) = &receiver2.kind + && let QPath::TypeRelative(ty2, rec2_path) = rec2_qpath + && rec2_path.ident.name.to_string() == "env" + && let rustc_hir::TyKind::Path(rec3_qpath) = &ty2.kind + && let QPath::Resolved(_, rec3_path) = rec3_qpath + && rec3_path.segments[0].ident.to_string() == "Self" + && self + .cx + .typeck_results() + .type_dependent_def_id(expr.hir_id) + .is_some() + { + self.callers_def_id.insert( + self.cx + .typeck_results() + .type_dependent_def_id(expr.hir_id) + .unwrap(), + ); + } + } + walk_expr(self, expr); + } + } + + let mut uvf_storage = UnprotectedVectorFinder { + cx, + callers_def_id: HashSet::default(), + push_def_id: None, + }; + + walk_expr(&mut uvf_storage, body.value); + + let mir_body = cx.tcx.optimized_mir(localdef); + + struct CallersAndVecOps<'tcx> { + callers: Vec<(&'tcx BasicBlockData<'tcx>, BasicBlock)>, + vec_ops: Vec<(&'tcx BasicBlockData<'tcx>, BasicBlock)>, + } + + fn find_caller_and_vec_ops_in_mir<'tcx>( + bbs: &'tcx BasicBlocks<'tcx>, + callers_def_id: HashSet, + push_def_id: Option, + ) -> CallersAndVecOps { + let mut callers_vec = CallersAndVecOps { + callers: vec![], + vec_ops: vec![], + }; + for (bb, bb_data) in bbs.iter().enumerate() { + if bb_data.terminator.as_ref().is_none() { + continue; + } + let terminator = bb_data.terminator.clone().unwrap(); + if let TerminatorKind::Call { func, .. } = terminator.kind { + if let Operand::Constant(fn_const) = func + && let Const::Val(_const_val, ty) = fn_const.const_ + && let TyKind::FnDef(def, _subs) = ty.kind() + { + if !callers_def_id.is_empty() { + for caller in &callers_def_id { + if caller == def { + callers_vec + .callers + .push((bb_data, BasicBlock::from_usize(bb))); + } + } + } else { + for op in &push_def_id { + if op == def { + callers_vec + .vec_ops + .push((bb_data, BasicBlock::from_usize(bb))); + } + } + } + } + } + } + callers_vec + } + + let caller_and_vec_ops = find_caller_and_vec_ops_in_mir( + &mir_body.basic_blocks, + uvf_storage.callers_def_id, + uvf_storage.push_def_id, + ); + + if !caller_and_vec_ops.vec_ops.is_empty() { + let unchecked_places = navigate_trough_basicblocks( + &mir_body.basic_blocks, + BasicBlock::from_u32(0), + &caller_and_vec_ops, + false, + &mut vec![], + &mut HashSet::::default(), + ); + for place in unchecked_places { + Detector::DosUnexpectedRevertWithVector.span_lint( + cx, + UNEXPECTED_REVERT_WARN, + place.1, + ); + } + } + + fn navigate_trough_basicblocks<'tcx>( + bbs: &'tcx BasicBlocks<'tcx>, + bb: BasicBlock, + caller_and_vec_ops: &CallersAndVecOps<'tcx>, + after_comparison: bool, + tainted_places: &mut Vec>, + visited_bbs: &mut HashSet, + ) -> Vec<(Place<'tcx>, Span)> { + let mut ret_vec = Vec::<(Place, Span)>::new(); + if visited_bbs.contains(&bb) { + return ret_vec; + } else { + visited_bbs.insert(bb); + } + if bbs[bb].terminator.is_none() { + return ret_vec; + } + for statement in &bbs[bb].statements { + if let StatementKind::Assign(assign) = &statement.kind { + match &assign.1 { + rustc_middle::mir::Rvalue::Ref(_, _, origplace) + | rustc_middle::mir::Rvalue::AddressOf(_, origplace) + | rustc_middle::mir::Rvalue::Len(origplace) + | rustc_middle::mir::Rvalue::CopyForDeref(origplace) => { + if tainted_places + .clone() + .into_iter() + .any(|place| place == *origplace) + { + tainted_places.push(assign.0); + } + } + rustc_middle::mir::Rvalue::Use(operand) => match &operand { + Operand::Copy(origplace) | Operand::Move(origplace) => { + if tainted_places + .clone() + .into_iter() + .any(|place| place == *origplace) + { + tainted_places.push(assign.0); + } + } + _ => {} + }, + _ => {} + } + } + } + match &bbs[bb].terminator().kind { + TerminatorKind::SwitchInt { discr, targets } => { + let comparison_with_caller = match discr { + Operand::Copy(place) | Operand::Move(place) => { + tainted_places + .iter() + .any(|tainted_place| tainted_place == place) + || after_comparison + } + Operand::Constant(_cons) => after_comparison, + }; + for target in targets.all_targets() { + ret_vec.append(&mut navigate_trough_basicblocks( + bbs, + *target, + caller_and_vec_ops, + comparison_with_caller, + tainted_places, + visited_bbs, + )); + } + return ret_vec; + } + TerminatorKind::Call { + destination, + args, + target, + fn_span, + .. + } => { + for arg in args { + match arg { + Operand::Copy(origplace) | Operand::Move(origplace) => { + if tainted_places + .clone() + .into_iter() + .any(|place| place == *origplace) + { + tainted_places.push(*destination); + } + } + Operand::Constant(_) => {} + } + } + for caller in &caller_and_vec_ops.callers { + if caller.1 == bb { + tainted_places.push(*destination); + } + } + for map_op in &caller_and_vec_ops.vec_ops { + if map_op.1 == bb + && !after_comparison + && args.get(1).map_or(true, |f| { + f.place().is_some_and(|f| !tainted_places.contains(&f)) + }) + { + ret_vec.push((*destination, *fn_span)) + } + } + if target.is_some() { + ret_vec.append(&mut navigate_trough_basicblocks( + bbs, + target.unwrap(), + caller_and_vec_ops, + after_comparison, + tainted_places, + visited_bbs, + )); + } + } + TerminatorKind::Assert { target, .. } + | TerminatorKind::Goto { target, .. } + | TerminatorKind::Drop { target, .. } => { + ret_vec.append(&mut navigate_trough_basicblocks( + bbs, + *target, + caller_and_vec_ops, + after_comparison, + tainted_places, + visited_bbs, + )); + } + TerminatorKind::Yield { resume, .. } => { + ret_vec.append(&mut navigate_trough_basicblocks( + bbs, + *resume, + caller_and_vec_ops, + after_comparison, + tainted_places, + visited_bbs, + )); + } + TerminatorKind::FalseEdge { real_target, .. } + | TerminatorKind::FalseUnwind { real_target, .. } => { + ret_vec.append(&mut navigate_trough_basicblocks( + bbs, + *real_target, + caller_and_vec_ops, + after_comparison, + tainted_places, + visited_bbs, + )); + } + TerminatorKind::InlineAsm { destination, .. } => { + if destination.is_some() { + ret_vec.append(&mut navigate_trough_basicblocks( + bbs, + destination.unwrap(), + caller_and_vec_ops, + after_comparison, + tainted_places, + visited_bbs, + )); + } + } + _ => {} + } + ret_vec + } + } +} \ No newline at end of file diff --git a/test-cases/dos-unexpected-revert-with-vector/remediated-example/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/remediated-example/Cargo.toml new file mode 100644 index 00000000..577569e2 --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/remediated-example/Cargo.toml @@ -0,0 +1,37 @@ +[package] +name = "unexpected_revert" +version = "0.1.0" +edition = "2021" + + +[lib] +crate-type = ["cdylib"] +path = "lib.rs" + +[dependencies] +soroban-sdk = "=20.0.0" + +[dev_dependencies] +soroban-sdk = { version = "=20.0.0", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] + +[profile.release] +opt-level = "z" +overflow-checks = true +debug = 0 +strip = "symbols" +debug-assertions = false +panic = "abort" +codegen-units = 1 +lto = true + +[profile.release-with-logs] +inherits = "release" +debug-assertions = true + +[workspace.metadata.dylint] +libraries = [ + { path="/home/sofiazcoaga/scout-soroban/detectors/dos-unexpected-revert-with-vector" }, +] \ No newline at end of file diff --git a/test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs b/test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs new file mode 100644 index 00000000..0e69680d --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs @@ -0,0 +1,157 @@ +#![no_std] + +use soroban_sdk::{ String, contracterror, contract, contracttype, contractimpl, Symbol, symbol_short, Env, Address, Map}; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum URError { // Unexpected Revert Error + AccountAlreadyVoted = 1, + CandidateAlreadyAdded = 2, + CandidateDoesntExist = 3, + Overflow = 4, + TimestampBeforeCurrentBlock = 5, + VoteEnded = 6 +} + +#[derive(Debug, Clone, PartialEq)] +#[contracttype] +pub struct State { + total_votes: u64, + total_candidates: u32, + candidates: Map, + votes: Map, + already_voted: Map, + most_voted_candidate: Address, + candidate_votes: u64, + vote_timestamp_end: u64, +} + +const STATE: Symbol = symbol_short!("STATE"); + +#[contract] +pub struct UnexpectedRevert; + +#[contractimpl] +impl UnexpectedRevert { + + pub fn init(env: Env, end_timestamp: u64) -> Result { + if end_timestamp <= env.ledger().timestamp() { + return Err(URError::TimestampBeforeCurrentBlock); + } + + let zero_string: String = String::from_str(&env, "00000000000000000000000000000000"); + let zero_addr = Address::from_string(&zero_string); //CHECK + let state = State { + total_votes:0, + total_candidates: 0, + most_voted_candidate: zero_addr, + candidate_votes: 0, + candidates: Map::new(&env), + already_voted: Map::new(&env), + votes: Map::new(&env), + vote_timestamp_end: end_timestamp, + }; + + env.storage().instance().set(&STATE, &state); + Ok(state) + + } + + pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + caller.require_auth(); + let mut state = Self::get_state(env.clone()); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + if state.already_voted.contains_key(caller.clone()) { + return Err(URError::AccountAlreadyVoted); + } else { + state.candidates.set(state.total_candidates, candidate.clone()); + state.total_candidates += 1; + state.votes.set(candidate, 0); + Ok(()) + } + + + + } + + pub fn get_votes_for_a_candidate(env: Env, candidate: Address) -> Result { + let state = Self::get_state(env.clone()); + state.votes + .get(candidate) + .ok_or(URError::CandidateDoesntExist) + } + + pub fn most_voted_candidate_votes(env: Env) -> u64 { + let state = Self::get_state(env); + state.candidate_votes + } + + pub fn most_voted_candidate(env: Env) -> Address { + let state = Self::get_state(env); + state.most_voted_candidate + } + + pub fn get_total_votes(env: Env) -> u64 { + let state = Self::get_state(env); + state.total_votes + } + + pub fn get_total_candidates(env: Env) -> u32 { + let state = Self::get_state(env); + state.total_candidates + } + + pub fn get_candidate(env: Env, index: u32) -> Result { + let state = Self::get_state(env); + match state.candidates.get(index) { + Some(candidate) => Ok(candidate), + None => Err(URError::CandidateDoesntExist), + } + } + + pub fn account_has_voted(env: Env, account: Address) -> bool { + let state = Self::get_state(env); + state.already_voted.get(account).unwrap_or(false) + } + + pub fn vote(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + caller.require_auth(); + let mut state = Self::get_state(env.clone()); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + + if state.already_voted.contains_key(caller.clone()) { + Err(URError::AccountAlreadyVoted) + } else { + state.already_voted.set(caller, true); + let votes = state + .votes + .get(candidate.clone()) + .ok_or(URError::CandidateDoesntExist)? + .checked_add(1) + .ok_or(URError::Overflow)?; + state.votes.set(candidate.clone(), votes); + state.total_votes.checked_add(1).ok_or(URError::Overflow)?; + if state.candidate_votes < votes { + state.candidate_votes = votes; + state.most_voted_candidate = candidate; + } + Ok(()) + } + } + + pub fn vote_ended(env: Env) -> bool { + let state = Self::get_state(env.clone()); + state.vote_timestamp_end <= env.ledger().timestamp() + } + + pub fn get_state(env: Env) -> State { + env.storage().instance().get(&STATE).unwrap() + } + + +} \ No newline at end of file diff --git a/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..577569e2 --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/Cargo.toml @@ -0,0 +1,37 @@ +[package] +name = "unexpected_revert" +version = "0.1.0" +edition = "2021" + + +[lib] +crate-type = ["cdylib"] +path = "lib.rs" + +[dependencies] +soroban-sdk = "=20.0.0" + +[dev_dependencies] +soroban-sdk = { version = "=20.0.0", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] + +[profile.release] +opt-level = "z" +overflow-checks = true +debug = 0 +strip = "symbols" +debug-assertions = false +panic = "abort" +codegen-units = 1 +lto = true + +[profile.release-with-logs] +inherits = "release" +debug-assertions = true + +[workspace.metadata.dylint] +libraries = [ + { path="/home/sofiazcoaga/scout-soroban/detectors/dos-unexpected-revert-with-vector" }, +] \ No newline at end of file diff --git a/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/lib.rs b/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/lib.rs new file mode 100644 index 00000000..d3f6b196 --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/lib.rs @@ -0,0 +1,157 @@ +#![no_std] + +use soroban_sdk::{ String, contracterror, contract, contracttype, contractimpl, Symbol, symbol_short, Env, Address, Map, Vec}; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum URError { // Unexpected Revert Error + AccountAlreadyVoted = 1, + CandidateAlreadyAdded = 2, + CandidateDoesntExist = 3, + Overflow = 4, + TimestampBeforeCurrentBlock = 5, + VoteEnded = 6 +} + +#[derive(Debug, Clone, PartialEq)] +#[contracttype] +pub struct State { + total_votes: u64, + candidates: Vec
, + votes: Map, + already_voted: Map, + most_voted_candidate: Address, + candidate_votes: u64, + vote_timestamp_end: u64, +} + +const STATE: Symbol = symbol_short!("STATE"); + +#[contract] +pub struct UnexpectedRevert; + +#[contractimpl] +impl UnexpectedRevert { + + pub fn init(env: Env, end_timestamp: u64) -> Result { + if end_timestamp <= env.ledger().timestamp() { + return Err(URError::TimestampBeforeCurrentBlock); + } + + let zero_string: String = String::from_str(&env, "00000000000000000000000000000000"); + let zero_addr = Address::from_string(&zero_string); //CHECK + let state = State { + total_votes:0, + most_voted_candidate: zero_addr, + candidate_votes: 0, + candidates: Vec::new(&env), + already_voted: Map::new(&env), + votes: Map::new(&env), + vote_timestamp_end: end_timestamp, + }; + + env.storage().instance().set(&STATE, &state); + Ok(state) + + } + + pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + caller.require_auth(); + let mut state = Self::get_state(env.clone()); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + if state.already_voted.contains_key(caller.clone()) { + return Err(URError::AccountAlreadyVoted); + } else { + state.candidates.push_back(candidate.clone()); + state.votes.set(candidate, 0); + Ok(()) + } + + + + } + + pub fn get_votes_for_a_candidate(env: Env, candidate: Address) -> Result { + let state = Self::get_state(env.clone()); + state.votes + .get(candidate) + .ok_or(URError::CandidateDoesntExist) + } + + pub fn most_voted_candidate_votes(env: Env) -> u64 { + let state = Self::get_state(env); + state.candidate_votes + } + + pub fn most_voted_candidate(env: Env) -> Address { + let state = Self::get_state(env); + state.most_voted_candidate + } + + pub fn get_total_votes(env: Env) -> u64 { + let state = Self::get_state(env); + state.total_votes + } + + pub fn get_total_candidates(env: Env) -> u64 { + let state = Self::get_state(env); + state.candidates.len() as u64 + } + + pub fn get_candidate(env: Env, index: u32) -> Result { + let state = Self::get_state(env); + if index < state.candidates.len() { + Ok(state.candidates.get(index).unwrap()) + } else { + Err(URError::CandidateDoesntExist) + } + } + + pub fn account_has_voted(env: Env, account: Address) -> bool { + let state = Self::get_state(env); + state.already_voted.get(account).unwrap_or(false) + } + + pub fn vote(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + caller.require_auth(); + let mut state = Self::get_state(env.clone()); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + + if state.already_voted.contains_key(caller.clone()) { + Err(URError::AccountAlreadyVoted) + } else { + state.already_voted.set(caller, true); + let votes = state + .votes + .get(candidate.clone()) + .ok_or(URError::CandidateDoesntExist)? + .checked_add(1) + .ok_or(URError::Overflow)?; + state.votes.set(candidate.clone(), votes); + state.total_votes.checked_add(1).ok_or(URError::Overflow)?; + if state.candidate_votes < votes { + state.candidate_votes = votes; + state.most_voted_candidate = candidate; + } + Ok(()) + } + } + + pub fn vote_ended(env: Env) -> bool { + let state = Self::get_state(env.clone()); + state.vote_timestamp_end <= env.ledger().timestamp() + } + + pub fn get_state(env: Env) -> State { + env.storage().instance().get(&STATE).unwrap() + } + + +} + + From e4189c1e95a7abe074c81dc2d5f3aff8d00caa73 Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Mon, 15 Apr 2024 15:31:20 -0300 Subject: [PATCH 02/16] fixing remediated example wip --- .../remediated-example/lib.rs | 87 +++++++++++++++---- 1 file changed, 69 insertions(+), 18 deletions(-) diff --git a/test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs b/test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs index 0e69680d..56c9d4b4 100644 --- a/test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs +++ b/test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs @@ -2,6 +2,7 @@ use soroban_sdk::{ String, contracterror, contract, contracttype, contractimpl, Symbol, symbol_short, Env, Address, Map}; + #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] #[repr(u32)] @@ -18,22 +19,53 @@ pub enum URError { // Unexpected Revert Error #[contracttype] pub struct State { total_votes: u64, - total_candidates: u32, - candidates: Map, - votes: Map, - already_voted: Map, + total_candidates: u32, most_voted_candidate: Address, candidate_votes: u64, vote_timestamp_end: u64, } -const STATE: Symbol = symbol_short!("STATE"); +#[derive(Debug, Clone, PartialEq)] +#[contracttype] +pub struct Candidate { + votes: u64 +} + +#[contracttype] +pub struct AlreadyVoted { + voted: bool +} + +impl Default for AlreadyVoted { + fn default() -> Self { + AlreadyVoted {voted:false} + } +} + +#[contracttype] +pub enum DataKey { + State, + AlreadyVoted(Address), + Candidate(Address) +} #[contract] pub struct UnexpectedRevert; #[contractimpl] impl UnexpectedRevert { + + pub fn set_candidate(env: Env, candidate: Address, id: u32, votes: u64) { + let cand = Candidate { + votes + }; + + env.storage().instance().set(&DataKey::Candidate(candidate), &cand); + } + + pub fn retrieve_candidate(env: Env, candidate: Address) -> Result { + env.storage().instance().get(&DataKey::Candidate(candidate)).unwrap_or(Err(URError::CandidateDoesntExist)) + } pub fn init(env: Env, end_timestamp: u64) -> Result { if end_timestamp <= env.ledger().timestamp() { @@ -41,19 +73,16 @@ impl UnexpectedRevert { } let zero_string: String = String::from_str(&env, "00000000000000000000000000000000"); - let zero_addr = Address::from_string(&zero_string); //CHECK + let zero_addr = Address::from_string(&zero_string); // Whenever this is zero address it will mean no candidate has yet been more voted let state = State { total_votes:0, total_candidates: 0, most_voted_candidate: zero_addr, candidate_votes: 0, - candidates: Map::new(&env), - already_voted: Map::new(&env), - votes: Map::new(&env), vote_timestamp_end: end_timestamp, }; - env.storage().instance().set(&STATE, &state); + env.storage().instance().set(&DataKey::State, &state); Ok(state) } @@ -64,12 +93,12 @@ impl UnexpectedRevert { if Self::vote_ended(env.clone()) { return Err(URError::VoteEnded); } - if state.already_voted.contains_key(caller.clone()) { + if Self::already_voted(env.clone(), caller.clone()) { return Err(URError::AccountAlreadyVoted); } else { - state.candidates.set(state.total_candidates, candidate.clone()); + env.storage().instance().set(&DataKey::Candidate(candidate.clone()), &Candidate{votes: 0}); state.total_candidates += 1; - state.votes.set(candidate, 0); + env.storage().instance().set(&DataKey::State, &state); Ok(()) } @@ -77,11 +106,18 @@ impl UnexpectedRevert { } + pub fn already_voted(env: Env, caller: Address) -> bool { + let already_voted: AlreadyVoted = env.storage().instance().get(&DataKey::AlreadyVoted(caller)).unwrap_or_default(); + already_voted.voted + } + pub fn get_votes_for_a_candidate(env: Env, candidate: Address) -> Result { - let state = Self::get_state(env.clone()); - state.votes - .get(candidate) - .ok_or(URError::CandidateDoesntExist) + let result: Option = env.storage().instance().get(&DataKey::Candidate(candidate)); + match result { + Some(cand) => Ok(cand.votes), + None => Err(URError::CandidateDoesntExist) + } + } pub fn most_voted_candidate_votes(env: Env) -> u64 { @@ -150,8 +186,23 @@ impl UnexpectedRevert { } pub fn get_state(env: Env) -> State { - env.storage().instance().get(&STATE).unwrap() + env.storage().instance().get(&DataKey::State).unwrap() } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn print(){ + let env = Env::default(); + let contract = UnexpectedRevertClient::new(&env, &env.register_contract(None, UnexpectedRevert{})); + let candidate_addr = Address::generate(&env); + //let result = contract.retrieve_candidate(&candidate_addr); + assert_eq!(contract.try_retrieve_candidate(&candidate_addr), Err(Ok(URError::CandidateDoesntExist))); + + } } \ No newline at end of file From d945a74f99bd7f29c3c2de3ac4bb1999ac58ba41 Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Fri, 19 Apr 2024 15:43:44 -0300 Subject: [PATCH 03/16] unexpected revert with vector detector fix and update, test cases fix --- .../Cargo.toml | 4 - .../src/lib.rs | 89 ++++--------------- .../remediated-example/Cargo.toml | 5 -- .../remediated-example/lib.rs | 51 +++-------- .../vulnerable-example/Cargo.toml | 5 -- 5 files changed, 29 insertions(+), 125 deletions(-) diff --git a/detectors/dos-unexpected-revert-with-vector/Cargo.toml b/detectors/dos-unexpected-revert-with-vector/Cargo.toml index fd462f96..1e0c38c6 100644 --- a/detectors/dos-unexpected-revert-with-vector/Cargo.toml +++ b/detectors/dos-unexpected-revert-with-vector/Cargo.toml @@ -11,10 +11,6 @@ scout-audit-clippy-utils = { workspace = true } dylint_linting = { workspace = true } if_chain = { workspace = true } -scout-audit-internal = { workspace = true } - -[dev-dependencies] -dylint_testing = { workspace = true } [package.metadata.rust-analyzer] rustc_private = true \ No newline at end of file diff --git a/detectors/dos-unexpected-revert-with-vector/src/lib.rs b/detectors/dos-unexpected-revert-with-vector/src/lib.rs index 4592f1da..1f9be01a 100644 --- a/detectors/dos-unexpected-revert-with-vector/src/lib.rs +++ b/detectors/dos-unexpected-revert-with-vector/src/lib.rs @@ -10,7 +10,6 @@ use std::collections::HashSet; use rustc_hir::intravisit::walk_expr; use rustc_hir::intravisit::Visitor; -use rustc_hir::QPath; use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::mir::Const; @@ -20,40 +19,22 @@ use rustc_middle::mir::{ use rustc_middle::ty::{Ty, TyKind}; use rustc_span::def_id::DefId; use rustc_span::Span; -use scout_audit_internal::{DetectorImpl, InkDetector as Detector}; +use scout_audit_clippy_utils::diagnostics::span_lint; + +const LINT_MESSAGE: &str = "This vector operation is called without considering storage limitations"; dylint_linting::impl_late_lint! { - /// ### What it does - /// Checks for array pushes without access control. - /// ### Why is this bad? - /// Arrays have a maximum size according to the storage cell. If the array is full, the push will revert. This can be used to prevent the execution of a function. - /// ### Known problems - /// If the owner validation is performed in an auxiliary function, the warning will be shown, resulting in a false positive. - /// ### Example - /// ```rust - /// if self.votes.contains(candidate) { - /// Err(Errors::CandidateAlreadyAdded) - /// } else { - /// self.candidates.push(candidate); - /// self.votes.insert(candidate, 0); - /// Ok(()) - /// } - /// ``` - /// Use instead: - /// ```rust - /// if self.votes.contains(candidate) { - /// Err(Errors::CandidateAlreadyAdded) - /// } else { - /// self.candidates.set(self.total_candidates, candidate); - /// self.total_candidates += 1; - /// self.votes.set(candidate, 0); - /// Ok(()) - /// } - /// ``` pub UNEXPECTED_REVERT_WARN, Warn, "", - UnexpectedRevertWarn::default() + UnexpectedRevertWarn::default(), + { + name: "Unexpected Revert Inserting to Storage", + long_message: " It occurs by preventing transactions by other users from being successfully executed forcing the blockchain state to revert to its original state.", + severity: "Medium", + help: "https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unexpected-revert-with-vector", + vulnerability_class: "Denial of Service", + } } #[derive(Default)] @@ -81,55 +62,14 @@ impl<'tcx> LateLintPass<'tcx> for UnexpectedRevertWarn { } impl<'tcx> Visitor<'tcx> for UnprotectedVectorFinder<'tcx, '_> { fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { - if let ExprKind::MethodCall(path, receiver, ..) = expr.kind { + if let ExprKind::MethodCall(path, _receiver, ..) = expr.kind { let defid = self.cx.typeck_results().type_dependent_def_id(expr.hir_id); let ty = Ty::new_foreign(self.cx.tcx, defid.unwrap()); if ty.to_string().contains("soroban_sdk::Vec") { if path.ident.name.to_string() == "push_back" || path.ident.name.to_string() == "push_front" { self.push_def_id = defid; } - } else if let ExprKind::MethodCall(rec_path, receiver2, ..) = receiver.kind - && rec_path.ident.name.to_string() == "env" - && let ExprKind::Path(rec2_qpath) = &receiver2.kind - && let QPath::Resolved(qualifier, rec2_path) = rec2_qpath - && rec2_path.segments.first().map_or(false, |seg| { - seg.ident.to_string() == "self" && qualifier.is_none() - }) - && path.ident.name.to_string() == "caller" - { - if self - .cx - .typeck_results() - .type_dependent_def_id(expr.hir_id) - .is_some() - { - self.callers_def_id.insert( - self.cx - .typeck_results() - .type_dependent_def_id(expr.hir_id) - .unwrap(), - ); - } - } else if let ExprKind::Call(receiver2, ..) = receiver.kind - && let ExprKind::Path(rec2_qpath) = &receiver2.kind - && let QPath::TypeRelative(ty2, rec2_path) = rec2_qpath - && rec2_path.ident.name.to_string() == "env" - && let rustc_hir::TyKind::Path(rec3_qpath) = &ty2.kind - && let QPath::Resolved(_, rec3_path) = rec3_qpath - && rec3_path.segments[0].ident.to_string() == "Self" - && self - .cx - .typeck_results() - .type_dependent_def_id(expr.hir_id) - .is_some() - { - self.callers_def_id.insert( - self.cx - .typeck_results() - .type_dependent_def_id(expr.hir_id) - .unwrap(), - ); - } + } } walk_expr(self, expr); } @@ -208,10 +148,11 @@ impl<'tcx> LateLintPass<'tcx> for UnexpectedRevertWarn { &mut HashSet::::default(), ); for place in unchecked_places { - Detector::DosUnexpectedRevertWithVector.span_lint( + span_lint( cx, UNEXPECTED_REVERT_WARN, place.1, + LINT_MESSAGE ); } } diff --git a/test-cases/dos-unexpected-revert-with-vector/remediated-example/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/remediated-example/Cargo.toml index 577569e2..218402e4 100644 --- a/test-cases/dos-unexpected-revert-with-vector/remediated-example/Cargo.toml +++ b/test-cases/dos-unexpected-revert-with-vector/remediated-example/Cargo.toml @@ -30,8 +30,3 @@ lto = true [profile.release-with-logs] inherits = "release" debug-assertions = true - -[workspace.metadata.dylint] -libraries = [ - { path="/home/sofiazcoaga/scout-soroban/detectors/dos-unexpected-revert-with-vector" }, -] \ No newline at end of file diff --git a/test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs b/test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs index 56c9d4b4..f561c7f9 100644 --- a/test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs +++ b/test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs @@ -1,6 +1,6 @@ #![no_std] -use soroban_sdk::{ String, contracterror, contract, contracttype, contractimpl, Symbol, symbol_short, Env, Address, Map}; +use soroban_sdk::{String, Env, contracterror, contract, contracttype, contractimpl, Address}; #[contracterror] @@ -55,7 +55,7 @@ pub struct UnexpectedRevert; #[contractimpl] impl UnexpectedRevert { - pub fn set_candidate(env: Env, candidate: Address, id: u32, votes: u64) { + pub fn set_candidate(env: Env, candidate: Address, votes: u64) { let cand = Candidate { votes }; @@ -93,7 +93,7 @@ impl UnexpectedRevert { if Self::vote_ended(env.clone()) { return Err(URError::VoteEnded); } - if Self::already_voted(env.clone(), caller.clone()) { + if Self::account_has_voted(env.clone(), caller.clone()) { return Err(URError::AccountAlreadyVoted); } else { env.storage().instance().set(&DataKey::Candidate(candidate.clone()), &Candidate{votes: 0}); @@ -106,7 +106,7 @@ impl UnexpectedRevert { } - pub fn already_voted(env: Env, caller: Address) -> bool { + pub fn account_has_voted(env: Env, caller: Address) -> bool { let already_voted: AlreadyVoted = env.storage().instance().get(&DataKey::AlreadyVoted(caller)).unwrap_or_default(); already_voted.voted } @@ -140,18 +140,14 @@ impl UnexpectedRevert { state.total_candidates } - pub fn get_candidate(env: Env, index: u32) -> Result { - let state = Self::get_state(env); - match state.candidates.get(index) { - Some(candidate) => Ok(candidate), - None => Err(URError::CandidateDoesntExist), - } + pub fn get_candidate(env: Env, addr: Address) -> Result { + let result: Option = env.storage().instance().get(&DataKey::Candidate(addr)); + match result { + Some(cand) => Ok(cand), + None => Err(URError::CandidateDoesntExist) + } } - pub fn account_has_voted(env: Env, account: Address) -> bool { - let state = Self::get_state(env); - state.already_voted.get(account).unwrap_or(false) - } pub fn vote(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { caller.require_auth(); @@ -160,18 +156,14 @@ impl UnexpectedRevert { return Err(URError::VoteEnded); } - if state.already_voted.contains_key(caller.clone()) { + if Self::account_has_voted(env.clone(), caller.clone()){ Err(URError::AccountAlreadyVoted) } else { - state.already_voted.set(caller, true); - let votes = state - .votes - .get(candidate.clone()) - .ok_or(URError::CandidateDoesntExist)? + env.storage().instance().set(&DataKey::AlreadyVoted(caller.clone()), &AlreadyVoted{voted: true}); + let votes = Self::get_candidate(env.clone(), candidate.clone())?.votes .checked_add(1) .ok_or(URError::Overflow)?; - state.votes.set(candidate.clone(), votes); - state.total_votes.checked_add(1).ok_or(URError::Overflow)?; + env.storage().instance().set(&DataKey::Candidate(candidate.clone()), &Candidate{votes}); if state.candidate_votes < votes { state.candidate_votes = votes; state.most_voted_candidate = candidate; @@ -191,18 +183,3 @@ impl UnexpectedRevert { } - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn print(){ - let env = Env::default(); - let contract = UnexpectedRevertClient::new(&env, &env.register_contract(None, UnexpectedRevert{})); - let candidate_addr = Address::generate(&env); - //let result = contract.retrieve_candidate(&candidate_addr); - assert_eq!(contract.try_retrieve_candidate(&candidate_addr), Err(Ok(URError::CandidateDoesntExist))); - - } -} \ No newline at end of file diff --git a/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/Cargo.toml index 577569e2..218402e4 100644 --- a/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/Cargo.toml +++ b/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/Cargo.toml @@ -30,8 +30,3 @@ lto = true [profile.release-with-logs] inherits = "release" debug-assertions = true - -[workspace.metadata.dylint] -libraries = [ - { path="/home/sofiazcoaga/scout-soroban/detectors/dos-unexpected-revert-with-vector" }, -] \ No newline at end of file From 1adce0f54730d41a87a0fa5aa6255f06d7674269 Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Tue, 23 Apr 2024 13:52:28 -0300 Subject: [PATCH 04/16] fix ci errors --- .../Cargo.toml | 21 ++++ .../remediated-example/Cargo.toml | 16 +++ .../remediated-example/src}/lib.rs | 114 ++++++++++-------- .../vulnerable-example/Cargo.toml | 16 +++ .../vulnerable-example/src}/lib.rs | 54 ++++----- .../remediated-example/Cargo.toml | 32 ----- .../vulnerable-example/Cargo.toml | 32 ----- 7 files changed, 139 insertions(+), 146 deletions(-) create mode 100644 test-cases/dos-unexpected-revert-with-vector/Cargo.toml create mode 100644 test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/Cargo.toml rename test-cases/dos-unexpected-revert-with-vector/{remediated-example => dos-unexpected-revert-with-vector-1/remediated-example/src}/lib.rs (60%) create mode 100644 test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/Cargo.toml rename test-cases/dos-unexpected-revert-with-vector/{vulnerable-example => dos-unexpected-revert-with-vector-1/vulnerable-example/src}/lib.rs (81%) delete mode 100644 test-cases/dos-unexpected-revert-with-vector/remediated-example/Cargo.toml delete mode 100644 test-cases/dos-unexpected-revert-with-vector/vulnerable-example/Cargo.toml diff --git a/test-cases/dos-unexpected-revert-with-vector/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/Cargo.toml new file mode 100644 index 00000000..21fad67c --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/Cargo.toml @@ -0,0 +1,21 @@ +[workspace] +exclude = [".cargo", "target"] +members = ["dos-unexpected-revert-with-vector-*/*"] +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/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..6e8aa2ee --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dos-unexpected-revert-with-vector-remediated-1" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { workspace = true } + +[dev_dependencies] +soroban-sdk = { workspace = true, features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/src/lib.rs similarity index 60% rename from test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs rename to test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/src/lib.rs index f561c7f9..26443f78 100644 --- a/test-cases/dos-unexpected-revert-with-vector/remediated-example/lib.rs +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/src/lib.rs @@ -1,25 +1,25 @@ #![no_std] -use soroban_sdk::{String, Env, contracterror, contract, contracttype, contractimpl, Address}; - +use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Address, Env, String}; #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] #[repr(u32)] -pub enum URError { // Unexpected Revert Error +pub enum URError { + // Unexpected Revert Error AccountAlreadyVoted = 1, - CandidateAlreadyAdded = 2, - CandidateDoesntExist = 3, - Overflow = 4, - TimestampBeforeCurrentBlock = 5, - VoteEnded = 6 + CandidateAlreadyAdded = 2, + CandidateDoesntExist = 3, + Overflow = 4, + TimestampBeforeCurrentBlock = 5, + VoteEnded = 6, } #[derive(Debug, Clone, PartialEq)] #[contracttype] pub struct State { total_votes: u64, - total_candidates: u32, + total_candidates: u32, most_voted_candidate: Address, candidate_votes: u64, vote_timestamp_end: u64, @@ -28,63 +28,64 @@ pub struct State { #[derive(Debug, Clone, PartialEq)] #[contracttype] pub struct Candidate { - votes: u64 + votes: u64, } #[contracttype] pub struct AlreadyVoted { - voted: bool + voted: bool, } impl Default for AlreadyVoted { fn default() -> Self { - AlreadyVoted {voted:false} + AlreadyVoted { voted: false } } } #[contracttype] pub enum DataKey { State, - AlreadyVoted(Address), - Candidate(Address) + AlreadyVoted(Address), + Candidate(Address), } #[contract] -pub struct UnexpectedRevert; +pub struct UnexpectedRevert; #[contractimpl] impl UnexpectedRevert { - pub fn set_candidate(env: Env, candidate: Address, votes: u64) { - let cand = Candidate { - votes - }; + let cand = Candidate { votes }; - env.storage().instance().set(&DataKey::Candidate(candidate), &cand); + env.storage() + .instance() + .set(&DataKey::Candidate(candidate), &cand); } pub fn retrieve_candidate(env: Env, candidate: Address) -> Result { - env.storage().instance().get(&DataKey::Candidate(candidate)).unwrap_or(Err(URError::CandidateDoesntExist)) + env.storage() + .instance() + .get(&DataKey::Candidate(candidate)) + .unwrap_or(Err(URError::CandidateDoesntExist)) } - + pub fn init(env: Env, end_timestamp: u64) -> Result { if end_timestamp <= env.ledger().timestamp() { - return Err(URError::TimestampBeforeCurrentBlock); + return Err(URError::TimestampBeforeCurrentBlock); } - + let zero_string: String = String::from_str(&env, "00000000000000000000000000000000"); - let zero_addr = Address::from_string(&zero_string); // Whenever this is zero address it will mean no candidate has yet been more voted + let zero_addr = Address::from_string(&zero_string); // Whenever this is zero address it will mean no candidate has yet been more voted let state = State { - total_votes:0, - total_candidates: 0, + total_votes: 0, + total_candidates: 0, most_voted_candidate: zero_addr, candidate_votes: 0, vote_timestamp_end: end_timestamp, }; - env.storage().instance().set(&DataKey::State, &state); + env.storage().instance().set(&DataKey::State, &state); Ok(state) - } pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { @@ -94,39 +95,43 @@ impl UnexpectedRevert { return Err(URError::VoteEnded); } if Self::account_has_voted(env.clone(), caller.clone()) { - return Err(URError::AccountAlreadyVoted); + return Err(URError::AccountAlreadyVoted); } else { - env.storage().instance().set(&DataKey::Candidate(candidate.clone()), &Candidate{votes: 0}); - state.total_candidates += 1; + env.storage().instance().set( + &DataKey::Candidate(candidate.clone()), + &Candidate { votes: 0 }, + ); + state.total_candidates += 1; env.storage().instance().set(&DataKey::State, &state); Ok(()) } - - - } pub fn account_has_voted(env: Env, caller: Address) -> bool { - let already_voted: AlreadyVoted = env.storage().instance().get(&DataKey::AlreadyVoted(caller)).unwrap_or_default(); + let already_voted: AlreadyVoted = env + .storage() + .instance() + .get(&DataKey::AlreadyVoted(caller)) + .unwrap_or_default(); already_voted.voted } pub fn get_votes_for_a_candidate(env: Env, candidate: Address) -> Result { - let result: Option = env.storage().instance().get(&DataKey::Candidate(candidate)); + let result: Option = + env.storage().instance().get(&DataKey::Candidate(candidate)); match result { - Some(cand) => Ok(cand.votes), - None => Err(URError::CandidateDoesntExist) - } - + Some(cand) => Ok(cand.votes), + None => Err(URError::CandidateDoesntExist), + } } - + pub fn most_voted_candidate_votes(env: Env) -> u64 { let state = Self::get_state(env); state.candidate_votes } pub fn most_voted_candidate(env: Env) -> Address { - let state = Self::get_state(env); + let state = Self::get_state(env); state.most_voted_candidate } @@ -143,12 +148,11 @@ impl UnexpectedRevert { pub fn get_candidate(env: Env, addr: Address) -> Result { let result: Option = env.storage().instance().get(&DataKey::Candidate(addr)); match result { - Some(cand) => Ok(cand), - None => Err(URError::CandidateDoesntExist) - } + Some(cand) => Ok(cand), + None => Err(URError::CandidateDoesntExist), + } } - pub fn vote(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { caller.require_auth(); let mut state = Self::get_state(env.clone()); @@ -156,14 +160,20 @@ impl UnexpectedRevert { return Err(URError::VoteEnded); } - if Self::account_has_voted(env.clone(), caller.clone()){ + if Self::account_has_voted(env.clone(), caller.clone()) { Err(URError::AccountAlreadyVoted) } else { - env.storage().instance().set(&DataKey::AlreadyVoted(caller.clone()), &AlreadyVoted{voted: true}); - let votes = Self::get_candidate(env.clone(), candidate.clone())?.votes + env.storage().instance().set( + &DataKey::AlreadyVoted(caller.clone()), + &AlreadyVoted { voted: true }, + ); + let votes = Self::get_candidate(env.clone(), candidate.clone())? + .votes .checked_add(1) .ok_or(URError::Overflow)?; - env.storage().instance().set(&DataKey::Candidate(candidate.clone()), &Candidate{votes}); + env.storage() + .instance() + .set(&DataKey::Candidate(candidate.clone()), &Candidate { votes }); if state.candidate_votes < votes { state.candidate_votes = votes; state.most_voted_candidate = candidate; @@ -180,6 +190,4 @@ impl UnexpectedRevert { pub fn get_state(env: Env) -> State { env.storage().instance().get(&DataKey::State).unwrap() } - - } diff --git a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..b0cf5914 --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dos-unexpected-revert-with-vector-vulnerable-1" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { workspace = true } + +[dev_dependencies] +soroban-sdk = { workspace = true, features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/lib.rs b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/src/lib.rs similarity index 81% rename from test-cases/dos-unexpected-revert-with-vector/vulnerable-example/lib.rs rename to test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/src/lib.rs index d3f6b196..24b19df6 100644 --- a/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/lib.rs +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/src/lib.rs @@ -1,17 +1,21 @@ #![no_std] -use soroban_sdk::{ String, contracterror, contract, contracttype, contractimpl, Symbol, symbol_short, Env, Address, Map, Vec}; +use soroban_sdk::{ + contract, contracterror, contractimpl, contracttype, symbol_short, Address, Env, Map, String, + Symbol, Vec, +}; #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] #[repr(u32)] -pub enum URError { // Unexpected Revert Error +pub enum URError { + // Unexpected Revert Error AccountAlreadyVoted = 1, - CandidateAlreadyAdded = 2, - CandidateDoesntExist = 3, - Overflow = 4, - TimestampBeforeCurrentBlock = 5, - VoteEnded = 6 + CandidateAlreadyAdded = 2, + CandidateDoesntExist = 3, + Overflow = 4, + TimestampBeforeCurrentBlock = 5, + VoteEnded = 6, } #[derive(Debug, Clone, PartialEq)] @@ -26,34 +30,32 @@ pub struct State { vote_timestamp_end: u64, } -const STATE: Symbol = symbol_short!("STATE"); +const STATE: Symbol = symbol_short!("STATE"); #[contract] -pub struct UnexpectedRevert; +pub struct UnexpectedRevert; #[contractimpl] impl UnexpectedRevert { - pub fn init(env: Env, end_timestamp: u64) -> Result { if end_timestamp <= env.ledger().timestamp() { - return Err(URError::TimestampBeforeCurrentBlock); + return Err(URError::TimestampBeforeCurrentBlock); } - + let zero_string: String = String::from_str(&env, "00000000000000000000000000000000"); - let zero_addr = Address::from_string(&zero_string); //CHECK + let zero_addr = Address::from_string(&zero_string); //CHECK let state = State { - total_votes:0, + total_votes: 0, most_voted_candidate: zero_addr, candidate_votes: 0, candidates: Vec::new(&env), - already_voted: Map::new(&env), + already_voted: Map::new(&env), votes: Map::new(&env), vote_timestamp_end: end_timestamp, }; - env.storage().instance().set(&STATE, &state); + env.storage().instance().set(&STATE, &state); Ok(state) - } pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { @@ -63,31 +65,29 @@ impl UnexpectedRevert { return Err(URError::VoteEnded); } if state.already_voted.contains_key(caller.clone()) { - return Err(URError::AccountAlreadyVoted); + return Err(URError::AccountAlreadyVoted); } else { state.candidates.push_back(candidate.clone()); state.votes.set(candidate, 0); Ok(()) } - - - } pub fn get_votes_for_a_candidate(env: Env, candidate: Address) -> Result { - let state = Self::get_state(env.clone()); - state.votes + let state = Self::get_state(env.clone()); + state + .votes .get(candidate) .ok_or(URError::CandidateDoesntExist) } - + pub fn most_voted_candidate_votes(env: Env) -> u64 { let state = Self::get_state(env); state.candidate_votes } pub fn most_voted_candidate(env: Env) -> Address { - let state = Self::get_state(env); + let state = Self::get_state(env); state.most_voted_candidate } @@ -150,8 +150,4 @@ impl UnexpectedRevert { pub fn get_state(env: Env) -> State { env.storage().instance().get(&STATE).unwrap() } - - } - - diff --git a/test-cases/dos-unexpected-revert-with-vector/remediated-example/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/remediated-example/Cargo.toml deleted file mode 100644 index 218402e4..00000000 --- a/test-cases/dos-unexpected-revert-with-vector/remediated-example/Cargo.toml +++ /dev/null @@ -1,32 +0,0 @@ -[package] -name = "unexpected_revert" -version = "0.1.0" -edition = "2021" - - -[lib] -crate-type = ["cdylib"] -path = "lib.rs" - -[dependencies] -soroban-sdk = "=20.0.0" - -[dev_dependencies] -soroban-sdk = { version = "=20.0.0", features = ["testutils"] } - -[features] -testutils = ["soroban-sdk/testutils"] - -[profile.release] -opt-level = "z" -overflow-checks = true -debug = 0 -strip = "symbols" -debug-assertions = false -panic = "abort" -codegen-units = 1 -lto = true - -[profile.release-with-logs] -inherits = "release" -debug-assertions = true diff --git a/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/Cargo.toml deleted file mode 100644 index 218402e4..00000000 --- a/test-cases/dos-unexpected-revert-with-vector/vulnerable-example/Cargo.toml +++ /dev/null @@ -1,32 +0,0 @@ -[package] -name = "unexpected_revert" -version = "0.1.0" -edition = "2021" - - -[lib] -crate-type = ["cdylib"] -path = "lib.rs" - -[dependencies] -soroban-sdk = "=20.0.0" - -[dev_dependencies] -soroban-sdk = { version = "=20.0.0", features = ["testutils"] } - -[features] -testutils = ["soroban-sdk/testutils"] - -[profile.release] -opt-level = "z" -overflow-checks = true -debug = 0 -strip = "symbols" -debug-assertions = false -panic = "abort" -codegen-units = 1 -lto = true - -[profile.release-with-logs] -inherits = "release" -debug-assertions = true From e3af9f80f33d3edb1e3b4c33c9d3a4bba1a8b3a1 Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Tue, 23 Apr 2024 14:19:51 -0300 Subject: [PATCH 05/16] fix ci errors --- .../src/lib.rs | 23 ++++++++----------- .../Cargo.toml | 1 + .../remediated-example/src/lib.rs | 9 ++------ .../vulnerable-example/src/lib.rs | 2 +- 4 files changed, 14 insertions(+), 21 deletions(-) diff --git a/detectors/dos-unexpected-revert-with-vector/src/lib.rs b/detectors/dos-unexpected-revert-with-vector/src/lib.rs index 1f9be01a..a7372363 100644 --- a/detectors/dos-unexpected-revert-with-vector/src/lib.rs +++ b/detectors/dos-unexpected-revert-with-vector/src/lib.rs @@ -21,7 +21,8 @@ use rustc_span::def_id::DefId; use rustc_span::Span; use scout_audit_clippy_utils::diagnostics::span_lint; -const LINT_MESSAGE: &str = "This vector operation is called without considering storage limitations"; +const LINT_MESSAGE: &str = + "This vector operation is called without considering storage limitations"; dylint_linting::impl_late_lint! { pub UNEXPECTED_REVERT_WARN, @@ -65,11 +66,12 @@ impl<'tcx> LateLintPass<'tcx> for UnexpectedRevertWarn { if let ExprKind::MethodCall(path, _receiver, ..) = expr.kind { let defid = self.cx.typeck_results().type_dependent_def_id(expr.hir_id); let ty = Ty::new_foreign(self.cx.tcx, defid.unwrap()); - if ty.to_string().contains("soroban_sdk::Vec") { - if path.ident.name.to_string() == "push_back" || path.ident.name.to_string() == "push_front" { - self.push_def_id = defid; - } - } + if ty.to_string().contains("soroban_sdk::Vec") + && (path.ident.name.to_string() == "push_back" + || path.ident.name.to_string() == "push_front") + { + self.push_def_id = defid; + } } walk_expr(self, expr); } @@ -148,12 +150,7 @@ impl<'tcx> LateLintPass<'tcx> for UnexpectedRevertWarn { &mut HashSet::::default(), ); for place in unchecked_places { - span_lint( - cx, - UNEXPECTED_REVERT_WARN, - place.1, - LINT_MESSAGE - ); + span_lint(cx, UNEXPECTED_REVERT_WARN, place.1, LINT_MESSAGE); } } @@ -325,4 +322,4 @@ impl<'tcx> LateLintPass<'tcx> for UnexpectedRevertWarn { ret_vec } } -} \ No newline at end of file +} diff --git a/test-cases/dos-unexpected-revert-with-vector/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/Cargo.toml index 21fad67c..82ef8b11 100644 --- a/test-cases/dos-unexpected-revert-with-vector/Cargo.toml +++ b/test-cases/dos-unexpected-revert-with-vector/Cargo.toml @@ -19,3 +19,4 @@ strip = "symbols" [profile.release-with-logs] debug-assertions = true inherits = "release" + diff --git a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/src/lib.rs b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/src/lib.rs index 26443f78..3dc6323c 100644 --- a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/src/lib.rs +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/src/lib.rs @@ -31,17 +31,12 @@ pub struct Candidate { votes: u64, } +#[derive(Default)] #[contracttype] pub struct AlreadyVoted { voted: bool, } -impl Default for AlreadyVoted { - fn default() -> Self { - AlreadyVoted { voted: false } - } -} - #[contracttype] pub enum DataKey { State, @@ -95,7 +90,7 @@ impl UnexpectedRevert { return Err(URError::VoteEnded); } if Self::account_has_voted(env.clone(), caller.clone()) { - return Err(URError::AccountAlreadyVoted); + Err(URError::AccountAlreadyVoted) } else { env.storage().instance().set( &DataKey::Candidate(candidate.clone()), diff --git a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/src/lib.rs b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/src/lib.rs index 24b19df6..1837aa9f 100644 --- a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/src/lib.rs +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/src/lib.rs @@ -65,7 +65,7 @@ impl UnexpectedRevert { return Err(URError::VoteEnded); } if state.already_voted.contains_key(caller.clone()) { - return Err(URError::AccountAlreadyVoted); + Err(URError::AccountAlreadyVoted) } else { state.candidates.push_back(candidate.clone()); state.votes.set(candidate, 0); From c098c52ab62b795a84a3d9b8f3570c88a94980ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Garc=C3=ADa=20Crosta?= Date: Fri, 26 Apr 2024 15:29:52 -0400 Subject: [PATCH 06/16] Update test-detectors.yml Try to fix mac-os build --- .github/workflows/test-detectors.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index ea1e7ded..cc654d7b 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -42,7 +42,7 @@ jobs: matrix: os: - ubuntu-latest - - macos-latest + - macos-13 runs-on: ${{ matrix.os }} steps: - name: Checkout repository From 27d05c6651dfa495a17980ec4d63ea68fd848f10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Garc=C3=ADa=20Crosta?= Date: Fri, 26 Apr 2024 15:38:06 -0400 Subject: [PATCH 07/16] Update test-detectors.yml Tests should also be performed on previous macos verdion --- .github/workflows/test-detectors.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index cc654d7b..7ee37af6 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -97,7 +97,7 @@ jobs: matrix: os: - ubuntu-latest - - macos-latest + - macos-13 detector: ${{fromJson(needs.prepare-detector-matrix.outputs.matrix)}} runs-on: ${{ matrix.os }} outputs: From 8efdbfa2e7c302168bdac9e3ff501b54139263a5 Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Tue, 30 Apr 2024 01:06:34 -0300 Subject: [PATCH 08/16] fix --- .../src/lib.rs | 264 ++---------------- .../vulnerable-example/src/lib.rs | 1 - 2 files changed, 18 insertions(+), 247 deletions(-) diff --git a/detectors/dos-unexpected-revert-with-vector/src/lib.rs b/detectors/dos-unexpected-revert-with-vector/src/lib.rs index a7372363..e3505dce 100644 --- a/detectors/dos-unexpected-revert-with-vector/src/lib.rs +++ b/detectors/dos-unexpected-revert-with-vector/src/lib.rs @@ -6,23 +6,16 @@ extern crate rustc_hir; extern crate rustc_middle; extern crate rustc_span; -use std::collections::HashSet; - use rustc_hir::intravisit::walk_expr; use rustc_hir::intravisit::Visitor; use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::mir::Const; -use rustc_middle::mir::{ - BasicBlock, BasicBlockData, BasicBlocks, Operand, Place, StatementKind, TerminatorKind, -}; -use rustc_middle::ty::{Ty, TyKind}; +use rustc_middle::ty::Ty; use rustc_span::def_id::DefId; use rustc_span::Span; use scout_audit_clippy_utils::diagnostics::span_lint; -const LINT_MESSAGE: &str = - "This vector operation is called without considering storage limitations"; +const LINT_MESSAGE: &str = "This vector operation is called without access control"; dylint_linting::impl_late_lint! { pub UNEXPECTED_REVERT_WARN, @@ -54,23 +47,29 @@ impl<'tcx> LateLintPass<'tcx> for UnexpectedRevertWarn { _: &'tcx rustc_hir::FnDecl<'tcx>, body: &'tcx rustc_hir::Body<'tcx>, _: Span, - localdef: rustc_span::def_id::LocalDefId, + _localdef: rustc_span::def_id::LocalDefId, ) { struct UnprotectedVectorFinder<'tcx, 'tcx_ref> { cx: &'tcx_ref LateContext<'tcx>, - callers_def_id: HashSet, push_def_id: Option, + push_span: Option, + require_auth: bool, } impl<'tcx> Visitor<'tcx> for UnprotectedVectorFinder<'tcx, '_> { fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { if let ExprKind::MethodCall(path, _receiver, ..) = expr.kind { let defid = self.cx.typeck_results().type_dependent_def_id(expr.hir_id); let ty = Ty::new_foreign(self.cx.tcx, defid.unwrap()); + if path.ident.name.to_string() == "require_auth" { + //println!("este es un require auth"); + self.require_auth = true; + } if ty.to_string().contains("soroban_sdk::Vec") && (path.ident.name.to_string() == "push_back" || path.ident.name.to_string() == "push_front") { self.push_def_id = defid; + self.push_span = Some(path.ident.span); } } walk_expr(self, expr); @@ -79,247 +78,20 @@ impl<'tcx> LateLintPass<'tcx> for UnexpectedRevertWarn { let mut uvf_storage = UnprotectedVectorFinder { cx, - callers_def_id: HashSet::default(), push_def_id: None, + push_span: None, + require_auth: false, }; walk_expr(&mut uvf_storage, body.value); - let mir_body = cx.tcx.optimized_mir(localdef); - - struct CallersAndVecOps<'tcx> { - callers: Vec<(&'tcx BasicBlockData<'tcx>, BasicBlock)>, - vec_ops: Vec<(&'tcx BasicBlockData<'tcx>, BasicBlock)>, - } - - fn find_caller_and_vec_ops_in_mir<'tcx>( - bbs: &'tcx BasicBlocks<'tcx>, - callers_def_id: HashSet, - push_def_id: Option, - ) -> CallersAndVecOps { - let mut callers_vec = CallersAndVecOps { - callers: vec![], - vec_ops: vec![], - }; - for (bb, bb_data) in bbs.iter().enumerate() { - if bb_data.terminator.as_ref().is_none() { - continue; - } - let terminator = bb_data.terminator.clone().unwrap(); - if let TerminatorKind::Call { func, .. } = terminator.kind { - if let Operand::Constant(fn_const) = func - && let Const::Val(_const_val, ty) = fn_const.const_ - && let TyKind::FnDef(def, _subs) = ty.kind() - { - if !callers_def_id.is_empty() { - for caller in &callers_def_id { - if caller == def { - callers_vec - .callers - .push((bb_data, BasicBlock::from_usize(bb))); - } - } - } else { - for op in &push_def_id { - if op == def { - callers_vec - .vec_ops - .push((bb_data, BasicBlock::from_usize(bb))); - } - } - } - } - } - } - callers_vec - } - - let caller_and_vec_ops = find_caller_and_vec_ops_in_mir( - &mir_body.basic_blocks, - uvf_storage.callers_def_id, - uvf_storage.push_def_id, - ); - - if !caller_and_vec_ops.vec_ops.is_empty() { - let unchecked_places = navigate_trough_basicblocks( - &mir_body.basic_blocks, - BasicBlock::from_u32(0), - &caller_and_vec_ops, - false, - &mut vec![], - &mut HashSet::::default(), + if uvf_storage.push_def_id.is_some() && !uvf_storage.require_auth { + span_lint( + uvf_storage.cx, + UNEXPECTED_REVERT_WARN, + uvf_storage.push_span.unwrap(), + LINT_MESSAGE, ); - for place in unchecked_places { - span_lint(cx, UNEXPECTED_REVERT_WARN, place.1, LINT_MESSAGE); - } - } - - fn navigate_trough_basicblocks<'tcx>( - bbs: &'tcx BasicBlocks<'tcx>, - bb: BasicBlock, - caller_and_vec_ops: &CallersAndVecOps<'tcx>, - after_comparison: bool, - tainted_places: &mut Vec>, - visited_bbs: &mut HashSet, - ) -> Vec<(Place<'tcx>, Span)> { - let mut ret_vec = Vec::<(Place, Span)>::new(); - if visited_bbs.contains(&bb) { - return ret_vec; - } else { - visited_bbs.insert(bb); - } - if bbs[bb].terminator.is_none() { - return ret_vec; - } - for statement in &bbs[bb].statements { - if let StatementKind::Assign(assign) = &statement.kind { - match &assign.1 { - rustc_middle::mir::Rvalue::Ref(_, _, origplace) - | rustc_middle::mir::Rvalue::AddressOf(_, origplace) - | rustc_middle::mir::Rvalue::Len(origplace) - | rustc_middle::mir::Rvalue::CopyForDeref(origplace) => { - if tainted_places - .clone() - .into_iter() - .any(|place| place == *origplace) - { - tainted_places.push(assign.0); - } - } - rustc_middle::mir::Rvalue::Use(operand) => match &operand { - Operand::Copy(origplace) | Operand::Move(origplace) => { - if tainted_places - .clone() - .into_iter() - .any(|place| place == *origplace) - { - tainted_places.push(assign.0); - } - } - _ => {} - }, - _ => {} - } - } - } - match &bbs[bb].terminator().kind { - TerminatorKind::SwitchInt { discr, targets } => { - let comparison_with_caller = match discr { - Operand::Copy(place) | Operand::Move(place) => { - tainted_places - .iter() - .any(|tainted_place| tainted_place == place) - || after_comparison - } - Operand::Constant(_cons) => after_comparison, - }; - for target in targets.all_targets() { - ret_vec.append(&mut navigate_trough_basicblocks( - bbs, - *target, - caller_and_vec_ops, - comparison_with_caller, - tainted_places, - visited_bbs, - )); - } - return ret_vec; - } - TerminatorKind::Call { - destination, - args, - target, - fn_span, - .. - } => { - for arg in args { - match arg { - Operand::Copy(origplace) | Operand::Move(origplace) => { - if tainted_places - .clone() - .into_iter() - .any(|place| place == *origplace) - { - tainted_places.push(*destination); - } - } - Operand::Constant(_) => {} - } - } - for caller in &caller_and_vec_ops.callers { - if caller.1 == bb { - tainted_places.push(*destination); - } - } - for map_op in &caller_and_vec_ops.vec_ops { - if map_op.1 == bb - && !after_comparison - && args.get(1).map_or(true, |f| { - f.place().is_some_and(|f| !tainted_places.contains(&f)) - }) - { - ret_vec.push((*destination, *fn_span)) - } - } - if target.is_some() { - ret_vec.append(&mut navigate_trough_basicblocks( - bbs, - target.unwrap(), - caller_and_vec_ops, - after_comparison, - tainted_places, - visited_bbs, - )); - } - } - TerminatorKind::Assert { target, .. } - | TerminatorKind::Goto { target, .. } - | TerminatorKind::Drop { target, .. } => { - ret_vec.append(&mut navigate_trough_basicblocks( - bbs, - *target, - caller_and_vec_ops, - after_comparison, - tainted_places, - visited_bbs, - )); - } - TerminatorKind::Yield { resume, .. } => { - ret_vec.append(&mut navigate_trough_basicblocks( - bbs, - *resume, - caller_and_vec_ops, - after_comparison, - tainted_places, - visited_bbs, - )); - } - TerminatorKind::FalseEdge { real_target, .. } - | TerminatorKind::FalseUnwind { real_target, .. } => { - ret_vec.append(&mut navigate_trough_basicblocks( - bbs, - *real_target, - caller_and_vec_ops, - after_comparison, - tainted_places, - visited_bbs, - )); - } - TerminatorKind::InlineAsm { destination, .. } => { - if destination.is_some() { - ret_vec.append(&mut navigate_trough_basicblocks( - bbs, - destination.unwrap(), - caller_and_vec_ops, - after_comparison, - tainted_places, - visited_bbs, - )); - } - } - _ => {} - } - ret_vec } } } diff --git a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/src/lib.rs b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/src/lib.rs index 1837aa9f..728137e9 100644 --- a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/src/lib.rs +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/src/lib.rs @@ -59,7 +59,6 @@ impl UnexpectedRevert { } pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { - caller.require_auth(); let mut state = Self::get_state(env.clone()); if Self::vote_ended(env.clone()) { return Err(URError::VoteEnded); From 5343cea094453df1f4132180c21f670c0a26be6e Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Tue, 30 Apr 2024 10:06:50 -0300 Subject: [PATCH 09/16] workflow fix --- .github/workflows/test-detectors.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index 7ee37af6..d5d374cb 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -121,7 +121,9 @@ jobs: restore-keys: ${{ runner.os }}-tests- - name: Run unit and integration tests - run: python scripts/run-tests.py --detector=${{ matrix.detector }} + run: | + rustup component add rust-src --toolchain nightly-2023-12-16-x86_64-apple-darwin + python scripts/run-tests.py --detector=${{ matrix.detector }} comment-on-pr: name: Comment on PR From cd20c306a2d7598abb17bef3eb1f28299b5afa7a Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Tue, 30 Apr 2024 11:14:48 -0300 Subject: [PATCH 10/16] fix workflow --- scripts/run-tests.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/run-tests.py b/scripts/run-tests.py index eba5931c..04dd558e 100644 --- a/scripts/run-tests.py +++ b/scripts/run-tests.py @@ -48,7 +48,7 @@ def run_unit_tests(root): def run_integration_tests(detector, root): start_time = time.time() - returncode, stdout, _ = run_subprocess( + returncode, stdout, stderr = run_subprocess( [ "cargo", "scout-audit", @@ -62,6 +62,8 @@ def run_integration_tests(detector, root): ) if stdout is None: + for line in stderr.splitlines(): + print(f" error: {line}") print( f"{RED}Failed to run integration tests in {root} - Metadata returned empty.{ENDC}" ) From 1b1991ac245592dd6c5f47f23263e85b1dd33b59 Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Tue, 30 Apr 2024 11:20:09 -0300 Subject: [PATCH 11/16] add dependency --- .github/workflows/test-detectors.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index d5d374cb..fa6d4fcd 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -122,6 +122,7 @@ jobs: - name: Run unit and integration tests run: | + rustup target add x86_64-apple-darwin rustup component add rust-src --toolchain nightly-2023-12-16-x86_64-apple-darwin python scripts/run-tests.py --detector=${{ matrix.detector }} From 9fcc62b34ac94708d9c1b53d0f22c9b7203914d9 Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Tue, 30 Apr 2024 12:16:38 -0300 Subject: [PATCH 12/16] modify workflow --- .github/workflows/test-detectors.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index fa6d4fcd..a20f3f95 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -122,7 +122,7 @@ jobs: - name: Run unit and integration tests run: | - rustup target add x86_64-apple-darwin + rustup toolchain install nightly-2023-12-16-x86_64-apple-darwin rustup component add rust-src --toolchain nightly-2023-12-16-x86_64-apple-darwin python scripts/run-tests.py --detector=${{ matrix.detector }} From 95b9b69df776aeea446a124ce7be6e868ccb2366 Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Tue, 30 Apr 2024 14:54:09 -0300 Subject: [PATCH 13/16] add no_std to test cases --- .../unsafe-expect-1/remediated-example/src/lib.rs | 1 + .../unsafe-expect-1/vulnerable-example/src/lib.rs | 1 + .../unsafe-expect-2/remediated-example/src/lib.rs | 1 + .../unsafe-expect-2/vulnerable-example/src/lib.rs | 1 + .../unsafe-expect-3/remediated-example/src/lib.rs | 1 + .../unsafe-expect-3/vulnerable-example/src/lib.rs | 1 + .../unsafe-expect-4/remediated-example/src/lib.rs | 1 + .../unsafe-expect-4/vulnerable-example/src/lib.rs | 3 ++- .../unsafe-expect-5/remediated-example/src/lib.rs | 1 + .../unsafe-expect-5/vulnerable-example/src/lib.rs | 1 + 10 files changed, 11 insertions(+), 1 deletion(-) diff --git a/test-cases/unsafe-expect/unsafe-expect-1/remediated-example/src/lib.rs b/test-cases/unsafe-expect/unsafe-expect-1/remediated-example/src/lib.rs index 18916280..37dcc366 100644 --- a/test-cases/unsafe-expect/unsafe-expect-1/remediated-example/src/lib.rs +++ b/test-cases/unsafe-expect/unsafe-expect-1/remediated-example/src/lib.rs @@ -1,3 +1,4 @@ +#![no_std] use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, Map, Symbol}; #[contract] diff --git a/test-cases/unsafe-expect/unsafe-expect-1/vulnerable-example/src/lib.rs b/test-cases/unsafe-expect/unsafe-expect-1/vulnerable-example/src/lib.rs index e3d8601f..f3f14ad8 100644 --- a/test-cases/unsafe-expect/unsafe-expect-1/vulnerable-example/src/lib.rs +++ b/test-cases/unsafe-expect/unsafe-expect-1/vulnerable-example/src/lib.rs @@ -1,3 +1,4 @@ +#![no_std] use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, Map, Symbol}; #[contract] diff --git a/test-cases/unsafe-expect/unsafe-expect-2/remediated-example/src/lib.rs b/test-cases/unsafe-expect/unsafe-expect-2/remediated-example/src/lib.rs index 7a5349c9..d487edd3 100644 --- a/test-cases/unsafe-expect/unsafe-expect-2/remediated-example/src/lib.rs +++ b/test-cases/unsafe-expect/unsafe-expect-2/remediated-example/src/lib.rs @@ -1,3 +1,4 @@ +#![no_std] use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, Map, Symbol}; #[contract] diff --git a/test-cases/unsafe-expect/unsafe-expect-2/vulnerable-example/src/lib.rs b/test-cases/unsafe-expect/unsafe-expect-2/vulnerable-example/src/lib.rs index e3d8601f..f3f14ad8 100644 --- a/test-cases/unsafe-expect/unsafe-expect-2/vulnerable-example/src/lib.rs +++ b/test-cases/unsafe-expect/unsafe-expect-2/vulnerable-example/src/lib.rs @@ -1,3 +1,4 @@ +#![no_std] use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, Map, Symbol}; #[contract] diff --git a/test-cases/unsafe-expect/unsafe-expect-3/remediated-example/src/lib.rs b/test-cases/unsafe-expect/unsafe-expect-3/remediated-example/src/lib.rs index e764f8b0..10b9fb40 100644 --- a/test-cases/unsafe-expect/unsafe-expect-3/remediated-example/src/lib.rs +++ b/test-cases/unsafe-expect/unsafe-expect-3/remediated-example/src/lib.rs @@ -1,3 +1,4 @@ +#![no_std] use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, Map, Symbol}; #[contract] diff --git a/test-cases/unsafe-expect/unsafe-expect-3/vulnerable-example/src/lib.rs b/test-cases/unsafe-expect/unsafe-expect-3/vulnerable-example/src/lib.rs index e3d8601f..f3f14ad8 100644 --- a/test-cases/unsafe-expect/unsafe-expect-3/vulnerable-example/src/lib.rs +++ b/test-cases/unsafe-expect/unsafe-expect-3/vulnerable-example/src/lib.rs @@ -1,3 +1,4 @@ +#![no_std] use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, Map, Symbol}; #[contract] diff --git a/test-cases/unsafe-expect/unsafe-expect-4/remediated-example/src/lib.rs b/test-cases/unsafe-expect/unsafe-expect-4/remediated-example/src/lib.rs index c7b1c42d..41ccefdd 100644 --- a/test-cases/unsafe-expect/unsafe-expect-4/remediated-example/src/lib.rs +++ b/test-cases/unsafe-expect/unsafe-expect-4/remediated-example/src/lib.rs @@ -1,3 +1,4 @@ +#![no_std] use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, Map, Symbol}; #[contract] diff --git a/test-cases/unsafe-expect/unsafe-expect-4/vulnerable-example/src/lib.rs b/test-cases/unsafe-expect/unsafe-expect-4/vulnerable-example/src/lib.rs index 726a6e8c..14c937b9 100644 --- a/test-cases/unsafe-expect/unsafe-expect-4/vulnerable-example/src/lib.rs +++ b/test-cases/unsafe-expect/unsafe-expect-4/vulnerable-example/src/lib.rs @@ -1,3 +1,4 @@ +#![no_std] use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, Map, Symbol}; #[contract] @@ -22,7 +23,7 @@ impl UnsafeExpect { // Save the state. env.storage().persistent().set(&STATE, &state); } - + state } diff --git a/test-cases/unsafe-expect/unsafe-expect-5/remediated-example/src/lib.rs b/test-cases/unsafe-expect/unsafe-expect-5/remediated-example/src/lib.rs index 3bada60e..d3acce06 100644 --- a/test-cases/unsafe-expect/unsafe-expect-5/remediated-example/src/lib.rs +++ b/test-cases/unsafe-expect/unsafe-expect-5/remediated-example/src/lib.rs @@ -1,3 +1,4 @@ +#![no_std] use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, Map, Symbol}; #[contract] diff --git a/test-cases/unsafe-expect/unsafe-expect-5/vulnerable-example/src/lib.rs b/test-cases/unsafe-expect/unsafe-expect-5/vulnerable-example/src/lib.rs index 6ee792de..a15ec3c7 100644 --- a/test-cases/unsafe-expect/unsafe-expect-5/vulnerable-example/src/lib.rs +++ b/test-cases/unsafe-expect/unsafe-expect-5/vulnerable-example/src/lib.rs @@ -1,3 +1,4 @@ +#![no_std] use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, Map, Symbol}; #[contract] From f58021540cacc88acf5455af85ae9c539fb073f7 Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Tue, 30 Apr 2024 15:10:21 -0300 Subject: [PATCH 14/16] modify test detectors workflow --- .github/workflows/test-detectors.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index a20f3f95..e14c388f 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -139,11 +139,13 @@ jobs: id: ubuntu_status working-directory: build-status-ubuntu-latest run: echo "status=$(cat status-ubuntu-latest.txt)" >> $GITHUB_OUTPUT + continue-on-error: true - name: Read macOS build status id: macos_status working-directory: build-status-macos-latest run: echo "status=$(cat status-macos-latest.txt)" >> $GITHUB_OUTPUT + continue-on-error: true - name: Find comment id: find_comment From f17e3476fff578de2be630f3dcd5513d61dcbac6 Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Tue, 30 Apr 2024 15:34:43 -0300 Subject: [PATCH 15/16] remove comment --- scripts/run-tests.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/scripts/run-tests.py b/scripts/run-tests.py index 04dd558e..eba5931c 100644 --- a/scripts/run-tests.py +++ b/scripts/run-tests.py @@ -48,7 +48,7 @@ def run_unit_tests(root): def run_integration_tests(detector, root): start_time = time.time() - returncode, stdout, stderr = run_subprocess( + returncode, stdout, _ = run_subprocess( [ "cargo", "scout-audit", @@ -62,8 +62,6 @@ def run_integration_tests(detector, root): ) if stdout is None: - for line in stderr.splitlines(): - print(f" error: {line}") print( f"{RED}Failed to run integration tests in {root} - Metadata returned empty.{ENDC}" ) From a16db54132b8a6c7f44b19950a24608335e01c51 Mon Sep 17 00:00:00 2001 From: Sofi Azcoaga Date: Tue, 30 Apr 2024 15:48:42 -0300 Subject: [PATCH 16/16] fix ci/cd fmt. add pycache to gitignore. fix fmt in unsafe-expect --- .gitignore | 2 ++ scripts/run-fmt.py | 6 +++++- scripts/utils.py | 7 ++++--- .../unsafe-expect-4/vulnerable-example/src/lib.rs | 2 +- 4 files changed, 12 insertions(+), 5 deletions(-) diff --git a/.gitignore b/.gitignore index 8bea821b..5c8a1ff3 100644 --- a/.gitignore +++ b/.gitignore @@ -41,3 +41,5 @@ node_modules .vscode/ .idea/ !/.vscode/ + +**/__pycache__/ diff --git a/scripts/run-fmt.py b/scripts/run-fmt.py index bc912426..7685aaaf 100644 --- a/scripts/run-fmt.py +++ b/scripts/run-fmt.py @@ -22,7 +22,11 @@ def run_fmt(directories): if is_rust_project(root): start_time = time.time() returncode, _, stderr = run_subprocess( - ["cargo", "fmt", "--all", "--check"], + ["cargo", "fmt"], + cwd=root, + ) + returncode, _, stderr = run_subprocess( + ["cargo", "fmt", "--check"], cwd=root, ) print_results( diff --git a/scripts/utils.py b/scripts/utils.py index 2678db8f..b9d0e8cf 100644 --- a/scripts/utils.py +++ b/scripts/utils.py @@ -78,6 +78,7 @@ def print_results(returncode, error_message, check_type, root, elapsed_time): ) if returncode != 0: print(f"\n{RED}{check_type.capitalize()} {issue_type} found in: {root}{ENDC}\n") - for line in error_message.strip().split("\n"): - print(f"| {line}") - print("\n") + if not error_message is None: + for line in error_message.strip().split("\n"): + print(f"| {line}") + print("\n") diff --git a/test-cases/unsafe-expect/unsafe-expect-4/vulnerable-example/src/lib.rs b/test-cases/unsafe-expect/unsafe-expect-4/vulnerable-example/src/lib.rs index 14c937b9..7a98f717 100644 --- a/test-cases/unsafe-expect/unsafe-expect-4/vulnerable-example/src/lib.rs +++ b/test-cases/unsafe-expect/unsafe-expect-4/vulnerable-example/src/lib.rs @@ -23,7 +23,7 @@ impl UnsafeExpect { // Save the state. env.storage().persistent().set(&STATE, &state); } - + state }