fix and improve signer-auth.md & reinitialization-attacks.md & pda-sharing.md & owner-checks.md & closing-accounts.md & arbitrary-cpi.md

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 = <address_to_send_lamports>)]`
-  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,66 +95,47 @@ 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<Close>) -> ProgramResult {
-        let account = ctx.accounts.account.to_account_info();
 
+    pub fn close(ctx: Context<Close>) -> 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(())
     }
 }
 
 #[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<ForceDefund>) -> 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 = <target_account>)]` constraint. This constraint handles
 everything required to securely close an account:
 
-1. Transfers the account's lamports to the given `<target_account>`
+1. Transfers the account’s lamports to the given `<target_account>`
 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,24 +449,27 @@ 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,
 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