diff --git a/content/courses/program-optimization/program-configuration.md b/content/courses/program-optimization/program-configuration.md index 58760c4b0..e203cdd62 100644 --- a/content/courses/program-optimization/program-configuration.md +++ b/content/courses/program-optimization/program-configuration.md @@ -1,13 +1,13 @@ --- -title: Program Configuration +title: admin configuration objectives: - Define program features in the `Cargo.toml` file - - Use the native Rust `cfg` attribute to conditionally compile code based on - which features are or are not enabled - - Use the native Rust `cfg!` macro to conditionally compile code based on - which features are or are not enabled + - Use the Rust `cfg` attribute to conditionally compile code based on which + features are or are not enabled + - Use the Rust `cfg!` macro to conditionally compile code based on which + features are or are not enabled - Create an admin-only instruction to set up a program account that can be - used to store program configuration values + used to store admin configuration values description: "Create distinct environments, feature flags and admin-only instructions." --- @@ -17,17 +17,15 @@ description: - There are no "out of the box" solutions for creating distinct environments in an onchain program, but you can achieve something similar to environment variables if you get creative. -- You can use the - [`cfg` attribute](https://doc.rust-lang.org/rust-by-example/attribute/cfg.html) - with **Rust features** (`#[cfg(feature = ...)]`) to run different code or - provide different variable values based on the Rust feature provided. _This - happens at compile-time and doesn't allow you to swap values after a program - has been deployed_. -- Similarly, you can use the - [`cfg!` **macro**](https://doc.rust-lang.org/std/macro.cfg.html) to compile - different code paths based on the enabled features. -- For environment-like variables post-deployment, create program accounts and - admin-only instructions accessible by the program's upgrade authority. +- You can use the `cfg` attribute with **Rust features** + (`#[cfg(feature = ...)]`) to run different code or provide different variable + values based on the Rust feature provided. _This happens at compile-time and + doesn't allow you to swap values after a program has been deployed_. +- Similarly, you can use the `cfg!` **macro** to compile different code paths + based on the features that are enabled. +- Alternatively, you can achieve something similar to environment variables that + can be modified after deployment by creating accounts and instructions that + are only accessible by the program’s upgrade authority. ## Lesson @@ -35,31 +33,30 @@ One of the difficulties engineers face across all types of software development is that of writing testable code and creating distinct environments for local development, testing, production, etc. -This is especially difficult in Solana program development. For instance, -imagine building an NFT staking program where each staked NFT earns 10 reward -tokens daily. How can you test the ability to claim rewards when tests run in -just a few hundred milliseconds—not nearly long enough to accrue rewards? +This can be particularly difficult in Solana program development. For example, +imagine creating an NFT staking program that rewards each staked NFT with 10 +reward tokens per day. How do you test the ability to claim rewards when tests +run in a few hundred milliseconds, not nearly long enough to earn rewards? -In traditional web development, this is often addressed through environment -variables, allowing different values in distinct "environments." However, Solana -programs currently lack a formal concept of environment variables. If they -existed, you could easily modify the rewards in your test environment to -something like 10,000,000 tokens per day, making it easier to test claiming -rewards. +Traditional web development solves some of this with environment variables whose +values can differ in each distinct "environment." Currently, there's no formal +concept of environment variables in a Solana program. If there were, you could +just make it so that rewards in your test environment are 10,000,000 tokens per +day and it would be easier to test the ability to claim rewards. -Luckily, you can mimic this functionality with a bit of creativity. The most -effective solution involves a combination of two techniques: +Fortunately, you can achieve similar functionality if you get creative. The best +approach is probably a combination of two things: -1. **Native Rust** feature flags that let you specify the "environment" during - your build, allowing the code to adjust values based on the specified build. -2. **Admin-only** program accounts and instructions that are only accessible by - the program's upgrade `authority` for setting and managing configuration - values post-deployment. +1. Rust feature flags that allow you to specify in your build command the + "environment" of the build, coupled with code that adjusts specific values + accordingly +2. Program "admin-only" accounts and instructions that are only accessible by + the program's upgrade authority -### Native Rust Feature Flags +### Rust feature flags One of the simplest ways to create environments is to use Rust features. -Features are defined in the `[features]` table of the program's `Cargo.toml` +Features are defined in the `[features]` table of the program’s `Cargo.toml` file. You may define multiple features for different use cases. ```toml @@ -82,7 +79,7 @@ You can also specify multiple features by separating them with a comma. anchor test -- --features "feature-one", "feature-two" ``` -#### Make Code Conditional Using the cfg Attribute +#### Make code conditional using the `cfg` attribute With a feature defined, you can then use the `cfg` attribute within your code to conditionally compile code based on whether or not a given feature is enabled. @@ -170,7 +167,7 @@ different implementations of the `constants` module. This allows the program to use different values for the `USDC_MINT_PUBKEY` constant depending on whether or not the `local-testing` feature is enabled. -#### Make Code Conditional using the cfg! Macro +#### Make code conditional using the `cfg!` macro Similar to the `cfg` attribute, the `cfg!` **macro** in Rust allows you to check the values of certain configuration flags at runtime. This can be useful if you @@ -210,7 +207,7 @@ the `local-testing` feature at runtime. If the `local-testing` feature is enabled, the first code path is executed. If the `local-testing` feature is not enabled, the second code path is executed instead. -### Admin-only Instructions +### Admin-only instructions Feature flags are great for adjusting values and code paths at compilation, but they don't help much if you end up needing to adjust something after you've @@ -246,7 +243,7 @@ In Anchor, that simply means creating an account struct and using a single seed to derive the account's address. ```rust -pub const SEED_PROGRAM_CONFIG: &[u8] = b"program_config"; +pub const SEED_ADMIN_CONFIG: &[u8] = b"admin_config"; #[account] pub struct ProgramConfig { @@ -272,8 +269,8 @@ simplest way to do this is to hard-code an admin's public key in your code and then add a simple signer check into your instruction's account validation comparing the signer to this public key. -In Anchor, constraining an `update_program_config` instruction handler to only -be usable by a hard-coded admin might look like this: +In Anchor, constraining an `update_program_config` instruction to only be usable +by a hard-coded admin might look like this: ```rust #[program] @@ -290,25 +287,25 @@ mod my_program { } } -pub const SEED_PROGRAM_CONFIG: &[u8] = b"program_config"; +pub const SEED_ADMIN_CONFIG: &[u8] = b"admin_config"; #[constant] pub const ADMIN_PUBKEY: Pubkey = pubkey!("ADMIN_WALLET_ADDRESS_HERE"); #[derive(Accounts)] pub struct UpdateProgramConfig<'info> { - #[account(mut, seeds = SEED_PROGRAM_CONFIG, bump)] + #[account(mut, seeds = SEED_ADMIN_CONFIG, bump)] pub program_config: Account<'info, ProgramConfig>, #[account(constraint = authority.key() == ADMIN_PUBKEY)] pub authority: Signer<'info>, } ``` -Before instruction handler logic even executes, a check will be performed to -make sure the instruction's signer matches the hard-coded `ADMIN_PUBKEY`. Notice -that the example above doesn't show the instruction handler that initializes the -config account, but it should have similar constraints to ensure that an -attacker can't initialize the account with unexpected values. +Before instruction logic even executes, a check will be performed to make sure +the instruction's signer matches the hard-coded `ADMIN_PUBKEY`. Notice that the +example above doesn't show the instruction that initializes the config account, +but it should have similar constraints to ensure that an attacker can't +initialize the account with unexpected values. While this approach works, it also means keeping track of an admin wallet on top of keeping track of a program's upgrade authority. With a few more lines of @@ -342,7 +339,7 @@ When completed, that looks like this: #[derive(Accounts)] pub struct UpdateProgramConfig<'info> { - #[account(mut, seeds = SEED_PROGRAM_CONFIG, bump)] + #[account(mut, seeds = [SEED_ADMIN_CONFIG], bump)] pub program_config: Account<'info, ProgramConfig>, #[account(constraint = program.programdata_address()? == Some(program_data.key()))] pub program: Program<'info, MyProgram>, @@ -353,7 +350,7 @@ pub struct UpdateProgramConfig<'info> { ``` Again, the example above doesn't show the instruction that initializes the -config account, but it should have the same constraints to ensure that the +config account, but it should have the same constraints to ensure that an attacker can't initialize the account with unexpected values. If this is the first time you've heard about the program data account, it's @@ -368,7 +365,7 @@ want to update the admin to be someone else? For that, you can store the admin on the config account. ```rust -pub const SEED_PROGRAM_CONFIG: &[u8] = b"program_config"; +pub const SEED_ADMIN_CONFIG: &[u8] = b"admin_config"; #[account] pub struct ProgramConfig { @@ -384,11 +381,11 @@ against the config account's `admin` field. ```rust ... -pub const SEED_PROGRAM_CONFIG: &[u8] = b"program_config"; +pub const SEED_ADMIN_CONFIG: &[u8] = b"admin_config"; #[derive(Accounts)] pub struct UpdateProgramConfig<'info> { - #[account(mut, seeds = SEED_PROGRAM_CONFIG, bump)] + #[account(mut, seeds = [SEED_ADMIN_CONFIG], bump)] pub program_config: Account<'info, ProgramConfig>, #[account(constraint = authority.key() == program_config.admin)] pub authority: Signer<'info>, @@ -396,7 +393,7 @@ pub struct UpdateProgramConfig<'info> { ``` There's one catch here: in the time between deploying a program and initializing -the config account, _there is no admin_. This means that the instruction for +the config account, _there is no admin_. Which means that the instruction for initializing the config account can't be constrained to only allow admins as callers. That means it could be called by an attacker looking to set themselves as the admin. @@ -422,25 +419,25 @@ We'll quickly learn while testing our program that it could benefit from the flexibility provided by an admin-controlled configuration account and some feature flags. -### 1. Starter +#### 1. Starter -Download the starter code from -the [`starter` branch of this repository](https://github.com/solana-developers/admin-instructions/tree/starter). -The code contains a program with a single instruction handler and a single test -in the `tests` directory. +Download the starter code from the `starter` branch +of [this repository](https://github.com/Unboxed-Software/solana-admin-instructions/tree/starter). +The code contains a program with a single instruction and a single test in the +`tests` directory. Let's quickly walk through how the program works. The `lib.rs` file includes a constant for the USDC address and a single `payment` instruction. The `payment` instruction simply calls the -`payment_handler` instruction handler in the `instructions/payment.rs` file -where the instruction handler logic is contained. +`payment_handler` function in the `instructions/payment.rs` file where the +instruction logic is contained. The `instructions/payment.rs` file contains both the `payment_handler` function as well as the `Payment` account validation struct representing the accounts -required by the `payment` instruction. The `payment_handler` instruction handler -calculates a 1% fee from the payment amount, transfers the fee to a designated -token account, and transfers the remaining amount to the payment recipient. +required by the `payment` instruction. The `payment_handler` function calculates +a 1% fee from the payment amount, transfers the fee to a designated token +account, and transfers the remaining amount to the payment recipient. Finally, the `tests` directory has a single test file, `config.ts` that simply invokes the `payment` instruction and asserts that the corresponding token @@ -449,7 +446,7 @@ account balances have been debited and credited accordingly. Before we continue, take a few minutes to familiarize yourself with these files and their contents. -### 2. Run the existing test +#### 2. Run the existing test Let's start by running the existing test. @@ -459,18 +456,18 @@ public key for your program printed to the console. This differs based on the keypair you have locally, so you need to update `lib.rs` and `Anchor.toml` to use _your_ key. -Finally, run `anchor test` to start the test. It should fail with the following -output: +Finally, run `anchor test --skip-deploy` to start the test. It should fail with +the following output: -```shell -Error: failed to send transaction: Transaction simulation failed: Error processing Instruction 0: incorrect program id for instruction +``` +Error: AnchorError occurred. Error Code: ConstraintTokenMint. Error Number: 2014. Error Message: A token mint constraint was violated. ``` The reason for this error is that we're attempting to use the mainnet USDC mint address (as hard-coded in the `lib.rs` file of the program), but that mint doesn't exist in the local environment. -### 3. Adding a local-testing feature +#### 3. Adding a `local-testing` feature To fix this, we need a mint we can use locally _and_ hard-code into the program. Since the local environment is reset often during testing, you'll need to store @@ -484,24 +481,16 @@ make the program use our local mint but otherwise use the production USDC mint. Generate a new keypair by running `solana-keygen grind`. Run the following command to generate a keypair with a public key that begins with "env". -```shell +``` solana-keygen grind --starts-with env:1 ``` Once a keypair is found, you should see an output similar to the following: -```shell +``` Wrote keypair to env9Y3szLdqMLU9rXpEGPqkjdvVn8YNHtxYNvCKXmHe.json ``` - - -Make sure to add the generated keypair file -(`env9Y3szLdqMLU9rXpEGPqkjdvVn8YNHtxYNvCKXmHe.json`) to your `.gitignore` file -to prevent accidentally committing and leaking your keypair to GitHub or other -version control platforms. If you plan to use the keypair later, securing it -properly is critical. - The keypair is written to a file in your working directory. Now that we have a placeholder USDC address, let's modify the `lib.rs` file. Use the `cfg` attribute to define the `USDC_MINT_PUBKEY` constant depending on whether the @@ -511,6 +500,7 @@ previous step rather than copying the one below. ```rust use anchor_lang::prelude::*; +use solana_program::{pubkey, pubkey::Pubkey}; mod instructions; use instructions::*; @@ -537,165 +527,71 @@ pub mod config { Next, add the `local-testing` feature to the `Cargo.toml` file located in `/programs`. -```shell +``` [features] ... local-testing = [] ``` Next, update the `config.ts` test file to create a mint using the generated -keypair. Start by deleting the `mint` constant. - -```typescript -const USDC_MINT = new PublicKey("EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v"); -``` - - - -The `anchor test` command, when run on a local network, starts a new test -validator using `solana-test-validator`. This test validator uses a -non-upgradeable loader. The non-upgradeable loader makes it so the program's -`program_data` account isn't initialized when the validator starts. You'll -recall from the lesson that this account is how we access the upgrade authority -from the program. - -To work around this, you can add a `deploy` function to the test file that runs -the deploy command for the program with an upgradeable loader. To use it, run -`anchor test --skip-deploy`, and call the `deploy` function within the test to -run the deploy command after the test validator has started. - -```typescript -import { execSync } from "child_process"; -import path from "path"; - -... - -const deploy = () => { - const workingDirectory = process.cwd(); - const programKeypairPath = path.join( - workingDirectory, - "target", - "deploy", - "config-keypair.json", - ); - const programBinaryPath = path.join( - workingDirectory, - "target", - "deploy", - "config.so", - ); - - const deploy_command = `solana program deploy --url localhost -v --program-id "${programKeypairPath}" "${programBinaryPath}"`; - - try { - execSync(deploy_command, { stdio: "inherit" }); - console.log("Program deployed successfully"); - } catch (error) { - console.error("Error deploying program:", error.message); - throw error; - } -}; - -... - -before(async () => { - deploy(); - ... -}); -``` - -For example, the command to run the test with features would look like this: - -```shell -anchor test --skip-deploy -- --features "local-testing" -``` +keypair. Next, update the test to create a mint using the keypair, which will enable us to reuse the same mint address each time the tests are run. Remember to replace the file name with the one generated in the previous step. ```typescript -let tokenMint: PublicKey; - -const deploy = () => { - const workingDirectory = process.cwd(); - const programKeypairPath = path.join( - workingDirectory, - "target", - "deploy", - "config-keypair.json", - ); - const programBinaryPath = path.join( - workingDirectory, - "target", - "deploy", - "config.so", - ); - - const deploy_command = `solana program deploy --url localhost -v --program-id "${programKeypairPath}" "${programBinaryPath}"`; - - try { - execSync(deploy_command, { stdio: "inherit" }); - console.log("Program deployed successfully"); - } catch (error) { - console.error("Error deploying program:", error.message); - throw error; - } -}; +let mint: anchor.web3.PublicKey before(async () => { - try { - deploy(); - const mintKeypairData = fs.readFileSync( - "envYcAnc9BvWEqDy4VKJsiECCbbc72Fynz87rBih6DV.json" - ); - const mintKeypair = Keypair.fromSecretKey( - new Uint8Array(JSON.parse(mintKeypairData)) - ); - - tokenMint = await createMint( - connection, - walletAuthority.payer, - walletAuthority.publicKey, - null, - 0, - mintKeypair - ); + let rawdata = fs.readFileSync( + "env9Y3szLdqMLU9rXpEGPqkjdvVn8YNHtxYNvCKXmHe.json" + ); + let keyData = JSON.parse(rawdata); + let key = anchor.web3.Keypair.fromSecretKey(new Uint8Array(keyData)); + mint = await spl.createMint( + connection, + wallet.payer, + wallet.publicKey, + null, + 0, + key + ); ... ``` Lastly, run the test with the `local-testing` feature enabled. -```shell +``` anchor test --skip-deploy -- --features "local-testing" ``` You should see the following output: -```shell -Config - ✔ completes payment successfully (432ms) - +``` +config + ✔ Initialize Admin should be successfully (416ms) + ✔ Payment should complete successfully (426ms) - 1 passing (21s) + 2 passing (2s) ``` Boom. Just like that, you've used features to run two different code paths for different environments. -### 4. Program Config +#### 4. admin config Features are great for setting different values at compilation, but what if you wanted to be able to dynamically update the fee percentage used by the program? -Let's make that possible by creating a Program Config account that allows us to +Let's make that possible by creating a admin config account that allows us to update the fee without upgrading the program. To begin, let's first update the `lib.rs` file to: -1. Include a `SEED_PROGRAM_CONFIG` constant, which will be used to generate the - PDA for the program config account. +1. Include a `SEED_ADMIN_CONFIG` constant, which will be used to generate the + PDA for the admin config account. 2. Include an `ADMIN` constant, which will be used as a constraint when - initializing the program config account. Run the `solana address` command to + initializing the admin config account. Run the `solana address` command to get your address to use as the constant's value. 3. Include a `state` module that we'll implement shortly. 4. Include the `initialize_program_config` and `update_program_config` @@ -705,44 +601,46 @@ To begin, let's first update the `lib.rs` file to: ```rust use anchor_lang::prelude::*; mod instructions; -use instructions::*; mod state; +use instructions::*; -declare_id!("FF3eGbZnharYruJNwRV7jqnDYvpLkyvgbSv5gsGbJHps"); +declare_id!("BC3RMBvVa88zSDzPXnBXxpnNYCrKsxnhR3HwwHhuKKei"); -#[cfg(not(feature = "local-testing"))] +#[cfg(feature = "local-testing")] #[constant] -pub const USDC_MINT_PUBKEY: Pubkey = pubkey!("EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v"); +pub const USDC_MINT_PUBKEY: Pubkey = pubkey!("envgiPXWwmpkHFKdy4QLv2cypgAWmVTVEm71YbNpYRu"); -#[cfg(feature = "local-testing")] +#[cfg(not(feature = "local-testing"))] #[constant] -pub const USDC_MINT_PUBKEY: Pubkey = pubkey!("envYcAnc9BvWEqDy4VKJsiECCbbc72Fynz87rBih6DV"); +pub const USDC_MINT_PUBKEY: Pubkey = pubkey!("EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v"); -pub const SEED_PROGRAM_CONFIG: &[u8] = b"program_config"; +pub const SEED_ADMIN_CONFIG: &[u8] = b"admin_config"; #[constant] -pub const ADMIN: Pubkey = pubkey!("GprrWv9r8BMxQiWea9MrbCyK7ig7Mj8CcseEbJhDDZXM"); +pub const ADMIN: Pubkey = pubkey!("..."); #[program] pub mod config { - use super::*; - pub fn payment(ctx: Context, amount: u64) -> Result<()> { - instructions::payment_handler(ctx, amount) - } - pub fn initialize_program_config(ctx: Context) -> Result<()> { instructions::initialize_program_config_handler(ctx) } - pub fn update_program_config(ctx: Context, new_fee: u64) -> Result<()> { + pub fn update_program_config( + ctx: Context, + new_fee: u64, + ) -> Result<()> { instructions::update_program_config_handler(ctx, new_fee) } + + pub fn payment(ctx: Context, amount: u64) -> Result<()> { + instructions::payment_handler(ctx, amount) + } } ``` -### 5. Program Config State +#### 5. admin config State Next, let's define the structure for the `ProgramConfig` state. This account will store the admin, the token account where fees are sent, and the fee rate. @@ -754,64 +652,66 @@ following code. ```rust use anchor_lang::prelude::*; +const DISCRIMINATOR_SIZE: usize = 8; + #[account] #[derive(InitSpace)] -pub struct ProgramConfig { +pub struct AdminConfig { pub admin: Pubkey, pub fee_destination: Pubkey, pub fee_basis_points: u64, } + +impl AdminConfig { + pub const LEN: usize = DISCRIMINATOR_SIZE + AdminConfig::INIT_SPACE; +} ``` -### 6. Add Initialize Program Config Account Instruction +#### 6. Add Initialize admin config Account Instruction -Now let's create the instruction logic for initializing the program config +Now let's create the instruction logic for initializing the admin config account. It should only be callable by a transaction signed by the `ADMIN` key and should set all the properties on the `ProgramConfig` account. Create a folder called `program_config` at the path `/src/instructions/program_config`. This folder will store all instructions -related to the program config account. +related to the admin config account. Within the `program_config` folder, create a file called `initialize_program_config.rs` and add the following code. ```rust -use crate::state::ProgramConfig; -use crate::{ADMIN, SEED_PROGRAM_CONFIG, USDC_MINT_PUBKEY}; +use crate::state::AdminConfig; +use crate::ADMIN_PUBKEY; +use crate::SEED_ADMIN_CONFIG; +use crate::USDC_MINT_PUBKEY; use anchor_lang::prelude::*; use anchor_spl::token::TokenAccount; -pub const DISCRIMINATOR_SIZE: usize = 8; - #[derive(Accounts)] -pub struct InitializeProgramConfig<'info> { - #[account( - init, - seeds = [SEED_PROGRAM_CONFIG], - bump, - payer = authority, - space = DISCRIMINATOR_SIZE + ProgramConfig::INIT_SPACE - )] - pub program_config: Account<'info, ProgramConfig>, - #[account(token::mint = USDC_MINT_PUBKEY)] +pub struct InitializeAdminConfig<'info> { + #[account(init, seeds = [SEED_ADMIN_CONFIG], bump, payer = authority, space = AdminConfig::LEN)] + pub admin_config: Account<'info, AdminConfig>, + #[account( token::mint = USDC_MINT_PUBKEY)] pub fee_destination: Account<'info, TokenAccount>, - #[account(mut, address = ADMIN)] + #[account(mut,address = ADMIN_PUBKEY)] pub authority: Signer<'info>, + #[account(constraint = program.programdata_address()? == Some(program_data.key()))] + pub program: Program<'info, Config>, + #[account(constraint = program_data.upgrade_authority_address == Some(authority.key()))] + pub program_data: Account<'info, ProgramData>, pub system_program: Program<'info, System>, } -pub fn initialize_program_config_handler(ctx: Context) -> Result<()> { - ctx.accounts.program_config.set_inner(ProgramConfig { - admin: ctx.accounts.authority.key(), - fee_destination: ctx.accounts.fee_destination.key(), - fee_basis_points: 100, - }); +pub fn initialize_admin_config_handler(ctx: Context) -> Result<()> { + ctx.accounts.admin_config.admin = ctx.accounts.authority.key(); + ctx.accounts.admin_config.fee_destination = ctx.accounts.fee_destination.key(); + ctx.accounts.admin_config.fee_basis_points = 100; Ok(()) } ``` -### 7. Add Update Program Config Fee Instruction +#### 7. Add Update admin config Fee Instruction Next, implement the instruction logic for updating the config account. The instruction should require that the signer match the `admin` stored in the @@ -822,17 +722,21 @@ Within the `program_config` folder, create a file called ```rust use crate::state::ProgramConfig; -use crate::{SEED_PROGRAM_CONFIG, USDC_MINT_PUBKEY}; +use crate::SEED_ADMIN_CONFIG; +use crate::USDC_MINT_PUBKEY; use anchor_lang::prelude::*; use anchor_spl::token::TokenAccount; #[derive(Accounts)] pub struct UpdateProgramConfig<'info> { - #[account(mut, seeds = [SEED_PROGRAM_CONFIG], bump)] + #[account(mut, seeds = [SEED_ADMIN_CONFIG], bump)] pub program_config: Account<'info, ProgramConfig>, - #[account(token::mint = USDC_MINT_PUBKEY)] + #[account( token::mint = USDC_MINT_PUBKEY)] pub fee_destination: Account<'info, TokenAccount>, - #[account(mut, address = program_config.admin)] + #[account( + mut, + address = program_config.admin, + )] pub admin: Signer<'info>, /// CHECK: arbitrarily assigned by existing admin pub new_admin: UncheckedAccount<'info>, @@ -849,7 +753,7 @@ pub fn update_program_config_handler( } ``` -### 8. Add mod.rs and update instructions.rs +#### 8. Add mod.rs and update instructions.rs Next, let's expose the instruction handlers we created so that the call from `lib.rs` doesn't show an error. Start by adding a file `mod.rs` in the @@ -875,32 +779,42 @@ mod payment; pub use payment::*; ``` -### 9. Update Payment Instruction +#### 9. Update Payment Instruction Lastly, let's update the payment instruction to check that the `fee_destination` account in the instruction matches the `fee_destination` stored in the program config account. Then update the instruction's fee calculation to be based on the -`fee_basis_point` stored in the program config account. +`fee_basis_point` stored in the admin config account. ```rust use crate::state::ProgramConfig; -use crate::{SEED_PROGRAM_CONFIG, USDC_MINT_PUBKEY}; +use crate::SEED_ADMIN_CONFIG; +use crate::USDC_MINT_PUBKEY; use anchor_lang::prelude::*; use anchor_spl::token::{self, Token, TokenAccount}; #[derive(Accounts)] pub struct Payment<'info> { #[account( - seeds = [SEED_PROGRAM_CONFIG], + seeds = [SEED_ADMIN_CONFIG], bump, has_one = fee_destination )] pub program_config: Account<'info, ProgramConfig>, - #[account(mut, token::mint = USDC_MINT_PUBKEY)] + #[account( + mut, + token::mint = USDC_MINT_PUBKEY + )] pub fee_destination: Account<'info, TokenAccount>, - #[account(mut, token::mint = USDC_MINT_PUBKEY)] + #[account( + mut, + token::mint = USDC_MINT_PUBKEY + )] pub sender_token_account: Account<'info, TokenAccount>, - #[account(mut, token::mint = USDC_MINT_PUBKEY)] + #[account( + mut, + token::mint = USDC_MINT_PUBKEY + )] pub receiver_token_account: Account<'info, TokenAccount>, pub token_program: Program<'info, Token>, #[account(mut)] @@ -910,10 +824,10 @@ pub struct Payment<'info> { pub fn payment_handler(ctx: Context, amount: u64) -> Result<()> { let fee_amount = amount .checked_mul(ctx.accounts.program_config.fee_basis_points) - .ok_or(ProgramError::ArithmeticOverflow)? + .unwrap() .checked_div(10000) - .ok_or(ProgramError::ArithmeticOverflow)?; - let remaining_amount = amount.checked_sub(fee_amount).ok_or(ProgramError::ArithmeticOverflow)?; + .unwrap(); + let remaining_amount = amount.checked_sub(fee_amount).unwrap(); msg!("Amount: {}", amount); msg!("Fee Amount: {}", fee_amount); @@ -947,208 +861,174 @@ pub fn payment_handler(ctx: Context, amount: u64) -> Result<()> { } ``` -### 10. Test +#### 10. Test -Now that we're done implementing our new program configuration struct and +Now that we're done implementing our new admin configuration struct and instructions, let's move on to testing our updated program. To begin, add the -PDA for the program config account to the test file. +PDA for the admin config account to the test file. ```typescript -describe("Config", () => { +describe("config", () => { ... - const programConfig = findProgramAddressSync( - [Buffer.from("program_config")], + const adminConfig = PublicKey.findProgramAddressSync( + [Buffer.from("admin_config")], program.programId - )[0] + )[0]; ... ``` Next, update the test file with three more tests testing that: -1. The program config account is initialized correctly +1. The admin config account is initialized correctly 2. The payment instruction is functioning as intended 3. The config account can be updated successfully by the admin 4. The config account cannot be updated by someone other than the admin -The first test initializes the program config account and verifies that the -correct fee is set and that the correct admin is stored on the program config +The first test initializes the admin config account and verifies that the +correct fee is set and that the correct admin is stored on the admin config account. ```typescript -it("initializes program config account", async () => { - try { - await program.methods - .initializeProgramConfig() - .accounts({ - programConfig: programConfig, - feeDestination: feeDestination, - authority: walletAuthority.publicKey, - systemProgram: anchor.web3.SystemProgram.programId, - }) - .rpc(); - - const configAccount = - await program.account.programConfig.fetch(programConfig); - expect(configAccount.feeBasisPoints.toNumber()).to.equal( - INITIAL_FEE_BASIS_POINTS, - ); - expect(configAccount.admin.toString()).to.equal( - walletAuthority.publicKey.toString(), - ); - } catch (error) { - console.error("Program config initialization failed:", error); - throw error; - } +it("Initialize Admin config should be successfully", async () => { + const tx = await program.methods + .initializeAdminConfig() + .accounts({ + feeDestination: feeDestination, + authority: wallet.publicKey, + programData: programDataAddress, + }) + .rpc(); + + assert.strictEqual( + ( + await program.account.adminConfig.fetch(adminConfig) + ).feeBasisPoints.toNumber(), + 100, + ); + assert.strictEqual( + (await program.account.adminConfig.fetch(adminConfig)).admin.toString(), + wallet.publicKey.toString(), + ); }); ``` The second test verifies that the payment instruction is working correctly, with the fee being sent to the fee destination and the remaining balance being transferred to the receiver. Here we update the existing test to include the -`programConfig` account. +`adminConfig` account. ```typescript -it("completes payment successfully", async () => { - try { - const transaction = await program.methods - .payment(new anchor.BN(PAYMENT_AMOUNT)) - .accounts({ - programConfig: programConfig, - feeDestination: feeDestination, - senderTokenAccount: senderTokenAccount, - receiverTokenAccount: receiverTokenAccount, - sender: sender.publicKey, - }) - .transaction(); - - await anchor.web3.sendAndConfirmTransaction(connection, transaction, [ - sender, - ]); +it("Payment should complete successfully", async () => { + const tx = await program.methods + .payment(new anchor.BN(10000)) + .accounts({ + programConfig: programConfig, + feeDestination: feeDestination, + senderTokenAccount: senderTokenAccount, + receiverTokenAccount: receiverTokenAccount, + sender: sender.publicKey, + }) + .transaction(); + + await anchor.web3.sendAndConfirmTransaction(connection, tx, [sender]); + + assert.strictEqual( + (await connection.getTokenAccountBalance(senderTokenAccount)).value + .uiAmount, + 0, + ); - const senderBalance = await getAccount(connection, senderTokenAccount); - const feeDestinationBalance = await getAccount(connection, feeDestination); - const receiverBalance = await getAccount(connection, receiverTokenAccount); + assert.strictEqual( + (await connection.getTokenAccountBalance(feeDestination)).value.uiAmount, + 100, + ); - expect(Number(senderBalance.amount)).to.equal(0); - expect(Number(feeDestinationBalance.amount)).to.equal( - (PAYMENT_AMOUNT * INITIAL_FEE_BASIS_POINTS) / 10000, - ); - expect(Number(receiverBalance.amount)).to.equal( - (PAYMENT_AMOUNT * (10000 - INITIAL_FEE_BASIS_POINTS)) / 10000, - ); - } catch (error) { - console.error("Payment failed:", error); - throw error; - } + assert.strictEqual( + (await connection.getTokenAccountBalance(receiverTokenAccount)).value + .uiAmount, + 9900, + ); }); ``` -The third test attempts to update the fee on the program config account, which +The third test attempts to update the fee on the admin config account, which should be successful. ```typescript -it("updates program config account", async () => { - try { - await program.methods - .updateProgramConfig(new anchor.BN(UPDATED_FEE_BASIS_POINTS)) - .accounts({ - programConfig: programConfig, - admin: walletAuthority.publicKey, - feeDestination: feeDestination, - newAdmin: walletAuthority.publicKey, - }) - .rpc(); - - const configAccount = - await program.account.programConfig.fetch(programConfig); - expect(configAccount.feeBasisPoints.toNumber()).to.equal( - UPDATED_FEE_BASIS_POINTS, - ); - } catch (error) { - console.error("Program config update failed:", error); - throw error; - } +it("Admin Config Update should be successfully", async () => { + const tx = await program.methods + .updateAdminConfig(new anchor.BN(200)) + .accounts({ + feeDestination: feeDestination, + newAdmin: sender.publicKey, + }) + .rpc(); + + assert.strictEqual( + ( + await program.account.adminConfig.fetch(adminConfig) + ).feeBasisPoints.toNumber(), + 200, + ); }); ``` -The fourth test tries to update the fee on the program config account, where the -admin is not the one stored on the program config account, and this should fail. +The fourth test tries to update the fee on the admin config account, where the +admin is not the one stored on the admin config account, and this should fail. ```typescript -it("fails to update program config account with unauthorized admin", async () => { +it("Admin Config Update with unauthorized admin should throw an exception", async () => { try { - const transaction = await program.methods - .updateProgramConfig(new anchor.BN(300)) + const tx = await program.methods + .updateAdminConfig(new anchor.BN(300)) .accounts({ - programConfig: programConfig, - admin: sender.publicKey, feeDestination: feeDestination, newAdmin: sender.publicKey, + admin: sender.publicKey, //ignore an error on this line }) - .transaction(); - - await anchor.web3.sendAndConfirmTransaction(connection, transaction, [ - sender, - ]); - throw new Error("Expected transaction to fail, but it succeeded"); - } catch (error) { - expect(error).to.exist; - console.log("Transaction failed as expected:", error.message); + .signers([sender]) + .rpc(); + } catch (err) { + expect(err); + console.log(err.message); + return; } + + assert.fail("should throw an exception"); }); ``` Finally, run the test using the following command: -```shell +``` anchor test --skip-deploy -- --features "local-testing" ``` You should see the following output: -```shell -Config - ✔ initializes program config account (430ms) - ✔ completes payment successfully (438ms) - ✔ updates program config account (416ms) -Transaction failed as expected: Simulation failed. -Message: Transaction simulation failed: Error processing Instruction 0: custom program error: 0x7dc. -Logs: -[ - "Program FF3eGbZnharYruJNwRV7jqnDYvpLkyvgbSv5gsGbJHps invoke [1]", - "Program log: Instruction: UpdateProgramConfig", - "Program log: AnchorError caused by account: admin. Error Code: ConstraintAddress. Error Number: 2012. Error Message: An address constraint was violated.", - "Program log: Left:", - "Program log: F32dEMPn4BtQjHBgXXwfuEMo5qBQJySs8cCDrtwWQdBr", - "Program log: Right:", - "Program log: GprrWv9r8BMxQiWea9MrbCyK7ig7Mj8CcseEbJhDDZXM", - "Program FF3eGbZnharYruJNwRV7jqnDYvpLkyvgbSv5gsGbJHps consumed 7868 of 200000 compute units", - "Program FF3eGbZnharYruJNwRV7jqnDYvpLkyvgbSv5gsGbJHps failed: custom program error: 0x7dc" -]. -Catch the `SendTransactionError` and call `getLogs()` on it for full details. - ✔ fails to update program config account with unauthorized admin - - - 4 passing (22s) +``` +config + ✔ Initialize Admin config should be successfully (416ms) + ✔ Payment should complete successfully (419ms) + ✔ Admin Config Update should be successfully (414ms) +AnchorError caused by account: admin. Error Code: ConstraintAddress. Error Number: 2012. Error Message: An address constraint was violated. +Program log: Left: +Program log: 9DFY8t5NsZ2g8bQDypYZ5spHzBt6qjbVUgKhFxATMBRv +Program log: Right: +Program log: CbcfaDHPwur22CzyWmzn6b4kBe3ntsw9Hu1UTWt9q33Y + ✔ Admin Config Update with unauthorized admin should throw an exception + +4 passing (8s) ``` And that's it! You've made the program a lot easier to work with moving forward. If you want to take a look at the final solution code you can find it on -the [`solution` branch of the same](https://github.com/solana-developers/admin-instructions/tree/solution). +the `solution` branch +of [the same repository](https://github.com/Unboxed-Software/solana-admin-instructions/tree/solution). ## Challenge -Now it's time for you to do some of this on your own. We mentioned being able to -use the program's upgrade authority as the initial admin. Go ahead and update -the lab's `initialize_program_config` so that only the upgrade authority can -call it rather than having a hardcoded `ADMIN`. - -Try doing this on your own, but if you get stuck, feel free to reference the -[`challenge` branch of the same repository](https://github.com/solana-developers/admin-instructions/tree/challenge) -to see one possible solution. - - Push your code to GitHub and [tell us what you thought of this lesson](https://form.typeform.com/to/IPH0UGz7#answers-lesson=02a7dab7-d9c1-495b-928c-a4412006ec20)! diff --git a/content/courses/program-security/arbitrary-cpi.md b/content/courses/program-security/arbitrary-cpi.md index edb15793d..2fc6c93b3 100644 --- a/content/courses/program-security/arbitrary-cpi.md +++ b/content/courses/program-security/arbitrary-cpi.md @@ -3,7 +3,7 @@ title: Arbitrary CPI objectives: - Explain the security risks associated with invoking a CPI to an unknown program - - Showcase how Anchor's CPI module prevents this from happening when making a + - Showcase how Anchor’s CPI module prevents this from happening when making a CPI from one Anchor program to another - Safely and securely make a CPI from an Anchor program to an arbitrary non-anchor program @@ -13,9 +13,9 @@ description: "How to safely invoke Solana programs from other Solana programs." ## Summary - To generate a CPI, the target program must be passed into the invoking - instruction handler as an account. This means that any target program could be - passed into the instruction handler. Your program should check for incorrect - or unexpected programs. + instruction as an account. This means that any target program could be passed + into the instruction. Your program should check for incorrect or unexpected + programs. - Perform program checks in native programs by simply comparing the public key of the passed-in program to the progam you expected. - If a program is written in Anchor, then it may have a publicly available CPI @@ -25,27 +25,27 @@ description: "How to safely invoke Solana programs from other Solana programs." ## Lesson -A cross program invocation (CPI) is when one program invokes an instruction -handler on another program. An “arbitrary CPI” is when a program is structured -to issue a CPI to whatever program is passed into the instruction handler rather -than expecting to perform a CPI to one specific program. Given that the callers -of your program's instruction handler can pass any program they'd like into the -instruction's list of accounts, failing to verify the address of a passed-in -program results in your program performing CPIs to arbitrary programs. +A cross program invocation (CPI) is when one program invokes an instruction on +another program. An “arbitrary CPI” is when a program is structured to issue a +CPI to whatever program is passed into the instruction rather than expecting to +perform a CPI to one specific program. Given that the callers of your program's +instruction can pass any program they'd like into the instruction's list of +accounts, failing to verify the address of a passed-in program results in your +program performing CPIs to arbitrary programs. This lack of program checks creates an opportunity for a malicious user to pass in a different program than expected, causing the original program to call an -instruction handler on this mystery program. There's no telling what the -consequences of this CPI could be. It depends on the program logic (both that of -the original program and the unexpected program), as well as what other accounts -are passed into the original instruction handler. +instruction on this mystery program. There’s no telling what the consequences of +this CPI could be. It depends on the program logic (both that of the original +program and the unexpected program), as well as what other accounts are passed +into the original instruction. -### Missing Program Checks +### Missing program checks -Take the following program as an example. The `cpi` instruction handler invokes -the `transfer` instruction handler on `token_program`, but there is no code that -checks whether or not the `token_program` account passed into the instruction -handler is, in fact, the SPL Token Program. +Take the following program as an example. The `cpi` instruction invokes the +`transfer` instruction on `token_program`, but there is no code that checks +whether or not the `token_program` account passed into the instruction is, in +fact, the SPL Token Program. ```rust use anchor_lang::prelude::*; @@ -85,14 +85,14 @@ pub struct Cpi<'info> { } ``` -An attacker could easily call this instruction handler and pass in a duplicate -token program that they created and control. +An attacker could easily call this instruction and pass in a duplicate token +program that they created and control. -### Add Program Checks +### Add program checks It's possible to fix this vulnerabilty by simply adding a few lines to the `cpi` -instruction handler to check whether or not `token_program`'s public key is that -of the SPL Token Program. +instruction to check whether or not `token_program`'s public key is that of the +SPL Token Program. ```rust pub fn cpi_secure(ctx: Context, amount: u64) -> ProgramResult { @@ -117,23 +117,21 @@ pub fn cpi_secure(ctx: Context, amount: u64) -> ProgramResult { } ``` -Now, if an attacker passes in a different token program, the instruction handler -will return the `ProgramError::IncorrectProgramId` error. +Now, if an attacker passes in a different token program, the instruction will +return the `ProgramError::IncorrectProgramId` error. -Depending on the program you're invoking with your CPI, you can either hard code -the address of the expected program ID or use the program's Rust crate to get +Depending on the program you’re invoking with your CPI, you can either hard code +the address of the expected program ID or use the program’s Rust crate to get the address of the program, if available. In the example above, the `spl_token` crate provides the address of the SPL Token Program. -### Use an Anchor CPI Module +### Use an Anchor CPI module -A simpler way to manage program checks is to use -[Anchor CPI](https://book.anchor-lang.com/anchor_in_depth/CPIs.html) module. We -learned in a -[previous lesson of Anchor CPI](/content/courses/onchain-development/anchor-cpi.md) -that Anchor can automatically generate CPI modules to make CPIs into the program +A simpler way to manage program checks is to use Anchor CPI modules. We learned +in a [previous lesson](/content/courses/onchain-development/anchor-cpi.md) that +Anchor can automatically generate CPI modules to make CPIs into the program simpler. These modules also enhance security by verifying the public key of the -program that's passed into one of its public instructions. +program that’s passed into one of its public instructions. Every Anchor program uses the `declare_id()` macro to define the address of the program. When a CPI module is generated for a specific program, it uses the @@ -183,15 +181,12 @@ impl<'info> Cpi<'info> { } ``` - - -Like the example above, Anchor has created a few +Note that, like the example above, Anchor has created a few [wrappers for popular native programs](https://github.com/coral-xyz/anchor/tree/master/spl/src) that allow you to issue CPIs into them as if they were Anchor programs. - -Additionally and depending on the program you're making the CPI to, you may be -able to use Anchor's +Additionally and depending on the program you’re making the CPI to, you may be +able to use Anchor’s [`Program` account type](https://docs.rs/anchor-lang/latest/anchor_lang/accounts/program/struct.Program.html) to validate the passed-in program in your account validation struct. Between the [`anchor_lang`](https://docs.rs/anchor-lang/latest/anchor_lang) and [`anchor_spl`](https://docs.rs/anchor_spl/latest/) crates, @@ -222,10 +217,10 @@ mints, distribution, and transfers, and a separate metadata program is used to assign metadata to tokens. So the vulnerability we go through here could also be applied to real tokens. -### 1. Setup +#### 1. Setup -We'll start with the -[`starter` branch of this repository](https://github.com/solana-developers/arbitrary-cpi/tree/starter). +We'll start with the `starter` branch of +[this repository](https://github.com/Unboxed-Software/solana-arbitrary-cpi/tree/starter). Clone the repository and then open it on the `starter` branch. Notice that there are three programs: @@ -246,7 +241,7 @@ look at the program. It has two instructions: The second program, `character-metadata`, is meant to be the "approved" program for handling character metadata. Have a look at this program. It has a single -instruction handler for `create_metadata` that creates a new PDA and assigns a +instruction for `create_metadata` that creates a new PDA and assigns a pseudo-random value between 0 and 20 for the character's health and power. The last program, `fake-metadata` is a "fake" metadata program meant to @@ -254,63 +249,59 @@ illustrate what an attacker might make to exploit our `gameplay` program. This program is almost identical to the `character-metadata` program, only it assigns a character's initial health and power to be the max allowed: 255. -### 2. Test create_character_insecure Instruction Handler +#### 2. Test `create_character_insecure` instruction There is already a test in the `tests` directory for this. It's long, but take a minute to look at it before we talk through it together: ```typescript it("Insecure instructions allow attacker to win every time successfully", async () => { - try { - // Initialize player one with real metadata program - await gameplayProgram.methods - .createCharacterInsecure() - .accounts({ - metadataProgram: metadataProgram.programId, - authority: playerOne.publicKey, - }) - .signers([playerOne]) - .rpc(); - - // Initialize attacker with fake metadata program - await gameplayProgram.methods - .createCharacterInsecure() - .accounts({ - metadataProgram: fakeMetadataProgram.programId, - authority: attacker.publicKey, - }) - .signers([attacker]) - .rpc(); - - // Fetch both player's metadata accounts - const [playerOneMetadataKey] = getMetadataKey( - playerOne.publicKey, - gameplayProgram.programId, - metadataProgram.programId, - ); - - const [attackerMetadataKey] = getMetadataKey( - attacker.publicKey, - gameplayProgram.programId, - fakeMetadataProgram.programId, - ); - - const playerOneMetadata = - await metadataProgram.account.metadata.fetch(playerOneMetadataKey); - - const attackerMetadata = - await fakeMetadataProgram.account.metadata.fetch(attackerMetadataKey); - // The regular player should have health and power between 0 and 20 - expect(playerOneMetadata.health).to.be.lessThan(20); - expect(playerOneMetadata.power).to.be.lessThan(20); - - // The attacker will have health and power of 255 - expect(attackerMetadata.health).to.equal(255); - expect(attackerMetadata.power).to.equal(255); - } catch (error) { - console.error("Test failed:", error); - throw error; - } + // Initialize player one with real metadata program + await gameplayProgram.methods + .createCharacterInsecure() + .accounts({ + metadataProgram: metadataProgram.programId, + authority: playerOne.publicKey, + }) + .signers([playerOne]) + .rpc(); + + // Initialize attacker with fake metadata program + await gameplayProgram.methods + .createCharacterInsecure() + .accounts({ + metadataProgram: fakeMetadataProgram.programId, + authority: attacker.publicKey, + }) + .signers([attacker]) + .rpc(); + + // Fetch both player's metadata accounts + const [playerOneMetadataKey] = getMetadataKey( + playerOne.publicKey, + gameplayProgram.programId, + metadataProgram.programId, + ); + + const [attackerMetadataKey] = getMetadataKey( + attacker.publicKey, + gameplayProgram.programId, + fakeMetadataProgram.programId, + ); + + const playerOneMetadata = + await metadataProgram.account.metadata.fetch(playerOneMetadataKey); + + const attackerMetadata = + await fakeMetadataProgram.account.metadata.fetch(attackerMetadataKey); + + // The regular player should have health and power between 0 and 20 + expect(playerOneMetadata.health).to.be.lessThan(20); + expect(playerOneMetadata.power).to.be.lessThan(20); + + // The attacker will have health and power of 255 + expect(attackerMetadata.health).to.equal(255); + expect(attackerMetadata.power).to.equal(255); }); ``` @@ -327,12 +318,12 @@ are each 255, making the attacker unbeatable. If you haven't already, run `anchor test` to see that this test in fact behaves as described. -### 3. Create a create_character_secure Instruction Handler +#### 3. Create a `create_character_secure` instruction -Let's fix this by creating a secure instruction handler for creating a new -character. This instruction handler should implement proper program checks and -use the `character-metadata` program's `cpi` crate to do the CPI rather than -just using `invoke`. +Let's fix this by creating a secure instruction for creating a new character. +This instruction should implement proper program checks and use the +`character-metadata` program's `cpi` crate to do the CPI rather than just using +`invoke`. If you want to test out your skills, try this on your own before moving ahead. @@ -371,27 +362,25 @@ pub struct CreateCharacterSecure<'info> { seeds::program = metadata_program.key(), bump, )] - /// CHECK: This account will not be checked by anchor + /// CHECK: manual checks pub metadata_account: AccountInfo<'info>, pub metadata_program: Program<'info, CharacterMetadata>, pub system_program: Program<'info, System>, } ``` -Lastly, we add the `create_character_secure` instruction handler. It will be the -same as before but will use the full functionality of Anchor CPIs rather than -using `invoke` directly: +Lastly, we add the `create_character_secure` instruction. It will be the same as +before but will use the full functionality of Anchor CPIs rather than using +`invoke` directly: ```rust pub fn create_character_secure(ctx: Context) -> Result<()> { - // Initialize character data let character = &mut ctx.accounts.character; character.metadata = ctx.accounts.metadata_account.key(); - character.authority = ctx.accounts.authority.key(); + character.auth = ctx.accounts.authority.key(); character.wins = 0; - // Prepare CPI context - let cpi_context = CpiContext::new( + let context = CpiContext::new( ctx.accounts.metadata_program.to_account_info(), CreateMetadata { character: ctx.accounts.character.to_account_info(), @@ -401,49 +390,46 @@ pub fn create_character_secure(ctx: Context) -> Result<() }, ); - // Perform CPI to create metadata - create_metadata(cpi_context)?; + create_metadata(context)?; Ok(()) } ``` -### 4. Test create_character_secure Instruction Handler +#### 4. Test `create_character_secure` Now that we have a secure way of initializing a new character, let's create a new test. This test just needs to attempt to initialize the attacker's character and expect an error to be thrown. ```typescript -it("prevents secure character creation with fake program", async () => { +it("Secure character creation with fake program should throw an exception", async () => { try { await gameplayProgram.methods .createCharacterSecure() .accounts({ - metadataProgram: fakeMetadataProgram.programId, + metadataProgram: fakeMetadataProgram.programId, // despite the compile error on this line. authority: attacker.publicKey, }) .signers([attacker]) .rpc(); - - throw new Error("Expected createCharacterSecure to throw an error"); } catch (error) { - expect(error).to.be.instanceOf(Error); + expect(error); console.log(error); } }); ``` Run `anchor test` if you haven't already. Notice that an error was thrown as -expected, detailing that the program ID passed into the instruction handler is -not the expected program ID: +expected, detailing that the program ID passed into the instruction is not the +expected program ID: ```bash 'Program log: AnchorError caused by account: metadata_program. Error Code: InvalidProgramId. Error Number: 3008. Error Message: Program ID was not as expected.', 'Program log: Left:', -'Program log: HQqG7PxftCD5BB9WUWcYksrjDLUwCmbV8Smh1W8CEgQm', +'Program log: FKBWhshzcQa29cCyaXc1vfkZ5U985gD5YsqfCzJYUBr', 'Program log: Right:', -'Program log: 4FgVd2dgsFnXbSHz8fj9twNbfx8KWcBJkHa6APicU6KS' +'Program log: D4hPnYEsAx4u3EQMrKEXsY3MkfLndXbBKTEYTwwm25TE' ``` That's all you need to do to protect against arbitrary CPIs! @@ -453,7 +439,8 @@ certainly won't stop you from architecting the program you need, but please take every precaution possible to ensure no vulnerabilities in your program. 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/arbitrary-cpi/tree/solution). +`solution` branch of +[the same repository](https://github.com/Unboxed-Software/solana-arbitrary-cpi/tree/solution). ## Challenge @@ -461,14 +448,13 @@ Just as with other lessons in this unit, your opportunity to practice avoiding this security exploit lies in auditing your own or other programs. Take some time to review at least one program and ensure that program checks are -in place for every program passed into the instruction handlers, particularly -those that are invoked via CPI. +in place for every program passed into the instructions, particularly those that +are invoked via CPI. 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=5bcaf062-c356-4b58-80a0-12cca99c29b0)! diff --git a/content/courses/program-security/closing-accounts.md b/content/courses/program-security/closing-accounts.md index d4a9b28de..09edd28ed 100644 --- a/content/courses/program-security/closing-accounts.md +++ b/content/courses/program-security/closing-accounts.md @@ -18,8 +18,7 @@ description: exempt. Closing accounts involves transferring the lamports stored in the account for rent exemption to another account of your choosing. - You can use the Anchor `#[account(close = )]` - constraint to securely close accounts and set the account discriminator to the - `CLOSED_ACCOUNT_DISCRIMINATOR` + constraint to securely close accounts. ```rust #[account(mut, close = receiver)] pub data_account: Account<'info, MyData>, @@ -33,7 +32,7 @@ While it sounds simple, closing accounts properly can be tricky. There are a number of ways an attacker could circumvent having the account closed if you don't follow specific steps. -To get a better understanding of these attack vectors, let's explore each of +To get a better understanding of these attack vectors, let’s explore each of these scenarios in depth. ### Insecure account closing @@ -45,10 +44,10 @@ account. This resets the owner from the owning program to the system program. Take a look at the example below. The instruction requires two accounts: 1. `account_to_close` - the account to be closed -2. `destination` - the account that should receive the closed account's lamports +2. `destination` - the account that should receive the closed account’s lamports The program logic is intended to close an account by simply increasing the -`destination` account's lamports by the amount stored in the `account_to_close` +`destination` account’s lamports by the amount stored in the `account_to_close` and setting the `account_to_close` lamports to 0. With this program, after a full transaction is processed, the `account_to_close` will be garbage collected by the runtime. @@ -96,58 +95,39 @@ the program and even drain a protocol. ### Secure account closing -The two most important things you can do to close this loophole are to zero out -the account data and add an account discriminator that represents the account -has been closed. You need _both_ of these things to avoid unintended program -behavior. - -An account with zeroed out data can still be used for some things, especially if -it's a PDA whose address derivation is used within the program for verification -purposes. However, the damage may be potentially limited if the attacker can't -access the previously-stored data. - -To further secure the program, however, closed accounts should be given an -account discriminator that designates it as "closed," and all instructions -should perform checks on all passed-in accounts that return an error if the -account is marked closed. +The two most important things you can do to close this loophole are to change +the account's ownership and reallocate the size of the account's data with 0 +bytes. Look at the example below. This program transfers the lamports out of an -account, zeroes out the account data, and sets an account discriminator in a -single instruction in hopes of preventing a subsequent instruction from -utilizing this account again before it has been garbage collected. Failing to do -any one of these things would result in a security vulnerability. +account, changes the account's ownership to the system program, and reallocates +the size of the account's data with 0 bytes in hopes of preventing a subsequent +instruction from utilizing th is account again before it has been garbage +collected. Failing to do any one of these things would result in a security +vulnerability. ```rust use anchor_lang::prelude::*; -use std::io::Write; -use std::ops::DerefMut; declare_id!("Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"); #[program] -pub mod closing_accounts_insecure_still_still { +pub mod closing_accounts_insecure { use super::*; + use anchor_lang::solana_program::system_program; - pub fn close(ctx: Context) -> ProgramResult { - let account = ctx.accounts.account.to_account_info(); + pub fn close(ctx: Context) -> ProgramResult { let dest_starting_lamports = ctx.accounts.destination.lamports(); + let account_to_close = tx.accounts.lottery_entry.to_account_info(); **ctx.accounts.destination.lamports.borrow_mut() = dest_starting_lamports - .checked_add(account.lamports()) + .checked_add(account_to_close.lamports()) .unwrap(); - **account.lamports.borrow_mut() = 0; + **account_to_close.lamports.borrow_mut() = 0; - let mut data = account.try_borrow_mut_data()?; - for byte in data.deref_mut().iter_mut() { - *byte = 0; - } - - let dst: &mut [u8] = &mut data; - let mut cursor = std::io::Cursor::new(dst); - cursor - .write_all(&anchor_lang::__private::CLOSED_ACCOUNT_DISCRIMINATOR) - .unwrap(); + account_to_close.assign(&system_program::ID); + account_to_close.realloc(0, false)?; Ok(()) } @@ -155,7 +135,7 @@ pub mod closing_accounts_insecure_still_still { #[derive(Accounts)] pub struct Close<'info> { - account: Account<'info, Data>, + account_to_close: Account<'info, Data>, destination: AccountInfo<'info>, } @@ -165,82 +145,16 @@ pub struct Data { } ``` -Note that the example above is using Anchor's `CLOSED_ACCOUNT_DISCRIMINATOR`. -This is simply an account discriminator where each byte is `255`. The -discriminator doesn't have any inherent meaning, but if you couple it with -account validation checks that return errors any time an account with this -discriminator is passed to an instruction, you'll stop your program from -unintentionally processing an instruction with a closed account. - -#### Manual Force Defund - -There is still one small issue. While the practice of zeroing out account data -and adding a "closed" account discriminator will stop your program from being -exploited, a user can still keep an account from being garbage collected by -refunding the account's lamports before the end of an instruction. This results -in one or potentially many accounts existing in a limbo state where they cannot -be used but also cannot be garbage collected. - -To handle this edge case, you may consider adding an instruction that will allow -_anyone_ to defund accounts tagged with the "closed" account discriminator. The -only account validation this instruction would perform is to ensure that the -account being defunded is marked as closed. It may look something like this: - -```rust -use anchor_lang::__private::CLOSED_ACCOUNT_DISCRIMINATOR; -use anchor_lang::prelude::*; -use std::io::{Cursor, Write}; -use std::ops::DerefMut; - -... - - pub fn force_defund(ctx: Context) -> ProgramResult { - let account = &ctx.accounts.account; - - let data = account.try_borrow_data()?; - assert!(data.len() > 8); - - let mut discriminator = [0u8; 8]; - discriminator.copy_from_slice(&data[0..8]); - if discriminator != CLOSED_ACCOUNT_DISCRIMINATOR { - return Err(ProgramError::InvalidAccountData); - } - - let dest_starting_lamports = ctx.accounts.destination.lamports(); - - **ctx.accounts.destination.lamports.borrow_mut() = dest_starting_lamports - .checked_add(account.lamports()) - .unwrap(); - **account.lamports.borrow_mut() = 0; - - Ok(()) - } - -... - -#[derive(Accounts)] -pub struct ForceDefund<'info> { - account: AccountInfo<'info>, - destination: AccountInfo<'info>, -} -``` - -Since anyone can call this instruction, this can act as a deterrent to attempted -revival attacks since the attacker is paying for account rent exemption but -anyone else can claim the lamports in a refunded account for themselves. - -While not necessary, this can help eliminate the waste of space and lamports -associated with these "limbo" accounts. - ### Use the Anchor `close` constraint Fortunately, Anchor makes all of this much simpler with the `#[account(close = )]` constraint. This constraint handles everything required to securely close an account: -1. Transfers the account's lamports to the given `` +1. Transfers the account’s lamports to the given `` 2. Zeroes out the account data -3. Sets the account discriminator to the `CLOSED_ACCOUNT_DISCRIMINATOR` variant +3. Assigning the owner of the account to the System Program and rellocating the + size of the account with 0 bytes. All you have to do is add it in the account validation struct to the account you want closed: @@ -258,9 +172,6 @@ pub struct CloseAccount { } ``` -The `force_defund` instruction is an optional addition that you'll have to -implement on your own if you'd like to utilize it. - ## Lab To clarify how an attacker might take advantage of a revival attack, let's work @@ -270,7 +181,7 @@ participation in the lottery. ### 1. Setup Start by getting the code on the `starter` branch from the -[following repo](https://github.com/Unboxed-Software/solana-closing-accounts/tree/starter). +[following repo](https://github.com/solana-developers/closing-accounts/tree/starter). The code has two instructions on the program and two tests in the `tests` directory. @@ -316,22 +227,27 @@ alive even after claiming rewards and then claim rewards again. That test looks like this: ```typescript -it("attacker can close + refund lottery acct + claim multiple rewards", async () => { +it("attacker can close + refund lottery acct + claim multiple rewards successfully", async () => { + const [attackerLotteryEntry, bump] = PublicKey.findProgramAddressSync( + [Buffer.from("test-seed"), authority.publicKey.toBuffer()], + program.programId, + ); // claim multiple times for (let i = 0; i < 2; i++) { + let tokenAcct = await getAccount(provider.connection, attackerAta); + const tx = new Transaction(); + // instruction claims rewards, program will try to close account tx.add( await program.methods .redeemWinningsInsecure() .accounts({ - lotteryEntry: attackerLotteryEntry, - user: attacker.publicKey, userAta: attackerAta, rewardMint: rewardMint, - mintAuth: mintAuth, - tokenProgram: TOKEN_PROGRAM_ID, + user: authority.publicKey, }) + .signers([authority]) .instruction(), ); @@ -343,21 +259,22 @@ it("attacker can close + refund lottery acct + claim multiple rewards", async ( ); tx.add( SystemProgram.transfer({ - fromPubkey: attacker.publicKey, + fromPubkey: authority.publicKey, toPubkey: attackerLotteryEntry, lamports: rentExemptLamports, }), ); // send tx - await sendAndConfirmTransaction(provider.connection, tx, [attacker]); + await sendAndConfirmTransaction(provider.connection, tx, [authority]); await new Promise(x => setTimeout(x, 5000)); } - const ata = await getAccount(provider.connection, attackerAta); + const tokenAcct = await getAccount(provider.connection, attackerAta); + const lotteryEntry = await program.account.lotteryAccount.fetch(attackerLotteryEntry); - expect(Number(ata.amount)).to.equal( + expect(Number(tokenAcct.amount)).to.equal( lotteryEntry.timestamp.toNumber() * 10 * 2, ); }); @@ -390,7 +307,7 @@ pub struct RedeemWinningsSecure<'info> { // program expects this account to be initialized #[account( mut, - seeds = [user.key().as_ref()], + seeds = [DATA_PDA_SEED.as_bytes(),user.key.as_ref()], bump = lottery_entry.bump, has_one = user, close = user @@ -421,10 +338,9 @@ pub struct RedeemWinningsSecure<'info> { It should be the exact same as the original `RedeemWinnings` account validation struct, except there is an additional `close = user` constraint on the `lottery_entry` account. This will tell Anchor to close the account by zeroing -out the data, transferring its lamports to the `user` account, and setting the -account discriminator to the `CLOSED_ACCOUNT_DISCRIMINATOR`. This last step is -what will prevent the account from being used again if the program has attempted -to close it already. +out the data, assigning the owner to the System Program, transferring its +lamports to the `user` account. This last step is what will prevent the account +from being used again if the program has attempted to close it already. Then, we can create a `mint_ctx` method on the new `RedeemWinningsSecure` struct to help with the minting CPI to the token program. @@ -533,16 +449,19 @@ like this: ```bash closing-accounts - ✔ Enter lottery (451ms) - ✔ attacker can close + refund lottery acct + claim multiple rewards (18760ms) -AnchorError caused by account: lottery_entry. Error Code: AccountDiscriminatorMismatch. Error Number: 3002. Error Message: 8 byte discriminator did not match what was expected. - ✔ attacker cannot claim multiple rewards with secure claim (414ms) + ✔ Enter lottery should be successful (451ms) + ✔ attacker can close + refund lottery acct + claim multiple rewards successfully (18760ms) +AnchorError caused by account: lottery_entry. Error Code: AccountOwnedByWrongProgram. Error Number: 3007. Error Message: The given account is owned by a different program than expected. +Program log: Left: +Program log: 11111111111111111111111111111111 +Program log: Right: +Program log: FqETzdh6PsE7aNjrdapuoyFeYGdjPKN8AgG2ZUghje8A + ✔ attacker cannot claim multiple rewards with secure claim successfully (414ms) ``` Note, this does not prevent the malicious user from refunding their account altogether - it just protects our program from accidentally re-using the account -when it should be closed. We haven't implemented a `force_defund` instruction so -far, but we could. If you're feeling up for it, give it a try yourself! +when it should be closed. The simplest and most secure way to close accounts is using Anchor's `close` constraint. If you ever need more custom behavior and can't use this constraint, @@ -550,7 +469,7 @@ make sure to replicate its functionality to ensure your program is secure. 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/Unboxed-Software/solana-closing-accounts/tree/solution). +[the same repository](https://github.com/solana-developers/closing-accounts/tree/solution). ## Challenge diff --git a/content/courses/program-security/duplicate-mutable-accounts.md b/content/courses/program-security/duplicate-mutable-accounts.md index b52f1e29e..321dc72b8 100644 --- a/content/courses/program-security/duplicate-mutable-accounts.md +++ b/content/courses/program-security/duplicate-mutable-accounts.md @@ -216,6 +216,8 @@ use constants::DISCRIMINATOR_SIZE; declare_id!("Lo5sj2wWy4BHbe8kCSUvgdhzFbv9c6CEERfgAXusBj9"); +const DISCRIMINATOR_SIZE: usize = 8; + #[program] pub mod duplicate_mutable_accounts { use super::*; @@ -259,13 +261,15 @@ pub struct RockPaperScissorsInsecure<'info> { } #[account] -#[derive(Default, InitSpace)] +#[derive(InitSpace)] pub struct PlayerState { - pub player: Pubkey, - pub choice: Option, + player: Pubkey, + choice: Option, } -#[derive(AnchorSerialize, AnchorDeserialize, Clone, Copy, PartialEq, Eq, InitSpace)] + +#[derive(Clone, Copy, AnchorDeserialize, AnchorSerialize)] +#[derive(InitSpace)] pub enum RockPaperScissors { Rock, Paper, @@ -283,8 +287,8 @@ passing in the `playerOne.publicKey` for as both `playerOne` and `playerTwo`. ```typescript describe("duplicate-mutable-accounts", () => { - ... - it("Invokes insecure instruction", async () => { + ... + it("Invoke insecure instruction with the same player should be successful", async () => { await program.methods .rockPaperScissorsShootInsecure({ rock: {} }, { scissors: {} }) .accounts({ @@ -308,9 +312,9 @@ incorrectly as `scissors`. ```bash duplicate-mutable-accounts - ✔ Initialized Player One (461ms) - ✔ Initialized Player Two (404ms) - ✔ Invoke insecure instruction (406ms) + ✔ Initialized Player One should be successful (461ms) + ✔ Initialized Player Two should be successful (404ms) + ✔ Invoke insecure instruction with the same player should be successful (406ms) ``` Not only does allowing duplicate accounts do not make a whole lot of sense for @@ -364,36 +368,36 @@ which we expect to fail. ```typescript describe("duplicate-mutable-accounts", () => { - ... - it("Invokes secure instruction", async () => { - await program.methods + ... + it("Invoke secure instruction with different players should be successful", async () => { + await program.methods + .rockPaperScissorsShootSecure({ rock: {} }, { scissors: {} }) + .accounts({ + playerOne: playerOne.publicKey, + playerTwo: playerTwo.publicKey, + }) + .rpc(); + + const p1 = await program.account.playerState.fetch(playerOne.publicKey); + const p2 = await program.account.playerState.fetch(playerTwo.publicKey); + assert.equal(JSON.stringify(p1.choice), JSON.stringify({ rock: {} })); + assert.equal(JSON.stringify(p2.choice), JSON.stringify({ scissors: {} })); + }); + + it("Invoke secure instruction with the same player should throw an expection", async () => { + try { + await program.methods .rockPaperScissorsShootSecure({ rock: {} }, { scissors: {} }) .accounts({ - playerOne: playerOne.publicKey, - playerTwo: playerTwo.publicKey, + playerOne: playerOne.publicKey, + playerTwo: playerOne.publicKey, }) - .rpc() - - const p1 = await program.account.playerState.fetch(playerOne.publicKey) - const p2 = await program.account.playerState.fetch(playerTwo.publicKey) - assert.equal(JSON.stringify(p1.choice), JSON.stringify({ rock: {} })) - assert.equal(JSON.stringify(p2.choice), JSON.stringify({ scissors: {} })) - }) - - it("Invoke secure instruction - expect error", async () => { - try { - await program.methods - .rockPaperScissorsShootSecure({ rock: {} }, { scissors: {} }) - .accounts({ - playerOne: playerOne.publicKey, - playerTwo: playerOne.publicKey, - }) - .rpc() - } catch (err) { - expect(err) - console.log(err) - } - }) + .rpc(); + } catch (err) { + expect(err); + console.log(err); + } + }); }) ``` diff --git a/content/courses/program-security/owner-checks.md b/content/courses/program-security/owner-checks.md index 99466ac52..a66df0e63 100644 --- a/content/courses/program-security/owner-checks.md +++ b/content/courses/program-security/owner-checks.md @@ -3,11 +3,11 @@ title: Owner Checks objectives: - Explain the security risks associated with not performing appropriate owner checks - - Use Anchor's `Account<'info, T>` wrapper and an account type to automate + - Implement owner checks using long-form Rust + - Use Anchor’s `Account<'info, T>` wrapper and an account type to automate owner checks - - Use Anchor's `#[account(owner = )]` constraint to explicitly define an + - Use Anchor’s `#[account(owner = )]` constraint to explicitly define an external program that should own an account - - Implement owner checks using native Rust description: "Understand the use of account owner checks when processing incoming instructions." @@ -15,17 +15,11 @@ description: ## Summary -- **Owner checks** ensure that accounts are owned by the expected program. - Without owner checks, accounts owned by other programs can be used in an - instruction handler. -- Anchor program account types implement the `Owner` trait, allowing - `Account<'info, T>` to automatically verify program ownership. -- You can also use Anchor's - [`#[account(owner = )]`](https://www.anchor-lang.com/docs/account-constraints) - constraint to define an account's owner when it's external to the current - program. -- To implement an owner check in native Rust, verify that the account's owner - matches the expected program ID. +- Use **Owner Checks** to verify that accounts are owned by the expected + program. Without appropriate owner checks, accounts owned by unexpected + programs could be used in an instruction. +- To implement an owner check in Rust, simply check that an account’s owner + matches an expected program ID ```rust if ctx.accounts.account.owner != ctx.program_id { @@ -33,15 +27,20 @@ if ctx.accounts.account.owner != ctx.program_id { } ``` +- Anchor program account types implement the `Owner` trait which allows the + `Account<'info, T>` wrapper to automatically verify program ownership +- Anchor gives you the option to explicitly define the owner of an account if it + should be anything other than the currently executing program + ## Lesson -Owner checks are used to verify that an account passed into an instruction -handler is owned by the expected program, preventing exploitation by accounts -from different programs. +Owner checks are used to verify that an account passed into an instruction is +owned by an expected program. This prevents accounts owned by an unexpected +program from being used in an instruction. -The `AccountInfo` struct contains several fields, including the `owner`, which -represents the **program** that owns the account. Owner checks ensure that this -`owner` field in the `AccountInfo` matches the expected program ID. +As a refresher, the `AccountInfo` struct contains the following fields. An owner +check refers to checking that the `owner` field in the `AccountInfo` matches an +expected program ID. ```rust /// Account information @@ -66,12 +65,27 @@ pub struct AccountInfo<'a> { } ``` -### Missing owner check +#### Missing owner check + +The example below shows an `admin_instruction` intended to be accessible only by +an `admin` account stored on an `admin_config` account. + +Although the instruction checks the `admin` account signed the transaction and +matches the `admin` field stored on the `admin_config` account, there is no +owner check to verify the `admin_config` account passed into the instruction is +owned by the executing program. -In the following example, an `admin_instruction` is intended to be restricted to -an `admin` account stored in the `admin_config` account. However, it fails to -check whether the program owns the `admin_config` account. Without this check, -an attacker can spoof the account. +Since the `admin_config` is unchecked as indicated by the `AccountInfo` type, a +fake `admin_config` account owned by a different program could be used in the +`admin_instruction`. This means that an attacker could create a program with an +`admin_config` whose data structure matches the `admin_config` of your program, +set their public key as the `admin` and pass their `admin_config` account into +your program. This would let them spoof your program into thinking that they are +the authorized admin for your program. + +This simplified example only prints the `admin` to the program logs. However, +you can imagine how a missing owner check could allow fake accounts to exploit +an instruction. ```rust use anchor_lang::prelude::*; @@ -81,7 +95,7 @@ declare_id!("Cft4eTTrt4sJU4Ar35rUQHx6PSXfJju3dixmvApzhWws"); #[program] pub mod owner_check { use super::*; - ... + ... pub fn admin_instruction(ctx: Context) -> Result<()> { let account_data = ctx.accounts.admin_config.try_borrow_data()?; @@ -98,8 +112,7 @@ pub mod owner_check { #[derive(Accounts)] pub struct Unchecked<'info> { - /// CHECK: This account will not be checked by Anchor - admin_config: UncheckedAccount<'info>, + admin_config: AccountInfo<'info>, admin: Signer<'info>, } @@ -109,10 +122,11 @@ pub struct AdminConfig { } ``` -### Add owner check +#### Add owner check -To resolve this issue in native Rust, compare the `owner` field with the program -ID: +In vanilla Rust, you could solve this problem by comparing the `owner` field on +the account to the program ID. If they do not match, you would return an +`IncorrectProgramId` error. ```rust if ctx.accounts.admin_config.owner != ctx.program_id { @@ -120,8 +134,9 @@ if ctx.accounts.admin_config.owner != ctx.program_id { } ``` -Adding an `owner` check ensures that accounts from other programs cannot be -passed into the instruction handler. +Adding an owner check prevents accounts owned by an unexpected program to be +passed in as the `admin_config` account. If a fake `admin_config` account was +used in the `admin_instruction`, then the transaction would fail. ```rust use anchor_lang::prelude::*; @@ -151,8 +166,7 @@ pub mod owner_check { #[derive(Accounts)] pub struct Unchecked<'info> { - /// CHECK: This account will not be checked by Anchor - admin_config: UncheckedAccount<'info>, + admin_config: AccountInfo<'info>, admin: Signer<'info>, } @@ -162,14 +176,26 @@ pub struct AdminConfig { } ``` -### Use Anchor's `Account<'info, T>` +#### Use Anchor’s `Account<'info, T>` -Anchor simplifies owner checks with the `Account` type, which wraps -`AccountInfo` and automatically verifies ownership. +Anchor can make this simpler with the `Account` type. -In the following example, `Account<'info, AdminConfig>` validates the -`admin_config` account, and the `has_one` constraint checks that the admin -account matches the `admin` field in `admin_config`. +`Account<'info, T>` is a wrapper around `AccountInfo` that verifies program +ownership and deserializes underlying data into the specified account type `T`. +This in turn allows you to use `Account<'info, T>` to easily validate ownership. + +For context, the `#[account]` attribute implements various traits for a data +structure representing an account. One of these is the `Owner` trait which +defines an address expected to own an account. The owner is set as the program +ID specified in the `declare_id!` macro. + +In the example below, `Account<'info, AdminConfig>` is used to validate the +`admin_config`. This will automatically perform the owner check and deserialize +the account data. Additionally, the `has_one` constraint is used to check that +the `admin` account matches the `admin` field stored on the `admin_config` +account. + +This way, you don’t need to clutter your instruction logic with owner checks. ```rust use anchor_lang::prelude::*; @@ -179,7 +205,7 @@ declare_id!("Cft4eTTrt4sJU4Ar35rUQHx6PSXfJju3dixmvApzhWws"); #[program] pub mod owner_check { use super::*; - ... + ... pub fn admin_instruction(ctx: Context) -> Result<()> { msg!("Admin: {}", ctx.accounts.admin_config.admin.to_string()); Ok(()) @@ -201,19 +227,19 @@ pub struct AdminConfig { } ``` -### Use Anchor's `#[account(owner = )]` constraint +#### Use Anchor’s `#[account(owner = )]` constraint -In addition to the `Account` type, you can use the Anchor's -[`owner` constraint](https://www.anchor-lang.com/docs/account-constraints) to -specify the program that should own an account when it differs from the -executing program. This is particularly useful when an instruction handler -expects an account to be a PDA created by another program. By using the `seeds` -and `bump` constraints along with the `owner`, you can properly derive and -verify the account's address. +In addition to the `Account` type, you can use an `owner` constraint. The +`owner` constraint allows you to define the program that should own an account +if it’s different from the currently executing one. This comes in handy if, for +example, you are writing an instruction that expects an account to be a PDA +derived from a different program. You can use the `seeds` and `bump` constraints +and define the `owner` to properly derive and verify the address of the account +passed in. -To apply the `owner` constraint, you need access to the public key of the -program expected to own the account. This can be provided either as an -additional account or by hard-coding the public key within your program. +To use the `owner` constraint, you’ll have to have access to the public key of +the program you expect to own an account. You can either pass the program in as +an additional account or hard-code the public key somewhere in your program. ```rust use anchor_lang::prelude::*; @@ -254,43 +280,43 @@ pub struct AdminConfig { ## Lab -In this lab, we'll demonstrate how the absence of an owner check can allow a -malicious actor to drain tokens from a simplified token vault. This is similar -to the lab from the -[Signer Authorization lesson](/content/courses/program-security/signer-auth.md). +In this lab we’ll use two programs to demonstrate how a missing owner check +could allow a fake account to drain the tokens from a simplified token “vault” +account (note that this is very similar to the lab from the Signer Authorization +lesson). -We'll use two programs to illustrate this: +To help illustrate this, one program will be missing an account owner check on +the vault account it withdraws tokens to. -1. One program lacks an owner check on the vault account it withdraws tokens - from. -2. The second program is a clone created by a malicious user to mimic the first - program's vault account. +The second program will be a direct clone of the first program created by a +malicious user to create an account identical to the first program’s vault +account. -Without the owner check, the malicious user can pass in their vault account -owned by a fake program, and the original program will still execute the -withdrawal. +Without the owner check, this malicious user will be able to pass in the vault +account owned by their “faked” program and the original program will still +execute. -### 1. Starter +#### 1. Starter -Begin by downloading the starter code from the -[`starter` branch of this repository](https://github.com/solana-developers/owner-checks/tree/starter). -The starter code includes two programs: `clone` and `owner_check`, and the setup -for the test file. +To get started, download the starter code from the `starter` branch of +[this repository](https://github.com/solana-developers/owner-checks/tree/starter). +The starter code includes two programs `clone` and `owner_check` and the +boilerplate setup for the test file. -The `owner_check` program includes two instruction handlers: +The `owner_check` program includes two instructions: -- `initialize_vault`: Initializes a simplified vault account storing the - addresses of a token account and an authority account. -- `insecure_withdraw`: Withdraws tokens from the token account but lacks an - owner check for the vault account. +- `initialize_vault` initializes a simplified vault account that stores the + addresses of a token account and an authority account +- `insecure_withdraw` withdraws tokens from the token account, but is missing an + owner check for the vault account ```rust use anchor_lang::prelude::*; use anchor_spl::token::{self, Mint, Token, TokenAccount}; -declare_id!("3uF3yaymq1YBmDDHpRPwifiaBf4eK8M2jLgaMcCTg9n9"); +declare_id!("2JzQxdnKh6RXwACK6desuPrsbk6Yd3ky4UHAPXQFFC9w"); -pub const DISCRIMINATOR_SIZE: usize = 8; +const DISCRIMINATOR_SIZE: usize = 8; #[program] pub mod owner_check { @@ -315,7 +341,7 @@ pub mod owner_check { let seeds = &[ b"token".as_ref(), - &[ctx.bumps.token_account], + &[ctx.bump.stoken_account], ]; let signer = [&seeds[..]]; @@ -361,7 +387,7 @@ pub struct InitializeVault<'info> { #[derive(Accounts)] pub struct InsecureWithdraw<'info> { - /// CHECK: This account will not be checked by anchor + /// CHECK: pub vault: UncheckedAccount<'info>, #[account( mut, @@ -376,26 +402,26 @@ pub struct InsecureWithdraw<'info> { } #[account] -#[derive(Default, InitSpace)] +#[derive(InitSpace)] pub struct Vault { token_account: Pubkey, authority: Pubkey, } ``` -The `clone` program includes a single instruction handler: +The `clone` program includes a single instruction: -- `initialize_vault`: Initializes a fake vault account that mimics the vault - account of the `owner_check` program, allowing the malicious user to set their - own authority. +- `initialize_vault` initializes a “vault” account that mimics the vault account + of the `owner_check` program. It stores the address of the real vault’s token + account, but allows the malicious user to put their own authority account. ```rust use anchor_lang::prelude::*; use anchor_spl::token::TokenAccount; -declare_id!("2Gn5MFGMvRjd548z6vhreh84UiL7L5TFzV5kKGmk4Fga"); +declare_id!("7967qGRBusM49RUdBiFU8s9YY3fSwA9rxCSELXgk8Tk1"); -pub const DISCRIMINATOR_SIZE: usize = 8; +const DISCRIMINATOR_SIZE: usize = 8; #[program] pub mod clone { @@ -423,97 +449,103 @@ pub struct InitializeVault<'info> { } #[account] -#[derive(Default, InitSpace)] +#[derive(InitSpace)] pub struct Vault { token_account: Pubkey, authority: Pubkey, } ``` -### 2. Test insecure_withdraw Instruction Handler +#### 2. Test `insecure_withdraw` instruction -The test file contains tests that initialize a vault in both programs. We'll add -a test to invoke the `insecure_withdraw` instruction handler, showing how the -lack of an owner check allows token withdrawal from the original program's -vault. +The test file includes a test to invoke the `initialize_vault` instruction on +the `owner_check` program using the provider wallet as the `authority` and then +mints 100 tokens to the token account. -```typescript -describe("Owner Check", () => { - ... - it("performs insecure withdraw", async () => { - try { - const transaction = await program.methods - .insecureWithdraw() - .accounts({ - vault: vaultCloneAccount.publicKey, - tokenAccount: tokenPDA, - withdrawDestination: unauthorizedWithdrawDestination, - authority: unauthorizedWallet.publicKey, - }) - .transaction(); +The test file also includes a test to invoke the `initialize_vault` instruction +on the `clone` program to initialize a fake `vault` account storing the same +`tokenPDA` account, but a different `authority`. Note that no new tokens are +minted here. - await anchor.web3.sendAndConfirmTransaction(connection, transaction, [ - unauthorizedWallet, - ]); +Let’s add a test to invoke the `insecure_withdraw` instruction. This test should +pass in the cloned vault and the fake authority. Since there is no owner check +to verify the `vaultClone` account is owned by the `owner_check` program, the +instruction’s data validation check will pass and show `walletFake` as a valid +authority. The tokens from the `tokenPDA` account will then be withdrawn to the +`withdrawDestinationFake` account. - const tokenAccountInfo = await getAccount(connection, tokenPDA); - expect(Number(tokenAccountInfo.amount)).to.equal(0); - } catch (error) { - console.error("Insecure withdraw failed:", error); - throw error; - } +```typescript +describe("owner-check", () => { + ... + it("Insecure withdraw should be successful", async () => { + const tx = await program.methods + .insecureWithdraw() + .accounts({ + vault: vaultClone.publicKey, + withdrawDestination: withdrawDestinationFake, + authority: walletFake.publicKey, + }) + .signers([walletFake]) + .rpc(); + + const balance = await connection.getTokenAccountBalance(tokenPDA); + expect(balance.value.uiAmount).to.eq(0); }); -}) + +}); ``` -Run an `anchor test` to verify that the `insecure_withdraw` is complete -successfully. +Run `anchor test` to see that the `insecure_withdraw` completes successfully. ```bash owner-check - ✔ initializes vault (866ms) - ✔ initializes fake vault (443ms) - ✔ performs insecure withdraw (444ms) + ✔ Initialize Vault should be successful (808ms) + ✔ Initialize Fake Vault should be successful (404ms) + ✔ Insecure withdraw should be successful (409ms) ``` - - -The `vaultCloneAccount` deserializes successfully due to both programs using the -same discriminator, derived from the identical `Vault` struct name. +Note that `vaultClone` deserializes successfully even though Anchor +automatically initializes new accounts with a unique 8 byte discriminator and +checks the discriminator when deserializing an account. This is because the +discriminator is a hash of the account type name. ```rust #[account] -#[derive(Default, InitSpace)] +#[derive(InitSpace)] pub struct Vault { token_account: Pubkey, authority: Pubkey, } ``` -### 3. Add secure_withdraw Instruction Handler +Since both programs initialize identical accounts and both structs are named +`Vault`, the accounts have the same discriminator even though they are owned by +different programs. -We'll now close the security loophole by adding a `secure_withdraw` instruction -handler with an `Account<'info, Vault>` type to ensure an owner check is -performed. +#### 3. Add `secure_withdraw` instruction -In the `lib.rs` file of the `owner_check` program, add a `secure_withdraw` -instruction handler and a `SecureWithdraw` accounts struct. The `has_one` -constraint will be used to ensure that the `token_account` and `authority` -passed into the instruction handler match the values stored in the `vault` -account. +Let’s close up this security loophole. + +In the `lib.rs` file of the `owner_check` program add a `secure_withdraw` +instruction and a `SecureWithdraw` accounts struct. + +In the `SecureWithdraw` struct, let’s use `Account<'info, Vault>` to ensure that +an owner check is performed on the `vault` account. We’ll also use the `has_one` +constraint to check that the `token_account` and `authority` passed into the +instruction match the values stored on the `vault` account. ```rust #[program] pub mod owner_check { use super::*; - ... + ... - pub fn secure_withdraw(ctx: Context) -> Result<()> { + pub fn secure_withdraw(ctx: Context) -> Result<()> { let amount = ctx.accounts.token_account.amount; let seeds = &[ b"token".as_ref(), - &[*ctx.bumps.get("token_account").unwrap()], + &[ctx.bumps.token_account], ]; let signer = [&seeds[..]]; @@ -553,116 +585,101 @@ pub struct SecureWithdraw<'info> { } ``` -### 4. Test secure_withdraw Instruction Handler +#### 4. Test `secure_withdraw` instruction -To test the `secure_withdraw` instruction handler, we'll invoke it twice. First, -we'll use the `vaultCloneAccount` account, expecting it to fail. Then, we'll -invoke the instruction handler with the correct `vaultAccount` account to verify -the instruction handler works as intended. +To test the `secure_withdraw` instruction, we’ll invoke the instruction twice. +First, we’ll invoke the instruction using the `vaultClone` account, which we +expect to fail. Then, we’ll invoke the instruction using the correct `vault` +account to check that the instruction works as intended. ```typescript -describe("Owner Check", () => { - ... - it("fails secure withdraw with incorrect authority", async () => { +describe("owner-check", () => { + ... + it("Secure withdraw should throw an error", async () => { try { - const transaction = await program.methods - .secureWithdraw() - .accounts({ - vault: vaultCloneAccount.publicKey, - tokenAccount: tokenPDA, - withdrawDestination: unauthorizedWithdrawDestination, - authority: unauthorizedWallet.publicKey, - }) - .transaction(); - - await anchor.web3.sendAndConfirmTransaction(connection, transaction, [ - unauthorizedWallet, - ]); - throw new Error("Expected transaction to fail, but it succeeded"); - } catch (error) { - expect(error).to.be.an("error"); - console.log("Error message:", error.message); - } - }); - - it("performs secure withdraw successfully", async () => { - try { - await mintTo( - connection, - walletAuthority.payer, - tokenMint, - tokenPDA, - walletAuthority.payer, - INITIAL_TOKEN_AMOUNT - ); - await program.methods .secureWithdraw() .accounts({ - vault: vaultAccount.publicKey, - tokenAccount: tokenPDA, - withdrawDestination: authorizedWithdrawDestination, - authority: walletAuthority.publicKey, + vault: vaultClone.publicKey, + withdrawDestination: withdrawDestinationFake, }) .rpc(); - - const tokenAccountInfo = await getAccount(connection, tokenPDA); - expect(Number(tokenAccountInfo.amount)).to.equal(0); - } catch (error) { - console.error("Secure withdraw failed:", error); - throw error; + } catch (err) { + expect(err); + console.log(err); } }); -}) + + it("Secure withdraw should be successful", async () => { + await spl.mintTo( + connection, + wallet.payer, + mint, + tokenPDA, + wallet.payer, + 100 + ); + await program.methods + .secureWithdraw() + .accounts({ + vault: vault.publicKey, + withdrawDestination: withdrawDestination, + }) + .rpc(); + const balance = await connection.getTokenAccountBalance(tokenPDA); + expect(balance.value.uiAmount).to.eq(0); + }); +}); ``` -Running `anchor test` will show that the transaction using the -`vaultCloneAccount` account fails, while the transaction using the -`vaultAccount` account withdraws successfully. +Run `anchor test` to see that the transaction using the `vaultClone` account +will now return an Anchor Error while the transaction using the `vault` account +completes successfully. ```bash -"Program 3uF3yaymq1YBmDDHpRPwifiaBf4eK8M2jLgaMcCTg9n9 invoke [1]", -"Program log: Instruction: SecureWithdraw", -"Program log: AnchorError caused by account: vault. Error Code: AccountOwnedByWrongProgram. Error Number: 3007. Error Message: The given account is owned by a different program than expected.", -"Program log: Left:", -"Program log: 2Gn5MFGMvRjd548z6vhreh84UiL7L5TFzV5kKGmk4Fga", -"Program log: Right:", -"Program log: 3uF3yaymq1YBmDDHpRPwifiaBf4eK8M2jLgaMcCTg9n9", -"Program 3uF3yaymq1YBmDDHpRPwifiaBf4eK8M2jLgaMcCTg9n9 consumed 4449 of 200000 compute units", -"Program 3uF3yaymq1YBmDDHpRPwifiaBf4eK8M2jLgaMcCTg9n9 failed: custom program error: 0xbbf" +'Program HQYNznB3XTqxzuEqqKMAD9XkYE5BGrnv8xmkoDNcqHYB invoke [1]', +'Program log: Instruction: SecureWithdraw', +'Program log: AnchorError caused by account: vault. Error Code: AccountOwnedByWrongProgram. Error Number: 3007. Error Message: The given account is owned by a different program than expected.', +'Program log: Left:', +'Program log: DUN7nniuatsMC7ReCh5eJRQExnutppN1tAfjfXFmGDq3', +'Program log: Right:', +'Program log: HQYNznB3XTqxzuEqqKMAD9XkYE5BGrnv8xmkoDNcqHYB', +'Program HQYNznB3XTqxzuEqqKMAD9XkYE5BGrnv8xmkoDNcqHYB consumed 5554 of 200000 compute units', +'Program HQYNznB3XTqxzuEqqKMAD9XkYE5BGrnv8xmkoDNcqHYB failed: custom program error: 0xbbf' ``` -Here we see how using Anchor's `Account<'info, T>` type simplifies the account -validation process by automating ownership checks. Additionally, Anchor errors -provide specific details, such as which account caused the error. For example, -the log indicates `AnchorError caused by account: vault`, which aids in -debugging. +Here we see how using Anchor’s `Account<'info, T>` type can simplify the account +validation process to automate the ownership check. Additionally, note that +Anchor Errors can specify the account that causes the error (e.g. the third line +of the logs above say `AnchorError caused by account: vault`). This can be very +helpful when debugging. ```bash -✔ fails secure withdraw with incorrect authority -✔ performs secure withdraw successfully (847ms) +✔ Secure withdraw should throw an error (78ms) +✔ Secure withdraw should be successful (10063ms) ``` -Ensuring account ownership checks is critical to avoid security vulnerabilities. -This example demonstrates how simple it is to implement proper validation, but -it's vital to always verify which accounts are owned by specific programs. +That’s all you need to ensure you check the owner on an account! Like some other +exploits, it’s fairly simple to avoid but very important. Be sure to always +think through which accounts should be owned by which programs and ensure that +you add appropriate validation. -If you'd like to review the final solution code, it's available on the -[`solution` branch of the repository](https://github.com/solana-developers/owner-checks/tree/solution). +If you want to take a look at the final solution code you can find it on the +`solution` branch of +[the repository](https://github.com/solana-developers/owner-checks/tree/solution). ## Challenge -As with other lessons in this unit, practice preventing security exploits by -auditing your own or other programs. +Just as with other lessons in this unit, your opportunity to practice avoiding +this security exploit lies in auditing your own or other programs. -Take time to review at least one program to confirm that ownership checks are -properly enforced on all accounts passed into each instruction handler. +Take some time to review at least one program and ensure that proper owner +checks are performed on the accounts passed into each instruction. -If you find a bug or exploit in another program, notify the developer. If you -find one in your own program, patch it immediately. +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=e3069010-3038-4984-b9d3-2dc6585147b1)! diff --git a/content/courses/program-security/pda-sharing.md b/content/courses/program-security/pda-sharing.md index 7b70de4a7..2156af7c1 100644 --- a/content/courses/program-security/pda-sharing.md +++ b/content/courses/program-security/pda-sharing.md @@ -3,7 +3,7 @@ title: PDA Sharing objectives: - Explain the security risks associated with PDA sharing - Derive PDAs that have discrete authority domains - - Use Anchor's `seeds` and `bump` constraints to validate PDA accounts + - Use Anchor’s `seeds` and `bump` constraints to validate PDA accounts description: "Understand the potential problems of reusing PDAs by using user and domain specific PDAs." @@ -15,7 +15,7 @@ description: possibility of users accessing data and funds that don't belong to them - Prevent the same PDA from being used for multiple accounts by using seeds that are user and/or domain-specific -- Use Anchor's `seeds` and `bump` constraints to validate that a PDA is derived +- Use Anchor’s `seeds` and `bump` constraints to validate that a PDA is derived using the expected seeds and bump ## Lesson @@ -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 +[the `pda-sharing` 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,64 +468,66 @@ 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); } }); ``` Lastly, since the `pool` account is a PDA derived using the -`withdraw_destination` token account, we can't create a fake `pool` account +`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)! diff --git a/content/courses/program-security/reinitialization-attacks.md b/content/courses/program-security/reinitialization-attacks.md index 715e6cfe8..3b7a62b26 100644 --- a/content/courses/program-security/reinitialization-attacks.md +++ b/content/courses/program-security/reinitialization-attacks.md @@ -2,10 +2,10 @@ title: Reinitialization Attacks objectives: - Explain security risks associated with a reinitialization vulnerability - - Using Anchor's `init` constraint to initialize accounts, which automatically + - Use long-form Rust check if an account has already been initialized + - Using Anchor’s `init` constraint to initialize accounts, which automatically sets an account discriminator that is checked to prevent the reinitialization of an account - - Use native Rust to check if an account has already been initialized description: "Understand the security risks of account reinitialized attacks being used to override data, and how to prevent them." @@ -13,37 +13,45 @@ description: ## Summary -- **Prevent Account Reinitialization:** Use an account discriminator or - initialization flag to prevent an account from being reinitialized and - overwriting existing data. -- **Anchor Approach:** Simplify this by using Anchor's `init` constraint to - create an account via a CPI to the system program, automatically setting its - discriminator. -- **Native Rust Approach:** In native Rust, set an is_initialized flag during - account initialization and check it before reinitializing: - +- Use an account discriminator or initialization flag to check whether an + account has already been initialized to prevent an account from being + reinitialized and overriding existing account data. +- To prevent account reinitialization in plain Rust, initialize accounts with an + `is_initialized` flag and check if it has already been set to true when + initializing an account ```rust if account.is_initialized { return Err(ProgramError::AccountAlreadyInitialized.into()); } ``` +- To simplify this, use Anchor’s `init` constraint to create an account via a + CPI to the system program and sets its discriminator ## Lesson -Initialization sets the data of a new account for the first time. It's essential -to check if an account has already been initialized to prevent overwriting -existing data. Note that creating and initializing an account are separate -actions. Creating an account involves invoking the `create_account` instruction -handler on the System Program, which allocates space, rent in lamports, and -assigns the program owner. Initialization sets the account data. These steps can -be combined into a single transaction. +Initialization refers to setting the data of a new account for the first time. +When initializing a new account, you should implement a way to check if the +account has already been initialized. Without an appropriate check, an existing +account could be reinitialized and have existing data overwritten. + +Note that initializing an account and creating an account are two separate +instructions. Creating an account requires invoking the `create_account` +instruction on the System Program which specifies the space required for the +account, the rent in lamports allocated to the account, and the program owner of +the account. Initialization is an instruction that sets the data of a newly +created account. Creating and initializing an account can be combined into a +single transaction. -### Missing Initialization Check +#### Missing Initialization Check -In the example below, there's no check on the `user` account. The `initialize` -instruction handler sets the `authority` field on the `User` account type and -serializes the data. Without checks, an attacker could reinitialize the account, -overwriting the existing `authority`. +In the example below, there are no checks on the `user` account. The +`initialize` instruction deserializes the data of the `user` account as a `User` +account type, sets the `authority` field, and serializes the updated account +data to the `user` account. + +Without checks on the `user` account, the same account could be passed into the +`initialize` instruction a second time by another party to overwrite the +existing `authority` stored on the account data. ```rust use anchor_lang::prelude::*; @@ -51,35 +59,37 @@ use anchor_lang::prelude::*; declare_id!("Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"); #[program] -pub mod initialization_insecure { +pub mod initialization_insecure { use super::*; pub fn initialize(ctx: Context) -> Result<()> { - ctx.accounts.user.authority = ctx.accounts.authority.key(); + let mut user = User::try_from_slice(&ctx.accounts.user.data.borrow()).unwrap(); + user.authority = ctx.accounts.authority.key(); + user.serialize(&mut *ctx.accounts.user.data.borrow_mut())?; Ok(()) } } #[derive(Accounts)] pub struct Initialize<'info> { + #[account(mut)] + user: AccountInfo<'info>, #[account(mut)] - pub user: Account<'info, User>, - #[account(mut)] - pub authority: Signer<'info>, - pub system_program: Program<'info, System>, + authority: Signer<'info>, } #[account] #[derive(InitSpace)] pub struct User { - pub authority: Pubkey, + authority: Pubkey, } ``` -### Add is_initialized Check +#### Add `is_initialized` check -To fix this, add an `is_initialized` field to the User account type and check it -before reinitializing: +One approach to fix this is to add an additional `is_initialized` field to the +`User` account type and use it as a flag to check if an account has already been +initialized. ```rust if user.is_initialized { @@ -87,13 +97,14 @@ if user.is_initialized { } ``` -This ensures the `user` account is only initialized once. If `is_initialized` is -true, the transaction fails, preventing an attacker from changing the account -authority. +By including a check within the `initialize` instruction, the `user` account +would only be initialized if the `is_initialized` field has not yet been set to +true. If the `is_initialized` field was already set, the transaction would fail, +thereby avoiding the scenario where an attacker could replace the account +authority with their own public key. ```rust use anchor_lang::prelude::*; -use anchor_lang::solana_program::program_error::ProgramError; declare_id!("Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"); @@ -102,137 +113,140 @@ pub mod initialization_secure { use super::*; pub fn initialize(ctx: Context) -> Result<()> { - let user = &mut ctx.accounts.user; - + let mut user = User::try_from_slice(&ctx.accounts.user.data.borrow()).unwrap(); if user.is_initialized { return Err(ProgramError::AccountAlreadyInitialized.into()); } - user.is_initialized = true; user.authority = ctx.accounts.authority.key(); + user.is_initialized = true; + user.serialize(&mut *ctx.accounts.user.data.borrow_mut())?; Ok(()) } } #[derive(Accounts)] pub struct Initialize<'info> { + #[account(mut)] + user: AccountInfo<'info>, #[account(mut)] - pub user: Account<'info, User>, - #[account(mut)] - pub authority: Signer<'info>, - pub system_program: Program<'info, System>, + authority: Signer<'info>, } -#[account] -#[derive(InitSpace)] + pub struct User { - pub is_initialized: bool, - pub authority: Pubkey, + is_initialized: bool, + authority: Pubkey, } ``` -### Use Anchor's init Constraint +#### Use Anchor’s `init` constraint + +Anchor provides an `init` constraint that can be used with the `#[account(...)]` +attribute to initialize an account. The `init` constraint creates the account +via a CPI to the system program and sets the account discriminator. + +The `init` constraint must be used in combination with the `payer` and `space` +constraints. The `payer` specifies the account paying for the initialization of +the new account. The `space` specifies the amount of space the new account +requires, which determines the amount of lamports that must be allocated to the +account. The first 8 bytes of data is set as a discriminator that Anchor +automatically adds to identify the account type. -[Anchor's `init` constraint](https://www.anchor-lang.com/docs/account-constraints), -used with the `#[account(...)]` attribute, initializes an account, sets the -account discriminator, and ensures that the instruction handler can only be -called once per account. The `init` constraint must be used with `payer` and -`space` constraints to specify the account paying for initialization and the -amount of space required. +Most importantly for this lesson, the `init` constraint ensures that this +instruction can only be called once per account, so you can set the initial +state of the account in the instruction logic and not have to worry about an +attacker trying to reinitialize the account. ```rust use anchor_lang::prelude::*; declare_id!("Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"); -const DISCRIMINATOR_SIZE: usize = 8; - #[program] pub mod initialization_recommended { use super::*; - pub fn initialize(ctx: Context) -> Result<()> { + pub fn initialize(_ctx: Context) -> Result<()> { msg!("GM"); - ctx.accounts.user.authority = ctx.accounts.authority.key(); Ok(()) } } #[derive(Accounts)] pub struct Initialize<'info> { - #[account( - init, - payer = authority, - space = DISCRIMINATOR_SIZE + User::INIT_SPACE - )] - pub user: Account<'info, User>, + #[account(init, payer = authority, space = DISCRIMINATOR_SIZE + User::INIT_SPACE)] + user: Account<'info, User>, #[account(mut)] - pub authority: Signer<'info>, - pub system_program: Program<'info, System>, + authority: Signer<'info>, + system_program: Program<'info, System>, } #[account] #[derive(InitSpace)] pub struct User { - pub authority: Pubkey, + authority: Pubkey, } ``` -#### Anchor's init_if_needed Constraint +#### Anchor’s `init_if_needed` constraint - +It’s worth noting that Anchor has an `init_if_needed` constraint. This +constraint should be used very cautiously. In fact, it is blocked behind a +feature flag so that you are forced to be intentional about using it. -[Anchor's `init_if_needed` constraint](https://www.anchor-lang.com/docs/account-constraints), -guarded by a feature flag, should be used with caution.It initializes an account -only if it hasn't been initialized yet. If the account is already initialized, -the instruction handler will still execute, so -it's \***\*\*\*\***extremely\***\*\*\*\*** important to include checks in your -instruction handler to prevent resetting the account to its initial state. - +The `init_if_needed` constraint does the same thing as the `init` constraint, +only if the account has already been initialized the instruction will still run. + +Given this, it’s \***\*\*\*\***extremely\***\*\*\*\*** important that when you +use this constraint you include checks to avoid resetting the account to its +initial state. -For example, if the `authority` field is set in the instruction handler, ensure -that your instruction handler includes checks to prevent an attacker from -reinitializing it after it's already been set. Typically, it's safer to have a -separate instruction handler for initializing account data. +For example, if the account stores an `authority` field that gets set in the +instruction using the `init_if_needed` constraint, you need checks that ensure +that no attacker could call the instruction after it has already been +initialized and have the `authority` field set again. + +In most cases, it’s safer to have a separate instruction for initializing +account data. ## Lab -In this lab, we'll create a simple Solana program with two instruction handlers: +For this lab we’ll create a simple program that does nothing but initialize +accounts. We’ll include two instructions: -- `insecure_initialization` - Initializes an account without checks, allowing - reinitialization. -- `recommended_initialization` - Initializes an account using Anchor's `init` - constraint, preventing reinitialization. +- `insecure_initialization` - initializes an account that can be reinitialized +- `recommended_initialization` - initialize an account using Anchor’s `init` + constraint -### 1. Starter +#### 1. Starter -To get started, download the starter code from the -[`starter` branch of this repository](https://github.com/solana-developers/reinitialization-attacks/tree/starter). -The starter code includes a program with one instruction handler and the -boilerplate setup for the test file. +To get started, download the starter code from the `starter` branch of +[this repository](https://github.com/solana-developers/reinitialization-attacks/tree/starter). +The starter code includes a program with one instruction and the boilerplate +setup for the test file. -The `insecure_initialization` instruction handler initializes a new `user` -account that stores the public key of an `authority`. The account is expected to -be allocated client-side and then passed into the program instruction. However, -there are no checks to verify if the `user` account's initial state has already -been set. This means the same account can be passed in a second time, allowing -the `authority` to be overwritten. +The `insecure_initialization` instruction initializes a new `user` account that +stores the public key of an `authority`. In this instruction, the account is +expected to be allocated client-side, then passed into the program instruction. +Once passed into the program, there are no checks to see if the `user` account's +initial state has already been set. This means the same account can be passed in +a second time to override the `authority` stored on an existing `user` account. ```rust use anchor_lang::prelude::*; -declare_id!("HLhxJzFYjtXCET4HxnSzv27SpXg16FWNDi2LvrNmSvzH"); +declare_id!("Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"); #[program] pub mod initialization { use super::*; pub fn insecure_initialization(ctx: Context) -> Result<()> { - let user = &mut ctx.accounts.user; - let mut user_data = User::try_from_slice(&user.data.borrow())?; - user_data.authority = ctx.accounts.authority.key(); - user_data.serialize(&mut *user.data.borrow_mut())?; + let mut user = User::try_from_slice(&ctx.accounts.user.data.borrow()).unwrap(); + user.authority = ctx.accounts.authority.key(); + user.serialize(&mut *ctx.accounts.user.data.borrow_mut())?; Ok(()) } } @@ -240,164 +254,139 @@ pub mod initialization { #[derive(Accounts)] pub struct Unchecked<'info> { #[account(mut)] - /// CHECK: This account will be initialized in the instruction - pub user: UncheckedAccount<'info>, - pub authority: Signer<'info>, + /// CHECK: + user: UncheckedAccount<'info>, + authority: Signer<'info>, } #[account] #[derive(InitSpace)] pub struct User { - pub authority: Pubkey, + authority: Pubkey, } ``` -### 2. Test insecure_initialization Instruction Handler +#### 2. Test `insecure_initialization` instruction The test file includes the setup to create an account by invoking the system -program and then invokes the `insecure_initialization` instruction handler twice -using the same account. +program and then invokes the `insecure_initialization` instruction twice using +the same account. -Since there are no checks in the `insecure_initialization` instruction handler -to verify that the account data has not already been initialized, this -instruction handler will execute successfully both times, even with a -_different_ authority account. +Since there are no checks the verify that the account data has not already been +initialized, the `insecure_initialization` instruction will complete +successfully both times, despite the second invocation providing a _different_ +authority account. ```typescript import * as anchor from "@coral-xyz/anchor"; import { Program } from "@coral-xyz/anchor"; -import { Initialization } from "../target/types/initialization"; -import { - Keypair, - LAMPORTS_PER_SOL, - SystemProgram, - Transaction, - SendTransactionError, -} from "@solana/web3.js"; import { expect } from "chai"; -import { airdropIfRequired } from "@solana-developers/helpers"; +import { Initialization } from "../target/types/initialization"; -describe("Initialization", () => { +describe("initialization", () => { const provider = anchor.AnchorProvider.env(); anchor.setProvider(provider); const program = anchor.workspace.Initialization as Program; - const walletAuthority = provider.wallet as anchor.Wallet; - const secondWallet = Keypair.generate(); - - const insecureUserAccount = Keypair.generate(); - const recommendedUserAccount = Keypair.generate(); - - const ACCOUNT_SPACE = 32; - const AIRDROP_AMOUNT = 1 * LAMPORTS_PER_SOL; - const MINIMUM_BALANCE_FOR_RENT_EXEMPTION = 1 * LAMPORTS_PER_SOL; + const wallet = anchor.workspace.Initialization.provider.wallet; + const walletTwo = anchor.web3.Keypair.generate(); + const userInsecure = anchor.web3.Keypair.generate(); + const userRecommended = anchor.web3.Keypair.generate(); before(async () => { - try { - const rentExemptionAmount = - await provider.connection.getMinimumBalanceForRentExemption( - ACCOUNT_SPACE, - ); - - const createAccountInstruction = SystemProgram.createAccount({ - fromPubkey: walletAuthority.publicKey, - newAccountPubkey: insecureUserAccount.publicKey, - space: ACCOUNT_SPACE, - lamports: rentExemptionAmount, + const tx = new anchor.web3.Transaction().add( + anchor.web3.SystemProgram.createAccount({ + fromPubkey: wallet.publicKey, + newAccountPubkey: userInsecure.publicKey, + space: 32, + lamports: + await provider.connection.getMinimumBalanceForRentExemption(32), programId: program.programId, - }); - - const transaction = new Transaction().add(createAccountInstruction); - - await anchor.web3.sendAndConfirmTransaction( - provider.connection, - transaction, - [walletAuthority.payer, insecureUserAccount], - ); - - await airdropIfRequired( - provider.connection, - secondWallet.publicKey, - AIRDROP_AMOUNT, - MINIMUM_BALANCE_FOR_RENT_EXEMPTION, - ); - } catch (error) { - console.error("Setup failed:", error); - throw error; - } + }), + ); + + await anchor.web3.sendAndConfirmTransaction(provider.connection, tx, [ + wallet.payer, + userInsecure, + ]); + + const airdropSignature = await provider.connection.requestAirdrop( + walletTwo.publicKey, + 1 * anchor.web3.LAMPORTS_PER_SOL, + ); + + const latestBlockHash = await provider.connection.getLatestBlockhash(); + + await provider.connection.confirmTransaction( + { + blockhash: latestBlockHash.blockhash, + lastValidBlockHeight: latestBlockHash.lastValidBlockHeight, + signature: airdropSignature, + }, + "confirmed", + ); }); - it("performs insecure initialization", async () => { - try { - await program.methods - .insecureInitialization() - .accounts({ - user: insecureUserAccount.publicKey, - authority: walletAuthority.publicKey, - }) - .signers([walletAuthority.payer]) - .rpc(); - } catch (error) { - console.error("Insecure initialization failed:", error); - throw error; - } + it("insecureInitialization should be successful", async () => { + await program.methods + .insecureInitialization() + .accounts({ + user: userInsecure.publicKey, + authority: wallet.publicKey, + }) + .signers([wallet.payer]) + .rpc(); }); - it("re-invokes insecure initialization with different authority", async () => { - try { - const transaction = await program.methods - .insecureInitialization() - .accounts({ - user: insecureUserAccount.publicKey, - authority: secondWallet.publicKey, - }) - .signers([secondWallet]) - .transaction(); - - await anchor.web3.sendAndConfirmTransaction( - provider.connection, - transaction, - [secondWallet], - ); - } catch (error) { - console.error("Re-invocation of insecure initialization failed:", error); - throw error; - } + it("insecureInitialization with a different authority should be successful again", async () => { + await program.methods + .insecureInitialization() + .accounts({ + user: userInsecure.publicKey, + authority: walletTwo.publicKey, + }) + .signers([walletTwo]) + .rpc(); }); }); ``` -Run `anchor test` to verify that the `insecure_initialization` instruction -handler executes successfully in both invocations. +Run `anchor test` to see that both transactions will complete successfully. ```bash -Initialization - ✔ performs insecure initialization (420ms) - ✔ re-invokes insecure initialization with different authority (419ms) +initialization + ✔ insecureInitialization should be successful (417ms) + ✔ insecureInitialization with a different authority should be successful again (417ms) ``` -### 3. Add recommended_initialization Instruction Handler +#### 3. Add `recommended_initialization` instruction -Now, let's create a new instruction handler called `recommended_initialization` -that addresses the issue. Unlike the insecure instruction handler, this one will +Let's create a new instruction called `recommended_initialization` that fixes +this problem. Unlike the previous insecure instruction, this instruction should handle both the creation and initialization of the user's account using Anchor's `init` constraint. -This constraint ensures the account is created via a CPI to the system program, -and the discriminator is set. This way, any subsequent invocation with the same -user account will fail, preventing reinitialization. +This constraint instructs the program to create the account via a CPI to the +system program, so the account no longer needs to be created client-side. The +constraint also sets the account discriminator. Your instruction logic can then +set the account's initial state. + +By doing this, you ensure that any subsequent invocation of the same instruction +with the same user account will fail rather than reset the account's initial +state. ```rust use anchor_lang::prelude::*; -use borsh::{BorshDeserialize, BorshSerialize}; declare_id!("Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"); +const DISCRIMINATOR_SIZE:usize = 8; + #[program] pub mod initialization { use super::*; - ... + ... pub fn recommended_initialization(ctx: Context) -> Result<()> { ctx.accounts.user.authority = ctx.accounts.authority.key(); Ok(()) @@ -406,118 +395,88 @@ pub mod initialization { #[derive(Accounts)] pub struct Checked<'info> { - #[account( - init, - payer = authority, - space = DISCRIMINATOR_SIZE + User::INIT_SPACE - )] + #[account(init, payer = authority, space = DISCRIMINATOR_SIZE + User::INIT_SPACE)] user: Account<'info, User>, #[account(mut)] - authority: Signer<'info>, - system_program: Program<'info, System>, -} - -#[account] -#[derive(InitSpace)] -pub struct User { - pub authority: Pubkey, + authority: Signer<'info> } ``` -### 4. Test recommended_initialization Instruction Handler +#### 4. Test `recommended_initialization` instruction -To test the `recommended_initialization` instruction handler, invoke it twice as -before. This time, the transaction should fail when attempting to initialize the -same account a second time. +To test the `recommended_initialization` instruction, we’ll invoke the +instruction twice just like before. This time, we expect the transaction to fail +when we try to initialize the same account a second time. ```typescript -describe("Initialization", () => { +describe("initialization", () => { ... - it("performs recommended initialization", async () => { - try { - await program.methods - .recommendedInitialization() - .accounts({ - user: recommendedUserAccount.publicKey, - }) - .signers([recommendedUserAccount]) - .rpc(); - } catch (error) { - console.error("Recommended initialization failed:", error); - throw error; - } +it("recommendedInitialization should be successful", async () => { + const tx = await program.methods + .recommendedInitialization() + .accounts({ + user: userRecommended.publicKey, + authority: wallet.publicKey, + }) + .signers([userRecommended]) + .rpc(); }); - it("fails to re-invoke recommended initialization with different authority", async () => { + it("recommendedInitialization with a different authority should throw an expection", async () => { try { - const transaction = await program.methods + const tx = await program.methods .recommendedInitialization() .accounts({ - user: recommendedUserAccount.publicKey, - authority: secondWallet.publicKey, + user: userRecommended.publicKey, + authority: walletTwo.publicKey, }) - .transaction(); - - await anchor.web3.sendAndConfirmTransaction( - provider.connection, - transaction, - [secondWallet, recommendedUserAccount], - { commitment: "confirmed" } - ); - - throw new Error("Re-invocation succeeded unexpectedly"); - } catch (error) { - if (error.message === "Re-invocation succeeded unexpectedly") { - throw error; - } - - if (error instanceof SendTransactionError) { - console.log("Transaction failed as expected"); - } else { - console.error("Unexpected error:", error); - } - console.log(error) - expect(error).to.exist; + .signers([userRecommended, walletTwo]) + .rpc(); + } catch (err) { + expect(err); + console.log(err); } }); }); ``` -Run `anchor test` to confirm that the second transaction fails with an error -indicating the account is already in use. +Run `anchor test` and to see that the second transaction which tries to +initialize the same account twice will now return an error stating the account +address is already in use. ```bash -'Program HLhxJzFYjtXCET4HxnSzv27SpXg16FWNDi2LvrNmSvzH invoke [1]', +'Program CpozUgSwe9FPLy9BLNhY2LTGqLUk1nirUkMMA5RmDw6t invoke [1]', 'Program log: Instruction: RecommendedInitialization', 'Program 11111111111111111111111111111111 invoke [2]', -'Allocate: account Address { address: FcW7tG71GKuRgxEbgFuuNQNV3HVSMmVyKATo74iCK4yi, base: None } already in use', +'Allocate: account Address { address: EMvbwzrs4VTR7G1sNUJuQtvRX1EuvLhqs4PFqrtDcCGV, base: None } already in use', 'Program 11111111111111111111111111111111 failed: custom program error: 0x0', -'Program HLhxJzFYjtXCET4HxnSzv27SpXg16FWNDi2LvrNmSvzH consumed 3330 of 200000 compute units', -'Program HLhxJzFYjtXCET4HxnSzv27SpXg16FWNDi2LvrNmSvzH failed: custom program error: 0x0' +'Program CpozUgSwe9FPLy9BLNhY2LTGqLUk1nirUkMMA5RmDw6t consumed 4018 of 200000 compute units', +'Program CpozUgSwe9FPLy9BLNhY2LTGqLUk1nirUkMMA5RmDw6t failed: custom program error: 0x0' ``` -Using Anchor's `init` constraint is usually sufficient to protect against -reinitialization attacks. While the fix for these security exploits is -straightforward, it is crucial. Every time you initialize an account, ensure -that you're either using the `init` constraint or implementing another check to -prevent resetting an existing account's initial state. +If you use Anchor's `init` constraint, that's usually all you need to protect +against reinitialization attacks! Remember, just because the fix for these +security exploits is simple doesn't mean it isn't important. Every time your +initialize an account, make sure you're either using the `init` constraint or +have some other check in place to avoid resetting an existing account's initial +state. -For the final solution code, refer to the -[`solution` branch of this repository](https://github.com/solana-developers/reinitialization-attacks/tree/solution). +If you want to take a look at the final solution code you can find it on the +`solution` branch of +[this repository](https://github.com/solana-developers/reinitialization-attacks/tree/solution). ## Challenge -Your challenge is to audit your own or other programs to practice avoiding this -security exploit. +Just as with other lessons in this unit, your opportunity to practice avoiding +this security exploit lies in auditing your own or other programs. -Take some time to review at least one program and confirm that instruction -handlers are adequately protected against reinitialization attacks. +Take some time to review at least one program and ensure that instructions are +properly protected against reinitialization attacks. -If you find a bug or exploit in another program, alert the developer. If you -find one in your own program, patch it immediately. +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=652c68aa-18d9-464c-9522-e531fd8738d5)! diff --git a/content/courses/program-security/signer-auth.md b/content/courses/program-security/signer-auth.md index 34777b07f..c5b480cbe 100644 --- a/content/courses/program-security/signer-auth.md +++ b/content/courses/program-security/signer-auth.md @@ -1,61 +1,61 @@ --- title: Signer Authorization objectives: - - Explain the security risks of not performing appropriate signer checks. - - Implement signer checks using native Rust - - Implement signer checks using Anchor's `Signer` type - - Implement signer checks using Anchor's `#[account(signer)]` constraint + - Explain the security risks associated with not performing appropriate signer + checks + - Implement signer checks using long-form Rust + - Implement signer checks using Anchor’s `Signer` type + - Implement signer checks using Anchor’s `#[account(signer)]` constraint description: - "Ensure instructions are only executed by authorized accounts by implementing - signer checks." + "Ensure instructions are only ran by authorized accounts by implmementing + Signer checks." --- ## Summary -- **Signer Checks** are essential to verify that specific accounts have signed a - transaction. Without proper signer checks, unauthorized accounts may execute - instructions they shouldn't be allowed to perform. -- In Anchor, you can use the `Signer` account type in your account validation - struct to automatically perform a signer check on a given account. -- Anchor also provides the - [`#[account(signer)]`](https://www.anchor-lang.com/docs/account-constraints) - constraint, which automatically verifies that a specified account has signed - the transaction. -- In native Rust, implement a signer check by verifying that an account's - `is_signer` property is `true`: - +- Use **Signer Checks** to verify that specific accounts have signed a + transaction. Without appropriate signer checks, accounts may be able to + execute instructions they shouldn’t be authorized to perform. +- To implement a signer check in Rust, simply check that an account’s + `is_signer` property is `true` ```rust if !ctx.accounts.authority.is_signer { - return Err(ProgramError::MissingRequiredSignature.into()); + return Err(ProgramError::MissingRequiredSignature.into()); } ``` +- In Anchor, you can use the **`Signer`** account type in your account + validation struct to have Anchor automatically perform a signer check on a + given account +- Anchor also has an account constraint that will automatically verify that a + given account has signed a transaction ## Lesson -**Signer checks** ensure that only authorized accounts can execute specific -instructions. Without these checks, any account might perform operations that -should be restricted, potentially leading to severe security vulnerabilities, -such as unauthorized access and control over program accounts. +Signer checks are used to verify that a given account’s owner has authorized a +transaction. Without a signer check, operations whose execution should be +limited to only specific accounts can potentially be performed by any account. +In the worst case scenario, this could result in wallets being completely +drained by attackers passing in whatever account they want to an instruction. + +#### Missing Signer Check -### Missing Signer Check +The example below shows an oversimplified version of an instruction that updates +the `authority` field stored on a program account. -Below is an oversimplified instruction handler that updates the `authority` -field on a program account. Notice that the `authority` field in the -`UpdateAuthority` account validation struct is of type `UncheckedAccount`. In -Anchor, the -[`UncheckedAccount`](https://docs.rs/anchor-lang/latest/anchor_lang/accounts/unchecked_account/struct.UncheckedAccount.html) -type indicates that no checks are performed on the account before executing the -instruction handler. +Notice that the `authority` field on the `UpdateAuthority` account validation +struct is of type `AccountInfo`. In Anchor, the `AccountInfo` account type +indicates that no checks are performed on the account prior to instruction +execution. -Although the `has_one` constraint ensures that the `authority` account passed to -the instruction handler matches the `authority` field on the `vault` account, -there is no verification that the `authority` account actually authorized the +Although the `has_one` constraint is used to validate the `authority` account +passed into the instruction matches the `authority` field stored on the `vault` +account, there is no check to verify the `authority` account authorized the transaction. -This omission allows an attacker to pass in the `authority` account's public key -and their own public key as the `new_authority` account, effectively reassigning -themselves as the new authority of the `vault` account. Once they have control, -they can interact with the program as the new authority. +This means an attacker can simply pass in the public key of the `authority` +account and their own public key as the `new_authority` account to reassign +themselves as the new authority of the `vault` account. At that point, they can +interact with the program as the new authority. ```rust use anchor_lang::prelude::*; @@ -79,10 +79,8 @@ pub struct UpdateAuthority<'info> { has_one = authority )] pub vault: Account<'info, Vault>, - /// CHECK: This account will not be checked by Anchor - pub new_authority: UncheckedAccount<'info>, - /// CHECK: This account will not be checked by Anchor - pub authority: UncheckedAccount<'info>, + pub new_authority: AccountInfo<'info>, + pub authority: AccountInfo<'info>, } #[account] @@ -92,20 +90,23 @@ pub struct Vault { } ``` -### Adding Signer Authorization Checks +#### Add signer authorization checks -To validate that the `authority` account signed the transaction, add a signer -check within the instruction handler: +All you need to do to validate that the `authority` account signed is to add a +signer check within the instruction. That simply means checking that +`authority.is_signer` is `true`, and returning a `MissingRequiredSignature` +error if `false`. -```rust +```typescript if !ctx.accounts.authority.is_signer { return Err(ProgramError::MissingRequiredSignature.into()); } ``` -By adding this check, the instruction handler will only proceed if the -`authority` account has signed the transaction. If the account is not signed, -the transaction will fail. +By adding a signer check, the instruction would only process if the account +passed in as the `authority` account also signed the transaction. If the +transaction was not signed by the account passed in as the `authority` account, +then the transaction would fail. ```rust use anchor_lang::prelude::*; @@ -133,10 +134,8 @@ pub struct UpdateAuthority<'info> { has_one = authority )] pub vault: Account<'info, Vault>, - /// CHECK: This account will not be checked by Anchor - pub new_authority: UncheckedAccount<'info>, - /// CHECK: This account will not be checked by Anchor - pub authority: UncheckedAccount<'info>, + pub new_authority: AccountInfo<'info>, + pub authority: AccountInfo<'info>, } #[account] @@ -146,15 +145,20 @@ pub struct Vault { } ``` -### Use Anchor's Signer Account Type +#### Use Anchor’s `Signer` account type + +However, putting this check into the instruction function muddles the separation +between account validation and instruction logic. -Incorporating the -[`signer`](https://docs.rs/anchor-lang/latest/anchor_lang/accounts/signer/struct.Signer.html) -check directly within the instruction handler logic can blur the separation -between account validation and instruction handler execution. To maintain this -separation, use Anchor's `Signer` account type. By changing the `authority` -account's type to `Signer` in the validation struct, Anchor automatically checks -at runtime that the specified account signed the transaction. +Fortunately, Anchor makes it easy to perform signer checks by providing the +`Signer` account type. Simply change the `authority` account’s type in the +account validation struct to be of type `Signer`, and Anchor will check at +runtime that the specified account is a signer on the transaction. This is the +approach we generally recommend since it allows you to separate the signer check +from instruction logic. + +In the example below, if the `authority` account does not sign the transaction, +then the transaction will fail before even reaching the instruction logic. ```rust use anchor_lang::prelude::*; @@ -178,8 +182,7 @@ pub struct UpdateAuthority<'info> { has_one = authority )] pub vault: Account<'info, Vault>, - /// CHECK: This account will not be checked by Anchor - pub new_authority: UncheckedAccount<'info>, + pub new_authority: AccountInfo<'info>, pub authority: Signer<'info>, } @@ -190,27 +193,33 @@ pub struct Vault { } ``` - -When you use the `Signer` type, no other ownership or type checks are +Note that when you use the `Signer` type, no other ownership or type checks are performed. - -### Using Anchor's `#[account(signer)]` Constraint - -While the `Signer` account type is useful, it doesn't perform other ownership or -type checks, limiting its use in instruction handler logic. The -[anchor's `#[account(signer)]`](https://www.anchor-lang.com/docs/account-constraints) -constraint addresses this by verifying that the account signed the transaction -while allowing access to its underlying data. - -For example, if you expect an account to be both a signer and a data source, -using the `Signer` type would require manual deserialization, and you wouldn't -benefit from automatic ownership and type checking. Instead, the -`#[account(signer)]` constraint allows you to access the data and ensure the -account signed the transaction. - -In this example, you can safely interact with the data stored in the `authority` -account while ensuring that it signed the transaction. +#### Use Anchor’s `#[account(signer)]` constraint + +While in most cases, the `Signer` account type will suffice to ensure an account +has signed a transaction, the fact that no other ownership or type checks are +performed means that this account can’t really be used for anything else in the +instruction. + +This is where the `signer` _constraint_ comes in handy. The `#[account(signer)]` +constraint allows you to verify the account signed the transaction, while also +getting the benefits of using the `Account` type if you wanted access to it’s +underlying data as well. + +As an example of when this would be useful, imagine writing an instruction that +you expect to be invoked via CPI that expects one of the passed in accounts to +be both a **\*\***signer**\*\*** on the transaciton and a \***\*\*\*\*\*\***data +source\***\*\*\*\*\*\***. Using the `Signer` account type here removes the +automatic deserialization and type checking you would get with the `Account` +type. This is both inconvenient, as you need to manually deserialize the account +data in the instruction logic, and may make your program vulnerable by not +getting the ownership and type checking performed by the `Account` type. + +In the example below, you can safely write logic to interact with the data +stored in the `authority` account while also verifying that it signed the +transaction. ```rust use anchor_lang::prelude::*; @@ -237,8 +246,7 @@ pub struct UpdateAuthority<'info> { has_one = authority )] pub vault: Account<'info, Vault>, - /// CHECK: This account will not be checked by Anchor - pub new_authority: UncheckedAccount<'info>, + pub new_authority: AccountInfo<'info>, #[account(signer)] pub authority: Account<'info, AuthState> } @@ -250,53 +258,52 @@ pub struct Vault { } #[account] pub struct AuthState{ - amount: u64, - num_depositors: u64, - num_vaults: u64 + amount: u64, + num_depositors: u64, + num_vaults: u64 } ``` ## Lab -In this lab, we'll create a simple program to demonstrate how a missing signer -check can allow an attacker to withdraw tokens that don't belong to them. This -program initializes a simplified token `vault` account and shows how the absence -of a signer check could result in the vault being drained. +Let’s practice by creating a simple program to demonstrate how a missing signer +check can allow an attacker to withdraw tokens that don’t belong to them. + +This program initializes a simplified token “vault” account and demonstrates how +a missing signer check could allow the vault to be drained. -### 1. Starter +#### 1. Starter -To get started, download the starter code from the -[`starter` branch of this repository](https://github.com/solana-developers/signer-auth/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 from the `starter` branch of +[this repository](https://github.com/solana-developers/solana-signer-auth/tree/starter). The +starter code includes a program with two instructions and the boilerplate setup +for the test file. -The `initialize_vault` instruction handler sets up two new accounts: `Vault` and -`TokenAccount`. The `Vault` account is initialized using a Program Derived -Address (PDA) and stores the address of a token account and the vault's -authority. The `vault` PDA will be the authority of the token account, enabling -the program to sign off on token transfers. +The `initialize_vault` instruction initializes two new accounts: `Vault` and +`TokenAccount`. The `Vault` account will be initialized using a Program Derived +Address (PDA) and store the address of a token account and the authority of the +vault. The authority of the token account will be the `vault` PDA which enables +the program to sign for the transfer of tokens. -The `insecure_withdraw` instruction handler transfers tokens from the `vault` -account's token account to a `withdraw_destination` token account. However, the -`authority` account in the `InsecureWithdraw` struct is of type -`UncheckedAccount`, a wrapper around `AccountInfo` that explicitly indicates the -account is unchecked. +The `insecure_withdraw` instruction will transfer tokens in the `vault` +account’s token account to a `withdraw_destination` token account. However, the +`authority` account in the `InsecureWithdraw` struct has a type of +`UncheckedAccount`. This is a wrapper around `AccountInfo` to explicitly +indicate the account is unchecked. -Without a signer check, anyone can provide the public key of the `authority` -account that matches the `authority` stored on the `vault` account, and the -`insecure_withdraw` instruction handler will continue processing. +Without a signer check, anyone can simply provide the public key of the +`authority` account that matches `authority` stored on the `vault` account and +the `insecure_withdraw` instruction would continue to process. -Although this example is somewhat contrived, as any DeFi program with a vault -would be more sophisticated, it effectively illustrates how the lack of a signer -check can lead to unauthorized token withdrawals. +While this is somewhat contrived in that any DeFi program with a vault would be +more sophisticated than this, it will show how the lack of a signer check can +result in tokens being withdrawn by the wrong party. ```rust use anchor_lang::prelude::*; use anchor_spl::token::{self, Mint, Token, TokenAccount}; -declare_id!("FeKh59XMh6BcN6UdekHnaFHsNH9NVE121GgDzSyYPKKS"); - -pub const DISCRIMINATOR_SIZE: usize = 8; +declare_id!("Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"); #[program] pub mod signer_authorization { @@ -334,7 +341,7 @@ pub struct InitializeVault<'info> { #[account( init, payer = authority, - space = DISCRIMINATOR_SIZE + Vault::INIT_SPACE, + space = 8 + 32 + 32, seeds = [b"vault"], bump )] @@ -373,91 +380,69 @@ pub struct InsecureWithdraw<'info> { } #[account] -#[derive(Default, InitSpace)] pub struct Vault { token_account: Pubkey, authority: Pubkey, } ``` -### 2. Test insecure_withdraw Instruction Handler +#### 2. Test `insecure_withdraw` instruction -The test file includes code to invoke the `initialize_vault` instruction -handler, using `walletAuthority` as the `authority` on the vault. The code then -mints 100 tokens to the `vaultTokenAccount` token account. Ideally, only the -`walletAuthority` key should be able to withdraw these 100 tokens from the -vault. +The test file includes the code to invoke the `initialize_vault` instruction +using `wallet` as the `authority` on the vault. The code then mints 100 tokens +to the `vault` token account. Theoretically, the `wallet` key should be the only +one that can withdraw the 100 tokens from the vault. -Next, we'll add a test to invoke `insecure_withdraw` on the program to -demonstrate that the current version allows a third party to withdraw those 100 -tokens. +Now, let’s add a test to invoke `insecure_withdraw` on the program to show that +the current version of the program allows a third party to in fact withdraw +those 100 tokens. -In the test, we'll use the `walletAuthority` public key as the `authority` -account but sign and send the transaction with a different keypair. +In the test, we’ll still use the public key of `wallet` as the `authority` +account, but we’ll use a different keypair to sign and send the transaction. ```typescript -describe("Signer Authorization", () => { - ... - it("performs insecure withdraw", async () => { - try { - const transaction = await program.methods - .insecureWithdraw() - .accounts({ - vault: vaultPDA, - tokenAccount: vaultTokenAccount.publicKey, - withdrawDestination: unauthorizedWithdrawDestination, - authority: walletAuthority.publicKey, - }) - .transaction(); - - await anchor.web3.sendAndConfirmTransaction(connection, transaction, [ - unauthorizedWallet, - ]); - - const tokenAccountInfo = await getAccount( - connection, - vaultTokenAccount.publicKey - ); - expect(Number(tokenAccountInfo.amount)).to.equal(0); - } catch (error) { - console.error("Insecure withdraw failed:", error); - throw error; - } - }); -}) +describe("signer-authorization", () => { + ... + it("Insecure withdraw should be successful", async () => { + const tx = await program.methods + .insecureWithdraw() + .accounts({ + withdrawDestination: withdrawDestinationFake, + }) + .transaction(); + + await anchor.web3.sendAndConfirmTransaction(connection, tx, [walletFake]); + + const balance = await connection.getTokenAccountBalance( + tokenAccount.publicKey + ); + expect(balance.value.uiAmount).to.eq(0); +}); ``` -Run `anchor test` to confirm that both transactions will be completed -successfully. +Run `anchor test` to see that both transactions will complete successfully. ```bash -Signer Authorization - ✔ initializes vault and mints tokens (882ms) - ✔ performs insecure withdraw (435ms) +signer-authorization + ✔ Initialize Vault should be successful (810ms) + ✔ Insecure withdraw should be successful (405ms) ``` -The `insecure_withdraw` instruction handler demonstrates a security -vulnerability. Since there is no signer check for the `authority` account, this -handler will transfer tokens from the `vaultTokenAccount` to the -`unauthorizedWithdrawDestination`, as long as the public key of the `authority` -account matches the `walletAuthority.publicKey` stored in the `vault` account's -`authority` field. - -In the test, we use the `unauthorizedWallet` to sign the transaction, while -still specifying the `walletAuthority.publicKey` as the authority in the -instruction accounts. This mismatch between the signer and the specified -`authority` would normally cause a transaction to fail. However, due to the lack -of a proper signer check in the `insecure_withdraw` handler, the transaction -succeeds. - -### 3. Add secure_withdraw Instruction Handler - -To fix this issue, we'll create a new instruction handler called -`secure_withdraw`. This instruction handler will be identical to -`insecure_withdraw`, but we'll use the `Signer` type in the Accounts struct to -validate the authority account in the `SecureWithdraw` struct. If the -`authority` account isn't a signer on the transaction, the transaction should -fail with an error. +Since there is no signer check for the `authority` account, the +`insecure_withdraw` instruction will transfer tokens from the `vault` token +account to the `withdrawDestinationFake` token account as long as the public key +of the`authority` account matches the public key stored on the authority field +of the `vault` account. Clearly, the `insecure_withdraw` instruction is as +insecure as the name suggests. + +#### 3. Add `secure_withdraw` instruction + +Let’s fix the problem in a new instruction called `secure_withdraw`. This +instruction will be identical to the `insecure_withdraw` instruction, except +we’ll use the `Signer` type in the Accounts struct to validate the `authority` +account in the `SecureWithdraw` struct. If the `authority` account is not a +signer on the transaction, then we expect the transaction to fail and return an +error. ```rust use anchor_lang::prelude::*; @@ -508,101 +493,70 @@ pub struct SecureWithdraw<'info> { } ``` -### 4. Test secure_withdraw Instruction Handler +#### 4. Test `secure_withdraw` instruction -With the new instruction handler in place, return to the test file to test the -`secureWithdraw` instruction handler. Invoke the `secureWithdraw` instruction -handler, using the `walletAuthority.publicKey` as the `authority` account, and -use the `unauthorizedWallet` keypair as the signer. Set the -`unauthorizedWithdrawDestination` as the withdraw destination. - -Since the `authority` account is validated using the `Signer` type, the -transaction should fail with a signature verification error. This is because the -`unauthorizedWallet` is attempting to sign the transaction, but it doesn't match -the `authority` specified in the instruction (which is -`walletAuthority.publicKey`). - -The test expects this transaction to fail, demonstrating that the secure -withdraw function properly validates the signer. If the transaction unexpectedly -succeeds, the test will throw an error indicating that the expected security -check did not occur. +With the instruction in place, return to the test file to test the +`secure_withdraw` instruction. Invoke the `secure_withdraw` instruction, again +using the public key of `wallet` as the `authority` account and the +`withdrawDestinationFake` keypair as the signer and withdraw destination. Since +the `authority` account is validated using the `Signer` type, we expect the +transaction to fail the signer check and return an error. ```typescript -describe("Signer Authorization", () => { - ... - it("fails to perform secure withdraw with incorrect signer", async () => { +describe("signer-authorization", () => { + ... + it("Secure withdraw should throw an exception", async () => { try { - const transaction = await program.methods + const tx = await program.methods .secureWithdraw() .accounts({ - vault: vaultPDA, - tokenAccount: vaultTokenAccount.publicKey, - withdrawDestination: unauthorizedWithdrawDestination, - authority: walletAuthority.publicKey, + withdrawDestination: withdrawDestinationFake, }) .transaction(); - await anchor.web3.sendAndConfirmTransaction(connection, transaction, [ - unauthorizedWallet, - ]); - throw new Error("Expected transaction to fail, but it succeeded"); - } catch (error) { - expect(error).to.be.an("error"); - console.log("Error message:", error.message); + await anchor.web3.sendAndConfirmTransaction(connection, tx, [walletFake]); + } catch (err) { + expect(err); + console.log(err); } }); -}) +}); ``` -Run `anchor test` to see that the transaction now returns a signature +Run `anchor test` to see that the transaction will now return a signature verification error. ```bash -signer-authorization -Error message: Signature verification failed. -Missing signature for public key [`GprrWv9r8BMxQiWea9MrbCyK7ig7Mj8CcseEbJhDDZXM`]. - ✔ fails to perform secure withdraw with incorrect signer +Error: Signature verification failed ``` -This example shows how important it is to think through who should authorize -instructions and ensure that each is a signer on the transaction. +That’s it! This is a fairly simple thing to avoid, but incredibly important. +Make sure to always think through who should who should be authorizing +instructions and make sure that each is a signer on the transaction. -To review the final solution code, you can find it on the -[`solution` branch of the repository](https://github.com/solana-developers/signer-auth/tree/solution). +If you want to take a look at the final solution code you can find it on the +`solution` branch of +[the repository](https://github.com/solana-developers/solana-signer-auth//tree/solution). ## Challenge -Now that you've worked through the labs and challenges in this course, it's time -to apply your knowledge in a practical setting. For this challenge and those -that follow on security vulnerabilities, audit your own programs for the -specific vulnerability discussed in each lesson. - -### Steps +At this point in the course, we hope you've started to work on programs and +projects outside the labs and Challenges provided in these lessons. For this and +the remainder of the lessons on security vulnerabilities, the Challenge for each +lesson will be to audit your own code for the security vulnerability discussed +in the lesson. -1. **Audit Your Program or Find an Open Source Project**: +Alternatively, you can find open source programs to audit. There are plenty of +programs you can look at. A good start if you don't mind diving into native Rust +would be the +[SPL programs](https://github.com/solana-labs/solana-program-library). - - Begin by auditing your own code for missing signer checks, or find an open - source Solana program to audit. A great place to start is with the - [program examples](https://github.com/solana-developers/program-examples) - repository. - -2. **Look for Signer Check Issues**: - - - Focus on instruction handlers where signer authorization is crucial, - especially those that transfer tokens or modify sensitive account data. - - Review the program for any `UncheckedAccount` types where signer validation - should be enforced. - - Ensure that any accounts that should require user authorization are defined - as `Signer` in the instruction handler. - -3. **Patch or Report**: - - If you find a bug in your own code, fix it by using the `Signer` type for - accounts that require signer validation. - - If the issue exists in an open source project, notify the project - maintainers or submit a pull request. +So for this lesson, take a look at a program (whether yours or one you've found +online) and audit it for signer checks. If you find a bug in somebody else's +program, please alert them! If you find a bug in your own program, be sure to +patch it right away. - -After completing the challenge, push your code to GitHub and +Push your code to GitHub and [tell us what you thought of this lesson](https://form.typeform.com/to/IPH0UGz7#answers-lesson=26b3f41e-8241-416b-9cfa-05c5ab519d80)!