-
Notifications
You must be signed in to change notification settings - Fork 5
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request #258 from CoinFabrik/256-review-test-cases-for-dos…
…-unexpected-revert-with-vector #256 review test cases for dos unexpected revert with vector
- Loading branch information
Showing
9 changed files
with
696 additions
and
153 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
246 changes: 246 additions & 0 deletions
246
docs/docs/vulnerabilities/17-dos-unexpected-revert-with-vector.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) |
59 changes: 0 additions & 59 deletions
59
docs/docs/vulnerabilities/6-unprotected-update-current-contract-wasm (copy).md
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.