-
Notifications
You must be signed in to change notification settings - Fork 2k
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
stake-pool: Add "IncreaseAdditionalValidatorStake" instruction #3924
Conversation
4d5407a
to
8d98aef
Compare
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.
just a couple nits here. lgtm!
Pull request has been modified.
stake-pool/program/src/processor.rs
Outdated
if validator_stake_info.transient_stake_lamports > 0 { | ||
// transient stake exists, try to merge from the source account | ||
Self::stake_merge( | ||
stake_pool_info.key, | ||
source_stake_account_info.clone(), | ||
withdraw_authority_info.clone(), | ||
AUTHORITY_WITHDRAW, | ||
stake_pool.stake_withdraw_bump_seed, | ||
transient_stake_account_info.clone(), | ||
clock_info.clone(), | ||
stake_history_info.clone(), | ||
stake_program_info.clone(), | ||
)?; | ||
} else { | ||
// no transient stake, split | ||
let transient_stake_bump_seed = check_transient_stake_address( | ||
program_id, | ||
stake_pool_info.key, | ||
transient_stake_account_info.key, | ||
vote_account_address, | ||
transient_stake_seed, | ||
)?; |
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.
we should move the call to check_transient_stake_address
outside the if/else. as it stands, if validator_stake_info.transient_stake_lamports > 0
is true, transient_stake_account_info
is never validated. the pool can be griefed by creating an unrelated stake account with the same lockup/authority to merge into instead, resulting in a miscalculation in validator_stake_info.transient_stake_lamports
at the end of this function, which could have unknown downstream effects
also for the record today i learned that merging stake accounts requires them to have the same authority, the missing check baited me into partway writing a poc for loss of funds before i learned this fact 😅
edit: wait, im double stupid, i forgot until i started reviewing the decrease pr that this instruction requires a staker signature 😭
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.
Great catch, thanks! I don't think you could steal funds, but you could definitely mess up the pool management if you spoof this.
other than that, looks good! |
…a-labs#3924) * stake-pool: Add "IncreaseAdditionalValidatorStake" instruction * Address feedback * Always check transient stake account address
Problem
As brought up during the single validator manager proposal #3881, there's an annoying "attack" vector where a person puts in the minimum stake delegation at the start of an epoch and immediately increases stake on the validator, preventing later SOL deposits from being activated.
Solution
There's nothing technically stopping two activating stakes from merging, so introduce a new instruction on the stake pool program which allows for a second increase in an epoch.
The normal instruction splits into the transient account and then activates it, so this instruction: