Security review

Automated review by Arc Studio, Circle's own tooling, plus Slither static analysis. This is not a human audit firm. The report below is what it returned, unedited.

# TokenFactory Security Review

**Contract:** `TokenFactory.sol` + `ArcRadarToken`  
**Target chain:** Arc Testnet / Arc Mainnet  
**Review date:** 2026-09-16  
**Severity tiers reviewed:** Critical · High · Medium · Low  
**Static analysis:** Slither 0.11.x (Foundry/Paris mode)

---

## Summary

The contract is lean and well-reasoned. No Critical or High findings were surfaced by either reviewer or by static analysis. There are **three Medium findings**, **three Low findings**, and several informational notes. The Mediums should be addressed before a mainnet deploy; the Lows are all straightforward hardening steps.

| Severity | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 3 |
| Low | 3 |
| Informational | 5 |

---

## Medium Findings

### M-1 — Fee-race griefing: callers can have `createToken` revert under them during a fee change

**Where:** `createToken` / `setCreationFee`

**What happens:** `createToken` requires `msg.value == creationFee` exactly. When the owner calls `setCreationFee` in the same block as a pending `createToken` call (or just before one that is already in the mempool), the honest caller's transaction reverts with `WrongFee` — their token never gets created, their gas is lost, and they have to retry. This is especially sharp when the owner *reduces* the fee (callers who sent the old, higher amount are now over-paying and get rejected).

**Arc context:** Arc's fast finality (sub-second blocks) makes this window very tight in practice, but the attack is still possible at a fee-decrease boundary, and the UX impact — a user's transaction failing silently with `WrongFee` — is jarring regardless.

**Recommended fix:** Accept `msg.value >= creationFee` instead of exact equality and refund the excess to `msg.sender` using a separate pull-refund pattern, OR store a "fee snapshot at last block" and allow callers to pay either the old or the new fee for one block after a change. Alternatively, add a time-lock on `setCreationFee` so fee changes become visible off-chain before taking effect.

---

### M-2 — Owner blocklist can permanently freeze accumulated fees

**Where:** `withdraw`

**What happens:** If the Arc protocol-level blocklist ever includes the owner's address (or the owner transfers ownership to an address that later gets blocklisted), `withdraw` will always revert at `to.call{value: amount}("")`, because Arc's native-USDC transfers to a blocklisted address fail at the protocol layer. Since `withdraw` is the only egress path, the fee balance becomes permanently locked in the contract with no recovery mechanism.

**The contract already correctly avoids the "push fee on every `createToken`" pattern** for exactly this reason — the pull design is the right call. But the same vulnerability surfaces on the pull side when the pull destination is blocklisted.

**Recommended fix:** Separate the `withdraw` destination from `owner()`. Add a `withdrawTo(address recipient)` function that allows the owner to specify an arbitrary recipient (not necessarily themselves), so the owner can route around a blocklisted address. Alternatively, implement a two-step "pending withdrawal recipient" pattern. At minimum, document the risk prominently.

---

### M-3 — `transferOwnership` to a zero address or uncontrolled address locks fees forever

**Where:** `Ownable.transferOwnership` (inherited) / `renounceOwnership` (blocked)

**What happens:** `renounceOwnership` is correctly blocked, which closes the most common ownership-loss path. However, `transferOwnership` to `address(0)` or to an address the new owner does not control (fat-finger, wrong paste) is not guarded. OpenZeppelin's `Ownable` does check for `address(0)` in `transferOwnership`, but does not implement a two-step confirmation. A one-step transfer to the wrong address is irreversible: `withdraw` will permanently revert because the `owner()` will always be the wrong address, and there is no `renounceOwnership` escape either.

**Recommended fix:** Use `Ownable2Step` from OpenZeppelin instead of `Ownable`. It requires the new owner to call `acceptOwnership()`, which eliminates the single-step mistake. This is a one-line import swap — `Ownable2Step` is already available in the pinned OpenZeppelin version.

---

## Low Findings

### L-1 — `MAX_SUPPLY` allows overflow in ERC-20 `totalSupply` depending on decimals

**Where:** `ArcRadarToken` constructor / `MAX_SUPPLY` constant

**What happens:** `MAX_SUPPLY` is `1_000_000_000_000_000e18` (10^33). When a token is created with 18 decimals (the ERC-20 default inherited from OpenZeppelin), this fits within `uint256` (max ~1.16 × 10^77). However, the supply check only validates against `MAX_SUPPLY` — it does not validate the *displayable* token amount. A creator can mint 10^15 tokens each with 18 decimal places. This is an unusual amount but technically valid; whether it is intentional (it could be) is a product decision. If only human-scale amounts are expected, a tighter `MAX_SUPPLY` (e.g. `10^27`) and a note in the NatDoc would make the intent clearer.

**Action:** No code change required unless the product intends to cap the display amount. Recommend adding a NatDoc comment explaining that supply is in raw `uint256` units (not display units) to avoid integrator confusion.

---

### L-2 — Slither `incorrect-equality` on `amount == 0` in `withdraw`

**Where:** `withdraw`, line 105

**What Slither flagged:** Slither's `incorrect-equality` detector fires on `amount == 0`. In this contract, `address(this).balance` can never be manipulated to be non-zero in a way that skips this guard — there is no ERC-20 `transfer` path that can alter `balance`, and a `selfdestruct` donation would only make `amount > 0`. Changing to `amount < 1` or using a `> 0` check is semantically identical, but the false-positive noise from tooling can be eliminated by replacing `if (amount == 0)` with `if (amount == 0) revert NothingToWithdraw();` using `amount < 1` — or more idiomatically, `if (amount == 0) revert NothingToWithdraw();` is already fine; the check could instead be written `require(amount > 0, ...)` to silence the detector. This is low-priority cleanup.

**Action:** Change `if (amount == 0)` to `if (amount < 1)` to silence Slither's strict-equality detector (they are identical on `uint256`).

---

### L-3 — Token name/symbol byte-length limits are code-point agnostic (UTF-8 multibyte)

**Where:** `createToken` validation

**What happens:** `bytes(name).length` counts **bytes**, not Unicode code points. A name with multibyte UTF-8 characters (e.g. emoji, CJK) will consume multiple bytes per character. A 3-byte emoji uses 3 of the 32-byte budget; a 4-byte emoji uses 4. This means:
- An attacker cannot bypass the limit — the check is conservative and correct.
- Legitimate users with non-ASCII names get fewer visible characters than the 32-byte ceiling suggests.

This is a documentation gap, not a security issue. Whether the limit should be characters or bytes is a product decision. If byte-length is intentional (gas predictability, indexer simplicity), add a comment saying so.

**Action:** Document the byte-length semantics in the NatDoc. No code change needed.

---

## Informational Notes

**I-1 — No `receive()` / `fallback()` — correct.** The contract only receives value through `createToken`'s `payable` modifier. There is no accidental bare-ETH entry point. This is the right design.

**I-2 — `creationFee = 0` is deliberately allowed.** The contract allows zero-fee operation. This is noted in the NatDoc and is a valid product decision. No issue.

**I-3 — `ArcRadarToken` has no owner, pause, or blacklist.** This is explicitly noted and is a feature, not a gap. Tokens are fully autonomous once deployed. No issue.

**I-4 — `emit FeesWithdrawn` before the `.call` is unusual but sound.** The NatDoc explains the reasoning: the CEI (checks-effects-interactions) convention is followed in spirit. If `to.call` reverts, the entire transaction reverts (including the event). There is no reentrancy vector here because `onlyOwner` is required, and re-entering `withdraw` from within the owner's `receive()` would only re-run the same function — it would observe `address(this).balance` as 0 after the first call succeeded, so `NothingToWithdraw` would fire. The event-before-external-call ordering is safe.

**I-5 — PREVRANDAO = 0 on Arc.** The contract does not use `PREVRANDAO`, `block.difficulty`, or any randomness primitive. No issue.

---

## Arc-Specific Checklist

| Check | Status |
|---|---|
| Native USDC (18 dec) used for `msg.value` fee accounting | Correct — `MAX_CREATION_FEE = 100e18` is denominated in native 18-dec units |
| ERC-20 USDC (6 dec) not confused with native | No ERC-20 USDC calls; no decimal confusion |
| Blocklist-aware on send path | Partially addressed (push avoided in `createToken`) — see M-2 for `withdraw` gap |
| PREVRANDAO / randomness | Not used — no issue |
| `selfdestruct` / `SELFDESTRUCT` | Not used — no issue |
| Paris EVM compatibility | No Cancun opcodes (`push0`, `mcopy`, etc.) used; compiles clean under Paris |
| `msg.value` exact-match griefing on fee change | See M-1 |

---

## Recommended Priority Order

1. **M-3 first** (swap `Ownable` → `Ownable2Step`) — one-line change, zero risk.
2. **M-2 second** (add `withdrawTo(address)` or a two-step recipient) — eliminates the permanent fee-lock risk.
3. **M-1 third** (fee-change race) — either accept `>=` + refund, or add a fee-change time-lock.
4. **L-2** (`amount < 1` for Slither) — cosmetic, worth the two-keystroke fix.
5. **L-3** (NatDoc byte-vs-character note) — documentation only.

---

## Audit Provenance

| Subagent | Dispatched | Files audited | Corpus rules cited |
|---|---|---|---|
| solidity-auditor | yes | 1 | none |
| functional-auditor | yes | 1 | — |