diff --git a/README.md b/README.md index 60318707..83fbc779 100644 --- a/README.md +++ b/README.md @@ -65,6 +65,7 @@ Visit [Scout's website](https://coinfabrik.github.io/scout-soroban/) to view the | [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 | ## Tests diff --git a/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md b/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md new file mode 100644 index 00000000..9656b552 --- /dev/null +++ b/docs/docs/detectors/17-dos-unexpected-revert-with-vector.md @@ -0,0 +1,57 @@ +# DoS unexpected revert with vector + +### 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 + 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(()) + } + } + +``` + +Use instead: + +```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 { + env.storage().instance().set(&DataKey::Candidate(candidate.clone()), &Candidate{votes: 0}); + state.total_candidates += 1; + env.storage().instance().set(&DataKey::State, &state); + Ok(()) + } +} +``` + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unexpected-revert-with-vector). diff --git a/docs/docs/vulnerabilities/README.md b/docs/docs/vulnerabilities/README.md index 6b461635..ce8de19a 100644 --- a/docs/docs/vulnerabilities/README.md +++ b/docs/docs/vulnerabilities/README.md @@ -222,3 +222,16 @@ Modifying mappings with an arbitrary key given by the user could lead to uninten This vulnerability falls under the [Validations and error handling category](#vulnerability-categories) and assigned it a Critical severity. +### DoS unexpected revert with vector + +Another type of Denial of Service attack is called unexpected revert. It occurs +by preventing transactions by other users from being successfully executed, +forcing the blockchain state to revert to its original state. + +A Denial of Service through unexpected revert can +accomplished by exploiting a smart contract that does not manage storage size +errors correctly. It can be prevented by using Mapping instead of Vec to avoid +storage limit problems. + +This vulnerability again falls under the [Denial of Service](#vulnerability-categories) category +and has a Medium severity.