fix: enforce canonical bump in ata verification#2249
Conversation
📝 WalkthroughWalkthroughThe PR removes explicit ATA bump fields and parameters across programs, SDKs, macros, and tests, shifting bump derivation to on-chain canonical computation. Function signatures, instruction data, macro attributes, and CPI/SDK builders were updated to stop passing or returning bump values and to return ATA addresses only. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This PR absorbs #2250 (issue #3) and #2255 (issue #4) — all three modify the same block in Combined changes:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
sdk-libs/token-sdk/src/instruction/decompress.rs (1)
126-135: 🧹 Nitpick | 🔵 Trivial
bumpfield still written to TLV but no longer consumed on-chain — consider follow-up cleanup.The
CompressedOnlyExtensionInstructionData.bumpis populated here (line 133) but the on-chain decompression path intoken.rsnow discards it (_ata_bump). The on-chainverify_pdare-derives the canonical bump independently. This is safe, but the field is now vestigial in the verification flow. A follow-up PR could remove it from the struct to avoid future confusion — though that would require a migration for existing compressed accounts that already have this field serialized.sdk-libs/macros/docs/accounts/associated_token.md (1)
251-254:⚠️ Potential issue | 🟡 MinorDocumentation is inconsistent with the new API —
derive_associated_token_accountno longer returns a tuple.The function now returns a single value (the ATA address), not
(address, bump). This example will confuse developers who try to copy-paste it.📝 Proposed fix
-let (expected_ata, _) = derive_associated_token_account(&owner, &mint); -assert_eq!(ata_pubkey, expected_ata); +let expected_ata = derive_associated_token_account(&owner, &mint); +assert_eq!(ata_pubkey, expected_ata);As per coding guidelines: "Verify that all function signatures, struct definitions, and behavior described in the documentation accurately match the actual implementation."
sdk-libs/macros/src/light_pdas/accounts/light_account.rs (1)
1003-1008:⚠️ Potential issue | 🟡 MinorStale doc comment —
associated_token::bumpmapping no longer exists.Line 1008 still documents
associated_token::bump->bumpas a valid mapping, but the implementation now explicitly rejects it with an error. This will confuse anyone reading the function docs.📝 Proposed fix
/// Build an AtaField from parsed namespaced key-value pairs. /// /// Mapping from new syntax to internal fields: /// - `associated_token::authority` -> `owner` (renamed to match Anchor's ATA naming) /// - `associated_token::mint` -> `mint` -/// - `associated_token::bump` -> `bump` +/// +/// Note: `associated_token::bump` is rejected — the bump is derived on-chain.sdk-libs/token-sdk/src/instruction/create_associated_token_account.rs (4)
53-70: 🧹 Nitpick | 🔵 TrivialThe 5-element tuple for compressible config is hard to read at a glance.
Line 106 defines
compressible_config: Option<(u8, Option<u32>, Pubkey, Pubkey, TokenDataVersion)>— a 5-element tuple with positional semantics. Inside the function, elements are destructured with_placeholders (lines 117, 155), which makes it easy to mix up positions.Consider using a small struct (or reusing
CreateCompressibleAssociatedTokenAccountInputsfields) instead of a bare tuple to improve clarity in this internal API.
134-163:⚠️ Potential issue | 🔴 CriticalAdd missing
ownerandmintaccounts to the accounts list.The accounts list (lines 147–163) must include
ownerandmintas the first two accounts to match the on-chain handler's expected account order:0. owner 1. mint 2. fee_payer 3. associated_token_account 4. system_program 5. [optional] compressible_config 6. [optional] rent_sponsorCurrently, the list starts with
payer, causing the on-chainAccountIteratorto read the wrong account at position 0 when it expectsowner. The on-chain handler needs bothownerandmintto validate the ATA derivation viavalidate_ata_derivation(associated_token_account, owner_bytes, mint_bytes).
12-14: 🧹 Nitpick | 🔵 TrivialDocument why 1-byte discriminators (100, 102) are used instead of Anchor's standard trait.
This Anchor program intentionally uses 1-byte discriminators for SPL Token compatibility (as noted in
programs/compressed-token/program/src/lib.rs). However, the SDK code here treats these hardcoded values as unexplained constants. Add a comment explaining that these match the program'sInstructionTypeenum values for SPL compatibility:/// Discriminators for create ATA instructions /// Note: 1-byte discriminators used for SPL Token instruction compatibility (see InstructionType enum in programs/compressed-token) const CREATE_ATA_DISCRIMINATOR: u8 = 100; const CREATE_ATA_IDEMPOTENT_DISCRIMINATOR: u8 = 102;
186-260:⚠️ Potential issue | 🔴 CriticalThe instruction's accounts list is missing and misaligned with the on-chain handler's expected account order.
The on-chain handler (programs/compressed-token/program/src/ctoken/create_ata.rs, lines 37–47) explicitly expects:
owner(non-mut, non-signer)mint(non-mut, non-signer)fee_payer(signer, mut)associated_token_account(mut)system_program
6–7. Optional compressible accountsHowever,
create_ata_instruction_unifiedbuilds accounts as:
payer(fee_payer) ← should beownerata_pubkey← should bemintsystem_program← should be at index 4
4–5. Optional compressible accounts (wrong position)The function receives
ownerandmintas parameters but discards them instead of adding them to the accounts list. This mismatch explains the asymmetry in the CPI wrappers: the non-idempotent variant attempts to passauthorityas an extra account hoping the handler reads fromremaining_accounts, while the idempotent variant doesn't pass it at all. Neither approach works because the handler explicitly reads from fixed account indices, notremaining_accounts.The owner and mint must be added to the accounts list in the correct positions before payer and ata_pubkey.
🤖 Fix all issues with AI agents
In `@programs/compressed-token/program/src/shared/create_pda_account.rs`:
- Around line 104-105: In create_pda_account.rs, update the PDA mismatch error
so that when the derived PDA doesn't equal the provided account key (the check
that currently returns ProgramError::InvalidAccountData), it returns
ProgramError::InvalidSeeds instead; locate the equality check around account_key
vs expected_pubkey in the function that derives/validates the PDA (e.g.,
create_pda_account or the PDA validation block) and replace the returned error
variant accordingly so callers receive InvalidSeeds for seed-derivation
failures.
In `@sdk-libs/macros/src/light_pdas/accounts/light_account.rs`:
- Around line 1624-1635: The test currently verifies that
parse_light_account_attr rejects the shorthand associated_token::bump but
doesn't assert on the error text; update the
test_parse_associated_token_bump_rejected case to capture the Err from
parse_light_account_attr and assert that the error message contains a clear
identifier like "associated_token::bump" (or the exact error string emitted by
parse_light_account_attr) so regressions in the error text will be caught;
reference parse_light_account_attr and the associated_token::bump attribute in
your assertion.
| if account_key != &expected_pubkey { | ||
| return Err(ProgramError::InvalidAccountData); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual code at the specified lines
cat -n programs/compressed-token/program/src/shared/create_pda_account.rs | sed -n '100,110p'Repository: Lightprotocol/light-protocol
Length of output: 404
🏁 Script executed:
# Search for verify_pda function definition and usage
rg -n 'fn verify_pda' --type=rust -A 10 programs/compressed-token/Repository: Lightprotocol/light-protocol
Length of output: 1207
🏁 Script executed:
# Check all callers of verify_pda to understand error handling
rg -n 'verify_pda' --type=rust -B 2 -A 5 programs/compressed-token/Repository: Lightprotocol/light-protocol
Length of output: 5592
🏁 Script executed:
# Look at the error definitions to understand available error types
rg -n 'enum.*Error|InvalidSeeds|InvalidAccountData' --type=rust programs/compressed-token/program/src/error.rsRepository: Lightprotocol/light-protocol
Length of output: 158
🏁 Script executed:
# Find error definitions in the compressed-token program
fd -t f 'error' programs/compressed-token/program/src/ | head -20Repository: Lightprotocol/light-protocol
Length of output: 141
🏁 Script executed:
# Search for error enum definitions
rg -n 'enum.*Error|InvalidSeeds|InvalidAccountData' --type=rust programs/compressed-token/program/src/Repository: Lightprotocol/light-protocol
Length of output: 3915
🏁 Script executed:
# Check the structure of the program directory
ls -la programs/compressed-token/program/src/Repository: Lightprotocol/light-protocol
Length of output: 613
Use InvalidSeeds for PDA mismatch validation.
The function derives a PDA and validates it matches the provided account key. When they don't match, return ProgramError::InvalidSeeds instead of InvalidAccountData—it more precisely describes the failure (the seeds don't derive to the expected account). The codebase already uses InvalidSeeds in similar validation contexts (e.g., withdraw_funding_pool.rs), and neither caller pattern-matches on the error type, so this change is safe.
Suggested diff
if account_key != &expected_pubkey {
- return Err(ProgramError::InvalidAccountData);
+ return Err(ProgramError::InvalidSeeds);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if account_key != &expected_pubkey { | |
| return Err(ProgramError::InvalidAccountData); | |
| if account_key != &expected_pubkey { | |
| return Err(ProgramError::InvalidSeeds); | |
| } |
🤖 Prompt for AI Agents
In `@programs/compressed-token/program/src/shared/create_pda_account.rs` around
lines 104 - 105, In create_pda_account.rs, update the PDA mismatch error so that
when the derived PDA doesn't equal the provided account key (the check that
currently returns ProgramError::InvalidAccountData), it returns
ProgramError::InvalidSeeds instead; locate the equality check around account_key
vs expected_pubkey in the function that derives/validates the PDA (e.g.,
create_pda_account or the PDA validation block) and replace the returned error
variant accordingly so callers receive InvalidSeeds for seed-derivation
failures.
ananas-block
changed the title
Audit issue #15 (HIGH): verify_pda used derive_address which accepts any bump seed, allowing non-canonical bumps for ATAs. Switch to find_program_address to derive the canonical bump and reject any non-canonical bump with InvalidSeeds error.
…on-chain Remove client-provided bump from CreateAssociatedTokenAccountInstructionData and all SDK/test callers. The on-chain program now derives the canonical bump via find_program_address, preventing non-canonical bump attacks (audit #15). - Remove bump field from instruction data structs - Update verify_pda to derive canonical bump and return it - Update validate_ata_derivation and decompress_mint callers - Remove _with_bump SDK variants and ATA2 dead code - Remove associated_token::bump from macro attribute support - Update derive_associated_token_account to return Pubkey only - Update all 100+ call sites across SDK, tests, and TypeScript
With canonical bumps, the program derives the bump internally so providing a wrong bump is no longer possible. Replace with a test that passes a wrong ATA address to verify PDA validation.
ananas-block
force-pushed
the
fix/audit-issue-15-canonical-bump
branch
from
947d024 to
3c24b30
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sdk-libs/macros/docs/accounts/associated_token.md (1)
251-254:⚠️ Potential issue | 🟡 MinorStale derivation example — won't compile with the new API.
derive_associated_token_accountnow returns aPubkeydirectly (not a tuple). This code example still destructures it aslet (expected_ata, _) = ..., which would be a compile error.📝 Proposed fix
-let (expected_ata, _) = derive_associated_token_account(&owner, &mint); +let expected_ata = derive_associated_token_account(&owner, &mint); assert_eq!(ata_pubkey, expected_ata);sdk-libs/macros/src/light_pdas/accounts/light_account.rs (1)
1003-1008:⚠️ Potential issue | 🟡 MinorStale doc comment —
associated_token::bumpmapping still listed.The function doc on Line 1008 still documents
associated_token::bump -> bumpas a valid mapping, but the body now explicitly rejects it. Keeping this line will confuse future readers into thinking bump is supported.🧹 Proposed fix
/// Build an AtaField from parsed namespaced key-value pairs. /// /// Mapping from new syntax to internal fields: /// - `associated_token::authority` -> `owner` (renamed to match Anchor's ATA naming) /// - `associated_token::mint` -> `mint` -/// - `associated_token::bump` -> `bump`
2 participants
Summary
derive_addresswithfind_program_addressinverify_pdato ensure only the canonical bump seed is accepted, preventing non-canonical bump attacks.Summary by CodeRabbit
Refactor
Documentation
Tests