Skip to content

Fix/v2 metadata deserialize cursor #4839

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.

Sign up for GitHub

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

Open
0x4ka5h wants to merge 3 commits into
base: anchor-next
Choose a base branch
from
+239 −17
Open

Fix/v2 metadata deserialize cursor #4839

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b32f550
fix(spl-v2): advance metadata deserialize cursors
Jul 23, 2026
e6cad0a
fix(tests-v2): add metadata deserialize cursor regression
Jul 23, 2026
e9edc0b
fix(tests-v2): prove metadata v1.2 field compatibility
Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion spl-v2/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ pinocchio = { workspace = true }
pinocchio-token = "0.6"
pinocchio-token-2022 = "0.3"
bytemuck = { version = "1", features = ["derive"] }
borsh = "1.6.1"
solana-address = { version = "2.0", features = ["bytemuck"] }
solana-instruction = "3.0"
solana-program-error = "3.0"
Expand All @@ -33,7 +34,6 @@ mpl-token-metadata = { version = "=5.1.2-alpha.2", optional = true }

[dev-dependencies]
anchor-lang-v2 = { path = "../lang-v2", version = "2.0.0", features = ["testing"] }
borsh = "1.6.1"
# Cross-checks `Mint` / `TokenAccount` layout against the canonical
# `spl-token-interface` types (see `tests/miri_spl_pod.rs`). Dev-only.
solana-program-pack = "3"
Expand Down
126 changes: 115 additions & 11 deletions spl-v2/src/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use {
require, AccountDeserialize, AnchorAccount, CpiContext, CpiHandle, CpiHandleMut, Id,
IdlAccountType, Result, ToCpiAccounts,
},
borsh::BorshDeserialize,
core::ops::Deref,
pinocchio::account::AccountView,
solana_address::Address,
Expand Down Expand Up @@ -715,9 +716,74 @@ pub struct MetadataAccount {

impl MetadataAccount {
#[inline]
fn parse(data: &[u8]) -> Result<mpl_token_metadata::accounts::Metadata> {
mpl_token_metadata::accounts::Metadata::safe_deserialize(data)
.map_err(|_| ProgramError::InvalidAccountData)
fn parse(buf: &mut &[u8]) -> Result<mpl_token_metadata::accounts::Metadata> {
use mpl_token_metadata::types::{
Collection, CollectionDetails, Data, Key, ProgrammableConfig, TokenStandard, Uses,
};

if buf.is_empty() || buf[0] != Key::MetadataV1 as u8 {
return Err(ProgramError::InvalidAccountData);
}

let key: Key =

@swaroop-osec swaroop-osec Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit — document the on-chain order in here while deserializing.

BorshDeserialize::deserialize(buf).map_err(|_| ProgramError::InvalidAccountData)?;
let update_authority: Pubkey =
BorshDeserialize::deserialize(buf).map_err(|_| ProgramError::InvalidAccountData)?;
let mint: Pubkey =
BorshDeserialize::deserialize(buf).map_err(|_| ProgramError::InvalidAccountData)?;
let data: Data =
BorshDeserialize::deserialize(buf).map_err(|_| ProgramError::InvalidAccountData)?;
let primary_sale_happened: bool =
BorshDeserialize::deserialize(buf).map_err(|_| ProgramError::InvalidAccountData)?;
let is_mutable: bool =
BorshDeserialize::deserialize(buf).map_err(|_| ProgramError::InvalidAccountData)?;
let edition_nonce: Option<u8> =
BorshDeserialize::deserialize(buf).map_err(|_| ProgramError::InvalidAccountData)?;

let token_standard_res: core::result::Result<Option<TokenStandard>, borsh::io::Error> =
BorshDeserialize::deserialize(buf);
let collection_res: core::result::Result<Option<Collection>, borsh::io::Error> =
BorshDeserialize::deserialize(buf);
let uses_res: core::result::Result<Option<Uses>, borsh::io::Error> =
BorshDeserialize::deserialize(buf);
let collection_details_res: core::result::Result<
Option<CollectionDetails>,
borsh::io::Error,
> = BorshDeserialize::deserialize(buf);
let programmable_config_res: core::result::Result<
Option<ProgrammableConfig>,
borsh::io::Error,
> = BorshDeserialize::deserialize(buf);

let (token_standard, collection, uses) =
match (token_standard_res, collection_res, uses_res) {
(Ok(token_standard), Ok(collection), Ok(uses)) => {
(token_standard, collection, uses)
}
_ => (None, None, None),
};
Comment on lines +758 to +764

@hacktron-app hacktron-app Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HIGH Loss of Token Standard and Collection Fields in Metadata Deserialization for Older Accounts

The custom MetadataAccount::parse implementation introduced in this PR manually deserializes the optional fields of a Metaplex Metadata account. However, it groups the deserialization results of token_standard, collection, and uses into a single match statement:

let (token_standard, collection, uses) =
    match (token_standard_res, collection_res, uses_res) {
        (Ok(token_standard), Ok(collection), Ok(uses)) => {
            (token_standard, collection, uses)
        }
        _ => (None, None, None),
    };

If any of these three fields fails to deserialize (for example, if an older metadata account is shorter and does not contain the uses field, causing uses_res to return an Err due to EOF), the entire match falls back to the wildcard pattern _ => (None, None, None).

This causes successfully deserialized token_standard and collection fields to be silently discarded and set to None for any older metadata accounts.

Attack Scenario & Impact

  1. Royalty/Ruleset Bypass: If a Programmable NFT (pNFT) is stored in an older account format that lacks the uses field, its token_standard will be parsed as None instead of Some(TokenStandard::ProgrammableNonFungible). Downstream programs (such as marketplaces) that rely on this helper to detect pNFTs will treat it as a legacy NFT, allowing users to bypass royalty enforcement and ruleset checks.
  2. Collection Verification Failure: Any downstream program checking metadata.collection to verify that an NFT belongs to a specific collection will see None and reject the NFT, causing a Denial of Service (DoS) for older NFTs.
Steps to Reproduce 
An older Metaplex Metadata account with a valid `token_standard` and `collection` but no `uses` field (EOF) is passed to `MetadataAccount::try_deserialize`. The function successfully parses `token_standard` and `collection`, but because `uses_res` is `Err(EOF)`, the match discards both successfully parsed fields and returns them as `None`.
Fix with AI

Open in Cursor Open in Claude

A security vulnerability was found by Hacktron.

File: spl-v2/src/metadata.rs
Lines: 758-764
Severity: high

Vulnerability: Loss of Token Standard and Collection Fields in Metadata Deserialization for Older Accounts

Description:
The custom `MetadataAccount::parse` implementation introduced in this PR manually deserializes the optional fields of a Metaplex `Metadata` account. However, it groups the deserialization results of `token_standard`, `collection`, and `uses` into a single `match` statement:

```rust
let (token_standard, collection, uses) =
    match (token_standard_res, collection_res, uses_res) {
        (Ok(token_standard), Ok(collection), Ok(uses)) => {
            (token_standard, collection, uses)
        }
        _ => (None, None, None),
    };
```

If any of these three fields fails to deserialize (for example, if an older metadata account is shorter and does not contain the `uses` field, causing `uses_res` to return an `Err` due to EOF), the entire match falls back to the wildcard pattern `_ => (None, None, None)`.

This causes successfully deserialized `token_standard` and `collection` fields to be silently discarded and set to `None` for any older metadata accounts.

### Attack Scenario & Impact
1. **Royalty/Ruleset Bypass**: If a Programmable NFT (pNFT) is stored in an older account format that lacks the `uses` field, its `token_standard` will be parsed as `None` instead of `Some(TokenStandard::ProgrammableNonFungible)`. Downstream programs (such as marketplaces) that rely on this helper to detect pNFTs will treat it as a legacy NFT, allowing users to bypass royalty enforcement and ruleset checks.
2. **Collection Verification Failure**: Any downstream program checking `metadata.collection` to verify that an NFT belongs to a specific collection will see `None` and reject the NFT, causing a Denial of Service (DoS) for older NFTs.

Proof of Concept:
An older Metaplex Metadata account with a valid `token_standard` and `collection` but no `uses` field (EOF) is passed to `MetadataAccount::try_deserialize`. The function successfully parses `token_standard` and `collection`, but because `uses_res` is `Err(EOF)`, the match discards both successfully parsed fields and returns them as `None`.

Affected Code:
        let (token_standard, collection, uses) =
            match (token_standard_res, collection_res, uses_res) {
                (Ok(token_standard), Ok(collection), Ok(uses)) => {
                    (token_standard, collection, uses)
                }
                _ => (None, None, None),
            };

Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.

Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.

Triage: Reply !fp <reason> (false positive), !valid (confirmed), !accepted_risk <reason>, or !fixed (resolved). Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.

View finding in Hacktron

@0x4ka5h 0x4ka5h Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!fp The grouped fallback in MetadataAccount::parse only drops token_standard, collection, and uses together when the trailing v1.2 fields are partially truncated or otherwise malformed


let collection_details = collection_details_res.unwrap_or(None);
let programmable_config = programmable_config_res.unwrap_or(None);

Ok(mpl_token_metadata::accounts::Metadata {
key,
update_authority,
mint,
name: data.name,
symbol: data.symbol,
uri: data.uri,
seller_fee_basis_points: data.seller_fee_basis_points,
creators: data.creators,
primary_sale_happened,
is_mutable,
edition_nonce,
token_standard,
collection,
uses,
collection_details,
programmable_config,
})
}
}

Expand All @@ -738,7 +804,7 @@ impl AnchorAccount for MetadataAccount {
fn load(view: AccountView) -> Result<Self> {
require!(view.owned_by(&ID), ProgramError::IllegalOwner);
let data_ref = view.try_borrow()?;
let data = Self::parse(&data_ref)?;
let data = Self::parse(&mut &data_ref[..])?;
drop(data_ref);
Ok(Self {
view: Some(view),
Expand Down Expand Up @@ -771,8 +837,12 @@ pub struct MasterEditionAccount {

impl MasterEditionAccount {
#[inline]
fn parse(data: &[u8]) -> Result<mpl_token_metadata::accounts::MasterEdition> {
mpl_token_metadata::accounts::MasterEdition::safe_deserialize(data)
fn parse(buf: &mut &[u8]) -> Result<mpl_token_metadata::accounts::MasterEdition> {
if buf.is_empty() || buf[0] != mpl_token_metadata::types::Key::MasterEditionV2 as u8 {
return Err(ProgramError::InvalidAccountData);
}

mpl_token_metadata::accounts::MasterEdition::deserialize(buf)
.map_err(|_| ProgramError::InvalidAccountData)
}
}
Expand All @@ -794,7 +864,7 @@ impl AnchorAccount for MasterEditionAccount {
fn load(view: AccountView) -> Result<Self> {
require!(view.owned_by(&ID), ProgramError::IllegalOwner);
let data_ref = view.try_borrow()?;
let data = Self::parse(&data_ref)?;
let data = Self::parse(&mut &data_ref[..])?;
drop(data_ref);
Ok(Self {
view: Some(view),
Expand Down Expand Up @@ -829,9 +899,43 @@ impl TokenRecordAccount {
pub const LEN: usize = mpl_token_metadata::accounts::TokenRecord::LEN;

#[inline]
fn parse(data: &[u8]) -> Result<mpl_token_metadata::accounts::TokenRecord> {
mpl_token_metadata::accounts::TokenRecord::safe_deserialize(data)
.map_err(|_| ProgramError::InvalidAccountData)
fn parse(buf: &mut &[u8]) -> Result<mpl_token_metadata::accounts::TokenRecord> {
const LOCKED_TRANSFER_SIZE: i64 = 33;

let length = Self::LEN as i64 - buf.len() as i64;
if !(length == 0 || length == LOCKED_TRANSFER_SIZE)
|| buf.first().copied() != Some(mpl_token_metadata::types::Key::TokenRecord as u8)
{
return Err(ProgramError::InvalidAccountData);
}

let key =
BorshDeserialize::deserialize(buf).map_err(|_| ProgramError::InvalidAccountData)?;
let bump =
BorshDeserialize::deserialize(buf).map_err(|_| ProgramError::InvalidAccountData)?;
let state =
BorshDeserialize::deserialize(buf).map_err(|_| ProgramError::InvalidAccountData)?;
let rule_set_revision =
BorshDeserialize::deserialize(buf).map_err(|_| ProgramError::InvalidAccountData)?;
let delegate =
BorshDeserialize::deserialize(buf).map_err(|_| ProgramError::InvalidAccountData)?;
let delegate_role =
BorshDeserialize::deserialize(buf).map_err(|_| ProgramError::InvalidAccountData)?;
let locked_transfer = if length == 0 {
BorshDeserialize::deserialize(buf).map_err(|_| ProgramError::InvalidAccountData)?
} else {
None
};

Ok(mpl_token_metadata::accounts::TokenRecord {
key,
bump,
state,
rule_set_revision,
delegate,
delegate_role,
locked_transfer,
})
}
}

Expand All @@ -852,7 +956,7 @@ impl AnchorAccount for TokenRecordAccount {
fn load(view: AccountView) -> Result<Self> {
require!(view.owned_by(&ID), ProgramError::IllegalOwner);
let data_ref = view.try_borrow()?;
let data = Self::parse(&data_ref)?;
let data = Self::parse(&mut &data_ref[..])?;
drop(data_ref);
Ok(Self {
view: Some(view),
Expand Down
57 changes: 53 additions & 4 deletions spl-v2/tests/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@

use {
anchor_lang_v2::{testing::AccountBuffer, AccountDeserialize, AnchorAccount},
anchor_spl_v2::metadata::{self, MetadataAccount},
anchor_spl_v2::metadata::{self, MasterEditionAccount, MetadataAccount, TokenRecordAccount},
borsh::to_vec,
solana_address::Address,
solana_program_error::ProgramError,
solana_pubkey::Pubkey,
};
Expand Down Expand Up @@ -35,6 +34,26 @@ fn sample_metadata() -> mpl_token_metadata::accounts::Metadata {
}
}

fn sample_master_edition() -> mpl_token_metadata::accounts::MasterEdition {
mpl_token_metadata::accounts::MasterEdition {
key: mpl_token_metadata::types::Key::MasterEditionV2,
supply: 42,
max_supply: Some(100),
}
}

fn sample_token_record() -> mpl_token_metadata::accounts::TokenRecord {
mpl_token_metadata::accounts::TokenRecord {
key: mpl_token_metadata::types::Key::TokenRecord,
bump: 7,
state: mpl_token_metadata::types::TokenState::Unlocked,
rule_set_revision: Some(3),
delegate: Some(Pubkey::from([8u8; 32])),
delegate_role: Some(mpl_token_metadata::types::TokenDelegateRole::Sale),
locked_transfer: Some(Pubkey::from([9u8; 32])),
}
}

#[test]
fn fixture_is_real_metadata_program_elf() {
let fixture = include_bytes!("fixtures/metaplex_token_metadata.so");
Expand All @@ -57,6 +76,36 @@ fn metadata_account_deserializes_raw_metaplex_bytes() {
);
}

#[test]
fn metadata_account_deserialize_advances_cursor() {
let data = to_vec(&sample_metadata()).unwrap();
let mut cursor = data.as_slice();
let account = MetadataAccount::try_deserialize(&mut cursor).unwrap();

assert_eq!(account.key, mpl_token_metadata::types::Key::MetadataV1);
assert!(cursor.is_empty());
}

#[test]
fn master_edition_account_deserialize_advances_cursor() {
let data = to_vec(&sample_master_edition()).unwrap();
let mut cursor = data.as_slice();
let account = MasterEditionAccount::try_deserialize(&mut cursor).unwrap();

assert_eq!(account.key, mpl_token_metadata::types::Key::MasterEditionV2);
assert!(cursor.is_empty());
}

#[test]
fn token_record_account_deserialize_advances_cursor() {
let data = to_vec(&sample_token_record()).unwrap();
let mut cursor = data.as_slice();
let account = TokenRecordAccount::try_deserialize(&mut cursor).unwrap();

assert_eq!(account.key, mpl_token_metadata::types::Key::TokenRecord);
assert!(cursor.is_empty());
}

#[test]
fn metadata_account_load_validates_owner_and_raw_data() {
let expected = sample_metadata();
Expand All @@ -72,7 +121,7 @@ fn metadata_account_load_validates_owner_and_raw_data() {
);
account.write_data(&data);

let loaded = MetadataAccount::load(unsafe { account.view() }, &Address::default()).unwrap();
let loaded = MetadataAccount::load(unsafe { account.view() }).unwrap();
assert_eq!(loaded.update_authority, expected.update_authority);
assert_eq!(loaded.seller_fee_basis_points, 250);
}
Expand All @@ -84,7 +133,7 @@ fn metadata_account_rejects_wrong_owner() {
account.init([9u8; 32], [3u8; 32], data.len(), false, false, false);
account.write_data(&data);

let err = MetadataAccount::load(unsafe { account.view() }, &Address::default()).unwrap_err();
let err = MetadataAccount::load(unsafe { account.view() }).unwrap_err();
assert_eq!(err, ProgramError::IllegalOwner);
}

Expand Down
Loading