Merge pull request #257 from CoinFabrik/documentation-new-detectors

Documentation for new detectors
CoinFabrik · Apr 29, 2024 · f4b661c · f4b661c
2 parents 9c2f77e + 6211e57
commit f4b661c
Show file tree

Hide file tree

Showing 53 changed files with 844 additions and 131 deletions.
diff --git a/README.md b/README.md
@@ -64,12 +64,12 @@ Visit [Scout's website](https://coinfabrik.github.io/scout/) to view the full do
 | [zero-or-test-address](https://coinfabrik.github.io/scout/docs/detectors/zero-or-test-address)                           | [Avoid zero or test address assignment to prevent contract control loss.](https://coinfabrik.github.io/scout/docs/vulnerabilities/zero-or-test-address)                                                   | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/zero-or-test-address/zero-or-test-address-1)                                                                                                                                                | Medium      |
 | [insufficiently-random-values](https://coinfabrik.github.io/scout/docs/detectors/insufficiently-random-values)           | [Avoid using block attributes for random number generation to prevent manipulation.](https://coinfabrik.github.io/scout/docs/vulnerabilities/insufficiently-random-values)                                | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/insufficiently-random-values/insufficiently-random-values-1)                                                                                                                                | Critical    |
 | [unrestricted-transfer-from](https://coinfabrik.github.io/scout/docs/detectors/unrestricted-transfer-from)               | [Avoid passing an user-defined parameter as a `from` field in transfer-from ](https://coinfabrik.github.io/scout/docs/vulnerabilities/unrestricted-transfer-from)                                         | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/unrestricted-transfer-from/unrestricted-transfer-from-1)                                                                                                                                    | Critical        |
-| [assert-violation](https://coinfabrik.github.io/scout/docs/detectors/assert-violation)                                   | [Avoid the usage of the macro `assert!`, it can panic.](https://coinfabrik.github.io/scout/docs/vulnerabilities/assert-violation)                                                                         | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/assert-violation/assert-violation-1)                                                                                                                                                        | Enhacement  |
-| [avoid-core-mem-forget](https://coinfabrik.github.io/scout/docs/detectors/avoid-core-mem-forget)                 | [The use of core::mem::forget could lead to memory leaks and logic errors](https://coinfabrik.github.io/scout/docs/vulnerabilities/avoid-core-mem-forget)                                                 | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1)                                                                                                                                              | Enhacement  |
-| [avoid-format-string](https://coinfabrik.github.io/scout/docs/detectors/avoid-format-string)                      | [The `format!` macro is not recommended. A custom error is recommended instead.](https://coinfabrik.github.io/scout/docs/vulnerabilities/avoid-format-string)                                            | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/avoid-format-string/avoid-format-string-1)                                                                                                                                                | Enhacement  |
+| [assert-violation](https://coinfabrik.github.io/scout/docs/detectors/assert-violation)                                   | [Avoid the usage of the macro `assert!`, it can panic.](https://coinfabrik.github.io/scout/docs/vulnerabilities/assert-violation)                                                                         | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/assert-violation/assert-violation-1)                                                                                                                                                        | Enhancement  |
+| [avoid-core-mem-forget](https://coinfabrik.github.io/scout/docs/detectors/avoid-core-mem-forget)                 | [The use of core::mem::forget could lead to memory leaks and logic errors](https://coinfabrik.github.io/scout/docs/vulnerabilities/avoid-core-mem-forget)                                                 | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1)                                                                                                                                              | Enhancement  |
+| [avoid-format-string](https://coinfabrik.github.io/scout/docs/detectors/avoid-format-string)                      | [The `format!` macro is not recommended. A custom error is recommended instead.](https://coinfabrik.github.io/scout/docs/vulnerabilities/avoid-format-string)                                            | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/avoid-format-string/avoid-format-string-1)                                                                                                                                                | Enhancement  |
 | [unprotected-self-destruct](https://coinfabrik.github.io/scout/docs/detectors/unprotected-self-destruct)                  | [If users are allowed to call terminate_contract, they can intentionally or accidentally destroy the contract.](https://coinfabrik.github.io/scout/docs/vulnerabilities/unprotected-self-destruct) | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/unprotected-self-destruct/unprotected-self-destruct-1)                                                                                                                                          | Critical  |
-| [iterators-over-indexing](https://coinfabrik.github.io/scout/docs/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.](https://coinfabrik.github.io/scout/docs/vulnerabilities/iterators-over-indexing) | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/iterators-over-indexing/iterators-over-indexing-1)                                                                                                                                          | Enhacement  |
-| [ink-version](https://coinfabrik.github.io/scout/docs/detectors/ink-version)                  | [Using an old version of ink! can be dangerous, as it may have bugs or security issues. Use the latest version available.](https://coinfabrik.github.io/scout/docs/vulnerabilities/ink-version) | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/ink-version/ink-version-1)                                                                                                                                          | Enhacement  |
+| [iterators-over-indexing](https://coinfabrik.github.io/scout/docs/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.](https://coinfabrik.github.io/scout/docs/vulnerabilities/iterators-over-indexing) | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/iterators-over-indexing/iterators-over-indexing-1)                                                                                                                                          | Enhancement  |
+| [ink-version](https://coinfabrik.github.io/scout/docs/detectors/ink-version)                  | [Using an old version of ink! can be dangerous, as it may have bugs or security issues. Use the latest version available.](https://coinfabrik.github.io/scout/docs/vulnerabilities/ink-version) | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/ink-version/ink-version-1)                                                                                                                                          | Enhancement  |
 | [unprotected-set-code-hash](https://coinfabrik.github.io/scout/docs/detectors/unprotected-set-code-hash)                  | [If users are allowed to call terminate_contract, they can intentionally modify the contract behaviour.](https://coinfabrik.github.io/scout/docs/vulnerabilities/unprotected-set-code-hash) | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/set-code-hash/set-code-hash-1)                                                                                                                                          | Critical  |
 | [unprotected-mapping-operation](https://coinfabrik.github.io/scout/docs/detectors/unprotected-mapping-operation)                  | [Modifying mappings with an arbitrary key given by the user could lead to unintented modifications of critical data, modifying data belonging to other users, causing denial of service, unathorized access, and other potential issues.](https://coinfabrik.github.io/scout/docs/vulnerabilities/unprotected-mapping-operation) | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1)                                                                                                                                          | Critical  |
 | [lazy-delegate](https://coinfabrik.github.io/scout/docs/detectors/lazy-delegate)                  | [Delegated calls in ink! need lazy storage.](https://coinfabrik.github.io/scout/docs/vulnerabilities/lazy-delegate) | [1](https://github.com/CoinFabrik/scout/tree/main/test-cases/lazy-delegate/lazy-delegate-1)                                                                                                                                          | Critical  |

diff --git a/detectors/assert-violation/src/lib.rs b/detectors/assert-violation/src/lib.rs
@@ -50,10 +50,10 @@ dylint_linting::impl_pre_expansion_lint! {
     LINT_MESSAGE,
     AssertViolation::default(),
     {
-        name: "Unprotected Mapping Operation",
-        long_message: "Modifying mappings with an arbitrary key given by the user could lead to unintented modifications of critical data, modifying data belonging to other users, causing denial of service, unathorized access, and other potential issues.    ",
-        severity: "Critical",
-        help: "https://coinfabrik.github.io/scout/docs/vulnerabilities/unprotected-mapping-operation",
+        name: "Assert violation",
+        long_message: "The assert! macro can cause the contract to panic. This is not a good practice.",
+        severity: "Enhancement",
+        help: "https://coinfabrik.github.io/scout/docs/vulnerabilities/assert-violation",
         vulnerability_class: "Validations and error handling",
     }
 }

diff --git a/detectors/incorrect-exponentiation/src/lib.rs b/detectors/incorrect-exponentiation/src/lib.rs
@@ -13,7 +13,7 @@ use rustc_lint::{LateContext, LateLintPass};
 use rustc_span::Span;
 use scout_audit_clippy_utils::diagnostics::span_lint_and_help;
 
-const LINT_MESSAGE: &str = "'^' It is not an exponential operator. It is a bitwise XOR one.";
+const LINT_MESSAGE: &str = "'^' It is not an exponential operator. It is a bitwise XOR.";
 const LINT_HELP: &str = "If you want to use XOR, use bitxor(). If you want to raise a number use .checked_pow() or .pow() ";
 
 dylint_linting::declare_late_lint! {
@@ -24,7 +24,7 @@ dylint_linting::declare_late_lint! {
         name: "Incorrect Exponentiation",
         long_message: LINT_MESSAGE,
         severity: "Critical",
-        help: "https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/",
+        help: "https://coinfabrik.github.io/scout/docs/vulnerabilities/incorrect-exponentiation",
         vulnerability_class: "Arithmetic",
     }
 

diff --git a/detectors/zero-or-test-address/README.md b/detectors/zero-or-test-address/README.md
diff --git a/docs/docs/detectors/11-delegate-call.md b/docs/docs/detectors/11-delegate-call.md
@@ -1,10 +1,10 @@
 # Delegate call
 
 ### What it does
-Checks for delegated calls to contracts passed as arguments.
+Checks for delegate calls to contracts passed as arguments.
 
 ### Why is this bad?
-Delegated calls to contracts passed as arguments can be used to change the expected behavior of the contract. If you need to change the target of a delegated call, you should use a storage variable, and make a function with proper access control to change it.
+Delegate calls to contracts passed as arguments can be used to change the expected behavior of the contract. If you need to change the target of a delegate call, you should use a storage variable, and make a function with proper access control to change it.
 
 ### Example
 

diff --git a/docs/docs/detectors/14-unrestricted-transfer-from.md b/docs/docs/detectors/14-unrestricted-transfer-from.md
@@ -10,7 +10,7 @@ An user Alice can approve a contract to spend their tokens. An user Bob can call
 
 ### Known problems
 
-None.
+Could generate false positives when using [Cardinal Cryptography's PSP22](https://github.com/Cardinal-Cryptography/PSP22).
 
 ### Example
 
@@ -48,6 +48,7 @@ Use instead:
 
 ```rust
 // build_call example
+    #[ink(message)]
     pub fn deposit(&mut self) -> Result<(), Error> {
         let call_params = build_call::<DefaultEnvironment>()
             .exec_input(
@@ -62,6 +63,7 @@ Use instead:
     }
 
 // ContractRef example
+    #[ink(message)]
     pub fn deposit(&mut self) -> Result<(), Error> {
         let res = PSP22Ref::transfer_from(
             &self.psp22_address,

diff --git a/docs/docs/detectors/17-avoid-format-string.md b/docs/docs/detectors/17-avoid-format-string.md
@@ -6,7 +6,7 @@ Checks for `format!` macro usage.
 
 ### Why is this bad?
 
-The usage of format! is not recommended.
+The usage of `format!` is not recommended.
 
 ### Example
 
@@ -35,4 +35,4 @@ Use instead:
 
 ### Implementation
 
-The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout/tree/main/detectors/avoid-format!-string).
+The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout/tree/main/detectors/avoid-format-string).
diff --git a/docs/docs/detectors/18-unprotected-self-destruct.md b/docs/docs/detectors/18-unprotected-self-destruct.md
@@ -2,19 +2,14 @@
 
 ### What it does
 
-It warns you if `terminate_contract` function is called without a previous check of the address of the caller.
+It warns you if `terminate_contract` function is called without previously checking the address of the caller.
 
 ### Why is this bad?
 
 If users are allowed to call `terminate_contract`, they can intentionally or accidentally destroy the contract, leading to the loss of all associated data and functionalities given by this contract or by others that depend on it.
 
-### Known problems
-
-None.
-
 ### Example
 
-
 ```rust
     #[ink(message)]
     pub fn delete_contract(&mut self, beneficiary: AccountId) {

diff --git a/docs/docs/detectors/19-iterators-over-indexing.md b/docs/docs/detectors/19-iterators-over-indexing.md
@@ -2,7 +2,7 @@
 
 ### What it does
 
-It warns if for loop uses indexing instead of iterator. If the indexing goes to `.len()` it will not warn.
+It warns if a `for` loop uses indexing instead of an iterator. If the indexing goes to `.len()` it will not warn.
 
 ### Why is this bad?
 

diff --git a/docs/docs/detectors/2-set-contract-storage.md b/docs/docs/detectors/2-set-contract-storage.md
@@ -2,15 +2,15 @@
 
 ### What it does
 
-Checks for calls to env::set_contract_storage.
+Checks for calls to `env::set_contract_storage`.
 
 ### Why is this bad?
 
-Functions using keys as variables without proper access control or input sanitation can allow users to perform changes in arbitrary memory locations.
+Functions using keys as variables without proper access control or input sanitization can allow users to perform changes in arbitrary memory locations.
 
 ### Known problems
 
-Only check the function call, so false positives could result.
+Only checks the function call, so false positives could result.
 
 ### Example
 

diff --git a/docs/docs/detectors/20-ink-version.md b/docs/docs/detectors/20-ink-version.md
@@ -2,11 +2,11 @@
 
 ### What it does
 
-Warns you if you are using an old version of ink!.
+Warns you if you are using an old version of `ink!`.
 
 ### Why is this bad?
 
-Using an old version of ink! can be dangerous, as it may have bugs or security issues.
+Using an old version of `ink!` can be dangerous, as it may have bugs or security issues.
 
 ### Example
 

diff --git a/docs/docs/detectors/21-unprotected-set-code-hash.md b/docs/docs/detectors/21-unprotected-set-code-hash.md
@@ -2,19 +2,14 @@
 
 ### What it does
 
-It warns you if `set_code_hash` function is called without a previous check of the address of the caller.
+It warns you if `set_code_hash` function is called without previously checking the address of the caller.
 
 ### Why is this bad?
 
-If users are allowed to call `terminate_contract`, 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.
-
-### Known problems
-
-None.
+If users are allowed to call `set_code_hash`, 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.
 
 ### Example
 
-
 ```rust
     #[ink(message)]
     pub fn update_code(&self, value: [u8; 32]) -> Result<(), Error> {

diff --git a/docs/docs/detectors/22-unprotected-mapping-operation.md b/docs/docs/detectors/22-unprotected-mapping-operation.md
@@ -8,14 +8,12 @@ It warns you if a mapping operation (`insert`, `take`, `remove`) function is cal
 
 Modifying mappings with an arbitrary key given by users can be a significant vulnerability for several reasons:
 
-- Unintended Modifications: Allowing users to provide arbitrary keys can lead to unintended modifications of critical data within the smart contract. If the input validation and sanitation are not done properly, users may be able to manipulate the data in ways that were not intended by the contract's author.
+- Unintended Modifications: Allowing users to provide arbitrary keys can lead to unintended modifications of critical data within the smart contract. If the input validation and sanitization are not done properly, users may be able to manipulate the data in ways that were not intended by the contract's author.
 
 - Data Corruption: Malicious users could intentionally provide keys that result in the corruption or manipulation of important data stored in the mapping. This could lead to incorrect calculations, unauthorized access, or other undesirable outcomes.
 
 - Denial-of-Service (DoS) Attacks: If users can set arbitrary keys, they may be able to create mappings with a large number of entries, potentially causing the contract to exceed its gas limit. This could lead to denial-of-service attacks, making the contract unusable for other users.
 
-### Known problems
-
 ### Example
 
 ```rust

diff --git a/docs/docs/detectors/23-lazy-delegate.md b/docs/docs/detectors/23-lazy-delegate.md
@@ -6,7 +6,7 @@ Checks for non-lazy storage when using delegate calls.
 
 ### Why is this bad?
 
-ink! has a bug that makes delegated calls not modify the storage of the caller.
+`ink!` has a bug that prevents delegate calls from modifying the storage of the caller.
 
 #### More info
 

diff --git a/docs/docs/detectors/24-incorrect-exponentiation.md b/docs/docs/detectors/24-incorrect-exponentiation.md
@@ -0,0 +1,34 @@
+# Incorrect Exponentiation
+
+### What it does
+
+Warns about `^` being a `bit XOR` operation instead of an exponentiation.  
+
+### Why is this bad?
+
+It can introduce unexpected behaviour in the smart contract.
+
+#### More info
+
+- https://doc.rust-lang.org/std/ops/trait.BitXor.html#tymethod.bitxor
+
+### Example
+
+```rust
+    #[ink(message)]
+    pub fn exp_data_by_3(&mut self) {
+        self.data ^= 3
+    }
+```
+Use instead:
+
+```rust
+    #[ink(message)]
+    pub fn exp_data_by_3(&mut self) {
+        self.data = self.data.pow(3)
+    }
+```
+
+### Implementation
+
+The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout/tree/main/detectors/incorrect-exponentiation).