From 97a4c15b7f8e001f6b9f13565a4dbc32827a1ee3 Mon Sep 17 00:00:00 2001 From: Jeff Wood Date: Sat, 24 Aug 2024 03:12:35 +0000 Subject: [PATCH 1/3] Update the Token Extensions Program link --- content/courses/tokens-and-nfts/token-program.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/content/courses/tokens-and-nfts/token-program.md b/content/courses/tokens-and-nfts/token-program.md index 08794cf32..fdb4b999a 100644 --- a/content/courses/tokens-and-nfts/token-program.md +++ b/content/courses/tokens-and-nfts/token-program.md @@ -28,7 +28,7 @@ description: - Creating Token Mints and Token Accounts requires allocating **rent** in SOL. The rent for a Token Account can be refunded when the account is closed. Additionally, tokens created with the - [Token Extensions Program](/developers/courses/token-extensions-for-mints/close-mint) + [Token Extensions Program](/content/courses/token-extensions/close-mint) can also close Token Mints. ### Lesson From 2ba6531b7f3723321a0b5c1ae8b072add7f9ad54 Mon Sep 17 00:00:00 2001 From: wuuer Date: Fri, 27 Sep 2024 09:59:44 +0800 Subject: [PATCH 2/3] sync token-program.md from main --- content/courses/tokens-and-nfts/token-program.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/content/courses/tokens-and-nfts/token-program.md b/content/courses/tokens-and-nfts/token-program.md index fdb4b999a..08794cf32 100644 --- a/content/courses/tokens-and-nfts/token-program.md +++ b/content/courses/tokens-and-nfts/token-program.md @@ -28,7 +28,7 @@ description: - Creating Token Mints and Token Accounts requires allocating **rent** in SOL. The rent for a Token Account can be refunded when the account is closed. Additionally, tokens created with the - [Token Extensions Program](/content/courses/token-extensions/close-mint) + [Token Extensions Program](/developers/courses/token-extensions-for-mints/close-mint) can also close Token Mints. ### Lesson From d3add239f1c4fcf3fa3354db6a506fea1ba0ee63 Mon Sep 17 00:00:00 2001 From: wuuer Date: Fri, 27 Sep 2024 10:04:01 +0800 Subject: [PATCH 3/3] =?UTF-8?q?Use=C2=A0[InitSpace](https://docs.rs/anchor?= =?UTF-8?q?-lang/latest/anchor=5Flang/derive.InitSpace.html)=20to=20calcul?= =?UTF-8?q?ate=20space=20needed=20for=20accounts.=20change=20"token=20acco?= =?UTF-8?q?unt"=20in=20the=20"Starter"=20section=20to=20"associated=20toke?= =?UTF-8?q?n=20account"=20change=20"token=20account"=20in=20the=20"Add=20`?= =?UTF-8?q?initialize=5Fpool=5Fsecure"=20section=20to=20"associated=20toke?= =?UTF-8?q?n=20account"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../courses/program-security/pda-sharing.md | 505 +++++++++--------- 1 file changed, 239 insertions(+), 266 deletions(-) diff --git a/content/courses/program-security/pda-sharing.md b/content/courses/program-security/pda-sharing.md index a57bf4a95..dd1868ba8 100644 --- a/content/courses/program-security/pda-sharing.md +++ b/content/courses/program-security/pda-sharing.md @@ -26,25 +26,25 @@ a global PDA to represent the program. However, this opens up the possibility of account validation passing but a user being able to access funds, transfers, or data not belonging to them. -### Insecure Global PDA +### Insecure global PDA In the example below, the `authority` of the `vault` account is a PDA derived using the `mint` address stored on the `pool` account. This PDA is passed into -the instruction handler as the `authority` account to sign for the transfer of -tokens from the `vault` to the `withdraw_destination`. +the instruction as the `authority` account to sign for the transfer tokens from +the `vault` to the `withdraw_destination`. Using the `mint` address as a seed to derive the PDA to sign for the `vault` is insecure because multiple `pool` accounts could be created for the same `vault` -token account, but with different `withdraw_destination` accounts. By using the -`mint` as a `seed` to derive the PDA for signing token transfers, any `pool` -account could sign for the transfer of tokens from a `vault` token account to an -arbitrary `withdraw_destination`. +token account, but a different `withdraw_destination`. By using the `mint` as a +seed derive the PDA to sign for token transfers, any `pool` account could sign +for the transfer of tokens from a `vault` token account to an arbitrary +`withdraw_destination`. ```rust use anchor_lang::prelude::*; use anchor_spl::token::{self, Token, TokenAccount}; -declare_id!("ABQaKhtpYQUUgZ9m2sAY7ZHxWv6KyNdhUJW8Dh8NQbkf"); +declare_id!("Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"); #[program] pub mod pda_sharing_insecure { @@ -53,7 +53,7 @@ pub mod pda_sharing_insecure { pub fn withdraw_tokens(ctx: Context) -> Result<()> { let amount = ctx.accounts.vault.amount; let seeds = &[ctx.accounts.pool.mint.as_ref(), &[ctx.accounts.pool.bump]]; - token::transfer(get_transfer_ctx(&ctx.accounts).with_signer(&[seeds]), amount) + token::transfer(ctx.accounts.transfer_ctx().with_signer(&[seeds]), amount) } } @@ -63,31 +63,29 @@ pub struct WithdrawTokens<'info> { pool: Account<'info, TokenPool>, vault: Account<'info, TokenAccount>, withdraw_destination: Account<'info, TokenAccount>, - /// CHECK: This is the PDA that signs for the transfer + /// CHECK: PDA authority: UncheckedAccount<'info>, token_program: Program<'info, Token>, } -pub fn get_transfer_ctx<'accounts, 'remaining, 'cpi_code, 'info>( - accounts: &'accounts WithdrawTokens<'info>, -) -> CpiContext<'accounts, 'remaining, 'cpi_code, 'info, token::Transfer<'info>> { - CpiContext::new( - accounts.token_program.to_account_info(), - token::Transfer { - from: accounts.vault.to_account_info(), - to: accounts.withdraw_destination.to_account_info(), - authority: accounts.authority.to_account_info(), - }, - ) +impl<'info> WithdrawTokens<'info> { + pub fn transfer_ctx(&self) -> CpiContext<'_, '_, '_, 'info, token::Transfer<'info>> { + let program = self.token_program.to_account_info(); + let accounts = token::Transfer { + from: self.vault.to_account_info(), + to: self.withdraw_destination.to_account_info(), + authority: self.authority.to_account_info(), + }; + CpiContext::new(program, accounts) + } } #[account] -#[derive(InitSpace)] pub struct TokenPool { - pub vault: Pubkey, - pub mint: Pubkey, - pub withdraw_destination: Pubkey, - pub bump: u8, + vault: Pubkey, + mint: Pubkey, + withdraw_destination: Pubkey, + bump: u8, } ``` @@ -96,7 +94,7 @@ pub struct TokenPool { One approach to create an account specific PDA is to use the `withdraw_destination` as a seed to derive the PDA used as the authority of the `vault` token account. This ensures the PDA signing for the CPI in the -`withdraw_tokens` instruction handler is derived using the intended +`withdraw_tokens` instruction is derived using the intended `withdraw_destination` token account. In other words, tokens from a `vault` token account can only be withdrawn to the `withdraw_destination` that was originally initialized with the `pool` account. @@ -117,7 +115,7 @@ pub mod pda_sharing_secure { ctx.accounts.pool.withdraw_destination.as_ref(), &[ctx.accounts.pool.bump], ]; - token::transfer(get_transfer_ctx(&ctx.accounts).with_signer(&[seeds]), amount) + token::transfer(ctx.accounts.transfer_ctx().with_signer(&[seeds]), amount) } } @@ -127,57 +125,54 @@ pub struct WithdrawTokens<'info> { pool: Account<'info, TokenPool>, vault: Account<'info, TokenAccount>, withdraw_destination: Account<'info, TokenAccount>, - /// CHECK: This is the PDA that signs for the transfer + /// CHECK: PDA authority: UncheckedAccount<'info>, token_program: Program<'info, Token>, } -pub fn get_transfer_ctx<'accounts, 'remaining, 'cpi_code, 'info>( - accounts: &'accounts WithdrawTokens<'info>, -) -> CpiContext<'accounts, 'remaining, 'cpi_code, 'info, token::Transfer<'info>> { - CpiContext::new( - accounts.token_program.to_account_info(), - token::Transfer { - from: accounts.vault.to_account_info(), - to: accounts.withdraw_destination.to_account_info(), - authority: accounts.authority.to_account_info(), - }, - ) +impl<'info> WithdrawTokens<'info> { + pub fn transfer_ctx(&self) -> CpiContext<'_, '_, '_, 'info, token::Transfer<'info>> { + let program = self.token_program.to_account_info(); + let accounts = token::Transfer { + from: self.vault.to_account_info(), + to: self.withdraw_destination.to_account_info(), + authority: self.authority.to_account_info(), + }; + CpiContext::new(program, accounts) + } } #[account] -#[derive(InitSpace)] pub struct TokenPool { - pub vault: Pubkey, - pub mint: Pubkey, - pub withdraw_destination: Pubkey, - pub bump: u8, + vault: Pubkey, + mint: Pubkey, + withdraw_destination: Pubkey, + bump: u8, } ``` -### Anchor’s seeds and bump Constraints +### Anchor’s `seeds` and `bump` constraints PDAs can be used as both the address of an account and allow programs to sign for the PDAs they own. The example below uses a PDA derived using the `withdraw_destination` as both -the address of the `pool` account and the owner of the `vault` token account. -This means that only the `pool` account associated with the correct `vault` and -`withdraw_destination` can be used in the `withdraw_tokens` instruction handler. +the address of the `pool` account and owner of the `vault` token account. This +means that only the `pool` account associated with correct `vault` and +`withdraw_destination` can be used in the `withdraw_tokens` instruction. -You can use Anchor’s `seeds` and `bump` constraints with the -[`#[account(...)]`](https://www.anchor-lang.com/docs/account-constraints) +You can use Anchor’s `seeds` and `bump` constraints with the `#[account(...)]` attribute to validate the `pool` account PDA. Anchor derives a PDA using the -`seeds` and `bump` specified and compares it against the account passed into the -instruction handler as the `pool` account. The `has_one` constraint is used to -further ensure that only the correct accounts stored on the `pool` account are -passed into the instruction handler. +`seeds` and `bump` specified and compare against the account passed into the +instruction as the `pool` account. The `has_one` constraint is used to further +ensure that only the correct accounts stored on the `pool` account are passed +into the instruction. ```rust use anchor_lang::prelude::*; use anchor_spl::token::{self, Token, TokenAccount}; -declare_id!("ABQaKhtpYQUUgZ9m2sAY7ZHxWv6KyNdhUJW8Dh8NQbkf"); +declare_id!("Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"); #[program] pub mod pda_sharing_recommended { @@ -189,161 +184,149 @@ pub mod pda_sharing_recommended { ctx.accounts.pool.withdraw_destination.as_ref(), &[ctx.accounts.pool.bump], ]; - token::transfer(get_transfer_ctx(&ctx.accounts).with_signer(&[seeds]), amount) + token::transfer(ctx.accounts.transfer_ctx().with_signer(&[seeds]), amount) } } #[derive(Accounts)] pub struct WithdrawTokens<'info> { #[account( - seeds = [withdraw_destination.key().as_ref()], - bump = pool.bump, - has_one = vault, - has_one = withdraw_destination, - )] + has_one = vault, + has_one = withdraw_destination, + seeds = [withdraw_destination.key().as_ref()], + bump = pool.bump, + )] pool: Account<'info, TokenPool>, - #[account(mut)] vault: Account<'info, TokenAccount>, - #[account(mut)] withdraw_destination: Account<'info, TokenAccount>, token_program: Program<'info, Token>, } -pub fn get_transfer_ctx<'accounts, 'remaining, 'cpi_code, 'info>( - accounts: &'accounts WithdrawTokens<'info>, -) -> CpiContext<'accounts, 'remaining, 'cpi_code, 'info, token::Transfer<'info>> { - CpiContext::new( - accounts.token_program.to_account_info(), - token::Transfer { - from: accounts.vault.to_account_info(), - to: accounts.withdraw_destination.to_account_info(), - authority: accounts.pool.to_account_info(), - }, - ) +impl<'info> WithdrawTokens<'info> { + pub fn transfer_ctx(&self) -> CpiContext<'_, '_, '_, 'info, token::Transfer<'info>> { + let program = self.token_program.to_account_info(); + let accounts = token::Transfer { + from: self.vault.to_account_info(), + to: self.withdraw_destination.to_account_info(), + authority: self.pool.to_account_info(), + }; + CpiContext::new(program, accounts) + } } #[account] -#[derive(InitSpace)] pub struct TokenPool { - pub vault: Pubkey, - pub mint: Pubkey, - pub withdraw_destination: Pubkey, - pub bump: u8, + vault: Pubkey, + mint: Pubkey, + withdraw_destination: Pubkey, + bump: u8, } ``` ## Lab -Let’s practice by creating a simple program to demonstrate how PDA sharing can -allow an attacker to withdraw tokens that don’t belong to them. This lab expands -on the examples above by including the instruction handlers to initialize the -required program accounts. +Let’s practice by creating a simple program to demonstrate how a PDA sharing can +allow an attacker to withdraw tokens that don’t belong to them. this lab expands +on the examples above by including the instructions to initialize the required +program accounts. -### 1. Starter +#### 1. Starter -To get started, download the starter code on the -[`starter` branch of this repository](https://github.com/solana-developers/pda-sharing/tree/starter). -The starter code includes a program with two instruction handlers and the -boilerplate setup for the test file. +To get started, download the starter code on the `starter` branch of +[this repository](https://github.com/solana-developers/pda-sharing/tree/starter). +The starter code includes a program with two instructions and the boilerplate +setup for the test file. -The `initialize_pool` instruction handler initializes a new `TokenPool` that -stores a `vault`, `mint`, `withdraw_destination`, and `bump`. The `vault` is a +The `initialize_pool` instruction initializes a new `TokenPool` that stores a +`vault`, `mint`, `withdraw_destination`, and `bump`. The `vault` is a associated token account where the authority is set as a PDA derived using the `mint` address. -The `withdraw_insecure` instruction handler will transfer tokens in the `vault` -token account to a `withdraw_destination` token account. +The `withdraw_insecure` instruction will transfer tokens in the `vault` token +account to a `withdraw_destination` token account. However, as written the seeds used for signing are not specific to the vault's -withdrawal destination, thus opening up the program to security exploits. Take a +withdraw destination, thus opening up the program to security exploits. Take a minute to familiarize yourself with the code before continuing on. -### 2. Test withdraw_insecure Instruction Handler +#### 2. Test `withdraw_insecure` instruction -The test file includes the code to invoke the `initialize_pool` instruction -handler and then mint 100 tokens to the `vault` token account. It also includes -a test to invoke the `withdraw_insecure` using the intended -`withdraw_destination`. This shows that the instruction handlers can be used as -intended. +The test file includes the code to invoke the `initialize_pool` instruction and +then mint 100 tokens to the `vault` token account. It also includes a test to +invoke the `withdraw_insecure` using the intended `withdraw_destination`. This +shows that the instructions can be used as intended. -After that, there are two more tests to show how the instruction handlers are -vulnerable to exploit. +After that, there are two more tests to show how the instructions are vulnerable +to exploit. -The first test invokes the `initialize_pool` instruction handler to create a -"fake" `pool` account using the same `vault` token account, but a different +The first test invokes the `initialize_pool` instruction to create a "fake" +`pool` account using the same `vault` token account, but a different `withdraw_destination`. The second test withdraws from this pool, stealing funds from the vault. ```typescript -it("allows insecure initialization with incorrect vault", async () => { - try { - await program.methods - .initializePool(insecureAuthorityBump) - .accounts({ - pool: insecurePoolFake.publicKey, - mint: tokenMint, - vault: insecureVault.address, - withdrawDestination: fakeWithdrawDestination, - }) - .signers([insecurePoolFake]) - .rpc(); - - await mintTo( - connection, - wallet.payer, - tokenMint, - insecureVault.address, - wallet.payer, - INITIAL_MINT_AMOUNT, - ); - - const vaultAccount = await getAccount(connection, insecureVault.address); - expect(Number(vaultAccount.amount)).to.equal(INITIAL_MINT_AMOUNT); - } catch (error) { - throw new Error(`Test failed: ${error.message}`); - } +it("Insecure initialize allows pool to be initialized with wrong vault", async () => { + await program.methods + .initializePool(authInsecureBump) + .accounts({ + pool: poolInsecureFake.publicKey, + mint: mint, + vault: vaultInsecure.address, + withdrawDestination: withdrawDestinationFake, + }) + .signers([poolInsecureFake]) + .rpc(); + + await spl.mintTo( + connection, + wallet.payer, + mint, + vaultInsecure.address, + wallet.payer, + 100, + ); + + const account = await spl.getAccount(connection, vaultInsecure.address); + expect(account.amount).eq(100n); }); -it("allows insecure withdrawal to incorrect destination", async () => { - try { - await program.methods - .withdrawInsecure() - .accounts({ - pool: insecurePoolFake.publicKey, - authority: insecureAuthority, - }) - .rpc(); +it("Insecure withdraw allows withdraw to wrong destination", async () => { + await program.methods + .withdrawInsecure() + .accounts({ + pool: poolInsecureFake.publicKey, + authority: authInsecure, + }) + .rpc(); - const vaultAccount = await getAccount(connection, insecureVault.address); - expect(Number(vaultAccount.amount)).to.equal(0); - } catch (error) { - throw new Error(`Test failed: ${error.message}`); - } + const account = await spl.getAccount(connection, vaultInsecure.address); + + expect(account.amount).eq(0n); }); ``` Run `anchor test` to see that the transactions complete successfully and the -`withdraw_instrucure` instruction handler allows the `vault` token account to be -drained to a fake withdraw destination stored on the fake `pool` account. +`withdraw_instrucure` instruction allows the `vault` token account to be drained +to a fake withdraw destination stored on the fake `pool` account. -### 3. Add initialize_pool_secure Instruction Handler +#### 3. Add `initialize_pool_secure` instruction -Now let's add a new instruction handler to the program for securely initializing -a pool. +Now let's add a new instruction to the program for securely initializing a pool. -This new `initialize_pool_secure` instruction handler will initialize a `pool` -account as a PDA derived using the `withdraw_destination`. It will also -initialize a `vault` token account with the authority set as the `pool` PDA. +This new `initialize_pool_secure` instruction will initialize a `pool` account +as a PDA derived using the `withdraw_destination`. It will also initialize a +`vault` associated token account with the authority set as the `pool` PDA. ```rust pub fn initialize_pool_secure(ctx: Context) -> Result<()> { ctx.accounts.pool.vault = ctx.accounts.vault.key(); ctx.accounts.pool.mint = ctx.accounts.mint.key(); ctx.accounts.pool.withdraw_destination = ctx.accounts.withdraw_destination.key(); - ctx.accounts.pool.bump = ctx.bumps.pool; + ctx.accounts.pool.bump = *ctx.bumps.get("pool").unwrap(); Ok(()) } + ... #[derive(Accounts)] @@ -373,27 +356,23 @@ pub struct InitializePoolSecure<'info> { } ``` -### 4. Add withdraw_secure Instruction Handler +#### 4. Add `withdraw_secure` instruction -Next, add a `withdraw_secure` instruction handler. This instruction handler will -withdraw tokens from the `vault` token account to the `withdraw_destination`. -The `pool` account is validated using the `seeds` and `bump` constraints to -ensure the correct PDA account is provided. The `has_one` constraints check that -the correct `vault` and `withdraw_destination` token accounts are provided. +Next, add a `withdraw_secure` instruction. This instruction will withdraw tokens +from the `vault` token account to the `withdraw_destination`. The `pool` account +is validated using the `seeds` and `bump` constraints to ensure the correct PDA +account is provided. The `has_one` constraints check that the correct `vault` +and `withdraw_destination` token accounts are provided. ```rust pub fn withdraw_secure(ctx: Context) -> Result<()> { let amount = ctx.accounts.vault.amount; let seeds = &[ - ctx.accounts.pool.withdraw_destination.as_ref(), - &[ctx.accounts.pool.bump], + ctx.accounts.pool.withdraw_destination.as_ref(), + &[ctx.accounts.pool.bump], ]; - token::transfer( - get_secure_transfer_ctx(&ctx.accounts).with_signer(&[seeds]), - amount, - ) + token::transfer(ctx.accounts.transfer_ctx().with_signer(&[seeds]), amount) } - ... #[derive(Accounts)] @@ -404,87 +383,79 @@ pub struct WithdrawTokensSecure<'info> { seeds = [withdraw_destination.key().as_ref()], bump = pool.bump, )] - pub pool: Account<'info, TokenPool>, + pool: Account<'info, TokenPool>, #[account(mut)] - pub vault: Account<'info, TokenAccount>, + vault: Account<'info, TokenAccount>, #[account(mut)] - pub withdraw_destination: Account<'info, TokenAccount>, - pub token_program: Program<'info, Token>, + withdraw_destination: Account<'info, TokenAccount>, + token_program: Program<'info, Token>, } -pub fn get_secure_transfer_ctx<'accounts, 'remaining, 'cpi_code, 'info>( - accounts: &'accounts WithdrawTokensSecure<'info>, -) -> CpiContext<'accounts, 'remaining, 'cpi_code, 'info, token::Transfer<'info>> { - CpiContext::new( - accounts.token_program.to_account_info(), - token::Transfer { - from: accounts.vault.to_account_info(), - to: accounts.withdraw_destination.to_account_info(), - authority: accounts.pool.to_account_info(), - }, - ) +impl<'info> WithdrawTokensSecure<'info> { + pub fn transfer_ctx(&self) -> CpiContext<'_, '_, '_, 'info, token::Transfer<'info>> { + let program = self.token_program.to_account_info(); + let accounts = token::Transfer { + from: self.vault.to_account_info(), + to: self.withdraw_destination.to_account_info(), + authority: self.pool.to_account_info(), + }; + CpiContext::new(program, accounts) + } } ``` -### 5. Test withdraw_secure Instruction Handler +#### 5. Test `withdraw_secure` instruction -Finally, return to the test file to test the `withdraw_secure` instruction -handler and show that by narrowing the scope of our PDA signing authority, we've -removed the vulnerability. +Finally, return to the test file to test the `withdraw_secure` instruction and +show that by narrowing the scope of our PDA signing authority, we've removed the +vulnerability. Before we write a test showing the vulnerability has been patched let's write a -test that simply shows that the initialization and withdraw instruction handlers -work as expected: +test that simply shows that the initialization and withdraw instructions work as +expected: ```typescript -it("performs secure pool initialization and withdrawal correctly", async () => { - try { - const initialWithdrawBalance = await getAccount( - connection, - withdrawDestination, - ); - - await program.methods - .initializePoolSecure() - .accounts({ - mint: tokenMint, - vault: recommendedVault.publicKey, - withdrawDestination: withdrawDestination, - }) - .signers([recommendedVault]) - .rpc(); - - await new Promise(resolve => setTimeout(resolve, 1000)); - - await mintTo( - connection, - wallet.payer, - tokenMint, - recommendedVault.publicKey, - wallet.payer, - INITIAL_MINT_AMOUNT, - ); - - await program.methods - .withdrawSecure() - .accounts({ - vault: recommendedVault.publicKey, - withdrawDestination: withdrawDestination, - }) - .rpc(); - - const finalWithdrawBalance = await getAccount( - connection, - withdrawDestination, - ); - - expect( - Number(finalWithdrawBalance.amount) - - Number(initialWithdrawBalance.amount), - ).to.equal(INITIAL_MINT_AMOUNT); - } catch (error) { - throw new Error(`Test failed: ${error.message}`); - } +it("Secure pool initialization and withdraw works", async () => { + const withdrawDestinationAccount = await spl.getAccount( + provider.connection, + withdrawDestination, + ); + + await program.methods + .initializePoolSecure() + .accounts({ + mint: mint, + vault: vaultRecommended.publicKey, + withdrawDestination: withdrawDestination, + }) + .signers([vaultRecommended]) + .rpc(); + + await new Promise(x => setTimeout(x, 1000)); + + await spl.mintTo( + connection, + wallet.payer, + mint, + vaultRecommended.publicKey, + wallet.payer, + 100, + ); + + await program.methods + .withdrawSecure() + .accounts({ + vault: vaultRecommended.publicKey, + withdrawDestination: withdrawDestination, + }) + .rpc(); + + const afterAccount = await spl.getAccount( + provider.connection, + withdrawDestination, + ); + + expect(afterAccount.amount - withdrawDestinationAccount.amount).eq(100n); }); ``` @@ -497,21 +468,21 @@ Add a test that shows you can't call `withdraw_secure` with the wrong withdrawal destination. It can use the pool and vault created in the previous test. ```typescript -it("prevents secure withdrawal to incorrect destination", async () => { +it("Secure withdraw doesn't allow withdraw to wrong destination", async () => { try { await program.methods .withdrawSecure() .accounts({ - vault: recommendedVault.publicKey, - withdrawDestination: fakeWithdrawDestination, + vault: vaultRecommended.publicKey, + withdrawDestination: withdrawDestinationFake, }) - .signers([recommendedVault]) + .signers([vaultRecommended]) .rpc(); - throw new Error("Expected an error but withdrawal succeeded"); + assert.fail("expected error"); } catch (error) { - expect(error).to.exist; - console.log("Error message:", error.message); + console.log(error.message); + expect(error); } }); ``` @@ -519,42 +490,44 @@ it("prevents secure withdrawal to incorrect destination", async () => { Lastly, since the `pool` account is a PDA derived using the `withdraw_destination` token account, we can’t create a fake `pool` account using the same PDA. Add one more test showing that the new -`initialize_pool_secure` instruction handler won't let an attacker put in the -wrong vault. +`initialize_pool_secure` instruction won't let an attacker put in the wrong +vault. ```typescript -it("prevents secure pool initialization with incorrect vault", async () => { +it("Secure pool initialization doesn't allow wrong vault", async () => { try { await program.methods .initializePoolSecure() .accounts({ - mint: tokenMint, - vault: insecureVault.address, + mint: mint, + vault: vaultInsecure.address, withdrawDestination: withdrawDestination, }) - .signers([recommendedVault]) + .signers([vaultRecommended]) .rpc(); - throw new Error("Expected an error but initialization succeeded"); + assert.fail("expected error"); } catch (error) { - expect(error).to.exist; - console.log("Error message:", error.message); + console.log(error.message); + expect(error); } }); ``` -Run `anchor test` to see that the new instruction handlers don't allow an -attacker to withdraw from a vault that isn't theirs. - -```bash - PDA sharing - ✔ allows insecure initialization with incorrect vault (852ms) - ✔ allows insecure withdrawal to incorrect destination (425ms) - ✔ performs secure pool initialization and withdrawal correctly (2150ms) -Error message: unknown signer: BpaG3NbsvLUqyFLZo9kWPwda3iPM8abJYkBfwBsASsgi - ✔ prevents secure withdrawal to incorrect destination -Error message: unknown signer: BpaG3NbsvLUqyFLZo9kWPwda3iPM8abJYkBfwBsASsgi - ✔ prevents secure pool initialization with incorrect vault +Run `anchor test` and to see that the new instructions don't allow an attacker +to withdraw from a vault that isn't theirs. + +``` + pda-sharing + ✔ Initialize Pool Insecure (981ms) + ✔ Withdraw (470ms) + ✔ Insecure initialize allows pool to be initialized with wrong vault (10983ms) + ✔ Insecure withdraw allows stealing from vault (492ms) + ✔ Secure pool initialization and withdraw works (2502ms) +unknown signer: ARjxAsEPj6YsAPKaBfd1AzUHbNPtAeUsqusAmBchQTfV + ✔ Secure withdraw doesn't allow withdraw to wrong destination +unknown signer: GJcHJLot3whbY1aC9PtCsBYk5jWoZnZRJPy5uUwzktAY + ✔ Secure pool initialization doesn't allow wrong vault ``` And that's it! Unlike some of the other security vulnerabilities we've @@ -563,7 +536,8 @@ particular Anchor type. You'll need to think through the architecture of your program and ensure that you aren't sharing PDAs across different domains. If you want to take a look at the final solution code you can find it on the -[`solution` branch of the same repository](https://github.com/solana-developers/pda-sharing/tree/solution). +`solution` branch of +[the same repository](https://github.com/solana-developers/pda-sharing/tree/solution). ## Challenge @@ -578,7 +552,6 @@ Remember, if you find a bug or exploit in somebody else's program, please alert them! If you find one in your own program, be sure to patch it right away. - Push your code to GitHub and [tell us what you thought of this lesson](https://form.typeform.com/to/IPH0UGz7#answers-lesson=5744079f-9473-4485-9a14-9be4d31b40d1)!