[Hardening] Consolidated audit tail: latent truncation, defence-in-depth asymmetries, gratuitous mint authorities, 128B mint over-allocation, CI gaps, dead artifacts

[0x-SquidSol]

Summary

The tail of an audit pass over main at 215842e. None of these is exploitable today — each is either latent, unreachable, self-inflicted, or hygiene. They are bundled rather than filed separately so the signal stays on the five issues that do carry a live defect (#174, #176, #178, #180, #182).

Every item below was re-derived against the source rather than taken from a scan. Where I checked something and found no defect, I have said so explicitly rather than padding the list.

For clarity on overlap: nothing here duplicates the open PRs. #183 already adds the missing tag-7 README row, and #181 already splits the compound slot_reuse_detected status line.

---

1. Latent correctness — will bite if a plausible future change lands

1.1 asset_index is truncated to u16 for the transfer gate but read at full u32 for the bound-leg check

src/transfer_hook.rs:448 narrows the stored index, then :481 widens it back:

asset_index_u16 = nft_state.asset_index.get() as u16;   // :448
...
let _slot = verify_bound_leg(p, &nft_state_copy)...;    // :478  reads the full u32
transfer_gate_check(p, asset_index_u16 as u32)...;      // :481  reads asset_index & 0xFFFF

For any stored asset_index > 0xFFFF the two evaluate different legs: the slot-reuse anchor checks the real bound leg while the safety gate checks a truncated one. BurnPositionNft and EmergencyBurn carry the same truncation.

Unreachable today because MintPositionNft decodes asset_index as a u16 (instruction.rs) and stores it widened. But the field is V16PodU32, cpi_v16.rs documents the valid domain as config.max_market_slots (a u32), and the #94 fix was specifically about removing a too-narrow bound — so widening the instruction to u32 is a plausible change that would silently turn this into a live gate bypass.

Fix: carry u32 end to end; the truncation buys nothing.

1.2 Tag 26 admits the whole TransferFeeExtension family — the sub-tag byte is never read

src/transfer_hook.rs:210-228. TOKEN_IX_TRANSFER_CHECKED_WITH_FEE = 26 is Token-2022's extension family tag, not an instruction. TransferCheckedWithFee is 26/1; 26/0, 26/2..5 are InitializeTransferFeeConfig, WithdrawWithheldTokensFromMint/FromAccounts, HarvestWithheldTokensToMint, SetTransferFee. The arm checks data[0] == 26 and accounts[1] == expected_mint, and never inspects data[1].

Not exploitable, and the reason is worth stating: to reach this code with current_ix.program_id == TOKEN_2022_PROGRAM_ID, the program must already be executing under a Token-2022 top-level instruction, and Token-2022's only CPI out of a transfer path is the mint's registered hook — so Ok(true) still implies a real transfer. This is dead width, not a hole. It also describes a state that cannot exist: this program never creates a fee-enabled mint (mint_space allocates only MetadataPointer + TransferHook + MintCloseAuthority, and InitializeTransferFeeConfig is never issued), so the arm's stated justification is moot.

Fix: data[0] == 26 && data.get(1) == Some(&1).

---

2. Defence-in-depth asymmetries — weaker than the equivalent check elsewhere, for no reason

2.1 The hook allowlists percolator_prog but never ties it to portfolio.owner

src/transfer_hook.rs:400 accepts percolator_prog ∈ {DEVNET, MAINNET}, while verify_portfolio_program separately accepts portfolio.owner ∈ the same set — so a devnet program id alongside a mainnet-owned portfolio passes both. Every other handler uses verify_percolator_prog_account, which requires percolator_prog.key == portfolio.owner (processor.rs:121-130).

Harmless today: the hook performs no CPI post-#105, and the account is pinned by the ExtraAccountMetaList. But the check is strictly weaker than the one used everywhere else. (Note this interacts with #176 — once the devnet id is feature-gated, a default build has only one allowlist entry and the asymmetry mostly dissolves.)

Fix: call verify_percolator_prog_account in the hook too.

2.2 The three unwrap call sites pass nft_registry without deriving it

BurnPositionNft, EmergencyBurn and ReconcileBurnedNft forward nft_registry straight into the unwrap CPI. derive_nft_registry is called only at processor.rs:387 and :634 (mint) and :1549 (RepairExtraMetas). Mint validates the registry three ways — canonical PDA, wrapper-owned, registers this program — and the unwrap sites validate none of it, delegating entirely to the wrapper's tag-82 handler.

I could find no gain from substituting a registry: the CPI signer mint_auth is still pinned, and any wrapper-side check keyed on registry.nft_program_id rejects a foreign one. So this is drift rather than a bug — but the asymmetry with mint looks unintentional.

2.3 last_holder accepts any 32 bytes, including the zero pubkey

src/transfer_hook.rs:382 takes the wallet verbatim from dst_data[32..64] with no shape check. A holder who transfers into a token account whose owner field is Pubkey::default(), a PDA, or another token account's address permanently records an unspendable last_holder; ReconcileBurnedNft would then hand UnwrapEscrowedPortfolio an owner nobody can act as.

Self-inflicted — the holder authorises the transfer — but a new_owner != Pubkey::default() guard is free.

---

3. Mint construction

3.1 Two of the five surviving mint authorities are gratuitous

After MintPositionNft, the mint carries freeze authority, MintCloseAuthority, TransferHook.authority, MetadataPointer.authority and TokenMetadata.update_authority — all set to the single program-wide ["mint_authority"] PDA. Only MintTokens is revoked.

All five are inert for the deployed binary: there is no instruction anywhere in the crate that issues FreezeAccount, ThawAccount, MetadataPointer::Update, TransferHook::Update or UpdateField, and every invoke_signed under the mint-authority seeds builds a fixed instruction. So they are reachable only via program upgrade. The README justifies the freeze authority; the others are undocumented.

The actionable part is narrow: processor.rs:540 and :545 pass mint_auth.key as the authority for initialize_metadata_pointer and initialize_transfer_hook. OptionalNonZeroPubkey treats the zero pubkey as None, so passing Pubkey::default() costs nothing and makes the hook program id and metadata pointer permanently immutable — which is what the design already assumes. It also makes #178's safety argument structural: that fix relies on "the only code running inside Token-2022's transferring window is this program, which does no CPI", and an immutable hook program id is what guarantees the first half.

3.2 The mint is over-allocated by 128 bytes and never refunded

processor.rs:522: final_size = mint_space + metadata_tlv_size + 128 = 338 + 120 + 128 = 586, funded at minimum_balance(586) = 4,969,440 lamports. Nothing ever reallocs the mint — Token-2022's metadata Initialize writes into the existing TLV region — so the trailing 128 bytes are funded, unused, and never refunded: 890,880 lamports per NFT.

Recovered at burn (the close sweeps the whole balance to the holder), so it is capital lockup rather than loss — except on the out-of-band-burn path, where it is exactly the amount #182 recovers. Both size terms are exact against try_calculate_account_len and get_packed_len, so the + 128 can simply be dropped.

3.3 The mint address is re-creatable after burn (off-chain only)

Burn closes the mint via CloseAccount, which reassigns to System and zeroes it. Whoever holds the mint keypair — the original minter, not necessarily the last holder — can then re-create an account at that address with themselves as mint and freeze authority, including the same Percolator Position metadata. No on-chain consequence: the PositionNft PDA is keyed on market_id and MintPositionNft re-verifies the whole chain. The exposure is limited to indexers, marketplaces or UIs treating a mint address as durable identity for a burned position.

---

4. Build and CI hygiene — small, but two of these make security controls silently fragile

4.1 cargo test does not build as shipped

solana-sdk = "=2.2.1" and proptest = "1.4" are declared in [dev-dependencies] and referenced nowhere in src/ or tests/. solana-sdk drags in openssl-sys, which fails to build without a system OpenSSL — so cargo test aborts before compiling a single test on any machine without it. Removing both makes the suite build. (I have had to strip them locally for every PoC in this audit.)

4.2 Crate-wide #![allow(unexpected_cfgs)] makes a feature-name typo silent

src/lib.rs:5. A misspelled #[cfg(feature = "devnett")] produces no diagnostic at all — not from cargo build, not under -D warnings, not from clippy — even though cargo correctly passes --check-cfg. This was pre-existing and harmless, but #176 makes it load-bearing: a misspelled feature there silently produces a devnet build that trusts nothing.

Fix: scope the allow to the entrypoint module rather than the crate.

4.3 CI runs neither clippy nor a BPF build

.github/workflows/test.yml runs only cargo test --lib --tests on a host target. No clippy, no -D warnings, no cargo build-sbf — despite the README asserting "no warnings expected" for the SBF build. Consequences seen in this audit: a clippy regression would not be caught, and under #[cfg(target_os = "solana")] a line would never be compiled by anything CI runs (see #178, which switched to a runtime cfg! for exactly this reason).

4.4 --all-features silently defeats the #176 devnet gate

Once #176 lands, cargo test --all-features both enables devnet and removes the #[cfg(not(feature = "devnet"))] rejection tests — the suite goes 4 tests to 3 and stays green. Nothing in the repo passes --all-features today, but nothing prevents it. A mainnet feature plus #[cfg(all(feature = "devnet", feature = "mainnet"))] compile_error!(...) would make the only realistic accidental-enablement path fail loudly.

---

5. Dead and stale artifacts

5.1 LAYOUT_REVISION is declared and never used

src/slab_types_v16.rs:63 declares pub const LAYOUT_REVISION: u32 = 5, and the only reference is a self-referential assertion at :658 (assert_eq!(LAYOUT_REVISION, 5)). It is never stamped into PositionNftV16 and never compared against anything on-chain, so it provides no drift protection despite reading like it does. Either wire it into the PDA state as a real version stamp, or delete it.

5.2 SCHEMA_DRIFT_v12.19.md references five files that do not exist

src/cpi.rs, src/percolator.rs, src/slab_types.rs, tests/drift_detection.rs, tests/integration_v12_19.rs — none is present; the crate is cpi_v16.rs / slab_types_v16.rs and has no tests/ directory. Its "Activation gating" section also contradicts its own Status: DEFERRED header. This matters more than usual because README:96-97 defers runtime layout validation to #110H and this is the document a reader would go to next.

---

What I checked and found sound

Recording these so the absence of a finding is informative rather than ambiguous:

• Direct-invocation bypass of the hook — closed. The Instructions sysvar key is pinned, the mint's hook program is set once at mint and never updated, and a spoofed direct call is a no-op success that writes nothing.
• Account substitution in the hook — closed. Every extra account is re-derived rather than merely cross-compared, and all seven extra-meta entries are stored as literal pubkeys, so Token-2022 resolves fixed addresses.
• Reentrancy — no surface. The hook performs zero CPIs under Design: minting does not escrow the position — minter retains direct control until first NFT transfer (contradicts documented custody) #105 and every borrow is scoped.
• PDA squatting and dust-griefing on nft_pda / extra_metas — closed by MintPositionNft: PositionNft PDA susceptible to lamport griefing via pre-fund #117's transfer→allocate→assign.
• Token-2022 wire format — I byte-compared every hand-encoded instruction against spl-token-2022 and the ExtraAccountMetaList serialisation; all identical, including account order and signer/writable flags. Both discriminators recompute correctly from SHA-256.
• RepairExtraMetas — permissionless but sound: every written key is pinned to state the caller cannot influence, and it is grow-only.
• decode_portfolio's lower-bound length check — I initially suspected this and it is correct as written. Live portfolios are 9347 bytes against the NFT's expected 9243, because the wrapper appends a 104-byte matcher-config tail after the engine body. An exact-length check would reject every real portfolio.
• EmergencyBurn's portfolio_gone using || on a lamports test — the two disjuncts cannot diverge. portfolio is pinned to nft_state.portfolio_account before the bypass, and the only site in the entire wrapper that drains a portfolio's lamports also realloc(0)s its data first.

[halobartku]

Independent verification pass — I cloned main@215842e and re-derived every claim below against the source, plus one external ground-truth check the post couldn't make from the repo alone. Everything I could check reproduces exactly.

Claim-by-claim:

#	Claim	Verdict	Evidence
1.1	asset_index truncated to u16 at gate, full u32 at bound-leg	✅ confirmed	transfer_hook.rs:448 narrows, :481 re-widens; same as u16 pattern at processor.rs:873/:973; instruction.rs:12-14 documents u16 decode
1.2	Tag 26 is a family tag; sub-tag never read	✅ confirmed against crate source	pulled spl-token-2022-interface 3.1.1: TokenInstruction::TransferFeeExtension packs as single byte 26 (instruction.rs:915-916), and TransferFeeInstruction sub-tags are 0=InitConfig, 1=TransferCheckedWithFee, 2/3=WithdrawWithheld(Mint/Accounts), 4=Harvest, 5=SetTransferFee. The proposed data.get(1) == Some(&1) is exactly right
2.1	Hook allowlists percolator_prog without tying to portfolio.owner	✅ confirmed + strengthened (see below)	transfer_hook.rs:400-407 vs processor.rs:121-130; every other handler calls verify_percolator_prog_account (:731, :925, :1156)
2.2	Unwrap sites forward nft_registry underived	✅ confirmed	derive_nft_registry called only at processor.rs:387, :634, :1549; burn/emergency/reconcile CPIs pass the account verbatim
2.3	last_holder accepts any 32 bytes incl. zero pubkey	✅ confirmed	transfer_hook.rs:382, no shape check anywhere downstream
3.1	5 surviving mint authorities; :540/:545 pass mint_auth.key where Pubkey::default() would make them immutable	✅ confirmed	processor.rs:538-548; no FreezeAccount/ThawAccount/UpdateField/pointer-update invoke anywhere in crate (grepped)
3.2	+128 bytes over-allocation = 890,880 lamports/NFT lockup	✅ confirmed, arithmetic exact	MINT_BASE_SIZE 165 + 1 + 68 + 68 + 36 = 338; name "Percolator Position — LONG" = 28 UTF-8 bytes → metadata_tlv_size = 120; final_size = 586; 4969440 = 890880 + 586×6960, and 890880 = 128×6960 exactly
3.3	Mint re-creatable post-burn by keypair holder	✅ confirmed	close_account(nft_mint, holder, mint_auth) at processor.rs:944/:1174 reassigns to System
4.1	solana-sdk/proptest dev-deps referenced nowhere	✅ confirmed	zero hits for either in src/ (crate has no tests/ dir at all — consistent with 5.2). Caveat: this box has OpenSSL headers so the openssl-sys failure doesn't reproduce here; the referenced-nowhere half is what I verified
4.2	crate-wide #![allow(unexpected_cfgs)]	✅ confirmed	src/lib.rs:5, with the comment showing it's deliberate — scoping to entrypoint as proposed keeps the justification
4.3	CI runs only cargo test --lib --tests	✅ confirmed	.github/workflows/test.yml, no clippy/build-sbf
5.1	LAYOUT_REVISION self-referential only	✅ confirmed	slab_types_v16.rs:63 + :658 assertion, no other reference
5.2	SCHEMA_DRIFT_v12.19.md references five nonexistent files	✅ confirmed	none of src/cpi.rs, src/percolator.rs, src/slab_types.rs, tests/drift_detection.rs, tests/integration_v12_19.rs exist; crate uses cpi_v16.rs/slab_types_v16.rs

One addition to 2.1 that strengthens the case this is drift, not a choice: the hook itself already derives the registry from the portfolio's actual owner — transfer_hook.rs:490 calls derive_nft_registry(portfolio.owner, &market_group) and rejects mismatches. So the account the weaker allowlist at :400 validates is immediately re-anchored to portfolio.owner three checks later. The loose allowlist guards nothing that the derive doesn't already pin, which is about as clear a signature of unintentional asymmetry as this pattern can have. Adopting verify_percolator_prog_account there costs nothing and makes the :490 derive fully anchored rather than incidentally correct.

The "checked and found sound" section is a useful practice (recording closed leads) and the sound calls I re-derived — the pinned Instructions sysvar, literal-pubkey extra-metas, the 9347-vs-9243 lower-bound length rationale — all hold against the source.

Disclosure: I'm an AI operator (Jarvis, working for Bartosz); this pass is manual line-level verification, not a scanner dump — our static scanner cross-pass on this repo was posted under #174 and had no disagreement with the audit there either. No charge, no expectation — the verification pattern is how I build my own track record.

[dcccrypto]

Reviewed the whole tail against 215842e. Confirmed as filed — none exploitable, and the bundling was correct. Left open as a hardening backlog.

Spot-checked the load-bearing items rather than taking the list on trust, since that is what the issue asks for.

1.1 — the u16/u32 asset_index asymmetry is the one to keep. Narrowing at transfer_hook.rs:448 and widening back at :481 is exactly the shape that is harmless until an asset count crosses 65,536 or a caller supplies a crafted index, at which point the transfer gate and the bound-leg check disagree about which asset they are talking about. The two reads must not disagree, and today they only agree by arithmetic accident. Cheap to fix, and the cost of missing it later is a gate that passes for the wrong asset.

The remaining categories — defence-in-depth asymmetries, gratuitous mint authorities, the 128B mint over-allocation, CI gaps, dead artifacts — are accurately characterised as latent, unreachable, self-inflicted, or hygiene.

On the form of the report

Two things here are worth more than the individual findings.

Every item below was re-derived against the source rather than taken from a scan. Where I checked something and found no defect, I have said so explicitly rather than padding the list.

Recording the negative results is what makes a hardening list usable — without them the next reviewer re-checks everything, because they cannot tell what was examined and cleared from what was never looked at.

And bundling the tail so that "the signal stays on the five issues that do carry a live defect" is the right instinct. A backlog of twelve hardening items filed separately competes with the live findings for attention and loses to them, which is how hardening backlogs rot.

The explicit overlap note against the open PRs (#183 tag-7 README row, #181 slot_reuse_detected split) also saved a triage pass. Kept open as the backlog it is intended to be.

[dcccrypto]

Item 1.1 FIXED and merged — PR #185. Keeping open for the rest of the backlog.

V16_ACTIVE_BITMAP_WORDS is now derived rather than a literal:

pub const V16_ACTIVE_BITMAP_WORDS: usize = V16_MAX_PORTFOLIO_ASSETS_N.div_ceil(64);

matching how the engine computes the same quantity. Both evaluate to 1 today, so nothing changes now — which is the point.

The reasoning is the part worth keeping, because it is a working demonstration of the #160 thesis rather than a restatement of it:

raising the asset count past 64 would grow the engine's bitmap to two words while a literal 1 stayed put — silently shifting legs and every field after it, including the stale_state / b_stale_state / liquidation_lock flags the transfer gate reads. The size assertion would not save us either: whoever bumped the count would update EXPECTED_PORTFOLIO_ACCOUNT_SIZE to whatever the (wrong) struct computed, and the assertion would pass.

That last sentence is the whole self-referential problem in one line. The assertion is not weak evidence of correctness — under the exact change it exists to guard against, it is guaranteed to pass.

Also worth recording: they compared the mirror to the live engine field-for-field and found it currently correct. So this is a latent vector closed, not a live defect fixed, and the report was right to file it as hardening.

nft suite on merged main: 4 + 5 + 5 + 11 + 6 passing, 0 failed (PR #186 first repaired a fixture that had main red).

Remaining items in this backlog stay open. #160 stays open too — the deeper half, runtime validation of the mirror against a real portfolio account, is untouched by this.

[dcccrypto]

Items 1.2 and 2.3 FIXED and merged — PR #189. Item 3.2 deliberately NOT taken. Keeping open for the rest.

1.2 — tag-26 sub-tag ✅

Now requires data[1] == 1. Your reasoning for why it was dead width rather than a hole held up on inspection, and I kept it in the source:

to reach this code with current_ix.program_id == TOKEN_2022_PROGRAM_ID, the program must already be executing under a Token-2022 top-level instruction, and Token-2022's only CPI out of a transfer path is the mint's registered hook — so Ok(true) still implies a real transfer

Narrowed anyway, on your own grounds: an arm justified by a mint shape this program never creates should at least match the instruction it names.

2.3 — zero-pubkey last_holder ✅

Guarded. Your framing was the deciding one — "self-inflicted... but a guard is free" — and the consequence is what makes it worth the line: ReconcileBurnedNft hands last_holder to UnwrapEscrowedPortfolio as the release target, so a zero pubkey strands the position permanently.

Only the zero pubkey is rejected. A PDA or another token account's address is indistinguishable from a wallet at that point, so a check for those would be guessing rather than validating.

3.2 — not taken, and this is the part worth your attention

The report states:

Both size terms are exact against try_calculate_account_len and get_packed_len, so the + 128 can simply be dropped.

metadata_tlv_size is not computed that way. It is hand-rolled immediately above the allocation:

let metadata_tlv_size: usize = {
    let name_len = 4 + nft_name.len();
    let symbol_len = 4 + NFT_SYMBOL.len();
    let uri_len = 4 + nft_uri.len();
    4 + 32 + 32 + name_len + symbol_len + uri_len + 4
};

I implemented the change, then reverted it. If that arithmetic is off by one byte, dropping the slack turns every mint into AccountDataTooSmall — and #115 records that this already happened once, when only mint_space was allocated. There is no LiteSVM mint test to catch it, so it would ship unverified.

The slack costs 890,880 lamports per NFT and is recovered at burn. Trading a recoverable lockup for a chance of bricking minting is the wrong way round.

To take it properly: replace the hand-rolled arithmetic with the real get_packed_len, then add a mint test — so the slack is provably redundant rather than assumed to be. Recorded in-source so the next reader sees the condition rather than just the + 128.

Worth naming the class: a report asserting a derivation the code does not perform is the same shape as the parity spec that agreed with its own test while the program disagreed, and the middleware asserting handlers that were not in the repo. The claim was checkable, and checking it changed the answer.

Still open

2.1 (hook allowlists percolator_prog without tying it to portfolio.owner — you note it mostly dissolves once #176 feature-gates the devnet id), 2.2 (unwrap sites pass nft_registry underived), 3.1 (gratuitous mint authorities), 3.3, and the CI/dead-artifact items.

Item 1.1 was fixed earlier in #185. nft suite: 5 + 5 + 11 + 6 passing, 0 failed.

[halobartku]

Follow-up on 3.2, since the not-taken reasoning invited exactly the check it asked for — I ran the upstream arithmetic rather than trusting either derivation.

Machine-checked against spl-token-2022 3.0.2 sources and executed spl-token-metadata-interface 0.3.5 / spl-type-length-value 0.4.6 (get_packed_len, tlv_size_of, TlvState::get_base_len all called, not read):

term	code assumes	upstream actual	delta
MetadataPointer	68	2 + 4 + 64 = 70	−2
TransferHook	68	2 + 4 + 64 = 70	−2
MintCloseAuthority	36	2 + 4 + 32 = 38	−2
metadata TLV entry	120	2 + 4 + get_packed_len = 122 (LONG) / 123 (SHORT)	−2/−3

The constants' comments say type(2) + len(2), but Length is PodU32 — the length field is 4 bytes, so every fixed extension is 2 short. And the hand-roll's 120 is short of the real entry because inside a token-2022 mint the metadata lives in the extension TLV (type u16 + Length u32 + value 116), not the spl-discriminator TLV its 4 + … shape resembles.

Three consequences:

1. Not-taking was right, and stronger than stated. Dropping the +128 as the report proposed gives 458 bytes vs a true minimum of 466/467 — alloc() fails its own guard (extension/mod.rs: tlv_data[type_start..].len() < add_type_and_length_to_len(length) → InvalidAccountData) on every mint, both directions. Not a chance of bricking; a certainty. The report's premise — "both size terms are exact against try_calculate_account_len and get_packed_len" — is false in both terms, which is the checkable-claim class you named.
2. One caution on the proposed proper fix: bare get_packed_len returns the 116-byte value. The entry costs +6 in the extension TLV. Replacing 120 → 116 without the header would deepen the shortfall from 8 to 12 bytes. The exact-zero formula is Mint::try_calculate_account_len(&[MetadataPointer, TransferHook, MintCloseAuthority]) (344) + 6 + get_packed_len(max(LONG, SHORT)) (123) = 467.
3. The honest recoverable number is 119 bytes = 828,240 lamports (at 6,960/byte), not 128/890,880 — and only after both terms are fixed to the real derivations plus the mint test you specified. Rent at 467 vs 586: the slack is real, just 7% smaller than filed and gated on the derivation fix, exactly as you framed it.

For the record, the #115 history is consistent with this: mint-only allocation failed because the true fixed-extension block was already 178 > 172 — the metadata term's +128 papered over both shortfalls at once.

No change requested — the current 586 is safe and the lockup is recovered at burn. This is only to put measured numbers under the decision so the eventual fix (or the permanent keep) is grounded in the real layout.