From 532044dc6aa8a31537865fdb3f91f3c66776804b Mon Sep 17 00:00:00 2001 From: MOZGIII Date: Sat, 13 Aug 2022 02:37:50 +0400 Subject: [PATCH 1/4] Make claim call free --- crates/pallet-token-claims/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/pallet-token-claims/src/lib.rs b/crates/pallet-token-claims/src/lib.rs index 8ba47b495..0808b146b 100644 --- a/crates/pallet-token-claims/src/lib.rs +++ b/crates/pallet-token-claims/src/lib.rs @@ -189,7 +189,7 @@ pub mod pallet { #[pallet::call] impl Pallet { /// Claim the tokens. - #[pallet::weight(T::WeightInfo::claim())] + #[pallet::weight((T::WeightInfo::claim(), Pays::No))] pub fn claim( origin: OriginFor, ethereum_address: EthereumAddress, From b6afd39760ae4a6a5f7db0f2c761bc5967536416 Mon Sep 17 00:00:00 2001 From: MOZGIII Date: Sat, 13 Aug 2022 03:18:59 +0400 Subject: [PATCH 2/4] Implement signed extension to verify the token claim calls --- crates/pallet-token-claims/src/lib.rs | 9 + crates/pallet-token-claims/src/signed_ext.rs | 120 ++++++++++++++ crates/pallet-token-claims/src/tests.rs | 164 ++++++++++++++++++- 3 files changed, 291 insertions(+), 2 deletions(-) create mode 100644 crates/pallet-token-claims/src/signed_ext.rs diff --git a/crates/pallet-token-claims/src/lib.rs b/crates/pallet-token-claims/src/lib.rs index 0808b146b..f5314b5a1 100644 --- a/crates/pallet-token-claims/src/lib.rs +++ b/crates/pallet-token-claims/src/lib.rs @@ -1,11 +1,20 @@ //! Token claims. +//! +//! # Security +//! +//! This pallet requires adding [`CheckTokenClaim`] to the tuple of signed extension checkers +//! at runtime to be utilized safely, otherwise it exposes a flooding vulnerability. +//! There is no way to ensure this would be automatically picked up by the runtime, so double-check +//! it at integration! #![cfg_attr(not(feature = "std"), no_std)] use frame_support::traits::{Currency, StorageVersion}; pub use self::pallet::*; +pub use self::signed_ext::*; +mod signed_ext; pub mod traits; pub mod types; pub mod weights; diff --git a/crates/pallet-token-claims/src/signed_ext.rs b/crates/pallet-token-claims/src/signed_ext.rs new file mode 100644 index 000000000..07bab0c71 --- /dev/null +++ b/crates/pallet-token-claims/src/signed_ext.rs @@ -0,0 +1,120 @@ +//! Signed extension implentation for token claims. + +use core::marker::PhantomData; + +use frame_support::{ + dispatch::Dispatchable, + pallet_prelude::*, + sp_runtime, + traits::IsSubType, + unsigned::{TransactionValidity, TransactionValidityError}, + weights::DispatchInfo, +}; +use sp_runtime::traits::{DispatchInfoOf, SignedExtension}; + +use super::*; +use crate::{traits::verify_ethereum_signature, types::EthereumSignatureMessageParams}; + +impl Pallet { + /// Validate that the `claim` is correct and should be allowed for inclusion. + /// + /// Implement the flood protection logic. + fn validate_claim_call(who: &T::AccountId, call: &Call) -> TransactionValidity { + // Check if the call matches. + let (ethereum_address, ethereum_signature) = match call { + // Allow `claim` call. + Call::claim { + ethereum_address, + ethereum_signature, + } => (ethereum_address, ethereum_signature), + // Deny all unknown calls. + _ => return Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + }; + + // Check the signature. + let message_params = EthereumSignatureMessageParams { + account_id: who.clone(), + ethereum_address: *ethereum_address, + }; + if !verify_ethereum_signature::<::EthereumSignatureVerifier>( + ethereum_signature, + &message_params, + ethereum_address, + ) { + return Err(TransactionValidityError::Invalid( + InvalidTransaction::BadProof, + )); + } + + // Check the presence of a claim. + if !>::contains_key(ethereum_address) { + return Err(TransactionValidityError::Invalid(InvalidTransaction::Call)); + } + + // All good, letting through. + Ok(ValidTransaction::default()) + } +} + +/// Check the `claim` call for validity. +/// +/// The call is fee, so this check is required to ensure it will be properly verified to +/// prevent chain flooding. +#[derive(Clone, Eq, PartialEq, codec::Encode, codec::Decode, scale_info::TypeInfo)] +#[scale_info(skip_type_params(T))] +pub struct CheckTokenClaim(PhantomData); + +impl SignedExtension for CheckTokenClaim +where + T::Call: Dispatchable, + ::Call: IsSubType>, +{ + const IDENTIFIER: &'static str = ""; + type AccountId = T::AccountId; + type Call = T::Call; + type AdditionalSigned = (); + type Pre = (); + + fn additional_signed( + &self, + ) -> Result { + Ok(()) + } + + fn pre_dispatch( + self, + who: &Self::AccountId, + call: &Self::Call, + info: &DispatchInfoOf, + len: usize, + ) -> Result { + self.validate(who, call, info, len).map(|_| ()) + } + + fn validate( + &self, + who: &Self::AccountId, + call: &Self::Call, + _info: &DispatchInfoOf, + _len: usize, + ) -> TransactionValidity { + match call.is_sub_type() { + Some(call) => Pallet::::validate_claim_call(who, call), + _ => Ok(Default::default()), + } + } +} + +impl core::fmt::Debug for CheckTokenClaim { + fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { + write!(f, "CheckTokenClaim") + } +} + +impl CheckTokenClaim { + /// Create a new [`CheckTokenClaim`] instance. + #[allow(clippy::new_without_default)] // following the pattern + pub fn new() -> Self { + Self(PhantomData) + } +} diff --git a/crates/pallet-token-claims/src/tests.rs b/crates/pallet-token-claims/src/tests.rs index db0640882..82ca9dab1 100644 --- a/crates/pallet-token-claims/src/tests.rs +++ b/crates/pallet-token-claims/src/tests.rs @@ -1,9 +1,14 @@ //! The tests for the pallet. -use frame_support::{assert_noop, assert_ok}; +use frame_support::{ + assert_noop, assert_ok, assert_storage_noop, + pallet_prelude::{InvalidTransaction, ValidTransaction}, + unsigned::TransactionValidityError, + weights::{DispatchClass, DispatchInfo, Pays}, +}; use mockall::predicate; use primitives_ethereum::EthereumAddress; -use sp_runtime::DispatchError; +use sp_runtime::{traits::SignedExtension, DispatchError}; use crate::{ mock::{ @@ -702,3 +707,158 @@ mod optional_vesting_interface { }) } } + +/// This test verifies that signed extension's `validate` works in the happy path. +#[test] +fn signed_ext_validate_works() { + new_test_ext().execute_with_ext(|_| { + // Check test preconditions. + assert!(>::contains_key(ð(EthAddr::Existing))); + assert_eq!(Balances::free_balance(42), 0); + + // Set mock expectations. + let recover_signer_ctx = MockEthereumSignatureVerifier::recover_signer_context(); + recover_signer_ctx + .expect() + .once() + .with( + predicate::eq(sig(1)), + predicate::eq(EthereumSignatureMessageParams { + account_id: 42, + ethereum_address: eth(EthAddr::Existing), + }), + ) + .return_const(Some(eth(EthAddr::Existing))); + let lock_under_vesting_ctx = MockVestingInterface::lock_under_vesting_context(); + lock_under_vesting_ctx.expect().never(); + + // Invoke the function under test. + let normal = DispatchInfo { + weight: 100, + class: DispatchClass::Normal, + pays_fee: Pays::No, + }; + let len = 0; + let ext = >::new(); + assert_storage_noop!(assert_ok!( + ext.validate( + &42, + &mock::Call::TokenClaims(Call::claim { + ethereum_address: eth(EthAddr::Existing), + ethereum_signature: sig(1), + }), + &normal, + len + ), + ValidTransaction::default() + )); + + // Assert mock invocations. + recover_signer_ctx.checkpoint(); + lock_under_vesting_ctx.checkpoint(); + }); +} + +/// This test verifies that signed extension's `validate` properly fails when the eth signature is +/// invalid. +#[test] +fn signed_ext_validate_fails_invalid_eth_signatue() { + new_test_ext().execute_with_ext(|_| { + // Check test preconditions. + assert!(>::contains_key(ð(EthAddr::Existing))); + assert_eq!(Balances::free_balance(42), 0); + + // Set mock expectations. + let recover_signer_ctx = MockEthereumSignatureVerifier::recover_signer_context(); + recover_signer_ctx + .expect() + .once() + .with( + predicate::eq(sig(1)), + predicate::eq(EthereumSignatureMessageParams { + account_id: 42, + ethereum_address: eth(EthAddr::Existing), + }), + ) + .return_const(None); + let lock_under_vesting_ctx = MockVestingInterface::lock_under_vesting_context(); + lock_under_vesting_ctx.expect().never(); + + // Invoke the function under test. + let normal = DispatchInfo { + weight: 100, + class: DispatchClass::Normal, + pays_fee: Pays::No, + }; + let len = 0; + let ext = >::new(); + assert_noop!( + ext.validate( + &42, + &mock::Call::TokenClaims(Call::claim { + ethereum_address: eth(EthAddr::Existing), + ethereum_signature: sig(1), + }), + &normal, + len + ), + TransactionValidityError::Invalid(InvalidTransaction::BadProof) + ); + + // Assert mock invocations. + recover_signer_ctx.checkpoint(); + lock_under_vesting_ctx.checkpoint(); + }); +} + +/// This test verifies that signed extension's `validate` properly fails when the claim is +/// not present in the state for the requested eth address. +#[test] +fn signed_ext_validate_fails_when_claim_is_absent() { + new_test_ext().execute_with_ext(|_| { + // Check test preconditions. + assert!(!>::contains_key(ð(EthAddr::Unknown))); + assert_eq!(Balances::free_balance(42), 0); + + // Set mock expectations. + let recover_signer_ctx = MockEthereumSignatureVerifier::recover_signer_context(); + recover_signer_ctx + .expect() + .once() + .with( + predicate::eq(sig(1)), + predicate::eq(EthereumSignatureMessageParams { + account_id: 42, + ethereum_address: eth(EthAddr::Unknown), + }), + ) + .return_const(Some(eth(EthAddr::Unknown))); + let lock_under_vesting_ctx = MockVestingInterface::lock_under_vesting_context(); + lock_under_vesting_ctx.expect().never(); + + // Invoke the function under test. + let normal = DispatchInfo { + weight: 100, + class: DispatchClass::Normal, + pays_fee: Pays::No, + }; + let len = 0; + let ext = >::new(); + assert_noop!( + ext.validate( + &42, + &mock::Call::TokenClaims(Call::claim { + ethereum_address: eth(EthAddr::Unknown), + ethereum_signature: sig(1), + }), + &normal, + len + ), + TransactionValidityError::Invalid(InvalidTransaction::Call) + ); + + // Assert mock invocations. + recover_signer_ctx.checkpoint(); + lock_under_vesting_ctx.checkpoint(); + }); +} From 738b946798d8a8479d1d77453ac613932c9aff16 Mon Sep 17 00:00:00 2001 From: MOZGIII Date: Tue, 16 Aug 2022 14:21:42 +0400 Subject: [PATCH 3/4] Fix a typo at the comment Co-authored-by: Noah Corona --- crates/pallet-token-claims/src/signed_ext.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/pallet-token-claims/src/signed_ext.rs b/crates/pallet-token-claims/src/signed_ext.rs index 07bab0c71..49ef12427 100644 --- a/crates/pallet-token-claims/src/signed_ext.rs +++ b/crates/pallet-token-claims/src/signed_ext.rs @@ -58,7 +58,7 @@ impl Pallet { /// Check the `claim` call for validity. /// -/// The call is fee, so this check is required to ensure it will be properly verified to +/// The call is free, so this check is required to ensure it will be properly verified to /// prevent chain flooding. #[derive(Clone, Eq, PartialEq, codec::Encode, codec::Decode, scale_info::TypeInfo)] #[scale_info(skip_type_params(T))] From bbff98a3ff38a90953fdb49ea1e7577f5bfa6eab Mon Sep 17 00:00:00 2001 From: MOZGIII Date: Tue, 16 Aug 2022 14:22:49 +0400 Subject: [PATCH 4/4] Set proper IDENTIFIER for signed ext --- crates/pallet-token-claims/src/signed_ext.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/pallet-token-claims/src/signed_ext.rs b/crates/pallet-token-claims/src/signed_ext.rs index 49ef12427..65d26be37 100644 --- a/crates/pallet-token-claims/src/signed_ext.rs +++ b/crates/pallet-token-claims/src/signed_ext.rs @@ -69,7 +69,7 @@ where T::Call: Dispatchable, ::Call: IsSubType>, { - const IDENTIFIER: &'static str = ""; + const IDENTIFIER: &'static str = "CheckTokenClaim"; type AccountId = T::AccountId; type Call = T::Call; type AdditionalSigned = ();