Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Reject faked stake/vote accounts in stake mgmt. #13615

Merged
ryoqun merged 2 commits into from
Nov 16, 2020
Merged

Reject faked stake/vote accounts in stake mgmt. #13615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df0eb06
Reject faked stake/vote accounts in stake mgmt.
ryoqun Nov 16, 2020
933dcfd
Use clearer name
ryoqun Nov 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions programs/stake/src/stake_instruction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,11 +532,20 @@ mod tests {
sysvar::stake_history::StakeHistory,
};
use std::cell::RefCell;
use std::str::FromStr;

fn create_default_account() -> RefCell<Account> {
RefCell::new(Account::default())
}

fn invalid_stake_state_pubkey() -> Pubkey {
Pubkey::from_str("BadStake11111111111111111111111111111111111").unwrap()
}

fn invalid_vote_state_pubkey() -> Pubkey {
Pubkey::from_str("BadVote111111111111111111111111111111111111").unwrap()
}

fn process_instruction(instruction: &Instruction) -> Result<(), InstructionError> {
let accounts: Vec<_> = instruction
.accounts
Expand All @@ -552,6 +561,14 @@ mod tests {
config::create_account(0, &config::Config::default())
} else if sysvar::rent::check_id(&meta.pubkey) {
account::create_account(&Rent::default(), 1)
} else if meta.pubkey == invalid_stake_state_pubkey() {
let mut account = Account::default();
account.owner = id();
account
} else if meta.pubkey == invalid_vote_state_pubkey() {
let mut account = Account::default();
account.owner = solana_vote_program::id();
account
} else {
Account::default()
})
Expand Down Expand Up @@ -599,14 +616,18 @@ mod tests {
&Pubkey::default(),
&Pubkey::default(),
100,
&Pubkey::default()
&invalid_stake_state_pubkey(),
)[1]
),
Err(InstructionError::InvalidAccountData),
);
assert_eq!(
process_instruction(
&merge(&Pubkey::default(), &Pubkey::default(), &Pubkey::default(),)[0]
&merge(
&Pubkey::default(),
&invalid_stake_state_pubkey(),
&Pubkey::default(),
)[0]
),
Err(InstructionError::InvalidAccountData),
);
Expand All @@ -616,7 +637,7 @@ mod tests {
&Pubkey::default(),
&Pubkey::default(),
100,
&Pubkey::default(),
&invalid_stake_state_pubkey(),
&Pubkey::default(),
"seed"
)[1]
Expand All @@ -627,7 +648,7 @@ mod tests {
process_instruction(&delegate_stake(
&Pubkey::default(),
&Pubkey::default(),
&Pubkey::default()
&invalid_vote_state_pubkey(),
)),
Err(InstructionError::InvalidAccountData),
);
Expand Down Expand Up @@ -764,12 +785,14 @@ mod tests {
);

// gets the check non-deserialize-able account in delegate_stake
let mut bad_vote_account = create_default_account();
bad_vote_account.get_mut().owner = solana_vote_program::id();
assert_eq!(
super::process_instruction(
&Pubkey::default(),
&[
KeyedAccount::new(&Pubkey::default(), true, &create_default_account()),
KeyedAccount::new(&Pubkey::default(), false, &create_default_account()),
KeyedAccount::new(&Pubkey::default(), false, &bad_vote_account),
KeyedAccount::new(
&sysvar::clock::id(),
false,
Expand Down
106 changes: 106 additions & 0 deletions programs/stake/src/stake_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -821,6 +821,10 @@ impl<'a> StakeAccount for KeyedAccount<'a> {
config: &Config,
signers: &HashSet<Pubkey>,
) -> Result<(), InstructionError> {
if vote_account.owner()? != solana_vote_program::id() {
return Err(InstructionError::IncorrectProgramId);
}
Comment on lines +824 to +826
Copy link
Copy Markdown
Contributor Author

@ryoqun ryoqun Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackcmay just making sure; it seems that IncorrectProgramId isn't used much; but I think this is a perfect usecase of IncorrectProgramId for these kinds of checks, I guess.

Copy link
Copy Markdown
Contributor

@jackcmay jackcmay Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description of this error is doesn't make much sense but the way you propose to use it here makes as much sense as any ;-)

Copy link
Copy Markdown
Contributor

@CriesofCarrots CriesofCarrots Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, this seems perfectly consistent with how the error is used in the Vest program

Copy link
Copy Markdown
Contributor Author

@ryoqun ryoqun Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I copied from that. :)


match self.state()? {
StakeState::Initialized(meta) => {
meta.authorized.check(signers, StakeAuthorize::Staker)?;
Expand Down Expand Up @@ -882,6 +886,10 @@ impl<'a> StakeAccount for KeyedAccount<'a> {
split: &KeyedAccount,
signers: &HashSet<Pubkey>,
) -> Result<(), InstructionError> {
if split.owner()? != id() {
return Err(InstructionError::IncorrectProgramId);
}

if let StakeState::Uninitialized = split.state()? {
// verify enough account lamports
if lamports > self.lamports()? {
Expand Down Expand Up @@ -981,6 +989,10 @@ impl<'a> StakeAccount for KeyedAccount<'a> {
stake_history: &StakeHistory,
signers: &HashSet<Pubkey>,
) -> Result<(), InstructionError> {
if source_stake.owner()? != id() {
return Err(InstructionError::IncorrectProgramId);
}

let meta = match self.state()? {
StakeState::Stake(meta, stake) => {
// stake must be fully de-activated
Expand Down Expand Up @@ -1480,6 +1492,21 @@ mod tests {
)
.is_ok());

// signed but faked vote account
let faked_vote_account = vote_account.clone();
faked_vote_account.borrow_mut().owner = solana_sdk::pubkey::new_rand();
let faked_vote_keyed_account = KeyedAccount::new(&vote_pubkey, false, &faked_vote_account);
assert_eq!(
stake_keyed_account.delegate(
&faked_vote_keyed_account,
&clock,
&StakeHistory::default(),
&Config::default(),
&signers,
),
Err(solana_sdk::instruction::InstructionError::IncorrectProgramId)
);

// verify that delegate() looks right, compare against hand-rolled
let stake = StakeState::stake_from(&stake_keyed_account.account.borrow()).unwrap();
assert_eq!(
Expand Down Expand Up @@ -3558,6 +3585,40 @@ mod tests {
}
}

#[test]
fn test_split_fake_stake_dest() {
let stake_pubkey = solana_sdk::pubkey::new_rand();
let stake_lamports = 42;

let split_stake_pubkey = solana_sdk::pubkey::new_rand();
let signers = vec![stake_pubkey].into_iter().collect();

let split_stake_account = Account::new_ref_data_with_space(
0,
&StakeState::Uninitialized,
std::mem::size_of::<StakeState>(),
&solana_sdk::pubkey::new_rand(),
)
.expect("stake_account");

let split_stake_keyed_account =
KeyedAccount::new(&split_stake_pubkey, true, &split_stake_account);

let stake_account = Account::new_ref_data_with_space(
stake_lamports,
&StakeState::Stake(Meta::auto(&stake_pubkey), Stake::just_stake(stake_lamports)),
std::mem::size_of::<StakeState>(),
&id(),
)
.expect("stake_account");
let stake_keyed_account = KeyedAccount::new(&stake_pubkey, true, &stake_account);

assert_eq!(
stake_keyed_account.split(stake_lamports / 2, &split_stake_keyed_account, &signers),
Err(InstructionError::IncorrectProgramId),
);
}

#[test]
fn test_split_to_account_with_rent_exempt_reserve() {
let stake_pubkey = solana_sdk::pubkey::new_rand();
Expand Down Expand Up @@ -4421,6 +4482,51 @@ mod tests {
}
}

#[test]
fn test_merge_fake_stake_source() {
let stake_pubkey = solana_sdk::pubkey::new_rand();
let source_stake_pubkey = solana_sdk::pubkey::new_rand();
let authorized_pubkey = solana_sdk::pubkey::new_rand();
let stake_lamports = 42;

let signers = vec![authorized_pubkey].into_iter().collect();

let stake_account = Account::new_ref_data_with_space(
stake_lamports,
&StakeState::Stake(
Meta::auto(&authorized_pubkey),
Stake::just_stake(stake_lamports),
),
std::mem::size_of::<StakeState>(),
&id(),
)
.expect("stake_account");
let stake_keyed_account = KeyedAccount::new(&stake_pubkey, true, &stake_account);

let source_stake_account = Account::new_ref_data_with_space(
stake_lamports,
&StakeState::Stake(
Meta::auto(&authorized_pubkey),
Stake::just_stake(stake_lamports),
),
std::mem::size_of::<StakeState>(),
&solana_sdk::pubkey::new_rand(),
)
.expect("source_stake_account");
let source_stake_keyed_account =
KeyedAccount::new(&source_stake_pubkey, true, &source_stake_account);

assert_eq!(
stake_keyed_account.merge(
&source_stake_keyed_account,
&Clock::default(),
&StakeHistory::default(),
&signers
),
Err(InstructionError::IncorrectProgramId)
);
}

#[test]
fn test_merge_active_stake() {
let stake_pubkey = solana_sdk::pubkey::new_rand();
Expand Down