fix(nft): record last_holder on CPI-mediated transfers via Token-2022's in-flight flag (#174) - #175
Conversation
…'s in-flight flag `last_holder` is the sole authorisation for ReconcileBurnedNft's release of an escrowed portfolio, but the hook only wrote it when the TOP-LEVEL instruction was Token-2022. Any transfer routed through another program — marketplace escrow, orderbook fill, multisig — skipped the write, so the field stayed at its initial value: the minter (processor.rs:500). A buyer who acquired the NFT through a marketplace was never recorded, so an out-of-band burn released the position and its collateral to the seller, who had already been paid, while the actual owner was refused by the same gate. Additionally authorise the write when Token-2022's own `TransferHookAccount.transferring` flag is set on BOTH the source and the destination. Token-2022 sets it immediately before `invoke_execute` and clears it immediately after, inside `process_transfer`, so it is true for every real transfer regardless of routing and false during a spoofed direct `Execute`. Reading it needs no new dependency: it is one byte of TLV, and this file already hand-parses the same accounts for mint/owner/state. The gate is a disjunction and therefore monotonic — every case authorised before still is — so dcccrypto#152/dcccrypto#153 stay closed, and behaviour degrades to exactly the prior (fail-closed) one if an account somehow lacks the extension. Also corrects the comments this falsifies, including the note claiming the flag was unreadable without the spl-token-2022 crate, and the `new_holder_recorded` log line, which reported only the first disjunct and so denied the write on exactly the transfers this fix exists to record. Closes dcccrypto#174 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe transfer hook now reads Token-2022’s in-flight ChangesHolder tracking and reconciliation
Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: 🔵 Low · up to The PR records the current holder for program-mediated NFT transfers, improving who can recover an escrowed portfolio after a burn while preserving the existing account and anti-spoof checks. It is mergeable with explicit owner awareness that recovery authorization depends on Token-2022’s transient transfer flags being set and cleared correctly, which should be validated with an integration check. Sequence Diagram(s)sequenceDiagram
participant Token2022
participant process_execute
participant PositionNft
participant ReconcileBurnedNft
Token2022->>process_execute: invoke transfer hook
process_execute->>process_execute: verify caller and read paired transferring flags
process_execute->>PositionNft: write last_holder when holder_recorded
ReconcileBurnedNft->>PositionNft: read last_holder for authorization
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The implementation addresses issue Full details: Docstring CoverageExplanation Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 4 files. (1 skipped: 1 unsupported.) ✨ 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 |
1 participant
Closes #174.
Problem
ReconcileBurnedNftreleases an entire escrowed portfolio — position and collateral — tonft_state.last_holder, and that field is its sole authorisation (processor.rs:1275,:1319,:1326). The transfer hook only wrote it when the top-level instruction was Token-2022 (transfer_hook.rs:166, gate at:542pre-change). Every transfer routed through another program — marketplace escrow, orderbook fill, multisig, aggregator — skipped the write.Since
last_holderis initialised to the minter (processor.rs:500), an NFT traded only through programs keptlast_holder = minterfor its entire life, and it never self-corrected. A marketplace buyer was never recorded, so if any later holder burned out-of-band — the exact scenario #138 addedReconcileBurnedNftto recover from — the escrow went to a seller who had already been paid, while the real owner was refused by the same gate.PR #159 named this as a known tradeoff ("routes to the prior genuine holder — no theft, no new drain"). That assessment does not hold: after a sale the prior holder has no remaining claim, the loss to the buyer is the whole position, and because #145 exists to enable program-mediated trading, the stale state is the steady state rather than a rare one.
Fix
Additionally authorise the write when Token-2022's own in-flight signal is set on both the source and the destination:
process_transfercallsset_transferringon both accounts immediately beforeinvoke_executeandunset_transferringimmediately after, so the flag is true for every real transfer regardless of routing and false during a spoofed directExecute, where no transfer is running. That is exactly the discrimination the top-level heuristic was standing in for, without the marketplace blind spot.Reading it needs no new dependency. The doc comment previously asserted this required the
spl-token-2022crate, which conflicts with thesolana-programpin; that is not so — the flag is one byte of TLV, and this file already hand-parses the same accounts for mint/owner/state. That comment is corrected here.Why #152/#153 stay closed
The gate is a disjunction, so it is monotonic: every case authorised before still is, and the new term only adds cases where a real transfer is provably in flight.
To set both flags an attacker must get Token-2022 to actually run
process_transferon this mint's source and destination, which moves the token;new_owneris then read from the account that actually received it. The flag bytes live in Token-2022-owned accounts, so they are not attacker-writable, and both accounts are already pinned to this mint (:369/:390,:404/:422) with the mint pinned to the PDA (:483). The window cannot be observed from outside, becauseset/unsetbracketinvoke_execute, a failing inner instruction aborts the whole transaction rather than committing a set-without-unset state, andprocess_executeperforms no CPI. That last point is load-bearing and is now documented in the helper so a future CPI cannot silently break it.verify_cpi_caller_is_token2022is unchanged and still returnsErrfor a malformed Token-2022 outer instruction, so the #103 plain-Transferrejection and the mint-match check are untouched.Verification
tests/poc_stale_last_holder.rs, including two isolating controls (one differing from the finding only in the top-level program, one differing from the rejection only in the recordedlast_holder), the [SECURITY][HIGH] Forgeable last_holder via relaxed transfer-hook caller check enables theft of an escrowed portfolio through ReconcileBurnedNft #152/[HIGH] ExecuteTransferHook direct invocation bypasses #145 caller check, poisons last_holder; combined with permissionless ReconcileBurnedNft enables escrowed position theft #153 spoof regression, a half-set flag pair, a non-Accountaccount_type, and the realistic on-chain ATA layout whereImmutableOwner(type 7, length 0) precedesTransferHookAccount.last_holder_antispoof_152_153module.cargo clippy --all-targetsclean;cargo build-sbf --no-default-features --tools-version v1.52succeeds.Verified against the vendored
spl-token-2022sources:ExtensionType::TransferHookAccount == 15, stable across 1.0.0 / 6.0.0 / 8.0.1 / interface-2.1.0 (it is the on-chain wire format, and every version appended only at the tail);TransferHookAccount { transferring: PodBool }is one byte;PodBool → boolis!= 0.Behaviour change worth naming
last_holdernow advances on every program-mediated hop. If a marketplace holds the NFT in a program-owned escrow account between listing and sale,last_holderbecomes that escrow address, and an out-of-band burn while escrowed would release the portfolio there. That is the correct party by the program's own rule ("last_holderis the current holder") and is strictly better than paying an already-paid seller, but it is a case that did not arise before.Notes
transfer_hook.rs,processor.rsandinstruction.rs. Two pre-existing comments (state_v16.rs:80-85,processor.rs:1217-1219) claimedlast_holderis "rewritten to the recipient on every transfer" — that was false before this change and is true after it.nft_pdais now wider (it includes marketplace CPI). No new requirement is introduced — Token-2022 builds entry [5] writable on every path.TransferHookextension authority is set to the mint-authority PDA rather thanNone(processor.rs:545), which would make the "no foreign code in the window" property structural instead of contingent; the unusedsolana-sdk/proptestdev-dependencies, which pullopenssl-sysand blockcargo teston Windows though nothing insrc/ortests/references them; and the README instruction table omitting tag 7ReconcileBurnedNft. Happy to file or fix any of these separately.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation