From 36484f4f08f3139350a6cf2b1fa01eb2453c76ad Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Sat, 26 Feb 2022 12:10:01 -0700 Subject: [PATCH] Prevent new RentPaying state created by paying fees (#23358) * Add failing test * Check fee-payer rent-state change on load * Add more test cases * Review comments --- runtime/src/account_rent_state.rs | 41 +++-- runtime/src/accounts.rs | 20 ++- runtime/src/bank.rs | 249 ++++++++++++++++++++++++++++++ 3 files changed, 298 insertions(+), 12 deletions(-) diff --git a/runtime/src/account_rent_state.rs b/runtime/src/account_rent_state.rs index 86808ed6dc0cf6..ec70a0541f74af 100644 --- a/runtime/src/account_rent_state.rs +++ b/runtime/src/account_rent_state.rs @@ -3,6 +3,7 @@ use { log::*, solana_sdk::{ account::{AccountSharedData, ReadableAccount}, + pubkey::Pubkey, rent::Rent, transaction::{Result, TransactionError}, transaction_context::TransactionContext, @@ -55,17 +56,35 @@ pub(crate) fn check_rent_state( index: usize, ) -> Result<()> { if let Some((pre_rent_state, post_rent_state)) = pre_rent_state.zip(post_rent_state) { - submit_rent_state_metrics(pre_rent_state, post_rent_state); - if !post_rent_state.transition_allowed_from(pre_rent_state) { - debug!( - "Account {:?} not rent exempt, state {:?}", - transaction_context.get_key_of_account_at_index(index), - transaction_context - .get_account_at_index(index) - .map(|account| account.borrow()), - ); - return Err(TransactionError::InvalidRentPayingAccount); - } + let expect_msg = "account must exist at TransactionContext index if rent-states are Some"; + check_rent_state_with_account( + pre_rent_state, + post_rent_state, + transaction_context + .get_key_of_account_at_index(index) + .expect(expect_msg), + &transaction_context + .get_account_at_index(index) + .expect(expect_msg) + .borrow(), + )?; + } + Ok(()) +} + +pub(crate) fn check_rent_state_with_account( + pre_rent_state: &RentState, + post_rent_state: &RentState, + address: &Pubkey, + account_state: &AccountSharedData, +) -> Result<()> { + submit_rent_state_metrics(pre_rent_state, post_rent_state); + if !post_rent_state.transition_allowed_from(pre_rent_state) { + debug!( + "Account {} not rent exempt, state {:?}", + address, account_state, + ); + return Err(TransactionError::InvalidRentPayingAccount); } Ok(()) } diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 6dc827d688a2d9..f933b838b442c1 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -1,5 +1,6 @@ use { crate::{ + account_rent_state::{check_rent_state_with_account, RentState}, accounts_db::{ AccountShrinkThreshold, AccountsAddRootTiming, AccountsDb, AccountsDbConfig, BankHashInfo, ErrorCounters, LoadHint, LoadedAccount, ScanStorageResult, @@ -342,7 +343,7 @@ impl Accounts { if payer_index != 0 { warn!("Payer index should be 0! {:?}", tx); } - let payer_account = &mut accounts[payer_index].1; + let (ref payer_address, ref mut payer_account) = accounts[payer_index]; if payer_account.lamports() == 0 { error_counters.account_not_found += 1; return Err(TransactionError::AccountNotFound); @@ -363,10 +364,27 @@ impl Accounts { error_counters.insufficient_funds += 1; return Err(TransactionError::InsufficientFundsForFee); } + let payer_pre_rent_state = + RentState::from_account(payer_account, &rent_collector.rent); payer_account .checked_sub_lamports(fee) .map_err(|_| TransactionError::InsufficientFundsForFee)?; + let payer_post_rent_state = + RentState::from_account(payer_account, &rent_collector.rent); + let rent_state_result = check_rent_state_with_account( + &payer_pre_rent_state, + &payer_post_rent_state, + payer_address, + payer_account, + ); + // Feature gate only wraps the actual error return so that the metrics and debug + // logging generated by `check_rent_state_with_account()` can be examined before + // feature activation + if feature_set.is_active(&feature_set::require_rent_exempt_accounts::id()) { + rent_state_result?; + } + let program_indices = message .instructions() .iter() diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 1a3912c28ce186..3bfa88a5735273 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -16476,6 +16476,255 @@ pub(crate) mod tests { assert!(result.is_ok()); } + #[test] + fn test_invalid_rent_state_changes_fee_payer() { + let GenesisConfigInfo { + mut genesis_config, + mint_keypair, + .. + } = create_genesis_config_with_leader(sol_to_lamports(100.), &Pubkey::new_unique(), 42); + genesis_config.rent = Rent::default(); + genesis_config.fee_rate_governor = FeeRateGovernor::new( + solana_sdk::fee_calculator::DEFAULT_TARGET_LAMPORTS_PER_SIGNATURE, + solana_sdk::fee_calculator::DEFAULT_TARGET_SIGNATURES_PER_SLOT, + ); + let rent_exempt_minimum = genesis_config.rent.minimum_balance(0); + + // Create legacy rent-paying System account + let rent_paying_fee_payer = Keypair::new(); + genesis_config.accounts.insert( + rent_paying_fee_payer.pubkey(), + Account::new(rent_exempt_minimum - 1, 0, &system_program::id()), + ); + // Create RentExempt recipient account + let recipient = Pubkey::new_unique(); + genesis_config.accounts.insert( + recipient, + Account::new(rent_exempt_minimum, 0, &system_program::id()), + ); + + // Activate features, including require_rent_exempt_accounts + activate_all_features(&mut genesis_config); + + let bank = Bank::new_for_tests(&genesis_config); + let recent_blockhash = bank.last_blockhash(); + + let check_account_is_rent_exempt = |pubkey: &Pubkey| -> bool { + let account = bank.get_account(pubkey).unwrap(); + Rent::default().is_exempt(account.lamports(), account.data().len()) + }; + + // Create just-rent-exempt fee-payer + let rent_exempt_fee_payer = Keypair::new(); + bank.transfer( + rent_exempt_minimum, + &mint_keypair, + &rent_exempt_fee_payer.pubkey(), + ) + .unwrap(); + + // Dummy message to determine fee amount + let dummy_message = SanitizedMessage::try_from(Message::new_with_blockhash( + &[system_instruction::transfer( + &rent_exempt_fee_payer.pubkey(), + &recipient, + sol_to_lamports(1.), + )], + Some(&rent_exempt_fee_payer.pubkey()), + &recent_blockhash, + )) + .unwrap(); + let fee = bank.get_fee_for_message(&dummy_message).unwrap(); + + // RentPaying fee-payer can remain RentPaying + let tx = Transaction::new( + &[&rent_paying_fee_payer, &mint_keypair], + Message::new( + &[system_instruction::transfer( + &mint_keypair.pubkey(), + &recipient, + rent_exempt_minimum, + )], + Some(&rent_paying_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert!(result.is_ok()); + assert!(!check_account_is_rent_exempt( + &rent_paying_fee_payer.pubkey() + )); + + // RentPaying fee-payer can remain RentPaying on failed executed tx + let sender = Keypair::new(); + let fee_payer_balance = bank.get_balance(&rent_paying_fee_payer.pubkey()); + let tx = Transaction::new( + &[&rent_paying_fee_payer, &sender], + Message::new( + &[system_instruction::transfer( + &sender.pubkey(), + &recipient, + rent_exempt_minimum, + )], + Some(&rent_paying_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert_ne!( + result.unwrap_err(), + TransactionError::InvalidRentPayingAccount + ); + assert_ne!( + fee_payer_balance, + bank.get_balance(&rent_paying_fee_payer.pubkey()) + ); + assert!(!check_account_is_rent_exempt( + &rent_paying_fee_payer.pubkey() + )); + + // RentPaying fee-payer can be emptied with fee and transaction + let tx = Transaction::new( + &[&rent_paying_fee_payer], + Message::new( + &[system_instruction::transfer( + &rent_paying_fee_payer.pubkey(), + &recipient, + bank.get_balance(&rent_paying_fee_payer.pubkey()) - fee, + )], + Some(&rent_paying_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert!(result.is_ok()); + assert_eq!(0, bank.get_balance(&rent_paying_fee_payer.pubkey())); + + // RentExempt fee-payer cannot become RentPaying from transaction fee + let tx = Transaction::new( + &[&rent_exempt_fee_payer, &mint_keypair], + Message::new( + &[system_instruction::transfer( + &mint_keypair.pubkey(), + &recipient, + rent_exempt_minimum, + )], + Some(&rent_exempt_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert_eq!( + result.unwrap_err(), + TransactionError::InvalidRentPayingAccount + ); + assert!(check_account_is_rent_exempt( + &rent_exempt_fee_payer.pubkey() + )); + + // RentExempt fee-payer cannot become RentPaying via failed executed tx + let tx = Transaction::new( + &[&rent_exempt_fee_payer, &sender], + Message::new( + &[system_instruction::transfer( + &sender.pubkey(), + &recipient, + rent_exempt_minimum, + )], + Some(&rent_exempt_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert_eq!( + result.unwrap_err(), + TransactionError::InvalidRentPayingAccount + ); + assert!(check_account_is_rent_exempt( + &rent_exempt_fee_payer.pubkey() + )); + + // For good measure, show that a RentExempt fee-payer that is also debited by a transaction + // cannot become RentPaying by that debit, but can still be charged for the fee + bank.transfer(fee, &mint_keypair, &rent_exempt_fee_payer.pubkey()) + .unwrap(); + let fee_payer_balance = bank.get_balance(&rent_exempt_fee_payer.pubkey()); + assert_eq!(fee_payer_balance, rent_exempt_minimum + fee); + let tx = Transaction::new( + &[&rent_exempt_fee_payer], + Message::new( + &[system_instruction::transfer( + &rent_exempt_fee_payer.pubkey(), + &recipient, + fee, + )], + Some(&rent_exempt_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert_eq!( + result.unwrap_err(), + TransactionError::InvalidRentPayingAccount + ); + assert_eq!( + fee_payer_balance - fee, + bank.get_balance(&rent_exempt_fee_payer.pubkey()) + ); + assert!(check_account_is_rent_exempt( + &rent_exempt_fee_payer.pubkey() + )); + + // Also show that a RentExempt fee-payer can be completely emptied via fee and transaction + bank.transfer(fee + 1, &mint_keypair, &rent_exempt_fee_payer.pubkey()) + .unwrap(); + assert!(bank.get_balance(&rent_exempt_fee_payer.pubkey()) > rent_exempt_minimum + fee); + let tx = Transaction::new( + &[&rent_exempt_fee_payer], + Message::new( + &[system_instruction::transfer( + &rent_exempt_fee_payer.pubkey(), + &recipient, + bank.get_balance(&rent_exempt_fee_payer.pubkey()) - fee, + )], + Some(&rent_exempt_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert!(result.is_ok()); + assert_eq!(0, bank.get_balance(&rent_exempt_fee_payer.pubkey())); + + // ... but not if the fee alone would make it RentPaying + bank.transfer( + rent_exempt_minimum + 1, + &mint_keypair, + &rent_exempt_fee_payer.pubkey(), + ) + .unwrap(); + assert!(bank.get_balance(&rent_exempt_fee_payer.pubkey()) < rent_exempt_minimum + fee); + let tx = Transaction::new( + &[&rent_exempt_fee_payer], + Message::new( + &[system_instruction::transfer( + &rent_exempt_fee_payer.pubkey(), + &recipient, + bank.get_balance(&rent_exempt_fee_payer.pubkey()) - fee, + )], + Some(&rent_exempt_fee_payer.pubkey()), + ), + recent_blockhash, + ); + let result = bank.process_transaction(&tx); + assert_eq!( + result.unwrap_err(), + TransactionError::InvalidRentPayingAccount + ); + assert!(check_account_is_rent_exempt( + &rent_exempt_fee_payer.pubkey() + )); + } + #[test] fn test_rent_state_list_len() { let GenesisConfigInfo {