From 9ccdc0d95d85beebfaa666e4a3f43ec5c5390329 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Tue, 23 Apr 2024 10:13:30 -0300 Subject: [PATCH 1/3] Add vulnerability description --- docs/docs/vulnerabilities/README.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) 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. From d938257a0171f6ac66bddfec56a93954e78a8420 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Tue, 23 Apr 2024 13:34:50 -0300 Subject: [PATCH 2/3] Add detector documentation --- .../17-dos-unexpected-revert-with-vector.md | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 docs/docs/detectors/17-dos-unexpected-revert-with-vector.md 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). From a98cd69974148a95db298cf047962390f43411ff Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Tue, 23 Apr 2024 13:49:01 -0300 Subject: [PATCH 3/3] Add detector to detectors table --- README.md | 1 + 1 file changed, 1 insertion(+) 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