-
Notifications
You must be signed in to change notification settings - Fork 75
fix: separate refund claim when there is no token account #633
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
64103df
fdf2201
3deb865
168dab2
e1be75d
120c198
1f31c01
79fa785
4c0fb22
397885a
2361d31
ec8e213
fdbd8d4
d6f5e15
e904f90
c7758ec
79476ba
cdb9ac3
d1caae6
655219d
bb1f745
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| use anchor_lang::prelude::*; | ||
| use anchor_spl::token_interface::{ | ||
| transfer_checked, Mint, TokenAccount, TokenInterface, TransferChecked, | ||
| }; | ||
|
|
||
| use crate::{ | ||
| constants::DISCRIMINATOR_SIZE, | ||
| error::CustomError, | ||
| state::{ClaimAccount, State}, | ||
| }; | ||
|
|
||
| #[derive(Accounts)] | ||
| #[instruction(mint: Pubkey, token_account: Pubkey)] | ||
| pub struct InitializeClaimAccount<'info> { | ||
| #[account(mut)] | ||
| pub signer: Signer<'info>, | ||
|
|
||
| #[account( | ||
| init, | ||
| payer = signer, | ||
| space = DISCRIMINATOR_SIZE + ClaimAccount::INIT_SPACE, | ||
| seeds = [b"claim_account", mint.as_ref(), token_account.as_ref()], | ||
| bump | ||
| )] | ||
| pub claim_account: Account<'info, ClaimAccount>, | ||
|
|
||
| pub system_program: Program<'info, System>, | ||
| } | ||
|
|
||
| #[derive(Accounts)] | ||
| pub struct ClaimRelayerRefund<'info> { | ||
| #[account(mut)] | ||
| pub signer: Signer<'info>, | ||
|
|
||
| #[account(mut, seeds = [b"state", state.seed.to_le_bytes().as_ref()], bump)] | ||
| pub state: Account<'info, State>, | ||
|
|
||
| #[account( | ||
| mut, | ||
| associated_token::mint = mint, | ||
| associated_token::authority = state, | ||
| associated_token::token_program = token_program | ||
| )] | ||
| pub vault: InterfaceAccount<'info, TokenAccount>, | ||
|
|
||
| // Mint address has been checked when executing the relayer refund leaf and it is part of claim account derivation. | ||
| #[account( | ||
| mint::token_program = token_program, | ||
| )] | ||
| pub mint: InterfaceAccount<'info, Mint>, | ||
|
|
||
| // Token address has been checked when executing the relayer refund leaf and it is part of claim account derivation. | ||
| #[account( | ||
| mut, | ||
| token::mint = mint, | ||
| token::token_program = token_program | ||
| )] | ||
| pub token_account: InterfaceAccount<'info, TokenAccount>, | ||
|
|
||
| #[account( | ||
| mut, | ||
| seeds = [b"claim_account", mint.key().as_ref(), token_account.key().as_ref()], | ||
| bump | ||
| )] | ||
| pub claim_account: Account<'info, ClaimAccount>, | ||
|
|
||
| pub token_program: Interface<'info, TokenInterface>, | ||
| } | ||
|
|
||
| pub fn claim_relayer_refund(ctx: Context<ClaimRelayerRefund>) -> Result<()> { | ||
| // Ensure the claim account holds a non-zero amount. | ||
| let claim_amount = ctx.accounts.claim_account.amount; | ||
| require!(claim_amount > 0, CustomError::ZeroRefundClaim); | ||
|
|
||
| // Reset the claim amount. | ||
| ctx.accounts.claim_account.amount = 0; | ||
|
|
||
| // Derive the signer seeds for the state required for the transfer form vault. | ||
| let state_seed_bytes = ctx.accounts.state.seed.to_le_bytes(); | ||
| let seeds = &[b"state", state_seed_bytes.as_ref(), &[ctx.bumps.state]]; | ||
| let signer_seeds = &[&seeds[..]]; | ||
|
|
||
| // Transfer the claim amount from the vault to the relayer token account. | ||
| let transfer_accounts = TransferChecked { | ||
| from: ctx.accounts.vault.to_account_info(), | ||
| mint: ctx.accounts.mint.to_account_info(), | ||
| to: ctx.accounts.token_account.to_account_info(), | ||
| authority: ctx.accounts.state.to_account_info(), | ||
| }; | ||
| let cpi_context = CpiContext::new_with_signer( | ||
| ctx.accounts.token_program.to_account_info(), | ||
| transfer_accounts, | ||
| signer_seeds, | ||
| ); | ||
| transfer_checked(cpi_context, claim_amount, ctx.accounts.mint.decimals) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| use anchor_lang::prelude::*; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit on this file naming: we call the instruction
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| use anchor_spl::token_interface::TokenAccount; | ||
|
|
||
| use crate::error::CustomError; | ||
|
|
||
| #[account] | ||
| #[derive(InitSpace)] | ||
| pub struct ClaimAccount { | ||
| pub amount: u64, | ||
| } | ||
|
|
||
| // When executing relayer refund leaf, refund accounts are passed as remaining accounts and can hold either a regular | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this mean that, in theory, a relayer could close their ATA right before the bundle execution tx lands on-chain, thereby making the execution of the bundle in-valid as the remaining account type is wrong? so when the executor looks at the tx to send they build TokenAccount, expecting that the relayer has an ATA but before the tx lands on-chain the relayer can front run the tx and delete their ATA, which then requires, for this method to work, that the executor passes in the ClaimAccount, not token account, which breaks the bundle execution and the whole thing reverts? In theory, this means that a relayer who can front run the execution can always "toggle" the status of their ATA (opening it or closing it), thereby blocking the bundle execution? if the executor sends these execution transactions via private mempool they might be able to protect themselves (you cant close/open the account before the execution) but this is not going to be failsafe? thoughts on this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In theory that's possible, but might be a bigger effort as there is no public mempool, so the attacker has to run Solana validator co-located close to our bot. We can reduce attack surface by sending the transaction to trusted validator. And if this is still an issue, we can fall-back using claim accounts after couple of reverts. |
||
| // token account or a claim account. This enum is used to differentiate between the two types. | ||
| pub enum RefundAccount<'info> { | ||
| TokenAccount(InterfaceAccount<'info, TokenAccount>), | ||
| ClaimAccount(Account<'info, ClaimAccount>), | ||
| } | ||
|
|
||
| impl<'c, 'info> RefundAccount<'info> | ||
| where | ||
| 'c: 'info, | ||
| { | ||
| // This function is used to parse a refund account from the remaining accounts list. It first tries to parse it as | ||
| // a token account and if that fails, it tries to parse it as a claim account. | ||
| pub fn try_from_remaining_account( | ||
| remaining_accounts: &'c [AccountInfo<'info>], | ||
| index: usize, | ||
| expected_token_account: &Pubkey, | ||
| expected_mint: &Pubkey, | ||
| token_program: &Pubkey, | ||
| ) -> Result<Self> { | ||
| let refund_account_info = remaining_accounts | ||
| .get(index) | ||
| .ok_or(ErrorCode::AccountNotEnoughKeys)?; | ||
|
|
||
| Self::try_token_account_from_account_info( | ||
| refund_account_info, | ||
| expected_token_account, | ||
| expected_mint, | ||
| token_program, | ||
| ) | ||
| .map(Self::TokenAccount) | ||
| .or_else(|| { | ||
| Self::try_claim_account_from_account_info( | ||
| refund_account_info, | ||
| expected_mint, | ||
| expected_token_account, | ||
| ) | ||
| .map(Self::ClaimAccount) | ||
| }) | ||
| .ok_or_else(|| { | ||
| error::Error::from(CustomError::InvalidRefund) | ||
| .with_account_name(&format!("remaining_accounts[{}]", index)) | ||
| }) | ||
| } | ||
|
|
||
| // This implements the following Anchor account constraints when parsing remaining account as a token account: | ||
| // #[account( | ||
| // mut, | ||
| // address = expected_token_account @ CustomError::InvalidRefund, | ||
| // token::mint = expected_mint, | ||
| // token::token_program = token_program | ||
| // )] | ||
| // pub token_account: InterfaceAccount<'info, TokenAccount>, | ||
|
Comment on lines
+59
to
+65
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this mean to be commented out?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, left them for auditing purposes as we reimplement what Anchor macro would have done on static account. |
||
| // Note: All errors are ignored and Option is returned as we do not log them anyway due to memory constraints. | ||
| fn try_token_account_from_account_info( | ||
| account_info: &'info AccountInfo<'info>, | ||
| expected_token_account: &Pubkey, | ||
| expected_mint: &Pubkey, | ||
| token_program: &Pubkey, | ||
| ) -> Option<InterfaceAccount<'info, TokenAccount>> { | ||
| // Checks ownership on deserialization for the TokenAccount interface. | ||
| let token_account: InterfaceAccount<'info, TokenAccount> = | ||
| InterfaceAccount::try_from(account_info).ok()?; | ||
|
|
||
| // Checks if the token account is writable. | ||
| if !account_info.is_writable { | ||
| return None; | ||
| } | ||
|
|
||
| // Checks the token address matches. | ||
| if account_info.key != expected_token_account { | ||
| return None; | ||
| } | ||
|
|
||
| // Checks if the token account is associated with the expected mint. | ||
| if &token_account.mint != expected_mint { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a meta-comment but this check has a point is: is there away to do this more consistently in this file and beyond?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but you're right that its not fully consistent with comparison in |
||
| return None; | ||
| } | ||
|
|
||
| // Checks ownership by specific token program. | ||
| if account_info.owner != token_program { | ||
| return None; | ||
| } | ||
|
|
||
| Some(token_account) | ||
| } | ||
|
|
||
| // This implements the following Anchor account constraints when parsing remaining account as a claim account: | ||
| // #[account( | ||
| // mut, | ||
| // seeds = [b"claim_account", mint.key().as_ref(), token_account.key().as_ref()], | ||
| // bump | ||
| // )] | ||
| // pub claim_account: Account<'info, ClaimAccount>, | ||
|
Comment on lines
+101
to
+106
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this meant to be commented out?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it is meant to document what Anchor macros this method is reimplementing, so its easier to audit knowing the intention of the implementation. |
||
| // Note: All errors are ignored and Option is returned as we do not log them anyway due to memory constraints. | ||
| fn try_claim_account_from_account_info( | ||
| account_info: &'info AccountInfo<'info>, | ||
| mint: &Pubkey, | ||
| token_account: &Pubkey, | ||
| ) -> Option<Account<'info, ClaimAccount>> { | ||
| // Checks ownership on deserialization for the ClaimAccount. | ||
| let claim_account: Account<'info, ClaimAccount> = Account::try_from(account_info).ok()?; | ||
|
|
||
| // Checks the PDA is derived from mint and token account keys. | ||
| let (pda_address, _bump) = Pubkey::find_program_address( | ||
| &[b"claim_account", mint.as_ref(), token_account.as_ref()], | ||
| &crate::ID, | ||
| ); | ||
| if account_info.key() != pda_address { | ||
| return None; | ||
| } | ||
|
|
||
| // Checks if the claim account is writable. | ||
| if !account_info.is_writable { | ||
| return None; | ||
| } | ||
|
|
||
| Some(claim_account) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you help me understand this bit of syntax a bit better? I also see it in the
lib.rs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the constraint that lifetime for remaining accounts (
'c) is at least as long as'infofor accounts that are referenced by this account array. I think its needed because of manual remaining account processing intry_from_remaining_accountmethod. Without these lifetime constraints the compiler errors.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
o that's interesting. ye, I've had other lifetime issues while implementing other things and was able to address it through other syntax but this is more complex here.