From 97a4c15b7f8e001f6b9f13565a4dbc32827a1ee3 Mon Sep 17 00:00:00 2001 From: Jeff Wood Date: Sat, 24 Aug 2024 03:12:35 +0000 Subject: [PATCH 1/3] Update the Token Extensions Program link --- content/courses/tokens-and-nfts/token-program.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/content/courses/tokens-and-nfts/token-program.md b/content/courses/tokens-and-nfts/token-program.md index 08794cf32..fdb4b999a 100644 --- a/content/courses/tokens-and-nfts/token-program.md +++ b/content/courses/tokens-and-nfts/token-program.md @@ -28,7 +28,7 @@ description: - Creating Token Mints and Token Accounts requires allocating **rent** in SOL. The rent for a Token Account can be refunded when the account is closed. Additionally, tokens created with the - [Token Extensions Program](/developers/courses/token-extensions-for-mints/close-mint) + [Token Extensions Program](/content/courses/token-extensions/close-mint) can also close Token Mints. ### Lesson From 2ba6531b7f3723321a0b5c1ae8b072add7f9ad54 Mon Sep 17 00:00:00 2001 From: wuuer Date: Fri, 27 Sep 2024 09:59:44 +0800 Subject: [PATCH 2/3] sync token-program.md from main --- content/courses/tokens-and-nfts/token-program.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/content/courses/tokens-and-nfts/token-program.md b/content/courses/tokens-and-nfts/token-program.md index fdb4b999a..08794cf32 100644 --- a/content/courses/tokens-and-nfts/token-program.md +++ b/content/courses/tokens-and-nfts/token-program.md @@ -28,7 +28,7 @@ description: - Creating Token Mints and Token Accounts requires allocating **rent** in SOL. The rent for a Token Account can be refunded when the account is closed. Additionally, tokens created with the - [Token Extensions Program](/content/courses/token-extensions/close-mint) + [Token Extensions Program](/developers/courses/token-extensions-for-mints/close-mint) can also close Token Mints. ### Lesson From ad99d204652013db6a62be207cadb47284040688 Mon Sep 17 00:00:00 2001 From: wuuer Date: Fri, 27 Sep 2024 10:36:40 +0800 Subject: [PATCH 3/3] =?UTF-8?q?Use=C2=A0InitSpace=20to=20calculate=20space?= =?UTF-8?q?=20needed=20for=20accounts.=20Delete=20"force=5Fdefund"=20?= =?UTF-8?q?=C2=A0because=20=E2=80=9CCLOSED=5FACCOUNT=5FDISCRIMINATOR?= =?UTF-8?q?=E2=80=9D=20was=20removed=20in=20the=20latest=20version=20of=20?= =?UTF-8?q?anchor-lang.=20Add=20the=20new=20secure=20instruction=20for=20a?= =?UTF-8?q?ccount=20closing=20in=20the=20"Secure=20account=20closing"=20se?= =?UTF-8?q?ction.=20Change=20the=20Logic=20of=20closing=20account=20"Sets?= =?UTF-8?q?=20the=20account=20discriminator=20to=20the=20`CLOSED=5FACCOUNT?= =?UTF-8?q?=5FDISCRIMINATOR`=20variant"=20to=20"Assigning=20the=20owner=20?= =?UTF-8?q?to=20the=20System=20Program".?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../program-security/closing-accounts.md | 174 +++++------------- 1 file changed, 47 insertions(+), 127 deletions(-) diff --git a/content/courses/program-security/closing-accounts.md b/content/courses/program-security/closing-accounts.md index 2f62f0c9c..5a55a1be7 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>, @@ -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,73 +145,6 @@ 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 @@ -240,7 +153,8 @@ everything required to securely close an account: 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 @@ -533,16 +450,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 +470,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