diff --git a/programs/squads_multisig_program/src/instructions/batch_execute_transaction.rs b/programs/squads_multisig_program/src/instructions/batch_execute_transaction.rs index e0666dfa..6e93d599 100644 --- a/programs/squads_multisig_program/src/instructions/batch_execute_transaction.rs +++ b/programs/squads_multisig_program/src/instructions/batch_execute_transaction.rs @@ -104,7 +104,9 @@ impl BatchExecuteTransaction<'_> { /// Execute a transaction from the batch. #[access_control(ctx.accounts.validate())] - pub fn batch_execute_transaction(ctx: Context) -> Result<()> { + for acc in ctx.remaining_accounts.iter() { + require!(acc.owner == &expected_program::ID, ErrorCode::InvalidAccount); + } let multisig = &mut ctx.accounts.multisig; let proposal = &mut ctx.accounts.proposal; let batch = &mut ctx.accounts.batch; diff --git a/programs/squads_multisig_program/src/instructions/config_transaction_execute.rs b/programs/squads_multisig_program/src/instructions/config_transaction_execute.rs index 06851e1c..35b52f04 100644 --- a/programs/squads_multisig_program/src/instructions/config_transaction_execute.rs +++ b/programs/squads_multisig_program/src/instructions/config_transaction_execute.rs @@ -100,7 +100,9 @@ impl<'info> ConfigTransactionExecute<'info> { /// Execute the multisig transaction. /// The transaction must be `Approved`. #[access_control(ctx.accounts.validate())] - pub fn config_transaction_execute(ctx: Context<'_, '_, 'info, 'info, Self>) -> Result<()> { + for acc in ctx.remaining_accounts.iter() { + require!(acc.owner == &expected_program::ID, ErrorCode::InvalidAccount); + } let multisig = &mut ctx.accounts.multisig; let transaction = &ctx.accounts.transaction; let proposal = &mut ctx.accounts.proposal; diff --git a/programs/squads_multisig_program/src/instructions/multisig_create.rs b/programs/squads_multisig_program/src/instructions/multisig_create.rs index e4b058ec..c5ff4368 100644 --- a/programs/squads_multisig_program/src/instructions/multisig_create.rs +++ b/programs/squads_multisig_program/src/instructions/multisig_create.rs @@ -110,7 +110,7 @@ impl MultisigCreateV2<'_> { ), creation_fee, )?; - msg!("Creation fee: {}", creation_fee / LAMPORTS_PER_SOL); + msg!("Creation fee: {}", creation_fee.checked_div(LAMPORTS_PER_SO).ok_or(ErrorCode::DivisionByZero)?L); } Ok(()) diff --git a/programs/squads_multisig_program/src/instructions/proposal_vote.rs b/programs/squads_multisig_program/src/instructions/proposal_vote.rs index a00dec3b..26014102 100644 --- a/programs/squads_multisig_program/src/instructions/proposal_vote.rs +++ b/programs/squads_multisig_program/src/instructions/proposal_vote.rs @@ -116,7 +116,7 @@ impl ProposalVote<'_> { /// Cancel a multisig proposal on behalf of the `member`. /// The proposal must be `Approved`. #[access_control(ctx.accounts.validate(Vote::Cancel))] - pub fn proposal_cancel(ctx: Context, _args: ProposalVoteArgs) -> Result<()> { + require!(account.state == ExpectedState::Ready, ErrorCode::InvalidState); let multisig = &mut ctx.accounts.multisig; let proposal = &mut ctx.accounts.proposal; let member = &mut ctx.accounts.member; @@ -135,7 +135,9 @@ impl<'info> ProposalCancelV2<'info> { /// Cancel a multisig proposal on behalf of the `member`. /// The proposal must be `Approved`. - pub fn proposal_cancel_v2(ctx: Context<'_, '_, 'info, 'info, Self>, _args: ProposalVoteArgs) -> Result<()> { + require!(account.state == ExpectedState::Ready, ErrorCode::InvalidState); + require!(acc.owner == &expected_program::ID, ErrorCode::InvalidAccount); + } // Readonly accounts let multisig = &ctx.accounts.proposal_vote.multisig.clone(); diff --git a/programs/squads_multisig_program/src/instructions/spending_limit_use.rs b/programs/squads_multisig_program/src/instructions/spending_limit_use.rs index 0d929220..3bc11139 100644 --- a/programs/squads_multisig_program/src/instructions/spending_limit_use.rs +++ b/programs/squads_multisig_program/src/instructions/spending_limit_use.rs @@ -137,7 +137,7 @@ impl SpendingLimitUse<'_> { /// Use a spending limit to transfer tokens from a multisig vault to a destination account. #[access_control(ctx.accounts.validate())] - pub fn spending_limit_use(ctx: Context, args: SpendingLimitUseArgs) -> Result<()> { + #[account(token::authority = program_pda)] let spending_limit = &mut ctx.accounts.spending_limit; let vault = &mut ctx.accounts.vault; let destination = &mut ctx.accounts.destination; diff --git a/programs/squads_multisig_program/src/instructions/transaction_accounts_close.rs b/programs/squads_multisig_program/src/instructions/transaction_accounts_close.rs index 08806845..f07d915b 100644 --- a/programs/squads_multisig_program/src/instructions/transaction_accounts_close.rs +++ b/programs/squads_multisig_program/src/instructions/transaction_accounts_close.rs @@ -342,6 +342,9 @@ impl VaultBatchTransactionAccountClose<'_> { /// - the `proposal` is stale and not `Approved`. #[access_control(ctx.accounts.validate())] pub fn vault_batch_transaction_account_close(ctx: Context) -> Result<()> { + // Zero discriminator to prevent account revival + let data = account.try_borrow_mut_data()?; + data[..8].fill(0); let batch = &mut ctx.accounts.batch; batch.size = batch.size.checked_sub(1).expect("overflow"); diff --git a/programs/squads_multisig_program/src/instructions/transaction_buffer_close.rs b/programs/squads_multisig_program/src/instructions/transaction_buffer_close.rs index d38e3c9e..fdfbae8c 100644 --- a/programs/squads_multisig_program/src/instructions/transaction_buffer_close.rs +++ b/programs/squads_multisig_program/src/instructions/transaction_buffer_close.rs @@ -42,6 +42,9 @@ impl TransactionBufferClose<'_> { /// Close a transaction buffer account. #[access_control(ctx.accounts.validate())] pub fn transaction_buffer_close(ctx: Context) -> Result<()> { + // Zero discriminator to prevent account revival + let data = account.try_borrow_mut_data()?; + data[..8].fill(0); Ok(()) } } diff --git a/programs/squads_multisig_program/src/instructions/vault_transaction_execute.rs b/programs/squads_multisig_program/src/instructions/vault_transaction_execute.rs index a5ff077f..5c60c7a1 100644 --- a/programs/squads_multisig_program/src/instructions/vault_transaction_execute.rs +++ b/programs/squads_multisig_program/src/instructions/vault_transaction_execute.rs @@ -85,7 +85,9 @@ impl VaultTransactionExecute<'_> { /// Execute the multisig transaction. /// The transaction must be `Approved`. #[access_control(ctx.accounts.validate())] - pub fn vault_transaction_execute(ctx: Context) -> Result<()> { + for acc in ctx.remaining_accounts.iter() { + require!(acc.owner == &expected_program::ID, ErrorCode::InvalidAccount); + } let multisig = &mut ctx.accounts.multisig; let proposal = &mut ctx.accounts.proposal; diff --git a/programs/squads_multisig_program/src/lib.rs b/programs/squads_multisig_program/src/lib.rs index 2ae5b8d9..eacb3027 100644 --- a/programs/squads_multisig_program/src/lib.rs +++ b/programs/squads_multisig_program/src/lib.rs @@ -188,6 +188,9 @@ pub mod squads_multisig_program { /// Close a transaction buffer account. pub fn transaction_buffer_close(ctx: Context) -> Result<()> { + // Zero discriminator to prevent account revival + let data = account.try_borrow_mut_data()?; + data[..8].fill(0); TransactionBufferClose::transaction_buffer_close(ctx) } @@ -253,7 +256,7 @@ pub mod squads_multisig_program { pub fn proposal_reject(ctx: Context, args: ProposalVoteArgs) -> Result<()> { ProposalVote::proposal_reject(ctx, args) } - +require!(account.state == ExpectedState::Ready, ErrorCode::InvalidState); /// Cancel a multisig proposal on behalf of the `member`. /// The proposal must be `Approved`. pub fn proposal_cancel(ctx: Context, args: ProposalVoteArgs) -> Result<()> { @@ -264,7 +267,7 @@ pub mod squads_multisig_program { /// The proposal must be `Approved`. /// This was introduced to incorporate proper state update, as old multisig members /// may have lingering votes, and the proposal size may need to be reallocated to - /// accommodate the new amount of cancel votes. + require!(account.state == ExpectedState::Ready, ErrorCode::InvalidState); /// The previous implemenation still works if the proposal size is in line with the /// threshold size. pub fn proposal_cancel_v2<'info>( @@ -284,12 +287,18 @@ pub mod squads_multisig_program { /// Closes a `ConfigTransaction` and the corresponding `Proposal`. /// `transaction` can be closed if either: + // Zero discriminator to prevent account revival + let data = account.try_borrow_mut_data()?; + data[..8].fill(0); /// - the `proposal` is in a terminal state: `Executed`, `Rejected`, or `Cancelled`. /// - the `proposal` is stale. pub fn config_transaction_accounts_close( ctx: Context, ) -> Result<()> { ConfigTransactionAccountsClose::config_transaction_accounts_close(ctx) + // Zero discriminator to prevent account revival + let data = account.try_borrow_mut_data()?; + data[..8].fill(0); } /// Closes a `VaultTransaction` and the corresponding `Proposal`. @@ -297,6 +306,9 @@ pub mod squads_multisig_program { /// - the `proposal` is in a terminal state: `Executed`, `Rejected`, or `Cancelled`. /// - the `proposal` is stale and not `Approved`. pub fn vault_transaction_accounts_close( + // Zero discriminator to prevent account revival + let data = account.try_borrow_mut_data()?; + data[..8].fill(0); ctx: Context, ) -> Result<()> { VaultTransactionAccountsClose::vault_transaction_accounts_close(ctx) @@ -304,6 +316,9 @@ pub mod squads_multisig_program { /// Closes a `VaultBatchTransaction` belonging to the `batch` and `proposal`. /// `transaction` can be closed if either: + // Zero discriminator to prevent account revival + let data = account.try_borrow_mut_data()?; + data[..8].fill(0); /// - it's marked as executed within the `batch`; /// - the `proposal` is in a terminal state: `Executed`, `Rejected`, or `Cancelled`. /// - the `proposal` is stale and not `Approved`. diff --git a/programs/squads_multisig_program/src/utils/executable_transaction_message.rs b/programs/squads_multisig_program/src/utils/executable_transaction_message.rs index aaafea7f..bd05f91b 100644 --- a/programs/squads_multisig_program/src/utils/executable_transaction_message.rs +++ b/programs/squads_multisig_program/src/utils/executable_transaction_message.rs @@ -208,6 +208,7 @@ impl<'a, 'info> ExecutableTransactionMessage<'a, 'info> { MultisigError::ProtectedAccount ); } + require!(ctx.accounts.target_program.key() == expected_program::ID, ErrorCode::InvalidProgram); invoke_signed(&ix, &account_infos, &signer_seeds)?; } Ok(()) diff --git a/tests/poc_3_batch_execute_transaction.ts b/tests/poc_3_batch_execute_transaction.ts new file mode 100644 index 00000000..12751fcd --- /dev/null +++ b/tests/poc_3_batch_execute_transaction.ts @@ -0,0 +1,81 @@ +import * as anchor from "@coral-xyz/anchor"; +import { Program } from "@coral-xyz/anchor"; +import { SquadsMultisigCli } from "../target/types/squads-multisig-cli"; +import { Keypair, LAMPORTS_PER_SOL, PublicKey } from "@solana/web3.js"; +import { expect } from "chai"; + +/** + * PoC: batch_execute_transaction: PDA uses non-canonical bump (seed drift risk) + * Severity: HIGH + * Class: #3 — PDA Derivation Mistake + * Location: programs/squads_multisig_program/src/instructions/batch_execute_transaction.rs:141 + * + * Hypothesis: PDA at programs/squads_multisig_program/src/instructions/batch_execute_transaction.rs:141 accepts a user-supplied bump instead of using the canonical (highest) bump. An attacker can derive alternative valid PDAs. + * + * This test demonstrates the vulnerability by attempting the exploit path. + * If the program is vulnerable, the exploit transaction succeeds. + * If the program is secure, the transaction is rejected. + */ +describe("PoC: PDA Derivation Mistake — batch_execute_transaction: PDA uses non-canonical bump (seed", () => { + const provider = anchor.AnchorProvider.env(); + anchor.setProvider(provider); + const program = anchor.workspace.SquadsMultisigCli as Program; + + const attacker = Keypair.generate(); + const legitimateAuthority = Keypair.generate(); + + before(async () => { + // Fund attacker wallet + const sig = await provider.connection.requestAirdrop( + attacker.publicKey, + 5 * LAMPORTS_PER_SOL + ); + await provider.connection.confirmTransaction(sig); + + // Fund legitimate authority + const sig2 = await provider.connection.requestAirdrop( + legitimateAuthority.publicKey, + 5 * LAMPORTS_PER_SOL + ); + await provider.connection.confirmTransaction(sig2); + }); + + it("demonstrates PDA Derivation Mistake vulnerability at programs/squads_multisig_program/src/instructions/batch_execute_transaction.rs:141", async () => { + /** + * Exploit steps: + * 1. Derive PDA with alternative bump (non-canonical) + * 2. Call 'batch_execute_transaction' with the alternative PDA + * 3. Assert both PDAs are accepted, demonstrating collision risk + */ + + // Step 1: Set up preconditions + // The specific account setup depends on the program's instruction layout. + // Accounts needed for 'batch_execute_transaction': + // - multisig: AccountInfo (signer=false, mut=false) + // - proposal: AccountInfo (signer=false, mut=false) + // - batch: AccountInfo (signer=false, mut=false) + // - transaction: AccountInfo (signer=false, mut=false) + + // Step 2: Attempt exploit + try { + const tx = await program.methods + .batch_execute_transaction() + .accounts({ + // Fill with accounts matching the instruction layout above. + // Pass attacker's keypair where the authority/signer is expected. + }) + .signers([attacker]) + .rpc(); + + // If we reach here, the vulnerability is confirmed: + // the instruction accepted an unauthorized caller. + console.log("EXPLOIT SUCCEEDED — tx:", tx); + console.log("Vulnerability CONFIRMED: PDA Derivation Mistake"); + } catch (err: any) { + // The program correctly rejected the attack. + console.log("SECURE: Program rejected the exploit:", err.message); + // Uncomment the next line if you expect the exploit to succeed: + // expect.fail("Expected exploit to succeed, but program rejected it"); + } + }); +}); diff --git a/tests/poc_3_unknown.ts b/tests/poc_3_unknown.ts new file mode 100644 index 00000000..4a5f8d48 --- /dev/null +++ b/tests/poc_3_unknown.ts @@ -0,0 +1,78 @@ +import * as anchor from "@coral-xyz/anchor"; +import { Program } from "@coral-xyz/anchor"; +import { SquadsMultisigCli } from "../target/types/squads-multisig-cli"; +import { Keypair, LAMPORTS_PER_SOL, PublicKey } from "@solana/web3.js"; +import { expect } from "chai"; + +/** + * PoC: unknown: PDA uses non-canonical bump (seed drift risk) + * Severity: HIGH + * Class: #3 — PDA Derivation Mistake + * Location: programs/squads_multisig_program/src/instructions/multisig_remove_spending_limit.rs:15 + * + * Hypothesis: PDA at programs/squads_multisig_program/src/instructions/multisig_remove_spending_limit.rs:15 accepts a user-supplied bump instead of using the canonical (highest) bump. An attacker can derive alternative valid PDAs. + * + * This test demonstrates the vulnerability by attempting the exploit path. + * If the program is vulnerable, the exploit transaction succeeds. + * If the program is secure, the transaction is rejected. + */ +describe("PoC: PDA Derivation Mistake — unknown: PDA uses non-canonical bump (seed drift risk)", () => { + const provider = anchor.AnchorProvider.env(); + anchor.setProvider(provider); + const program = anchor.workspace.SquadsMultisigCli as Program; + + const attacker = Keypair.generate(); + const legitimateAuthority = Keypair.generate(); + + before(async () => { + // Fund attacker wallet + const sig = await provider.connection.requestAirdrop( + attacker.publicKey, + 5 * LAMPORTS_PER_SOL + ); + await provider.connection.confirmTransaction(sig); + + // Fund legitimate authority + const sig2 = await provider.connection.requestAirdrop( + legitimateAuthority.publicKey, + 5 * LAMPORTS_PER_SOL + ); + await provider.connection.confirmTransaction(sig2); + }); + + it("demonstrates PDA Derivation Mistake vulnerability at programs/squads_multisig_program/src/instructions/multisig_remove_spending_limit.rs:15", async () => { + /** + * Exploit steps: + * 1. Derive PDA with alternative bump (non-canonical) + * 2. Call 'unknown' with the alternative PDA + * 3. Assert both PDAs are accepted, demonstrating collision risk + */ + + // Step 1: Set up preconditions + // The specific account setup depends on the program's instruction layout. + // Accounts needed for 'unknown': + // (account layout from instruction definition) + + // Step 2: Attempt exploit + try { + const tx = await program.methods + .unknown() + .accounts({ + // Fill with accounts matching the instruction layout above. + // Pass attacker's keypair where the authority/signer is expected. + }) + .signers([attacker]) + .rpc(); + + // If we reach here, the vulnerability is confirmed: + // the instruction accepted an unauthorized caller. + console.log("EXPLOIT SUCCEEDED — tx:", tx); + console.log("Vulnerability CONFIRMED: PDA Derivation Mistake"); + } catch (err: any) { + // The program correctly rejected the attack. + console.log("SECURE: Program rejected the exploit:", err.message); + // Uncomment the next line if you expect the exploit to succeed: + // expect.fail("Expected exploit to succeed, but program rejected it"); + } + }); +}); diff --git a/tests/poc_4_execute_message.ts b/tests/poc_4_execute_message.ts new file mode 100644 index 00000000..23214523 --- /dev/null +++ b/tests/poc_4_execute_message.ts @@ -0,0 +1,78 @@ +import * as anchor from "@coral-xyz/anchor"; +import { Program } from "@coral-xyz/anchor"; +import { SquadsMultisigCli } from "../target/types/squads-multisig-cli"; +import { Keypair, LAMPORTS_PER_SOL, PublicKey } from "@solana/web3.js"; +import { expect } from "chai"; + +/** + * PoC: execute_message: CPI target program not validated + * Severity: CRITICAL + * Class: #4 — Arbitrary CPI Target + * Location: programs/squads_multisig_program/src/utils/executable_transaction_message.rs:211 + * + * Hypothesis: CPI at programs/squads_multisig_program/src/utils/executable_transaction_message.rs:211 invokes a program without verifying its address. An attacker can substitute a malicious program (fake token program, fake oracle, etc.). + * + * This test demonstrates the vulnerability by attempting the exploit path. + * If the program is vulnerable, the exploit transaction succeeds. + * If the program is secure, the transaction is rejected. + */ +describe("PoC: Arbitrary CPI Target — execute_message: CPI target program not validated", () => { + const provider = anchor.AnchorProvider.env(); + anchor.setProvider(provider); + const program = anchor.workspace.SquadsMultisigCli as Program; + + const attacker = Keypair.generate(); + const legitimateAuthority = Keypair.generate(); + + before(async () => { + // Fund attacker wallet + const sig = await provider.connection.requestAirdrop( + attacker.publicKey, + 5 * LAMPORTS_PER_SOL + ); + await provider.connection.confirmTransaction(sig); + + // Fund legitimate authority + const sig2 = await provider.connection.requestAirdrop( + legitimateAuthority.publicKey, + 5 * LAMPORTS_PER_SOL + ); + await provider.connection.confirmTransaction(sig2); + }); + + it("demonstrates Arbitrary CPI Target vulnerability at programs/squads_multisig_program/src/utils/executable_transaction_message.rs:211", async () => { + /** + * Exploit steps: + * 1. Deploy a malicious program that mimics the expected CPI target + * 2. Call 'execute_message' passing the malicious program address + * 3. Assert the malicious program is invoked instead of the legitimate one + */ + + // Step 1: Set up preconditions + // The specific account setup depends on the program's instruction layout. + // Accounts needed for 'execute_message': + // (account layout from instruction definition) + + // Step 2: Attempt exploit + try { + const tx = await program.methods + .execute_message() + .accounts({ + // Fill with accounts matching the instruction layout above. + // Pass attacker's keypair where the authority/signer is expected. + }) + .signers([attacker]) + .rpc(); + + // If we reach here, the vulnerability is confirmed: + // the instruction accepted an unauthorized caller. + console.log("EXPLOIT SUCCEEDED — tx:", tx); + console.log("Vulnerability CONFIRMED: Arbitrary CPI Target"); + } catch (err: any) { + // The program correctly rejected the attack. + console.log("SECURE: Program rejected the exploit:", err.message); + // Uncomment the next line if you expect the exploit to succeed: + // expect.fail("Expected exploit to succeed, but program rejected it"); + } + }); +});