# Audit — `fakfun-wallet-v18`: new smart-router trading + USDCx/sBTC swap

Scope per the brief: **only the diff from v17**, plus the four supporting contracts.
All sources fetched from the chain (`api.hiro.so/v2/contracts/source`) under
`SPV9K21TBFAK4KNRJXF5DFP8N7W46G4V9RCJDC22`. Line numbers refer to that source.

## The diff, established first

A token-level (whitespace-insensitive) diff of v17 → v18 via `difflib` shows exactly
**five semantic changes**; the remaining ~35 unified-diff hunks are pure reformatting
(argument wrapping in `contract-call?`, trailing commas in tuple literals).

1. `:11` — `(use-trait smart-trait '…faktory-smart-trait-v1.smart-trait)`
2. `:33` — `(define-constant err-router-not-approved (err u4033))`
3. `:1220-1389` — private `authorize-smart` plus the four public entries
   `smart-buy-sbtc` (op `0x00`), `smart-buy-stx` (`0x01`), `smart-sell-sbtc` (`0x02`),
   `smart-sell-stx` (`0x03`), inserted between `get-byte` and `faktory-execute-limit`
4. `:2388-2390` in `onboard` — `usdcx-sbtc-swap` added to `whitelisted-extensions`
5. `onboard` — registered principal moved from `fakfun-wallet-v17` to `-v18`

No existing v17 function is semantically modified. Findings below are confined to the new code.

---

## Findings, ranked

### 1. The four new entries do not check `token-lock-enabled` (low)

**Where.** `smart-buy-sbtc` 1295-1303, `smart-buy-stx` 1322-1330, `smart-sell-sbtc`
1351-1359, `smart-sell-stx` 1380-1388; `authorize-smart` 1237-1276.

`token-lock-enabled` is the wallet's brake against a compromised passkey. The assert is
present in every comparable path:

```clarity
903:  (asserts! (not (var-get token-lock-enabled)) err-token-locked)   ;; sip010-transfer
```

and likewise at `760` (`stx-transfer`), `849` (`extension-call`), `998`
(`sbtc-initiate-withdrawal`), `1104` (`sip009-transfer`), and unconditionally at `1411`
(`faktory-execute-limit`). In the first five it sits **inside the passkey branch** — the
admin branch is exempt by design.

It appears **nowhere** in `authorize-smart` or in any of the four new entries.

**Impact.** With the token lock engaged, a passkey signature still moves SIP-010 out of the
wallet through an approved router (and sBTC/STX via the buy entries). The owner's emergency
brake does not cover the new trading surface.

**Sequence.**
1. Owner suspects passkey compromise, calls `toggle-token-lock` with `enabled=true` (`391`).
2. Attacker calls `sip010-transfer` → fails `err-token-locked u4023` (`903`). Brake works.
3. Attacker calls `smart-sell-sbtc(smart=<any approved router for that token>,
   token=<token>, token-name=<name>, amount=<full balance>, min-out=u1,
   sig-auth=<their signature>)`. No token-lock check anywhere on the path;
   `authorize-smart` passes; tokens leave to the router.
4. With `min-out=u1` the attacker recovers the rest by sandwiching the pool.

**Honest severity note.** The v17 function `faktory-execute` (v18 1137-1211, untouched by
the diff) also lacks this assert, so the *class* is pre-existing; the new entries extend it
to a new router set and new assets. That is why this is filed **low**, not medium.

**Fix.** Add `(asserts! (not (var-get token-lock-enabled)) err-token-locked)` to the passkey
branch of `authorize-smart` (before 1247), or unconditionally after the registry check
(after 1244) to match `faktory-execute-limit:1411`. Close the same gap in `faktory-execute`.

---

### 2. Registry owner gating uses `tx-sender`, not `contract-caller` (low)

**Where.** `fakfun-smart-router-registry.clar:39, 53, 67, 79` — `propose-router`,
`confirm-router`, `revoke-pending`, `propose-owner`.

All four owner-gated writes authorise on the transaction **origin**. Any contract the owner
principal calls in an ordinary transaction can, within that same transaction, call back into
the registry and pass the owner check — classic `tx-sender` confusion.

**Impact is bounded by the cooldowns**, and I want to state that plainly rather than inflate
it: a hostile `propose-router` only starts a clock and needs a second owner interaction ≥144
blocks later to confirm (`54`, also `tx-sender`-gated). A hostile `propose-owner` (`79`, no
delay on propose) needs the legitimate owner not to notice for 144 blocks before
`accept-owner` (`94`) lands, and the owner can overwrite a hijacked pending-owner at any time.

**Fix.** Use `contract-caller` at 39, 53, 67, 79, or assert both:
`(and (is-eq tx-sender owner) (is-eq contract-caller owner))`. Ownership of a global
allowlist would also be better behind a multisig than a single EOA.

---

### 3. Pending router proposals never expire and survive an ownership change (low)

**Where.** `fakfun-smart-router-registry.clar:54` (lower bound only), `51-63` (`confirm-router`),
`91-105` (`accept-owner`).

The cooldown has a lower bound and **no upper bound**, so a proposal made months ago can be
confirmed in any future block. Clarity maps are not enumerable, so there is no on-chain way
to inventory what is pending — only off-chain reconstruction from `propose-router` print
events, which have no matching "expired" event.

**Impact.** Monitors cannot treat "nothing proposed in the last 144 blocks" as "nothing can
go live now" — effective notice before a router activates can be zero. Worse,
`accept-owner` (91-105) clears only `pending-owner`; the `pending` map is untouched, so
every proposal created by a previous — possibly compromised or rotated-out — owner survives
the handover and stays confirmable.

**Fix.** Add an upper bound so proposals age out:
`(asserts! (and (>= burn-block-height (+ proposed-at COOLDOWN)) (< burn-block-height (+ proposed-at WINDOW))) ERR-COOLDOWN)`.
Store the proposing owner alongside the timestamp and reject confirmation when the current
owner differs, or clear `pending` on `accept-owner`.

---

### 4-7. Informational

- **The sell challenge does not bind `token`/`token-name`.** `authorize-smart` (1220-1236)
  never receives them, so the signed hash (`smart-execute-auth-helper:23-32`) does not cover
  the asset whose allowance is opened at 1356/1385. **No exploitable path found**: the trait
  `sell-for-sbtc (uint uint uint bool)` takes no token argument, so each router pulls its own
  hard-wired token; a substituted token is simply not covered by the allowance and the call
  aborts (fail-closed, state rolled back, signature not burned). It becomes dangerous only if
  a multi-token router is ever approved. Fix: include `(contract-of token)` and `token-name`
  in the hashed tuple.
- **The wallet does not compare the realised output against the signed `min-out`**, unlike
  native `faktory-execute-limit`. Slippage protection is fully delegated to the router.
  Defence-in-depth, no live path to loss.
- **Gas contract is not bound by the signature** — the bearer of a valid challenge chooses
  the recipient of the sBTC gas allowance up to `max-gas-amount`.
- **No revocation, no pause, registry address hard-coded** in the wallet. Pre-declared in the
  brief, so noted rather than filed as a vulnerability.

---

## Invariants verified (54 checked; the load-bearing ones)

- **Registry gate precedes both branches.** The `asserts!` at 1238-1244 runs before
  `match sig-auth` at 1245, so an unapproved router is rejected in the passkey *and* admin
  branches before any signature work. **No path routes an unapproved trait-conforming router.**
- **Op confusion is impossible.** Each entry passes a hard-coded literal — `0x00` (1297),
  `0x01` (1324), `0x02` (1353), `0x03` (1382); there is no caller-supplied `op`. The field is
  part of a typed `to-consensus-buff?` tuple, so it serialises with type tag and length —
  no field-boundary ambiguity. A buy signature cannot authorise a sell.
- **Router replay is closed.** `smart` (the router principal) is in the hash
  (`helper:28`), and the same value feeds both the registry check (1238-1244) and the dispatch
  (1301/1328/1357/1386) — no TOCTOU between check and call.
- **Amount/parameter replay is closed.** `amount`, `min-out`, `fak-ratio`, `flag` are all
  hashed (`helper:29-32`); changing any yields a different message hash.
- **Signature reuse is closed twice over.** `consume-signature` (1886-1908) requires both
  `used-pubkey-authorizations[message-hash]` and `used-assertions[assertion-id]` to be absent
  — where `assertion-id = (sha256 (concat authenticator-data signature))` (1894) — and writes
  both (1904-1905).
- **Cross-wallet and cross-chain replay closed.** The domain hash carries
  `wallet: contract-caller` (`helper:8`) and `chain-id` (`helper:7`); v18 calls the helper
  directly (1248-1250), so no intermediary distorts `contract-caller`.
- **Admin branch** (1271-1274) resolves to `is-admin-calling tx-sender` (629, 633-635):
  an unsigned call requires the admin principal. No bypass found.
- **`SIP018_MSG_PREFIX`** = `0x534950303138` = ASCII `"SIP018"` (`helper:1`), conforming to SIP-018.

**Noted, not filed:** `auth-id` is not a sequential nonce — it is never compared or stored
anywhere in v18. Signatures are therefore executable out of order and with gaps. Not
exploitable on its own, since every economically meaningful field is bound, but it compounds
the absence of an expiry field.

---

## Gaps — what I could not check (30 declared; the material ones)

1. **No test was executed.** No Clarinet SDK in the environment, no stxer mainnet-fork run.
   Everything is source reading and hand analysis. The fail-closed behaviour asserted in
   finding 4 is reasoned from the trait signature, **not** confirmed by execution.
2. **The nine approved routers' sources were not read.** Finding 4's "no exploitable path"
   rests on every router hard-wiring a single token. I could not verify that for all nine.
3. **`smart-wallet-standard-auth-helpers-v7/v10` not provided**, so I cannot prove full
   domain/topic incompatibility with other challenge builders — only that this builder's
   topic is `"smart-execute"` (`helper:24`).
4. **Bitflow DLMM internals not read**, so `usdcx-sbtc-swap` behaviour under partial fill is
   assessed from its own source only.

---

## Method

Four independent passes over the deployed source, each preceded by an explicit v17→v18 diff
so no finding could be filed against pre-existing code. Every candidate then went to a
reviewer instructed to refute it, required to confirm quoted lines verbatim **and** that the
code is genuinely new in v18. **14 of 21 candidates were dropped** at that stage — several
because the finder had missed a guard a few lines away, others because the code predated v18.
What survives is above, with severities argued down where the pre-existing-class argument applies.