Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 125 additions & 0 deletions .claude/prompts/async-lifecycle-review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
# Security review: the asynchronous lifecycle

For any contract whose code is reached by an inbound cross-chain message, defers work to a queue,
or is called back later: hooks, managers, adapters, request managers, snapshot and price
publishers.

Find every place this contract **assumes something about when, whether, in what order, or how many
times it runs** — and every place its own failure stops more than itself.

## Why this class matters here

Centrifuge core is asynchronous by construction, and it gives fewer guarantees than the code
built on it tends to assume. Four properties do most of the damage.

**1. Delivery is at-most-once, but unordered and indefinitely deferrable.** A message cannot execute
twice: each adapter delegates replay protection to its transport (Axelar's `validateContractCall` is
one-shot per command id, and the others are equivalent), and the protocol keeps no dedup of its own
because it assumes the transport provides it. So do not go looking for double execution.

What is *not* guaranteed is when, or whether, or in what order. A message that reverted is stored
indefinitely and anyone may `retry()` it with no deadline; underpaid messages queue for anyone to
`repay()` later. Nothing establishes that a later message supersedes an earlier one, and config
payloads carry no version or nonce. So a message created under one configuration can first execute
under a completely different one, months later, at a moment its sender does not choose — which is a
different hazard from replay and needs different questions.

**2. A batch is atomic exactly as far as its messages cannot fail.** Managers bundle messages that
are only correct together, and the design intends them to land together — that is a fair assumption
to write code against. But each message runs in its own `try/catch`: one that reverts is caught and
parked while the rest of the bundle proceeds. So the assumption holds only while no message in the
bundle *can* fail, and it silently stops holding the moment one can.

The question is therefore not "can an attacker split this bundle" but **"can any message in it
revert, and what do the others assume about it having landed?"** Gas is one way a message fails, and
mis-benchmarked gas limits are an accepted risk rather than something to hunt; a message that reverts
on its own logic is the more interesting case.

**3. Your revert is not local.** Hooks and managers run *inside* inbound message processing, under
a fixed `GasService` allowance, inside the gateway's `try/catch`. A revert or gas overrun does not
fail your function in isolation — it parks the message in `failedMessages`, and where messages are
ordered by a snapshot nonce, everything behind it stops too. The escape hatch may not exist for the
exact case you blocked: a guard that rejects one legitimate value can wedge a stream with no way to
pass that single item without relaxing the limit for everyone.

**4. Core publishes on every write.** `updateHoldingValue` journals and immediately runs the
snapshot hook; on a same-chain deployment `notifySharePrice` lands synchronously with no message or
delay. Core has no notion of "these writes are one economic change, settle them together". If you
model one change as two writes, the intermediate state is not merely observable — a sync-deposit
vault will transact on it.

## Calibration: real instances

- *A bundle split by a failure.* An approve and its issuance were bundled; the second could revert on
its own, and the first had already landed, leaving a state the sender never intended to be
observable.
- *Stale supersession.* A replayed `SetPoolAdapters` rolled a route back to a superseded adapter
set; a stale oracle retry overwrote a newer price, and redemptions were then blessed at it.
- *Wedged stream.* A legitimate NAV swing tripped a guard, the revert unwound the snapshot nonce,
and every later accounting message failed `InvalidNonce` — retry never cleared it.
- *Gas as a weapon.* A hook staticalled `poolId()` on a *user* address with no gas cap, so a holder
whose `poolId()` burned gas inflated `freeze()` from ~46k to ~1.56M and made freeze messages fail
— the attacker watching for the failure event and moving tokens before the retry.
- *Partial publication.* An asset leg and its offsetting liability leg were repriced in two
transactions. NAV was unchanged at the end, but between them the published share price moved
~10%, and a sync deposit in that window minted at a false price, irreversibly.

## What to examine

1. **Reorder, delay, never.** For each message or deferred action this contract handles: if it lands
out of order, six months late, or not at all, what does it overwrite and what waits on it? Can the
receiver prove it is not superseded — a nonce, a monotonic timestamp, a version? If the answer is
"the sender won't do that", find what enforces it. Do not ask what happens if it arrives twice;
the transport prevents that.
2. **Configuration drift across the gap.** What did this action assume at creation time that a pool
can change before execution — a manager, an adapter set, a hook, a price, a role? A revoked
permission that is still sitting in a retryable failed message is a live grant.
3. **Pairings.** Which correctness properties depend on two messages or two writes landing
together? What stops a third party executing them separately, or under-gassing one?
4. **Your revert set.** Enumerate every way this hook can revert or exceed its gas allowance,
including on attacker-chosen input and optional interface members that may not exist. For each:
what stops in core, not here? Does a nonce stream stall? Is there a bounded path for an operator
to pass one legitimate item without disabling the check?
5. **Worst-case gas.** Any unbounded loop, any call into a caller-influenced address without a gas
cap, any return-data-sized cost. Compare against the `GasService` allowance for that message.
6. **Publication points.** Which of this contract's writes cause core to publish a price or NAV
downstream? Between any two of them, is there a state this contract considers incomplete but
which a sync vault, an oracle reader, or another pool can transact on?
7. **Zero and unset as legal states.** A price of zero is permitted and a fresh share class is
`(0, 0)`. If this contract early-returns on zero, or arms a baseline only on a non-equal write,
does its check go blind exactly when pricing is broken?

## Method

- Read the whole flow from message receipt to final state write, including the failure branch.
These bugs live in what happens *after* the happy path returns.
- For timing claims, name the two orderings and say which core mechanism forbids the bad one. If
none does, that is the finding.
- Distinguish a liveness bug (something wedges) from a value bug (something is lost or mispriced),
and say which — both matter here, and they have different fixes.

## Before you report: check it is not already accepted

`docs/audits/out-of-scope/` holds the accepted-design list handed to the bug bounty, and the
protocol has been through more than thirty audits. Most well-formed candidates you find are already
known. Before writing up anything:

- Read the out-of-scope list for the release under review, and say for each candidate which bullet
does or does not cover it.
- A bullet that acknowledges a *mechanism* but scopes its consequence to the pool's own liveness,
its own users, or its own configuration does **not** cover a loss that crosses to another pool or
another user. Say so explicitly and keep the finding.
- Where a comment in the code states an invariant, check the code establishes it. A stated
precondition that nothing enforces is itself worth reporting, separately from any exploit.

Reporting a known item as new costs the reader more than missing it, so state the scope argument
for every finding rather than leaving it implied.

## Output

Per finding, severity order: **Title**; **Scenario** (the precise ordering, timing or gas condition,
with `file:line`); **What breaks** — say whether value is lost or the pipeline stalls, and what
else stops with it; **Who can trigger it, and is it permissionless**; **Fix**.

Report suspected-but-incomplete paths separately as *unconfirmed*, naming what you could not
establish.
130 changes: 130 additions & 0 deletions .claude/prompts/integration-boundary-review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
# Security review: the integration boundary

For contracts that core calls into, or that call into core on someone's behalf:
`fromHub`/`fromSpoke` targets, transfer hooks, valuations, registrars, request managers,
snapshot hooks, routers, and anything holding a pool role.

Find every place this contract **trusts something core handed it without checking**, or
**lends its own authority to a caller who does not have it**.

## Why this class matters here

Core's authorisation stops at its own edge. It proves *that* a call was authorised for a pool;
it does not prove what the call points at, who stands behind it, or that the transaction context
you are running in is still yours. Three specific properties of Centrifuge core create the traps.

**1. Routing is a tuple, but authorisation is one field of it.** Core routes by
`(poolId, scId, assetId)` and authorises by `poolId` alone. `IManagerCall` says it outright: the
call is "pool-scoped: any `scId` is encoded in `payload`". So core hands you an authenticated
`poolId` and an **unvalidated `scId` inside opaque bytes**, and expects you to self-validate.
Registry keys (a vault address, a token address) are global, while the authority writing them is
pool-scoped.

There is a rule that decides whether you must check:

- If you **key state** by the identifier — `state[poolId][scId]` — an unvalidated `scId` only
pollutes that pool's own namespace. Safe by construction.
- If you **resolve an object** from the identifier — a token, a vault, a manager, an escrow — and
then act on it, you have left your pool's namespace and must re-validate what came back
against the tuple you were authorised for.

Nearly every finding in this class is a contract that resolved and forgot to check.

**2. Core sees your contract, not your caller.** Permissions are address-based:
`wards[msg.sender]`, `isManager(poolId, msgSender())`. Once a pool grants your contract a role,
core cannot tell who called *you*, and it does not check that an `owner` / `from` / `controller` /
`receiver` you name has any relationship to your caller. Core also endorses periphery routers
wholesale, so every call such a router makes looks, to core, exactly like the user's own.

**3. The transaction context is ambient and outlives your call.** `BatchedMulticall` caches the
initiating caller in `_sender` for the whole batch, and `msgSender()`/`msgValue()` substitute that
cache — the caller, and a zero value — in place of the real ones. `Gateway` keeps `isBatching`, fuel
and payment mode the same way.

Read the substitution condition before reasoning about it: both are gated on
`msg.sender == address(gateway)`, so a callee that re-enters from anywhere else sees the real
`msg.sender` and the real value. That gate is doing the security work, and it is the thing to check
in any contract that keeps its own equivalent — a cached principal without such a gate is the bug.

What core does *not* do is close the context before handing control to an untrusted address, so the
batch stays open across every hook, adapter, refund and token callback made inside it. Historically a
batching `send()` also reported a zero cost, which let payment checks pass for free; that is fixed —
`Gateway.send` returns nothing today — but it is the shape to watch for in any value core reports
back to you mid-batch.

## Calibration: real instances

- *Under-checked tuple.* `OnOfframpManager.update()` validated `poolId` and `msg.sender == spoke`
but discarded `scId`, so a message authorised for one share class could be aimed at another
class's manager and rewire its ramps and relayers. The same shape recurs from 2023 to 2026
across three auditors — it is the signature Centrifuge integration bug.
- *Borrowed authority.* After a router was endorsed, anyone could call
`router.requestDeposit(..., controller: attacker, owner: victim)`: the router was an operator of
the victim, so nothing fired. Rated Critical.
- *Unpinned address.* A permissionless helper transferred a flash loan to whatever manager address
the caller named and then called back into it, so a malicious "pool" satisfied every callback
check while holding the funds.
- *Context outliving the call.* An ERC-20 re-entered a withdrawal while `_sender` still held the
manager, so `isManager()` still returned true and the whole escrow was reachable.

## What to examine

For every externally reachable function, and every call this contract makes into core:

1. **Every identifier received.** Which are compared against something, and which are merely used?
For each unvalidated one, ask who is authorised to supply it. Apply the key-vs-resolve rule
above: if the identifier selects an object you then act on, find the check — or the finding.
2. **Every address received as an argument** that this contract then sends value to, calls into,
or trusts the answer of. What on-chain fact proves it is the canonical instance for that pool —
a factory `getAddress`, a registry lookup — and is that fact checked *here*, or assumed to have
been checked by the caller?
3. **Every principal named in calldata** (`owner`, `from`, `to`, `controller`, `receiver`,
`refund`, `reserver`). Is it derived from the authenticated initiator, or taken on trust? If
this contract holds a role, does a permissionless entrypoint forward straight into it?
4. **The obligations core states but cannot enforce.** `IManagerCall` requires `msg.sender ==
envoy`; `fromSpoke` additionally "MUST validate `(centrifugeId, sender)`" because it bypasses
the pool's policy entirely. These are prose, not types. Verify each one is present.
5. **Every external call, and what is still open behind it.** What privileged or transient context
is live — cached sender, open batch, un-zeroed accounting — and what could a re-entrant call do
with it? Check effects are written before the call, not after.
6. **Anything read from core whose meaning changes under batching.** `msgValue()` returns zero for
the whole window, and `isBatching` is observable and flippable by any caller. Does any accounting,
refund or limit here depend on a value that differs inside a batch?
7. **Discriminated payloads.** If a `kind`/`selector` is decoded, what happens on an unrecognised
value — revert, or fall through as a silent no-op?

## Method

- Work from the code. A comment asserting a check exists is a hypothesis; find the line.
- Compare siblings. Where several contracts implement the same core interface, diff their guards
against each other — the odd one out is usually the finding, and the check the others perform is
the one core could not enforce.
- For any candidate, name the entry point, the authentication that passes, the identifier or
address that is not verified, and the state or transfer that results. Without all four you do
not have a finding.

## Before you report: check it is not already accepted

`docs/audits/out-of-scope/` holds the accepted-design list handed to the bug bounty, and the
protocol has been through more than thirty audits. Most well-formed candidates you find are already
known. Before writing up anything:

- Read the out-of-scope list for the release under review, and say for each candidate which bullet
does or does not cover it.
- A bullet that acknowledges a *mechanism* but scopes its consequence to the pool's own liveness,
its own users, or its own configuration does **not** cover a loss that crosses to another pool or
another user. Say so explicitly and keep the finding.
- Where a comment in the code states an invariant, check the code establishes it. A stated
precondition that nothing enforces is itself worth reporting, separately from any exploit.

Reporting a known item as new costs the reader more than missing it, so state the scope argument
for every finding rather than leaving it implied.

## Output

Per finding, severity order: **Title**; **Path** (entry point → what authenticated → what went
unchecked → the resulting effect, with `file:line` at each step); **Who can reach it**; **Impact**;
**Fix** (the smallest check that restores the invariant).

Report anything you suspect but cannot complete separately as *unconfirmed*, naming the specific
thing you could not establish. A single confirmed path is worth more than ten speculative ones.
Loading
Loading