-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reject faked stake/vote accounts in stake mgmt. #13615
Reject faked stake/vote accounts in stake mgmt. #13615
Conversation
@CriesofCarrots Could you review this before #13461 ? @rwalker-com Hi, could you review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix and process tests lgtm!
One nit to make stake_instruction test changes more clear.
if vote_account.owner()? != solana_vote_program::id() { | ||
return Err(InstructionError::IncorrectProgramId); | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ;-)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. :)
Pull request has been modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@CriesofCarrots @rob-solana Thanks for reviewing! I'm going to merge this shortly after now. I'm doing last-minute local test this. |
Merging this despite the quoted build failure; I'm aware this is already being addressed and the failure isn't related to this pr. |
* Reject faked stake/vote accounts in stake mgmt. * Use clearer name (cherry picked from commit 2b3faa1) # Conflicts: # programs/stake/src/stake_instruction.rs
* Reject faked stake/vote accounts in stake mgmt. * Use clearer name (cherry picked from commit 2b3faa1)
* Reject faked stake/vote accounts in stake mgmt. * Use clearer name (cherry picked from commit 2b3faa1)
Problem
There is no
account.owner
check for referenced stake/vote accounts when delegating/splitting/merging stake accounts.Because these referenced accounts are read-only, the runtime doesn't enforce the
account.owner
check. Users can pass any accounts of any owner as long as it deserializes intoStakeState
orVoteState
, so we must checkaccount.owner
by ourselves.This time I checked this trait for possible similar issues, but I found other uses of
KeyedAccount
withoutowner
check is legitimate:solana/programs/stake/src/stake_state.rs
Lines 53 to 106 in 6c5fb32
Summary of Changes
Add guards and outright reject such transactions.
Also, this is feature-gated under
stake_program_v2
, implicitly.Fixes #