fix(nft): portfolio provenance + RepairExtraMetas recovery, with indentation/seed/tests fixed (supersedes #171) - #172
Merged
Merged
Conversation
…171 Follow-up to the provenance/repair commit. The three provenance checks are sound and break no live NFT — the wrapper's only production initializer sets portfolio_account_id = portfolio_ai.key, MintPositionNft at the deployed commit f18da24 already enforces the same equality, and all live devnet portfolios carry bytes[48..80] == their own address. The problems were in how it shipped. 1. Indentation. The new ownership gate and the entire resize/recovery block were written at column 0 inside the function body, so a security-critical account-ownership check read as though it were top-level. Re-indented to the surrounding block. (Only these regions — `main` does not pass `cargo fmt --check` and no workflow enforces it, so a repo-wide reformat would be unrelated churn.) 2. Hardcoded seed literal. The recovery path derived its signer seeds from b"extra-account-metas" while the mint path (:631) uses EXTRA_METAS_SEED, which is already imported at :28. Now both use the constant, so a seed change cannot desynchronise them. 3. Zero tests. The PR changed four on-chain behaviours and shipped no tests; CI exercised none of the new lines. Added the two arms of verify_portfolio_account_id plus a zeroed-provenance case (an all-zero header must not pass by accident). Negative control: stubbing the comparison to `false` fails two of the three. 4. Justified the System-owned recovery branch rather than leaving it unexplained. It is the never-created case: a nonexistent account is presented as System-owned with no data, so the old `owner != program_id -> reject` gate meant RepairExtraMetas could only fix a wrong-SIZED metas account, never a MISSING one — the case that actually bricks transfers. It is not the post-burn state: close_extra_metas (:747) zeroes lamports and data but never assigns back to System, and a burn drains nft_pda too, so a burned NFT cannot reach the checks. Safety rests on the address being pinned to our derivation, the account being required empty, and nft_pda being independently validated. SEVERITY CORRECTION for the linked issues: #169/#170 are graded "High", but the precondition — a wrapper-owned, decodable portfolio whose provenance_header.portfolio_account_id differs from its own address — cannot be produced by percolator-prog. These checks are defence-in-depth against a future initializer, not fixes for a reachable exploit, and the issues should be re-graded before anyone merges on that basis. cargo build clean; 47 lib tests pass (44 before, +3 new). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
Second-pass review findings. Two are substantive. 1. UNDISCLOSED DESTRUCTIVE CAPABILITY. The resize guard was changed from `data.len() < EXTRA_METAS_ACCOUNT_LEN` (main) to `!=`, which also SHRANK an oversized program-owned extra_metas back to 261 bytes — through a PERMISSIONLESS instruction, and with no mention in the PR. Reverted to grow-only. The recovery case is unaffected: a never-created account has len 0, which is already `<` the target, so `<` covers it. This also removes the now -redundant needs_grow branch. 2. FALSE PREMISE COMMITTED TO ON-CHAIN SOURCE. The comment asserted "this is NOT the post-burn state: close_extra_metas zeroes lamports and data but never assigns the account back to the System Program". The runtime reaps zero-lamport accounts at end of transaction, so on any LATER transaction a burned NFT's extra_metas DOES load as System-owned and empty and DOES reach this branch. The branch is still safe — but via the next gate, not this one: the same burn drains nft_pda, so `nft_pda.owner != program_id` rejects before anything is written. Corrected, with the old claim flagged as wrong so a future reader does not re-derive it. 3. INDENTATION FIX WAS INCOMPLETE — the irony of a commit titled "repair the indentation". The PR's own added line at transfer_hook.rs:474 sat at 4-space inside an 8-space block, with a stray blank line splitting it from the statement it depends on. Both fixed. cargo build clean; 47 lib tests pass, full suite green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #171 by @Bayyan16. Their commit is preserved as-is; my corrections are a follow-up commit on top.
The three provenance checks are sound and break no live NFT — the wrapper's only production initializer sets
portfolio_account_id = portfolio_ai.key(percolator-prog v16_program.rs:6558-6565),MintPositionNftat the deployed commitf18da24already enforces the same equality (processor.rs:304-307), and all live devnet portfolios carrybytes[48..80] == their own address. The problems were in how it shipped.1. Indentation. The new ownership gate and the entire resize/recovery block were written at column 0 inside the function body, so a security-critical account-ownership check read as though it were top-level. Re-indented to the surrounding block. (Only these regions —
maindoes not passcargo fmt --checkand no workflow enforces it, so a repo-wide reformat would be unrelated churn.)2. Hardcoded seed literal. The recovery path derived its signer seeds from
b"extra-account-metas"while the mint path (:631) usesEXTRA_METAS_SEED, already imported at:28. Both now use the constant.3. Zero tests. The PR changed four on-chain behaviours and shipped no tests; CI exercised none of the new lines. Added both arms of
verify_portfolio_account_idplus a zeroed-provenance case. Negative control: stubbing the comparison tofalsefails two of the three.4. Justified the System-owned recovery branch rather than leaving it unexplained. It is the never-created case: a nonexistent account is presented as System-owned with no data, so the old
owner != program_id -> rejectgate meantRepairExtraMetascould only fix a wrong-sized metas account, never a missing one — the case that actually bricks transfers. It is not the post-burn state:close_extra_metas(:747) zeroes lamports and data but never assigns back to System, and a burn drainsnft_pdatoo, so a burned NFT cannot reach the checks. Safety rests on the address being pinned to our derivation, the account being required empty, andnft_pdabeing independently validated.SEVERITY CORRECTION for the linked issues: #169/#170 are graded "High", but the precondition — a wrapper-owned, decodable portfolio whose
provenance_header.portfolio_account_iddiffers from its own address — cannot be produced by percolator-prog. These checks are defence-in-depth against a future initializer, not fixes for a reachable exploit, and the issues should be re-graded before anyone merges on that basis.cargo buildclean; 47 lib tests pass (44 before, +3 new).