Skip to content

Commit 0e56295

Browse files
joncinque0xa5df-c
andauthored
Merge commit from fork
* program: Prohibit decreasing stake on a non-active validator #### Problem It's possible to decrease stake on a validator that isn't in the `Active` state. This logic is inconsistent with increases, which prohibit increases if the validator is being removed. #### Summary of changes Add the same check as during increases. * program: Protect against cluster-restart hijacks #### Problem During cluster restart in which a stake account is reset to `Initialized`, the stake pool will properly merge those funds back into the pool, but does not update the validator status. This means it's possible for exploiters to take control of a stake account and have it be counted by the stake pool. #### Summary of changes This includes two fixes: * only count active validator stakes if they're usable by the pool, similar to how transient stakes are handled * update the validator status correctly after merging the inactive stake into the reserve * program: Don't remove validator if any lamports present #### Problem In some situations where the transient account is hijacked and a validator has been marked for removal, the stake pool incorrectly counts the transient stake as being part of the pool. During the cleanup phase, the transient lamports immediately get wiped out, causing a share price to drop. Malicious actors can profit from knowing when these spikes will happen. #### Summary of changes This is not the full fix to the issue, but as a start, to avoid spikes in the pool token value by incorrectly wiping out a validator with lamports, simply check to make sure that there are no more lamports delegated to the validator before cleaning up the entry. If a stake pool does enter this state, another update will be required to deactivate the transient stake so that the lamports can be reclaimed. * program: Harden transient stake account checks during update #### Problem Because of the permissionless pool updating functions of the stake pool program, it's possible for malicious users to hijack stake accounts owned by the pool. If someone hijacks a transient stake account and reassigns it to the pool's withdraw authority, it's possible for the pool to wrongly account for the lamports in the hijacked stake account. #### Summary of changes Harden the accounting logic for transient stakes during update. This handles a few edge cases. First, ignore any transient stake account delegated to the incorrect validator. This is a simple check to filter out potential failures later. Next, take proper action based on the `StakeStatus` for the validator: * if `Active`, do the same thing as before * if `DeactivatingTransient` or `DeactivatingAll`, mostly do the same thing as before, but ignore if it's not deactivating or inactive * if `ReadyForRemoval` or `DeactivatingValidator`, ignore * Changes in the way `UpdateValidatorListBalance` treats a transient account * Ignore transient account if the `transient_stake_lamports == 0` (prevents new hijacked accounts from entering the system) * Don't ignore stake due to validator status (ensure we don't lose funds we've accounted for previously) * If the transient account is supposed to deactivate and it isn't - deactivate it * program: Remove delegated validator check for transient #### Problem Now that the update-validator-list code deactivates transient accounts, we no longer need to check if a transient account is properly delegated. In fact, the check will exclude valid transient stake accounts that were once included in the pool. #### Summary of changes Remove the check. * Run cargo fmt * program: Prevent removing validator with transient lamports #### Problem During `WithdrawStake`, it's possible to remove a validator stake account if all the stake accounts are at minimum. However, if that particular validator has a transient stake account, then those lamports are inadvertently removed from the pool at the same time, essentially lost and no longer usable by the pool. #### Summary of changes Add an additional check to not remove a validator if it has any transient lamports associated with it. * program: Reset preferred validator during withdrawal #### Problem During `WithdrawStake`, it's possible to completely remove stake accounts if all validator stake accounts are at a minimum amount. If a preferred validator is removed in this way, the preferred validator is not reset, making it impossible for any other withdrawal to happen. #### Summary of changes Reset the preferred validator during withdraw-stake if needed. At the same time, don't brick all withdrawals if the preferred validator is not found for some reason, and proceed either way. * program: Fail update during reward payout #### Problem It's possible to successfully update a stake pool during rewards payout using the `no_merge` option. This behavior allows users to then interact with the stake pool at an incorrect rate. Although all stake operations would fail until the end of the reward payout period, users could still deposit and withdraw SOL from the reserve. Additionally, after the reward payout phase is finished, there's no way to know that an incorrect update was performed, so someone would need to run another update to get the correct pool values. #### Summary of changes Fail `update_validator_list_balance` if the rewards payout phase is active. * program: Reset preferred validators if inactive or not found #### Summary of changes Added logic to check and reset the preferred deposit and withdrawal validators in the stake pool if they are not found in the active validator list or if their status is not active. This ensures that the stake pool maintains valid preferred validators, preventing potential issues during deposit or withdraw * Instructions.rs: Make the stake pool account writable in the cleanup instruction generation This is following the change to the cleanup instruction, which would check the preferred validators if the stake pool is writable. * program/tests: Add tests for preferred validator behavior during removal and cleanup This commit introduces tests to verify the correct handling of preferred validators in the stake pool. The tests ensure that preferred validators are reset when: 1. A validator is completely removed from the pool through withdrawal. 2. Preferred validators reference non-existent validators. 3. Preferred validators reference inactive validators. These additions enhance the robustness of the stake pool by confirming that the preferred validator settings are appropriately managed during various scenarios, maintaining the integrity of the pool's operations. * Refactor transient stake handling logic in Processor - Simplified the conditions for merging transient stakes into reserves. - Removed redundant checks for validator status and streamlined the logic for handling active and deactivating stakes. - Ensured that transient stake lamports are consistently accounted for after processing. * program: Cleanup clippy lint, fix preferred test #### Problem There were some clippy lints in the program and new tests. Also, the new test failed during the validator removal process. #### Summary of changes Pacify the clippy lints, and copy the validator removal process from the first test so it works as intended. * program: Set status to `DeactivatingAll` if transient lamports #### Problem During a cluster restart situation, or a situation in which stake accounts can be modified outside of the stake protocol, there can be a situation in which a transient stake account is in the `Initialized` state, but there are transient lamports accounted for by the pool. In the current logic, the status would be wrongly set to `DeactivatingValidator`, even though there exists a stake account with transient lamports associated. #### Summary of changes Always set the status to `DeactivatingAll` if transient lamports exist on a given validator. * tests: Fixup enum name * program: Restrict stake check during withdrawal to active #### Problem During withdrawal, we check that there's no possible validator with enough active stake to service the withdrawal, but we also prevent withdrawing from a stake account that's marked for removal. This means it's possible to have a situation where the only withdrawal candidate is being removed, making it impossible to withdraw. #### Summary of changes Restrict the stake check during withdraw-stake to only include active valdiators. * program: Restrict max number of validators in the pool #### Problem If a stake pool has more than ~35k validators, the program will not be able to complete a stake withdrawal in 1.4M CUs, effectively bricking user funds. #### Summary of changes Cap the max number of validators to 20k during initialization, and also when adding new validators, to cover for existing pools. * program: Restrict withdrawal amount from reserve #### Problem It's currently possible to withdraw everything from the reserve using the withdraw-stake instruction, rendering the pool unusable, and in malicious situations, allowing someone to take control of the reserve. #### Summary of changes Check that the reserve has enough lamports to service the withdrawal in withdraw-stake, same as with the withdraw-sol instruction. --------- Co-authored-by: 0xa5df-c <[email protected]>
1 parent 8f705b3 commit 0e56295

File tree

11 files changed

+1075
-110
lines changed

11 files changed

+1075
-110
lines changed

program/src/error.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,12 @@ pub enum StakePoolError {
164164
/// Missing required sysvar account
165165
#[error("Missing required sysvar account")]
166166
MissingRequiredSysvar,
167+
/// Epoch reward distribution is currently in progress, stakes are still being updated
168+
#[error("Epoch reward distribution is currently in progress, stakes are still being updated")]
169+
EpochRewardDistributionInProgress,
170+
/// The stake pool has too many validators in the pool
171+
#[error("The stake pool has too many validators in the pool")]
172+
TooManyValidatorsInPool,
167173
}
168174
impl From<StakePoolError> for ProgramError {
169175
fn from(e: StakePoolError) -> Self {

program/src/instruction.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1640,7 +1640,7 @@ pub fn cleanup_removed_validator_entries(
16401640
validator_list_storage: &Pubkey,
16411641
) -> Instruction {
16421642
let accounts = vec![
1643-
AccountMeta::new_readonly(*stake_pool, false),
1643+
AccountMeta::new(*stake_pool, false),
16441644
AccountMeta::new(*validator_list_storage, false),
16451645
];
16461646
Instruction {

program/src/lib.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,10 @@ pub const WITHDRAWAL_BASELINE_FEE: Fee = Fee {
6161
/// transaction account limits.
6262
pub const MAX_TRANSIENT_STAKE_ACCOUNTS: usize = 10;
6363

64+
/// The maximum number of validators that can be supported in a pool in order
65+
/// for stake withdrawals to still work
66+
pub const MAX_VALIDATORS_IN_POOL: u32 = 20_000;
67+
6468
/// Get the stake amount under consideration when calculating pool token
6569
/// conversions
6670
#[inline]

program/src/processor.rs

Lines changed: 228 additions & 96 deletions
Large diffs are not rendered by default.

program/src/state.rs

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -754,9 +754,15 @@ impl ValidatorStakeInfo {
754754
u64::try_from_slice(&data[8..16]).unwrap() > *lamports
755755
}
756756

757-
/// Check that the validator stake info is valid
758-
pub fn is_not_removed(data: &[u8]) -> bool {
759-
FromPrimitive::from_u8(data[40]) != Some(StakeStatus::ReadyForRemoval)
757+
/// Check that the validator stake info is totally removed
758+
pub fn is_removed(data: &[u8]) -> bool {
759+
FromPrimitive::from_u8(data[40]) == Some(StakeStatus::ReadyForRemoval)
760+
&& data[0..16] == [0; 16] // active and transient stake lamports are 0
761+
}
762+
763+
/// Check that the validator stake info is active
764+
pub fn is_active(data: &[u8]) -> bool {
765+
FromPrimitive::from_u8(data[40]) == Some(StakeStatus::Active)
760766
}
761767
}
762768

program/tests/force_destake.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,11 @@ async fn success_update() {
127127
&voter_pubkey,
128128
)
129129
.await;
130+
131+
// move forward to after rewards payout
132+
let first_normal_slot = context.genesis_config().epoch_schedule.first_normal_slot;
133+
context.warp_to_slot(first_normal_slot + 1).unwrap();
134+
130135
let pre_reserve_lamports = context
131136
.banks_client
132137
.get_account(stake_pool_accounts.reserve_stake.pubkey())

program/tests/initialize.rs

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,9 @@ use {
2323
},
2424
solana_stake_interface as stake,
2525
solana_system_interface::instruction as system_instruction,
26-
spl_stake_pool::{error, id, instruction, state, MINIMUM_RESERVE_LAMPORTS},
26+
spl_stake_pool::{
27+
error, id, instruction, state, MAX_VALIDATORS_IN_POOL, MINIMUM_RESERVE_LAMPORTS,
28+
},
2729
spl_token_2022::extension::ExtensionType,
2830
test_case::test_case,
2931
};
@@ -1632,3 +1634,30 @@ async fn fail_with_incorrect_mint_decimals() {
16321634
)
16331635
);
16341636
}
1637+
1638+
#[tokio::test]
1639+
async fn fail_with_too_many_validators() {
1640+
let (mut banks_client, payer, recent_blockhash) = program_test().start().await;
1641+
let stake_pool_accounts = StakePoolAccounts {
1642+
max_validators: MAX_VALIDATORS_IN_POOL + 1,
1643+
..Default::default()
1644+
};
1645+
let error = stake_pool_accounts
1646+
.initialize_stake_pool(
1647+
&mut banks_client,
1648+
&payer,
1649+
&recent_blockhash,
1650+
MINIMUM_RESERVE_LAMPORTS,
1651+
)
1652+
.await
1653+
.unwrap_err()
1654+
.unwrap();
1655+
1656+
assert_eq!(
1657+
error,
1658+
TransactionError::InstructionError(
1659+
2,
1660+
InstructionError::Custom(error::StakePoolError::TooManyValidatorsInPool as u32),
1661+
)
1662+
);
1663+
}

0 commit comments

Comments
 (0)