Fix RepairExtraMetas recovery and enforce portfolio provenance checks - #171
Fix RepairExtraMetas recovery and enforce portfolio provenance checks#171Bayyan16 wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds ChangesPortfolio Provenance Validation + Repair Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/processor.rs (1)
1485-1519: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNormalize oversized
extra_metasaccounts too.Line 1486 only repairs accounts shorter than
EXTRA_METAS_ACCOUNT_LEN; a program-owned oversized account keeps stale trailing bytes after the deterministic prefix is rewritten. Resize or rejectdata.len() > EXTRA_METAS_ACCOUNT_LENso repair produces the canonical buffer.Proposed fix
- if data.len() < EXTRA_METAS_ACCOUNT_LEN { + if data.len() != EXTRA_METAS_ACCOUNT_LEN { + let needs_grow = data.len() < EXTRA_METAS_ACCOUNT_LEN; drop(data); - let rent = Rent::get()?; - let needed = rent.minimum_balance(EXTRA_METAS_ACCOUNT_LEN); - let current = extra_metas.lamports(); - if needed > current { - let top_up = needed - current; - invoke( - &system_instruction::transfer(payer.key, extra_metas.key, top_up), - &[payer.clone(), extra_metas.clone(), system_program.clone()], - )?; + if needs_grow { + let rent = Rent::get()?; + let needed = rent.minimum_balance(EXTRA_METAS_ACCOUNT_LEN); + let current = extra_metas.lamports(); + if needed > current { + let top_up = needed - current; + invoke( + &system_instruction::transfer(payer.key, extra_metas.key, top_up), + &[payer.clone(), extra_metas.clone(), system_program.clone()], + )?; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/processor.rs` around lines 1485 - 1519, The repair path in processor::process_extra_metas only handles undersized accounts, but oversized program-owned extra_metas can keep stale trailing bytes after rewriting the deterministic prefix. Update the normalization logic in the extra_metas handling block to either resize oversized data down to EXTRA_METAS_ACCOUNT_LEN or reject it before continuing, and make sure the final buffer is canonical after the existing allocate/assign flow and extra_metas.resize call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/processor.rs`:
- Around line 1485-1519: The repair path in processor::process_extra_metas only
handles undersized accounts, but oversized program-owned extra_metas can keep
stale trailing bytes after rewriting the deterministic prefix. Update the
normalization logic in the extra_metas handling block to either resize oversized
data down to EXTRA_METAS_ACCOUNT_LEN or reject it before continuing, and make
sure the final buffer is canonical after the existing allocate/assign flow and
extra_metas.resize call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5898c299-07dc-4e2a-b964-22806f3e85ca
📒 Files selected for processing (4)
src/cpi_v16.rssrc/processor.rssrc/transfer_hook.rssrc/valuation.rs
33d1eda to
a705212
Compare
|
Thanks @Bayyan16 — this is landing as #172, which keeps your commit with your authorship and adds the corrections as a follow-up commit on top. Why a new PR rather than a push here: your branch was cut from an older base, so rebasing it cleanly meant cherry-picking your commit onto the current trunk on a branch in this repo. I don't force-push contributor forks. What changed on top of your work: The provenance checks are sound and break no live NFT (I verified against the deployed commit and live devnet portfolios). What needed fixing: the new ownership gate and the whole resize/recovery block were indented at column 0 inside the function body; the recovery path hardcoded Full detail and the verification are in #172. Closing this in favour of it — your change is in there, not discarded. |
…ntation/seed/tests fixed (supersedes #171) (#172) * fix: harden portfolio provenance and repair extra metas recovery * fix(nft): repair the indentation, seed constant, and missing tests in #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> * fix(nft): revert the permissionless shrink, correct the post-burn claim 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> --------- Co-authored-by: Bayyan16 <mpllanggeng16@gmail.com> Co-authored-by: dcccrypto <dcccrypto@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
This PR fixes four issues across
RepairExtraMetas,GetPositionValue,SettleFunding, andTransferHook.Fixes:
The changes focus on two security/integrity areas:
ExtraAccountMetaListPDA accounts inRepairExtraMetas;Changes
1. RepairExtraMetas recovery
RepairExtraMetaspreviously rejected a canonicalExtraAccountMetaListPDA when the account was System-owned / prefunded before the instruction could recover and rewrite it.This PR updates the recovery flow so
RepairExtraMetascan recover only the canonical empty System-owned PDA.The updated flow now:
extra_metasaccount is the canonical PDA for the NFT mint;ExtraAccountMetaList.Non-canonical accounts, third-party-owned accounts, and non-empty System-owned accounts remain rejected. This keeps the recovery path permissionless but still fail-closed for unsafe account states.
2. Shared portfolio provenance binding helper
This PR adds a shared helper:
The helper verifies that decoded portfolio data is actually bound to the portfolio account passed into the current instruction:
This check is intentionally applied at each portfolio-consuming handler. Portfolio decoding validates the internal v16 layout, while the handler must also verify that the decoded provenance matches the actual account it is about to trust.
3. GetPositionValue provenance validation
GetPositionValuenow rejects decoded portfolio data when the internalprovenance_header.portfolio_account_iddoes not match the passedportfolio.key.This prevents the valuation path from emitting trusted
POSITION_VALUE_V16logs for provenance-mismatched portfolio data.Affected issue:
4. SettleFunding provenance validation
SettleFundingnow validates decoded portfolio provenance before reading the current leg funding snapshot and mutating:This prevents the NFT funding snapshot from being updated using portfolio data whose internal provenance does not bind to the passed portfolio account.
Affected issue:
5. TransferHook provenance validation
TransferHooknow validates decoded portfolio provenance before:PositionNftV16.last_holdermutation.This prevents transfer gating and holder-state mutation from using provenance-mismatched portfolio data.
Affected issue:
Security Impact
This PR fixes the following classes of issues:
ExtraAccountMetaListPDA recovery failure;GetPositionValue;SettleFunding;last_holderstate-integrity issue inTransferHook.The portfolio-consuming paths now fail closed unless the decoded portfolio provenance matches the actual portfolio account passed to the instruction.
This PR does not loosen account validation. The recovery path for
RepairExtraMetasis limited to the canonical PDA and only permits empty System-owned accounts to be recovered. Any non-canonical or unsafe account state remains rejected.Validation
Tested locally:
cargo test --features no-entrypointResult:
Final commit includes changes in:
src/cpi_v16.rssrc/processor.rssrc/transfer_hook.rssrc/valuation.rsRecommended checks before merge:
Summary by CodeRabbit