From d30d70a82a17149bf0fc5427c809331250bcf7a5 Mon Sep 17 00:00:00 2001 From: arturoBeccar Date: Fri, 5 Jul 2024 12:15:43 -0300 Subject: [PATCH 1/9] Split test cases for dos-unexpected-revert-with-vector --- .../remediated-example/src/lib.rs | 127 +++++------- .../remediated-example/Cargo.toml | 16 ++ .../remediated-example/src/lib.rs | 187 ++++++++++++++++++ .../vulnerable-example/Cargo.toml | 16 ++ .../vulnerable-example/src/lib.rs | 152 ++++++++++++++ 5 files changed, 418 insertions(+), 80 deletions(-) create mode 100644 test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/Cargo.toml create mode 100644 test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/src/lib.rs create mode 100644 test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/vulnerable-example/Cargo.toml create mode 100644 test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/vulnerable-example/src/lib.rs 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 3dc6323c..4c55a1a9 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 @@ -1,6 +1,9 @@ #![no_std] -use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Address, Env, String}; +use soroban_sdk::{ + contract, contracterror, contractimpl, contracttype, symbol_short, Address, Env, Map, String, + Symbol, Vec, +}; #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] @@ -19,105 +22,65 @@ pub enum URError { #[contracttype] pub struct State { total_votes: u64, - total_candidates: u32, + candidates: Vec
, + votes: Map, + already_voted: Map, most_voted_candidate: Address, candidate_votes: u64, vote_timestamp_end: u64, + admin: Address, } -#[derive(Debug, Clone, PartialEq)] -#[contracttype] -pub struct Candidate { - votes: u64, -} - -#[derive(Default)] -#[contracttype] -pub struct AlreadyVoted { - voted: bool, -} - -#[contracttype] -pub enum DataKey { - State, - AlreadyVoted(Address), - Candidate(Address), -} +const STATE: Symbol = symbol_short!("STATE"); #[contract] pub struct UnexpectedRevert; #[contractimpl] impl UnexpectedRevert { - pub fn set_candidate(env: Env, candidate: Address, 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 { + pub fn init(env: Env, end_timestamp: u64, admin: Address) -> 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); // Whenever this is zero address it will mean no candidate has yet been more voted + 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: Vec::new(&env), + already_voted: Map::new(&env), + votes: Map::new(&env), vote_timestamp_end: end_timestamp, + admin: admin, }; - env.storage().instance().set(&DataKey::State, &state); + 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()); + state.admin.require_auth(); if Self::vote_ended(env.clone()) { return Err(URError::VoteEnded); } - if Self::account_has_voted(env.clone(), caller.clone()) { + if state.already_voted.contains_key(caller.clone()) { Err(URError::AccountAlreadyVoted) } else { - env.storage().instance().set( - &DataKey::Candidate(candidate.clone()), - &Candidate { votes: 0 }, - ); - state.total_candidates += 1; - env.storage().instance().set(&DataKey::State, &state); + state.candidates.push_back(candidate.clone()); + state.votes.set(candidate, 0); 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(); - 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)); - match result { - Some(cand) => Ok(cand.votes), - None => Err(URError::CandidateDoesntExist), - } + let state = Self::get_state(env.clone()); + state + .votes + .get(candidate) + .ok_or(URError::CandidateDoesntExist) } pub fn most_voted_candidate_votes(env: Env) -> u64 { @@ -135,19 +98,25 @@ impl UnexpectedRevert { state.total_votes } - pub fn get_total_candidates(env: Env) -> u32 { + pub fn get_total_candidates(env: Env) -> u64 { let state = Self::get_state(env); - state.total_candidates + state.candidates.len() as u64 } - 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 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()); @@ -155,20 +124,18 @@ impl UnexpectedRevert { return Err(URError::VoteEnded); } - if Self::account_has_voted(env.clone(), caller.clone()) { + if state.already_voted.contains_key(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())? + state.already_voted.set(caller, true); + let votes = state .votes + .get(candidate.clone()) + .ok_or(URError::CandidateDoesntExist)? .checked_add(1) .ok_or(URError::Overflow)?; - env.storage() - .instance() - .set(&DataKey::Candidate(candidate.clone()), &Candidate { votes }); + 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; @@ -183,6 +150,6 @@ impl UnexpectedRevert { } pub fn get_state(env: Env) -> State { - env.storage().instance().get(&DataKey::State).unwrap() + env.storage().instance().get(&STATE).unwrap() } -} +} \ No newline at end of file diff --git a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/Cargo.toml new file mode 100644 index 00000000..dad1352e --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dos-unexpected-revert-with-vector-remediated-2" +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/dos-unexpected-revert-with-vector-2/remediated-example/src/lib.rs b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/src/lib.rs new file mode 100644 index 00000000..65ee078a --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/src/lib.rs @@ -0,0 +1,187 @@ +#![no_std] + +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 + 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, + most_voted_candidate: Address, + candidate_votes: u64, + vote_timestamp_end: u64, +} + +#[derive(Debug, Clone, PartialEq)] +#[contracttype] +pub struct Candidate { + votes: u64, +} + +#[derive(Default)] +#[contracttype] +pub struct AlreadyVoted { + voted: bool, +} + +#[contracttype] +pub enum DataKey { + State, + AlreadyVoted(Address), + Candidate(Address), +} + +#[contract] +pub struct UnexpectedRevert; + +#[contractimpl] +impl UnexpectedRevert { + pub fn set_candidate(env: Env, candidate: Address, 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() { + 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 state = State { + 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); + Ok(state) + } + + pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + let mut state = Self::get_state(env.clone()); + if Self::vote_ended(env.clone()) { + return Err(URError::VoteEnded); + } + if Self::account_has_voted(env.clone(), caller.clone()) { + Err(URError::AccountAlreadyVoted) + } else { + 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(); + 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)); + match result { + 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); + 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, 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 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 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 + .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; + } + 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(&DataKey::State).unwrap() + } +} diff --git a/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/vulnerable-example/Cargo.toml b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..c75916e5 --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/vulnerable-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dos-unexpected-revert-with-vector-vulnerable-2" +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/dos-unexpected-revert-with-vector-2/vulnerable-example/src/lib.rs b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..728137e9 --- /dev/null +++ b/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/vulnerable-example/src/lib.rs @@ -0,0 +1,152 @@ +#![no_std] + +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 + 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> { + 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.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 3442ecc9a48b2e244054ce0e32e62e9db6b0c76d Mon Sep 17 00:00:00 2001 From: arturoBeccar Date: Fri, 5 Jul 2024 13:04:40 -0300 Subject: [PATCH 2/9] Add new test case to detector's table for dos with vector --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index c7c5c37f..6833c4a1 100644 --- a/README.md +++ b/README.md @@ -96,7 +96,7 @@ Currently Scout includes the following detectors. | [iterators-over-indexing](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/iterators-over-indexing) |Iterating with hardcoded indexes is slower than using an iterator. Also, if the index is out of bounds, it will panic. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/iterators-over-indexing-1) | Enhancement | | [assert-violation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/assert-violation) | Avoid the usage of the macro assert!, it can panic.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/assert-violation/assert-violation-1) | Enhancement | | [unprotected-mapping-operation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-mapping-operation) | Modifying mappings with an arbitrary key given by users can be a significant vulnerability. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-2) | Critical | -| [dos-unexpected-revert-with-vector](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unexpected-revert-with-vector) | DoS due to improper storage. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1) | Medium | +| [dos-unexpected-revert-with-vector](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unexpected-revert-with-vector) | DoS due to improper storage. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2) | Medium | | [unrestricted-transfer-from](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unrestricted-transfer-from) | Avoid passing an user-defined parameter as a `from` field in transfer-from. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unrestricted-transfer-from/unrestricted-transfer-from-1) | Critical | | [unsafe-map-get](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-map-get) | Inappropriate usage of the `get` method for `Map` in soroban | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unsafe-map-get/unsafe-map-get-1) | Medium | | [zero-or-test-address](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/zero-or-test-address) | Avoid zero or test address assignment to prevent contract control loss. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/zero-or-test-address/zero-or-test-address-1) | Medium | From 9932ca75463ddbe092ff043a6ea67b6b4645d77f Mon Sep 17 00:00:00 2001 From: arturoBeccar Date: Fri, 5 Jul 2024 13:13:55 -0300 Subject: [PATCH 3/9] Update suggested remediation options --- .../17-dos-unexpected-revert-with-vector.md | 46 +++++++++++++------ 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md b/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md index 9656b552..d5b228ea 100644 --- a/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md +++ b/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md @@ -15,25 +15,42 @@ If the owner validation is performed in an auxiliary function, the warning will ### Example ```rust - 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 add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + 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(()) + } +} ``` Use instead: +```rust +pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + let mut state = Self::get_state(env.clone()); + // Require authorization from an admin set at contract initalization. + state.admin.require_auth(); + 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(()) + } +} +``` +Or + ```rust pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { caller.require_auth(); @@ -44,6 +61,7 @@ Use instead: if Self::account_has_voted(env.clone(), caller.clone()) { return Err(URError::AccountAlreadyVoted); } else { + // Replace the Vector with a mapping like structure made with a DataKey enum. env.storage().instance().set(&DataKey::Candidate(candidate.clone()), &Candidate{votes: 0}); state.total_candidates += 1; env.storage().instance().set(&DataKey::State, &state); From af73ad5f0026dfcd8a1cdaded901ba7c4e286b0d Mon Sep 17 00:00:00 2001 From: arturoBeccar Date: Fri, 5 Jul 2024 13:17:24 -0300 Subject: [PATCH 4/9] Remove duplicated 6-unprotected-update-current-contract-wamsm copy --- ...ted-update-current-contract-wasm (copy).md | 59 ------------------- 1 file changed, 59 deletions(-) delete mode 100644 docs/docs/vulnerabilities/6-unprotected-update-current-contract-wasm (copy).md diff --git a/docs/docs/vulnerabilities/6-unprotected-update-current-contract-wasm (copy).md b/docs/docs/vulnerabilities/6-unprotected-update-current-contract-wasm (copy).md deleted file mode 100644 index 803af1db..00000000 --- a/docs/docs/vulnerabilities/6-unprotected-update-current-contract-wasm (copy).md +++ /dev/null @@ -1,59 +0,0 @@ -# Unprotected update current contract wasm - -## Description - -- Vulnerability Category: `Authorization` -- Vulnerability Severity: `Critical` -- Detectors: [`unprotected-update-current-contract-wasm`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-update-current-contract-wasm) -- Test Cases: [`unprotected-update-current-contract-wasm-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1) [`unprotected-update-current-contract-wasm-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-2) - -It warns you if `update_current_contract_wasm()` function is called without a previous check of the address of the caller. If users are allowed to call `update_current_contract_wasm()`, they can intentionally modify the contract behaviour, leading to the loss of all associated data/tokens and functionalities given by this contract or by others that depend on it. - -## Exploit Scenario - -Consider the following `Soroban` contract: - -```rust -#[contractimpl] -impl UpgradeableContract { - pub fn init(e: Env, admin: Address) { - e.storage().instance().set(&DataKey::Admin, &admin); - } - - pub fn version() -> u32 { - 1 - } - - pub fn upgrade(e: Env, new_wasm_hash: BytesN<32>) { - e.deployer().update_current_contract_wasm(new_wasm_hash); - } -} -``` -This contract allows upgrades through the `update_current_contract_wasm` function. If just anyone can call this function, they could modify the contract behaviour. - -The vulnerable code example can be found [`here`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/vulnerable-example). - -## Remediation - -To prevent this, the function should be restricted to administrators or authorized users only. - -```rust -#[contractimpl] -impl UpgradeableContract { - pub fn init(e: Env, admin: Address) { - e.storage().instance().set(&DataKey::Admin, &admin); - } - - pub fn version() -> u32 { - 1 - } - - pub fn upgrade(e: Env, new_wasm_hash: BytesN<32>) { - let admin: Address = e.storage().instance().get(&DataKey::Admin).unwrap(); - admin.require_auth(); - - e.deployer().update_current_contract_wasm(new_wasm_hash); - } -} -``` -The remediated code example can be found [`here`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/remediated-example). \ No newline at end of file From c3804e9314fbd3c50ed203538d4234a6635e8f7a Mon Sep 17 00:00:00 2001 From: arturoBeccar Date: Fri, 5 Jul 2024 13:43:17 -0300 Subject: [PATCH 5/9] Add documentation for dos-unexpected-revert-with-vector vulnerablilty and detector --- .../17-dos-unexpected-revert-with-vector.md | 246 ++++++++++++++++++ 1 file changed, 246 insertions(+) create mode 100644 docs/docs/vulnerabilities/17-dos-unexpected-revert-with-vector.md diff --git a/docs/docs/vulnerabilities/17-dos-unexpected-revert-with-vector.md b/docs/docs/vulnerabilities/17-dos-unexpected-revert-with-vector.md new file mode 100644 index 00000000..5586d367 --- /dev/null +++ b/docs/docs/vulnerabilities/17-dos-unexpected-revert-with-vector.md @@ -0,0 +1,246 @@ +# DoS unexpected revert with vector + +## Description + +- Vulnerability Category: `DoS` +- Severity: `Medium` +- Detectors: [`dos-unexpected-revert-with-vector`](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unexpected-revert-with-vector) +- Test Cases: [`dos-unexpected-revert-with-vector-1`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1), [`dos-unexpected-revert-with-vector-2`](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2) + +This vulnerability of DoS through unexpected revert arises when a smart +contract does not handle storage size errors correctly, and a user can add an +excessive number of entries, leading to an unexpected revert of transactions +by other users and a Denial of Service. This vulnerability can be exploited by +an attacker to perform a DoS attack on the network and can result in lost +funds, poor user experience, and even harm the network's overall security. + +## Exploit Scenario + +The vulnerable smart contract we developed for his example allows users to +vote for one of different candidates. +The smart contract contains a struct named `UnexpectedRevert` that stores the +total number of votes, a list of candidates, their votes, and whether an +account has voted. It also stores information about the most voted candidate +and when the vote will end. + +```rust +#![no_std] + +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 + 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> { + 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.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() + } +} +``` + +The smart contract has several functions that allow adding a candidate, getting +votes for a specific candidate, getting the account ID of the most voted +candidate, getting the total votes, getting the total number of candidates, +getting a candidate by index, checking if an account has voted, and voting for +a candidate. + +In this case, we see that a vector is being used to store the array of candidates for an election. +Notice how the candidates array push operation has no access control or proper storage management in the function `add_candidate()`, which can cause a revert if the array is full. + + +The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout-soroban/blob/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/src/lib.rs). + +## Remediation + +This issue can be addressed in different ways. + +On the one hand, if the amount of candidates is going to be limited, and only authorized users are going to add new candidates, then enforcing this authorization would be a sufficient fix to prevent attackers from filling the array and producing a denial of service attack. + +```rust +pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { + let mut state = Self::get_state(env.clone()); + // Require authorization from an admin set at contract initalization. + state.admin.require_auth(); + 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(()) + } +} +``` +This remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/blob/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/src/lib.rs). + + +Alternatively, if any user should be authorized to add new candidates, a different data structure should be used, one without the limitations of a vector. For example, a dictionary can be implemented in Soroban by defining a struct for the `Candidate`, accessible through a `DataKey` enum like the one we have here. This data structure does not have the storage limitations of vectors, and using it to handle new candidates would prevent the issue. + +```rust + 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 Self::account_has_voted(env.clone(), caller.clone()) { + return Err(URError::AccountAlreadyVoted); + } else { + // Replace the Vector with a mapping like structure made with a DataKey enum. + env.storage().instance().set(&DataKey::Candidate(candidate.clone()), &Candidate{votes: 0}); + state.total_candidates += 1; + env.storage().instance().set(&DataKey::State, &state); + Ok(()) + } +} +``` + +This remediated code example can be found [here](https://github.com/CoinFabrik/scout-soroban/blob/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-2/remediated-example/src/lib.rs). + +## References + +- [SWC-113](https://swcregistry.io/docs/SWC-113) +- https://consensys.github.io/smart-contract-best-practices/attacks/denial-of-service/#dos-with-unexpected-revert +- [Ethernaut: King](https://github.com/OpenZeppelin/ethernaut/blob/master/contracts/src/levels/King.sol) \ No newline at end of file From 7a0afbc1bf1957e50da3e84548ff7818f2b5e6ae Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Fri, 5 Jul 2024 13:48:06 -0300 Subject: [PATCH 6/9] Correct indentation --- docs/docs/detectors/17-dos-unexpected-revert-with-vector.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md b/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md index d5b228ea..bd56c7ad 100644 --- a/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md +++ b/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md @@ -35,8 +35,8 @@ Use instead: ```rust pub fn add_candidate(env: Env, candidate: Address, caller: Address) -> Result<(), URError> { let mut state = Self::get_state(env.clone()); - // Require authorization from an admin set at contract initalization. - state.admin.require_auth(); + // Require authorization from an admin set at contract initalization. + state.admin.require_auth(); if Self::vote_ended(env.clone()) { return Err(URError::VoteEnded); } From 3489ba1b7d3f9eb6ae47217cae4533683e1fd927 Mon Sep 17 00:00:00 2001 From: arturoBeccar Date: Wed, 10 Jul 2024 10:34:53 -0300 Subject: [PATCH 7/9] Remove empty line --- .../remediated-example/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 4c55a1a9..f5f0bace 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 @@ -152,4 +152,4 @@ impl UnexpectedRevert { pub fn get_state(env: Env) -> State { env.storage().instance().get(&STATE).unwrap() } -} \ No newline at end of file +} From bc7ccf4f8b7724896a2459d6d2cf2a45bbcb88fd Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Fri, 12 Jul 2024 10:36:39 -0300 Subject: [PATCH 8/9] Update lib.rs admin attribute --- .../remediated-example/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 f5f0bace..d8c39715 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 @@ -53,7 +53,7 @@ impl UnexpectedRevert { already_voted: Map::new(&env), votes: Map::new(&env), vote_timestamp_end: end_timestamp, - admin: admin, + admin, }; env.storage().instance().set(&STATE, &state); From 5e38ce629b772d8604e0a6b48cb1d1c67537cf39 Mon Sep 17 00:00:00 2001 From: david weil Date: Thu, 18 Jul 2024 12:09:07 -0300 Subject: [PATCH 9/9] fix: remove use of deprecated function uint::max_value --- .../avoid-panic-error-1/remediated-example/src/lib.rs | 2 +- .../avoid-panic-error-1/vulnerable-example/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test-cases/avoid-panic-error/avoid-panic-error-1/remediated-example/src/lib.rs b/test-cases/avoid-panic-error/avoid-panic-error-1/remediated-example/src/lib.rs index 871aa92d..4a50c7d8 100644 --- a/test-cases/avoid-panic-error/avoid-panic-error-1/remediated-example/src/lib.rs +++ b/test-cases/avoid-panic-error/avoid-panic-error-1/remediated-example/src/lib.rs @@ -60,7 +60,7 @@ mod tests { let client = AvoidPanicErrorClient::new(&env, &contract_id); // When - let _max_value = client.try_add(&u32::max_value()); + let _max_value = client.try_add(&u32::MAX); let overflow = client.try_add(&1); // Then diff --git a/test-cases/avoid-panic-error/avoid-panic-error-1/vulnerable-example/src/lib.rs b/test-cases/avoid-panic-error/avoid-panic-error-1/vulnerable-example/src/lib.rs index 2ead32b9..7119ed1d 100644 --- a/test-cases/avoid-panic-error/avoid-panic-error-1/vulnerable-example/src/lib.rs +++ b/test-cases/avoid-panic-error/avoid-panic-error-1/vulnerable-example/src/lib.rs @@ -54,7 +54,7 @@ mod tests { let client = AvoidPanicErrorClient::new(&env, &contract_id); // When - client.add(&u32::max_value()); + client.add(&u32::MAX); client.add(&1); // Then