Skip to content

Commit

Permalink
Reject durable nonce transactions not signed by authority (solana-lab…
Browse files Browse the repository at this point in the history
  • Loading branch information
jstarry authored and jeffwashington committed Jun 29, 2022
1 parent 781e800 commit 616425a
Show file tree
Hide file tree
Showing 7 changed files with 116 additions and 34 deletions.
6 changes: 4 additions & 2 deletions runtime/src/accounts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3530,7 +3530,8 @@ mod tests {
assert!(nonce_account::verify_nonce_account(
&collected_nonce_account,
durable_nonce.as_hash(),
));
)
.is_some());
}

#[test]
Expand Down Expand Up @@ -3635,7 +3636,8 @@ mod tests {
assert!(nonce_account::verify_nonce_account(
&collected_nonce_account,
durable_nonce.as_hash(),
));
)
.is_some());
}

#[test]
Expand Down
42 changes: 23 additions & 19 deletions runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ use {
message::{AccountKeys, SanitizedMessage},
native_loader,
native_token::sol_to_lamports,
nonce::{self, state::DurableNonce},
nonce::{self, state::DurableNonce, NONCED_TX_MARKER_IX_INDEX},
nonce_account,
packet::PACKET_DATA_SIZE,
precompiles::get_precompiles,
Expand Down Expand Up @@ -4132,15 +4132,25 @@ impl Bank {
}

fn check_message_for_nonce(&self, message: &SanitizedMessage) -> Option<TransactionAccount> {
message
.get_durable_nonce(self.feature_set.is_active(&nonce_must_be_writable::id()))
.and_then(|nonce_address| {
self.get_account_with_fixed_root(nonce_address)
.map(|nonce_account| (*nonce_address, nonce_account))
})
.filter(|(_, nonce_account)| {
nonce_account::verify_nonce_account(nonce_account, message.recent_blockhash())
})
let nonce_address =
message.get_durable_nonce(self.feature_set.is_active(&nonce_must_be_writable::id()))?;
let nonce_account = self.get_account_with_fixed_root(nonce_address)?;
let nonce_data =
nonce_account::verify_nonce_account(&nonce_account, message.recent_blockhash())?;

if self
.feature_set
.is_active(&feature_set::nonce_must_be_authorized::ID)
{
let nonce_is_authorized = message
.get_ix_signers(NONCED_TX_MARKER_IX_INDEX as usize)
.any(|signer| signer == &nonce_data.authority);
if !nonce_is_authorized {
return None;
}
}

Some((*nonce_address, nonce_account))
}

fn check_transaction_for_nonce(
Expand Down Expand Up @@ -12831,22 +12841,16 @@ pub(crate) mod tests {
let initial_custodian_balance = custodian_account.lamports();
assert_eq!(
bank.process_transaction(&nonce_tx),
Err(TransactionError::InstructionError(
0,
InstructionError::MissingRequiredSignature,
))
Err(TransactionError::BlockhashNotFound),
);
/* Check fee charged and nonce has *not* advanced */
/* Check fee was *not* charged and nonce has *not* advanced */
let mut recent_message = nonce_tx.message;
recent_message.recent_blockhash = bank.last_blockhash();
assert_eq!(
bank.get_balance(&custodian_pubkey),
initial_custodian_balance
- bank
.get_fee_for_message(&recent_message.try_into().unwrap())
.unwrap()
);
assert_ne!(
assert_eq!(
nonce_hash,
get_nonce_blockhash(&bank, &nonce_pubkey).unwrap()
);
Expand Down
13 changes: 8 additions & 5 deletions runtime/src/nonce_keyed_account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1214,7 +1214,8 @@ mod test {
.unwrap()
.borrow(),
get_durable_nonce(&invoke_context).as_hash(),
));
)
.is_some());
}

#[test]
Expand All @@ -1226,13 +1227,14 @@ mod test {
_instruction_context,
instruction_accounts
);
assert!(!verify_nonce_account(
assert!(verify_nonce_account(
&transaction_context
.get_account_at_index(NONCE_ACCOUNT_INDEX)
.unwrap()
.borrow(),
&Hash::default()
));
)
.is_none());
}

#[test]
Expand Down Expand Up @@ -1263,12 +1265,13 @@ mod test {
.unwrap();
set_invoke_context_blockhash!(invoke_context, 1);
drop(nonce_account);
assert!(!verify_nonce_account(
assert!(verify_nonce_account(
&transaction_context
.get_account_at_index(NONCE_ACCOUNT_INDEX)
.unwrap()
.borrow(),
get_durable_nonce(&invoke_context).as_hash(),
));
)
.is_none());
}
}
57 changes: 56 additions & 1 deletion sdk/program/src/message/sanitized.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,21 @@ impl SanitizedMessage {
}
}

/// Get a list of signers for the instruction at the given index
pub fn get_ix_signers(&self, ix_index: usize) -> impl Iterator<Item = &Pubkey> {
self.instructions()
.get(ix_index)
.into_iter()
.flat_map(|ix| {
ix.accounts
.iter()
.copied()
.map(usize::from)
.filter(|index| self.is_signer(*index))
.filter_map(|signer_index| self.account_keys().get(signer_index))
})
}

/// If the message uses a durable nonce, return the pubkey of the nonce account
pub fn get_durable_nonce(&self, nonce_must_be_writable: bool) -> Option<&Pubkey> {
self.instructions()
Expand Down Expand Up @@ -256,7 +271,7 @@ impl SanitizedMessage {

#[cfg(test)]
mod tests {
use {super::*, crate::message::v0};
use {super::*, crate::message::v0, std::collections::HashSet};

#[test]
fn test_try_from_message() {
Expand Down Expand Up @@ -336,4 +351,44 @@ mod tests {

assert_eq!(v0_message.num_readonly_accounts(), 3);
}

#[test]
fn test_get_ix_signers() {
let signer0 = Pubkey::new_unique();
let signer1 = Pubkey::new_unique();
let non_signer = Pubkey::new_unique();
let loader_key = Pubkey::new_unique();
let instructions = vec![
CompiledInstruction::new(3, &(), vec![2, 0]),
CompiledInstruction::new(3, &(), vec![0, 1]),
CompiledInstruction::new(3, &(), vec![0, 0]),
];

let message = SanitizedMessage::try_from(LegacyMessage::new_with_compiled_instructions(
2,
1,
2,
vec![signer0, signer1, non_signer, loader_key],
Hash::default(),
instructions,
))
.unwrap();

assert_eq!(
message.get_ix_signers(0).collect::<HashSet<_>>(),
HashSet::from_iter([&signer0])
);
assert_eq!(
message.get_ix_signers(1).collect::<HashSet<_>>(),
HashSet::from_iter([&signer0, &signer1])
);
assert_eq!(
message.get_ix_signers(2).collect::<HashSet<_>>(),
HashSet::from_iter([&signer0])
);
assert_eq!(
message.get_ix_signers(3).collect::<HashSet<_>>(),
HashSet::default()
);
}
}
15 changes: 15 additions & 0 deletions sdk/src/feature_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,18 @@ pub mod enable_durable_nonce {
solana_sdk::declare_id!("4EJQtF2pkRyawwcTVfQutzq4Sa5hRhibF6QAK1QXhtEX");
}

pub mod vote_state_update_credit_per_dequeue {
solana_sdk::declare_id!("CveezY6FDLVBToHDcvJRmtMouqzsmj4UXYh5ths5G5Uv");
}

pub mod quick_bail_on_panic {
solana_sdk::declare_id!("DpJREPyuMZ5nDfU6H3WTqSqUFSXAfw8u7xqmWtEwJDcP");
}

pub mod nonce_must_be_authorized {
solana_sdk::declare_id!("HxrEu1gXuH7iD3Puua1ohd5n4iUKJyFNtNxk9DVJkvgr");
}

lazy_static! {
/// Map of feature identifiers to user-visible description
pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
Expand Down Expand Up @@ -523,6 +535,9 @@ lazy_static! {
(warp_timestamp_with_a_vengeance::id(), "warp timestamp again, adjust bounding to 150% slow #25666"),
(separate_nonce_from_blockhash::id(), "separate durable nonce and blockhash domains #25744"),
(enable_durable_nonce::id(), "enable durable nonce #25744"),
(vote_state_update_credit_per_dequeue::id(), "Calculate vote credits for VoteStateUpdate per vote dequeue to match credit awards for Vote instruction"),
(quick_bail_on_panic::id(), "quick bail on panic"),
(nonce_must_be_authorized::id(), "nonce must be authorized"),
/*************** ADD NEW FEATURES HERE ***************/
]
.iter()
Expand Down
15 changes: 9 additions & 6 deletions sdk/src/nonce_account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ use {
account::{AccountSharedData, ReadableAccount},
account_utils::StateMut,
hash::Hash,
nonce::{state::Versions, State},
nonce::{
state::{Data, Versions},
State,
},
},
std::cell::RefCell,
};
Expand All @@ -21,13 +24,13 @@ pub fn create_account(lamports: u64) -> RefCell<AccountSharedData> {
}

// TODO: Consider changing argument from Hash to DurableNonce.
pub fn verify_nonce_account(acc: &AccountSharedData, hash: &Hash) -> bool {
pub fn verify_nonce_account(acc: &AccountSharedData, hash: &Hash) -> Option<Data> {
if acc.owner() != &crate::system_program::id() {
return false;
return None;
}
match StateMut::<Versions>::state(acc).map(|v| v.convert_to_current()) {
Ok(State::Initialized(ref data)) => hash == &data.blockhash(),
_ => false,
Ok(State::Initialized(data)) => (hash == &data.blockhash()).then(|| data),
_ => None,
}
}

Expand Down Expand Up @@ -56,6 +59,6 @@ mod tests {
&program_id,
)
.expect("nonce_account");
assert!(!verify_nonce_account(&account, &Hash::default()));
assert!(verify_nonce_account(&account, &Hash::default()).is_none());
}
}
2 changes: 1 addition & 1 deletion send-transaction-service/src/send_transaction_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -597,7 +597,7 @@ impl SendTransactionService {
.last_sent_time
.map(|last| now.duration_since(last) >= retry_rate)
.unwrap_or(false);
if !nonce_account::verify_nonce_account(&nonce_account, &durable_nonce)
if nonce_account::verify_nonce_account(&nonce_account, &durable_nonce).is_none()
&& signature_status.is_none()
&& expired
{
Expand Down

0 comments on commit 616425a

Please sign in to comment.