# Security Review — `SeriesCode.sol` (ArtBlocks ABX)

**Repo:** https://github.com/ArtBlocks/abx
**Commit:** `1e3061e945a9c32b4d14af51a49448ee6f9b4837` (default-branch HEAD at audit time; verified reachable with `git ls-remote`)
**Scope:** `contracts/src/tokens/SeriesCode.sol` (329 lines). Directly composed mixins were read for context: `AbxErc721Base`, `MaxInvocations`, `ExternalMinter`, `Paused`, `SeedSourceExtension`, `CreatorToken`, `ConfigurableParams`, `AbxParamsLib` (seed, hooks), `SeriesCodeFactory`, `AbxSeedSource`.
**Threat model (per client):** the creator/owner is trusted by design. Findings target unintentional bugs, footguns, over-broad trust, and trust in third parties.

**Severity counts:** 0 Critical · 0 High · 1 Medium · 4 Low · 4 Informational

**Method note:** This was run as a single-contract engagement. Instead of the full multi-agent fan-out, the audit was a manual, adversarial line-by-line review of the one contract plus its inherited mixins, driven by the protocol map below. No code was executed, and `forge` coverage was not run (policy). Dependency files outside the scope were read only to resolve how SeriesCode wires them.

## Summary
`SeriesCode` is a thin composition layer over well-factored mixins. The state-machine ordering in `_mintNext` is correct: the cursor is advanced before the external seed call, and the cap floor reads the cursor rather than `totalSupply`. The initializer is locked on the implementation, and creator-token enrollment is placed last in `initialize`. I found no way for an unprivileged attacker to steal funds, over-mint past the cap, re-initialize a clone, or rewrite a settled seed. The findings are about the trust the owner holds over *collectors*, and about the weak randomness of the reference seed source.

### Access-control map (SeriesCode surface)
| Function | Gate |
|---|---|
| `initialize` | `initializer` (one-shot; implementation locked in constructor) |
| `mint`, `mintMany` | owner always; minter only when `!paused` |
| `setMinter`, `setPaused`, `setSeedSource`, `setMaxInvocations` (decrease only), `setTransferValidator`, `setParamSchema`, `setParamHooks`, `lockParamHooks`, `setDelegateRegistry` | owner |
| `burn` | token owner/approved, only if `burnable` |
| `multicall` | anyone; each subcall delegatecalls with the real `msg.sender`, so it adds no authority |

---

## Findings

### [M-1] Owner-set transfer hook and validator can freeze every holder's transfers, and the hook can also brick minting
**Severity:** Medium
**Confidence:** [85]
**Location:** `SeriesCode.sol:320-326` (`_afterTokenTransfer` → `_notifyTransferHook`), `SeriesCode.sol:310-316` (`_beforeTokenTransfer` → `_validateTransfer`); `ConfigurableParams.sol:225`; `CreatorToken.sol:68`, `CreatorToken.sol:212`; `AbxParamsLib.sol:478`, `AbxParamsLib.sol:502`.

```solidity
function _afterTokenTransfer(address from, address to, uint256 id) internal override(...) {
    super._afterTokenTransfer(from, to, id);
    _notifyTransferHook(id, from, to);          // external call, reverts bubble
}
```
`_notifyTransferHook` makes an unconditional external call to an owner-chosen address on **every** mint, transfer and burn. A revert vetoes the movement.

**Exploit path (trusted-owner abuse or compromise):**
1. Owner calls `setParamHooks(0, 0, H)` where `H.onTokenTransfer` reverts, selectively (e.g. on a marketplace operator) or always.
2. Every transfer of every token reverts. Because the hook also runs on `from == 0`, **all mints revert too**, including the owner's.
3. If the owner called `lockParamHooks()` first, or later renounces ownership, this is permanent. The code documents that `lockParamHooks` freezes *addresses* only, so a locked proxy hook can be upgraded to revert later.
4. Independently, `setTransferValidator(V)` with any `V` that passes the probe in `_requireHasCode` makes `validateTransfer` revert, which freezes transfers. The owner can do this at any time, and the validator can never be locked or removed.

The code is explicit that this is an accepted trust assumption. It is still the largest trust opening for *buyers*: a collector's token has no guaranteed transferability, and a compromised owner key can freeze or hold the whole collection hostage (for example, to force a ransom or to disable secondary sales except via an owner-approved path).
**Recommendation:** (a) Make hook failures non-fatal for mints and burns, or wrap the hook call in `try/catch` with a gas cap, so a bad hook cannot block minting. (b) Offer a *permanent* "no hook / no validator changes" lock for projects that want a hard promise (a one-way `lockTransferValidator`). (c) Surface `paramHooks()` and `getTransferValidator()` prominently to buyers. (d) Consider a timelock on re-pointing either.

### [L-1] `setSeedSource` can be changed at any time and cannot be locked, so a "random" collection can be switched to a curated or grindable source mid-drop
**Severity:** Low
**Confidence:** [90]
**Location:** `SeedSourceExtension.sol:34-37`, `SeedSourceExtension.sol:52-62`; `SeriesCode.sol:238-241`

```solidity
function setSeedSource(address source) external onlyOwner {
    SeedSourceStorage.layout().seedSource = source;
```
Only the *settled* seeds are frozen. The source that will assign seeds to all *future* mints can be swapped between sales. The owner (or a compromised key) can point it at a source returning chosen values for favoured recipients, or clear it so tokens mint with no seed and are later schema-assigned. The `seed` governance schema is frozen by `latchSeedAssignment`, but the *source* is not.
Related: `IAbxSeedSource.seed()` is called with the new token id reserved but **not yet minted** (`SeriesCode.sol:236-248`), so a source that calls back into view functions such as `ownerOf(tokenId)` reverts, and a source that reverts blocks every non-owner mint until the owner swaps it (an owner-recoverable DoS only while the owner exists).
**Recommendation:** Add `lockSeedSource()` (one-way) consistent with `lockParamHooks`, and document that buyers should check it.

### [L-2] Reference `AbxSeedSource` randomness can be ground by a contract minter (rarity sniping)
**Severity:** Low (documented, but it is the default path)
**Confidence:** [90]
**Location:** `seed/AbxSeedSource.sol` (`seed`), invoked from `SeriesCode.sol:239-240`

```solidity
keccak256(abi.encodePacked(msg.sender, tokenId, block.prevrandao, blockhash(block.number - 1), block.timestamp))
```
All inputs are known inside the minting transaction, and `AbxFixedPriceMinter.purchase*` (line 190: `IAbxSequentialMint(token).mint(to)`) is callable by contracts. A buyer wrapper calls `purchaseTo(wrapper)`, reads the token's `seed` param, renders/evaluates it, and reverts unless it is rare. The only cost is gas. The source's own NatSpec discloses this; it is flagged because `SeriesCode`'s seed promise ("the token draws one") reads as fairer than it is for any project where rare outcomes carry price premiums.
**Recommendation:** Keep the warning prominent in the factory and SDK defaults, or offer a commit/reveal source (seed from a *later* block) as the recommended option for priced scarcity.

### [L-3] `_mint` instead of `_safeMint` can strand tokens in non-receiver contracts
**Severity:** Low
**Confidence:** [95]
**Location:** `SeriesCode.sol:248`

`purchaseTo`, `mint(to)` and `mintMany(to, ...)` deliver to any `to`, including contracts that cannot transfer ERC-721s. With burning off by default there is no recovery. The decision is documented and deliberate; the footgun is real for buyers using `purchaseTo` with a wrong or contract address.
**Recommendation:** Keep, but have the minter UI/SDK warn when `to` is a contract without `onERC721Received`, or offer an opt-in safe variant outside the sale path.

### [L-4] The single minter can mint the entire remaining supply; `mintMany` has no per-call or per-minter bound
**Severity:** Low
**Confidence:** [85]
**Location:** `SeriesCode.sol:205-216`, `SeriesCode.sol:253-257`

```solidity
function mintMany(address to, uint256 count) external { _requireMintAuth(); for (...) _mintNext(to); }
```
`mint`/`mintMany` give the authorized minter the same capability as the owner (minus pause bypass): it can mint *all remaining* tokens to any address in one call, for free, bypassing whatever pricing the minter contract was meant to enforce. Any bug or upgradeability in the minter contract therefore translates directly into a supply drain, and the cap is the only bound. `mintMany` is also an unbounded loop that can run out of gas (self-limiting).
**Recommendation:** Expose only `mint(to)` to the minter and keep `mintMany` owner-only, or give the minter a per-call/total allowance. Document that the minter address is fully trusted with the whole remaining supply.

### [I-1] Owner mint bypasses `paused`, and renouncing while paused permanently ends minting
**Severity:** Informational
**Confidence:** [90]
**Location:** `SeriesCode.sol:253-257`; `Paused.sol:34`
`owner()` skips the pause check by design. If the owner renounces (or loses the key) while `paused == true`, nobody can ever unpause and the remaining supply is unmintable. Consider refusing `renounceOwnership` while paused, or documenting it.

### [I-2] Deterministic deployment with an unguarded salt allows address front-running
**Severity:** Informational
**Confidence:** [75]
**Location:** `SeriesCodeFactory.sol:48-56`, `SeriesCodeFactory.sol:78-82`
The clone address depends only on the implementation, salt and factory, **not** on `params`. With a salt whose leading 20 bytes are zero (`_guardSalt` accepts it), anyone seeing the intended address can deploy first at that address with different init params (owner, validator, seed source). Harmless when the address is read after deployment, but dangerous for integrations that pre-commit to a predicted address. Prefer salts prefixed with the deployer address, and mix `keccak(params)` into the salt in the SDK.

### [I-3] `initialize` is callable by anyone on a clone that was not initialized in the same transaction
**Severity:** Informational
**Confidence:** [90]
**Location:** `SeriesCode.sol:150`
The factory initializes atomically (`SeriesCodeFactory.sol:68-73`), so this is not exploitable through the supported path. A hand-rolled `LibClone.clone` without an initializing call would be takeable by a front-runner. The implementation itself is protected (`_disableInitializers`, `SeriesCode.sol:127-130`).

### [I-4] Seed from the source is stored without validation; a source returning `bytes32(0)` marks the seed "set"
**Severity:** Informational
**Confidence:** [60]
**Location:** `SeriesCode.sol:238-241`
`_setTokenParam(tokenId, "seed", value, false, source)` persists whatever the source returns, including zero, and `isSet` becomes true so it can never be redrawn. Zero is a plausible "uninitialized" return from a buggy custom source. A non-zero check (or a documented sentinel) would avoid permanently settling a degenerate seed.

---

## Verified non-issues (checked, no vulnerability)
- **Cap vs. reentrancy:** `_mintNext` writes `nextTokenId` before the seed call (`SeriesCode.sol:235-237`); a reentrant mint sees the advanced cursor, so `nextTokenId <= maxInvocations` holds. `_maxInvocationsFloor` reads the cursor, so lowering the cap mid-frame cannot be bypassed.
- **Settled seed:** `AbxParamsLib.setTokenParam` (line ~270) refuses a second seed write unless a `seed` schema exists; `latchSeedAssignment` closes the schema-declaration reentrancy window before the external call.
- **Creator-token enrolment:** `_initCreatorToken` runs last in `initialize`, after all caps are written, so the validator callback cannot see a half-initialized clone.
- **Multicall:** every subcall delegatecalls with the original `msg.sender` and re-runs its own gate; `msg.value` is rejected by Solady; no ERC-2771.
- **Validator probe:** a documented misconfiguration guard, not a security boundary.