Fix/v2 metadata deserialize cursor#4839
Conversation
|
Someone is attempting to deploy a commit to the OtterSec Team on Vercel. A member of the Team first needs to authorize it. |
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
| 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), | ||
| }; |
There was a problem hiding this comment.
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
- Royalty/Ruleset Bypass: If a Programmable NFT (pNFT) is stored in an older account format that lacks the
usesfield, itstoken_standardwill be parsed asNoneinstead ofSome(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. - Collection Verification Failure: Any downstream program checking
metadata.collectionto verify that an NFT belongs to a specific collection will seeNoneand 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
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.
There was a problem hiding this comment.
!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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## anchor-next #4839 +/- ##
==============================================
Coverage ? 63.61%
==============================================
Files ? 126
Lines ? 13666
Branches ? 0
==============================================
Hits ? 8694
Misses ? 4972
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| return Err(ProgramError::InvalidAccountData); | ||
| } | ||
|
|
||
| let key: Key = |
There was a problem hiding this comment.
Nit — document the on-chain order in here while deserializing.
3 participants
finding
The metadata account wrappers in
spl-v2deserialized from a copied slice and did not advance the caller's input cursor. That contradicts theAccountDeserializecontract and can misalign chained deserialization.Fix
Implemented cursor-advancing deserialization for
MetadataAccount,MasterEditionAccount, andTokenRecordAccount. This restores the expectedAccountDeserializesemantics while preserving the existing type checks.