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