Skip to content

priority fee distribution account owner is unchecked #293

Description

@neko

describe the bug
copy_priority_fee_distribution checks the expected pda seeds for distribution_account, but does not check that the account is owned by the configured priority fee distribution program.

code:

/// CHECK: Avoiding struct deserialization here to avoid default Owner trait check.
/// `owner = config.priority_fee_distribution_program.key()` here is sufficient.
#[account(
seeds = [
PriorityFeeDistributionAccount::SEED,
vote_account.key().as_ref(),
epoch.to_le_bytes().as_ref(),
],
bump,
seeds::program = config.priority_fee_distribution_program.key(),
)]
pub distribution_account: UncheckedAccount<'info>,
#[account(mut)]
pub signer: Signer<'info>,
}
pub fn handle_copy_priority_fee_distribution_account(
ctx: Context<CopyPriorityFeeDistribution>,
epoch: u64,
) -> Result<()> {
// cant set data in validator history for future epochs
if epoch > Clock::get()?.epoch {
return Err(ValidatorHistoryError::EpochOutOfRange.into());
}
// The PDFA's priority_fee data is not valid until epoch n+1
if epoch == Clock::get()?.epoch {
return Err(ValidatorHistoryError::PriorityFeeDistributionAccountNotFinalized.into());
}
let epoch = cast_epoch(epoch)?;
let mut validator_history_account = ctx.accounts.validator_history_account.load_mut()?;
let validator_history_entry_for_epoch = validator_history_account
.history
.arr_mut()
.iter_mut()
.find(|entry| entry.epoch == epoch);
// This ensures there is no possibility to overwrite a validator history entry after the PFDA
// rent has been reclaimed and introduce an erroneous unstake.
if let Some(entry) = validator_history_entry_for_epoch {
if entry.priority_fee_merkle_root_upload_authority != MerkleRootUploadAuthority::Unset {
return Err(ValidatorHistoryError::PriorityFeeDistributionAccountAlreadyCopied.into());
}
}
let mut pdfa_data: &[u8] = &ctx.accounts.distribution_account.try_borrow_data()?;
let distribution_account = PriorityFeeDistributionAccount::try_deserialize(&mut pdfa_data)
.unwrap_or(PriorityFeeDistributionAccount {
validator_vote_account: Pubkey::default(),
merkle_root_upload_authority: DNE_AUTHORITY,
validator_commission_bps: 0,
total_lamports_transferred: 0,
merkle_root: None,
epoch_created_at: 0,
expires_at: 0,
bump: 0,
});
// If the distribution account is not found, we set the default values of 0 for the commission and priority fees earned
let commission_bps = distribution_account.validator_commission_bps;
let priority_fees_transferred = distribution_account.total_lamports_transferred;
// If the distribution account is not found, we set '11111111111111111111111111111111' as the merkle root upload authority
// passing this to MerkleRootUploadAuthority::from_pubkey resolve to a DNE authority
let merkle_root_upload_authority = distribution_account.merkle_root_upload_authority;
validator_history_account.set_priority_fees_transferred_and_commission(
epoch,
commission_bps,
priority_fees_transferred,
MerkleRootUploadAuthority::from_pubkey(&merkle_root_upload_authority),
)?;

if a caller passes the correct pda address before the real keeper copy, deserialization can fail and the instruction records the account as dne. that poisoned value can persist in validator history.

impact
steward can later treat the validator as having 100% realized priority fee commission and/or a bad priority fee upload authority, reducing score or contributing to unstake decisions once priority fee scoring is active.

note: on the current jitosol steward config this appears effectively disabled today because priority_fee_scoring_start_epoch is in the future and the configured max commission plus error margin is above the 100% value recorded for dne. the current risk is persistent validator-history poisoning and future delegation impact if priority fee scoring is enabled while poisoned epochs remain in lookback.

related scoring paths:

/// Given a validator's tips and total fees, determine their realized commission rate
pub fn calculate_realized_commission_bps(tips: &Option<u64>, total_fees: &Option<u64>) -> u16 {
// total_fees is None when the ValidatorHistoryEntry has been created, but the
// priority_fee_oracle_authority has not called UpdatePriorityFeeHistory
if total_fees.is_none() || total_fees.iter().all(|&f| f == 0) {
return 0;
}
// Default the tips to 0 because we assume the PFDA was not created and the validator is not
// distributing priority fees. This forces inverse_commission to 0 and commission to
// BASIS_POINTS_MAX
let tips = tips.unwrap_or(0);
// Default the total_fees to u64::MAX to force inverse_commission towards 0 and commission
// to BASIS_POINTS_MAX
let total_fees = total_fees.unwrap_or(u64::MAX);
let validators_rake = total_fees.saturating_sub(tips);
// We scale by BASIS_POINTS_MAX before division, so the output is in bps
let numerator = validators_rake.saturating_mul(BASIS_POINTS_MAX as u64);
let commission = numerator.checked_div(total_fees).unwrap_or(0u64);
u16::try_from(commission).unwrap_or(BASIS_POINTS_MAX)

expected behavior
distribution_account should be required to be owned by config.priority_fee_distribution_program.

suggested fix

#[account(owner = config.priority_fee_distribution_program.key())]
pub distribution_account: uncheckedaccount<'info>,

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions