Skip to content
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

lang: potential bug/footgun in generate ATA constraint function #1223

Closed
paul-schaaf opened this issue Jan 2, 2022 · 1 comment · Fixed by #1240
Closed

lang: potential bug/footgun in generate ATA constraint function #1223

paul-schaaf opened this issue Jan 2, 2022 · 1 comment · Fixed by #1240
Labels
bug Something isn't working lang

Comments

@paul-schaaf
Copy link
Contributor

paul-schaaf commented Jan 2, 2022

Currently, the generate ATA constraint function looks like this

fn generate_constraint_associated_token(
    f: &Field,
    c: &ConstraintAssociatedToken,
) -> proc_macro2::TokenStream {
    let name = &f.ident;
    let wallet_address = &c.wallet;
    let spl_token_mint_address = &c.mint;
    quote! {
        let __associated_token_address = anchor_spl::associated_token::get_associated_token_address(&#wallet_address.key(), &#spl_token_mint_address.key());
        if #name.to_account_info().key != &__associated_token_address {
            return Err(anchor_lang::__private::ErrorCode::ConstraintAssociated.into());
        }
    }
}

That is, it only checks the incoming address, but not the incoming authority. This may be unexpected because unfortunately, the authority of an ATA can change which this constraint will not catch because the address will still be correct.

The question is whether we want this constraint to catch this case or not (mb if this discussion has already been held and I missed it).

imo, it should catch this case because writing associated_token::authority = target but then having that authority ignored in an edge case feels unintuitive.

The init_if_needed checks that run when init is not needed do check for this. Whatever we choose, the behaviour should be the same everywhere

@armaniferrante @cqfd

@paul-schaaf paul-schaaf added bug Something isn't working lang labels Jan 2, 2022
@armaniferrante
Copy link
Member

armaniferrante commented Jan 4, 2022

Agreed it should catch the case, although, one should never change the authority for the ATA. That violates a core assumption. So this check is in the belt and suspenders category, but we should still do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lang
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants