diff --git a/README.md b/README.md index 8a535c85..10931911 100644 --- 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 index d3d07131..2a593d72 100644 --- 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 index 04d639a2..dbd2c5ae 100644 --- 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 deleted file mode 100644 index 269c7b70..00000000 --- a/detectors/zero-or-test-address/README.md +++ /dev/null @@ -1,17 +0,0 @@ -# template - -### What it does - -### Why is this bad? - -### Known problems -Remove if none. - -### Example -```rust -// example code where a warning is issued -``` -Use instead: -```rust -// example code that does not raise a warning -``` diff --git a/docs/docs/detectors/11-delegate-call.md b/docs/docs/detectors/11-delegate-call.md index c6cee502..dcbae375 100644 --- 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 index 045c4538..79f07c32 100644 --- 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::() .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 index d84c9c98..41b18148 100644 --- 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 index 428b17ad..79d04c81 100644 --- 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 index cecdbc96..2434d5f4 100644 --- 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 index 9c501238..1c240fcc 100644 --- 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 index 3c2ccd30..4a50757e 100644 --- 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 index 27fe6204..0831fa5e 100644 --- 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 index f8fcb1cf..f2999efc 100644 --- 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 index f274c231..d3e49018 100644 --- 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 new file mode 100644 index 00000000..54031779 --- /dev/null +++ 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). diff --git a/docs/docs/detectors/25-buffering-unsized-types.md b/docs/docs/detectors/25-buffering-unsized-types.md new file mode 100644 index 00000000..0f09b3b8 --- /dev/null +++ b/docs/docs/detectors/25-buffering-unsized-types.md @@ -0,0 +1,41 @@ +# Buffering Unsized Types + +### What it does + +It checks for the correct use of fallible methods when reading/writing data into `StorageVec` type variables. + +### Why is this bad? + +`StorageVec` is a Lazy type. Hence the static buffer to store the encoded data is of limited size. Because of that, reading/writing methods can fail and trap the contract. + +### Example + +```rust + #[ink(message)] + pub fn do_something(&mut self, data: String) { + let caller = self.env().caller(); + + let example = format!("{caller:?}: {data}"); + + // Panics if data overgrows the static buffer size! + self.on_chain_log.insert(caller, &example); + } +``` + +Use instead: + +```rust + #[ink(message)] + pub fn do_something2(&mut self, data: String) -> Result<(), Error> { + let caller = self.env().caller(); + + match self.on_chain_log.try_insert(caller, &data) { + Ok(_) => Ok(()), + Err(_) => Err(Error::InsertFailed), + } + } +``` + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout/blob/main/detectors/buffering-unsized-types). diff --git a/docs/docs/detectors/26-avoid-unsafe-block.md b/docs/docs/detectors/26-avoid-unsafe-block.md new file mode 100644 index 00000000..06e9f10b --- /dev/null +++ b/docs/docs/detectors/26-avoid-unsafe-block.md @@ -0,0 +1,52 @@ +# Avoid Unsafe Block + +### What it does + +It tells not to use `Unsafe Rust`. + +### Why is this bad? + +`Unsafe Rust` does not enforce memory safety guarantees at compile time. + + +### Example + +```rust + #[ink(message)] + pub fn unsafe_function(&mut self, n: u64) -> u64 { + unsafe { + let mut i = n as f64; + let mut y = i.to_bits(); + y = 0x5fe6ec85e7de30da - (y >> 1); + i = f64::from_bits(y); + i *= 1.5 - 0.5 * n as f64 * i * i; + i *= 1.5 - 0.5 * n as f64 * i * i; + + let result_ptr: *mut f64 = &mut i; + + (*result_ptr).to_bits() + } + } +``` + +Use instead: + +```rust + #[ink(message)] + pub fn safe_function(&mut self, n: u64) -> u64 { + let mut i = n as f64; + let mut y = i.to_bits(); + y = 0x5fe6ec85e7de30da - (y >> 1); + i = f64::from_bits(y); + i *= 1.5 - 0.5 * n as f64 * i * i; + i *= 1.5 - 0.5 * n as f64 * i * i; + + let result = &mut i; + + result.to_bits() + } +``` + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout/tree/main/detectors/avoid-unsafe-block). diff --git a/docs/docs/detectors/27-warning-sr25519-verify.md b/docs/docs/detectors/27-warning-sr25519-verify.md new file mode 100644 index 00000000..61895c84 --- /dev/null +++ b/docs/docs/detectors/27-warning-sr25519-verify.md @@ -0,0 +1,40 @@ +# Warning sr25519_verify + +### What it does + +Warns about `sr25519_verify()` usage. + +### Why is this bad? + +This function is not available on production chains. + +#### More info + +- https://docs.rs/ink_env/5.0.0/ink_env/fn.sr25519_verify.html + +### Example + +```rust + #[ink(message)] + pub fn example(&self) -> bool { + let signature: [u8; 64] = [ + 184, 49, 74, 238, 78, 165, 102, 252, 22, 92, 156, 176, 124, 118, 168, 116, 247, 99, + 0, 94, 2, 45, 9, 170, 73, 222, 182, 74, 60, 32, 75, 64, 98, 174, 69, 55, 83, 85, + 180, 98, 208, 75, 231, 57, 205, 62, 4, 105, 26, 136, 172, 17, 123, 99, 90, 255, + 228, 54, 115, 63, 30, 207, 205, 131, + ]; + let message: &[u8; 11] = b"hello world"; + let pub_key: [u8; 32] = [ + 212, 53, 147, 199, 21, 253, 211, 28, 97, 20, 26, 189, 4, 169, 159, 214, 130, 44, + 133, 88, 133, 76, 205, 227, 154, 86, 132, 231, 165, 109, 162, 125, + ]; + + ink::env::sr25519_verify(&signature, message.as_slice(), &pub_key).is_ok() + } +``` + +Do not use it. + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout/tree/main/detectors/warning-sr25519-verify). \ No newline at end of file diff --git a/docs/docs/detectors/29-avoid-autokey-upgradable.md b/docs/docs/detectors/29-avoid-autokey-upgradable.md new file mode 100644 index 00000000..6b6c3d52 --- /dev/null +++ b/docs/docs/detectors/29-avoid-autokey-upgradable.md @@ -0,0 +1,17 @@ +# Avoid AutoKey Upgradable + +### What it does + +Warns about the usage of `Lazy` storage (`Mapping`, `Lazy` and `StorageVec`) without a `ManualKey<...>` when the function `set_code_hash` is used in the contract. + +### Why is this bad? + +If the hash passed to `set_code_hash` corresponds to a contract that the compiler assigned `AutoKey<>` to the lazy values, the data in the old keys will be lost. This could lead not only to data loss, but to a locked contract too. + +### More info + +- https://use.ink/datastructures/storage-layout/#manual-vs-automatic-key-generation + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout/tree/main/detectors/avoid-autokey-upgradable). diff --git a/docs/docs/detectors/3-reentrancy.md b/docs/docs/detectors/3-reentrancy.md index 2a2c7799..9aac9139 100644 --- a/docs/docs/detectors/3-reentrancy.md +++ b/docs/docs/detectors/3-reentrancy.md @@ -2,7 +2,7 @@ ### What it does -This linting rule checks whether the 'check-effect' interaction pattern has been properly followed by code that invokes a contract that may call back the original one. +This linting rule checks whether the 'check-effects-interaction' pattern has been properly followed by any code that invokes a contract that may call back to the original one. ### Why is this bad? @@ -11,7 +11,7 @@ If state modifications are made after a contract call, reentrant calls may not d ### Known problems If called method does not perform a malicious reentrancy (i.e. known method from known contract) false positives will arise. -If the usage of set_allow_reentry(true) or later state changes are performed in an auxiliary function, this detector will not detect the reentrancy. +If the usage of `set_allow_reentry(true)` or later state changes are performed in an auxiliary function, this detector will not detect the reentrancy. ### Example diff --git a/docs/docs/detectors/30-non-payable-transferred-value.md b/docs/docs/detectors/30-non-payable-transferred-value.md new file mode 100644 index 00000000..b439bf50 --- /dev/null +++ b/docs/docs/detectors/30-non-payable-transferred-value.md @@ -0,0 +1,18 @@ +# Non payable transferred value + +### What it does + +Warns about the usage of `self.env().transferred_value()` in non-`payable` functions. + +### Why is this bad? + +`self.env().transferred_value()` will always return `0` in non-`payable` functions. If `transferred_value()` is needed, the function should have `#[ink(..., payable)]` + +#### More info + +- https://docs.rs/ink/latest/ink/struct.EnvAccess.html#method.transferred_value + + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout/tree/main/detectors/non-payable-transferred-value). diff --git a/docs/docs/detectors/31-vec-could-be-mapping.md b/docs/docs/detectors/31-vec-could-be-mapping.md new file mode 100644 index 00000000..3807583b --- /dev/null +++ b/docs/docs/detectors/31-vec-could-be-mapping.md @@ -0,0 +1,17 @@ +# Vec could be mapping + +### What it does + +Warns about the usage of the `.find()` method in vector of tuples of the storage. This can be replaced by a `Mapping` + +### Why is this bad? + +Iterating over a vector is more expensive if you are trying to find something in a vector than if you are using a `Mapping`. + +#### More info + +- https://docs.rs/ink/latest/ink/storage/struct.Mapping.html + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout/tree/main/detectors/vec-could-be-mapping). diff --git a/docs/docs/detectors/32-dont-use-invoke-contract-v1.md b/docs/docs/detectors/32-dont-use-invoke-contract-v1.md new file mode 100644 index 00000000..233769aa --- /dev/null +++ b/docs/docs/detectors/32-dont-use-invoke-contract-v1.md @@ -0,0 +1,17 @@ +# Don't use invoke_contract_v1 + +### What it does + +Checks that method `invoke_contract_v1` is not used in the contract. + +### Why is this bad? + +This will call into the original version of the host function. It is recommended to use `invoke_contract` to use the latest version if the target runtime supports it. + +#### More info + +- https://docs.rs/ink_env/5.0.0/ink_env/fn.invoke_contract_v1.html + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout/tree/main/detectors/dont-use-invoke-contract-v1). diff --git a/docs/docs/detectors/4-panic-error.md b/docs/docs/detectors/4-panic-error.md index 6c2ff7f6..9fe82aef 100644 --- a/docs/docs/detectors/4-panic-error.md +++ b/docs/docs/detectors/4-panic-error.md @@ -2,16 +2,16 @@ ### What it does -The panic! macro is used to stop execution when a condition is not met. -This is useful for testing and prototyping, but should be avoided in production code +The `panic!` macro is used to stop execution when a condition is not met. +This is useful for testing and prototyping, but should be avoided in production code. ### Why is this bad? -The usage of panic! is not recommended because it will stop the execution of the caller contract. +The usage of `panic!` is not recommended because it will stop the execution of the caller contract. ### Known problems -While this linter detects explicit calls to panic!, there are some ways to raise a panic such as unwrap() or expect(). +While this linter detects explicit calls to `panic!`, there are some ways to raise a panic such as `unwrap()` or `expect()`. ### Example @@ -25,7 +25,7 @@ pub fn add(&mut self, value: u32) { } ``` -// example code that does not raise a warning +Here is an example of code that does not raise a warning: ```rust #[derive(Debug, PartialEq, Eq, scale::Encode, scale::Decode)] diff --git a/docs/docs/detectors/5-unused-return-enum.md b/docs/docs/detectors/5-unused-return-enum.md index 62043f8e..9fe38a52 100644 --- a/docs/docs/detectors/5-unused-return-enum.md +++ b/docs/docs/detectors/5-unused-return-enum.md @@ -10,7 +10,7 @@ If any of the variants (Ok/Err) is not used, the code could be simplified or it ### Known problems -If definitions of Err() and/or Ok() are in the code but do not flow to the return value due to the definition of a variable or because they are defined in a dead code block, the warning will not be shown. If the definitions are made in an auxiliary method, the warning will be shown, resulting in a false positive. +If definitions of `Err()` and/or `Ok()` are in the code but do not flow to the return value due to the definition of a variable or because they are defined in a dead code block, the warning will not be shown. If the definitions are made in an auxiliary method, the warning will be shown, resulting in a false positive. ### Example diff --git a/docs/docs/detectors/6-dos-unbounded-operation.md b/docs/docs/detectors/6-dos-unbounded-operation.md index 53df63eb..37a18e15 100644 --- a/docs/docs/detectors/6-dos-unbounded-operation.md +++ b/docs/docs/detectors/6-dos-unbounded-operation.md @@ -2,7 +2,7 @@ ### What it does -This detector checks that when using for or while loops, their conditions limit the execution to a constant number of iterations. +This detector checks that when using `for` or `while` loops, their conditions limit the execution to a constant number of iterations. ### Why is this bad? diff --git a/docs/docs/vulnerabilities/1-integer-overflow-or-underflow.md b/docs/docs/vulnerabilities/1-integer-overflow-or-underflow.md index 92e529a1..2ec1c783 100644 --- a/docs/docs/vulnerabilities/1-integer-overflow-or-underflow.md +++ b/docs/docs/vulnerabilities/1-integer-overflow-or-underflow.md @@ -37,28 +37,7 @@ if a user adds a value that exceeds the maximum value that can be stored in an `u8` variable, then the addition operation overflows the variable and the value wraps to zero (ignoring the carry), potentially leading to unexpected behavior. -This vulnerability is effectively realized if overflow and underflow checks are -disabled at the time of compilation. This can be done by modifying the -`Cargo.toml` file with the following configuration: - -```toml -[profile.release] -overflow-checks = false -``` - -This way, the overflow checks will be disabled whenever the contract is built -using the `release` profile. More info can be found -[here](https://doc.rust-lang.org/cargo/reference/profiles.html). - -Please note that if the check is enabled, the vulnerability will not be exploitable, but a panic error will be raised. Raising a panic error is not the recommended way to handle this type of issue. In the Remediation section below, we explain a better approach to address it. - -To deploy this smart contract, you would need to compile it using the `ink!` -compiler and deploy it to a Polkadot Substrate network using a suitable -deployment tool such as Polkadot JS. Once deployed, users could interact with -the contract by calling its functions using a compatible wallet or blockchain -explorer. - -The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example/lib.rs). +The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example/src/lib.rs). ### Deployment Before deployment, the contract must be built using the tool `cargo-contract`: @@ -76,14 +55,15 @@ cargo contract instantiate --constructor new --args 0 --suri //Alice ## Remediation -Even though enabling the overflow/underflow checks in the `Cargo.toml` would eliminate the possibility of the + +It is recommended that the code be changed to explicitly use checked, overflowing, or saturating arithmetic. For example: ```rust #[derive(Debug, PartialEq, Eq, scale::Encode, scale::Decode)] @@ -118,13 +98,13 @@ pub fn sub(&mut self, value: u8) -> Result<(), Error> { } ``` -The remediated code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example/lib.rs). +The remediated code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example/src/lib.rs). Other rules could be added to improve the checking. The set of rules can be found [here](https://rust-lang.github.io/rust-clippy/master/). ## References - [SWC-101](https://swcregistry.io/docs/SWC-101) -- [Ethernaut: Token](https://ethernaut.openzeppelin.com/level/0x63bE8347A617476CA461649897238A31835a32CE) +- [Ethernaut: Token](https://github.com/OpenZeppelin/ethernaut/blob/master/contracts/src/levels/Token.sol) - [20 cases of overflow/underflow](https://github.com/ethereum/solidity/issues/796#issuecomment-253578925) - https://blog.sigmaprime.io/solidity-security.html#ouflow \ No newline at end of file diff --git a/docs/docs/vulnerabilities/11-delegate-call.md b/docs/docs/vulnerabilities/11-delegate-call.md index fb89be33..cd886e48 100644 --- a/docs/docs/vulnerabilities/11-delegate-call.md +++ b/docs/docs/vulnerabilities/11-delegate-call.md @@ -70,5 +70,5 @@ The remediated code example can be found [`here`](https://github.com/CoinFabrik/ - https://solidity-by-example.org/hacks/delegatecall/ - https://blog.sigmaprime.io/solidity-security.html#delegatecall - [SWC-112](https://swcregistry.io/docs/SWC-112) -- [Ethernaut: Delegation](https://ethernaut.openzeppelin.com/level/0x9451961b7Aea1Df57bc20CC68D72f662241b5493) +- [Ethernaut: Delegation](https://github.com/OpenZeppelin/ethernaut/blob/master/contracts/src/levels/Delegation.sol) - [Slither: Delegatecall](https://github.com/crytic/slither/wiki/Detector-Documentation#controlled-delegatecall) \ No newline at end of file diff --git a/docs/docs/vulnerabilities/12-zero-or-test-address.md b/docs/docs/vulnerabilities/12-zero-or-test-address.md index 506007ce..5ad19486 100644 --- a/docs/docs/vulnerabilities/12-zero-or-test-address.md +++ b/docs/docs/vulnerabilities/12-zero-or-test-address.md @@ -33,7 +33,7 @@ The vulnerable code example can be found [`here`](https://github.com/CoinFabrik/ ## Remediation -To remediate this problem, verify in your code whether the `admin` provided is the zero address and return an Error if this is the case. +To remediate this problem, verify in your code whether the `admin` provided is the zero address and return an `Error` if this is the case. ```rust #[ink(message)] diff --git a/docs/docs/vulnerabilities/13-insufficiently-random-values.md b/docs/docs/vulnerabilities/13-insufficiently-random-values.md index c1c8c187..e247082f 100644 --- a/docs/docs/vulnerabilities/13-insufficiently-random-values.md +++ b/docs/docs/vulnerabilities/13-insufficiently-random-values.md @@ -48,6 +48,6 @@ Avoid using block attributes like `block_timestamp` or `block_number` for random - https://blog.sigmaprime.io/solidity-security.html#SP-6 - [SWC-120](https://swcregistry.io/docs/SWC-120) - [SWC-116](https://swcregistry.io/docs/SWC-116) -- [Ethernaut: Coinflip](https://ethernaut.openzeppelin.com/level/0x4dF32584890A0026e56f7535d0f2C6486753624f) +- [Ethernaut: Coinflip](https://github.com/OpenZeppelin/ethernaut/blob/master/contracts/src/levels/CoinFlip.sol) - [Slither: Weak PRNG](https://github.com/crytic/slither/wiki/Detector-Documentation#weak-PRNG) - [Slither: Dangerous usage of block.timestamp](https://github.com/crytic/slither/wiki/Detector-Documentation#block-timestamp) \ No newline at end of file diff --git a/docs/docs/vulnerabilities/15-assert-violation.md b/docs/docs/vulnerabilities/15-assert-violation.md index d71c8d96..cefaa29a 100644 --- a/docs/docs/vulnerabilities/15-assert-violation.md +++ b/docs/docs/vulnerabilities/15-assert-violation.md @@ -3,7 +3,7 @@ ## Description - Vulnerability Category: `Validations and error handling` -- Vulnerability Severity: `Enhacement` +- Vulnerability Severity: `Enhancement` - Detectors: [`assert-violation`](https://github.com/CoinFabrik/scout/tree/main/detectors/assert-violation) - Test Cases: [`assert-violation-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/assert-violation/assert-violation-1) diff --git a/docs/docs/vulnerabilities/16-avoid-core-mem-forget.md b/docs/docs/vulnerabilities/16-avoid-core-mem-forget.md index 6445df85..eceefee9 100644 --- a/docs/docs/vulnerabilities/16-avoid-core-mem-forget.md +++ b/docs/docs/vulnerabilities/16-avoid-core-mem-forget.md @@ -3,7 +3,7 @@ ## Description - Vulnerability Category: `Best practices` -- Vulnerability Severity: `Enhacement` +- Vulnerability Severity: `Enhancement` - Detectors: [`avoid-core-mem-forget`](https://github.com/CoinFabrik/scout/tree/main/detectors/avoid-core-mem-forget) - Test Cases: [`avoid-core-mem-forget-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/avoid-core-mem-forget/avoid-core-mem-forget-1) diff --git a/docs/docs/vulnerabilities/17-avoid-format-string.md b/docs/docs/vulnerabilities/17-avoid-format-string.md index 396aa01d..340a3d49 100644 --- a/docs/docs/vulnerabilities/17-avoid-format-string.md +++ b/docs/docs/vulnerabilities/17-avoid-format-string.md @@ -3,9 +3,9 @@ ## Description - Vulnerability Category: `Validations and error handling` -- Vulnerability Severity: `Enhacement` -- Detectors: [`avoid-format!-string`](https://github.com/CoinFabrik/scout/tree/main/detectors/avoid-format!-string) -- Test Cases: [`avoid-format!-string-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/avoid-format!-string/avoid-format!-string-1) +- Vulnerability Severity: `Enhancement` +- Detectors: [`avoid-format!-string`](https://github.com/CoinFabrik/scout/tree/main/detectors/avoid-format-string) +- Test Cases: [`avoid-format!-string-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/avoid-format-string/avoid-format-string-1) The `format!` macro is not recommended. A custom error is recommended instead. @@ -25,7 +25,7 @@ Consider the following `ink!` contract: The problem arises from the use of the `format!` macro. This is used to format a string with the given arguments. Returning a custom error is desirable. -The vulnerable code example can be found [`here`](https://github.com/CoinFabrik/scout/tree/main/test-cases/avoid-format!-string/avoid-format!-string-1/vulnerable-example). +The vulnerable code example can be found [`here`](https://github.com/CoinFabrik/scout/tree/main/test-cases/avoid-format-string/avoid-format-string-1/vulnerable-example). ## Remediation diff --git a/docs/docs/vulnerabilities/18-unprotected-self-destruct.md b/docs/docs/vulnerabilities/18-unprotected-self-destruct.md index 919e6322..279dbe63 100644 --- a/docs/docs/vulnerabilities/18-unprotected-self-destruct.md +++ b/docs/docs/vulnerabilities/18-unprotected-self-destruct.md @@ -19,9 +19,6 @@ Allowing users to call `terminate_contract` can be a significant vulnerability d Consider the following `ink!` contract: -### Example - - ```rust #[ink(message)] pub fn delete_contract(&mut self, beneficiary: AccountId) { diff --git a/docs/docs/vulnerabilities/19-iterators-over-indexing.md b/docs/docs/vulnerabilities/19-iterators-over-indexing.md index 2eb23317..8e49782e 100644 --- a/docs/docs/vulnerabilities/19-iterators-over-indexing.md +++ b/docs/docs/vulnerabilities/19-iterators-over-indexing.md @@ -3,7 +3,7 @@ ## Description - Vulnerability Category: `Best practices` -- Vulnerability Severity: `Enhacement` +- Vulnerability Severity: `Enhancement` - Detectors: [`iterators-over-indexing`](https://github.com/CoinFabrik/scout/tree/main/detectors/iterators-over-indexing) - Test Cases: [`iterators-over-indexing-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/iterators-over-indexing/iterators-over-indexing-1) @@ -34,4 +34,4 @@ The remediated code example can be found [`here`](https://github.com/CoinFabrik/ ## References -- [Memory management](https://docs.alephzero.org/aleph-zero/security-course-by-kudelski-security/ink-developers-security-guideline#memory-management) +- [Aleph Zero ink! Developer security guidelines](https://docs.alephzero.org/aleph-zero/security-course-by-kudelski-security/ink-developers-security-guideline#memory-management) diff --git a/docs/docs/vulnerabilities/2-set-contract-storage.md b/docs/docs/vulnerabilities/2-set-contract-storage.md index a6af7ed3..7d2ddeb1 100644 --- a/docs/docs/vulnerabilities/2-set-contract-storage.md +++ b/docs/docs/vulnerabilities/2-set-contract-storage.md @@ -38,14 +38,14 @@ impl MisusedSetContractStorage for Erc20 { } ``` -The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/set-contract-storage/set-contract-storage-1/vulnerable-example/lib.rs). +The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/set-contract-storage/set-contract-storage-1/vulnerable-example/src/lib.rs). ### Deployment To compile this example, `cargo-contract` v2.0.1 (or above) is required. In order to run this exploit, [download](https://github.com/paritytech/substrate-contracts-node/releases), unzip and run a substrate node with `./substrate-contract-node`. Download the contents of the `example` folder associated to this detector and compile the contract running `cargo contract build` and build the binary. -Afterwards, upload the the binary into the running network with the account `Alice` using `cargo contract upload --suri //Alice ./target/ink/my_contract.contract`. +Afterwards, upload the binary into the running network with the account `Alice` using `cargo contract upload --suri //Alice ./target/ink/my_contract.contract`. ```bash @@ -261,7 +261,7 @@ fn misused_set_contract_storage(&mut self, user_input_key: [u8; 68], user_input_ } ``` -The remediated code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/set-contract-storage/set-contract-storage-1/remediated-example/lib.rs). +The remediated code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/set-contract-storage/set-contract-storage-1/remediated-example/src/lib.rs). ## References * https://use.ink/datastructures/storage-layout diff --git a/docs/docs/vulnerabilities/20-ink-version.md b/docs/docs/vulnerabilities/20-ink-version.md index 861d1f35..1f3bf19a 100644 --- a/docs/docs/vulnerabilities/20-ink-version.md +++ b/docs/docs/vulnerabilities/20-ink-version.md @@ -3,7 +3,7 @@ ## Description - Vulnerability Category: `Best practices` -- Vulnerability Severity: `Enhacement` +- Vulnerability Severity: `Enhancement` - Detectors: [`ink-version`](https://github.com/CoinFabrik/scout/tree/main/detectors/ink-version) - Test Cases: [`ink-version-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/ink-version/ink-version-1) diff --git a/docs/docs/vulnerabilities/21-unprotected-set-code-hash.md b/docs/docs/vulnerabilities/21-unprotected-set-code-hash.md index 3e5a2aa3..84271b62 100644 --- a/docs/docs/vulnerabilities/21-unprotected-set-code-hash.md +++ b/docs/docs/vulnerabilities/21-unprotected-set-code-hash.md @@ -56,7 +56,6 @@ To prevent this, the function should be restricted to administrators or authoriz } ``` - ## References - [Slither: Unprotected upgradeable contract](https://github.com/crytic/slither/wiki/Detector-Documentation#unprotected-upgradeable-contract) \ No newline at end of file diff --git a/docs/docs/vulnerabilities/22-unprotected-mapping-operation.md b/docs/docs/vulnerabilities/22-unprotected-mapping-operation.md index d1363180..43bd4888 100644 --- a/docs/docs/vulnerabilities/22-unprotected-mapping-operation.md +++ b/docs/docs/vulnerabilities/22-unprotected-mapping-operation.md @@ -5,7 +5,7 @@ - Vulnerability Category: `Authorization` - Vulnerability Severity: `Critical` - Detectors: [`unprotected-mapping-operation`](https://github.com/CoinFabrik/scout/tree/main/detectors/unprotected-mapping-operation) -- Test Cases: [`unprotected-mapping-operation-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation1) +- Test Cases: [`unprotected-mapping-operation-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1) Modifying mappings with an arbitrary key given by users can be a significant vulnerability for several reasons: @@ -37,7 +37,7 @@ Consider the following `ink!` contract: The vulnerability in this `withdraw` function arises from the use of `from`, an user-defined parameter used as key in the mapping without prior sanitizing. Alice can withdraw tokens from any user to the user balance. -The vulnerable code example can be found [`here`](https://github.com/CoinFabrik/scout/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation1/vulnerable-example). +The vulnerable code example can be found [`here`](https://github.com/CoinFabrik/scout/tree/main/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/vulnerable-example). ## Remediation Avoid using user-given arguments as `key` parameter in mapping. Instead, use `self.env().caller()` or sanitize the values. diff --git a/docs/docs/vulnerabilities/23-lazy-delegate.md b/docs/docs/vulnerabilities/23-lazy-delegate.md index 55e96772..8eadfc5c 100644 --- a/docs/docs/vulnerabilities/23-lazy-delegate.md +++ b/docs/docs/vulnerabilities/23-lazy-delegate.md @@ -7,7 +7,7 @@ - Detectors: [`lazy-delegate`](https://github.com/CoinFabrik/scout/tree/main/detectors/lazy-delegate) - Test Cases: [`lazy-delegate-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/lazy-delegate/lazy-delegate-1) -ink! has a bug that makes delegated calls not modify the storage of the caller, unless it's using `Lazy` with `ManualKey` or `Mapping`. +`ink!` has a bug that makes delegated calls not modify the storage of the caller, unless it's using `Lazy` with `ManualKey` or `Mapping`. ## Exploit Scenario diff --git a/docs/docs/vulnerabilities/24-incorrect-exponentiation.md b/docs/docs/vulnerabilities/24-incorrect-exponentiation.md new file mode 100644 index 00000000..9cafa21c --- /dev/null +++ b/docs/docs/vulnerabilities/24-incorrect-exponentiation.md @@ -0,0 +1,42 @@ +# Incorrect Exponentiation + +## Description + +- Vulnerability Category: `Arithmetic` +- Vulnerability Severity: `Critical` +- Detectors: [`incorrect-exponentiation`](https://github.com/CoinFabrik/scout/tree/main/detectors/incorrect-exponentiation) +- Test Cases: [`incorrect-exponentiation-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/incorrect-exponentiation/incorrect-exponentiation-1) + + +The operator `^` is not an exponential operator, it is a bitwise XOR. Make sure to use `pow()` instead for exponentiation. In case of performing a XOR operation, use `.bitxor()` for clarity. + +## Exploit Scenario + +In the following example, the `^` operand is being used for exponentiation. But in Rust, `^` is the operand for an XOR operation. If misused, +this could lead to unexpected behaviour in our contract. + +```rust + #[ink(message)] + pub fn exp_data_by_3(&mut self) { + self.data ^= 3 + } +``` + +The vulnerable code example can be found [`here`](https://github.com/CoinFabrik/scout/tree/main/test-cases/incorrect-exponentiation/incorrect-exponentiation-1/vulnerable-example). + +## Remediation + +A possible solution is to use the method `pow()`. But, if a XOR operation is wanted, `.bitxor()` method is recommended. + +```rust + #[ink(message)] + pub fn exp_data_by_3(&mut self) { + self.data = self.data.pow(3) + } +``` + +The remediated code example can be found [`here`](https://github.com/CoinFabrik/scout/tree/main/test-cases/incorrect-exponentiation/incorrect-exponentiation-1/remediated-example). + +## References + +- https://doc.rust-lang.org/std/ops/trait.BitXor.html diff --git a/docs/docs/vulnerabilities/25-buffering-unsized-types.md b/docs/docs/vulnerabilities/25-buffering-unsized-types.md new file mode 100644 index 00000000..612a4e87 --- /dev/null +++ b/docs/docs/vulnerabilities/25-buffering-unsized-types.md @@ -0,0 +1,54 @@ +# Buffering Unsized Types + +## Description + +- Vulnerability Category: `Best practices` +- Vulnerability Severity: `Enhancement` +- Detectors: [`vec-considerations`](https://github.com/CoinFabrik/scout/tree/main/detectors/buffering-unsized-types) +- Test Cases: [`vec-considerations-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/buffering-unsized-types/buffering-unsized-types-1) + + +Avoid using fallible methods like `insert`, `pop`, `push`, `set` or `peek` with an unsized (dynamically sized) type. To prevent the contract for panicking, use `try_` (fallible) storage methods. + +## Exploit Scenario + +Consider the following `ink!` contract: + +```rust + #[ink(message)] + pub fn do_something(&mut self, data: String) { + let caller = self.env().caller(); + let example = format!("{caller:?}: {data}"); + + // Panics if data overgrows the static buffer size! + self.on_chain_log.insert(caller, &example); + } +``` + +The problem arises from the use of `.insert()` since `ink!`'s static buffer defaults to 16KB in size. If data overgrows this size, the contract will `panic!`. + +The vulnerable code example can be found [`here`](https://github.com/CoinFabrik/scout/tree/main/test-cases/buffering-unsized-types/buffering-unsized-types-1/vulnerable-example). + +## Remediation + +Instead, when working with dynamically sized values, use fallible storage methods. For instance: + +```rust + #[ink(message)] + pub fn do_something2(&mut self, data: String) -> Result<(), Error> { + let caller = self.env().caller(); + + match self.on_chain_log.try_insert(caller, &data) { + Ok(_) => Ok(()), + Err(_) => Err(Error::InsertFailed), + } + } +``` + + +The remediated code example can be found [`here`](https://github.com/CoinFabrik/scout/tree/main/test-cases/buffering-unsized-types/buffering-unsized-types-1/remediated-example). + +## References + +- https://docs.rs/ink_storage/5.0.0/ink_storage/struct.StorageVec.html +- https://use.ink/datastructures/storagevec/ diff --git a/docs/docs/vulnerabilities/26-avoid-unsafe-block.md b/docs/docs/vulnerabilities/26-avoid-unsafe-block.md new file mode 100644 index 00000000..7dbb15ac --- /dev/null +++ b/docs/docs/vulnerabilities/26-avoid-unsafe-block.md @@ -0,0 +1,57 @@ +# Avoid Unsafe Block +## Description +- Vulnerability Category: `Best practices` +- Vulnerability Severity: `Enhancement` +- Detectors: [`avoid-unsafe-block`](https://github.com/CoinFabrik/scout/tree/main/detectors/avoid-unsafe-block) +- Test Cases: [`avoid-unsafe-block-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1) + + +The use of `unsafe` blocks in Rust is discouraged because they bypass Rust's memory safety checks, potentially leading to issues like undefined behavior and security vulnerabilities. + +## Exploit Scenario + +Rust enforces memory safety at compile time. When using an `unsafe` block in Rust, it's up to the programmer to take this security measure into acount. However, this could lead to memory issues. For instance, disregarding the borrow checker, or dereferencing a null pointer. + +```rust + #[ink(message)] + pub fn unsafe_function(&mut self, n: u64) -> u64 { + unsafe { + let mut i = n as f64; + let mut y = i.to_bits(); + y = 0x5fe6ec85e7de30da - (y >> 1); + i = f64::from_bits(y); + i *= 1.5 - 0.5 * n as f64 * i * i; + i *= 1.5 - 0.5 * n as f64 * i * i; + + let result_ptr: *mut f64 = &mut i; + + (*result_ptr).to_bits() + } + } +``` + + +## Remediation + +To enforce memory safety, it's recommended not to use `unsafe`. + +```rust + #[ink(message)] + pub fn safe_function(&mut self, n: u64) -> u64 { + let mut i = n as f64; + let mut y = i.to_bits(); + y = 0x5fe6ec85e7de30da - (y >> 1); + i = f64::from_bits(y); + i *= 1.5 - 0.5 * n as f64 * i * i; + i *= 1.5 - 0.5 * n as f64 * i * i; + + let result = &mut i; + result.to_bits() + } +``` + +The remediated code example can be found [`here`](https://github.com/CoinFabrik/scout/blob/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/remediated-example/src/lib.rs). + +## References + +- https://doc.rust-lang.org/book/ch19-01-unsafe-rust.html \ No newline at end of file diff --git a/docs/docs/vulnerabilities/27-warning-sr25519-verify.md b/docs/docs/vulnerabilities/27-warning-sr25519-verify.md new file mode 100644 index 00000000..de6eb5b0 --- /dev/null +++ b/docs/docs/vulnerabilities/27-warning-sr25519-verify.md @@ -0,0 +1,44 @@ +# Warning sr25519_verify + +## Description + +- Vulnerability Category: `Known Bugs` +- Vulnerability Severity: `Medium` +- Detectors: [`warning-sr25519-verify`](https://github.com/CoinFabrik/scout/tree/main/detectors/warning-sr25519-verify) +- Test Cases: [`warning-sr25519-verify-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/warning-sr25519-verify/warning-sr25519-verify-1) + +This function is from the unstable interface, which is unsafe and normally is not available on production chains. + +## Exploit Scenario + +Consider the following `ink!` contract: + +```rust + #[ink(message)] + pub fn example(&self) -> bool { + let signature: [u8; 64] = [ + 184, 49, 74, 238, 78, 165, 102, 252, 22, 92, 156, 176, 124, 118, 168, 116, 247, 99, + 0, 94, 2, 45, 9, 170, 73, 222, 182, 74, 60, 32, 75, 64, 98, 174, 69, 55, 83, 85, + 180, 98, 208, 75, 231, 57, 205, 62, 4, 105, 26, 136, 172, 17, 123, 99, 90, 255, + 228, 54, 115, 63, 30, 207, 205, 131, + ]; + let message: &[u8; 11] = b"hello world"; + let pub_key: [u8; 32] = [ + 212, 53, 147, 199, 21, 253, 211, 28, 97, 20, 26, 189, 4, 169, 159, 214, 130, 44, + 133, 88, 133, 76, 205, 227, 154, 86, 132, 231, 165, 109, 162, 125, + ]; + + ink::env::sr25519_verify(&signature, message.as_slice(), &pub_key).is_ok() + } +``` +`sr25519_verify` is a function not available on production chains. If used, it will cause the contract to fail. + +The vulnerable code example can be found [`here`](https://github.com/CoinFabrik/scout/tree/main/test-cases/warning-sr25519-verify/warning-sr25519-verify-1/vulnerable-example). + +## Remediation + +Because of that do not use the `sr25519_verify` function. + +The remediated code example can be found [`here`](https://github.com/CoinFabrik/scout/tree/main/test-cases/warning-sr25519-verify/warning-sr25519-verify-1/remediated-example). + + diff --git a/docs/docs/vulnerabilities/29-avoid-autokey-upgradable.md b/docs/docs/vulnerabilities/29-avoid-autokey-upgradable.md new file mode 100644 index 00000000..f15c44c3 --- /dev/null +++ b/docs/docs/vulnerabilities/29-avoid-autokey-upgradable.md @@ -0,0 +1,87 @@ +# Avoid autokey upgradable + +## Description + +- Vulnerability Category: `Upgradability` +- Vulnerability Severity:`Critical` +- Detectors: [`avoid-autokey-upgradable`](https://github.com/CoinFabrik/scout/tree/main/detectors/avoid-autokey-upgradable) +- Test Cases: [`avoid-autokey-upgradable-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/avoid-autokey-upgradable/avoid-autokey-upgradable-1) + + + +## Exploit Scenario + +Consider the following contract: + +```rust + #[ink(storage)] + pub struct AvoidAutoKeyUpgradable { + balances: Mapping, + total_supply: Lazy, + } + + pub enum Error { + NotAnAdmin, + FailedSetCodeHash, + } + + impl Contract { + /* --- snip --- */ + #[ink(message)] + pub fn upgrade_contract(&self, value: [u8; 32]) -> Result<(), Error> { + if self.admin != Self::env().caller() { + return Err(Error::NotAnAdmin); + } + + match self.env().set_code_hash(&value.into()) { + Ok(_) => Ok(()), + Err(_) => Err(Error::FailedSetCodeHash), + } + } + /* --- snip --- */ + } + +``` + +When you have a contract that has any kind of `Lazy` storage (`Lazy`, `Mapping` or `StorageVec`) and your contract is upgradable, you need to ensure that every `Lazy` storage has a manual key. If you don't do this, the compiler can assign a new key to the `Lazy` storage when you upgrade the contract, and you will lose all that data. + +## Remediation + +Use `ManualKey` to ensure that the `Lazy` storage has a fixed key. You can use either a literal value or an Enum variant. + +```rust + pub enum Keys { + TotalSupply, + } + + #[ink(storage)] + pub struct AvoidAutoKeyUpgradable { + balances: Mapping>, + total_supply: Lazy>, + } + + pub enum Error { + NotAnAdmin, + FailedSetCodeHash, + } + + impl Contract { + /* --- snip --- */ + #[ink(message)] + pub fn upgrade_contract(&self, value: [u8; 32]) -> Result<(), Error> { + if self.admin != Self::env().caller() { + return Err(Error::NotAnAdmin); + } + + match self.env().set_code_hash(&value.into()) { + Ok(_) => Ok(()), + Err(_) => Err(Error::FailedSetCodeHash), + } + } + /* --- snip --- */ + } +``` + +## References + +- https://use.ink/datastructures/storage-layout diff --git a/docs/docs/vulnerabilities/3-reentrancy.md b/docs/docs/vulnerabilities/3-reentrancy.md index 2d18bf4d..4e189a65 100644 --- a/docs/docs/vulnerabilities/3-reentrancy.md +++ b/docs/docs/vulnerabilities/3-reentrancy.md @@ -3,7 +3,7 @@ * Vulnerability Category: `Reentrancy` * Severity: `Critical` * Detectors: [`reentrancy-1`](https://github.com/CoinFabrik/scout/tree/main/detectors/reentrancy-1), [`reentrancy-2`](https://github.com/CoinFabrik/scout/tree/main/detectors/reentrancy-2) -* Test Cases: [`reentrancy-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1), [`reentrancy-2`](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-2), [`reentrancy-3`](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-2/reentrancy-1) +* Test Cases: [`reentrancy-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1-1), [`reentrancy-2`](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1-2), [`reentrancy-3`](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-2/reentrancy-2-1) Smart contracts can call other contracts and send tokens to them. These operations imply external calls where control flow is passed to the called @@ -110,12 +110,12 @@ pub fn exploit(&mut self) { ``` -The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1/vulnerable-example). +The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1-1/vulnerable-example). ### Deployment -Vault and Exploit files can be found under the directories -[vulnerable-example/exploit](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1/vulnerable-example/exploit) and -[vulnerable-example/vault](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1/vulnerable-example/vault). +Exploit and Vault files can be found under the directories +[vulnerable-example/exploit](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1-1/vulnerable-example/exploit) and +[vulnerable-example/src](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1-1/vulnerable-example/src). The whole exploit example can be run automatically using the `deploy.sh` file. ## Recommendation @@ -123,7 +123,7 @@ In general, risks associated to reentrancy can be addressed with the Check-Effect-Interaction pattern, a best practice that indicates that external calls should be the last thing to be executed in a function. In this example, this can be done by inserting the balance before transferring the value (see -[remediated-example-1](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1/remediated-example)). +[remediated-example-1](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1-1/remediated-example)). ```rust @@ -156,11 +156,11 @@ pub fn call_with_value(&mut self, address: AccountId, amount: Balance, selector: } ``` -The remediated code example can be found [here](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1/remediated-example). +The remediated code example can be found [here](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1-1/remediated-example). Alternatively, if reentrancy by an external contract is not needed, the `set_allow_reentry(true)` should be removed altogether (see -[remediated-example-2](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-2/remediated-example)). This is equivalent in Substrate to using a +[remediated-example-2](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1-2/remediated-example)). This is equivalent in Substrate to using a [reentrancy guard](https://github.com/Supercolony-net/openbrush-contracts/tree/main/contracts/src/security/reentrancy_guard) like the one offered by [OpenBrush](https://github.com/Supercolony-net/openbrush-contracts). @@ -191,7 +191,7 @@ pub fn call_with_value(&mut self, address: AccountId, amount: Balance, selector: } ``` -The remediated code example can be found [here](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-2/remediated-example). +The remediated code example can be found [here](https://github.com/CoinFabrik/scout/tree/main/test-cases/reentrancy-1/reentrancy-1-2/remediated-example). ## References * https://use.ink/datastructures/storage-layout diff --git a/docs/docs/vulnerabilities/30-non-payable-transferred-value.md b/docs/docs/vulnerabilities/30-non-payable-transferred-value.md new file mode 100644 index 00000000..c1a783fe --- /dev/null +++ b/docs/docs/vulnerabilities/30-non-payable-transferred-value.md @@ -0,0 +1,32 @@ +# Non payable transferred value + +## Description + +- Vulnerability Category: `Best practices` +- Vulnerability Severity: `Enhancement` +- Detectors: [`non-payable-transferred-value`](https://github.com/CoinFabrik/scout/tree/main/detectors/non-payable-transferred-value) +- Test Cases: [`non-payable-transferred-value-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/non-payable-transferred-value/non-payable-transferred-value-1) + +## Exploit Scenario + +Consider the following function. + +```rust + #[ink(message)] + pub fn something(&self) -> bool { + self.env().transferred_value() > 0 + } +``` + +This function is not payable as it does not have the `#[ink(payable)]` attribute, but it checks for `self.env().transferred_value()` and it will always evaluate to `0` if the function is not payable. + +## Remediation + +Make the function `payable` if you want to check the transferred value. + +```rust + #[ink(message, payable)] + pub fn something(&self) -> bool { + self.env().transferred_value() > 0 + } +``` diff --git a/docs/docs/vulnerabilities/31-vec-could-be-mapping.md b/docs/docs/vulnerabilities/31-vec-could-be-mapping.md new file mode 100644 index 00000000..f8744092 --- /dev/null +++ b/docs/docs/vulnerabilities/31-vec-could-be-mapping.md @@ -0,0 +1,63 @@ +# Vec could be mapping + +## Description + +- Vulnerability Category: `Gas usage` +- Vulnerability Severity: `Enhancement` +- Detectors: [`vec-could-be-mapping`](https://github.com/CoinFabrik/scout/tree/main/vec-could-be-mapping/) +- Test Cases: [`vec-could-be-mapping-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/vec-could-be-mapping/vec-could-be-mapping-1) + +When using a `Vec` to store key-value pairs, it is possible to use a `Mapping` instead. This will reduce the gas usage of the contract, as the `Vec` will have to iterate over all elements to find the desired key-value pair. + +## Exploit Scenario + +Consider the following ink! contract, where you have a `balances` vec of tuples of `(AccountId, Balance)`. If you want to find the `Balance` from a specific `AccountId`, you will have to iterate over all elements of the `balances` vec to find the desired `AccountId`. + +```rust + pub struct Contract { + balances: Vec<(AccountId, Balance)>, + } + + pub enum Error { + NotFound, + } + + impl Contract { + /* --- snip --- */ + #[ink(message)] + pub fn get_balance(&mut self, acc: AccountId) -> Result { + self.balances + .iter() + .find(|(a, _)| *a == acc) + .map(|(_, b)| *b) + .ok_or(Error::NotFound) + } + /* --- snip --- */ + } + +``` + +Using `.find(...)` over an iterator of tuples consumes more gas than using a `Mapping` to store the key-value pairs. + +## Remediation + +```rust + pub struct VecCouldBeMapping { + balances: Mapping, + } + + pub enum Error { + NotFound, + } + + impl Contract { + /* --- snip --- */ + #[ink(message)] + pub fn get_balance(&mut self, acc: AccountId) -> Result { + self.balances.get(&acc).ok_or(Error::NotFound) + } + /* --- snip --- */ + } + +``` + diff --git a/docs/docs/vulnerabilities/32-dont-use-invoke-contract-v1.md b/docs/docs/vulnerabilities/32-dont-use-invoke-contract-v1.md new file mode 100644 index 00000000..b5fe708c --- /dev/null +++ b/docs/docs/vulnerabilities/32-dont-use-invoke-contract-v1.md @@ -0,0 +1,103 @@ +# Don't use invoke_contract_v1 + +## Description +- Vulnerability Category: `Best Practices` +- Vulnerability Severity: `Enhancement` +- Detectors: [`dont-use-invoke-contract-v1`](https://github.com/CoinFabrik/scout/tree/main/detectors/dont-use-invoke-contract-v1) +- Test Cases: [`dont-use-invoke-contract-v1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/dont-use-invoke-contract-v1) + +Avoid using `invoke_contract_v1` as it is a low level way to evaluate another smart contract. If needed, use `invoke_contract` instead. + + +## Exploit Scenario + +Consider the following example + +```rust + /// Calls the given address with the given amount and selector. + #[ink(message)] + pub fn call_with_value( + &mut self, + address: AccountId, + amount: Balance, + selector: u32, + ) -> Balance { + ink::env::debug_println!( + "call_with_value function called from {:?}", + self.env().caller() + ); + let caller_addr = self.env().caller(); + let caller_balance = self.balances.get(caller_addr).unwrap_or(0); + if amount <= caller_balance { + //The call is built without allowing reentrancy calls + let call = build_call::() + .call_v1(address) + .transferred_value(amount) + .exec_input(ink::env::call::ExecutionInput::new(Selector::new( + selector.to_be_bytes(), + ))) + .returns::<()>() + .params(); + self.env() + .invoke_contract_v1(&call) + .unwrap_or_else(|err| panic!("Err {:?}", err)) + .unwrap_or_else(|err| panic!("LangErr {:?}", err)); + self.balances + .insert(caller_addr, &(caller_balance - amount)); + + Ok(()) + caller_balance - amount + } else { + caller_balance + } + } +``` + +## Remediation + +```rust + // Dont use it altogether or use invoke_contract but beware it can have several Errors. + #[ink(message)] + pub fn call_with_value( + &mut self, + address: AccountId, + amount: Balance, + selector: u32, + ) -> Balance { + ink::env::debug_println!( + "call_with_value function called from {:?}", + self.env().caller() + ); + let caller_addr = self.env().caller(); + let caller_balance = self.balances.get(caller_addr).unwrap_or(0); + if amount <= caller_balance { + //The call is built without allowing reentrancy calls + let call = build_call::() + .call(address) + .transferred_value(amount) + .exec_input(ink::env::call::ExecutionInput::new(Selector::new( + selector.to_be_bytes(), + ))) + .returns::<()>() + .params(); + self.env() + .invoke_contract(&call) + .unwrap_or_else(|err| panic!("Err {:?}", err)) + .unwrap_or_else(|err| panic!("LangErr {:?}", err)); + self.balances + .insert(caller_addr, &(caller_balance - amount)); + + caller_balance - amount + } else { + caller_balance + } + } +``` + +## References + +- https://docs.rs/ink_env/5.0.0/ink_env/fn.invoke_contract_v1.html + + + + diff --git a/docs/docs/vulnerabilities/4-panic-error.md b/docs/docs/vulnerabilities/4-panic-error.md index 86413975..effcecae 100644 --- a/docs/docs/vulnerabilities/4-panic-error.md +++ b/docs/docs/vulnerabilities/4-panic-error.md @@ -44,7 +44,7 @@ The usage of `panic!` in this example, is not recommended because it will stop the execution of the caller contract. If the method was called by the user, then he will receive `ContractTrapped` as the only error message. -The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/panic-error/panic-error-1/vulnerable-example/lib.rs). +The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/panic-error/panic-error-1/vulnerable-example/src/lib.rs). ## Remediation A possible remediation goes as follows: @@ -75,7 +75,7 @@ By first defining the `Error` enum and then returning a `Result<(), Error>`, more information is added to the caller and, e.g. the caller contract could decide to revert the transaction or to continue execution. -The remediated code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/panic-error/panic-error-1/remediated-example/lib.rs). +The remediated code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/panic-error/panic-error-1/remediated-example/src/lib.rs). ## References diff --git a/docs/docs/vulnerabilities/6-dos-unbounded-operation.md b/docs/docs/vulnerabilities/6-dos-unbounded-operation.md index dacd1527..063479c8 100644 --- a/docs/docs/vulnerabilities/6-dos-unbounded-operation.md +++ b/docs/docs/vulnerabilities/6-dos-unbounded-operation.md @@ -2,7 +2,7 @@ ## Description - Vulnerability Category: `Denial of Service` - Severity: `Medium` -- Detectors: [`dos-unbounded-operation`](https://github.com/CoinFabrik/scout/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-1) +- Detectors: [`dos-unbounded-operation`](https://github.com/CoinFabrik/scout/tree/main/detectors/dos-unbounded-operation/src) - Test Cases: [`dos-unbounded-operation-1`](https://github.com/CoinFabrik/scout/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-1) Each block in a Substrate Blockchain has an upper bound on the amount of gas @@ -41,7 +41,7 @@ pub fn add_payee(&mut self) -> u128 { self.next_payee_ix.checked_sub(1).unwrap() } ``` -The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-1/vulnerable-example/lib.rs). +The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-1/vulnerable-example/src/lib.rs). ### Deployment An example can be found under the directory @@ -58,7 +58,7 @@ If looping over an array of unknown size is absolutely necessary, then it should be planned to potentially take multiple blocks, and therefore require multiple transactions. -The remediated code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-1/remediated-example/lib.rs). +The remediated code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-1/remediated-example/src/lib.rs). ## References - https://consensys.github.io/smart-contract-best-practices/attacks/denial-of-service diff --git a/docs/docs/vulnerabilities/7-dos-unexpected-revert-with-vector.md b/docs/docs/vulnerabilities/7-dos-unexpected-revert-with-vector.md index 47470c89..d3cbabd3 100644 --- a/docs/docs/vulnerabilities/7-dos-unexpected-revert-with-vector.md +++ b/docs/docs/vulnerabilities/7-dos-unexpected-revert-with-vector.md @@ -265,7 +265,7 @@ 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. -The #[cfg(test)] block contains a single test that adds 512 candidates to the +The `#[cfg(test)]` block contains a single test that adds 512 candidates to the smart contract. It initializes the contract with the current timestamp + 10 minutes and then uses a loop to add each candidate. The test verifies that the function to add a candidate fails with an error indicating that the vote @@ -289,7 +289,7 @@ is indicated by the `#[should_panic(expected = "add_candidate failed: CallDryRun attribute on the test function. This test _does_ trigger an unexpected revert due to the contract's storage size. -The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/lib.rs). +The vulnerable code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/vulnerable-example/src/lib.rs). ### Deployment (of the vulnerable contract) @@ -585,7 +585,7 @@ mod unexpected_revert { } ``` -The remediated code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/lib.rs). +The remediated code example can be found [here](https://github.com/CoinFabrik/scout/blob/main/test-cases/dos-unexpected-revert-with-vector/dos-unexpected-revert-with-vector-1/remediated-example/src/lib.rs). ### Deployment (of the remediated contract) @@ -630,4 +630,4 @@ test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; fini - [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://ethernaut.openzeppelin.com/level/0x43BA674B4fbb8B157b7441C2187bCdD2cdF84FD5) +- [Ethernaut: King](https://github.com/OpenZeppelin/ethernaut/blob/master/contracts/src/levels/King.sol) diff --git a/docs/docs/vulnerabilities/README.md b/docs/docs/vulnerabilities/README.md index 850bf8ba..66c74f36 100644 --- a/docs/docs/vulnerabilities/README.md +++ b/docs/docs/vulnerabilities/README.md @@ -346,3 +346,49 @@ This vulnerability falls under the [Known Bugs](#vulnerability-categories) categ and has a Critical severity. Check the following [documentation](23-lazy-delegate.md) for a more detailed explanation of this vulnerability class. + +### 24 - Incorrect exponentiation + +It's common to use `^` for exponentiation. However in Rust, `^` is the XOR operator. If the `^` operator is used, it could lead to unexpected behaviour in the contract. It's recommended to use the method `pow()` for exponentiation or `.bitxor()` for XOR operations. + +Check the following [documentation](24-incorrect-exponentiation.md) for a more detailed explanation of this vulnerability class. + +### 25 - Buffering unsized types + +The static buffer in ink! defaults to 16KB in size. If data overgrows this size, the contract will panic. Instead, when working with dynamically sized values, use fallible storage methods. + +Check the following [documentation](25-buffering-unsized-types.md) for a more detailed explanation of this vulnerability class. + +### 26 - Avoid unsafe block + +Avoid using the `unsafe` block in Rust, as it can lead to memory unsafety and undefined behavior. + +Check the following [documentation](26-avoid-unsafe-block.md) for a more detailed explanation of this vulnerability class. + +### 27 - Warning sr25519_verify + +It is clear that any production code should not rely on unstable features, as they may change in future versions of the language. This is the case for `sr25529_verify` method. + +Check the following [documentation](27-warning-sr25519-verify.md) for a more detailed explanation of this vulnerability class. + +### 28 - Lazy values not set + +Check the following [documentation](28-lazy-values-not-set.md) for a more detailed explanation of this vulnerability class. + +### 29 - Avoid autokey upgradable + +Check the following [documentation](29-avoid-autokey-upgradable.md) for a more detailed explanation of this vulnerability class. + +### 30 - Non payable transferred value + +Check the following [documentation](30-non-payable-transferred-value.md) for a more detailed explanation of this vulnerability class. + +### 31 - Vector of tuples could be mapping + +Check the following [documentation](31-vec-could-be-mapping.md) for a more detailed explanation of this vulnerability class. + +### 32 - Don't use invoke contract v1 + +This is a low level way to evaluate another smart contract. Prefer to use the `ink!` guided and type safe approach to using this. + +Check the following [documentation](32-dont-use-invoke-contract-v1.md) for a more detailed explanation of this vulnerability class. \ No newline at end of file