Skip to content

Commit

Permalink
Make CheckNonce refuse transactions signed by accounts with no provid…
Browse files Browse the repository at this point in the history 
…ers (#1578)

See #1453.

Co-authored-by: Bastian Köcher <[email protected]>
  • Loading branch information
@zdave-parity @bkchr
zdave-parity and bkchr authored Oct 10, 2023
1 parent 98286ad commit 93d9c8c
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 21 deletions.
2 changes: 1 addition & 1 deletion substrate/bin/node/executor/tests/submit_transaction.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ fn submitted_transaction_should_be_valid() {
let author = extrinsic.signature.clone().unwrap().0;
let address = Indices::lookup(author).unwrap();
let data = pallet_balances::AccountData { free: 5_000_000_000_000, ..Default::default() };
let account = frame_system::AccountInfo { data, ..Default::default() };
let account = frame_system::AccountInfo { providers: 1, data, ..Default::default() };
<frame_system::Account<Runtime>>::insert(&address, account);

// check validity
Expand Down
25 changes: 8 additions & 17 deletions substrate/client/service/test/src/client/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ use sp_runtime::{
};
use sp_state_machine::{backend::Backend as _, InMemoryBackend, OverlayedChanges, StateMachine};
use sp_storage::{ChildInfo, StorageKey};
use sp_trie::{LayoutV0, TrieConfiguration};
use std::{collections::HashSet, sync::Arc};
use substrate_test_runtime::TestAPI;
use substrate_test_runtime_client::{
Expand All @@ -62,22 +61,17 @@ fn construct_block(
backend: &InMemoryBackend<BlakeTwo256>,
number: BlockNumber,
parent_hash: Hash,
state_root: Hash,
txs: Vec<Transfer>,
) -> (Vec<u8>, Hash) {
) -> Vec<u8> {
let transactions = txs.into_iter().map(|tx| tx.into_unchecked_extrinsic()).collect::<Vec<_>>();

let iter = transactions.iter().map(Encode::encode);
let extrinsics_root = LayoutV0::<BlakeTwo256>::ordered_trie_root(iter).into();

let mut header = Header {
parent_hash,
number,
state_root,
extrinsics_root,
state_root: Default::default(),
extrinsics_root: Default::default(),
digest: Digest { logs: vec![] },
};
let hash = header.hash();
let mut overlay = OverlayedChanges::default();
let backend_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(backend);
let runtime_code = backend_runtime_code.runtime_code().expect("Code is part of the backend");
Expand Down Expand Up @@ -124,19 +118,16 @@ fn construct_block(
.unwrap();
header = Header::decode(&mut &ret_data[..]).unwrap();

(vec![].and(&Block { header, extrinsics: transactions }), hash)
vec![].and(&Block { header, extrinsics: transactions })
}

fn block1(genesis_hash: Hash, backend: &InMemoryBackend<BlakeTwo256>) -> (Vec<u8>, Hash) {
fn block1(genesis_hash: Hash, backend: &InMemoryBackend<BlakeTwo256>) -> Vec<u8> {
construct_block(
backend,
1,
genesis_hash,
array_bytes::hex_n_into_unchecked(
"25e5b37074063ab75c889326246640729b40d0c86932edc527bc80db0e04fe5c",
),
vec![Transfer {
from: AccountKeyring::Alice.into(),
from: AccountKeyring::One.into(),
to: AccountKeyring::Two.into(),
amount: 69 * DOLLARS,
nonce: 0,
Expand Down Expand Up @@ -175,7 +166,7 @@ fn construct_genesis_should_work_with_native() {
let genesis_hash = insert_genesis_block(&mut storage);

let backend = InMemoryBackend::from((storage, StateVersion::default()));
let (b1data, _b1hash) = block1(genesis_hash, &backend);
let b1data = block1(genesis_hash, &backend);
let backend_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&backend);
let runtime_code = backend_runtime_code.runtime_code().expect("Code is part of the backend");

Expand Down Expand Up @@ -206,7 +197,7 @@ fn construct_genesis_should_work_with_wasm() {
let genesis_hash = insert_genesis_block(&mut storage);

let backend = InMemoryBackend::from((storage, StateVersion::default()));
let (b1data, _b1hash) = block1(genesis_hash, &backend);
let b1data = block1(genesis_hash, &backend);
let backend_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&backend);
let runtime_code = backend_runtime_code.runtime_code().expect("Code is part of the backend");

Expand Down
56 changes: 53 additions & 3 deletions substrate/frame/system/src/extensions/check_nonce.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use codec::{Decode, Encode};
use frame_support::dispatch::DispatchInfo;
use scale_info::TypeInfo;
use sp_runtime::{
traits::{DispatchInfoOf, Dispatchable, One, SignedExtension},
traits::{DispatchInfoOf, Dispatchable, One, SignedExtension, Zero},
transaction_validity::{
InvalidTransaction, TransactionLongevity, TransactionValidity, TransactionValidityError,
ValidTransaction,
Expand Down Expand Up @@ -80,6 +80,10 @@ where
_len: usize,
) -> Result<(), TransactionValidityError> {
let mut account = crate::Account::<T>::get(who);
if account.providers.is_zero() && account.sufficients.is_zero() {
// Nonce storage not paid for
return Err(InvalidTransaction::Payment.into())
}
if self.0 != account.nonce {
return Err(if self.0 < account.nonce {
InvalidTransaction::Stale
Expand All @@ -100,8 +104,11 @@ where
_info: &DispatchInfoOf<Self::Call>,
_len: usize,
) -> TransactionValidity {
// check index
let account = crate::Account::<T>::get(who);
if account.providers.is_zero() && account.sufficients.is_zero() {
// Nonce storage not paid for
return InvalidTransaction::Payment.into()
}
if self.0 < account.nonce {
return InvalidTransaction::Stale.into()
}
Expand Down Expand Up @@ -137,7 +144,7 @@ mod tests {
crate::AccountInfo {
nonce: 1,
consumers: 0,
providers: 0,
providers: 1,
sufficients: 0,
data: 0,
},
Expand All @@ -164,4 +171,47 @@ mod tests {
);
})
}

#[test]
fn signed_ext_check_nonce_requires_provider() {
new_test_ext().execute_with(|| {
crate::Account::<Test>::insert(
2,
crate::AccountInfo {
nonce: 1,
consumers: 0,
providers: 1,
sufficients: 0,
data: 0,
},
);
crate::Account::<Test>::insert(
3,
crate::AccountInfo {
nonce: 1,
consumers: 0,
providers: 0,
sufficients: 1,
data: 0,
},
);
let info = DispatchInfo::default();
let len = 0_usize;
// Both providers and sufficients zero
assert_noop!(
CheckNonce::<Test>(1).validate(&1, CALL, &info, len),
InvalidTransaction::Payment
);
assert_noop!(
CheckNonce::<Test>(1).pre_dispatch(&1, CALL, &info, len),
InvalidTransaction::Payment
);
// Non-zero providers
assert_ok!(CheckNonce::<Test>(1).validate(&2, CALL, &info, len));
assert_ok!(CheckNonce::<Test>(1).pre_dispatch(&2, CALL, &info, len));
// Non-zero sufficients
assert_ok!(CheckNonce::<Test>(1).validate(&3, CALL, &info, len));
assert_ok!(CheckNonce::<Test>(1).pre_dispatch(&3, CALL, &info, len));
})
}
}

0 comments on commit 93d9c8c

Please sign in to comment.