Open iurage opened 1 year ago
I am guessing this is because there isn't a check that the account is owned by the TokenProgram?
First, the code attempts to log a message after deserializing the provided account (token) using the SplTokenAccount::unpack utility from the spl_token crate. However, it does not verify that the token account is indeed owned by the expected token program (such as the SPL Token program). Second, although the code performs an owner check using ctx.accounts.authority.key != &token.owner, where ctx.accounts.authority is expected to be the authorized signer, this check only verifies that the signer (ctx.accounts.authority) is the owner of the token account, not that the token account itself is a valid SPL Token account. This means that an attacker could create a malicious program that creates a fake account and sets any arbitrary owner address to pass this check. As a result of these issues, a malicious actor could potentially pass any arbitrary account, and as long as they have control over the account (i.e., the private key corresponding to the authority), the program will not throw an error. This could lead to unintended consequences, such as reading sensitive data from unrelated accounts or performing unauthorized actions. To ensure security, it's crucial to verify that the provided token account is a legitimate SPL Token account owned by the SPL Token program before performing any operations or checks on it. This can be achieved by checking the account's program_id field against the correct program ID for the SPL Token program. The correct program ID can be obtained using spl_token::id(). Here's an example of how to fix the code by adding the necessary checks:
use anchor_lang::prelude::*; use anchor_lang::solana_program::program_error::ProgramError; use anchor_lang::solana_program::program_pack::Pack; use spl_token::state::Account as SplTokenAccount;
// ... (rest of the code)
pub mod owner_checks_secure { use super::*;
pub fn log_message(ctx: Context<LogMessage>) -> ProgramResult {
let token = SplTokenAccount::unpack(&ctx.accounts.token.data.borrow())?;
// Ensure the token account is owned by the correct Token program
if *ctx.accounts.token.to_account_info().key != spl_token::id() {
return Err(ProgramError::InvalidAccountData);
}
// Perform the authority check to ensure the signer is the owner of the token account.
if ctx.accounts.authority.key != &token.owner {
return Err(ProgramError::InvalidAccountData);
}
msg!("Your account balance is: {}", token.amount);
Ok(())
}
}
// ... (rest of the code)
By adding the check to ensure the token account is owned by the correct Token program, the code becomes more secure and prevents unauthorized accounts from passing the checks. This way, we can avoid potential security vulnerabilities and maintain the integrity of the program's operations.
In this example, the account was deserialized with the SplTokenAccount util and owner check is performed after. Why is this example insecure other than not deserializing with the Anchor TokenAccount struct?