# 🔐 Security Review — AughtNaughts

## Scope

|                          |                                                                    |
|--------------------------|--------------------------------------------------------------------|
| **Target**               | `AughtNaughts.sol` — 1/1 onchain-art NFT + embedded reserve auction |
| **Source origin**        | Contract source pasted directly in the client's job description (not a git repo); saved to `src/AughtNaughts.sol`, 441 lines, `pragma ^0.8.24` |
| **Dependencies**         | OpenZeppelin `ERC721`/`ERC2981`/`Ownable`/`ReentrancyGuard`/`Base64`/`Strings` (v5-shaped API) and `./SSTORE2.sol` — **neither is present in the supplied source**; both are treated as trusted, already-audited external dependencies and are out of scope for this review |
| **Methodology**          | Three-phase audit: context mapping (protocol map + access-control inventory + threat catalog) → breadth pass (5 checklist domains: general, precision-math, erc721, access-control, dos) → depth pass (12 independent attacker-mindset agents, run blind to the breadth pass) → cross-phase reconciliation |
| **Confidence threshold** | 50 (findings below this bar are listed as Leads, not scored findings) |

**Note for the client**: this target has been submitted for audit before; per our engagement policy every job runs a fully independent audit from scratch — the findings below come entirely from this run's own three-phase analysis, not from reuse of any prior report.

---

## Reconciliation Summary

- **Breadth-phase findings**: 10 items (1 High, 2 Medium, 4 Low, 3 Informational) before cross-phase merge.
- **Depth-phase findings**: 4 promoted findings (1 High, 2 Medium, 1 Low) from 12 independent agents, plus a shared set of confirmed-non-issues.
- **Overlap**: 6 · **Breadth-only**: 2 (stranded-token rescue gap, returndata gas-griefing) · **Depth-only**: 1 (`setHiResData` mid-auction mutation) · **Re-examined leads kept**: 2 (mimeType injection escalated Low→Medium; timeBuffer/increment issue escalated to include a distinct overflow-DoS mechanism) · **Demoted**: 0.
- **Coverage gate**: 17 external/public state-changing entrypoints in the access-control inventory, 17 addressed (by a finding or an explicit "examined, no issue" note). 8 threat-catalog rows, 8 answered. **Coverage holes closed this pass: 0** — both phases independently reached full coverage of the entrypoint/threat surface; reconciliation combined and cross-verified rather than filling gaps.
- **Confidence floor**: 50. Three items below that bar are listed under **Leads**, not as scored findings.

---

## Access-Control Inventory

| Function | Guard | Caller | Writes / moves |
|---|---|---|---|
| `writeChunks(batchId, chunks)` | `onlyOwner` (L155) | owner | deploys SSTORE2 chunk contracts, no `AughtNaughts` storage |
| `mint(pointers, name, description, mimeType)` | `onlyOwner` (L174) | owner | `nextTokenId`, `_pieces[id]`, mints NFT to `owner()` |
| `setHiResData(id, pointers)` | `onlyOwner` (L192) + append-once check (L195) | owner | `_pieces[id].hiResPointers` |
| `startAuction(id, reserve, duration)` | `onlyOwner` (L282), `nonReentrant` | owner | `auctions[id]` (full overwrite), escrows token to `address(this)` |
| `createBid(id)` | `nonReentrant` only — **no caller check** (L306) | **anyone** | `auctions[id].amount/bidder/endTime`, payable, refunds prior bidder |
| `settleAuction(id)` | `nonReentrant` only — **no caller check** (L337) | **anyone** | `auctions[id].settled`, transfers NFT, pays ETH |
| `cancelAuction(id)` | `onlyOwner` (L359), `nonReentrant`, only if zero bids | owner | `auctions[id].settled`, returns NFT |
| `setTimeBuffer` / `setMinBidIncrementPercentage` | `onlyOwner`, **unbounded** | owner | globals read live by open auctions |
| `setDefaultRoyalty` | `onlyOwner` | owner | ERC2981 default royalty |
| views (`imageData`, `hiResData`, `imagePointers`, `hiResPointers`, `pieceInfo`, `tokenURI`, `tokenJSON`, `supportsInterface`) | none / existence check | anyone | — |
| Inherited `transferOwnership` / `renounceOwnership` | `onlyOwner`, **one-step** (plain `Ownable`, not `Ownable2Step`), not overridden | owner | `Ownable._owner` |
| Inherited `approve` / `setApprovalForAll` / `transferFrom` / `safeTransferFrom`×2 | OZ internal auth checks | token owner/approved | ERC721 custody state |

**Roles**: exactly one — `owner()`. Granted at deploy (`Ownable(msg.sender)`), transferable/renounceable one-step, unlocks all 9 owner-gated functions above and is the live-read destination for auction proceeds/token-returns (see Finding #1).

**Unguarded (permissionless) state-changing entrypoints**: `createBid`, `settleAuction`, plus inherited `approve`/`setApprovalForAll`/`transferFrom`/`safeTransferFrom` (each scoped to tokens the caller owns/is approved for). No `receive()`/`fallback()`/withdraw function exists anywhere in the contract.

---

## Threat Model

| Actor | Reaches | Could gain | Status |
|---|---|---|---|
| `owner()` | `renounceOwnership`/`transferOwnership` mid-auction | Redirect or permanently burn auction proceeds/token | **Finding #1 (High)** |
| `owner()` | `setTimeBuffer`/`setMinBidIncrementPercentage` mid-auction | Neutralize anti-snipe protection or freeze out a bidder; or brick bidding entirely via overflow | **Finding #2 (Medium)** |
| `owner()` | `mint`'s `mimeType` argument | Inject arbitrary JSON keys into token metadata rendered by marketplaces | **Finding #3 (Medium)** |
| `owner()` | `setHiResData` while a token is escrowed/sold | Mutate token data after bidders have committed funds | **Finding #4 (Low)** |
| Any address | plain `transferFrom` a token directly to the contract | Accidentally/adversarially strand a token permanently | **Finding #5 (Low)** |
| WETH contract / misconfigured `weth` | uncapped `deposit`/`transfer` calls in the refund fallback | Block or silently drop a refund/payout | **Finding #6 (Low)**, invariant holds under a canonical WETH9 |
| Bidder-controlled contract | outbid-refund call | Force refund down the WETH path; impose bounded extra gas cost on the next bidder | **Finding #7 (Low)** — refund path itself is not blockable |
| Any address (well-funded) | repeated self-bidding | Delay settlement via anti-snipe re-extension | invariant holds — cost compounds 5%/round, self-limiting (see Leads) |
| Arbitrary caller | `settleAuction` | Nothing beyond gas cost — permissionless by design | invariant holds |

---

## Findings

[92] **1. `owner()` is read live at settlement — `renounceOwnership`/`transferOwnership` permanently burns proceeds or locks the NFT**

`AughtNaughts.settleAuction` / `AughtNaughts.cancelAuction` · Confidence: 92 · **[both phases, corroborated by 8 independent agents]**

**Description**
Every payout/return path calls `owner()` live instead of using a value snapshotted when the auction started, so a standard, unmodified `renounceOwnership()` (or a routine `transferOwnership()`) made while an auction is open silently redirects or destroys its outcome — with no revert, no event indicating loss, and no recovery function anywhere in the contract.

```solidity
// settleAuction() — no-bid path
if (a.bidder == address(0)) {
    _transfer(address(this), owner(), tokenId);   // L346 — reverts forever if owner()==address(0)
    emit AuctionSettled(tokenId, address(0), 0);
} else {
    _transfer(address(this), a.bidder, tokenId);   // L352
    _safeTransferETHWithFallback(owner(), a.amount); // L353 — succeeds trivially against address(0), burning funds
    emit AuctionSettled(tokenId, a.bidder, a.amount);
}
```

**Proof of Concept**
1. Owner starts an auction on `tokenId=7` (zero bids). Owner calls the inherited `renounceOwnership()`. `owner()` becomes `address(0)`. After `endTime`, anyone calls `settleAuction(7)`: L346's `_transfer(address(this), address(0), 7)` reverts every time (OZ v5 `ERC721._transfer` explicitly rejects `to==address(0)` with `ERC721InvalidReceiver`), and `cancelAuction` is `onlyOwner`-gated so no caller can ever satisfy it again — `tokenId=7` is permanently stuck in the contract with no rescue path.
2. Same setup but with a winning bid of `X` ETH. After the same `renounceOwnership()` call, `settleAuction` succeeds in transferring the NFT to the winner (L352, `a.bidder != address(0)`), but L353's `_safeTransferETHWithFallback(address(0), X)` executes `address(0).call{value: X, gas: 30_000}("")` — a plain value-call to a codeless address always returns `success = true` — so the entire winning bid is silently and irrecoverably burned, while `AuctionSettled` still emits as if the sale completed normally.
3. A non-renounce variant is equally real: owner calls plain, unguarded `transferOwnership(newAddr)` (routine key rotation, or a typo) while an auction is open — the next `settleAuction`/`cancelAuction` call sends the proceeds/token-return to `newAddr`, not to the address that actually ran the auction.

**Fix**

```diff
 struct Auction {
     uint96 amount;
     uint96 reservePrice;
     uint40 startTime;
     uint40 endTime;
     address bidder;
     bool settled;
+    address beneficiary;
 }
...
 function startAuction(uint256 tokenId, uint96 reservePrice, uint40 duration) external onlyOwner nonReentrant {
     ...
     auctions[tokenId] = Auction({
         amount: 0,
         reservePrice: reservePrice,
         startTime: start,
         endTime: start + duration,
         bidder: address(0),
-        settled: false
+        settled: false,
+        beneficiary: owner()
     });
     ...
 }
 // settleAuction / cancelAuction: replace every owner() read in a transfer/payout with a.beneficiary
```
Optionally also override `renounceOwnership()` to revert while any `auctions[id]` is started-and-unsettled, and/or adopt `Ownable2Step` for `transferOwnership`.

---

[78] **2. Global `timeBuffer`/`minBidIncrementPercentage` are not snapshotted per-auction — retroactively alter or brick in-flight auctions**

`AughtNaughts.createBid` · Confidence: 78 · **[both phases]**

**Description**
Unlike `reservePrice`/`endTime` (fixed into the `Auction` struct at `startAuction`), `timeBuffer` and `minBidIncrementPercentage` are global variables read live by every currently-open auction, with no upper bound on either setter. Two distinct mechanisms follow from this one gap:

- **(a) Fairness defeat**: the owner can set `minBidIncrementPercentage` to an extreme value (e.g. 255, the `uint8` max) mid-auction to price out a specific disfavored bidder, or set `timeBuffer=0` right before a bid lands inside the advertised anti-snipe window so the extension silently never fires — the exact interference `cancelAuction`'s `AuctionHasBids` guard exists to prevent, achieved via a side door those guards don't cover.
- **(b) Overflow DoS**: the owner (maliciously, or via a unit mistake — e.g. entering raw seconds where minutes were intended) sets `timeBuffer` to a large-but-legal `uint40` value. The very next bid that lands inside the (now enormous) anti-snipe window computes `uint40(block.timestamp) + timeBuffer`, which — because Solidity ^0.8.x applies checked arithmetic even to fixed-width types — overflows `uint40` and reverts, permanently blocking every future bid on that auction until the owner lowers `timeBuffer` again.

```solidity
bool extended = a.endTime - block.timestamp < timeBuffer;      // L322 — live global read
if (extended) {
    a.endTime = uint40(block.timestamp) + timeBuffer;           // L324 — checked-arithmetic revert if timeBuffer is large
    emit AuctionExtended(tokenId, a.endTime);
}
```

**Proof of Concept**
`type(uint40).max = 1,099,511,627,775`. At `block.timestamp ≈ 1,785,283,200` (2026-07-29), owner calls `setTimeBuffer(1_099_000_000_000)` — inside the legal `uint40` range, no bounds check exists in `setTimeBuffer`. On a normal 7-day auction (`duration=604800`), the very first bid satisfies `604800 < 1,099,000,000,000` (anti-snipe triggers), then computes `1,785,283,200 + 1,099,000,000,000 = 1,100,785,283,200`, which exceeds `type(uint40).max` by `1,273,655,425` — the addition reverts, the bid never lands, and the auction silently expires with zero bids/zero revenue.

**Fix**

```diff
 function setTimeBuffer(uint40 newTimeBuffer) external onlyOwner {
+    require(newTimeBuffer <= 1 days, "timeBuffer too large");
     timeBuffer = newTimeBuffer;
     emit TimeBufferUpdated(newTimeBuffer);
 }
```
Additionally, snapshot both `timeBuffer` and `minBidIncrementPercentage` into the `Auction` struct at `startAuction` time and reference the stored per-auction values in `createBid`, so a global parameter change cannot retroactively alter the terms of an already-running auction. (This mechanism is recoverable by the owner lowering the parameter again, unlike Finding #1's `renounceOwnership` path — hence Medium rather than High.)

---

[75] **3. `mimeType` is interpolated unescaped in `tokenJSON` — arbitrary JSON-key injection into token metadata**

`AughtNaughts.tokenJSON` · Confidence: 75 · **[both phases; escalated from an initial Low "malformed JSON" framing to a working injection PoC]**

**Description**
`name` and `description` are passed through `_escapeJSON` before being interpolated into the metadata JSON, but `mimeType` is spliced in raw:

```solidity
'","image":"data:',
p.mimeType,          // L267 — not escaped, unlike name/description above
";base64,",
Base64.encode(SSTORE2.readAll(p.pointers)),
'"}'
```

Because the base64 alphabet (`A-Za-z0-9+/=`) contains no characters requiring JSON escaping, a `mimeType` value can close the `"image"` string early and inject additional, attacker-chosen top-level keys that survive to the end of the object — a genuine metadata-injection primitive, not just JSON breakage.

**Proof of Concept**
Owner mints a token with `mimeType = 'image/png","external_url":"https://evil.example/claim","x":"y'`. `tokenJSON` renders:
```json
{"name":"...","description":"...","image":"data:image/png","external_url":"https://evil.example/claim","x":"y;base64,<b64-data>"}
```
This is valid JSON containing an injected `external_url` key that marketplaces render as a clickable link. The same technique via an `animation_url` key set to a `data:text/html,<script>...` URI would be rendered inside an iframe by marketplaces that support it — a stored-XSS/phishing surface against every viewer of that specific token. `mimeType` is owner-supplied at mint time with only an empty-string check (L183 — `mint()`), and is permanent thereafter (no fix-up path for any `Piece` field post-mint).

**Fix**

```diff
                 '","image":"data:',
-                p.mimeType,
+                _escapeJSON(p.mimeType),
                 ";base64,",
```
Or, more robustly, validate `mimeType` against a small allowlist of known content types at mint time.

---

[65] **4. `setHiResData` has no check that the token isn't currently escrowed in an active auction (or already sold)**

`AughtNaughts.setHiResData` · Confidence: 65 · **[depth phase only]**

**Description**
`setHiResData` only checks token existence (`_requireOwned`, L193) and the append-once flag (L195) — unlike `startAuction`, it never checks `ownerOf(tokenId)==owner()` (L286 in `startAuction`). A token escrowed for auction (`ownerOf==address(this)`) still passes `_requireOwned`, so the owner can attach hi-res companion data mid-auction, after bidders have already committed ETH having checked `hiResPointers(id).length==0`. Since hi-res data is not part of `tokenURI`/`tokenJSON` (only the canonical image is), a bidder relying solely on standard metadata gets no on-chain signal that this could still change. It is also reachable post-sale — the owner can attach hi-res data to a token now held by a third party, since nothing ties the write to `owner()` still holding it.

**Proof of Concept**
Token 9 is escrowed via `startAuction` at `t=0` (`endTime=3600`). Bidder B bids at `t=1000` after checking `hiResPointers(9).length==0`. At `t=3500`, owner calls `setHiResData(9, [newPointer])` — succeeds, since `_requireOwned(9)` only requires the token to exist (true throughout escrow), and `HiResAlreadySet` never fires since it was never previously set. `HiResSet` emits and `hiResPointers(9).length` flips to 1 before the auction (and B's earlier bid) ever closes.

**Fix**

```diff
 function setHiResData(uint256 tokenId, address[] calldata pointers) external onlyOwner {
     _requireOwned(tokenId);
+    require(ownerOf(tokenId) == owner(), "token escrowed or sold");
     Piece storage p = _pieces[tokenId];
```

---

[60] **5. Tokens sent to the contract via plain `transferFrom` (or ETH force-fed via `selfdestruct`) are permanently unrecoverable**

`AughtNaughts` (whole-contract — no rescue function) · Confidence: 60 · **[breadth phase]**

**Description**
The contract only escrows tokens via its own internal `_transfer` call inside `startAuction`, and only releases them via `settleAuction`/`cancelAuction`, both gated on that specific token's `Auction.startTime`. `safeTransferFrom` into the contract is self-protecting (no `onERC721Received` implementation → OZ's receiver check reverts), but plain `transferFrom` performs no receiver check at all, so any current holder can send a token directly to `address(this)`. `ownerOf` becomes `address(this)`, but `auctions[id].startTime` is unaffected — `startAuction` then fails (`ownerOf(id)!=owner()`) and `settleAuction`/`cancelAuction` also fail (`AuctionNotActive`) — the token is stranded with no rescue path anywhere in the contract. The same applies symmetrically to ETH force-fed via `selfdestruct`/coinbase.

**Proof of Concept**
Holder of minted, never-auctioned token #5 calls `transferFrom(holder, address(AughtNaughtsContract), 5)` — succeeds (no receiver check). `startAuction(5,...)` now reverts (`TokenNotHeldByOwner`); `settleAuction(5)`/`cancelAuction(5)` both revert (`AuctionNotActive`). Token #5 is permanently locked with no function able to move it.

**Fix**: override the OZ v5 `_update` hook to revert transfers where `to==address(this)` outside the contract's own internal escrow call, or add a guarded `onlyOwner` rescue function scoped to non-actively-escrowed tokens.

---

[55] **6. `weth` address is unvalidated at construction; its `transfer()` return value is unchecked**

`AughtNaughts` constructor / `_safeTransferETHWithFallback` · Confidence: 55 · **[both phases]**

**Description**
`weth` is `immutable`, set directly from the constructor argument with no zero-address or codesize check (L142). `IWETH.deposit()` has no return value, so a call against a no-code address "succeeds" silently (funds lost); the next line, `IWETH(weth).transfer(to, amount)`, declares a `bool` return, so Solidity's implicit codesize check reverts the whole refund/settlement transaction if `weth` has no code — contradicting the code's own comment that "refunds and payouts can therefore never revert." Separately, even against a contract with code, `transfer`'s boolean return is never checked (L397), so a non-reverting "return false on failure" token would silently strand funds with no recovery function. Under the stated trust assumption — a genuine, canonical WETH9 — this is not concretely exploitable: `deposit`/`transfer` never fail in the paths this contract exercises, and every phase-2 agent that traced this confirmed the same.

**Proof of Concept**: deploy with `weth_` misconfigured (wrong chain's address, or an EOA). Any refund/payout whose primary 30k-gas ETH send fails (common for any contract-wallet recipient lacking a trivial `receive()`) falls through to the WETH path and reverts unrecoverably for that auction instance — immutable `weth`, no admin override, no way to un-stick it.

**Fix**: validate `weth_ != address(0) && weth_.code.length > 0` in the constructor; check `IWETH.transfer`'s return value (or use `SafeERC20.safeTransfer`).

---

[52] **7. Bounded gas-griefing via uncapped returndata copy in the refund call**

`AughtNaughts._safeTransferETHWithFallback` · Confidence: 52 · **[breadth phase]**

**Description**: `to.call{value: amount, gas: 30_000}("")` (L394) has its return data copied by Solidity's high-level `.call` even though the second tuple element is discarded. A malicious former-bidder contract can `return` a large memory block within its ~32.3k gas budget (pure memory-expansion, no writes needed), imposing the `RETURNDATACOPY` cost on the transaction of whoever outbids them next — a bounded but real gas-cost griefing/deterrent vector, not a fund-loss path.

**Fix**: use inline assembly for the refund call (`call(...)` with a zero-size output buffer) to skip the returndata copy regardless of callee behavior.

---

Findings List

| # | Confidence | Title |
|---|---|---|
| 1 | [92] | `owner()` read live at settlement — renounce/transfer ownership burns proceeds or locks the NFT |
| 2 | [78] | Unsnapshotted `timeBuffer`/`minBidIncrementPercentage` — fairness defeat + overflow DoS |
| 3 | [75] | `mimeType` unescaped — JSON metadata injection |
| 4 | [65] | `setHiResData` mutable mid-auction / post-sale |
| 5 | [60] | Tokens/ETH sent outside the intended flow are permanently unrecoverable |
| 6 | [55] | `weth` address unvalidated; `transfer()` return unchecked |
| 7 | [52] | Bounded gas-griefing via uncapped returndata copy |

---

## Informational

- **Integer-division floor nullifies the bid-increment percentage for near-zero bid amounts** — `createBid()` L311: `(uint256(a.amount) * minBidIncrementPercentage) / 100` floors to 0 whenever `a.amount < 20` wei under the default 5% setting, collapsing the increment rule to "strictly greater by ≥1 wei" in that dust range. Economically negligible and self-limiting after a few bids.
- **`a.amount = uint96(msg.value)` is an unchecked narrowing cast** — `createBid()` L318. `type(uint96).max` ≈ 7.9×10^28 wei ≈ 79.2 billion ETH, ~600x the entire circulating ETH supply — not reachable with any real `msg.value`. Every agent across both phases that examined this (7 total) confirmed it unreachable. Defense-in-depth only: `SafeCast.toUint96`.
- **`pragma ^0.8.24` defaults to the `PUSH0` opcode** — L30. The constructor's own NatSpec implies multi-chain deployment intent; some L2s/alt-EVMs may lack `PUSH0` support. Pin `--evm-version` explicitly per target chain.

---

## Leads

_Vulnerability trails with concrete code smells where the full exploit path could not be completed in one analysis pass. These are not false positives — they are high-signal leads for manual review. Not scored (confidence &lt; 50)._

- **Pointer-provenance / metamorphic-contract risk** — `AughtNaughts.mint` / `setHiResData` — Code smells: the only validation on owner-supplied SSTORE2 pointer arrays is `totalSize` reverting on malformed data, with no on-chain linkage to actual `writeChunks` output. Requires an out-of-scope assumption about the (absent) SSTORE2 implementation plus a deliberately malicious or careless owner; not independently verified.
- **Repeated self-bidding to indefinitely delay settlement** — `AughtNaughts.createBid` — Code smells: the anti-snipe extension has no hard cap on cumulative extensions. Cost compounds at ≥5% per round, making this self-limiting rather than a free griefing vector; flagged for completeness, not independently verified as profitable or practical.
- **`_escapeJSON` does not cover all JSON control characters** — `AughtNaughts._escapeJSON` — Code smells: only `"`, `\`, LF, CR, TAB are escaped; RFC 8259 requires all of U+0000–U+001F. Unverified whether any real downstream JSON parser in this project's actual consumer set is strict enough to reject the remaining unescaped control bytes.

---

> ⚠️ This review was performed by an AI-driven, multi-agent audit pipeline (context mapping → checklist breadth pass → independent attacker-mindset depth pass → cross-phase reconciliation). AI analysis can never verify the complete absence of vulnerabilities and no guarantee of security is given. A human security review, and monitoring after deployment, are strongly recommended before this contract handles material value.