CollectorCrypt.sol

This document summarizes the security-focused review of CollectorCrypt.sol, following a line-by-line and entry-point analysis. The contract is a UUPS-upgradeable ERC-721 that also serves as the migration target for Slab tokens: a holder can burn their Slab and atomically mint an equivalent CollectorCrypt. Findings are ordered by severity; each includes context, impact, and a recommended fix where applicable.

[HIGH] Non-Upgradeable ReentrancyGuard in a UUPS Contract

CollectorCrypt is deployed behind a UUPS proxy but imports the non-upgradeable ReentrancyGuard from @openzeppelin/contracts/utils/ReentrancyGuard.sol. That contract declares uint256 private _status as a regular storage variable and initializes it in a constructor. Constructors only run on the implementation contract, never on the proxy, so the proxy starts with _status = 0 (uninitialized) rather than _status = NOT_ENTERED (= 1).

In OpenZeppelin v5 the nonReentrant check is _status == ENTERED (= 2), so the first call passes accidentally (0 != 2) and from then on _status settles at 1. The modifier works — but by coincidence rather than design — and the cost is real:

1. _status occupies a regular storage slot in front of frozenTokens, _nextTokenId, and slab. Any future migration to ReentrancyGuardUpgradeable (which uses ERC-7201 namespaced storage) would free that slot and silently shift every variable underneath it — classic storage corruption on upgrade.
2. The OpenZeppelin Upgrades plugin treats non-upgradeable contracts in an upgradeable hierarchy as unsafe, so deployment will require an @custom:oz-upgrades-unsafe-allow escape hatch and the safety checks that exist to prevent this class of bug are disabled.

Recommended Solution

Replace ReentrancyGuard with ReentrancyGuardTransient from @openzeppelin/contracts/utils/ReentrancyGuardTransient.sol. It uses EIP-1153 transient storage (tstore / tload) keyed off a fixed keccak256-derived slot, allocates no regular storage, has no constructor, and is marked @custom:stateless — so the storage-layout hazard disappears entirely and no __init call is needed. Solidity ^0.8.27 supports it, and any Cancun-enabled chain (March 2024+) provides the underlying opcodes.

import {ReentrancyGuardTransient} from "@openzeppelin/contracts/utils/ReentrancyGuardTransient.sol";

contract CollectorCrypt is
    Initializable,
    ERC721Upgradeable,
    // ...
    ReentrancyGuardTransient,
    UUPSUpgradeable
{
    // No `__ReentrancyGuard_init()` needed.
}

After the swap, slot 0 of the proxy becomes frozenTokens, then _nextTokenId, then slab — a layout that is forward-safe across upgrades.

[LOW] migrateMint Requires Slab Approval but Advertises Ownership

_migrateMint checks slab.ownerOf(slabTokenId) == msg.sender and then calls slab.burn(slabTokenId). Because the burn runs with _msgSender() = CollectorCrypt (this contract, not the user), the Slab token must have either approve(CollectorCrypt, slabId) or setApprovalForAll(CollectorCrypt, true) set; otherwise the burn reverts inside Slab with ERC721InsufficientApproval.

The ownership check at the top of the function reads as if ownership is sufficient, so users who only own the Slab — without an approval — will hit a confusing OpenZeppelin revert long after spending gas on ownerOf. The approval requirement is not documented in NatSpec, not surfaced in a custom error, and not validated up front.

Recommended Solution

Add a pre-flight authorization check before invoking the Slab and revert with a dedicated error so the failure mode is obvious. Document the approval requirement in NatSpec on both migrateMint and batchMigrateMint:

error CollectorCrypt__SlabNotApproved(uint256 slabTokenId);

function _migrateMint(uint256 slabTokenId) private {
    if (slab.ownerOf(slabTokenId) != msg.sender) {
        revert CollectorCrypt__NotOwnerOfToken(slabTokenId);
    }

    if (
        slab.getApproved(slabTokenId) != address(this) &&
        !slab.isApprovedForAll(msg.sender, address(this))
    ) {
        revert CollectorCrypt__SlabNotApproved(slabTokenId);
    }

    // ... rest unchanged
}

A cleaner alternative is to require the user to call a single slab.setApprovalForAll(CollectorCrypt, true) once and document it as a one-time setup step, mirroring the pattern used by BatchZap for redeem and burn flows.

[LOW] Arbitrary Code Runs After the Slab Burn in _migrateMint

_migrateMint performs three observable actions in order:

1. slab.burn(slabTokenId) — an irreversible state change on the Slab contract.
2. _setTokenURI(...) and emit TokenMinted(...) — local state and event writes.
3. _safeMint(msg.sender, newTokenId) — which invokes onERC721Received on the recipient (msg.sender).

If msg.sender is a contract, step 3 runs arbitrary user-controlled code with the Slab already burned and the new CollectorCrypt already in their hands. The nonReentrant modifier prevents re-entry into other CollectorCrypt batch functions, but the callback can still:

• Forward the new NFT to a third party in the same transaction
• Call into unrelated protocols that index Transfer mints or that still believe the Slab exists
• Interact with Slab itself for other tokens

Funds are not at risk (a revert in onERC721Received reverts the whole transaction including the burn), but the surface for unexpected composition is large and provides no benefit: the recipient is always msg.sender, who must already be ERC-721-aware to have called migrateMint in the first place.

Recommended Solution

Use _mint instead of _safeMint in the migration path. The receiver-callback safety net is unnecessary when the recipient is also the caller:

function _migrateMint(uint256 slabTokenId) private {
    // ... checks unchanged ...

    string memory uri = slab.tokenURI(slabTokenId);
    slab.burn(slabTokenId);

    uint256 newTokenId = _nextTokenId++;
    _mint(msg.sender, newTokenId);
    _setTokenURI(newTokenId, uri);
    emit TokenMinted(newTokenId, msg.sender);
}

If the team prefers to keep _safeMint for parity with the minter-driven path, leave it but be aware that this is an external-code-after-irreversible-state-change pattern and treat any future logic added between steps 1 and 3 with extra care.

[LOW] Inconsistent nonReentrant Coverage on Batch and Burn Paths

batchTransfer, migrateMint, batchMigrateMint, safeMint, and batchSafeMint are all marked nonReentrant. batchBurn is not, and the inherited burn(uint256) from ERC721BurnableUpgradeable is not either. Burns themselves do not trigger receiver callbacks, so there is no direct reentrancy vector today, but the inconsistency is a code-smell that opens regressions whenever the burn path grows (e.g. a future royalty hook, an external bookkeeping callback, or a cross-chain notification).

Recommended Solution

Mark batchBurn as nonReentrant so the entire batch surface has uniform reentrancy semantics:

function batchBurn(uint256[] calldata _tokenIds) external nonReentrant {
    // ... unchanged
}

The inherited single burn is harder to retrofit without overriding, and is low-risk as long as the burn path stays free of external calls. Adding a regression test that fails if burn ever grows an external interaction is a cheap way to keep this assumption visible.

[INFO] Event Ordering: MetadataUpdate and TokenMinted Fire Before Transfer

Both private mint helpers (_safeMintOne and _migrateMint) call _setTokenURI and emit TokenMinted before invoking _safeMint. The resulting event log per mint is:

1. MetadataUpdate(tokenId) — emitted by _setTokenURI for a token that does not exist yet
2. TokenMinted(tokenId, to) — the custom event, also before the token exists
3. Transfer(address(0), to, tokenId) — the actual ERC-721 mint

On-chain state, authorization, re-entrancy, and the onERC721Received callback are all unaffected by the ordering: if _safeMint reverts, the whole transaction (including the URI write) reverts atomically, and OpenZeppelin v5's _setTokenURI does not check token existence so no revert occurs. The current order is in fact the friendlier one for contract recipients, because the URI is already set by the time onERC721Received runs and callbacks that read tokenURI(id) see the correct value.

The only downstream effect is observability. ERC-721 indexers (NFT marketplaces, subgraphs, EIP-4906 listeners) that ignore MetadataUpdate for tokens they have not yet observed a Transfer for may drop the initial URI write. Most production indexers fall back to fetching tokenURI(id) lazily, so even this is recoverable. The order is also a little surprising to anyone reading a trace and expecting the natural "mint, then describe" narrative.

Recommended Solution

This can be left as-is with no functional impact. If you prefer a more conventional trace, move the _setTokenURI call between _safeMint and emit TokenMinted — this preserves the "URI available during callback" property while emitting the EIP-4906 event after the Transfer:

function _safeMintOne(address to, string memory uri) private returns (uint256 tokenId) {
    tokenId = _nextTokenId++;
    _setTokenURI(tokenId, uri);
    _safeMint(to, tokenId);
    emit TokenMinted(tokenId, to);
}

The relative order of _setTokenURI and _safeMint is the meaningful choice (callback visibility vs. log order); the TokenMinted custom event should always come last so it acts as an unambiguous "minting complete" signal.

[INFO] Redundant __AccessControl_init() Call in initialize

initialize explicitly calls __AccessControl_init() and then __AccessControlDefaultAdminRules_init(...). The latter already chains through __AccessControl_init_unchained(), so the explicit call is redundant. Under OpenZeppelin v5 the unchained body is empty, so this is harmless — but it suggests confusion about the upgradeable initializer pattern and adds noise that obscures the (correct) initialization order for future maintainers.

Recommended Solution

Drop the explicit call:

function initialize(address defaultAdmin, address minter, address slabAddress) public initializer {
    // ... zero-address and code-length checks unchanged ...

    __ERC721_init("CollectorCrypt", "CCRYPT");
    __ERC721Enumerable_init();
    __ERC721URIStorage_init();
    __ERC721Pausable_init();
    __ERC721Burnable_init();
    __AccessControlDefaultAdminRules_init(uint48(2 days), defaultAdmin);

    slab = ISlab(slabAddress);
    _grantRole(MINTER_ROLE, minter);
}

[INFO] Reuse _safeMintOne Inside _migrateMint to Remove Duplication

_migrateMint reimplements the same four-line mint sequence that _safeMintOne already encapsulates: increment _nextTokenId, set the URI, emit TokenMinted, and call _safeMint. Any future change to the canonical mint flow (event payload, URI handling, mint hook) has to be made twice and kept in sync by hand.

Recommended Solution

After the Slab is burned, delegate the actual mint to _safeMintOne:

function _migrateMint(uint256 slabTokenId) private {
    if (slab.ownerOf(slabTokenId) != msg.sender) {
        revert CollectorCrypt__NotOwnerOfToken(slabTokenId);
    }

    string memory uri = slab.tokenURI(slabTokenId);
    slab.burn(slabTokenId);

    _safeMintOne(msg.sender, uri);
}

This shrinks _migrateMint to the parts that are actually migration-specific (the ownership check, the URI read, and the burn) and makes _safeMintOne the single source of truth for the mint sequence.

Note: this recommendation interacts with the [LOW] "Arbitrary Code Runs After the Slab Burn in _migrateMint" finding. If that fix is taken (switch to _mint to drop the post-burn callback), the cleanest shape is to extract a sibling _mintOne helper that mirrors _safeMintOne but calls _mint, and have _migrateMint call that. Either way, the migration path should not hand-roll its own mint sequence.

Summary

Addressing the high-severity item (switching to ReentrancyGuardTransient) removes a long-lived storage-layout fragility and is a near-zero-risk change. The medium-severity items surface a pre-existing UX assumption and shrink the migration callback surface; neither requires a redesign. The low and informational items are good candidates for the same PR.