[WIP] Not-so-smart pallets (#124)

* initial commit to update e2e documentation

* config option fix

* added text about etheno edge cases

* docker instructions improved

* 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

* Remove transfer function from mint.rs

* 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

* Changed comment

TotalSupplyDefaultValue is not the actual total supply but instead the default one. So changed comment to note that accordingly.

* 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.

* 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.

* Initial commit for validate unsigned transactions

* Adding example where pallet panics instead of proper error handling

* 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

* Comment update in pallet-dont-panic.rs

* Bad randomness example

* Update folder names with underscores instead of hyphens

* README for arithmetic_overflow complete

* README for dont_panic complete

* README for weights_and_fees complete

* Renamed runtime_storage folder to verify_first folder

* Add link to README for weights_and_fees

* README for verify_first complete

* Rename pallet-bad-oracle to pallet-bad-unsigned

* README for validate_unsigned complete

* Add source for pallet-bad-unsigned.rs

* Rename pallet-lottery.rs to pallet-bad-lottery.rs

* README for randomness complete

* README for NSS pallets

* Minor copyediting changes in all READMEs

* Added bad origins pallet and README

* Updated NSS pallet README

* Fix bad origins README

* Fix bad randomness README

* Fix validate unsigned README

* Fix weights and fees README

* minor doc changes

* fix note

* fix weight function

Co-authored-by: Natalie Chin <natalie.chin@trailofbits.com>

Author: anishnaik

not-so-smart-contracts/substrate/README.md

@@ -0,0 +1,31 @@
+# (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 |
+| [Bad origin](./origins/README.md) | Incorrect use of call origin can lead to bypassing access controls |
+
+## 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.

not-so-smart-contracts/substrate/arithmetic_overflow/README.md

@@ -0,0 +1,44 @@
+# 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<T>,
+        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, which artificially inflates their balance. 
+
+**Note**: Aside from the stronger mitigations mentioned below, a check to make sure that `sender` has at least `amount` balance would have also prevented an underflow.
+
+# Mitigations
+- 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)
+
+# 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/

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<Event<Self>> + IsType<<Self as frame_system::Config>::Event>;
+	}
+
+	#[pallet::pallet]
+	#[pallet::generate_store(pub(super) trait Store)]
+	pub struct Pallet<T>(_);
+
+	// DEFAULT total supply of tokens
+	#[pallet::type_value]
+	pub(super) fn TotalSupplyDefaultValue<T: Config>() -> u64 {
+		1000
+	}
+
+	// Data structure that holds the total supply of tokens
+	#[pallet::storage]
+	#[pallet::getter(fn total_supply)]
+	pub(super) type TotalSupply<T: Config> =
+		StorageValue<_, u64, ValueQuery, TotalSupplyDefaultValue<T>>;
+
+	// 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<T: Config> = StorageValue<_, bool, ValueQuery>;
+
+	/// Storage item for balances to accounts mapping.
+	#[pallet::storage]
+	#[pallet::getter(fn get_balance)]
+	pub(super) type BalanceToAccount<T: Config> = 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<T: Config> {
+		/// 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<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {}
+
+	#[pallet::error]
+	pub enum Error<T> {
+		/// Attempted to initialize the token after it had already been initialized.
+		AlreadyInitialized,
+	}
+	
+	#[pallet::call]
+	impl<T:Config> Pallet<T> {
+		/// 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<T>) -> DispatchResultWithPostInfo {
+			let sender = ensure_signed(origin)?;
+			ensure!(!Self::is_init(), <Error<T>>::AlreadyInitialized);
+
+			<BalanceToAccount<T>>::insert(sender, Self::total_supply());
+
+			Init::<T>::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<T>,
+			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.
+			<BalanceToAccount<T>>::insert(&sender, update_sender);
+			<BalanceToAccount<T>>::insert(&to, update_to);
+
+			// Emit event.
+			Self::deposit_event(Event::Transferred(sender, to, amount));
+			Ok(().into())
+		}
+	}
+}

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-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
+    ///
+    /// 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<T>,
+        useful_amounts: Vec<u64>,
+    ) -> DispatchResultWithPostInfo {
+        let sender = ensure_signed(origin)?;
+
+        ensure!(useful_amounts[0] > 1_000, <Error<T>>::NoImportantValueFound);
+        
+        // Found the important value
+        ImportantValue::<T>::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

not-so-smart-contracts/substrate/dont_panic/pallet-dont-panic.rs

@@ -0,0 +1,68 @@
+#![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::*;
+	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<Event<Self>> + IsType<<Self as frame_system::Config>::Event>;
+	}
+
+	#[pallet::pallet]
+	#[pallet::without_storage_info]
+	#[pallet::generate_store(pub(super) trait Store)]
+	pub struct Pallet<T>(_);
+
+	/// Storage item for holding an ImportantValue
+	#[pallet::storage]
+	#[pallet::getter(fn get_val)]
+	pub(super) type ImportantValue<T: Config> = StorageValue<_, u64, ValueQuery>;
+
+	#[pallet::event]
+	#[pallet::generate_deposit(pub(super) fn deposit_event)]
+	pub enum Event<T: Config> {
+		/// Emit after val is found
+		FoundVal(T::AccountId, u64),
+	}
+
+	#[pallet::hooks]
+	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {}
+
+    #[pallet::error]
+	pub enum Error<T> {
+		NoImportantValueFound,
+	}
+
+	#[pallet::call]
+	impl<T:Config> Pallet<T> {
+		/// Find important value
+		///
+		/// 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<T>,
+			useful_amounts: Vec<u64>,
+		) -> DispatchResultWithPostInfo {
+			let sender = ensure_signed(origin)?;
+
+			ensure!(useful_amounts[0] > 1_000, <Error<T>>::NoImportantValueFound);
+			
+			// Found the important value
+			ImportantValue::<T>::put(&useful_amounts[0]);
+						
+			// Emit event
+			Self::deposit_event(Event::FoundVal(sender, useful_amounts[0]));
+			Ok(().into())
+		}
+	}
+}

not-so-smart-contracts/substrate/origins/README.md

@@ -0,0 +1,59 @@
+# Origins
+
+The origin of a call tells a dispatchable function where the call has come from. Origins are a way to implement access controls in the system. 
+
+There are three types of origins that can used in the runtime:
+
+```rust
+pub enum RawOrigin<AccountId> {
+	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`. 
+
+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`.
+
+```rust
+#[pallet::call]
+impl<T:Config> Pallet<T> {
+    /// Set the important val
+    /// Should be only callable by ForceOrigin
+    #[pallet::weight(10_000)]
+    pub fn set_important_val(
+        origin: OriginFor<T>,
+        new_val: u64
+    ) -> DispatchResultWithPostInfo {
+        let sender = ensure_signed(origin)?;
+        // Change to new value
+        <ImportantVal<T>>::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.
+- 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/
+- 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