From 7bf2782da29b6106589ebbf247593b6c22617d0d Mon Sep 17 00:00:00 2001 From: guibescos <59208140+guibescos@users.noreply.github.com> Date: Fri, 1 Mar 2024 17:57:29 +0000 Subject: [PATCH] fix: audit feedback 1 (#1331) * Audit 1 * Pin wh sdk * Typo * Typo * Add constraint to initialize --- .../pyth-solana-receiver/src/error.rs | 2 ++ .../programs/pyth-solana-receiver/src/lib.rs | 20 ++++++++++++++----- .../tests/test_governance.rs | 14 +++++++++++++ 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/target_chains/solana/programs/pyth-solana-receiver/src/error.rs b/target_chains/solana/programs/pyth-solana-receiver/src/error.rs index cefc0fd82a..b792db8c67 100644 --- a/target_chains/solana/programs/pyth-solana-receiver/src/error.rs +++ b/target_chains/solana/programs/pyth-solana-receiver/src/error.rs @@ -51,4 +51,6 @@ pub enum ReceiverError { TargetGovernanceAuthorityMismatch, #[msg("The governance authority needs to request a transfer first")] NonexistentGovernanceAuthorityTransferRequest, + #[msg("The minimum number of signatures should be at least 1")] + ZeroMinimumSignatures, } diff --git a/target_chains/solana/programs/pyth-solana-receiver/src/lib.rs b/target_chains/solana/programs/pyth-solana-receiver/src/lib.rs index b9f6e37325..bd4f0c7b7a 100644 --- a/target_chains/solana/programs/pyth-solana-receiver/src/lib.rs +++ b/target_chains/solana/programs/pyth-solana-receiver/src/lib.rs @@ -53,6 +53,10 @@ pub mod pyth_solana_receiver { use super::*; pub fn initialize(ctx: Context, initial_config: Config) -> Result<()> { + require!( + initial_config.minimum_signatures > 0, + ReceiverError::ZeroMinimumSignatures + ); let config = &mut ctx.accounts.config; **config = initial_config; Ok(()) @@ -101,14 +105,21 @@ pub mod pyth_solana_receiver { pub fn set_minimum_signatures(ctx: Context, minimum_signatures: u8) -> Result<()> { let config = &mut ctx.accounts.config; + require!(minimum_signatures > 0, ReceiverError::ZeroMinimumSignatures); config.minimum_signatures = minimum_signatures; Ok(()) } /// Post a price update using a VAA and a MerklePriceUpdate. /// This function allows you to post a price update in a single transaction. - /// Compared to post_update, it is less secure since you won't be able to verify all guardian signatures if you use this function because of transaction size limitations. - /// Typically, you can fit 5 guardian signatures in a transaction that uses this. + /// Compared to `post_update`, it only checks whatever signatures are present in the provided VAA and doesn't fail if the number of signatures is lower than the Wormhole quorum of two thirds of the guardians. + /// The number of signatures that were in the VAA is stored in the `VerificationLevel` of the `PriceUpdateV1` account. + /// + /// We recommend using `post_update_atomic` with 5 signatures. This is close to the maximum signatures you can verify in one transaction without exceeding the transaction size limit. + /// + /// # Warning + /// + /// Using partially verified price updates is dangerous, as it lowers the threshold of guardians that need to collude to produce a malicious price update. pub fn post_update_atomic( ctx: Context, params: PostUpdateAtomicParams, @@ -195,7 +206,7 @@ pub mod pyth_solana_receiver { pub fn post_update(ctx: Context, params: PostUpdateParams) -> Result<()> { let config = &ctx.accounts.config; let payer: &Signer<'_> = &ctx.accounts.payer; - let encoded_vaa = VaaAccount::load(&ctx.accounts.encoded_vaa)?; + let encoded_vaa = VaaAccount::load(&ctx.accounts.encoded_vaa)?; // IMPORTANT: This line checks that the encoded_vaa has ProcessingStatus::Verified. This check is critical otherwise the program could be tricked into accepting unverified VAAs. let treasury: &AccountInfo<'_> = &ctx.accounts.treasury; let price_update_account: &mut Account<'_, PriceUpdateV1> = &mut ctx.accounts.price_update_account; @@ -269,9 +280,8 @@ pub struct PostUpdate<'info> { pub encoded_vaa: AccountInfo<'info>, #[account(seeds = [CONFIG_SEED.as_ref()], bump)] pub config: Account<'info, Config>, - #[account(seeds = [TREASURY_SEED.as_ref(), &[params.treasury_id]], bump)] /// CHECK: This is just a PDA controlled by the program. There is currently no way to withdraw funds from it. - #[account(mut)] + #[account(mut, seeds = [TREASURY_SEED.as_ref(), &[params.treasury_id]], bump)] pub treasury: AccountInfo<'info>, /// The contraint is such that either the price_update_account is uninitialized or the payer is the write_authority. /// Pubkey::default() is the SystemProgram on Solana and it can't sign so it's impossible that price_update_account.write_authority == Pubkey::default() once the account is initialized diff --git a/target_chains/solana/programs/pyth-solana-receiver/tests/test_governance.rs b/target_chains/solana/programs/pyth-solana-receiver/tests/test_governance.rs index 3a308a1c7f..d330701a07 100644 --- a/target_chains/solana/programs/pyth-solana-receiver/tests/test_governance.rs +++ b/target_chains/solana/programs/pyth-solana-receiver/tests/test_governance.rs @@ -253,6 +253,20 @@ async fn test_governance() { initial_config.minimum_signatures ); + // Minimum signatures can't be 0 + assert_eq!( + program_simulator + .process_ix_with_default_compute_limit( + SetMinimumSignatures::populate(governance_authority.pubkey(), 0,), + &vec![&governance_authority], + None, + ) + .await + .unwrap_err() + .unwrap(), + into_transaction_error(ReceiverError::ZeroMinimumSignatures) + ); + program_simulator .process_ix_with_default_compute_limit( SetMinimumSignatures::populate(