From c268d91097764dfafb054dfa0c4c58ae1b900260 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Tue, 5 Jul 2022 17:54:03 -0400 Subject: [PATCH 01/38] initial commit to update e2e documentation --- .../echidna/end-to-end-testing.md | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/program-analysis/echidna/end-to-end-testing.md b/program-analysis/echidna/end-to-end-testing.md index c398e13c..3abdcfe4 100644 --- a/program-analysis/echidna/end-to-end-testing.md +++ b/program-analysis/echidna/end-to-end-testing.md @@ -24,18 +24,20 @@ $ cd drizzle-box $ npm i truffle ``` -If ganache and ganache-cli are not installed, add them manually. In our example, we will run: +If `ganache` is not installed, add it manually. In our example, we will run: ``` -$ npm i ganache ganache-cli +$ npm i ganache ``` Other projects using yarn will require: ``` -$ yarn add ganache ganache-cli +$ yarn add ganache ``` +Ensure that `$ ganache --version` outputs `ganache v7.3.3` or greater. + It is also important to select *one* test script from the available tests. Ideally, this test will deploy all (or most) contracts, including mock/test ones. For this example, we are going to take a look to the `SimpleStorage` contract: ```solidity @@ -79,10 +81,18 @@ Before starting to write interesting properties, it is necessary to to collect a First, start Etheno: ``` -$ etheno --ganache --ganache-args "--deterministic --gasLimit 10000000" -x init.json +$ etheno --ganache --ganache-args="--gasLimit 10000000" -x init.json ``` -If Etheno fails to produce any output, then it fails to execute `ganache`. Check if `ganache` can be executed correctly from your terminal. +Note that by default the following Ganache arguments are set via Etheno: +- `-d`: Ganache will use a pre-defined, deterministic seed to create all accounts. +- `--chain.allowUnlimitedContractSize`: Allows unlimited contract sizes while debugging. This is set so that there is no size limitations on the contracts that are going to be deployed +- `-p `: The `port_num` will be set to (1) the value of `--ganache-port` or (2) Etheno will choose the smallest port number higher than the port number on which Etheno’s JSON RPC server is running. + +Note that if the deployment fails to complete successfully due to an out-of-gas exception, increasing the `--gasLimit` value can help. +This is especially helpful for large contract deployments. Learn more about the various Ganache command-line arguments that can be set by clicking [here](https://www.npmjs.com/package/ganache). + +Additionally, if Etheno fails to produce any output, then it fails to execute `ganache`. Check if `ganache` can be executed correctly from your terminal. Meanwhile, in another terminal, run *one* test or the deployment process. How to run it depends on how the project was developed. For instance, for truffle, use: @@ -102,7 +112,7 @@ In the Drizzle example, we will run: $ truffle test test/simplestorage.js --network develop. ``` -After Etheno finishes, gently kill it using ctrl+c (twice). It will save the `init.json` file. If your test fails for some reason or you want to run a different one, restart etheno and re-run the test. +After Etheno finishes, gently kill it by using Ctrl+c (or Command+C on Mac). It will save the `init.json` file. If your test fails for some reason or you want to run a different one, restart Etheno and re-run the test. ## Writing and running a property: @@ -119,6 +129,8 @@ contract E2E { } ``` +For large, multi-contract deployments, using `console.log` to print out the deployed, contract addresses can be valuable in quickly setting up the Echidna testing contract. + This simple property checks if the stored data remains constant. To run it you will need the following echidna config file (`echidna.yaml`): ```yaml From 5f612aae8e077536ccb73886f3794b4d67112549 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Wed, 6 Jul 2022 16:55:33 -0400 Subject: [PATCH 02/38] config option fix --- program-analysis/echidna/end-to-end-testing.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/program-analysis/echidna/end-to-end-testing.md b/program-analysis/echidna/end-to-end-testing.md index 3abdcfe4..26e236e8 100644 --- a/program-analysis/echidna/end-to-end-testing.md +++ b/program-analysis/echidna/end-to-end-testing.md @@ -81,7 +81,7 @@ Before starting to write interesting properties, it is necessary to to collect a First, start Etheno: ``` -$ etheno --ganache --ganache-args="--gasLimit 10000000" -x init.json +$ etheno --ganache --ganache-args="--miner.blockGasLimit 10000000" -x init.json ``` Note that by default the following Ganache arguments are set via Etheno: From d5377155b9987cb3359fbcfc836b5f1939073667 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Thu, 7 Jul 2022 15:48:51 -0400 Subject: [PATCH 03/38] added text about etheno edge cases --- program-analysis/echidna/end-to-end-testing.md | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/program-analysis/echidna/end-to-end-testing.md b/program-analysis/echidna/end-to-end-testing.md index 26e236e8..a57192a1 100644 --- a/program-analysis/echidna/end-to-end-testing.md +++ b/program-analysis/echidna/end-to-end-testing.md @@ -36,7 +36,7 @@ Other projects using yarn will require: $ yarn add ganache ``` -Ensure that `$ ganache --version` outputs `ganache v7.3.3` or greater. +Ensure that `$ ganache --version` outputs `ganache v7.3.2` or greater. It is also important to select *one* test script from the available tests. Ideally, this test will deploy all (or most) contracts, including mock/test ones. For this example, we are going to take a look to the `SimpleStorage` contract: @@ -89,10 +89,10 @@ Note that by default the following Ganache arguments are set via Etheno: - `--chain.allowUnlimitedContractSize`: Allows unlimited contract sizes while debugging. This is set so that there is no size limitations on the contracts that are going to be deployed - `-p `: The `port_num` will be set to (1) the value of `--ganache-port` or (2) Etheno will choose the smallest port number higher than the port number on which Etheno’s JSON RPC server is running. -Note that if the deployment fails to complete successfully due to an out-of-gas exception, increasing the `--gasLimit` value can help. +Note that if the deployment fails to complete successfully due to a `ProviderError: exceeds block gas limit` exception, increasing the `--miner.blockGasLimit` value can help. This is especially helpful for large contract deployments. Learn more about the various Ganache command-line arguments that can be set by clicking [here](https://www.npmjs.com/package/ganache). -Additionally, if Etheno fails to produce any output, then it fails to execute `ganache`. Check if `ganache` can be executed correctly from your terminal. +Additionally, if Etheno fails to produce any output, then it failed to execute `ganache` under-the-hood. Check if `ganache` (with the associated command-line arguments) can be executed correctly from your terminal without the use of Etheno. Meanwhile, in another terminal, run *one* test or the deployment process. How to run it depends on how the project was developed. For instance, for truffle, use: @@ -114,7 +114,7 @@ $ truffle test test/simplestorage.js --network develop. After Etheno finishes, gently kill it by using Ctrl+c (or Command+C on Mac). It will save the `init.json` file. If your test fails for some reason or you want to run a different one, restart Etheno and re-run the test. -## Writing and running a property: +## Writing and running a property Once we have a json file with saved transactions, we can verify that the `SimpleStorage` contract is deployed at `0x871DD7C2B4b25E1Aa18728e9D5f2Af4C4e431f5c`, so we can easily write a contract (`./contracts/crytic/E2E.sol`) with a simple a property to test it: @@ -152,4 +152,9 @@ crytic_const_storage: failed!💥 For this last step, make sure you are using `.` as a target for `echidna-test`. If you use the path to the `E2E.sol` file instead, then Echidna will not be able get information from all the deployed contracts to call the `set(uint256)` function and the property will never fail. +## Key considerations: +When using Etheno with Echidna, note that there are two edge cases that may cause unexpected behavior: +1. Function calls that use ether: The accounts that are created and used for testing in Ganache are not the same as the accounts that are used to send transactions in Echidna. Thus, the account balances of the Ganache accounts do not carry over to the accounts used by Echidna. So, if there is a function call logged by Etheno that requires the transfer of some ether from an account that exists in Ganache, this call will fail in Echidna. +2. Fuzz tests that rely on `block.timestamp`: The concept of time is different between Ganache and Echidna. Echidna always starts with a fixed timestamp while Etheno will use Ganache's concept of time. This means that assertions or requirements in a fuzz test that rely on timestamp comparisons / evaluations may fail in Echidna. + In the next part of this tutorial, we will explore how to easily find where contracts are deployed with a specific tool based on Slither. This will be useful if the deployment process is complex and we need to test an specific contract. From 396e5f0641ea87d93a577a4e07c8060190273634 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Thu, 7 Jul 2022 16:33:10 -0400 Subject: [PATCH 04/38] docker instructions improved --- program-analysis/echidna/end-to-end-testing.md | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/program-analysis/echidna/end-to-end-testing.md b/program-analysis/echidna/end-to-end-testing.md index a57192a1..866703c5 100644 --- a/program-analysis/echidna/end-to-end-testing.md +++ b/program-analysis/echidna/end-to-end-testing.md @@ -80,15 +80,27 @@ Before starting to write interesting properties, it is necessary to to collect a First, start Etheno: -``` +```bash $ etheno --ganache --ganache-args="--miner.blockGasLimit 10000000" -x init.json ``` -Note that by default the following Ganache arguments are set via Etheno: +By default the following Ganache arguments are set via Etheno: - `-d`: Ganache will use a pre-defined, deterministic seed to create all accounts. - `--chain.allowUnlimitedContractSize`: Allows unlimited contract sizes while debugging. This is set so that there is no size limitations on the contracts that are going to be deployed - `-p `: The `port_num` will be set to (1) the value of `--ganache-port` or (2) Etheno will choose the smallest port number higher than the port number on which Etheno’s JSON RPC server is running. +**NOTE:** If you are using Docker to run etheno, the commands should be: + +```bash +$ docker run -it -p 8545:8545 -v ~/etheno:/home/etheno/ trailofbits/etheno +(you will now be working within the Docker instance) +$ etheno --ganache --ganache-args="--miner.blockGasLimit 10000000" -x init.json +``` +- The `-p` in the _first command_ publishes (i.e. exposes) port 8545 from inside the Docker container out to port 8545 on the host. +- The `-v` in the _first command_ maps a directory from inside the Docker container to one outside the Docker container. After Etheno exits, the `init.json` file will now be in the `~/etheno` folder on the host. + + + Note that if the deployment fails to complete successfully due to a `ProviderError: exceeds block gas limit` exception, increasing the `--miner.blockGasLimit` value can help. This is especially helpful for large contract deployments. Learn more about the various Ganache command-line arguments that can be set by clicking [here](https://www.npmjs.com/package/ganache). From 01b39d55314a791359ed1d5629a8eed7498ff01a Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Fri, 15 Jul 2022 14:37:32 -0400 Subject: [PATCH 05/38] Initial commit for Substrate not-so-smart 1. Added a pallet that does not comply with Verify first, write last principle. 2. Added a pallet that has an integer overflow / underflow 3. Added README.md for both pallets with almost no content --- .../substrate/arithmetic_overflow/README.md | 1 + .../substrate/arithmetic_overflow/transfer.rs | 37 ++++++++++++ .../substrate/runtime_storage/README.md | 6 ++ .../substrate/runtime_storage/mint.rs | 58 +++++++++++++++++++ 4 files changed, 102 insertions(+) create mode 100644 not-so-smart-contracts/substrate/arithmetic_overflow/README.md create mode 100644 not-so-smart-contracts/substrate/arithmetic_overflow/transfer.rs create mode 100644 not-so-smart-contracts/substrate/runtime_storage/README.md create mode 100644 not-so-smart-contracts/substrate/runtime_storage/mint.rs diff --git a/not-so-smart-contracts/substrate/arithmetic_overflow/README.md b/not-so-smart-contracts/substrate/arithmetic_overflow/README.md new file mode 100644 index 00000000..99a95992 --- /dev/null +++ b/not-so-smart-contracts/substrate/arithmetic_overflow/README.md @@ -0,0 +1 @@ +There is an overflow/underflow issue for the `updated_from_balance` and `updated_to_balance`. \ No newline at end of file diff --git a/not-so-smart-contracts/substrate/arithmetic_overflow/transfer.rs b/not-so-smart-contracts/substrate/arithmetic_overflow/transfer.rs new file mode 100644 index 00000000..cb401fe4 --- /dev/null +++ b/not-so-smart-contracts/substrate/arithmetic_overflow/transfer.rs @@ -0,0 +1,37 @@ +pub enum Event { + MintedNewSupply(T::AccountId), + Transfered(T::AccountId, T::AccountId, T::Balance), +} + +#[pallet::storage] +#[pallet::getter(fn get_balance)] +pub(super) type BalanceToAccount = StorageMap < + _, + Blake2_128Concat, + T::AccountId, + T::Balance, + ValueQuery +>; + +pub(super) fn transfer( + origin: OriginFor, + to: T::AccountId, + #[pallet::compact] amount: T::Balance, +) -> DispatchResultWithPostInfo { + let sender = ensure_signed(origin)?; + // Current balances + let sender_balance = Self::get_balance(&sender); + let receiver_balance = Self::get_balance(&to); + + // Calculate new balances. + let updated_from_balance = sender_balance.sub(value); + let updated_to_balance = receiver_balance.add(value); + + // Update state + >::insert(&sender, updated_from_balance); + >::insert(&to, updated_to_balance); + + // Emit event + Self::deposit_event(RawEvent::Transfer(sender, to, value)); + Ok(()) +} \ No newline at end of file diff --git a/not-so-smart-contracts/substrate/runtime_storage/README.md b/not-so-smart-contracts/substrate/runtime_storage/README.md new file mode 100644 index 00000000..76201fe0 --- /dev/null +++ b/not-so-smart-contracts/substrate/runtime_storage/README.md @@ -0,0 +1,6 @@ +Goal here is to show that state changes should not be made before all potentially reverting operations have been validated: +Verify first, write last principle from [here](https://docs.substrate.io/main-docs/build/runtime-storage/) + +Also, the mint function in this pallet is public, which can be problematic in itself. + +Can make this not-so-smart-pallet contain two issues, I guess \ No newline at end of file diff --git a/not-so-smart-contracts/substrate/runtime_storage/mint.rs b/not-so-smart-contracts/substrate/runtime_storage/mint.rs new file mode 100644 index 00000000..03a14d49 --- /dev/null +++ b/not-so-smart-contracts/substrate/runtime_storage/mint.rs @@ -0,0 +1,58 @@ +pub enum Event { + MintedNewSupply(T::AccountId), + Transfered(T::AccountId, T::AccountId, T::Balance), +} + +#[pallet::storage] +#[pallet::getter(fn get_balance)] +pub(super) type BalanceToAccount = StorageMap < + _, + Blake2_128Concat, + T::AccountId, + T::Balance, + ValueQuery +>; + +#[pallet::weight(10_000 + T::DbWeight::get().writes(1))] +pub(super) fn mint( + origin: OriginFor, + #[pallet::compact] amount: T::Balance +) -> DispatchResultWithPostInfo { + + let sender = ensure_signed(origin)?; + + // Update storage + >::insert(&sender, amount); + + // Check if the kitty does not already exist in storage + ensure!(Self::kitties(&kitty_id) == None, >::KittyExists); + + // Emit an event + Self::deposit_event(Event::MintedNewSupply(sender)); + + // Return successful DispatchResultWithPostInfo + Ok(().into()) +} + +pub(super) fn transfer( + origin: OriginFor, + to: T::AccountId, + #[pallet::compact] amount: T::Balance, +) -> DispatchResultWithPostInfo { + let sender = ensure_signed(origin)?; + // Current balances + let sender_balance = Self::get_balance(&sender); + let receiver_balance = Self::get_balance(&to); + + // Calculate new balances. + let updated_from_balance = sender_balance.checked_sub(value).ok_or(>::InsufficientFunds)?; + let updated_to_balance = receiver_balance.checked_add(value).expect("Entire supply fits in u64, qed"); + + // Update state + >::insert(&sender, updated_from_balance); + >::insert(&to, updated_to_balance); + + // Emit event + Self::deposit_event(RawEvent::Transfer(sender, to, value)); + Ok(()) +} \ No newline at end of file From 91da30ad7b206aa466613bf047caf4c6e98ad564 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Fri, 15 Jul 2022 16:07:47 -0400 Subject: [PATCH 06/38] Remove transfer function from mint.rs --- .../substrate/runtime_storage/mint.rs | 23 ------------------- 1 file changed, 23 deletions(-) diff --git a/not-so-smart-contracts/substrate/runtime_storage/mint.rs b/not-so-smart-contracts/substrate/runtime_storage/mint.rs index 03a14d49..ab3c6dea 100644 --- a/not-so-smart-contracts/substrate/runtime_storage/mint.rs +++ b/not-so-smart-contracts/substrate/runtime_storage/mint.rs @@ -32,27 +32,4 @@ pub(super) fn mint( // Return successful DispatchResultWithPostInfo Ok(().into()) -} - -pub(super) fn transfer( - origin: OriginFor, - to: T::AccountId, - #[pallet::compact] amount: T::Balance, -) -> DispatchResultWithPostInfo { - let sender = ensure_signed(origin)?; - // Current balances - let sender_balance = Self::get_balance(&sender); - let receiver_balance = Self::get_balance(&to); - - // Calculate new balances. - let updated_from_balance = sender_balance.checked_sub(value).ok_or(>::InsufficientFunds)?; - let updated_to_balance = receiver_balance.checked_add(value).expect("Entire supply fits in u64, qed"); - - // Update state - >::insert(&sender, updated_from_balance); - >::insert(&to, updated_to_balance); - - // Emit event - Self::deposit_event(RawEvent::Transfer(sender, to, value)); - Ok(()) } \ No newline at end of file From d1aa7bb7b6866c9db2851b6608b50834a9536eec Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Wed, 3 Aug 2022 15:11:04 -0400 Subject: [PATCH 07/38] Adding functioning pallet-overflow.rs 1. Renamed transfer.rs to pallet-overflow.rs to comply with substrate naming convention 2. pallet-overflow.rs will compile in substrate-node-template and the bug is exploitable --- .../arithmetic_overflow/pallet-overflow.rs | 113 ++++++++++++++++++ .../substrate/arithmetic_overflow/transfer.rs | 37 ------ 2 files changed, 113 insertions(+), 37 deletions(-) create mode 100644 not-so-smart-contracts/substrate/arithmetic_overflow/pallet-overflow.rs delete mode 100644 not-so-smart-contracts/substrate/arithmetic_overflow/transfer.rs diff --git a/not-so-smart-contracts/substrate/arithmetic_overflow/pallet-overflow.rs b/not-so-smart-contracts/substrate/arithmetic_overflow/pallet-overflow.rs new file mode 100644 index 00000000..d37cf7cc --- /dev/null +++ b/not-so-smart-contracts/substrate/arithmetic_overflow/pallet-overflow.rs @@ -0,0 +1,113 @@ +#![cfg_attr(not(feature = "std"), no_std)] + +pub use pallet::*; +#[frame_support::pallet] +pub mod pallet { + use frame_support::{dispatch::DispatchResultWithPostInfo, pallet_prelude::*}; + use frame_system::pallet_prelude::*; + + /// Pallet configuration + #[pallet::config] + pub trait Config: frame_system::Config { + /// Because this pallet emits events, it depends on the runtime's definition of an event. + type Event: From> + IsType<::Event>; + } + + #[pallet::pallet] + #[pallet::generate_store(pub(super) trait Store)] + pub struct Pallet(_); + + // Total supply of tokens + #[pallet::type_value] + pub(super) fn TotalSupplyDefaultValue() -> u64 { + 1000 + } + + // Data structure that holds the total supply of tokens + #[pallet::storage] + #[pallet::getter(fn total_supply)] + pub(super) type TotalSupply = + StorageValue<_, u64, ValueQuery, TotalSupplyDefaultValue>; + + // Data structure that holds whether or not the pallet's init() function has been called + #[pallet::storage] + #[pallet::getter(fn is_init)] + pub(super) type Init = StorageValue<_, bool, ValueQuery>; + + /// Storage item for balances to accounts mapping. + #[pallet::storage] + #[pallet::getter(fn get_balance)] + pub(super) type BalanceToAccount = StorageMap< + _, + Blake2_128Concat, + T::AccountId, + u64, + ValueQuery + >; + + /// Token mint can emit two Event types. + #[pallet::event] + #[pallet::generate_deposit(pub(super) fn deposit_event)] + pub enum Event { + /// Token was initialized by user + Initialized(T::AccountId), + /// Tokens were successfully transferred between accounts. [from, to, value] + Transferred(T::AccountId, T::AccountId, u64), + } + + #[pallet::hooks] + impl Hooks> for Pallet {} + + #[pallet::error] + pub enum Error { + /// Attempted to initialize the token after it had already been initialized. + AlreadyInitialized, + } + + #[pallet::call] + impl Pallet { + /// Initialize the token + /// Transfers the total_supply amount to the caller + /// If init() has already been called, throw AlreadyInitialized error + #[pallet::weight(10_000)] + pub fn init(origin: OriginFor) -> DispatchResultWithPostInfo { + let sender = ensure_signed(origin)?; + ensure!(!Self::is_init(), >::AlreadyInitialized); + + >::insert(sender, Self::total_supply()); + + Init::::put(true); + Ok(().into()) + } + + /// Allow minting account to transfer a given balance to another account. + /// + /// Parameters: + /// - `to`: The account to receive the transfer. + /// - `amount`: The amount of balance to transfer. + /// + /// Emits `Transferred` event when successful. + #[pallet::weight(10_000)] + pub fn transfer( + origin: OriginFor, + to: T::AccountId, + amount: u64, + ) -> DispatchResultWithPostInfo { + let sender = ensure_signed(origin)?; + let sender_balance = Self::get_balance(&sender); + let receiver_balance = Self::get_balance(&to); + + // Calculate new balances. + let update_sender = sender_balance - amount; + let update_to = receiver_balance + amount; + + // Update both accounts storage. + >::insert(&sender, update_sender); + >::insert(&to, update_to); + + // Emit event. + Self::deposit_event(Event::Transferred(sender, to, amount)); + Ok(().into()) + } + } +} \ No newline at end of file diff --git a/not-so-smart-contracts/substrate/arithmetic_overflow/transfer.rs b/not-so-smart-contracts/substrate/arithmetic_overflow/transfer.rs deleted file mode 100644 index cb401fe4..00000000 --- a/not-so-smart-contracts/substrate/arithmetic_overflow/transfer.rs +++ /dev/null @@ -1,37 +0,0 @@ -pub enum Event { - MintedNewSupply(T::AccountId), - Transfered(T::AccountId, T::AccountId, T::Balance), -} - -#[pallet::storage] -#[pallet::getter(fn get_balance)] -pub(super) type BalanceToAccount = StorageMap < - _, - Blake2_128Concat, - T::AccountId, - T::Balance, - ValueQuery ->; - -pub(super) fn transfer( - origin: OriginFor, - to: T::AccountId, - #[pallet::compact] amount: T::Balance, -) -> DispatchResultWithPostInfo { - let sender = ensure_signed(origin)?; - // Current balances - let sender_balance = Self::get_balance(&sender); - let receiver_balance = Self::get_balance(&to); - - // Calculate new balances. - let updated_from_balance = sender_balance.sub(value); - let updated_to_balance = receiver_balance.add(value); - - // Update state - >::insert(&sender, updated_from_balance); - >::insert(&to, updated_to_balance); - - // Emit event - Self::deposit_event(RawEvent::Transfer(sender, to, value)); - Ok(()) -} \ No newline at end of file From ee2fd2cf5839662ebfe9746df2b51a520803c946 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Wed, 3 Aug 2022 15:35:09 -0400 Subject: [PATCH 08/38] Changed comment TotalSupplyDefaultValue is not the actual total supply but instead the default one. So changed comment to note that accordingly. --- .../substrate/arithmetic_overflow/pallet-overflow.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/not-so-smart-contracts/substrate/arithmetic_overflow/pallet-overflow.rs b/not-so-smart-contracts/substrate/arithmetic_overflow/pallet-overflow.rs index d37cf7cc..87def723 100644 --- a/not-so-smart-contracts/substrate/arithmetic_overflow/pallet-overflow.rs +++ b/not-so-smart-contracts/substrate/arithmetic_overflow/pallet-overflow.rs @@ -17,7 +17,7 @@ pub mod pallet { #[pallet::generate_store(pub(super) trait Store)] pub struct Pallet(_); - // Total supply of tokens + // DEFAULT total supply of tokens #[pallet::type_value] pub(super) fn TotalSupplyDefaultValue() -> u64 { 1000 From c70c90b55f6dcb18a63597372eb0719d7d3f6a5b Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Thu, 4 Aug 2022 10:33:12 -0400 Subject: [PATCH 09/38] Add verify first, write list pallet This pallet is no longer insecure after polkadot-v0.9.25. However, it is still considered best practice so I'm including it in here. --- .../substrate/runtime_storage/mint.rs | 35 ----- .../runtime_storage/pallet-verify-first.rs | 130 ++++++++++++++++++ 2 files changed, 130 insertions(+), 35 deletions(-) delete mode 100644 not-so-smart-contracts/substrate/runtime_storage/mint.rs create mode 100644 not-so-smart-contracts/substrate/runtime_storage/pallet-verify-first.rs diff --git a/not-so-smart-contracts/substrate/runtime_storage/mint.rs b/not-so-smart-contracts/substrate/runtime_storage/mint.rs deleted file mode 100644 index ab3c6dea..00000000 --- a/not-so-smart-contracts/substrate/runtime_storage/mint.rs +++ /dev/null @@ -1,35 +0,0 @@ -pub enum Event { - MintedNewSupply(T::AccountId), - Transfered(T::AccountId, T::AccountId, T::Balance), -} - -#[pallet::storage] -#[pallet::getter(fn get_balance)] -pub(super) type BalanceToAccount = StorageMap < - _, - Blake2_128Concat, - T::AccountId, - T::Balance, - ValueQuery ->; - -#[pallet::weight(10_000 + T::DbWeight::get().writes(1))] -pub(super) fn mint( - origin: OriginFor, - #[pallet::compact] amount: T::Balance -) -> DispatchResultWithPostInfo { - - let sender = ensure_signed(origin)?; - - // Update storage - >::insert(&sender, amount); - - // Check if the kitty does not already exist in storage - ensure!(Self::kitties(&kitty_id) == None, >::KittyExists); - - // Emit an event - Self::deposit_event(Event::MintedNewSupply(sender)); - - // Return successful DispatchResultWithPostInfo - Ok(().into()) -} \ No newline at end of file diff --git a/not-so-smart-contracts/substrate/runtime_storage/pallet-verify-first.rs b/not-so-smart-contracts/substrate/runtime_storage/pallet-verify-first.rs new file mode 100644 index 00000000..9c558654 --- /dev/null +++ b/not-so-smart-contracts/substrate/runtime_storage/pallet-verify-first.rs @@ -0,0 +1,130 @@ +#![cfg_attr(not(feature = "std"), no_std)] + +pub use pallet::*; + +#[frame_support::pallet] +pub mod pallet { + use frame_support::{dispatch::DispatchResultWithPostInfo, pallet_prelude::*}; + use frame_system::pallet_prelude::*; + + /// Pallet configuration + #[pallet::config] + pub trait Config: frame_system::Config { + /// Because this pallet emits events, it depends on the runtime's definition of an event. + type Event: From> + IsType<::Event>; + } + + #[pallet::pallet] + #[pallet::generate_store(pub(super) trait Store)] + pub struct Pallet(_); + + // DEFAULT Total supply of tokens + #[pallet::type_value] + pub(super) fn TotalSupplyDefaultValue() -> u64 { + 1000 + } + + // Data structure that holds the total supply of tokens + #[pallet::storage] + #[pallet::getter(fn total_supply)] + pub(super) type TotalSupply = + StorageValue<_, u64, ValueQuery, TotalSupplyDefaultValue>; + + // Data structure that holds whether or not the pallet's init() function has been called + #[pallet::storage] + #[pallet::getter(fn is_init)] + pub(super) type Init = StorageValue<_, bool, ValueQuery>; + + /// Storage item for balances to accounts mapping. + #[pallet::storage] + #[pallet::getter(fn get_balance)] + pub(super) type BalanceToAccount = StorageMap< + _, + Blake2_128Concat, + T::AccountId, + u64, + ValueQuery + >; + + /// Token mint can emit two Event types. + #[pallet::event] + #[pallet::generate_deposit(pub(super) fn deposit_event)] + pub enum Event { + /// Token was initialized by user + Initialized(T::AccountId), + /// Tokens were successfully transferred between accounts. [from, to, value] + Transferred(T::AccountId, T::AccountId, u64), + } + + #[pallet::hooks] + impl Hooks> for Pallet {} + + #[pallet::error] + pub enum Error { + /// Attempted to initialize the token after it had already been initialized. + AlreadyInitialized, + /// Attempted to transfer more funds than were available + InsufficientFunds, + } + + #[pallet::call] + impl Pallet { + /// Initialize the token + /// Transfers the total_supply amount to the caller + /// If init() has already been called, throw AlreadyInitialized error + #[pallet::weight(10_000)] + pub fn init( + origin: OriginFor, + supply: u64 + ) -> DispatchResultWithPostInfo { + let sender = ensure_signed(origin)?; + + if supply > 0 { + >::put(&supply); + } + // Set sender's balance to total_supply() + >::insert(&sender, supply); + + // Revert above changes if init() has already been called + ensure!(!Self::is_init(), >::AlreadyInitialized); + + // Set Init StorageValue to `true` + Init::::put(true); + + // Emit event + Self::deposit_event(Event::Initialized(sender)); + + Ok(().into()) + } + + /// Allow minting account to transfer a given balance to another account. + /// + /// Parameters: + /// - `to`: The account to receive the transfer. + /// - `amount`: The amount of balance to transfer. + /// + /// Emits `Transferred` event when successful. + #[pallet::weight(10_000)] + pub fn transfer( + origin: OriginFor, + to: T::AccountId, + amount: u64, + ) -> DispatchResultWithPostInfo { + let sender = ensure_signed(origin)?; + let sender_balance = Self::get_balance(&sender); + let receiver_balance = Self::get_balance(&to); + + // Calculate new balances. + let update_sender = sender_balance.checked_sub(amount).ok_or(>::InsufficientFunds)?; + let update_to = receiver_balance.checked_add(amount).expect("Entire supply should fit in u64"); + + // Update both accounts storage. + >::insert(&sender, update_sender); + >::insert(&to, update_to); + + // Emit event. + Self::deposit_event(Event::Transferred(sender, to, amount)); + Ok(().into()) + } + } +} \ No newline at end of file From 50e211ca4de941112b1861fb529720d33e10c9fe Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Thu, 4 Aug 2022 14:43:18 -0400 Subject: [PATCH 10/38] Add pallets-bad-weight.rs This pallet demonstrates the difficulty in creating proper weight functions. If the length of useful_amounts is zero, the weight for DoWork is zero. This also demonstrates the need for benchmarking. --- .../weights_and_fees/pallet-bad-weights.rs | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 not-so-smart-contracts/substrate/weights_and_fees/pallet-bad-weights.rs diff --git a/not-so-smart-contracts/substrate/weights_and_fees/pallet-bad-weights.rs b/not-so-smart-contracts/substrate/weights_and_fees/pallet-bad-weights.rs new file mode 100644 index 00000000..02860807 --- /dev/null +++ b/not-so-smart-contracts/substrate/weights_and_fees/pallet-bad-weights.rs @@ -0,0 +1,85 @@ +#![cfg_attr(not(feature = "std"), no_std)] + +pub use pallet::*; + +#[frame_support::pallet] +pub mod pallet { + use frame_support::{dispatch::DispatchResultWithPostInfo, pallet_prelude::*, weights::*}; + use frame_system::pallet_prelude::*; + use sp_std::prelude::*; + + /// Pallet configuration + #[pallet::config] + pub trait Config: frame_system::Config { + /// Because this pallet emits events, it depends on the runtime's definition of an event. + type Event: From> + IsType<::Event>; + } + + #[pallet::pallet] + #[pallet::without_storage_info] + #[pallet::generate_store(pub(super) trait Store)] + pub struct Pallet(_); + + /// Storage item for useful_amounts passed to the do_work function + #[pallet::storage] + #[pallet::getter(fn useful_amounts)] + pub(super) type UsefulAmounts = StorageValue<_, Vec, ValueQuery>; + + #[pallet::event] + #[pallet::generate_deposit(pub(super) fn deposit_event)] + pub enum Event { + /// Emit after do_work successfully completes + DidWork(T::AccountId), + } + + #[pallet::hooks] + impl Hooks> for Pallet {} + + /// Custom weight function implementation + pub struct MyWeightFunction(u64); + + /// The weight is linearly proportional to the length of the amounts array + impl WeighData<(&Vec,)> for MyWeightFunction { + fn weigh_data(&self, (amounts,): (&Vec,)) -> Weight { + self.0.saturating_mul(amounts.len() as u64).into() + } + } + + /// Custom weight function implementations need to implement the PaysFee trait + impl PaysFee for MyWeightFunction { + fn pays_fee(&self, _: T) -> Pays { + Pays::Yes + } + } + + /// Custom weight function implementations need to implement the ClassifyDispatch trait + impl ClassifyDispatch for MyWeightFunction { + fn classify_dispatch(&self, _: T) -> DispatchClass { + // Classify all calls as Normal (which is the default) + Default::default() + } + } + + #[pallet::call] + impl Pallet { + /// Do some work + /// + /// Parameters: + /// - `useful_amount`: A vector of u64 values that we want to store. + /// + /// Emits `DidWork` event when successful. + #[pallet::weight(MyWeightFunction(10_000_000))] + pub fn do_work( + origin: OriginFor, + useful_amounts: Vec, + ) -> DispatchResultWithPostInfo { + let sender = ensure_signed(origin)?; + UsefulAmounts::::put(useful_amounts); + // Do other important constant-time (O(1)) work + + // Emit event + Self::deposit_event(Event::DidWork(sender)); + Ok(().into()) + } + } +} \ No newline at end of file From 48cf018e84d4697bf80c52d7439ead0971de806f Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 8 Aug 2022 13:45:20 -0400 Subject: [PATCH 11/38] Initial commit for validate unsigned transactions --- .../substrate/validate-unsigned/README.md | 36 ++++ .../validate-unsigned/pallet-bad-oracle.rs | 185 ++++++++++++++++++ 2 files changed, 221 insertions(+) create mode 100644 not-so-smart-contracts/substrate/validate-unsigned/README.md create mode 100644 not-so-smart-contracts/substrate/validate-unsigned/pallet-bad-oracle.rs diff --git a/not-so-smart-contracts/substrate/validate-unsigned/README.md b/not-so-smart-contracts/substrate/validate-unsigned/README.md new file mode 100644 index 00000000..382491fc --- /dev/null +++ b/not-so-smart-contracts/substrate/validate-unsigned/README.md @@ -0,0 +1,36 @@ +Some code snippets I will need for the README.md +```rust + fn validate_transaction_parameters( + _block_number: &T::BlockNumber, + _new_price: &u32, + ) -> TransactionValidity { + let valid_tx = | provide | { + ValidTransaction::with_tag_prefix("ExampleOffchainWorker") + .priority(UNSIGNED_TXS_PRIORITY) + .and_provides([&provide]) + .longevity(5) + .propagate(true) + .build() + }; + valid_tx(b"submit_price_unsigned".to_vec()) + } +``` + +```rust + if let Call::submit_price_unsigned_with_signed_payload { + price_payload: ref payload, + ref signature, + } = call + { + let signature_valid = + SignedPayload::::verify::(payload, signature.clone()); + if !signature_valid { + return InvalidTransaction::BadProof.into() + } + Self::validate_transaction_parameters(&payload.block_number, &payload.price) + } else if let Call::submit_price_unsigned { block_number, price: new_price } = call { + Self::validate_transaction_parameters(block_number, new_price) + } else { + InvalidTransaction::Call.into() + } +``` \ No newline at end of file diff --git a/not-so-smart-contracts/substrate/validate-unsigned/pallet-bad-oracle.rs b/not-so-smart-contracts/substrate/validate-unsigned/pallet-bad-oracle.rs new file mode 100644 index 00000000..83dea80b --- /dev/null +++ b/not-so-smart-contracts/substrate/validate-unsigned/pallet-bad-oracle.rs @@ -0,0 +1,185 @@ +#![cfg_attr(not(feature = "std"), no_std)] + +use frame_support::traits::Get; +use frame_system::{ + self as system, + offchain::{SubmitTransaction, SendTransactionTypes}, +}; +use sp_runtime::{ + transaction_validity::{InvalidTransaction, TransactionValidity, ValidTransaction}, +}; +const UNSIGNED_TXS_PRIORITY: u64 = 110; + +pub use pallet::*; + +#[frame_support::pallet] +pub mod pallet { + use super::*; + use frame_support::pallet_prelude::*; + use frame_system::pallet_prelude::*; + + /// This pallet's configuration trait + #[pallet::config] + pub trait Config: SendTransactionTypes> + frame_system::Config { + + /// The overarching event type. + type Event: From> + IsType<::Event>; + + /// The overarching dispatch call type. + type Call: From>; + + /// Maximum number of prices. + #[pallet::constant] + type MaxPrices: Get; + + /// A configuration for base priority of unsigned transactions. + /// + /// This is exposed so that it can be tuned for particular runtime, when + /// multiple pallets send unsigned transactions. + #[pallet::constant] + type UnsignedPriority: Get; + } + + #[pallet::pallet] + #[pallet::generate_store(pub(super) trait Store)] + pub struct Pallet(_); + + /// A vector of recently submitted prices. + /// This is used to calculate average price, should have bounded size. + #[pallet::storage] + #[pallet::getter(fn prices)] + pub(super) type Prices = StorageValue<_, BoundedVec, ValueQuery>; + + /// Events for the pallet. + #[pallet::event] + #[pallet::generate_deposit(pub(super) fn deposit_event)] + pub enum Event { + /// Event generated when new price is accepted to contribute to the list of prices. + NewPrice { price: u32, block_number: T::BlockNumber, maybe_who: Option }, + } + + #[pallet::hooks] + impl Hooks> for Pallet { + /// Offchain Worker entry point. + /// By implementing `fn offchain_worker` you declare a new offchain worker. + fn offchain_worker(block_number: T::BlockNumber) { + log::info!("Hello I'm an offchain worker!"); + + // Print debug information + let parent_hash = >::block_hash(block_number - 1u32.into()); + log::debug!("Current block: {:?} (parent hash: {:?}", block_number, parent_hash); + + // Get and log average price + let average: Option = Self::average_price(); + log::debug!("Current price: {:?}", average); + + // Send unsigned transaction to log latest price from an offchain resource + let res = Self::fetch_price_and_send_raw_unsigned(block_number); + if let Err(e) = res { + // Throw error + log::error!("Error: {}", e); + } + } + } + + /// A public part of the pallet. + #[pallet::call] + impl Pallet { + /// Submit new price to the list via unsigned transaction. + /// + /// It's important to specify `weight` for unsigned calls as well, because even though + /// they don't charge fees, we still don't want a single block to contain unlimited + /// number of such transactions. + #[pallet::weight(0)] + pub fn submit_price_unsigned( + origin: OriginFor, + _block_number: T::BlockNumber, + price: u32, + ) -> DispatchResultWithPostInfo { + // This ensures that the function can only be called via unsigned transaction. + ensure_none(origin)?; + // Add the price to the on-chain list, but mark it as coming from an empty address. + Self::add_price(None, price, _block_number); + Ok(().into()) + } + + } + + /// Validate unsigned call to this module. + #[pallet::validate_unsigned] + impl ValidateUnsigned for Pallet { + type Call = Call; + + /// 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. + 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. + if let Call::submit_price_unsigned { block_number, price: new_price } = call { + let avg_price = Self::average_price() + .map(|price| if &price > new_price { price - new_price } else { new_price - price }) + .unwrap_or(0); + + let valid_tx = | provide | { + ValidTransaction::with_tag_prefix("ExampleOffchainWorker") + .priority(T::UnsignedPriority::get().saturating_add(avg_price as _)) + .and_provides([&provide]) + .longevity(5) + .propagate(true) + .build() + }; + valid_tx(b"submit_price_unsigned".to_vec()) + } else { + InvalidTransaction::Call.into() + } + } + } +} + +// Helper functions of the pallet +impl Pallet { + + /// A helper function to fetch the price and send a raw unsigned transaction. + fn fetch_price_and_send_raw_unsigned(block_number: T::BlockNumber) -> Result<(), &'static str> { + // Make an external "HTTP request" to fetch the current price. + // Note this call will block until response is received. + let price = Self::fetch_price(); + + // Create wrapped `Call` struct that will end up calling `submit_price_unsigned` + let call = Call::submit_price_unsigned { block_number, price }; + + // Submit the unsigned transaction on-chain + SubmitTransaction::>::submit_unsigned_transaction(call.into()) + .map_err(|()| "Unable to submit unsigned transaction.")?; + + Ok(()) + } + + + /// Naively "fetch" current price and return the result. + fn fetch_price() -> u32 { + 1000 + } + + /// Calculate current average price. + fn average_price() -> Option { + let prices = >::get(); + if prices.is_empty() { + None + } else { + Some(prices.iter().fold(0_u32, |a, b| a.saturating_add(*b)) / prices.len() as u32) + } + } + + /// Add new price to the list. + fn add_price(maybe_who: Option, price: u32, block_number: T::BlockNumber) { + >::mutate(|prices| { + if prices.try_push(price).is_err() { + prices[(price % T::MaxPrices::get()) as usize] = price; + } + }); + // Here we are raising the NewPrice event + Self::deposit_event(Event::NewPrice { price, block_number, maybe_who }); + } +} \ No newline at end of file From 48ba65f77c111d0f332311899b1d65797407181b Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 8 Aug 2022 15:00:22 -0400 Subject: [PATCH 12/38] Adding example where pallet panics instead of proper error handling --- .../substrate/dont-panic/pallet-dont-panic.rs | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 not-so-smart-contracts/substrate/dont-panic/pallet-dont-panic.rs diff --git a/not-so-smart-contracts/substrate/dont-panic/pallet-dont-panic.rs b/not-so-smart-contracts/substrate/dont-panic/pallet-dont-panic.rs new file mode 100644 index 00000000..9818f4db --- /dev/null +++ b/not-so-smart-contracts/substrate/dont-panic/pallet-dont-panic.rs @@ -0,0 +1,60 @@ +#![cfg_attr(not(feature = "std"), no_std)] + +pub use pallet::*; + +#[frame_support::pallet] +pub mod pallet { + use frame_support::{dispatch::DispatchResultWithPostInfo, pallet_prelude::*, weights::*}; + use frame_system::pallet_prelude::*; + use sp_std::prelude::*; + + /// Pallet configuration + #[pallet::config] + pub trait Config: frame_system::Config { + /// Because this pallet emits events, it depends on the runtime's definition of an event. + type Event: From> + IsType<::Event>; + } + + #[pallet::pallet] + #[pallet::without_storage_info] + #[pallet::generate_store(pub(super) trait Store)] + pub struct Pallet(_); + + /// Storage item for useful_amounts passed to the do_work function + #[pallet::storage] + #[pallet::getter(fn get_val)] + pub(super) type ImportantValue = StorageValue<_, u64, ValueQuery>; + + #[pallet::event] + #[pallet::generate_deposit(pub(super) fn deposit_event)] + pub enum Event { + /// Emit after do_work successfully completes + FoundVal(T::AccountId, u64), + } + + #[pallet::hooks] + impl Hooks> for Pallet {} + + #[pallet::call] + impl Pallet { + /// Do some work + /// + /// Parameters: + /// - `useful_amount`: A vector of u64 values that we want to store. + /// + /// Emits `DidWork` event when successful. + #[pallet::weight(10_000)] + pub fn do_work( + origin: OriginFor, + useful_amounts: Vec, + ) -> DispatchResultWithPostInfo { + let sender = ensure_signed(origin)?; + if useful_amounts[0] > 0 { + ImportantValue::::put(&useful_amounts[0]); + } + // Emit event + Self::deposit_event(Event::FoundVal(sender, useful_amounts[0])); + Ok(().into()) + } + } +} \ No newline at end of file From 13fe9ac60d026208bf7c96e6bc5e1e88366b9596 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 8 Aug 2022 15:09:19 -0400 Subject: [PATCH 13/38] Code cleanup for pallet-dont-panic.rs Decided to add an error type so that `ensure!()` can be used to cause the out-of-bounds check --- .../substrate/dont-panic/pallet-dont-panic.rs | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/not-so-smart-contracts/substrate/dont-panic/pallet-dont-panic.rs b/not-so-smart-contracts/substrate/dont-panic/pallet-dont-panic.rs index 9818f4db..cc09ae6c 100644 --- a/not-so-smart-contracts/substrate/dont-panic/pallet-dont-panic.rs +++ b/not-so-smart-contracts/substrate/dont-panic/pallet-dont-panic.rs @@ -4,7 +4,7 @@ pub use pallet::*; #[frame_support::pallet] pub mod pallet { - use frame_support::{dispatch::DispatchResultWithPostInfo, pallet_prelude::*, weights::*}; + use frame_support::{dispatch::DispatchResultWithPostInfo, pallet_prelude::*}; use frame_system::pallet_prelude::*; use sp_std::prelude::*; @@ -20,7 +20,7 @@ pub mod pallet { #[pallet::generate_store(pub(super) trait Store)] pub struct Pallet(_); - /// Storage item for useful_amounts passed to the do_work function + /// Storage item for holding an ImportantValue #[pallet::storage] #[pallet::getter(fn get_val)] pub(super) type ImportantValue = StorageValue<_, u64, ValueQuery>; @@ -28,30 +28,38 @@ pub mod pallet { #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event { - /// Emit after do_work successfully completes + /// Emit after val is found FoundVal(T::AccountId, u64), } #[pallet::hooks] impl Hooks> for Pallet {} + #[pallet::error] + pub enum Error { + NoImportantValueFound, + } + #[pallet::call] impl Pallet { /// Do some work /// /// Parameters: - /// - `useful_amount`: A vector of u64 values that we want to store. + /// - `useful_amounts`: A vector of u64 values in which there is a important value. /// /// Emits `DidWork` event when successful. #[pallet::weight(10_000)] - pub fn do_work( + pub fn find_important_value( origin: OriginFor, useful_amounts: Vec, ) -> DispatchResultWithPostInfo { let sender = ensure_signed(origin)?; - if useful_amounts[0] > 0 { - ImportantValue::::put(&useful_amounts[0]); - } + + ensure!(useful_amounts[0] > 1_000, >::NoImportantValueFound); + + // Found the important value + ImportantValue::::put(&useful_amounts[0]); + // Emit event Self::deposit_event(Event::FoundVal(sender, useful_amounts[0])); Ok(().into()) From 64bdd33cd2a738396da1ab2722a253e1ccec4efa Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 8 Aug 2022 15:11:12 -0400 Subject: [PATCH 14/38] Comment update in pallet-dont-panic.rs --- .../substrate/dont-panic/pallet-dont-panic.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/not-so-smart-contracts/substrate/dont-panic/pallet-dont-panic.rs b/not-so-smart-contracts/substrate/dont-panic/pallet-dont-panic.rs index cc09ae6c..7321b7ec 100644 --- a/not-so-smart-contracts/substrate/dont-panic/pallet-dont-panic.rs +++ b/not-so-smart-contracts/substrate/dont-panic/pallet-dont-panic.rs @@ -47,7 +47,7 @@ pub mod pallet { /// Parameters: /// - `useful_amounts`: A vector of u64 values in which there is a important value. /// - /// Emits `DidWork` event when successful. + /// Emits `FoundVal` event when successful. #[pallet::weight(10_000)] pub fn find_important_value( origin: OriginFor, From 1ea500f41e9a7823612c75f54bbed117e3cf81bd Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 8 Aug 2022 16:23:13 -0400 Subject: [PATCH 15/38] Bad randomness example --- .../substrate/randomness/README.md | 4 + .../substrate/randomness/pallet-lottery.rs | 82 +++++++++++++++++++ 2 files changed, 86 insertions(+) create mode 100644 not-so-smart-contracts/substrate/randomness/README.md create mode 100644 not-so-smart-contracts/substrate/randomness/pallet-lottery.rs diff --git a/not-so-smart-contracts/substrate/randomness/README.md b/not-so-smart-contracts/substrate/randomness/README.md new file mode 100644 index 00000000..527c5772 --- /dev/null +++ b/not-so-smart-contracts/substrate/randomness/README.md @@ -0,0 +1,4 @@ +Bad randomness choice by directly using `random_seed`. Click [here](https://docs.substrate.io/main-docs/build/randomness/#security-properties) + + +"The first implementation provided by Substrate is the [Randomness Collective Flip Pallet](https://paritytech.github.io/substrate/master/pallet_randomness_collective_flip/index.html). This pallet is based on collective coin flipping. It is quite performant, but not very secure. This pallet should be used only when testing randomness-consuming pallets, not it production." \ No newline at end of file diff --git a/not-so-smart-contracts/substrate/randomness/pallet-lottery.rs b/not-so-smart-contracts/substrate/randomness/pallet-lottery.rs new file mode 100644 index 00000000..3db15eee --- /dev/null +++ b/not-so-smart-contracts/substrate/randomness/pallet-lottery.rs @@ -0,0 +1,82 @@ +#![cfg_attr(not(feature = "std"), no_std)] + +pub use pallet::*; + +#[frame_support::pallet] +pub mod pallet { + use frame_support::{dispatch::DispatchResultWithPostInfo, pallet_prelude::{*, ValueQuery}}; + use frame_system::pallet_prelude::*; + use frame_support::traits::Randomness; + use sp_std::prelude::*; + + /// Pallet configuration + #[pallet::config] + pub trait Config: frame_system::Config { + /// Because this pallet emits events, it depends on the runtime's definition of an event. + type Event: From> + IsType<::Event>; + /// Create a randomness type for this pallet + type MyRandomness: Randomness; + } + + #[pallet::pallet] + #[pallet::generate_store(pub(super) trait Store)] + pub struct Pallet(_); + + /// Storage item for nonce + #[pallet::storage] + #[pallet::getter(fn get_nonce)] + pub(super) type Nonce = StorageValue<_, u64, ValueQuery>; + + /// Storage item for current winner + #[pallet::storage] + #[pallet::getter(fn get_winner)] + pub(super) type Winner = StorageValue<_, T::AccountId, OptionQuery>; + + #[pallet::event] + #[pallet::generate_deposit(pub(super) fn deposit_event)] + pub enum Event { + /// Emit when new winner is found + NewWinner(T::AccountId), + } + + #[pallet::hooks] + impl Hooks> for Pallet {} + + #[pallet::error] + pub enum Error { + /// Guessed the wrong number. + IncorrectGuess, + } + + #[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); + + Self::deposit_event(Event::NewWinner(sender)); + + 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() + } + } +} \ No newline at end of file From f51030a0eb4dc08f849f880c18175e7bbefa7688 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 8 Aug 2022 16:34:16 -0400 Subject: [PATCH 16/38] Update folder names with underscores instead of hyphens --- .../substrate/{dont-panic => dont_panic}/pallet-dont-panic.rs | 0 .../substrate/{validate-unsigned => validate_unsigned}/README.md | 0 .../{validate-unsigned => validate_unsigned}/pallet-bad-oracle.rs | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename not-so-smart-contracts/substrate/{dont-panic => dont_panic}/pallet-dont-panic.rs (100%) rename not-so-smart-contracts/substrate/{validate-unsigned => validate_unsigned}/README.md (100%) rename not-so-smart-contracts/substrate/{validate-unsigned => validate_unsigned}/pallet-bad-oracle.rs (100%) diff --git a/not-so-smart-contracts/substrate/dont-panic/pallet-dont-panic.rs b/not-so-smart-contracts/substrate/dont_panic/pallet-dont-panic.rs similarity index 100% rename from not-so-smart-contracts/substrate/dont-panic/pallet-dont-panic.rs rename to not-so-smart-contracts/substrate/dont_panic/pallet-dont-panic.rs diff --git a/not-so-smart-contracts/substrate/validate-unsigned/README.md b/not-so-smart-contracts/substrate/validate_unsigned/README.md similarity index 100% rename from not-so-smart-contracts/substrate/validate-unsigned/README.md rename to not-so-smart-contracts/substrate/validate_unsigned/README.md diff --git a/not-so-smart-contracts/substrate/validate-unsigned/pallet-bad-oracle.rs b/not-so-smart-contracts/substrate/validate_unsigned/pallet-bad-oracle.rs similarity index 100% rename from not-so-smart-contracts/substrate/validate-unsigned/pallet-bad-oracle.rs rename to not-so-smart-contracts/substrate/validate_unsigned/pallet-bad-oracle.rs From 9579c7ffc46e7e92338c8005200a7dd2d42ccb30 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Fri, 12 Aug 2022 12:21:56 -0400 Subject: [PATCH 17/38] README for arithmetic_overflow complete --- .../substrate/arithmetic_overflow/README.md | 45 ++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/not-so-smart-contracts/substrate/arithmetic_overflow/README.md b/not-so-smart-contracts/substrate/arithmetic_overflow/README.md index 99a95992..957711c6 100644 --- a/not-so-smart-contracts/substrate/arithmetic_overflow/README.md +++ b/not-so-smart-contracts/substrate/arithmetic_overflow/README.md @@ -1 +1,44 @@ -There is an overflow/underflow issue for the `updated_from_balance` and `updated_to_balance`. \ No newline at end of file +# 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. + +# Example +In the [`pallet-overflow`](./pallet-overflow.rs) pallet, notice 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. + /// + /// Parameters: + /// - `to`: The account to receive the transfer. + /// - `amount`: The amount of balance to transfer. + /// + /// Emits `Transferred` event when successful. + #[pallet::weight(10_000)] + pub fn transfer( + origin: OriginFor, + to: T::AccountId, + amount: u64, + ) -> DispatchResultWithPostInfo { + let sender = ensure_signed(origin)?; + let sender_balance = Self::get_balance(&sender); + let receiver_balance = Self::get_balance(&to); + + // Calculate new balances. + let update_sender = sender_balance - amount; + let update_to = receiver_balance + amount; + [...] + } +``` + +The sender of the extrinsic can exploit this vulnerability by causing `update_sender` to underflow and artificially inflating their balance. + +**Note**: This toy example has additional issues like the fact that that there is no check whether the `sender` has enough tokens to transfer `amount`. However, for the sake of simplicity, this example is sufficient. + +# Mitigations +- Use `checked` or `saturating` functions for arithmetic operations that should not overflow. + - [`CheckedAdd` trait](https://docs.rs/num/0.4.0/num/traits/trait.CheckedAdd.html) + - [`Saturating` trait](https://docs.rs/num/0.4.0/num/traits/trait.Saturating.html) + +# References +- https://doc.rust-lang.org/book/ch03-02-data-types.html#integer-overflow +- https://docs.substrate.io/reference/how-to-guides/basics/use-helper-functions/ From 0aafb46d6c188df9a437bc4a88b38b9c55ebc2ff Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Fri, 12 Aug 2022 12:51:52 -0400 Subject: [PATCH 18/38] README for dont_panic complete --- .../substrate/dont_panic/README.md | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 not-so-smart-contracts/substrate/dont_panic/README.md diff --git a/not-so-smart-contracts/substrate/dont_panic/README.md b/not-so-smart-contracts/substrate/dont_panic/README.md new file mode 100644 index 00000000..55cdc97c --- /dev/null +++ b/not-so-smart-contracts/substrate/dont_panic/README.md @@ -0,0 +1,39 @@ +# 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. + +# Example +In the [`pallet-dont-panic`](./pallet-overflow.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]`. + +```rust + /// Do some work + /// + /// Parameters: + /// - `useful_amounts`: A vector of u64 values in which there is a important value. + /// + /// Emits `FoundVal` event when successful. + #[pallet::weight(10_000)] + pub fn find_important_value( + origin: OriginFor, + useful_amounts: Vec, + ) -> DispatchResultWithPostInfo { + let sender = ensure_signed(origin)?; + + ensure!(useful_amounts[0] > 1_000, >::NoImportantValueFound); + + // Found the important value + ImportantValue::::put(&useful_amounts[0]); + [...] + } +``` + +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. + +# 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. + +# References +- https://docs.substrate.io/main-docs/build/events-errors/#errors From bb64f06f39ae74e9434777d3c0fe1fba9575b339 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Fri, 12 Aug 2022 15:52:18 -0400 Subject: [PATCH 19/38] README for weights_and_fees complete --- .../substrate/weights_and_fees/README.md | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 not-so-smart-contracts/substrate/weights_and_fees/README.md diff --git a/not-so-smart-contracts/substrate/weights_and_fees/README.md b/not-so-smart-contracts/substrate/weights_and_fees/README.md new file mode 100644 index 00000000..0d14a75a --- /dev/null +++ b/not-so-smart-contracts/substrate/weights_and_fees/README.md @@ -0,0 +1,49 @@ +# 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 higher 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 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. + +# Example +In the [`pallet-bad-weights`](./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. + +```rust +impl WeighData<(&Vec,)> for MyWeightFunction { + fn weigh_data(&self, (amounts,): (&Vec,)) -> Weight { + self.0.saturating_mul(amounts.len() as u64).into() + } +} +``` + +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. + +One potential fix for this 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 { + if amounts.len() > 0 { + self.0.saturating_mul(amounts.len() as u64).into() + } else { + self.0 + } + } +} +``` + +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`). + +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`. + +**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. + +# 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. + +# References +- https://docs.substrate.io/main-docs/build/tx-weights-fees/ +- https://docs.substrate.io/reference/how-to-guides/weights/add-benchmarks/ +- https://docs.substrate.io/reference/how-to-guides/weights/use-custom-weights/ +- https://docs.substrate.io/reference/how-to-guides/weights/use-conditional-weights/ \ No newline at end of file From a7a95c87046afb4bd5b18b46e31f72a34f244c32 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Fri, 12 Aug 2022 16:09:15 -0400 Subject: [PATCH 20/38] Renamed runtime_storage folder to verify_first folder --- .../substrate/{runtime_storage => verify_first}/README.md | 0 .../{runtime_storage => verify_first}/pallet-verify-first.rs | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename not-so-smart-contracts/substrate/{runtime_storage => verify_first}/README.md (100%) rename not-so-smart-contracts/substrate/{runtime_storage => verify_first}/pallet-verify-first.rs (100%) diff --git a/not-so-smart-contracts/substrate/runtime_storage/README.md b/not-so-smart-contracts/substrate/verify_first/README.md similarity index 100% rename from not-so-smart-contracts/substrate/runtime_storage/README.md rename to not-so-smart-contracts/substrate/verify_first/README.md diff --git a/not-so-smart-contracts/substrate/runtime_storage/pallet-verify-first.rs b/not-so-smart-contracts/substrate/verify_first/pallet-verify-first.rs similarity index 100% rename from not-so-smart-contracts/substrate/runtime_storage/pallet-verify-first.rs rename to not-so-smart-contracts/substrate/verify_first/pallet-verify-first.rs From 813d9cad3f16a9344fd102c04b2e2b5fac3a3ac7 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Fri, 12 Aug 2022 16:23:40 -0400 Subject: [PATCH 21/38] Add link to README for weights_and_fees --- not-so-smart-contracts/substrate/weights_and_fees/README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 0d14a75a..6fe8db67 100644 --- a/not-so-smart-contracts/substrate/weights_and_fees/README.md +++ b/not-so-smart-contracts/substrate/weights_and_fees/README.md @@ -46,4 +46,5 @@ On the other hand, if an attacker sends a `useful_amounts` vector that is incred - https://docs.substrate.io/main-docs/build/tx-weights-fees/ - https://docs.substrate.io/reference/how-to-guides/weights/add-benchmarks/ - https://docs.substrate.io/reference/how-to-guides/weights/use-custom-weights/ -- https://docs.substrate.io/reference/how-to-guides/weights/use-conditional-weights/ \ No newline at end of file +- https://docs.substrate.io/reference/how-to-guides/weights/use-conditional-weights/ +- https://www.shawntabrizi.com/substrate/substrate-weight-and-fees/ \ No newline at end of file From 97ec3bf780df18e215ea76561293410c31b7448f Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Fri, 12 Aug 2022 16:47:08 -0400 Subject: [PATCH 22/38] README for verify_first complete --- .../substrate/verify_first/README.md | 56 +++++++++++++++++-- 1 file changed, 52 insertions(+), 4 deletions(-) diff --git a/not-so-smart-contracts/substrate/verify_first/README.md b/not-so-smart-contracts/substrate/verify_first/README.md index 76201fe0..c8e373da 100644 --- a/not-so-smart-contracts/substrate/verify_first/README.md +++ b/not-so-smart-contracts/substrate/verify_first/README.md @@ -1,6 +1,54 @@ -Goal here is to show that state changes should not be made before all potentially reverting operations have been validated: -Verify first, write last principle from [here](https://docs.substrate.io/main-docs/build/runtime-storage/) +# Verify First, Write Last -Also, the mint function in this pallet is public, which can be problematic in itself. +**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. -Can make this not-so-smart-pallet contain two issues, I guess \ No newline at end of file +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. + +```rust +{ + // all checks and throwing code go here + + // ** no throwing code below this line ** + + // all event emissions & storage writes go here +} +``` +# Example +In the [`pallet-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`. + +```rust +/// Initialize the token +/// Transfers the total_supply amount to the caller +/// If init() has already been called, throw AlreadyInitialized error +#[pallet::weight(10_000)] +pub fn init( + origin: OriginFor, + supply: u64 +) -> DispatchResultWithPostInfo { + let sender = ensure_signed(origin)?; + + if supply > 0 { + >::put(&supply); + } + // Set sender's balance to total_supply() + >::insert(&sender, supply); + + // Revert above changes if init() has already been called + ensure!(!Self::is_init(), >::AlreadyInitialized); + + // Set Init StorageValue to `true` + Init::::put(true); + + // Emit event + Self::deposit_event(Event::Initialized(sender)); + + Ok(().into()) +} +``` + +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. + +# Mitigations +- Follow the "Verify First, Write Last" practice by doing all the necessary data validation before performing state changes and emitting events. +# References +- https://docs.substrate.io/main-docs/build/runtime-storage/#best-practices \ No newline at end of file From 89e542d12452542fd5a0543030250d8f8f29b8ba Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 15 Aug 2022 10:30:27 -0400 Subject: [PATCH 23/38] Rename pallet-bad-oracle to pallet-bad-unsigned --- .../{pallet-bad-oracle.rs => pallet-bad-unsigned.rs} | 1 - 1 file changed, 1 deletion(-) rename not-so-smart-contracts/substrate/validate_unsigned/{pallet-bad-oracle.rs => pallet-bad-unsigned.rs} (99%) diff --git a/not-so-smart-contracts/substrate/validate_unsigned/pallet-bad-oracle.rs b/not-so-smart-contracts/substrate/validate_unsigned/pallet-bad-unsigned.rs similarity index 99% rename from not-so-smart-contracts/substrate/validate_unsigned/pallet-bad-oracle.rs rename to not-so-smart-contracts/substrate/validate_unsigned/pallet-bad-unsigned.rs index 83dea80b..034b6685 100644 --- a/not-so-smart-contracts/substrate/validate_unsigned/pallet-bad-oracle.rs +++ b/not-so-smart-contracts/substrate/validate_unsigned/pallet-bad-unsigned.rs @@ -8,7 +8,6 @@ use frame_system::{ use sp_runtime::{ transaction_validity::{InvalidTransaction, TransactionValidity, ValidTransaction}, }; -const UNSIGNED_TXS_PRIORITY: u64 = 110; pub use pallet::*; From f402901c8f1eded4d3500a4087a6fe48d8cc3970 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 15 Aug 2022 11:42:08 -0400 Subject: [PATCH 24/38] README for validate_unsigned complete --- .../substrate/validate_unsigned/README.md | 75 +++++++++++-------- 1 file changed, 44 insertions(+), 31 deletions(-) diff --git a/not-so-smart-contracts/substrate/validate_unsigned/README.md b/not-so-smart-contracts/substrate/validate_unsigned/README.md index 382491fc..5bb32a70 100644 --- a/not-so-smart-contracts/substrate/validate_unsigned/README.md +++ b/not-so-smart-contracts/substrate/validate_unsigned/README.md @@ -1,36 +1,49 @@ -Some code snippets I will need for the README.md -```rust - fn validate_transaction_parameters( - _block_number: &T::BlockNumber, - _new_price: &u32, - ) -> TransactionValidity { - let valid_tx = | provide | { - ValidTransaction::with_tag_prefix("ExampleOffchainWorker") - .priority(UNSIGNED_TXS_PRIORITY) - .and_provides([&provide]) - .longevity(5) - .propagate(true) - .build() - }; - valid_tx(b"submit_price_unsigned".to_vec()) - } -``` +# 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. The validation of an unsigned transaction must be provided by the pallet that chooses to accept them. One of the main use cases for unsigned transactions is for off-chain workers (OCWs). OCWs can use unsigned transactions to submit data about some computation that was performed off-chain. + +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 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. + +# Example +The [`pallet-bad-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. ```rust - if let Call::submit_price_unsigned_with_signed_payload { - price_payload: ref payload, - ref signature, - } = call - { - let signature_valid = - SignedPayload::::verify::(payload, signature.clone()); - if !signature_valid { - return InvalidTransaction::BadProof.into() - } - Self::validate_transaction_parameters(&payload.block_number, &payload.price) - } else if let Call::submit_price_unsigned { block_number, price: new_price } = call { - Self::validate_transaction_parameters(block_number, new_price) +/// 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. +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. + if let Call::submit_price_unsigned { block_number, price: new_price } = call { + let avg_price = Self::average_price() + .map(|price| if &price > new_price { price - new_price } else { new_price - price }) + .unwrap_or(0); + + let valid_tx = | provide | { + ValidTransaction::with_tag_prefix("ExampleOffchainWorker") + .priority(T::UnsignedPriority::get().saturating_add(avg_price as _)) + .and_provides([&provide]) + .longevity(5) + .propagate(true) + .build() + }; + valid_tx(b"submit_price_unsigned".to_vec()) } else { InvalidTransaction::Call.into() } -``` \ No newline at end of file +} +``` + +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 (or the past few blocks) 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 transaction. + +# 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. +# References +- https://docs.substrate.io/main-docs/fundamentals/transaction-types/#unsigned-transactions +- https://docs.substrate.io/main-docs/fundamentals/offchain-operations/ +- https://github.com/paritytech/substrate/blob/polkadot-v0.9.26/frame/examples/offchain-worker/src/lib.rs +- https://docs.substrate.io/reference/how-to-guides/offchain-workers/offchain-transactions/ +- https://docs.substrate.io/reference/how-to-guides/offchain-workers/offchain-http-requests/ \ No newline at end of file From 600366645e5d116d9e5e736d1f77bd5885d8dd28 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 15 Aug 2022 11:43:22 -0400 Subject: [PATCH 25/38] Add source for pallet-bad-unsigned.rs --- .../substrate/validate_unsigned/pallet-bad-unsigned.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/not-so-smart-contracts/substrate/validate_unsigned/pallet-bad-unsigned.rs b/not-so-smart-contracts/substrate/validate_unsigned/pallet-bad-unsigned.rs index 034b6685..167086ab 100644 --- a/not-so-smart-contracts/substrate/validate_unsigned/pallet-bad-unsigned.rs +++ b/not-so-smart-contracts/substrate/validate_unsigned/pallet-bad-unsigned.rs @@ -1,3 +1,5 @@ +/// Source: https://github.com/paritytech/substrate/blob/e8a7d161f39db70cb27fdad6c6e215cf493ebc3b/frame/examples/offchain-worker/src/lib.rs +/// This example was re-purposed from the above source #![cfg_attr(not(feature = "std"), no_std)] use frame_support::traits::Get; From 538a205eadcb3c5a97e59113518d5e297ed2332a Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 15 Aug 2022 14:36:15 -0400 Subject: [PATCH 26/38] Rename pallet-lottery.rs to pallet-bad-lottery.rs --- .../randomness/{pallet-lottery.rs => pallet-bad-lottery.rs} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename not-so-smart-contracts/substrate/randomness/{pallet-lottery.rs => pallet-bad-lottery.rs} (100%) diff --git a/not-so-smart-contracts/substrate/randomness/pallet-lottery.rs b/not-so-smart-contracts/substrate/randomness/pallet-bad-lottery.rs similarity index 100% rename from not-so-smart-contracts/substrate/randomness/pallet-lottery.rs rename to not-so-smart-contracts/substrate/randomness/pallet-bad-lottery.rs From 1d76af8b92695e44d50c5f5b92106827baee9a72 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 15 Aug 2022 14:44:33 -0400 Subject: [PATCH 27/38] README for randomness complete --- .../substrate/randomness/README.md | 57 ++++++++++++++++++- 1 file changed, 55 insertions(+), 2 deletions(-) diff --git a/not-so-smart-contracts/substrate/randomness/README.md b/not-so-smart-contracts/substrate/randomness/README.md index 527c5772..1b4a741a 100644 --- a/not-so-smart-contracts/substrate/randomness/README.md +++ b/not-so-smart-contracts/substrate/randomness/README.md @@ -1,4 +1,57 @@ -Bad randomness choice by directly using `random_seed`. Click [here](https://docs.substrate.io/main-docs/build/randomness/#security-properties) +# 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 gaining 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. -"The first implementation provided by Substrate is the [Randomness Collective Flip Pallet](https://paritytech.github.io/substrate/master/pallet_randomness_collective_flip/index.html). This pallet is based on collective coin flipping. It is quite performant, but not very secure. This pallet should be used only when testing randomness-consuming pallets, not it production." \ No newline at end of file +Substrate provides the [Randomness Collective Flip Pallet](https://paritytech.github.io/substrate/master/pallet_randomness_collective_flip/index.html) 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. + + +# Example +The [`pallet-bad-lottery`](./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. + +```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); + + Self::deposit_event(Event::NewWinner(sender)); + + 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() +} +} +``` + +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. 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. + + +# References +- https://docs.substrate.io/main-docs/build/randomness/ +- https://docs.substrate.io/reference/how-to-guides/pallet-design/incorporate-randomness/ \ No newline at end of file From b3c5fbf4f1180910baecdda0adb1909fc97fe812 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 15 Aug 2022 14:53:37 -0400 Subject: [PATCH 28/38] README for NSS pallets --- not-so-smart-contracts/substrate/README.md | 30 ++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 not-so-smart-contracts/substrate/README.md diff --git a/not-so-smart-contracts/substrate/README.md b/not-so-smart-contracts/substrate/README.md new file mode 100644 index 00000000..c7efe6cb --- /dev/null +++ b/not-so-smart-contracts/substrate/README.md @@ -0,0 +1,30 @@ +# (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. + +## 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 +* A mock pallet that exhibits the flaw +* References to third-party resources with more information + +## Vulnerabilities + +| Not So Smart Pallet | Description | +| --- | --- | +| [Arithmetic overflow](./arithmetic_overflow/README.md) | Integer overflow due to incorrect use of arithmetic operators | +| [Don't panic!](./dont_panic/README.md) | System panics create a potential DoS attack vector | +| [Weights and fees](./weights_and_fees/README.md) | Incorrect weight calculations can create a potential DoS attack vector | +| [Verify first](./verify_first/README.md) | Verify first, write last | +| [Unsigned transaction validation](./validate_unsigned/README.md) | Insufficient validation of unsigned transactions | +| [Bad randomness](./randomness/README.md) | Unsafe sources of randomness in Substrate | + +## Credits + +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. \ No newline at end of file From bafac138fcd26e20eec8c4904bada33efd3343e8 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 15 Aug 2022 16:27:17 -0400 Subject: [PATCH 29/38] Minor copyediting changes in all READMEs --- .../substrate/arithmetic_overflow/README.md | 4 ++-- not-so-smart-contracts/substrate/dont_panic/README.md | 2 +- not-so-smart-contracts/substrate/randomness/README.md | 2 +- not-so-smart-contracts/substrate/validate_unsigned/README.md | 4 ++-- not-so-smart-contracts/substrate/weights_and_fees/README.md | 5 ++--- 5 files changed, 8 insertions(+), 9 deletions(-) diff --git a/not-so-smart-contracts/substrate/arithmetic_overflow/README.md b/not-so-smart-contracts/substrate/arithmetic_overflow/README.md index 957711c6..8d0c383f 100644 --- a/not-so-smart-contracts/substrate/arithmetic_overflow/README.md +++ b/not-so-smart-contracts/substrate/arithmetic_overflow/README.md @@ -30,12 +30,12 @@ In the [`pallet-overflow`](./pallet-overflow.rs) pallet, notice that the `transf } ``` -The sender of the extrinsic can exploit this vulnerability by causing `update_sender` to underflow and artificially inflating their balance. +The sender of the extrinsic can exploit this vulnerability by causing `update_sender` to underflow, which artificially inflates their balance. **Note**: This toy example has additional issues like the fact that that there is no check whether the `sender` has enough tokens to transfer `amount`. However, for the sake of simplicity, this example is sufficient. # Mitigations -- Use `checked` or `saturating` functions for arithmetic operations that should not overflow. +- Use `checked` or `saturating` functions for arithmetic operations. - [`CheckedAdd` trait](https://docs.rs/num/0.4.0/num/traits/trait.CheckedAdd.html) - [`Saturating` trait](https://docs.rs/num/0.4.0/num/traits/trait.Saturating.html) diff --git a/not-so-smart-contracts/substrate/dont_panic/README.md b/not-so-smart-contracts/substrate/dont_panic/README.md index 55cdc97c..45a36467 100644 --- a/not-so-smart-contracts/substrate/dont_panic/README.md +++ b/not-so-smart-contracts/substrate/dont_panic/README.md @@ -3,7 +3,7 @@ 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. # Example -In the [`pallet-dont-panic`](./pallet-overflow.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`](./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]`. ```rust /// Do some work diff --git a/not-so-smart-contracts/substrate/randomness/README.md b/not-so-smart-contracts/substrate/randomness/README.md index 1b4a741a..0ef41e1e 100644 --- a/not-so-smart-contracts/substrate/randomness/README.md +++ b/not-so-smart-contracts/substrate/randomness/README.md @@ -1,6 +1,6 @@ # 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 gaining 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. 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. diff --git a/not-so-smart-contracts/substrate/validate_unsigned/README.md b/not-so-smart-contracts/substrate/validate_unsigned/README.md index 5bb32a70..d8a18f60 100644 --- a/not-so-smart-contracts/substrate/validate_unsigned/README.md +++ b/not-so-smart-contracts/substrate/validate_unsigned/README.md @@ -34,9 +34,9 @@ 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 (or the past few blocks) 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. +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 transaction. +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. # 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. 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 6fe8db67..26fd269c 100644 --- a/not-so-smart-contracts/substrate/weights_and_fees/README.md +++ b/not-so-smart-contracts/substrate/weights_and_fees/README.md @@ -1,6 +1,6 @@ # 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 higher 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 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 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. @@ -35,8 +35,7 @@ In the example above, if the length of `amounts` (i.e. `useful_amounts`) is zero 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`. -**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 also be carefully evaluated and tested to ensure that the risk of DoS attacks is mitigated. # 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. From 67abdec89e2cb0e4b3cb1fb0d7b86de35c5170ed Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 15 Aug 2022 17:35:52 -0400 Subject: [PATCH 30/38] Added bad origins pallet and README --- .../substrate/origins/README.md | 56 +++++++++++++++++ .../substrate/origins/pallet-bad-origin.rs | 61 +++++++++++++++++++ 2 files changed, 117 insertions(+) create mode 100644 not-so-smart-contracts/substrate/origins/README.md create mode 100644 not-so-smart-contracts/substrate/origins/pallet-bad-origin.rs diff --git a/not-so-smart-contracts/substrate/origins/README.md b/not-so-smart-contracts/substrate/origins/README.md new file mode 100644 index 00000000..a5ca4a73 --- /dev/null +++ b/not-so-smart-contracts/substrate/origins/README.md @@ -0,0 +1,56 @@ +# Origins + +The origin of a call tells a dispatchable function where the call has come from. There are three types of origins that can used in the runtime: + +```rust +pub enum RawOrigin { + Root, + Signed(AccountId), + None, +} +``` + +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`. + +# Example +In the [`pallet-bad-origin`](./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`. + +```rust +#[pallet::call] +impl Pallet { + /// Set the important val + /// Should be only callable by ForceOrigin + #[pallet::weight(10_000)] + pub fn set_important_val( + origin: OriginFor, + new_val: u64 + ) -> DispatchResultWithPostInfo { + let sender = ensure_signed(origin)?; + // Change to new value + >::put(new_val); + + // Emit event + Self::deposit_event(Event::ImportantValSet(sender, new_val)); + + Ok(().into()) + } +} +``` +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: + +```rust +T::ForceOrigin::ensure_origin(origin.clone())?; +let sender = ensure_signed(origin)?; +``` + + + +# Mitigations +- Ensure that the correct access controls are placed on privileged functions. +- A thorough suite of unit tests should be implemented. + +# References +- https://docs.substrate.io/main-docs/build/origins/ +- https://docs.substrate.io/tutorials/work-with-pallets/specify-the-origin-for-a-call/ +- https://paritytech.github.io/substrate/master/pallet_sudo/index.html# +- https://paritytech.github.io/substrate/master/pallet_democracy/index.html diff --git a/not-so-smart-contracts/substrate/origins/pallet-bad-origin.rs b/not-so-smart-contracts/substrate/origins/pallet-bad-origin.rs new file mode 100644 index 00000000..1b84bc4d --- /dev/null +++ b/not-so-smart-contracts/substrate/origins/pallet-bad-origin.rs @@ -0,0 +1,61 @@ +#![cfg_attr(not(feature = "std"), no_std)] + +pub use pallet::*; + +#[frame_support::pallet] +pub mod pallet { + use frame_support::{dispatch::DispatchResultWithPostInfo, pallet_prelude::{*, ValueQuery}}; + use frame_system::pallet_prelude::*; + use sp_std::prelude::*; + + /// Pallet configuration + #[pallet::config] + pub trait Config: frame_system::Config { + /// Because this pallet emits events, it depends on the runtime's definition of an event. + type Event: From> + IsType<::Event>; + + // Specifies the account that can perform some action + type ForceOrigin: EnsureOrigin; + } + + #[pallet::pallet] + #[pallet::generate_store(pub(super) trait Store)] + pub struct Pallet(_); + + /// Storage item for important value that should be editable only by root + #[pallet::storage] + #[pallet::getter(fn get_important_val)] + pub(super) type ImportantVal = StorageValue<_, u64, ValueQuery>; + + #[pallet::event] + #[pallet::generate_deposit(pub(super) fn deposit_event)] + pub enum Event { + /// Emit when new important val is set. + ImportantValSet(T::AccountId, u64), + } + + #[pallet::hooks] + impl Hooks> for Pallet {} + + #[pallet::call] + impl Pallet { + /// Set the important val + /// Should be only callable by ForceOrigin + #[pallet::weight(10_000)] + pub fn set_important_val( + origin: OriginFor, + new_val: u64 + ) -> DispatchResultWithPostInfo { + T::ForceOrigin::ensure_origin(origin.clone())?; + let sender = ensure_signed(origin)?; + // Change to new value + >::put(new_val); + + // Emit event + Self::deposit_event(Event::ImportantValSet(sender, new_val)); + + Ok(().into()) + } + } + +} \ No newline at end of file From d0bc8815c3590347fc349254ff180f4d66d8398d Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Mon, 15 Aug 2022 17:37:38 -0400 Subject: [PATCH 31/38] Updated NSS pallet README --- not-so-smart-contracts/substrate/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/not-so-smart-contracts/substrate/README.md b/not-so-smart-contracts/substrate/README.md index c7efe6cb..9896200b 100644 --- a/not-so-smart-contracts/substrate/README.md +++ b/not-so-smart-contracts/substrate/README.md @@ -22,6 +22,7 @@ Each _Not So Smart Pallet_ includes a standard set of information: | [Verify first](./verify_first/README.md) | Verify first, write last | | [Unsigned transaction validation](./validate_unsigned/README.md) | Insufficient validation of unsigned transactions | | [Bad randomness](./randomness/README.md) | Unsafe sources of randomness in Substrate | +| [Bad origin](./origins/README.md) | Incorrect use of call origin can lead to bypassing access controls | ## Credits From df5dfe940caf957b9df692f885d3ba3bd8991c91 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Thu, 18 Aug 2022 15:54:02 -0400 Subject: [PATCH 32/38] Fix bad origins README --- not-so-smart-contracts/substrate/origins/README.md | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/not-so-smart-contracts/substrate/origins/README.md b/not-so-smart-contracts/substrate/origins/README.md index a5ca4a73..111177d8 100644 --- a/not-so-smart-contracts/substrate/origins/README.md +++ b/not-so-smart-contracts/substrate/origins/README.md @@ -1,6 +1,8 @@ # Origins -The origin of a call tells a dispatchable function where the call has come from. There are three types of origins that can used in the runtime: +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. + +There are three types of origins that can used in the runtime: ```rust pub enum RawOrigin { @@ -10,7 +12,9 @@ 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`. +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`. + +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`. # Example In the [`pallet-bad-origin`](./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`. @@ -43,11 +47,10 @@ T::ForceOrigin::ensure_origin(origin.clone())?; let sender = ensure_signed(origin)?; ``` - - # Mitigations - Ensure that the correct access controls are placed on privileged functions. -- A thorough suite of unit tests should be implemented. +- 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. # References - https://docs.substrate.io/main-docs/build/origins/ From 5097302a5e72651e28460d3b95620fd847ec8c44 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Thu, 18 Aug 2022 15:54:29 -0400 Subject: [PATCH 33/38] Fix bad randomness README --- not-so-smart-contracts/substrate/randomness/README.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/not-so-smart-contracts/substrate/randomness/README.md b/not-so-smart-contracts/substrate/randomness/README.md index 0ef41e1e..9d932a88 100644 --- a/not-so-smart-contracts/substrate/randomness/README.md +++ b/not-so-smart-contracts/substrate/randomness/README.md @@ -6,6 +6,7 @@ To use randomness in a Substrate pallet, all you need to do is require a source Substrate provides the [Randomness Collective Flip Pallet](https://paritytech.github.io/substrate/master/pallet_randomness_collective_flip/index.html) 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. +A bad source of randomness can lead to a variety of exploits such as the theft of funds or undefined system behavior. # Example The [`pallet-bad-lottery`](./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. @@ -44,7 +45,7 @@ pub fn get_and_increment_nonce() -> Vec { } ``` -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. 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 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." # 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.**" @@ -54,4 +55,7 @@ Note that the quality of randomness provided to the `pallet-bad-lottery` pallet # References - https://docs.substrate.io/main-docs/build/randomness/ -- https://docs.substrate.io/reference/how-to-guides/pallet-design/incorporate-randomness/ \ No newline at end of file +- https://docs.substrate.io/reference/how-to-guides/pallet-design/incorporate-randomness/ +- https://ethresear.ch/t/rng-exploitability-analysis-assuming-pure-randao-based-main-chain/1825/7 +- https://ethresear.ch/t/collective-coin-flipping-csprng/3252/21 +- https://github.com/paritytech/ink/issues/57#issuecomment-486998848 \ No newline at end of file From fb19932986da81594cd56ccf3522c58ab547e6eb Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Thu, 18 Aug 2022 16:02:58 -0400 Subject: [PATCH 34/38] Fix validate unsigned README --- not-so-smart-contracts/substrate/validate_unsigned/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/not-so-smart-contracts/substrate/validate_unsigned/README.md b/not-so-smart-contracts/substrate/validate_unsigned/README.md index d8a18f60..50051c08 100644 --- a/not-so-smart-contracts/substrate/validate_unsigned/README.md +++ b/not-so-smart-contracts/substrate/validate_unsigned/README.md @@ -1,8 +1,8 @@ # 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. The validation of an unsigned transaction must be provided by the pallet that chooses to accept them. One of the main use cases for unsigned transactions is for off-chain workers (OCWs). OCWs can use unsigned transactions to submit data about some computation that was performed off-chain. +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. -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 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. +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 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. # Example From a4fc1bd39a6dd24e5a305d1287e8e9c95860e192 Mon Sep 17 00:00:00 2001 From: Anish Naik Date: Thu, 18 Aug 2022 16:13:46 -0400 Subject: [PATCH 35/38] Fix weights and fees README --- not-so-smart-contracts/substrate/weights_and_fees/README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 26fd269c..00182f59 100644 --- a/not-so-smart-contracts/substrate/weights_and_fees/README.md +++ b/not-so-smart-contracts/substrate/weights_and_fees/README.md @@ -4,6 +4,8 @@ Weights and transaction fees are the two main ways to regulate the consumption o 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. +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. + # Example In the [`pallet-bad-weights`](./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. @@ -23,7 +25,7 @@ One potential fix for this is to set a fixed weight if the length of `useful_amo impl WeighData<(&Vec,)> for MyWeightFunction { fn weigh_data(&self, (amounts,): (&Vec,)) -> Weight { if amounts.len() > 0 { - self.0.saturating_mul(amounts.len() as u64).into() + self.0 + self.0.saturating_mul(amounts.len() as u64).into() } else { self.0 } From 49e23bc5830eca760b2b45f5209a6b5fe459c668 Mon Sep 17 00:00:00 2001 From: Natalie Chin Date: Thu, 27 Oct 2022 14:33:27 -0400 Subject: [PATCH 36/38] minor doc changes --- .../substrate/dont_panic/pallet-dont-panic.rs | 2 +- not-so-smart-contracts/substrate/validate_unsigned/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/not-so-smart-contracts/substrate/dont_panic/pallet-dont-panic.rs b/not-so-smart-contracts/substrate/dont_panic/pallet-dont-panic.rs index 7321b7ec..caf87898 100644 --- a/not-so-smart-contracts/substrate/dont_panic/pallet-dont-panic.rs +++ b/not-so-smart-contracts/substrate/dont_panic/pallet-dont-panic.rs @@ -42,7 +42,7 @@ pub mod pallet { #[pallet::call] impl Pallet { - /// Do some work + /// Find important value /// /// Parameters: /// - `useful_amounts`: A vector of u64 values in which there is a important value. diff --git a/not-so-smart-contracts/substrate/validate_unsigned/README.md b/not-so-smart-contracts/substrate/validate_unsigned/README.md index 50051c08..d1a85e5b 100644 --- a/not-so-smart-contracts/substrate/validate_unsigned/README.md +++ b/not-so-smart-contracts/substrate/validate_unsigned/README.md @@ -2,7 +2,7 @@ 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. -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 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. +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. # Example From 5e84c96e7c86c9941150626063244c1e1abd1f3d Mon Sep 17 00:00:00 2001 From: anishnaik Date: Fri, 28 Oct 2022 14:09:40 -0400 Subject: [PATCH 37/38] fix note --- not-so-smart-contracts/substrate/arithmetic_overflow/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/not-so-smart-contracts/substrate/arithmetic_overflow/README.md b/not-so-smart-contracts/substrate/arithmetic_overflow/README.md index 8d0c383f..1d7adfb9 100644 --- a/not-so-smart-contracts/substrate/arithmetic_overflow/README.md +++ b/not-so-smart-contracts/substrate/arithmetic_overflow/README.md @@ -32,7 +32,7 @@ In the [`pallet-overflow`](./pallet-overflow.rs) pallet, notice that the `transf The sender of the extrinsic can exploit this vulnerability by causing `update_sender` to underflow, which artificially inflates their balance. -**Note**: This toy example has additional issues like the fact that that there is no check whether the `sender` has enough tokens to transfer `amount`. However, for the sake of simplicity, this example is sufficient. +**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. # Mitigations - Use `checked` or `saturating` functions for arithmetic operations. From 2f7851363dfd41e8e830b7d9a693c1d739261400 Mon Sep 17 00:00:00 2001 From: anishnaik Date: Thu, 3 Nov 2022 10:02:32 -0400 Subject: [PATCH 38/38] fix weight function --- .../substrate/weights_and_fees/README.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) 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 00182f59..41741fd4 100644 --- a/not-so-smart-contracts/substrate/weights_and_fees/README.md +++ b/not-so-smart-contracts/substrate/weights_and_fees/README.md @@ -24,11 +24,9 @@ One potential fix for this is to set a fixed weight if the length of `useful_amo ```rust impl WeighData<(&Vec,)> for MyWeightFunction { fn weigh_data(&self, (amounts,): (&Vec,)) -> Weight { - if amounts.len() > 0 { - self.0 + self.0.saturating_mul(amounts.len() as u64).into() - } else { - self.0 - } + // 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`) + self.0 + self.0.saturating_mul(amounts.len() as u64).into() } } ```