#256 review test cases for dos unexpected revert with vector

README.md

@@ -58,7 +58,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  |

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);

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<Address>,
+    votes: Map<Address, u64>,
+    already_voted: Map<Address, bool>,
+    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<State, URError> {
+        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<u64, URError> {
+        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<Address, URError> {
+        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)