From 5e6721f797b7a86c6c460c1e566c50545a103196 Mon Sep 17 00:00:00 2001 From: aelmanaa Date: Mon, 17 Aug 2026 13:32:43 +0100 Subject: [PATCH] A stale roles{} anchor now refuses instead of silently snapshotting the replaced contract, with REANCHOR=true as the explicit opt-in. --- Makefile | 4 +- docs/config-schema.md | 2 +- docs/deployed-addresses.md | 2 +- docs/primitives/catalog.json | 2 +- docs/roles.md | 16 ++ docs/troubleshooting/index.md | 26 +++ script/config/VerifyChain.s.sol | 63 +++++++- src/roles/RolesSnapshot.sol | 197 +++++++++++++++++++++-- test/config/VerifyChainAnchorDrift.t.sol | 90 ++++++++++- test/roles/RolesAnchorReanchor.t.sol | 172 ++++++++++++++++++++ 10 files changed, 548 insertions(+), 26 deletions(-) create mode 100644 test/roles/RolesAnchorReanchor.t.sol diff --git a/Makefile b/Makefile index 9d75ca2..e897410 100644 --- a/Makefile +++ b/Makefile @@ -57,7 +57,7 @@ GROUP_DIR := $(if $(GROUP),$(GROUP)/,) # still apply. DEPLOY_VARS := TOKEN_NAME TOKEN_SYMBOL TOKEN_DECIMALS TOKEN_MAX_SUPPLY TOKEN_PRE_MINT \ TOKEN_PRE_MINT_RECIPIENT CCIP_ADMIN_ADDRESS ROLES_RECIPIENT TOKEN TOKEN_POOL LOCK_BOX DECIMALS \ - POOL_HOOKS AUTHORIZED_CALLERS FORCE_REDEPLOY + POOL_HOOKS AUTHORIZED_CALLERS FORCE_REDEPLOY REANCHOR $(foreach v,$(DEPLOY_VARS),$(if $(strip $($(v))),$(eval export $(v)))) # Preflight per-call inputs, forwarded to the forge script the same conditional way as DEPLOY_VARS @@ -327,7 +327,7 @@ deploy-new-chain: tools ## Guided deploy: add-chain -> deploy-token -> deploy-po # exit-remap note as sync-check: the 0/1/2 contract lives in `script/config/roles-check.sh`; CI calls # the script directly, `make roles-check` is pass/fail only. -snapshot-chain: tools ## Backfill the declared roles{} authority block FROM chain (CHAIN= required; GROUP= scopes to one token group; opt: TOKEN= TOKEN_POOL= TAR= SCAN_FROM_BLOCK=) +snapshot-chain: tools ## Backfill the declared roles{} authority block FROM chain (CHAIN= required; GROUP= scopes to one token group; opt: TOKEN= TOKEN_POOL= TAR= SCAN_FROM_BLOCK= REANCHOR=true) $(if $(CHAIN),,$(error CHAIN is required: make snapshot-chain CHAIN=)) $(require-chain-config) FOUNDRY_PROFILE=sync PROJECT_GROUP="$(GROUP)" forge script script/config/SnapshotChain.s.sol $(call evm-version-flag,$(CHAIN)) --sig "run(string)" "$(CHAIN)" diff --git a/docs/config-schema.md b/docs/config-schema.md index 79f5f33..c7f201d 100644 --- a/docs/config-schema.md +++ b/docs/config-schema.md @@ -615,7 +615,7 @@ engine never assumes one: "token": { "address": "0xa1f7882a...", // the token this block describes (the snapshot/audit anchor; // make doctor WARNs when it diverges from addresses.active.token - // - re-anchor after a repoint with make snapshot-chain) + // - a plain snapshot-chain refuses; re-anchor with REANCHOR=true) "type": "crosschain", // crosschain | burnmint | factory | byo - selects the admin model "ccipAdmin": "0xGov...", // getCCIPAdmin() - the TAR registration authority (one-step, owner-gated) "defaultAdmin": "0xGov...", // crosschain only: defaultAdmin() (single-holder, two-step) diff --git a/docs/deployed-addresses.md b/docs/deployed-addresses.md index 9dfa46b..22789e3 100644 --- a/docs/deployed-addresses.md +++ b/docs/deployed-addresses.md @@ -40,7 +40,7 @@ Two terms, kept distinct throughout these docs: | Run-time divergence notice | **Warner** | When an env override differs from `active.`, a broadcasting script prints both values + the exact `make adopt-token …` to reconcile | | `make doctor` TAR rung | **Warner** | Compares `active.tokenPool` against the on-chain TokenAdminRegistry and WARNs on divergence | | `make doctor` registry rung | **Warner** | WARNs when `deployments{}` holds more than one token pool while `active.tokenPool` points at one (the multi-token ambiguity), naming a token `GROUP=` as the durable fix and the `{CHAIN}_TOKEN_POOL` override as the one-off | -| `make doctor` roles rung | **Warner** | WARNs when a `roles.token/pool.address` anchor diverges from `addresses.active.` (a repoint after the snapshot) - re-anchor with `make snapshot-chain` | +| `make doctor` roles rung | **Warner** | WARNs when a `roles.token/pool.address` anchor diverges from `addresses.active.` (a repoint after the snapshot). A plain `make snapshot-chain` REFUSES here; re-anchor with `REANCHOR=true make snapshot-chain CHAIN=` | **Env overrides are READ-ONLY inputs: an env-driven run never writes the store.** An override changes only what a single run resolves; to make a value the durable default, adopt it (`make adopt-token`). The diff --git a/docs/primitives/catalog.json b/docs/primitives/catalog.json index 16e3802..a4956a8 100644 --- a/docs/primitives/catalog.json +++ b/docs/primitives/catalog.json @@ -1123,7 +1123,7 @@ }, { "target": "snapshot-chain", - "help": "Backfill the declared roles{} authority block FROM chain (CHAIN= required; GROUP= scopes to one token group; opt: TOKEN= TOKEN_POOL= TAR= SCAN_FROM_BLOCK=)" + "help": "Backfill the declared roles{} authority block FROM chain (CHAIN= required; GROUP= scopes to one token group; opt: TOKEN= TOKEN_POOL= TAR= SCAN_FROM_BLOCK= REANCHOR=true)" }, { "target": "roles-check", diff --git a/docs/roles.md b/docs/roles.md index 9bf1220..7aa0dc9 100644 --- a/docs/roles.md +++ b/docs/roles.md @@ -184,6 +184,22 @@ flowchart TD non-blocking) keep surfacing the drift as a `[FAIL]`/`::warning::` until it is reconciled one way or the other. Reconciling means the two agree again - either the chain was fixed or the declaration was. +### `REANCHOR=true` - the repoint case + +The runbook above reconciles the role HOLDERS. A repoint moves the SUBJECT: redeploy a token or pool +under the same group and `roles..address` still names the replaced contract while +`addresses.active.*` names the live one. `snapshot-chain` refuses that divergence rather than pick a +side - following the anchor would re-declare roles for a contract nobody uses, and following `active` +would let any redeploy silently re-point what the audit reconciles. + +```bash +REANCHOR=true make snapshot-chain CHAIN= # move the anchor to addresses.active.* +``` + +Use it only when the repoint was deliberate; if it was not, point the store back instead. The refusal +names both addresses, so you can tell which record is the wrong one. The anchor still outranks +`TOKEN`/`TOKEN_POOL`, which stay the way to snapshot a contract on a chain that has no declaration yet. + ### `setDynamicConfig` router-preservation footgun `rateLimitAdmin` and `feeAdmin` are both set through `setDynamicConfig(router, rateLimitAdmin, diff --git a/docs/troubleshooting/index.md b/docs/troubleshooting/index.md index 6fa2afe..64ed46d 100644 --- a/docs/troubleshooting/index.md +++ b/docs/troubleshooting/index.md @@ -75,6 +75,32 @@ command, the fix, and a self-verify. Start from the error you see. redeploy, rewire the `TokenAdminRegistry` with `SetPool` (the registry still points at the old pool). - **Verify.** `make doctor CHAIN=` shows the registry pointing at the intended pool. +## A setup script reverts naming an admin address you never configured + +`Admin of token doesn't match the expected admin address` from `ClaimAndAcceptAdmin`, or +`OnlyAdministrator(0x1804c8AB1F12E6bbf3894d4083f33e07309d1f38, )` from `SetPool`, on a token +whose `getCCIPAdmin()` and `owner()` both read back as your keystore account. + +- **Diagnosis.** The passphrase never reached Foundry, so the keystore stayed locked and the simulation + ran as Foundry's default sender `0x1804c8AB1F12E6bbf3894d4083f33e07309d1f38`. It is the same fallback + described under [the keystore password](../workflows/greenfield-deploy.md#keystore-password-non-interactive-runs), + seen from the other end: the revert names the token or the registry, never the signer, so the error + does not read like an authentication problem. Foundry prompts only when it can reach a terminal, so a + run with stdin closed (`` otherwise. `--sender` is not needed; the account resolves once the keystore + unlocks. + ```bash + forge script script/setup/ClaimAndAcceptAdmin.s.sol --rpc-url "$ETHEREUM_SEPOLIA_RPC_URL" \ + --account "$KEYSTORE_NAME" --password '' --broadcast + ``` + Do not take Foundry's own hint here (`You seem to be using Foundry's default sender. Be sure to set + your own --sender`) at face value: `--sender` fixes which account the run simulates as, but the + keystore is still locked, so a non-interactive broadcast then dies at signing with `Device not + configured (os error 6)` - the passphrase is the fix in both cases. +- **Verify.** `cast call "getTokenConfig(address)((address,address,address))" ` + returns your account as the administrator. + ## Inbound transfer reverts after removing a remote pool - **Diagnosis.** Removing a remote pool leaves the chain supported with zero pools, so inbound diff --git a/script/config/VerifyChain.s.sol b/script/config/VerifyChain.s.sol index dbbd76b..7ae9ab9 100644 --- a/script/config/VerifyChain.s.sol +++ b/script/config/VerifyChain.s.sol @@ -259,6 +259,10 @@ contract ChainProbe { contract VerifyChain is Script { uint256 private s_fails; uint256 private s_warns; + // The last WARN text, kept so a unit test can assert WHICH warning fired. Two shapes of the same + // rung can both emit exactly one WARN, so a count alone cannot tell them apart, and `console.log` + // is not capturable in-process. + string private s_lastWarn; uint256 private s_skips; // unverified-gap skips only (see _skipUnverified); designed skips do not count bool private s_forked; ChainProbe private s_probe; @@ -274,6 +278,7 @@ contract VerifyChain is Script { function _warn(string memory msg_) private { s_warns++; + s_lastWarn = msg_; console.log(string.concat("[WARN] ", msg_)); } @@ -1380,10 +1385,15 @@ contract VerifyChain is Script { /// @notice Test hook: runs ONLY the roles anchor-drift check (`_warnAnchorDrift` for the `token` and /// `pool` anchors) for `name` against its project store, returning `(fails, warns)`. Lets a UNIT test /// (no RPC, no auditor) assert the WARN-not-FAIL contract: a declared `roles..address` anchor that - /// diverges from `addresses.active.` emits exactly one WARN naming both + `make snapshot-chain`, + /// diverges from `addresses.active.` emits exactly one WARN naming both + `REANCHOR=true make snapshot-chain`, /// while a matching anchor, an absent anchor, or a store with no active pointer stays silent. Not used /// by any production path - the production caller is `_checkRoles` (behind the roles-block + RPC gates). function warnAnchorDriftForTest(string memory name) public returns (uint256 failsOut, uint256 warnsOut) { + // `_warn`/`_fail` accumulate: without this reset a second call on the same instance carries the + // first store's tally and message, so an assertion lands on the earlier fixture, not this one. + s_fails = 0; + s_warns = 0; + s_lastWarn = ""; s_probe = new ChainProbe(); string memory projectJson = _readProject(name); _warnAnchorDrift(name, projectJson, ".roles.token.address", "token", "roles.token.address"); @@ -1391,6 +1401,13 @@ contract VerifyChain is Script { return (s_fails, s_warns); } + /// @notice TEST-ONLY hook: the text of the last WARN emitted. A count cannot distinguish two + /// warnings from the same rung - a zero anchor and a diverging anchor both emit exactly one - so a + /// test that only counts passes whichever branch fired. + function lastWarnForTest() public view returns (string memory) { + return s_lastWarn; + } + /// @notice TEST-ONLY hook: runs the multi-pool ambiguity check for `name` against its project /// store, returning `(fails, warns)`. Lets a UNIT test assert the WARN-not-FAIL contract: two or /// more `deployments{}` token pools emit exactly one WARN; zero or one pool stays silent. Not used @@ -1461,6 +1478,15 @@ contract VerifyChain is Script { /// @dev WARN (never FAIL) when a declared roles anchor differs from the store's `active.`. /// Silent when the anchor is absent or the store has no active pointer (nothing to reconcile). + /// + /// Two shapes warn with their OWN remedy rather than the drift one. A malformed anchor is read + /// through a try/catch because this rung runs OUTSIDE the guard that wraps the auditor, so a raw + /// parse abort here takes the whole doctor down - no verdict, no exit code - on a store the + /// snapshot half refuses cleanly. A zero anchor is not a usable declaration: `RolesAuditor` counts + /// a present-but-zero key as declared, gates every check behind a non-zero token, and then reports + /// "reconciles clean" having audited nothing, so it has to be named. Neither may quote the drift + /// remedy: a plain `snapshot-chain` resolves past a zero anchor rather than refusing it, and it + /// refuses a malformed one for a different reason than staleness. function _warnAnchorDrift( string memory name, string memory projectJson, @@ -1469,8 +1495,37 @@ contract VerifyChain is Script { string memory label ) private { if (!vm.keyExistsJson(projectJson, anchorPath)) return; - address anchor = vm.parseJsonAddress(projectJson, anchorPath); + address anchor; + try vm.parseJsonAddress(projectJson, anchorPath) returns (address a) { + anchor = a; + } catch { + _warn( + string.concat( + "roles: ", + label, + " is not an address - the audit cannot resolve what it is meant to reconcile.", + " Set it to a deployed address or remove the key, then: make snapshot-chain CHAIN=", + name + ) + ); + return; + } address active = RegistryWriter._read(name, role); + if (anchor == address(0)) { + _warn( + string.concat( + "roles: ", + label, + " is the zero address, so every role check under it SKIPs and the roles rung reports", + " clean having audited nothing. A plain snapshot-chain does not refuse a zero anchor - it", + " re-resolves from addresses.active.", + role, + ", and errors when the store has none: make snapshot-chain CHAIN=", + name + ) + ); + return; + } if (active == address(0) || active == anchor) return; _warn( string.concat( @@ -1482,7 +1537,9 @@ contract VerifyChain is Script { role, " ", vm.toString(active), - " - the audit reconciles the anchored value; re-anchor after a repoint: make snapshot-chain CHAIN=", + " - the audit reconciles the ANCHORED value, so it is auditing the replaced contract.", + " A plain snapshot-chain will NOT fix this: it refuses a stale anchor rather than guess", + " which record is right. Re-anchor with: REANCHOR=true make snapshot-chain CHAIN=", name ) ); diff --git a/src/roles/RolesSnapshot.sol b/src/roles/RolesSnapshot.sol index 0be8b5c..008e2a9 100644 --- a/src/roles/RolesSnapshot.sol +++ b/src/roles/RolesSnapshot.sol @@ -5,7 +5,6 @@ import {Vm} from "forge-std/Vm.sol"; import {console} from "forge-std/console.sol"; import {RolesProbes} from "./RolesProbes.sol"; -import {RegistryWriter} from "../utils/RegistryWriter.sol"; import {ProjectStore} from "../utils/ProjectStore.sol"; /// @title RolesSnapshot @@ -75,19 +74,36 @@ contract RolesSnapshot { return _assemble(c, projectJson); } - /// @dev Token/pool resolution - the declared `roles{}` wins (it IS the durable record; the - /// `project/.json` `addresses{}` store is single-valued per role and, in this repo, - /// gitignored), then the `TOKEN`/`TOKEN_POOL` env overrides, then the store's active pointers. + /// @dev Token/pool resolution: the declared `roles{}` anchor, then the `TOKEN`/`TOKEN_POOL` env, + /// then the store's `addresses.active.*`. The anchor deliberately wins: `roles{}` is the durable + /// declaration, while `TOKEN`/`TOKEN_POOL` are process-wide (the Makefile exports them for the + /// deploy targets), so an ambient env value that outranked the declaration would let any stray + /// export silently re-point what the audit reconciles. + /// + /// A stale anchor REFUSES rather than resolve. After a repoint - a redeploy under the same group - + /// `roles.*.address` names the replaced contract while `addresses.active.*` names the live one, and + /// a snapshot that followed the anchor would rewrite role holders under it, print "wrote .roles + /// block" and exit 0 having changed nothing the operator asked for - while `make doctor` prescribes + /// that very command as the remedy. Following `active` instead is no better: it would let any + /// redeploy silently re-point the audit's subject, which is what the declaration exists to prevent. + /// `REANCHOR=true` moves the anchor to `addresses.active.*`, as an explicit instruction. + /// + /// The refusal is scoped to a value that CAME FROM the anchor. `TOKEN`/`TOKEN_POOL` on a chain with + /// no declaration yet is the documented way to snapshot a contract other than the active one, so + /// policing it would kill the override in the only case it exists for - and would do it while + /// naming a `roles.*.address` key the file does not contain. function _resolveProject(string memory name, string memory json) private view returns (address token, address pool) { - if (VM.keyExistsJson(json, ".roles.token.address")) { - token = VM.parseJsonAddress(json, ".roles.token.address"); - } - if (token == address(0)) token = VM.envOr("TOKEN", address(0)); - if (token == address(0)) token = RegistryWriter._read(name, "token"); + bool reanchor = _rsReanchor(); + + address activeToken = _activeFromJson(json, "token"); + token = _anchorFromJson(json, name, "token"); + bool tokenFromAnchor = token != address(0); + if (token == address(0)) token = _rsEnvOr("TOKEN"); + if (token == address(0)) token = activeToken; require( token != address(0), string.concat( @@ -96,11 +112,20 @@ contract RolesSnapshot { " addresses.active.token)" ) ); - if (VM.keyExistsJson(json, ".roles.pool.address")) { - pool = VM.parseJsonAddress(json, ".roles.pool.address"); + if (!tokenFromAnchor) { + // Nothing declared: the value came from TOKEN or from active, so there is no anchor to move + // or to police. + } else if (reanchor) { + token = _reanchor(name, "token", "token", token, activeToken); + } else { + _requireAnchorIsCurrent(name, "token", "token", token, activeToken); } - if (pool == address(0)) pool = VM.envOr("TOKEN_POOL", address(0)); - if (pool == address(0)) pool = RegistryWriter._read(name, "tokenPool"); + + address activePool = _activeFromJson(json, "tokenPool"); + pool = _anchorFromJson(json, name, "pool"); + bool poolFromAnchor = pool != address(0); + if (pool == address(0)) pool = _rsEnvOr("TOKEN_POOL"); + if (pool == address(0)) pool = activePool; require( pool != address(0), string.concat( @@ -109,6 +134,152 @@ contract RolesSnapshot { " addresses.active.tokenPool)" ) ); + if (!poolFromAnchor) { + // As above: no declaration, nothing to re-anchor or police. + } else if (reanchor) { + pool = _reanchor(name, "pool", "tokenPool", pool, activePool); + } else { + _requireAnchorIsCurrent(name, "pool", "tokenPool", pool, activePool); + } + } + + /// @dev The declared `roles..address`, or zero when there is none. Wrapped in try/catch to + /// name the file and the key: a half-finished hand edit (`""`, a number, an object) otherwise + /// aborts with a raw `vm.parseJsonAddress` error that quotes the offending value alone. Unlike + /// `_activeFromJson`, a parse failure REVERTS instead of reading as zero - a mistyped anchor that + /// read as zero would resolve on to `active` and snapshot a contract nobody declared. + function _anchorFromJson(string memory json, string memory name, string memory role) + private + view + returns (address) + { + string memory key = string.concat(".roles.", role, ".address"); + try VM.keyExistsJson(json, key) returns (bool exists) { + if (!exists) return address(0); + } catch { + revert(string.concat("[snapshot] ", ProjectStore._display(name), " is not valid JSON")); + } + try VM.parseJsonAddress(json, key) returns (address a) { + return a; + } catch { + revert( + string.concat( + "[snapshot] ", + ProjectStore._display(name), + " has a malformed ", + key, + ": expected an address string, or remove the key" + ) + ); + } + } + + /// @dev Move the anchor to `addresses.active.` under `REANCHOR=true`, and SAY SO: the + /// operator is overwriting the declared audit subject, and the caller's usual "wrote .roles block" + /// line would leave that re-point indistinguishable from a routine re-snapshot - including when a + /// stray `REANCHOR` export did it. + /// + /// A half with nothing to move to is a logged no-op, NOT a refusal. `REANCHOR` is one flag over a + /// store holding two anchors: refusing here would let the token half - which may have no + /// `addresses.active.token` at all, the shape of an adopted token under a redeployed pool - block + /// the pool half the operator actually came to fix, and the doctor prescribes this exact command. + function _reanchor( + string memory name, + string memory role, + string memory activeKey, + address anchored, + address active + ) private view returns (address) { + if (active == address(0)) { + console.log( + string.concat( + "[snapshot] REANCHOR: no usable ", + ProjectStore._display(name), + " addresses.active.", + activeKey, + " (absent, zero, or not an address) to move roles.", + role, + ".address to - keeping ", + VM.toString(anchored) + ) + ); + return anchored; + } + if (anchored != active) { + console.log( + string.concat( + "[snapshot] REANCHOR: roles.", role, ".address ", VM.toString(anchored), " -> ", VM.toString(active) + ) + ); + } + return active; + } + + /// @dev `addresses.active.` read from the SAME document the anchor came from. Re-reading the + /// store off disk would compare an anchor from one document against an active pointer from another, + /// so any caller holding a project doc that is not the on-disk file - every test with an in-memory + /// fixture - would get a spurious STALE ANCHOR. Absent key or a non-EVM (base58) value reads as zero, which the + /// divergence check treats as "nothing to compare", matching `RegistryWriter._read`. + function _activeFromJson(string memory json, string memory role) private view returns (address) { + string memory key = string.concat(".addresses.active.", role); + if (bytes(json).length == 0) return address(0); + try VM.keyExistsJson(json, key) returns (bool exists) { + if (!exists) return address(0); + } catch { + return address(0); + } + try VM.parseJsonAddress(json, key) returns (address a) { + return a; + } catch { + return address(0); + } + } + + /// @dev Virtual input seam (the `_dcEnvOr` / `_rlEnv*` idiom): env vars are process-wide and forge + /// runs suites in parallel, so tests pin the override through a subclass, never `vm.setEnv`. + function _rsEnvOr(string memory name) internal view virtual returns (address) { + return VM.envOr(name, address(0)); + } + + /// @dev The explicit re-anchor instruction, through the same seam and for the same reason. + function _rsReanchor() internal view virtual returns (bool) { + return VM.envOr("REANCHOR", false); + } + + /// @dev REFUSES when the declared anchor names a different contract than `addresses.active.*` and + /// the operator gave no explicit instruction. Both values are real records with different writers - + /// `roles{}` is the declared authority, `addresses{}` is what the deploy scripts last wrote - so + /// picking either silently is a guess: following the anchor re-declares roles for a contract that + /// was replaced, and following `active` would let any redeploy silently re-point the audit's + /// subject, which is exactly the property `roles{}` exists to deny. The failure is loud because the + /// alternative is a run that prints "wrote .roles block" and exits 0 having changed nothing an + /// operator asked for; the message names both addresses and both ways out, so the fix is a + /// copy-paste rather than a code read. + function _requireAnchorIsCurrent( + string memory name, + string memory role, + string memory activeKey, + address anchored, + address active + ) private view { + if (active == address(0) || anchored == active) return; + revert( + string.concat( + "[snapshot] STALE ANCHOR: roles.", + role, + ".address is ", + VM.toString(anchored), + " but ", + ProjectStore._display(name), + " addresses.active.", + activeKey, + " is ", + VM.toString(active), + ". Refusing to snapshot: writing role holders under the old address would leave the audit", + " reconciling a contract that was replaced. Re-anchor to the deployed one with REANCHOR=true,", + " or keep the declaration and point the store back if the repoint was a mistake." + ) + ); } function _assemble(Ctx memory c, string memory json) private returns (string memory) { diff --git a/test/config/VerifyChainAnchorDrift.t.sol b/test/config/VerifyChainAnchorDrift.t.sol index 4d331ea..5a1df54 100644 --- a/test/config/VerifyChainAnchorDrift.t.sol +++ b/test/config/VerifyChainAnchorDrift.t.sol @@ -10,8 +10,8 @@ import {ProjectStore} from "../../src/utils/ProjectStore.sol"; /// self-contained, but a later deploy can repoint `addresses.active.` off the anchored token - /// the auditor would then reconcile the STALE anchored value clean (a false green). `_warnAnchorDrift` /// catches that: a declared anchor that DIVERGES from the store's active pointer emits exactly one WARN -/// (naming both addresses + `make snapshot-chain`), while a matching anchor, an absent anchor, or a -/// store with no active pointer stays silent. It is WARN-only (never FAIL) and needs no RPC - a pure +/// (naming both addresses + `REANCHOR=true make snapshot-chain`), while a matching anchor, an absent +/// anchor, or a store with no active pointer stays silent. It is WARN-only (never FAIL) and needs no RPC - a pure /// file compare - so this suite pins the (fails, warns) contract as a unit test via /// `warnAnchorDriftForTest`, which runs ONLY that check. Each test writes its own uniquely-named scratch /// project file (suites run in parallel and share the filesystem) and cleans it in setUp() (revert-safe). @@ -24,6 +24,9 @@ contract VerifyChainAnchorDriftTest is Test { string internal constant SEL_NOACTIVE = "zz-scratch-anchordrift-noactive"; string internal constant SEL_NOANCHOR = "zz-scratch-anchordrift-noanchor"; string internal constant SEL_DRIFT = "zz-scratch-anchordrift-drift"; + string internal constant SEL_ZERO = "zz-scratch-anchordrift-zero"; + string internal constant SEL_MALFORMED = "zz-scratch-anchordrift-malformed"; + string internal constant SEL_REUSE = "zz-scratch-anchordrift-reuse"; function setUp() public { _clean(); @@ -34,7 +37,7 @@ contract VerifyChainAnchorDriftTest is Test { /// ONLY the fixtures it owns at the end of its body (suite siblings run in parallel), so a green /// run leaves no residue. function _clean() private { - string[4] memory sels = [SEL_MATCH, SEL_NOACTIVE, SEL_NOANCHOR, SEL_DRIFT]; + string[7] memory sels = [SEL_MATCH, SEL_NOACTIVE, SEL_NOANCHOR, SEL_DRIFT, SEL_ZERO, SEL_MALFORMED, SEL_REUSE]; for (uint256 i = 0; i < sels.length; i++) { string memory p = ProjectStore._path(sels[i]); if (vm.exists(p)) vm.removeFile(p); @@ -119,8 +122,8 @@ contract VerifyChainAnchorDriftTest is Test { /// @dev INDUCED DRIFT: `roles.token.address = ANCHOR` but `addresses.active.token = ACTIVE` (a /// repoint after the snapshot). Exactly one WARN, zero FAIL. Only the token anchor is declared, so /// the pool half stays silent - proving the single WARN is the token divergence alone. The WARN - /// message text (both addresses + `make snapshot-chain CHAIN=`) is composed inline in - /// `_warnAnchorDrift` and is source-pinned; console.log output is not in-test capturable, so the + /// message text (both addresses + `REANCHOR=true make snapshot-chain CHAIN=`) is composed + /// inline in `_warnAnchorDrift` and is source-pinned; console.log output is not in-test capturable, so the /// contract asserted here is the (fails, warns) tally, exactly as the other doctor-rung suites do. function test_AnchorDrift_TokenAnchorDivergesFromActive_OneWarn() public { _writeProject(SEL_DRIFT, _active("token", ACTIVE), _anchor("token", ANCHOR)); @@ -129,4 +132,81 @@ contract VerifyChainAnchorDriftTest is Test { assertEq(warns, 1, "a token anchor diverging from active.token must emit exactly one WARN"); _clean(SEL_DRIFT); } + + /// A zero anchor is a declaration the auditor cannot use: `RolesAuditor` counts a present-but-zero + /// key as declared, gates every check behind a non-zero token, and reports "reconciles clean" + /// having audited nothing. It must be NAMED, and not with the drift remedy - a plain + /// `snapshot-chain` does not refuse a zero anchor, it resolves past it. + function test_ZeroAnchor_WarnsOnItsOwnTerms() public { + _writeProject(SEL_ZERO, _active("token", ACTIVE), _anchor("token", address(0))); + VerifyChain vc = new VerifyChain(); + (uint256 fails, uint256 warns) = vc.warnAnchorDriftForTest(SEL_ZERO); + assertEq(fails, 0, "a zero anchor is a WARN, never a FAIL"); + assertEq(warns, 1, "a zero anchor must be named, not silently skipped"); + // The count alone proves nothing: with the zero branch removed this case falls through to the + // DRIFT warning, which is also exactly one WARN. Assert WHICH warning fired. + string memory got = vc.lastWarnForTest(); + assertTrue(_contains(got, "is the zero address"), "must name the zero anchor as the problem"); + assertTrue( + !_contains(got, "refuses a stale anchor"), "must not quote the drift remedy - snapshot does not refuse here" + ); + _clean(SEL_ZERO); + } + + /// This rung runs OUTSIDE the try/catch that wraps the auditor, so a bare parse of a malformed + /// anchor took the whole doctor down - no verdict, no exit code - on a store the snapshot half + /// refuses cleanly. It must degrade to a WARN. + function test_MalformedAnchor_WarnsInsteadOfAbortingTheDoctor() public { + vm.writeFile( + ProjectStore._path(SEL_MALFORMED), + string.concat( + "{\"addresses\":{\"active\":{", + _active("token", ACTIVE), + "},\"deployments\":{}},\"lanes\":{},\"roles\":{\"token\":{\"address\":\"\"}},\"schema\":3}" + ) + ); + VerifyChain vc = new VerifyChain(); + (uint256 fails, uint256 warns) = vc.warnAnchorDriftForTest(SEL_MALFORMED); + assertEq(fails, 0, "a malformed anchor is a WARN, never a FAIL"); + assertEq(warns, 1, "a malformed anchor must degrade to one WARN, not abort the run"); + assertTrue( + _contains(vc.lastWarnForTest(), "is not an address"), "must name the unparseable anchor as the problem" + ); + _clean(SEL_MALFORMED); + } + + /// @dev Substring test: the WARN text is assembled at runtime, so a full-string equality would pin + /// wording rather than meaning. + function _contains(string memory haystack, string memory needle) private pure returns (bool) { + bytes memory h = bytes(haystack); + bytes memory n = bytes(needle); + if (n.length == 0 || n.length > h.length) return false; + for (uint256 i = 0; i <= h.length - n.length; i++) { + bool ok = true; + for (uint256 j = 0; j < n.length; j++) { + if (h[i + j] != n[j]) { + ok = false; + break; + } + } + if (ok) return true; + } + return false; + } + + /// The hook resets the tally, so every call reports its OWN store. Without that a second call carries + /// the FIRST store's verdict - a zero-anchor assertion would then pass against a healthy store. + function test_HookResetsBetweenCalls() public { + _writeProject(SEL_ZERO, _active("token", ACTIVE), _anchor("token", address(0))); + _writeProject(SEL_REUSE, _active("token", ACTIVE), _anchor("token", ACTIVE)); + VerifyChain vc = new VerifyChain(); + (, uint256 firstWarns) = vc.warnAnchorDriftForTest(SEL_ZERO); + assertEq(firstWarns, 1, "the zero-anchor store warns once"); + (uint256 fails, uint256 warns) = vc.warnAnchorDriftForTest(SEL_REUSE); + assertEq(warns, 0, "an agreeing anchor must report zero warns even on a reused instance"); + assertEq(fails, 0, "and zero fails"); + assertEq(vc.lastWarnForTest(), "", "the previous store's message must not survive the call"); + _clean(SEL_ZERO); + _clean(SEL_REUSE); + } } diff --git a/test/roles/RolesAnchorReanchor.t.sol b/test/roles/RolesAnchorReanchor.t.sol new file mode 100644 index 0000000..91c9b99 --- /dev/null +++ b/test/roles/RolesAnchorReanchor.t.sol @@ -0,0 +1,172 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.24; + +import {Test} from "forge-std/Test.sol"; +import {RolesSnapshot} from "../../src/roles/RolesSnapshot.sol"; +import {ProjectScratch} from "../utils/ProjectScratch.sol"; + +/// @dev Pins `REANCHOR=true` through the virtual seam instead of `vm.setEnv`: env vars are +/// process-wide and forge runs suites in parallel, so a real env write would leak sideways. +contract RolesSnapshotReanchoring is RolesSnapshot { + function _rsReanchor() internal pure override returns (bool) { + return true; + } +} + +/// @dev Pins `TOKEN`/`TOKEN_POOL` through the same seam, for the no-declaration case. +contract RolesSnapshotEnvOverride is RolesSnapshot { + function _rsEnvOr(string memory name) internal pure override returns (address) { + if (keccak256(bytes(name)) == keccak256("TOKEN")) return address(uint160(0xDD01)); + if (keccak256(bytes(name)) == keccak256("TOKEN_POOL")) return address(uint160(0x2102)); + return address(0); + } +} + +/// @title RolesAnchorReanchor +/// @notice The repoint case: replacing a token/pool under one group leaves `roles.*.address` naming the +/// OLD contracts while `addresses.active.*` names the new ones. An anchor that won unconditionally made +/// the `TOKEN`/`TOKEN_POOL` rung and the store rung unreachable the moment a declaration existed, so +/// `snapshot-chain` re-read the same stale anchor, rewrote role holders under it, printed "wrote .roles +/// block" and exited 0 having changed nothing the operator asked for - while `make doctor` prescribed +/// that very command as the remedy. +/// +/// These tests pin the two halves of the answer: a stale anchor with no explicit instruction REFUSES (it +/// never silently picks a side), and `REANCHOR=true` RE-ANCHORS (the rung is reachable again). Both +/// assert against `build`'s FIRST step, so they need no fork: resolution runs before any probe. +contract RolesAnchorReanchorTest is Test { + // Deliberately not the live fixture addresses: this suite asserts resolution, not chain state. + address internal constant OLD_TOKEN = address(uint160(0xDD01)); + address internal constant OLD_POOL = address(uint160(0x2102)); + address internal constant NEW_TOKEN = address(uint160(0x4F03)); + address internal constant NEW_POOL = address(uint160(0x0404)); + + // One scratch store per test. forge runs the functions of a suite in PARALLEL, so a shared + // selectorName would have them writing and removing each other's file - the sibling + // `VerifyChainAnchorDrift` suite names its four the same way for the same reason. + string internal constant CHAIN_STALE = "zz-scratch-anchor-reanchor-stale"; + string internal constant CHAIN_MOVE = "zz-scratch-anchor-reanchor-move"; + string internal constant CHAIN_AGREE = "zz-scratch-anchor-reanchor-agree"; + string internal constant CHAIN_ENV = "zz-scratch-anchor-reanchor-env"; + + function setUp() public { + // Revert-safe sweep BEFORE the body, per the ProjectScratch hard rule. + ProjectScratch.clean(CHAIN_STALE); + ProjectScratch.clean(CHAIN_MOVE); + ProjectScratch.clean(CHAIN_AGREE); + ProjectScratch.clean(CHAIN_ENV); + } + + /// @dev A project store whose declaration and active pointers disagree - the post-repoint state. + function _storeWithRepoint() internal pure returns (string memory) { + return string.concat( + '{"addresses":{"active":{"token":"', + vm.toString(NEW_TOKEN), + '","tokenPool":"', + vm.toString(NEW_POOL), + '"},"deployments":{}},"roles":{"token":{"address":"', + vm.toString(OLD_TOKEN), + '"},"pool":{"address":"', + vm.toString(OLD_POOL), + '"}},"schema":3}' + ); + } + + function _writeStore(string memory chain, string memory json) internal { + vm.writeFile(ProjectScratch.projectPath(chain), json); + } + + /// The defect case: resolving to OLD_TOKEN and snapshotting it silently. It must refuse instead, and + /// the message must name BOTH addresses so the operator can act without reading the source. + function test_StaleAnchor_WithNoOverride_Refuses() public { + _writeStore(CHAIN_STALE, _storeWithRepoint()); + RolesSnapshot snap = new RolesSnapshot(); + vm.expectRevert( + bytes( + string.concat( + "[snapshot] STALE ANCHOR: roles.token.address is ", + vm.toString(OLD_TOKEN), + " but project/", + CHAIN_STALE, + ".json addresses.active.token is ", + vm.toString(NEW_TOKEN), + ". Refusing to snapshot: writing role holders under the old address would leave the audit", + " reconciling a contract that was replaced. Re-anchor to the deployed one with REANCHOR=true,", + " or keep the declaration and point the store back if the repoint was a mistake." + ) + ) + ); + snap.build(CHAIN_STALE, "{}", _storeWithRepoint()); + ProjectScratch.clean(CHAIN_STALE); + } + + /// The explicit opt-out: REANCHOR=true moves the anchor to addresses.active.*, and the block it + /// writes names the DEPLOYED contract - not merely "did not revert". + function test_Reanchor_MovesAnchorToActive() public { + _writeStore(CHAIN_MOVE, _storeWithRepoint()); + RolesSnapshot snap = new RolesSnapshotReanchoring(); + string memory rolesJson = snap.build(CHAIN_MOVE, "{}", _storeWithRepoint()); + assertEq( + vm.parseJsonAddress(rolesJson, ".token.address"), + NEW_TOKEN, + "REANCHOR must move roles.token.address to the deployed token" + ); + assertEq( + vm.parseJsonAddress(rolesJson, ".pool.address"), + NEW_POOL, + "REANCHOR must move roles.pool.address to the deployed pool" + ); + ProjectScratch.clean(CHAIN_MOVE); + } + + /// An anchor that still matches the store is untouched - the refusal is scoped to a real divergence, + /// so an ordinary re-snapshot of an unchanged chain keeps working. + function test_AnchorMatchingActive_DoesNotRefuse() public { + string memory json = string.concat( + '{"addresses":{"active":{"token":"', + vm.toString(NEW_TOKEN), + '","tokenPool":"', + vm.toString(NEW_POOL), + '"},"deployments":{}},"roles":{"token":{"address":"', + vm.toString(NEW_TOKEN), + '"},"pool":{"address":"', + vm.toString(NEW_POOL), + '"}},"schema":3}' + ); + _writeStore(CHAIN_AGREE, json); + RolesSnapshot snap = new RolesSnapshot(); + string memory rolesJson = snap.build(CHAIN_AGREE, "{}", json); + assertEq( + vm.parseJsonAddress(rolesJson, ".token.address"), NEW_TOKEN, "an agreeing anchor is snapshotted unchanged" + ); + ProjectScratch.clean(CHAIN_AGREE); + } + + /// The refusal must not spread to the rung it is not about. With no declaration yet, `TOKEN` is the + /// documented way to snapshot a contract other than the active one - so a `TOKEN` that differs from + /// `addresses.active.token` is the NORMAL use of the override, not a stale anchor. Policing it here + /// would kill the flag in the only case it exists for, and would refuse while naming a + /// `roles.token.address` key the file does not contain. + function test_EnvOverride_WithNoDeclaration_DoesNotRefuse() public { + string memory json = string.concat( + '{"addresses":{"active":{"token":"', + vm.toString(NEW_TOKEN), + '","tokenPool":"', + vm.toString(NEW_POOL), + '"},"deployments":{}},"schema":3}' + ); + _writeStore(CHAIN_ENV, json); + RolesSnapshot snap = new RolesSnapshotEnvOverride(); + string memory rolesJson = snap.build(CHAIN_ENV, "{}", json); + assertEq( + vm.parseJsonAddress(rolesJson, ".token.address"), + OLD_TOKEN, + "TOKEN must still select its contract when no anchor is declared" + ); + assertEq( + vm.parseJsonAddress(rolesJson, ".pool.address"), + OLD_POOL, + "TOKEN_POOL must still select its contract when no anchor is declared" + ); + ProjectScratch.clean(CHAIN_ENV); + } +}