fix(audit): M-1/L-1/L-2 — docs correctness, read-only hook accounts, EmergencyBurn decode fallback - #163
Closed
v1ktorrr0x wants to merge 1 commit into
Closed
fix(audit): M-1/L-1/L-2 — docs correctness, read-only hook accounts, EmergencyBurn decode fallback#163v1ktorrr0x wants to merge 1 commit into
v1ktorrr0x wants to merge 1 commit into
Conversation
…EmergencyBurn decode fallback M-1 (docs): remove false claim that LiteSVM integration tests are the runtime ground truth for the vendored portfolio layout. The const_assert! macros verify the mirror struct's internal consistency only; alignment with the live engine layout requires runtime validation. Corrected in cpi_v16.rs, slab_types_v16.rs, and README.md. L-1 (writable flags): flip PositionNft PDA (entry 5) and Portfolio (entry 6) from writable to read-only in the ExtraAccountMetaList for both MintPositionNft and RepairExtraMetas. Post-dcccrypto#105 the transfer hook is validation-only and never writes either account; the writable flags imposed unnecessary write-locks on portfolio accounts during transfers. RepairExtraMetas rationale updated to reflect the new correct flags. L-2 (#110B): EmergencyBurn no longer hard-reverts when a wrapper-owned portfolio is present but fails decode (e.g. future layout migration). Decode failure now skips the eligibility check and falls through to the unwrap CPI, which the wrapper handles on its own terms. The existing portfolio_gone path (dcccrypto#131) is unchanged.
📝 WalkthroughWalkthroughThe PR updates layout-validation documentation, changes ExtraAccountMetaList writability for mint and repair flows, and adjusts emergency burn handling when the portfolio is absent or cannot be decoded. ChangesValidation documentation
Extra metas and burn flow
Sequence Diagram(s)sequenceDiagram
participant Burn as process_emergency_burn
participant Decode as decode_portfolio
participant Check as "cpi_v16::emergency_burn_ok"
Burn->>Burn: check portfolio_gone
alt portfolio_gone
Burn-->>Burn: log and skip eligibility check
else portfolio present
Burn->>Decode: decode portfolio
alt decode fails
Decode-->>Burn: decode error
Burn-->>Burn: log and continue to unwrap path
else decode succeeds
Burn->>Check: verify eligibility
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/processor.rs`:
- Around line 1327-1328: The success log in RepairExtraMetas no longer matches
the rewritten account flags, since entries 5 and 6 are now read-only. Update the
msg! emitted after the repair to describe the new state accurately, and make
sure the text in RepairExtraMetas reflects that the portfolio entry is no longer
writable.
- Around line 655-658: The downstream hook contract docs are out of sync with
the serialized account flags in `processor.rs`. Update the documentation in
`transfer_hook.rs` for the `PositionNft PDA` and `Portfolio account` entries so
they are described as read-only, matching the current hook contract behavior and
avoiding a docs/runtime mismatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83bd55f2-bf6f-4d8b-b557-d939944dd36f
📒 Files selected for processing (5)
README.mdsrc/cpi_v16.rssrc/instruction.rssrc/processor.rssrc/slab_types_v16.rs
Comment on lines
+655
to
+658
| // 5: PositionNft PDA — read-only (#105: hook is validation-only, no longer writes f_snap_at_mint) | ||
| (*nft_pda.key, false, false), | ||
| // 6: Portfolio account — read-only (#105: B-3 ownership CPI moved to mint/burn; hook only reads) | ||
| (*portfolio.key, false, false), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the downstream hook contract docs too.
These entries now serialize PositionNft PDA and Portfolio account as read-only, but src/transfer_hook.rs still documents them as [writable]. Leaving that mismatch will confuse integrators and audits.
🤖 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 655 - 658, The downstream hook contract docs
are out of sync with the serialized account flags in `processor.rs`. Update the
documentation in `transfer_hook.rs` for the `PositionNft PDA` and `Portfolio
account` entries so they are described as read-only, matching the current hook
contract behavior and avoiding a docs/runtime mismatch.
Comment on lines
+1327
to
+1328
| (*nft_pda.key, false, false), // 5: PositionNft PDA — read-only (#105) | ||
| (*portfolio.key, false, false), // 6: Portfolio account — read-only (#105) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the repair success message to match the new flags.
RepairExtraMetas now rewrites entries 5 and 6 as read-only, but the success msg! still says portfolio now writable. That log line becomes misleading as soon as this change lands.
🤖 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 1327 - 1328, The success log in
RepairExtraMetas no longer matches the rewritten account flags, since entries 5
and 6 are now read-only. Update the msg! emitted after the repair to describe
the new state accurately, and make sure the text in RepairExtraMetas reflects
that the portfolio entry is no longer writable.
Owner
|
Thanks @v1ktorrr0x — this is landing as #173, 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: M-1 is accurate and kept as-is. L-1 would have bricked live transfers: your "#105: hook is validation-only" was true when you filed, but #152/#153 since added a new write — the hook sets Full detail and the verification are in #173. Closing this in favour of it — your change is in there, not discarded. |
dcccrypto
added a commit
that referenced
this pull request
Aug 17, 2026
…and narrowing L-2 (supersedes #163) (#173) * fix(audit): M-1/L-1/L-2 — docs correctness, read-only hook accounts, EmergencyBurn decode fallback M-1 (docs): remove false claim that LiteSVM integration tests are the runtime ground truth for the vendored portfolio layout. The const_assert! macros verify the mirror struct's internal consistency only; alignment with the live engine layout requires runtime validation. Corrected in cpi_v16.rs, slab_types_v16.rs, and README.md. L-1 (writable flags): flip PositionNft PDA (entry 5) and Portfolio (entry 6) from writable to read-only in the ExtraAccountMetaList for both MintPositionNft and RepairExtraMetas. Post-#105 the transfer hook is validation-only and never writes either account; the writable flags imposed unnecessary write-locks on portfolio accounts during transfers. RepairExtraMetas rationale updated to reflect the new correct flags. L-2 (#110B): EmergencyBurn no longer hard-reverts when a wrapper-owned portfolio is present but fails decode (e.g. future layout migration). Decode failure now skips the eligibility check and falls through to the unwrap CPI, which the wrapper handles on its own terms. The existing portfolio_gone path (#131) is unchanged. * fix(nft): keep meta entry [5] writable and narrow the EmergencyBurn fallback Follow-up to the M-1/L-1/L-2 commit. M-1 (doc corrections) is accurate and kept as-is. The other two needed work. L-1 — entry [5] must stay WRITABLE. The PR flipped both hook accounts to read-only on the stated basis that "#105: hook is validation-only". That was true when the PR was opened, but main has since gained #152/#153, which added a NEW write: process_transfer_hook does `nft_state.last_holder = new_owner` at transfer_hook.rs:542-545 on every genuine Token-2022 transfer. A read-only meta for the PositionNft PDA therefore makes every direct TransferChecked fail — and since RepairExtraMetas is PERMISSIONLESS, that is a free brick-any-NFT vector. Both sites are restored to writable (the repair path had the same flip, which is the dangerous one). Entry [6] (Portfolio) read-only is KEPT, argued separately as the review asked: verified against this build that the hook performs no invoke/invoke_signed at all and never mutably borrows `portfolio` — it only reads it to check the NFT PDA binding. The old "B-3 CPI mutates portfolio.owner" rationale moved to mint/burn in #105. Read-only is correct here and narrows the write lock. To stop the two entry tables drifting apart again, the flags are extracted into a single `EXTRA_META_ENTRY_FLAGS` table used by both the mint path and RepairExtraMetas, so divergence is now structurally impossible rather than a convention. Four tests pin the flags; the 34 pre-existing tests all passed with entry [5] read-only, so nothing in the repo would have caught this. Negative control: flipping entry 5 back to read-only fails `position_nft_pda_meta_entry_is_writable`. L-2 — narrowed. Skipping the eligibility gate on ANY PortfolioDecodeError falls through to UnwrapEscrowedPortfolio, which is not itself leg-gated, so it could emergency-burn a position never proven flat. The fallback now admits only BadVersion / BadAccountVersion / BadLayoutDiscriminator — the "future layout migration" case the PR justified it with, on an account already verified wrapper-owned. TooShort / BadMagic / BadKind / Cast / OwnerMismatch now propagate as errors; OwnerMismatch is a violated engine invariant and must never widen burn eligibility. Conflict resolution note: main's #110C provenance check is preserved on the Ok arm — the PR's version dropped it. cargo build clean; 48 lib tests pass (44 before, +4 new). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(nft): update the statements this PR made false, and derive the meta count Second-pass review findings. All of these were pre-existing text that THIS change turned into a lie — the failure mode the PR itself set out to fix. 1. ON-CHAIN LOG. RepairExtraMetas emitted `msg!("... (portfolio now writable)")`. That was accurate on main; this PR makes portfolio read-only, so the deployed program would log a false statement on every permissionless repair. Now reports the actual shape ("nft_pda writable, portfolio read-only"). 2. instruction.rs doc claimed "Post-#105 the hook is validation-only and no longer CPIs; both PositionNft PDA and Portfolio are now read-only". The first half is the exact reasoning that made L-1 dangerous — #152/#153 re-added a write, so entry [5] is writable. Rewritten to state the current flags and WHY each is what it is. 3. transfer_hook.rs:284 still labelled the portfolio account "(writable)". 4. README M-1 cited "#110B" for deferred runtime layout validation. Per the issue, item B is the EmergencyBurn wedge (what L-2 addresses) and item H is the layout cross-check. Corrected to #110H. 5. EXTRA_META_COUNT was re-declared as a literal `7` in BOTH call sites with nothing tying it to EXTRA_META_ENTRY_FLAGS — the table that actually decides the entries. Extracting the flags removed one drift risk and introduced another. Both are now `EXTRA_META_ENTRY_FLAGS.len()`. Verified: shrinking the table to 6 entries is a compile error ("expected an array with a size of 6, found one with a size of 7") rather than a silent mismatch. cargo build clean; 48 lib tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: v1ktorrrr <developer@zama-chat.com> Co-authored-by: dcccrypto <dcccrypto@users.noreply.github.com> 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.
Summary
cpi_v16.rs,slab_types_v16.rs, andREADME.md. Theconst_assert!macros only verify the mirror struct's own internal consistency, not alignment with the live engine layout.ExtraAccountMetaListfor bothMintPositionNftandRepairExtraMetas. Post-Design: minting does not escrow the position — minter retains direct control until first NFT transfer (contradicts documented custody) #105 the transfer hook is validation-only and never writes either account; the writable flags caused unnecessary write-locks on every transfer.RepairExtraMetasrationale updated to match.EmergencyBurnno longer hard-reverts when a wrapper-owned portfolio is present but fails decode (e.g. future layout migration changed magic/version). Decode failure now skips the eligibility check and falls through to the unwrap CPI. Theportfolio_gonepath (EmergencyBurn reverts when the bound escrowed portfolio was closed by the core → permanently unburnable NFT + stranded rent (#110-B, escalated by #105 escrow) #131) is unchanged.Summary by CodeRabbit
Bug Fixes
Documentation