5

I've written an onchain program with anchor. This is how my withdraw function looks like(admin public key is random and used for demonstration only):

const ADMIN_PUBKEY: &str = "76rcGHdPvgs8G1XrzCXUTWtwgT59AFDvpB4VbTS2TBBJ";
const HOUSE_ACCOUNT_SEED: &str = "house_account_seed";

#[derive(Accounts)]
    #[instruction(amount: u64, bump: u8)]
    pub struct HouseAccountWithdraw<'info> {
        /// CHECK: This is not dangerous because we don't read or write from this account
        #[account(mut, signer, constraint = admin.key.to_string() == ADMIN_PUBKEY)]
        admin: AccountInfo<'info>,
        /// CHECK: This is not dangerous because we don't read or write from this account
        #[account(mut, seeds=[&HOUSE_ACCOUNT_SEED.as_bytes()], bump=bump)]
        house_account: AccountInfo<'info>,
        pub system_program: Program<'info, System>,
    }

    pub fn house_account_withdraw(
        ctx: Context<HouseAccountWithdraw>,
        amount: u64,
        bump: u8,
    ) -> Result<()> {
        let admin = &ctx.accounts.admin;
        let house_account = &ctx.accounts.house_account;
        let system_program = &ctx.accounts.system_program;
        let lamports_to_transfer = amount;
        invoke_signed(
            &system_instruction::transfer(house_account.key, admin.key, lamports_to_transfer),
            &[
                house_account.clone(),
                admin.clone(),
                system_program.to_account_info(),
            ],
            &[&[&HOUSE_ACCOUNT_SEED.as_bytes(), &[bump]]],
        )?;
        Ok(())
    }

I've checked corals repo with security practices for signer authorization: https://github.com/coral-xyz/sealevel-attacks/tree/master/programs/0-signer-authorization

This explains insecure authority type:

#[derive(Accounts)]
pub struct LogMessage<'info> {
    authority: AccountInfo<'info>,
}

Recommended type should be Signer:

#[derive(Accounts)]
pub struct LogMessage<'info> {
    authority: Signer<'info>,
}

And secure without recommended type, with using is_signer check in function:

#[program]
pub mod signer_authorization_secure {
    use super::*;

    pub fn log_message(ctx: Context<LogMessage>) -> ProgramResult {
        if !ctx.accounts.authority.is_signer {
            return Err(ProgramError::MissingRequiredSignature);
        }
        msg!("GM {}", ctx.accounts.authority.key().to_string());
        Ok(())
    }
}

#[derive(Accounts)]
pub struct LogMessage<'info> {
    authority: AccountInfo<'info>,
}

In my code above, my admin is defined as:

/// CHECK: This is not dangerous because we don't read or write from this account
#[account(mut, signer, constraint = admin.key.to_string() == ADMIN_PUBKEY)]
admin: AccountInfo<'info>,

It has signer constraint, which I thought if admin is not signer it would not be possible for someone else to drain it? So I am not really sure why signer constraint exists if it doesn't protect.

If my method is vuln, is there a way to reproduce it, so I can be sure my onchain program is secure before I deploy fixed version. Currently I use this ts snippet to withdraw, and if I put wrong signer it doesn't allow me:

describe("house deposit and withdraw sol", () => {
  const provider = anchor.AnchorProvider.env();
  anchor.setProvider(provider);

  const program = anchor.workspace.OnlyDice as Program<OnlyDice>;

  let houseProgramDerivedAccount: PublicKey = null;
  let houseProgramDerivedAccountBump: number = null;

  it("setup house SOL PDAs", async () => {
    // --------- GET HOUSE SOL PDA ACCOUNT
    [houseProgramDerivedAccount, houseProgramDerivedAccountBump] =
      await anchor.web3.PublicKey.findProgramAddress(
        [Buffer.from(HOUSE_ACCOUNT_SEED)],
        PROGRAM_ID
      );

    console.log("House SOL PDA: " + houseProgramDerivedAccount);
  });

  it("house withdraw sol", async () => {
    const houseProgramDerivedAccountBalanceInit =
      await program.provider.connection.getBalance(houseProgramDerivedAccount);

    console.log(
      "houseProgramDerivedAccountBalanceInit balance: " +
        houseProgramDerivedAccountBalanceInit
    );

    let amount = new anchor.BN(LAMPORTS_PER_SOL * 0.001);
    let txId = "";

    try {
      txId = await program.methods
        .houseAccountWithdraw(amount, houseProgramDerivedAccountBump)
        .accounts({
          admin: ADMIN_KEYPAIR.publicKey,
          systemProgram: SystemProgram.programId,
          houseAccount: houseProgramDerivedAccount,
        })
        .signers([ADMIN_KEYPAIR])
        .rpc();
    } catch (err) {
      console.log("house sol withdraw error: " + err);
    }

    const houseProgramDerivedAccountBalanceAfter =
      await program.provider.connection.getBalance(houseProgramDerivedAccount);

    assert.equal(
      houseProgramDerivedAccountBalanceInit - amount,
      houseProgramDerivedAccountBalanceAfter,
      "After initial house sol deposit pda should be " +
        houseProgramDerivedAccountBalanceInit +
        " lamports"
    );

    console.log(
      "houseProgramDerivedAccountBalanceAfter balance: " +
        houseProgramDerivedAccountBalanceAfter
    );

    console.log("txId - house withdraw: " + txId);
  });
}

My onchain program also support SPL deposit and withdrawal to PDA, and got drained also for some DUST, I checked the instruction log for that tx and it seems like the hacker created ATA with his address and somehow changed the ownership (immutable ownership extension https://spl.solana.com/token-2022/extensions#immutable-owner):

enter image description here

I know 135SOL is lost, it's not cheap. I am going to implement recommended signer authentication type to signer, but I would like to know how could I replicate the hacker attempt, to be sure I cannot withdraw funds if the admin is really not the signer, that's why I would be grateful to learn from this if anyone is generous to explain how could I replicate, and what exactly is vulnerability in my onchain program method, aka why signer constraint is not enough.

2
  • 3
    Could you share the tx hash of the transactions, both the SOL hack and the DUST hack? Jan 21, 2023 at 11:51
  • 4
    So my guess is, given this is a house account, this is some kind of gambling contract. What likely happened is someone exploited your contract by using a subsequent instruction to fail the tx if they didn't win the coin flip (or whatever other random element). Using this they were able to drain the house account.
    – Noah
    Jul 17, 2023 at 22:14

2 Answers 2

2

I look up "OnlyDice" and found https://onlydice.io . Looking at the last couple transactions of the "dealer hot wallet" C1VqZ33mMD3G3ATpGLqZKSbBwiEcDkRCG3VsgYmcjsYJ, it seems like they were large SOL withdrawals that were indeed signed by 5hTtE2Z68QBQ4Gvv1yoYzXhSiJmLedpMw1HYF7EfCrUC, which presumably is your admin pubkey. If thats the case, this means your admin pubkey is compromised. Did you store it in Slope Wallet 2 years ago or smth?

-1

Heya not a smart contract expert, but ive looked into the PDA closing thing before. AFAIK i think you need to look at the hasone constraint.

I have put the tutorial link below, scroll down till you find PDAs in anchor, and then the last bit covers closing accounts/PDAs, but i do not think your contract was exploited like that, but more so like noah said where the logic was able to be shortciruited in some way.

https://academy.patika.dev/courses/solana-development-ii/pdas-in-anchor

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.