From a3924ca271237b83969aa137eaed20360e2b5b2d Mon Sep 17 00:00:00 2001 From: 0xphaze <0xphaze@gmail.com> Date: Wed, 12 Apr 2023 14:03:42 +0100 Subject: [PATCH] GPT Review --- not-so-smart-contracts/substrate/README.md | 8 +-- .../substrate/arithmetic_overflow/README.md | 10 +-- .../substrate/dont_panic/README.md | 16 ++--- .../substrate/origins/README.md | 18 ++--- .../substrate/randomness/README.md | 66 +++++++++---------- .../substrate/validate_unsigned/README.md | 20 +++--- .../substrate/verify_first/README.md | 21 +++--- .../substrate/weights_and_fees/README.md | 26 ++++---- 8 files changed, 92 insertions(+), 93 deletions(-) diff --git a/not-so-smart-contracts/substrate/README.md b/not-so-smart-contracts/substrate/README.md index 2ccd84c3..8397c686 100644 --- a/not-so-smart-contracts/substrate/README.md +++ b/not-so-smart-contracts/substrate/README.md @@ -1,14 +1,14 @@ # (Not So) Smart Pallets -This repository contains examples of common Substrate pallet vulnerabilities. Use Not So Smart Pallets to learn about Substrate vulnerabilities, as a reference when performing security reviews, and as a benchmark for security and analysis tools. +This repository contains examples of common vulnerabilities in Substrate pallets. Use the Not So Smart Pallets to learn about Substrate vulnerabilities, as a reference for performing security reviews, and as a benchmark for security analysis tools. ## Features Each _Not So Smart Pallet_ includes a standard set of information: - Description of the vulnerability type -- Attack scenarios to exploit the vulnerability -- Recommendations to eliminate or mitigate the vulnerability +- Attack scenarios for exploiting the vulnerability +- Recommendations for eliminating or mitigating the vulnerability - A mock pallet that exhibits the flaw - References to third-party resources with more information @@ -28,4 +28,4 @@ Each _Not So Smart Pallet_ includes a standard set of information: These examples are developed and maintained by [Trail of Bits](https://www.trailofbits.com/). -If you have questions, problems, or just want to learn more, then join the #ethereum channel on the [Empire Hacking Slack](https://empireslacking.herokuapp.com/) or [contact us](https://www.trailofbits.com/contact/) directly. +If you have questions, need assistance, or just want to learn more, then join the #ethereum channel on the [Empire Hacking Slack](https://empireslacking.herokuapp.com/) or [contact us](https://www.trailofbits.com/contact/) directly. diff --git a/not-so-smart-contracts/substrate/arithmetic_overflow/README.md b/not-so-smart-contracts/substrate/arithmetic_overflow/README.md index 338885ec..48f9165d 100644 --- a/not-so-smart-contracts/substrate/arithmetic_overflow/README.md +++ b/not-so-smart-contracts/substrate/arithmetic_overflow/README.md @@ -1,10 +1,10 @@ -# Arithmetic overflow +# Arithmetic Overflow -Arithmetic overflow in Substrate occurs when arithmetic operations are performed using primitive operations instead of specialized functions that check for overflow. When a Substrate node is compiled in `debug` mode, integer overflows will cause the program to panic. However, when the node is compiled in `release` mode (e.g. `cargo build --release`), Substrate will perform two's complement wrapping. A production-ready node will be compiled in `release` mode, which makes it vulnerable to arithmetic overflow. +In Substrate, arithmetic overflow occurs when arithmetic operations use primitive operations instead of specialized functions designed to check for overflow. When a Substrate node is compiled in `debug` mode, integer overflows will cause the program to panic. However, when the node is compiled in `release` mode (e.g. `cargo build --release`), Substrate performs two's complement wrapping. A production-ready node is compiled in `release` mode, which makes it susceptible to arithmetic overflow. # Example -In the [`pallet-overflow`](https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/substrate/arithmetic_overflow/pallet-overflow.rs) pallet, notice that the `transfer` function sets `update_sender` and `update_to` using primitive arithmetic operations. +In the [`pallet-overflow`](https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/substrate/arithmetic_overflow/pallet-overflow.rs) pallet, note that the `transfer` function sets `update_sender` and `update_to` using primitive arithmetic operations. ```rust /// Allow minting account to transfer a given balance to another account. @@ -31,9 +31,9 @@ In the [`pallet-overflow`](https://github.com/crytic/building-secure-contracts/b } ``` -The sender of the extrinsic can exploit this vulnerability by causing `update_sender` to underflow, which artificially inflates their balance. +The sender of the extrinsic can exploit this vulnerability by causing `update_sender` to underflow, thus artificially inflating their balance. -**Note**: Aside from the stronger mitigations mentioned below, a check to make sure that `sender` has at least `amount` balance would have also prevented an underflow. +**Note**: In addition to the stronger mitigations mentioned below, checking to make sure that `sender` has at least `amount` balance would have also prevented an underflow. # Mitigations diff --git a/not-so-smart-contracts/substrate/dont_panic/README.md b/not-so-smart-contracts/substrate/dont_panic/README.md index 8fa3b443..deb13bcd 100644 --- a/not-so-smart-contracts/substrate/dont_panic/README.md +++ b/not-so-smart-contracts/substrate/dont_panic/README.md @@ -1,16 +1,16 @@ # Don't Panic! -Panics occur when the node enters a state that it cannot handle and stops the program / process instead of trying to proceed. Panics can occur for a large variety of reasons such as out-of-bounds array access, incorrect data validation, type conversions, and much more. A well-designed Substrate node must NEVER panic! If a node panics, it opens up the possibility for a denial-of-service (DoS) attack. +Panics occur when a node enters a state it cannot handle, which causes the program or process to stop instead of attempting to proceed. Various factors can lead to panics, such as out-of-bounds array access, improper data validation, type conversions, and many others. A well-designed Substrate node must NEVER panic! If a node panics, it becomes vulnerable to denial-of-service (DoS) attacks. # Example -In the [`pallet-dont-panic`](https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/substrate/dont_panic/pallet-dont-panic.rs) pallet, the `find_important_value` dispatchable checks to see if `useful_amounts[0]` is greater than `1_000`. If so, it sets the `ImportantVal` `StorageValue` to the value held in `useful_amounts[0]`. +In the [`pallet-dont-panic`](https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/substrate/dont_panic/pallet-dont-panic.rs) pallet, the `find_important_value` dispatchable checks if `useful_amounts[0]` is greater than `1_000`. If it is, the `ImportantVal` `StorageValue` is set to the value contained in `useful_amounts[0]`. ```rust /// Do some work /// /// Parameters: - /// - `useful_amounts`: A vector of u64 values in which there is a important value. + /// - `useful_amounts`: A vector of u64 values which contains an important value. /// /// Emits `FoundVal` event when successful. #[pallet::weight(10_000)] @@ -28,14 +28,14 @@ In the [`pallet-dont-panic`](https://github.com/crytic/building-secure-contracts } ``` -However, notice that there is no check before the array indexing to see whether the length of `useful_amounts` is greater than zero. Thus, if `useful_amounts` is empty, the indexing will cause an array out-of-bounds error which will make the node panic. Since the `find_important_value` function is callable by anyone, an attacker can set `useful_amounts` to an empty array and spam the network with malicious transactions to launch a DoS attack. +However, note that there is no check before the array indexing to verify whether the length of `useful_amounts` is greater than zero. Consequently, if `useful_amounts` is empty, the indexing will trigger an array out-of-bounds error, causing the node to panic. Since the `find_important_value` function can be called by anyone, an attacker could set `useful_amounts` to an empty array and spam the network with malicious transactions, launching a DoS attack. # Mitigations -- Write non-throwing Rust code (e.g. prefer returning [`Result`](https://paritytech.github.io/substrate/master/frame_support/dispatch/result/enum.Result.html) types, use [`ensure!`](https://paritytech.github.io/substrate/master/frame_support/macro.ensure.html), etc.). -- Proper data validation of all input parameters is crucial to ensure that an unexpected panic does not occur. -- A thorough suite of unit tests should be implemented. -- Fuzz testing (e.g. using [`test-fuzz`](https://github.com/trailofbits/test-fuzz)) can aid in exploring more of the input space. +- Write non-throwing Rust code (e.g., prefer returning [`Result`](https://paritytech.github.io/substrate/master/frame_support/dispatch/result/enum.Result.html) types, use [`ensure!`](https://paritytech.github.io/substrate/master/frame_support/macro.ensure.html), etc.). +- Thoroughly validate all input parameters to avoid unexpected panics. +- Implement a comprehensive suite of unit tests. +- Perform fuzz testing (e.g., using [`test-fuzz`](https://github.com/trailofbits/test-fuzz)) to explore a wider range of inputs. # References diff --git a/not-so-smart-contracts/substrate/origins/README.md b/not-so-smart-contracts/substrate/origins/README.md index e5d9d412..2f3f880d 100644 --- a/not-so-smart-contracts/substrate/origins/README.md +++ b/not-so-smart-contracts/substrate/origins/README.md @@ -1,8 +1,8 @@ # Origins -The origin of a call tells a dispatchable function where the call has come from. Origins are a way to implement access controls in the system. +In the context of dispatchable functions, the origin of a call indicates where the call has come from. Origins serve as a means to implement access controls in a system. -There are three types of origins that can used in the runtime: +There are three types of origins that can be used in the runtime: ```rust pub enum RawOrigin { @@ -12,13 +12,13 @@ pub enum RawOrigin { } ``` -Outside of the out-of-box origins, custom origins can also be created that are catered to a specific runtime. The primary use case for custom origins is to configure privileged access to dispatch calls in the runtime, outside of `RawOrigin::Root`. +Aside from the default origins, custom origins tailored for specific runtimes can also be created. The primary purpose of custom origins is to configure privileged access for dispatch calls in the runtime, outside of `RawOrigin::Root`. -Using privileged origins, like `RawOrigin::Root` or custom origins, can lead to access control violations if not used correctly. It is a common error to use `ensure_signed` in place of `ensure_root` which would allow any user to bypass the access control placed by using `ensure_root`. +Using privileged origins, such as `RawOrigin::Root` or custom origins, can result in access control violations if not employed properly. A common mistake is using `ensure_signed` instead of `ensure_root`, which allows any user to bypass the access control imposed by `ensure_root`. # Example -In the [`pallet-bad-origin`](https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/substrate/origins/pallet-bad-origin.rs) pallet, there is a `set_important_val` function that should be only callable by the `ForceOrigin` _custom_ origin type. This custom origin allows the pallet to specify that only a specific account can call `set_important_val`. +The [`pallet-bad-origin`](https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/substrate/origins/pallet-bad-origin.rs) pallet features a `set_important_val` function that should be callable only by the `ForceOrigin` _custom_ origin type. This custom origin enables the pallet to specify that only a specific account can call `set_important_val`. ```rust #[pallet::call] @@ -42,7 +42,7 @@ impl Pallet { } ``` -However, the `set_important_val` is using `ensure_signed`; this allows any account to set `ImportantVal`. To allow only the `ForceOrigin` to call `set_important_val` the following change can be made: +However, since `set_important_val` is using `ensure_signed`, any account can set `ImportantVal`. To restrict `set_important_val` to `ForceOrigin` calls only, the following change can be made: ```rust T::ForceOrigin::ensure_origin(origin.clone())?; @@ -51,9 +51,9 @@ let sender = ensure_signed(origin)?; # Mitigations -- Ensure that the correct access controls are placed on privileged functions. -- Develop user documentation on all risks associated with the system, including those associated with privileged users. -- A thorough suite of unit tests that validates access controls is crucial. +- Ensure that appropriate access controls are applied to privileged functions. +- Create user documentation that covers all risks associated with the system, including those related to privileged users. +- Develop a comprehensive suite of unit tests to verify access controls. # References diff --git a/not-so-smart-contracts/substrate/randomness/README.md b/not-so-smart-contracts/substrate/randomness/README.md index db11afac..7fd73436 100644 --- a/not-so-smart-contracts/substrate/randomness/README.md +++ b/not-so-smart-contracts/substrate/randomness/README.md @@ -1,59 +1,59 @@ # Bad Randomness -To use randomness in a Substrate pallet, all you need to do is require a source of randomness in the `Config` trait of a pallet. This source of Randomness must implement the [`Randomness`](https://paritytech.github.io/substrate/master/frame_support/traits/trait.Randomness.html) trait. The trait provides two methods for obtaining randomness. +To utilize randomness in a Substrate pallet, simply require a source of randomness in the `Config` trait of a pallet. This source of randomness must implement the [`Randomness`](https://paritytech.github.io/substrate/master/frame_support/traits/trait.Randomness.html) trait. The trait provides two methods for obtaining randomness: -1. `random_seed`: This function takes no arguments and returns back a random value. Calling this value multiple times in a block will result in the same value. -2. `random`: Takes in a byte-array (a.k.a "context-identifier") and returns a value that is as independent as possible from other contexts. +1. `random_seed`: This function takes no arguments and returns a random value. Calling this function multiple times in a block results in the same value. +2. `random`: Takes in a byte array (a.k.a "context-identifier") and returns a value that is as independent as possible from other contexts. -Substrate provides the [Randomness Collective Flip Pallet](https://docs.rs/pallet-randomness-collective-flip/latest/pallet_randomness_collective_flip/) and a Verifiable Random Function implementation in the [BABE pallet](https://paritytech.github.io/substrate/master/pallet_babe/index.html). Developers can also choose to build their own source of randomness. +Substrate offers the [Randomness Collective Flip Pallet](https://docs.rs/pallet-randomness-collective-flip/latest/pallet_randomness_collective_flip/) and a Verifiable Random Function implementation in the [BABE pallet](https://paritytech.github.io/substrate/master/pallet_babe/index.html). Developers can also opt to build their own source of randomness. -A bad source of randomness can lead to a variety of exploits such as the theft of funds or undefined system behavior. +A poor source of randomness can lead to various exploits, such as theft of funds or unpredictable system behavior. # Example -The [`pallet-bad-lottery`](https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/substrate/randomness/pallet-bad-lottery.rs) pallet is a simplified "lottery" system that requires one to guess the next random number. If they guess correctly, they are the winner of the lottery. +The [`pallet-bad-lottery`](https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/substrate/randomness/pallet-bad-lottery.rs) pallet is a simplified "lottery" system that requires one to guess the next random number. If a user guesses correctly, they are declared the winner of the lottery. ```rust #[pallet::call] impl Pallet { -/// Guess the random value -/// If you guess correctly, you become the winner -#[pallet::weight(10_000)] -pub fn guess( - origin: OriginFor, - guess: T::Hash -) -> DispatchResultWithPostInfo { - let sender = ensure_signed(origin)?; - // Random value. - let nonce = Self::get_and_increment_nonce(); - let (random_value, _) = T::MyRandomness::random(&nonce); - // Check if guess is correct - ensure!(guess == random_value, >::IncorrectGuess); - >::put(&sender); + /// Guess the random value + /// If you guess correctly, you become the winner + #[pallet::weight(10_000)] + pub fn guess( + origin: OriginFor, + guess: T::Hash + ) -> DispatchResultWithPostInfo { + let sender = ensure_signed(origin)?; + // Random value + let nonce = Self::get_and_increment_nonce(); + let (random_value, _) = T::MyRandomness::random(&nonce); + // Check if guess is correct + ensure!(guess == random_value, >::IncorrectGuess); + >::put(&sender); - Self::deposit_event(Event::NewWinner(sender)); + Self::deposit_event(Event::NewWinner(sender)); - Ok(().into()) -} + Ok(().into()) + } } impl Pallet { -/// Increment the nonce each time guess() is called -pub fn get_and_increment_nonce() -> Vec { - let nonce = Nonce::::get(); - Nonce::::put(nonce.wrapping_add(1)); - nonce.encode() -} + /// Increment the nonce each time guess() is called + pub fn get_and_increment_nonce() -> Vec { + let nonce = Nonce::::get(); + Nonce::::put(nonce.wrapping_add(1)); + nonce.encode() + } } ``` -Note that the quality of randomness provided to the `pallet-bad-lottery` pallet is related to the randomness source. If the randomness source is the "Randomness Collective Flip Pallet", this lottery system is insecure. This is because the collective flip pallet implements "low-influence randomness". This makes it vulnerable to a collusion attack where a small minority of participants can give the same random number contribution making it highly likely to have the seed be this random number (click [here](https://ethresear.ch/t/rng-exploitability-analysis-assuming-pure-randao-based-main-chain/1825/7) to learn more). Additionally, as mentioned in the Substrate documentation, "low-influence randomness can be useful when defending against relatively weak adversaries. Using this pallet as a randomness source is advisable primarily in low-security situations like testing." +Note that the quality of randomness provided to the `pallet-bad-lottery` pallet depends on the randomness source. If the randomness source is the "Randomness Collective Flip Pallet", this lottery system is insecure. This is because the collective flip pallet implements "low-influence randomness", making it vulnerable to a collusion attack where a small minority of participants can provide the same random number contribution, making it highly likely that the seed will be this random number. (To learn more, click [here](https://ethresear.ch/t/rng-exploitability-analysis-assuming-pure-randao-based-main-chain/1825/7)). Additionally, as mentioned in the Substrate documentation, "low-influence randomness can be useful when defending against relatively weak adversaries. Using this pallet as a randomness source is advisable primarily in low-security situations like testing." # Mitigations -- Use the randomness implementation provided by the [BABE pallet](https://paritytech.github.io/substrate/master/pallet_babe/index.html). This pallet provides "production-grade randomness, and is used in Polkadot. **Selecting this randomness source dictates that your blockchain use Babe consensus.**" -- Defer from creating a custom source of randomness unless specifically necessary for the runtime being developed. -- Do not use `random_seed` as the method of choice for randomness unless specifically necessary for the runtime being developed. +- Use the randomness implementation provided by the [BABE pallet](https://paritytech.github.io/substrate/master/pallet_babe/index.html), which offers "production-grade randomness" and is used in Polkadot. **Selecting this randomness source mandates that your blockchain utilize Babe consensus.** +- Refrain from creating a custom source of randomness unless specifically necessary for the runtime being developed. +- Avoid using `random_seed` as the method of choice for randomness unless specifically necessary for the runtime being developed. # References diff --git a/not-so-smart-contracts/substrate/validate_unsigned/README.md b/not-so-smart-contracts/substrate/validate_unsigned/README.md index 40d11cb4..12bbe12c 100644 --- a/not-so-smart-contracts/substrate/validate_unsigned/README.md +++ b/not-so-smart-contracts/substrate/validate_unsigned/README.md @@ -1,17 +1,17 @@ # Unsigned Transaction Validation -There are three types of transactions allowed in a Substrate runtime: signed, unsigned, and inherent. An unsigned transaction does not require a signature and does not include information about who sent the transaction. This is naturally problematic because there is no by-default deterrent to spam or replay attacks. Because of this, Substrate allows users to create custom functions to validate unsigned transaction. However, incorrect or improper validation of an unsigned transaction can allow _anyone_ to perform potentially malicious actions. Usually, unsigned transactions are allowed only for select parties (e.g., off-chain workers (OCW)). But, if improper data validation of an unsigned transaction allows a malicious actor to spoof data as if it came from an OCW, this can lead to unexpected behavior. Additionally, improper validation opens up the possibility to replay attacks where the same transaction can be sent to the transaction pool again and again to perform some malicious action repeatedly. +Substrate runtime supports three types of transactions: signed, unsigned, and inherent. Unsigned transactions do not require a signature and do not include information about the sender. This makes them vulnerable to spam, replay attacks, and other malicious actions. To mitigate these risks, Substrate enables users to create custom functions to validate unsigned transactions. Correctly validating unsigned transactions is crucial, as failure in this area could allow malicious actors to spoof data and cause unexpected behavior or replay attacks. -The validation of an unsigned transaction must be provided by the pallet that chooses to accept them. To allow unsigned transactions, a pallet must implement the [`frame_support::unsigned::ValidateUnsigned`](https://paritytech.github.io/substrate/master/frame_support/attr.pallet.html#validate-unsigned-palletvalidate_unsigned-optional) trait. The `validate_unsigned` function, which must be implemented as part of the `ValidateUnsigned` trait, will provide the logic necessary to ensure that the transaction is valid. An off chain worker (OCW) can be implemented directly in a pallet using the `offchain_worker` [hook](https://paritytech.github.io/substrate/master/frame_support/attr.pallet.html#hooks-pallethooks-optional). The OCW can send an unsigned transaction by calling [`SubmitTransaction::submit_unsigned_transaction`](https://paritytech.github.io/substrate/master/frame_system/offchain/struct.SubmitTransaction.html). Upon submission, the `validate_unsigned` function will ensure that the transaction is valid and then pass the transaction on towards towards the final dispatchable function. +Unsigned transaction validation must be provided by the pallet that decides to accept them. To do this, a pallet must implement the [`frame_support::unsigned::ValidateUnsigned`](https://paritytech.github.io/substrate/master/frame_support/attr.pallet.html#validate-unsigned-palletvalidate_unsigned-optional) trait. The `validate_unsigned` function, which must be implemented as part of this trait, provides the necessary logic to ensure that the transaction is valid. Pallets can also incorporate an off-chain worker (OCW) using the `offchain_worker` [hook](https://paritytech.github.io/substrate/master/frame_support/attr.pallet.html#hooks-pallethooks-optional), which can send unsigned transactions by calling [`SubmitTransaction::submit_unsigned_transaction`](https://paritytech.github.io/substrate/master/frame_system/offchain/struct.SubmitTransaction.html). The `validate_unsigned` function then validates the transaction before passing it to the final dispatchable function. # Example -The [`pallet-bad-unsigned`](https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/substrate/validate_unsigned/pallet-bad-unsigned.rs) pallet is an example that showcases improper unsigned transaction validation. The pallet tracks the average, rolling price of some "asset"; this price data is being retrieved by an OCW. The `fetch_price` function, which is called by the OCW, naively returns 100 as the current price (note that an [HTTP request](https://github.com/paritytech/substrate/blob/e8a7d161f39db70cb27fdad6c6e215cf493ebc3b/frame/examples/offchain-worker/src/lib.rs#L572-L625) can be made here for true price data). The `validate_unsigned` function (see below) simply validates that the `Call` is being made to `submit_price_unsigned` and nothing else. +The [`pallet-bad-unsigned`](https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/substrate/validate_unsigned/pallet-bad-unsigned.rs) pallet demonstrates improper unsigned transaction validation. This example pallet tracks the average rolling price of an asset fetched by an OCW. The `fetch_price` function, which the OCW calls, naively returns a fixed price of 100 (note that a real-world implementation would use an [HTTP request](https://github.com/paritytech/substrate/blob/e8a7d161f39db70cb27fdad6c6e215cf493ebc3b/frame/examples/offchain-worker/src/lib.rs#L572-L625) to fetch actual price data). The `validate_unsigned` function simply validates that the `Call` is made to `submit_price_unsigned` without verifying the submitted data. ```rust -/// By default unsigned transactions are disallowed, but implementing the validator -/// here we make sure that some particular calls (the ones produced by offchain worker) -/// are being whitelisted and marked as valid. +/// By default, unsigned transactions are disallowed, but implementing the validator. +/// Here, we make sure that only specific calls (those made by the off-chain worker) +/// are whitelisted and considered valid. fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> TransactionValidity { // If `submit_price_unsigned` is being called, the transaction is valid. // Otherwise, it is an InvalidTransaction. @@ -35,14 +35,12 @@ fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> Transacti } ``` -However, notice that there is nothing preventing an attacker from sending malicious price data. Both `block_number` and `price` can be set to arbitrary values. For `block_number`, it would be valuable to ensure that it is not a block number in the future; only price data for the current block can be submitted. Additionally, medianization can be used to ensure that the reported price is not severely affected by outliers. Finally, unsigned submissions can be throttled by enforcing a delay after each submission. - -Note that the simplest solution would be to sign the offchain submissions so that the runtime can validate that a known OCW is sending the price submission transactions. +In this example, nothing prevents an attacker from submitting malicious price data with arbitrary `block_number` and `price` values. To improve the validation process, only current block numbers should be allowed, with proper data ranges and limitations in place. Also, consider medianization or throttling submission rates to minimize the impact of malicious data. Nonetheless, the most straightforward solution would involve signing off-chain submissions, allowing the runtime to verify the authenticity of OCW transactions. # Mitigations -- Consider whether unsigned transactions is a requirement for the runtime that is being built. OCWs can also submit signed transactions or transactions with signed payloads. -- Ensure that each parameter provided to `validate_unsigned` is validated to prevent the runtime from entering a state that is vulnerable or undefined. +- Evaluate whether unsigned transactions are necessary for the runtime under development. Alternatives include using signed transactions or transactions with signed payloads for OCWs. +- Thoroughly validate each parameter provided to the `validate_unsigned` function, ensuring that the runtime remains secure and well-defined at all times. # References diff --git a/not-so-smart-contracts/substrate/verify_first/README.md b/not-so-smart-contracts/substrate/verify_first/README.md index f5feb554..9392f597 100644 --- a/not-so-smart-contracts/substrate/verify_first/README.md +++ b/not-so-smart-contracts/substrate/verify_first/README.md @@ -1,27 +1,27 @@ # Verify First, Write Last -**NOTE**: As of [Polkadot v0.9.25](https://github.com/substrate-developer-hub/substrate-docs/issues/1215), the **Verify First, Write Last** practice is no longer required. However, since older versions are still vulnerable and because it is still best practice, it is worth discussing the "Verify First, Write Last" idiom. +**NOTE**: As of [Polkadot v0.9.25](https://github.com/substrate-developer-hub/substrate-docs/issues/1215), the **Verify First, Write Last** practice is no longer required. However, since older versions are still vulnerable, and since it is still considered best practice, it is worth discussing the "Verify First, Write Last" idiom. -Substrate does not cache state prior to extrinsic dispatch. Instead, state changes are made as they are invoked. This is in contrast to a transaction in Ethereum where, if the transaction reverts, no state changes will persist. In the case of Substrate, if a state change is made and then the dispatch throws a `DispatchError`, the original state change will persist. This unique behavior has led to the "Verify First, Write Last" practice. +Substrate does not cache state prior to extrinsic dispatch; instead, it makes state changes as they are invoked. This contrasts with Ethereum transactions, where, if a transaction reverts, no state changes will persist. In the case of Substrate, if a state change is made and the dispatch then throws a `DispatchError`, the original state change will persist. This unique behavior has led to the adoption of the "Verify First, Write Last" practice. ```rust { - // all checks and throwing code go here + // Place all checks and throwing code here - // ** no throwing code below this line ** + // ** No throwing code should be below this line ** - // all event emissions & storage writes go here + // Place all event emissions & storage writes here } ``` # Example -In the [`pallet-verify-first`](https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/substrate/verify_first/pallet-verify-first.rs) pallet, the `init` dispatchable is used to set up the `TotalSupply` of the token and transfer them to the `sender`. `init` should be only called once. Thus, the `Init` boolean is set to `true` when it is called initially. If `init` is called more than once, the transaction will throw an error because `Init` is already `true`. +In the [`pallet-verify-first`](https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/substrate/verify_first/pallet-verify-first.rs) pallet, the `init` dispatchable is used to set up the `TotalSupply` of the token and transfer it to the `sender`. `init` should be called only once. Therefore, the `Init` boolean is set to `true` when it is initially called. If `init` is called more than once, the transaction will generate an error because the `Init` value is already `true`. ```rust /// Initialize the token /// Transfers the total_supply amount to the caller -/// If init() has already been called, throw AlreadyInitialized error +/// Throws an AlreadyInitialized error if init() has already been called #[pallet::weight(10_000)] pub fn init( origin: OriginFor, @@ -32,10 +32,11 @@ pub fn init( if supply > 0 { >::put(&supply); } + // Set sender's balance to total_supply() >::insert(&sender, supply); - // Revert above changes if init() has already been called + // Revert previous changes if init() has already been called ensure!(!Self::is_init(), >::AlreadyInitialized); // Set Init StorageValue to `true` @@ -48,11 +49,11 @@ pub fn init( } ``` -However, notice that the setting of `TotalSupply` and the transfer of funds happens before the check on `Init`. This violates the "Verify First, Write Last" practice. In an older version of Substrate, this would allow a malicious `sender` to call `init` multiple times and change the value of `TotalSupply` and their balance of the token. +However, notice that the setting of `TotalSupply` and the transfer of funds occur before the check on `Init`. This violates the "Verify First, Write Last" practice. In an older version of Substrate, this would enable a malicious `sender` to call `init` multiple times and alter the values of `TotalSupply` and their token balance. # Mitigations -- Follow the "Verify First, Write Last" practice by doing all the necessary data validation before performing state changes and emitting events. +- Adhere to the "Verify First, Write Last" practice by performing all necessary data validation before executing state changes and emitting events. # References diff --git a/not-so-smart-contracts/substrate/weights_and_fees/README.md b/not-so-smart-contracts/substrate/weights_and_fees/README.md index 1300a127..c3fe75cc 100644 --- a/not-so-smart-contracts/substrate/weights_and_fees/README.md +++ b/not-so-smart-contracts/substrate/weights_and_fees/README.md @@ -1,14 +1,14 @@ # Weights and Fees -Weights and transaction fees are the two main ways to regulate the consumption of blockchain resources. The overuse of blockchain resources can allow a malicious actor to spam the network to cause a denial-of-service (DoS). Weights are used to manage the time it takes to validate the block. The larger the weight, the more "resources" / time the computation takes. Transaction fees provide an economic incentive to limit the number of resources used to perform operations; the fee for a given transaction is a function of the weight required by the transaction. +Weights and transaction fees are the two primary methods for regulating the consumption of blockchain resources. Overusing blockchain resources can enable a malicious actor to spam the network, resulting in a denial-of-service (DoS) attack. Weights manage the time it takes to validate a block. The larger the weight, the more time and resources the computation requires. Transaction fees provide an economic incentive to limit resource usage for operations. The fee for a given transaction depends on the weight required by the transaction. -Weights can be fixed or a custom "weight annotation / function" can be implemented. A weight function can calculate the weight, for example, based on the number of database read / writes and the size of the input paramaters (e.g. a long `Vec<>`). To optimize the weight such that users do not pay too little or too much for a transaction, benchmarking can be used to empirically determine the correct weight in worst case scenarios. +Weights can be fixed or use a custom "weight annotation/function." A weight function might calculate the weight based on the number of database reads/writes and the size of the input parameters (e.g., a long `Vec<>`). To optimize the weight, ensuring users don't pay too little or too much for a transaction, benchmarking can be used to empirically determine the correct weight in worst-case scenarios. -Specifying the correct weight function and benchmarking it is crucial to protect the Substrate node from denial-of-service (DoS) attacks. Since fees are a function of weight, a bad weight function implies incorrect fees. For example, if some function performs heavy computation (which takes a lot of time) but specifies a very small weight, it is cheap to call that function. In this way an attacker can perform a low-cost attack while still stealing a large amount of block execution time. This will prevent regular transactions from being fit into those blocks. +Specifying the accurate weight function and benchmarking it is vital to protect the Substrate node from DoS attacks. Since fees depend on weight, a poorly defined weight function implies incorrect fees. For example, if a function performs heavy computation (requiring much time) but specifies a minimal weight, that function is cheap to call. In this case, an attacker can execute a low-cost attack while still stealing a substantial amount of block execution time, preventing regular transactions from being included in those blocks. # Example -In the [`pallet-bad-weights`](https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/substrate/weights_and_fees/pallet-bad-weights.rs) pallet, a custom weight function, `MyWeightFunction`, is used to calculate the weight for a call to `do_work`. The weight required for a call to `do_work` is `10_000_000` times the length of the `useful_amounts` vector. +In the [`pallet-bad-weights`](https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/substrate/weights_and_fees/pallet-bad-weights.rs) pallet, a custom weight function, `MyWeightFunction`, calculates the weight for a call to `do_work`. The weight required for calling `do_work` is `10_000_000` times the length of the `useful_amounts` vector. ```rust impl WeighData<(&Vec,)> for MyWeightFunction { @@ -18,31 +18,31 @@ impl WeighData<(&Vec,)> for MyWeightFunction { } ``` -However, if the length of the `useful_amounts` vector is zero, the weight to call `do_work` would be zero. A weight of zero implies that calling this function is financially cheap. This opens the opportunity for an attacker to call `do_work` a large number of times to saturate the network with malicious transactions without having to pay a large fee and could cause a DoS attack. +However, if the length of the `useful_amounts` vector is zero, the weight for calling `do_work` is also zero. A weight of zero implies that the function is financially cheap to call. This situation opens up opportunities for an attacker to call `do_work` numerous times, saturating the network with malicious transactions without paying a large fee, potentially causing a DoS attack. -One potential fix for this is to set a fixed weight if the length of `useful_amounts` is zero. +One possible solution is to set a fixed weight if the length of `useful_amounts` is zero. ```rust impl WeighData<(&Vec,)> for MyWeightFunction { fn weigh_data(&self, (amounts,): (&Vec,)) -> Weight { // The weight function is `y = mx + b` where `m` and `b` are both `self.0` (the static fee) and `x` is the length of the `amounts` array. - // If `amounts.len() == 0` then the weight is simply the static fee (i.e. `y = b`) + // If `amounts.len() == 0`, then the weight is simply the static fee (i.e., `y = b`) self.0 + self.0.saturating_mul(amounts.len() as u64).into() } } ``` -In the example above, if the length of `amounts` (i.e. `useful_amounts`) is zero, then the function will return `self.0` (i.e. `10_000_000`). +In the example above, if the length of `amounts` (i.e., `useful_amounts`) is zero, the function returns `self.0` (i.e., `10_000_000`). -On the other hand, if an attacker sends a `useful_amounts` vector that is incredibly large, the returned `Weight` can become large enough such that the dispatchable takes up a large amount block execution time and prevents other transactions from being fit into the block. A fix for this would be to bound the maximum allowable length for `useful_amounts`. +On the other hand, if an attacker submits a `useful_amounts` vector with an incredibly large size, the returned `Weight` could become so large that the dispatchable consumes a significant amount of block execution time, preventing other transactions from fitting into the block. A solution would be to set an upper limit on the maximum allowable length for `useful_amounts`. -**Note**: Custom _fee_ functions can also be created. These functions should also be carefully evaluated and tested to ensure that the risk of DoS attacks is mitigated. +**Note**: Custom _fee_ functions can also be created. These functions should be carefully evaluated and tested to ensure that they mitigate the risk of DoS attacks. # Mitigations -- Use [benchmarking](https://docs.substrate.io/main-docs/test/benchmark/) to empirically test the computational resources utilized by various dispatchable functions. Additionally, use benchmarking to define a lower and upper weight bound for each dispatchable. -- Create bounds for input arguments to prevent a transaction from taking up too many computational resources. For example, if a `Vec<>` is being taken as an input argument to a function, prevent the length of the `Vec<>` from being too large. -- Be wary of fixed weight dispatchables (e.g. `#[pallet::weight(1_000_000)]`). A weight that is completely agnostic to database read / writes or input parameters may open up avenues for DoS attacks. +- Use [benchmarking](https://docs.substrate.io/main-docs/test/benchmark/) to empirically test the computational resources utilized by various dispatchable functions. Use benchmarking to define lower and upper weight bounds for each dispatchable. +- Create limits for input arguments to prevent a transaction from consuming too many computational resources. For example, if a `Vec<>` is an input argument to a function, restrict the length of the `Vec<>` to prevent it from becoming excessively large. +- Be cautious with fixed-weight dispatchables (e.g., `#[pallet::weight(1_000_000)]`). A weight that doesn't consider database reads/writes or input parameters may expose the system to DoS attacks. # References