diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6d9051183..888e91cb5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -133,7 +133,9 @@ Example query paths. Each row starts from a small set of known contracts; every Any new contract address that other contracts need to read must be looked up through `DotnsProtocolRegistry` at the point of use. Do not hardcode it in a constructor, store it in an `immutable`, or expose a one-off `setX(address)` setter. The protocol registry is the only address a contract may hold directly; everything else is fetched on demand so rotation is a single `protocolRegistry.set(KEY, newAddress)` call with no upgrade. -If you are adding a new contract category, add a `bytes32` key for it in `DotnsConstants.sol`, wire it up in `WireDeployments.s.sol`, and list the contract and its interface in `.github/abi-contracts.txt` so their ABIs ship in the release artifact. Read it the same way every existing contract does. +If you are adding a new contract category, add a `bytes32` key for it in `DotnsConstants.sol`, deploy it from one of the stages in `scripts/deploy/run.sh` (not from `DotnsDeployer.s.sol`, which is not in that list and which `scripts/genesis/build-genesis.sh` therefore never runs), wire it up in `WireDeployments.s.sol`, and list the contract and its interface in `.github/abi-contracts.txt` so their ABIs ship in the release artifact. Read it the same way every existing contract does. + +There is exactly one sanctioned exception, `DotnsRootGateway`. It is a non-upgradeable leaf that holds the protocol registry in an `immutable`, because it must not sit behind a proxy: `ISystem.callerIsRoot` resolves the caller two frames below the precompile, and a delegatecall frame displaces the one it inspects. The rule the exception preserves rather than breaks is the point of the rule — the immutable it holds *is* the registry, so every sibling it touches is still resolved on demand, and the gateway itself is rotated by `protocolRegistry.set(ROOT_GATEWAY, ...)` rather than by an upgrade. Bad — the registrar address is frozen at construction, so rotating it needs an upgrade: diff --git a/DEPLOYMENTS.md b/DEPLOYMENTS.md index 9ff0c1180..5f053e257 100644 --- a/DEPLOYMENTS.md +++ b/DEPLOYMENTS.md @@ -283,40 +283,59 @@ If the deployment was intended to update a public environment, update the addres ## Name grants (whitelisting) -Reserved registration is gated on `DotnsNameWhitelist`. A grant binds one label to one beneficiary address and is single use: `registerReserved` requires a grant naming `registration.owner`, spends it on the mint, and refuses a second attempt. See the [README economics section](./README.md#economics) for what a grant does and does not confer; this section covers the mechanics. +Reserved registration is gated on `DotnsNameWhitelist`. A grant binds one label to one beneficiary address and is single use: on the public branch `registerReserved` requires a grant naming `registration.owner`, spends it on the mint, and refuses a second attempt. A governance mint arriving through the Root gateway skips both the grant check and the spend, so that governance can issue a withheld label without first granting it to itself; that branch is reachable only from the single address registered under `ROOT_GATEWAY`. See the [README economics section](./README.md#economics) for what a grant does and does not confer; this section covers the mechanics. **Every admin action on the whitelist is a substrate Root dispatch.** Granting, revoking, accepting, rejecting, reserving, setting the request window and retuning the caps all require it. No signed account can do any of them, the contract owner included: the owner's authority is deployment and upgrade, not allocation. A signed call reverts with `NotGovernance`. There is no operator role and no address allowlist. -That is the point of the design. As a security measure, no single key can grant a name; a grant costs a referendum, or on a test network a sudo-dispatched Root call. +That is the point of the design. As a security measure, no single key can grant a name; a grant costs a referendum, or on a test network a sudo-dispatched Root call. "Root dispatch" here means a dispatch whose `dest` is the `ROOT_GATEWAY` contract, and the gates admit that address alone. See [Governance dispatch](#governance-dispatch). -Two owner-level routes reach the same outcome and are **not** closed by this. `DotnsProtocolRegistry.set` is `onlyOwner`, so the owner can point the `nameWhitelist` key at a contract whose `isGrantedTo` returns true for everything. `DotnsRegistrar.addController` is `onlyOwner`, and a controller can mint any available name directly, without the whitelist at all. Treat the guarantee here as covering the whitelist's own admin surface, not the protocol as a whole, until upgrade authority and deploy-time ownership are settled. +Three owner-level routes reach the same outcome and are **not** closed by this. `DotnsProtocolRegistry.set` is `onlyOwner`, so the owner can point the `nameWhitelist` key at a contract whose `isGrantedTo` returns true for everything, or point `rootGateway` at a contract it controls and call the governance surface through it. `DotnsRegistrar.addController` is `onlyOwner`, and a controller can mint any available name directly, without the whitelist at all. None of these is new: the owner authorises every UUPS upgrade, so it could always replace an implementation with one that has no gate. Treat the guarantee here as covering the whitelist's own admin surface, not the protocol as a whole, until upgrade authority and deploy-time ownership are settled. The controller carries no roles either. It reads grants and consumes them, so `setRole` on the controller reverts with `UnsupportedRole`. -### Dispatching a grant +### Governance dispatch -The dispatch is a Substrate extrinsic with the contract call nested inside it: +Every governance call goes through `DotnsRootGateway`, the address registered under `ROOT_GATEWAY`. It is the `dest` of the `Revive.call`; the call you actually want is wrapped inside its `execute`: ``` Root origin - └─ Revive.call { dest: , value: 0, data: } - └─ grantName(string,address) + └─ Revive.call { dest: , value: 0, data: } + └─ execute(address[],bytes[]) + └─ [], [grantName(string,address)] ``` -`cast` can be used for the innermost layer to encode `data`: +`cast` encodes the inner payloads: ```bash # grant one label to one beneficiary -cast calldata "grantName(string,address)" "$LABEL" "$ADDRESS" +INNER=$(cast calldata "grantName(string,address)" "$LABEL" "$ADDRESS") # grant several labels to one beneficiary, up to maxGrantBatch -cast calldata "grantNames(string[],address)" '["alpha01","beta02"]' "$ADDRESS" +INNER=$(cast calldata "grantNames(string[],address)" '["alpha01","beta02"]' "$ADDRESS") # release an unspent grant -cast calldata "revokeName(string)" "$LABEL" +INNER=$(cast calldata "revokeName(string)" "$LABEL") ``` -Building the `Revive.call` around that hex and dispatching it as Root is done on the Substrate side. Wrap it in `Sudo.sudo` on a test network; put it up as a referendum on a production one. Both produce the same Root origin the contract checks, so the same `data` works either way. +then wrap them for the gateway: + +```bash +cast calldata "execute(address[],bytes[])" "[$WHITELIST]" "[$INNER]" +``` + +`execute` takes positionally matched arrays, so several governance actions ride in one dispatch and either all apply or none do: + +```bash +cast calldata "execute(address[],bytes[])" \ + "[$WHITELIST,$POP_RULES]" \ + "[$GRANT_CALLDATA,$SHORT_NAMES_CALLDATA]" +``` + +Building the `Revive.call` around that hex and dispatching it as Root is done on the Substrate side. Wrap it in `Sudo.sudo` on a test network; put it up as a referendum on a production one. Both produce the same Root origin the gateway checks, so the same `data` works either way. + +**The gateway must be the direct `dest` of the dispatch.** It authorises with `ISystem.callerIsRoot`, which resolves the caller two frames below the precompile. Any contract sitting between Root and the gateway — `Multicall3`, or another forwarder — occupies that frame and the check reads `false`, reverting with `NotRoot`. Batch through `execute` instead of through `Multicall3`. A Substrate-level `Utility.batch` of several separate `Revive.call`s is fine, since each one starts its own call stack. + +Targets must be registered in the protocol registry; anything else reverts with `TargetNotProtocol`. The gateway holds no storage, has no owner, and is deliberately not upgradeable: a proxy would insert a delegatecall frame and break `callerIsRoot`, bricking the whole governance surface. Replacing it means deploying a new one and repointing `ROOT_GATEWAY`, which is an `onlyOwner` call on the protocol registry. The gas and storage-deposit limits belong to the `Revive.call` extrinsic rather than the contract call. Dry-run the call to size them rather than guessing, as an underestimate fails the whole dispatch. diff --git a/README.md b/README.md index 91c475eca..de7587478 100644 --- a/README.md +++ b/README.md @@ -149,7 +149,7 @@ Commit-reveal controller for the public registration path. A caller first submit ### DotnsPopController -Dedicated controller for the Proof-of-Personhood gateway flow. Lives behind its own UUPS proxy with its own storage and is registered on the registrar via addController alongside the commit-reveal controller. Its gated entry points are callable only under a substrate Root origin, which the controller verifies itself by reading `originIsRoot` from the revive System precompile. +Dedicated controller for the Proof-of-Personhood gateway flow. Lives behind its own UUPS proxy with its own storage and is registered on the registrar via addController alongside the commit-reveal controller. Its gated entry points are callable only by governance, which reaches them through `DotnsRootGateway`: the controller checks that `msg.sender` is the address registered under the `rootGateway` key rather than inspecting the transaction's origin. Today the Pop gateway does not write a standalone user-status mapping. It materialises the PoP flow through gateway-issued labels, PoP resolver records, and reservation queue state; user tier checks for public pricing still come from the personhood precompile/context read. diff --git a/contracts/external/revive/ISystem.sol b/contracts/external/revive/ISystem.sol index 51bb8ad35..dde02ed51 100644 --- a/contracts/external/revive/ISystem.sol +++ b/contracts/external/revive/ISystem.sol @@ -10,13 +10,31 @@ pragma solidity ^0.8.34; /// rather than mirrored wholesale, so the audit surface stays small /// and any upstream change is a deliberate review event. interface ISystem { - /// Returning true iff the immediate caller's substrate origin is `Root`. - /// Reverts on a `RuntimeOrigin::Signed(_)` or non-Root origin. + /// Checks whether the caller of the contract calling this function is root. + /// + /// @dev Returns `false` on a signed origin; it does not revert. + /// + /// Frame caveat, and the reason this cannot be used from a DotNS contract directly: the + /// check resolves the caller two frames below the precompile, and a `delegatecall` + /// occupies a frame of its own. A UUPS implementation therefore reads `false` here even + /// on a direct one-hop Root dispatch, because the frame at that depth is the proxy's own + /// rather than Root. Only a contract Root calls directly, with no proxy in front of it, + /// reads `true`. See @custom:contract DotnsRootGateway, which exists to be that shape. function callerIsRoot() external view returns (bool); - /// Returning true iff the transaction-level substrate origin is `Root`. - /// @dev Reads the stack origin rather than the immediate caller, so it holds through a UUPS - /// proxy's delegatecall frame where `callerIsRoot` returns false. Returns false, rather - /// than reverting, on a non-Root origin. + /// Checks whether the origin of the whole call stack is root. + /// + /// @dev Unlike `callerIsRoot`, this does not require the immediate caller to be the origin: it + /// returns `true` whenever the top-level dispatch was made with a root origin, regardless + /// of how many contract or delegate-call frames separate the precompile from that + /// dispatch. This is the analogue of `tx.origin == ROOT` and is intended for upgradeable + /// proxy patterns where root authority needs to flow through intermediate frames. Returns + /// false, rather than reverting, on a non-Root origin. + /// + /// NOT AN AUTHORISATION PRIMITIVE. Because it holds in every frame of a Root transaction, + /// any contract reached during that transaction reads `true` here, including code the + /// Root dispatch did not intend to trust. It answers "was this transaction + /// Root-dispatched", not "is Root my caller". DotNS gates on the latter, through + /// @custom:contract DotnsRootGateway. function originIsRoot() external view returns (bool); } diff --git a/contracts/governance/DotnsRootGateway.sol b/contracts/governance/DotnsRootGateway.sol new file mode 100644 index 000000000..6f55df5c2 --- /dev/null +++ b/contracts/governance/DotnsRootGateway.sol @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.34; + +import {IDotnsProtocolRegistry} from "../registry/IDotnsProtocolRegistry.sol"; +import {IDotnsRootGateway} from "./IDotnsRootGateway.sol"; +import {SystemUtils} from "../utils/SystemUtils.sol"; + +/// @title DotnsRootGateway +/// @notice Sole authorised caller of the governance surface on the registrar controller, +/// `PopRules`, `DotnsNameWhitelist` and `DotnsPopController`. +/// @dev MUST NOT be deployed behind a proxy, and MUST NOT be made upgradeable. The entire value of +/// this contract is that it is the frame shape in which `ISystem.callerIsRoot` reads true: a +/// plain contract that Root calls directly. A `delegatecall` frame, or any contract sitting +/// between Root and this one, displaces the frame the precompile inspects and the check reads +/// false. There is deliberately no owner, no initialiser and no mutable storage. +/// +/// `msg.sender` is never read here. This contract is called by Root, which has no account, so +/// the PVM `caller` syscall would trap. Authority comes from `callerIsRoot` alone. +/// +/// Registration under `ROOT_GATEWAY` makes `IDotnsProtocolRegistry.isRegisteredAddress` true +/// for this address. That confers nothing on its own: store writes are gated by +/// @custom:contract StoreAuth, which admits the registrar, the registrar's controllers and +/// the registry, and deliberately rejects registry membership as authority. +/// +/// Forwards no value. `execute` is not `payable` and makes every call with zero value, so a +/// governance entry point that is made `payable` later would be reachable from here but +/// never funded. Add value plumbing deliberately if that day comes; do not assume it works. +/// @custom:security-contact admin@parity.io +contract DotnsRootGateway is IDotnsRootGateway { + /// @notice Protocol-level address registry used to validate forwarding targets. + /// @dev Immutable and set at construction. The repository's rule that contracts resolve + /// siblings through the registry rather than an immutable peer address is satisfied: the + /// immutable here *is* the registry, which remains the indirection layer. This contract + /// is a non-upgradeable leaf, so it holds no storage to rewire. + IDotnsProtocolRegistry public immutable protocolRegistry; + + /// @param registry Protocol-level address registry. + constructor(IDotnsProtocolRegistry registry) { + require(address(registry) != address(0), InvalidRegistry()); + protocolRegistry = registry; + } + + /// @inheritdoc IDotnsRootGateway + function execute(address[] calldata targets, bytes[] calldata payloads) external override { + // Asks "is Root my caller", not "was this transaction Root-dispatched". It is answerable + // here, and only here, because this contract is not behind a proxy: `callerIsRoot` + // resolves the caller two frames below the precompile, and with no delegatecall frame in + // the path that resolution falls through to the transaction origin. + // + // This is also what makes the function non-reentrant without a guard. A callee that calls + // back into `execute` puts its own frame at the depth the precompile inspects, so the + // check reads false and the re-entry reverts. Do not add a reentrancy guard on the + // assumption that re-entry is otherwise possible, and do not weaken this check without + // reinstating that property some other way. + require(SystemUtils.callerIsRoot(), NotRoot()); + require(targets.length == payloads.length, LengthMismatch()); + require(targets.length != 0, EmptyBatch()); + + for (uint256 i = 0; i < targets.length; i++) { + address target = targets[i]; + bytes calldata payload = payloads[i]; + + // Confine the gateway to the protocol. Root can dispatch anywhere on its own; what it + // must not be able to do is borrow this contract's identity to reach code outside the + // set, since downstream gates authorise on exactly that identity. + require(protocolRegistry.isRegisteredAddress(target), TargetNotProtocol(target)); + + emit RootCallForwarded(target, payload.length >= 4 ? bytes4(payload[:4]) : bytes4(0)); + + // A regular CALL, not a delegatecall: the callee must observe this contract as + // `msg.sender`, which is the whole authorisation mechanism. + (bool ok, bytes memory returndata) = target.call(payload); + if (!ok) { + // Bubble the callee's revert data unchanged so governance sees the real error + // rather than a generic gateway failure. + assembly ("memory-safe") { + revert(add(returndata, 0x20), mload(returndata)) + } + } + } + } +} diff --git a/contracts/governance/IDotnsRootGateway.sol b/contracts/governance/IDotnsRootGateway.sol new file mode 100644 index 000000000..8ef4e9f8b --- /dev/null +++ b/contracts/governance/IDotnsRootGateway.sol @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.34; + +/// @title IDotnsRootGateway +/// @notice Interface for the non-upgradeable gateway that fronts every governance entry point in +/// DotNS. +/// @dev Exists to give the Root-gated surface a caller-side constraint. revive exposes two +/// primitives and neither is usable directly from a UUPS implementation frame: +/// `ISystem.callerIsRoot` resolves the caller two frames below the precompile and a +/// `delegatecall` occupies a frame of its own, so a proxy reads false even on a direct +/// one-hop Root dispatch; and `msg.sender` traps under a Root origin, which has no account. +/// That left `ISystem.originIsRoot`, which is transaction-scoped and therefore true in every +/// frame of a Root transaction, including frames belonging to code the Root dispatch did not +/// intend to trust. +/// +/// This contract closes that gap by being the one shape where `callerIsRoot` works: a plain, +/// non-upgradeable contract that Root calls directly, with no proxy in front of it. It proves +/// Root in its own frame and forwards by regular `CALL`, so every gated contract downstream +/// sees a readable `msg.sender` equal to this address and can authorise on it. +/// @custom:security-contact admin@parity.io +interface IDotnsRootGateway { + /// @notice Thrown when the constructor is given a zero protocol registry. + error InvalidRegistry(); + + /// @notice Thrown when the dispatch is not a direct substrate Root call into this contract. + /// @dev Also thrown when Root reaches this contract through another contract rather than + /// directly: an intermediate frame displaces the one `callerIsRoot` inspects. Governance + /// must make this contract the `dest` of the `Revive.call` dispatch, not a callee of some + /// other contract such as `Multicall3`. Batch through `execute` instead. + error NotRoot(); + + /// @notice Thrown when `targets` and `payloads` differ in length. + error LengthMismatch(); + + /// @notice Thrown when an empty batch is submitted. + error EmptyBatch(); + + /// @notice Thrown when a target is not a member of the protocol registry. + /// @param target Rejected target address. + error TargetNotProtocol(address target); + + /// @notice Emitted for each call the gateway forwards. + /// @param target Contract called. + /// @param selector First four bytes of the payload, or zero for an empty payload. + event RootCallForwarded(address indexed target, bytes4 indexed selector); + + /// @notice Forwards one or more calls to protocol contracts under Root authority. + /// @dev Reverts unless substrate Root is the immediate caller of this contract. Each target + /// must be registered in the protocol registry, so the gateway can never be used to reach + /// code outside the protocol. Calls run in order and the whole batch reverts if any one + /// of them does, bubbling the callee's revert data unchanged. + /// @param targets Protocol contracts to call, in order. + /// @param payloads ABI-encoded calldata for each target, positionally matched. + function execute(address[] calldata targets, bytes[] calldata payloads) external; +} diff --git a/contracts/pop/IPopRules.sol b/contracts/pop/IPopRules.sol index fc4201702..724f4c678 100644 --- a/contracts/pop/IPopRules.sol +++ b/contracts/pop/IPopRules.sol @@ -36,7 +36,7 @@ interface IPopRules { /// @notice Emitted when the public market for names shorter than nine characters is opened or /// closed. - /// @dev Set by the Root-gated @custom:function setShortNamesEnabled. + /// @dev Set by the governance-gated @custom:function setShortNamesEnabled. /// @param enabled Whether names shorter than nine characters may now be bought. event ShortNamesEnabledUpdated(bool enabled); @@ -44,10 +44,11 @@ interface IPopRules { /// @param reason Human-readable explanation of the failure condition. error PopError(string reason); - /// @notice Thrown when @custom:function setShortNamesEnabled is called without a substrate - /// Root origin. - /// @dev Carries no caller parameter: a Root origin has no account to report, and reading - /// `msg.sender` under one traps. + /// @notice Thrown when @custom:function setShortNamesEnabled is called by anything other than + /// the Root gateway. + /// @dev Carries no caller parameter: the only accepted caller is the single address under + /// `DotnsConstants.ROOT_GATEWAY`, so the rejected value adds nothing. Also thrown when that + /// key is unset, so an unwired deployment fails closed. error NotRoot(); /// @notice Thrown when a caller is not an authorised controller on the registrar. @@ -110,9 +111,9 @@ interface IPopRules { returns (PopStatus requirement, string memory message); /// @notice Opens or closes the public market for names shorter than nine characters. - /// @dev Restricted to a substrate Root origin; any other caller triggers @custom:reverts + /// @dev Restricted to the Root gateway; any other caller triggers @custom:reverts /// NotRoot. Short names are otherwise issued through the PoP gateway, so this flag is the - /// Root-only lever that additionally admits them on the public paid path. + /// governance-only lever that additionally admits them on the public paid path. /// While closed, which is the deploy default, @custom:function priceWithCheck and /// @custom:function priceWithoutCheck trigger @custom:reverts PopError for a base length /// below nine, so no public caller buys a short name. The gateway free grant and the diff --git a/contracts/pop/PopRules.sol b/contracts/pop/PopRules.sol index af3f08092..09743a993 100644 --- a/contracts/pop/PopRules.sol +++ b/contracts/pop/PopRules.sol @@ -10,13 +10,13 @@ import { ERC165Upgradeable } from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; import {StringUtils} from "../utils/StringUtils.sol"; -import {SystemUtils} from "../utils/SystemUtils.sol"; import {IPopRules} from "./IPopRules.sol"; import {IDotnsCostModelRegistry} from "./IDotnsCostModelRegistry.sol"; import {IDotnsProtocolRegistry} from "../registry/IDotnsProtocolRegistry.sol"; import {IDotnsController} from "../registrars/IDotnsController.sol"; import {DotnsRegistrar} from "../registrars/DotnsRegistrar.sol"; import {DotnsConstants} from "../utils/DotnsConstants.sol"; +import {GovernanceAuth} from "../utils/GovernanceAuth.sol"; import {IPersonhood} from "../external/personhood/IPersonhood.sol"; /// @title PopRules @@ -31,8 +31,8 @@ import {IPersonhood} from "../external/personhood/IPersonhood.sol"; /// model registered under `DotnsConstants.COST_MODEL`, which owns the curve; this contract /// passes it only the base length and keeps the classification, reservation, and tier rules. /// Personhood only unlocks the premium band. Base lengths below nine are closed to the public -/// paid path until Root sets `shortNamesEnabled`; the gateway and registerReserved do -/// not consult it. +/// paid path until governance sets `shortNamesEnabled`; the PoP controller and +/// registerReserved do not consult it. /// @custom:security-contact admin@parity.io contract PopRules is Initializable, @@ -84,10 +84,9 @@ contract PopRules is /// @inheritdoc IPopRules function setShortNamesEnabled(bool enabled) external override { - // Opening the short-name band to the public path is a governance decision, so it is gated - // on a substrate Root origin rather than the owner. `msg.sender` is deliberately not read: - // a Root origin has no account behind it, so reading it would trap. - require(SystemUtils.originIsRoot(), NotRoot()); + // Opening the short-name band to the public path is a governance decision, so the gate + // is governance rather than the owner. See `GovernanceAuth` for what counts as governance. + require(GovernanceAuth.isGovernance(protocolRegistry, msg.sender), NotRoot()); shortNamesEnabled = enabled; emit ShortNamesEnabledUpdated(enabled); } diff --git a/contracts/registrars/DotnsPopController.sol b/contracts/registrars/DotnsPopController.sol index 6ef222a93..939161ea9 100644 --- a/contracts/registrars/DotnsPopController.sol +++ b/contracts/registrars/DotnsPopController.sol @@ -26,7 +26,7 @@ import {SubnodeUtils} from "../utils/SubnodeUtils.sol"; import {IDotnsRegistry} from "../registry/IDotnsRegistry.sol"; import {StringUtils} from "../utils/StringUtils.sol"; import {DotnsConstants} from "../utils/DotnsConstants.sol"; -import {SystemUtils} from "../utils/SystemUtils.sol"; +import {GovernanceAuth} from "../utils/GovernanceAuth.sol"; /// @title DotnsPopController /// @notice Dedicated PoP controller orchestrating lite-person and full-person username @@ -147,7 +147,7 @@ contract DotnsPopController is /// @dev Reserved storage space to allow for layout changes in future upgrades. uint256[50] private __gap; - /// @notice Restricts calls to a Root origin. + /// @notice Restricts calls to the Root gateway, the protocol's governance entry point. modifier onlyRoot() { _onlyRoot(); _; @@ -920,20 +920,16 @@ contract DotnsPopController is delete _reservedBaseLabel[labelhash]; } - /// @notice Internal check enforcing a Root origin. - /// @dev Authorises a call when @custom:function SystemUtils.originIsRoot is true, and - /// reverts with NotRoot otherwise. `msg.sender` is deliberately not consulted: a - /// Root origin has no account behind it, so reading `msg.sender` traps. That holds - /// for this frame and any delegatecall sharing it; a nested call sees the calling - /// contract as its sender and reads normally. - /// - /// The check also holds for the whole Root transaction rather than the entry frame - /// alone, so nothing reachable from an onlyRoot entrypoint may call a - /// user-controlled address: such a callee could re-enter a gated function and still - /// pass. Every call out of this contract goes to a protocol contract resolved - /// through the registry. + /// @notice Internal check enforcing that the call arrived through the Root gateway. + /// @dev Authorises on `msg.sender` and reverts with NotRoot otherwise. See + /// @custom:contract GovernanceAuth for what counts as governance, and for why this must + /// not be replaced by a check on the transaction's origin. + /// @dev Calls out of this contract go only to protocol contracts resolved through the + /// registry. That is defence in depth rather than the gate itself, and is worth keeping: + /// it bounds what a governance dispatch can reach from here, independently of how the + /// gate is implemented. function _onlyRoot() internal view { - require(SystemUtils.originIsRoot(), NotRoot()); + require(GovernanceAuth.isGovernance(protocolRegistry, msg.sender), NotRoot()); } /// @inheritdoc UUPSUpgradeable diff --git a/contracts/registrars/DotnsRegistrarController.sol b/contracts/registrars/DotnsRegistrarController.sol index e0697aaba..da1b0678c 100644 --- a/contracts/registrars/DotnsRegistrarController.sol +++ b/contracts/registrars/DotnsRegistrarController.sol @@ -24,10 +24,10 @@ import {IStoreFactory} from "../store/IStoreFactory.sol"; import {IDotnsProtocolRegistry} from "../registry/IDotnsProtocolRegistry.sol"; import {IDotnsRegistry} from "../registry/IDotnsRegistry.sol"; import {DotnsConstants} from "../utils/DotnsConstants.sol"; +import {GovernanceAuth} from "../utils/GovernanceAuth.sol"; import {LabelUtils} from "../utils/LabelUtils.sol"; import {RegistrationUtils} from "../utils/RegistrationUtils.sol"; import {StoreUtils} from "../utils/StoreUtils.sol"; -import {SystemUtils} from "../utils/SystemUtils.sol"; /// @title Dotns Registrar Controller /// @notice Allocates top-level labels using a commit reveal scheme. @@ -289,13 +289,17 @@ contract DotnsRegistrarController is /// @inheritdoc IDotnsRegistrarController function registerReserved(Registration calldata registration) external override nonReentrant { - // Read Root once, up front. Everything below must stay callable under a substrate Root - // origin, which has no account, so no branch may read `msg.sender`: the grant is checked - // against `registration.owner`, the commitment is keyed on its own hash, and the mint - // targets the owner. - bool isRoot = SystemUtils.originIsRoot(); + // Two branches on one entry point. The governance branch mints a withheld label without + // a grant; the public branch requires one naming `registration.owner` and spends it. Read + // the branch once, up front, so the two checks below cannot disagree. See + // @custom:contract GovernanceAuth for what counts as governance. + // + // Neither branch treats `msg.sender` as the beneficiary: the grant is checked against + // `registration.owner`, the commitment is keyed on its own hash, and the mint targets the + // owner. That is what lets a relayer submit on a beneficiary's behalf. + bool isGovernance = GovernanceAuth.isGovernance(protocolRegistry, msg.sender); IDotnsNameWhitelist whitelist; - if (!isRoot) { + if (!isGovernance) { whitelist = _nameWhitelist(); require( whitelist.isGrantedTo(registration.label, registration.owner), @@ -307,13 +311,15 @@ contract DotnsRegistrarController is _consumeCommitment(registration); // Spend the grant before minting so a grant in the wrong state fails before any name is - // issued. Root skips it: a governance mint must not consume a grant held by someone else. + // issued. Governance skips it: a governance mint must not consume a grant held by someone + // else. // - // A consequence worth knowing: if Root mints a label that is `Claimed` or `Reserved` on - // the whitelist, that record survives the mint. The beneficiary's own `registerReserved` - // then fails `NameNotAvailable`, and the node stays in the whitelist's active set until - // governance calls `revokeName`. Nothing is lost, but the grant is stranded. - if (!isRoot) { + // A consequence worth knowing: if governance mints a label that is `Claimed` or `Reserved` + // on the whitelist, that record survives the mint. The beneficiary's own + // `registerReserved` then fails `NameNotAvailable`, and the node stays in the whitelist's + // active set until governance calls `revokeName`. Nothing is lost, but the grant is + // stranded. + if (!isGovernance) { whitelist.consume(registration.label, registration.owner); } diff --git a/contracts/registrars/IDotnsPopController.sol b/contracts/registrars/IDotnsPopController.sol index cff38f80e..7af98fc47 100644 --- a/contracts/registrars/IDotnsPopController.sol +++ b/contracts/registrars/IDotnsPopController.sol @@ -200,9 +200,10 @@ interface IDotnsPopController is IDotnsController { /// @param newHead Address now holding the head slot. event ReservationHeadAdvanced(bytes32 indexed labelhash, address indexed newHead); - /// @notice Thrown when a gated entrypoint is reached without a Root origin. - /// @dev Carries no caller parameter: a Root origin has no account to report, - /// and reading `msg.sender` under one traps. + /// @notice Thrown when a gated entrypoint is called by anything other than the Root gateway. + /// @dev Carries no caller parameter: exactly one address is accepted, the one registered under + /// `DotnsConstants.ROOT_GATEWAY`, so the rejected value adds nothing. Also thrown when + /// that key is unset, so an unwired deployment fails closed. error NotRoot(); /// @notice Thrown when a supplied lite-person label does not match `stem.NN`. @@ -255,7 +256,7 @@ interface IDotnsPopController is IDotnsController { /// @notice Registers a lite-person username on behalf of the supplied user /// and optionally enqueues a reservation for a base name they intend to /// claim as a full person later. - /// @dev Callable only under a Root origin (otherwise @custom:reverts NotRoot). The + /// @dev Callable only through the Root gateway (otherwise @custom:reverts NotRoot). The /// lite leg validates the `stem.NN` shape and requires the label to classify outside the /// governance-reserved tier (otherwise @custom:reverts InvalidLiteLabel), and rejects a /// supplied chat key whose length is neither zero nor `CHAT_KEY_LENGTH` @@ -284,7 +285,7 @@ interface IDotnsPopController is IDotnsController { function reserveBaseName(BaseReservation calldata params) external; /// @notice Enqueues only the full/base-name reservation for a user. - /// @dev Callable only under a Root origin (otherwise @custom:reverts NotRoot). + /// @dev Callable only through the Root gateway (otherwise @custom:reverts NotRoot). /// This is the second step of the split /// gateway flow: @custom:function reserveLiteName mints the lite username first, then this /// function reserves the full/base label in a separate transaction so proof-size stays below @@ -299,7 +300,7 @@ interface IDotnsPopController is IDotnsController { /// @notice Registers a lite-person username on behalf of the supplied /// user without touching the base-name reservation queue. - /// @dev Callable only under a Root origin (otherwise @custom:reverts NotRoot). The + /// @dev Callable only through the Root gateway (otherwise @custom:reverts NotRoot). The /// supplied label must satisfy the `stem.NN` shape and must classify outside the /// governance-reserved tier (otherwise @custom:reverts InvalidLiteLabel); a supplied chat /// key whose length is neither zero nor `CHAT_KEY_LENGTH` reverts @@ -326,7 +327,7 @@ interface IDotnsPopController is IDotnsController { function isPopIssued(string calldata label) external view returns (bool issued); /// @notice Registers a full-person username on behalf of the supplied user. - /// @dev Callable only under a Root origin (otherwise @custom:reverts NotRoot). The + /// @dev Callable only through the Root gateway (otherwise @custom:reverts NotRoot). The /// base label must be a letters-only person label, and therefore a true base label, /// (otherwise @custom:reverts InvalidBaseLabel), and the label must not /// classify as governance-reserved (otherwise @custom:reverts InvalidBaseLabel). The diff --git a/contracts/registrars/IDotnsRegistrarController.sol b/contracts/registrars/IDotnsRegistrarController.sol index f49fd494f..f9624a064 100644 --- a/contracts/registrars/IDotnsRegistrarController.sol +++ b/contracts/registrars/IDotnsRegistrarController.sol @@ -174,7 +174,7 @@ interface IDotnsRegistrarController is IDotnsController { /// @dev Grant-backed issuance path: skips the PoP price check and the escrow deposit, but /// reuses the same commit-reveal pipeline so the same anti-front-running guarantees apply. /// - /// Authority is either a substrate Root dispatch or a grant naming `registration.owner` on the + /// Authority is either the Root gateway or a grant naming `registration.owner` on the /// name whitelist registered under `DotnsConstants.NAME_WHITELIST`; anything else /// @custom:reverts NameNotGranted, and an unset registry key /// @custom:reverts WhitelistNotConfigured. The gate reads `registration.owner` rather than the diff --git a/contracts/utils/DotnsConstants.sol b/contracts/utils/DotnsConstants.sol index 41f367182..08e1dcefc 100644 --- a/contracts/utils/DotnsConstants.sol +++ b/contracts/utils/DotnsConstants.sol @@ -14,8 +14,9 @@ library DotnsConstants { /// that opts the precompile in. /// @dev Mirrors the upstream `SYSTEM_ADDR` constant in /// `substrate/frame/revive/uapi/sol/ISystem.sol`. Consumed by - /// `DotnsPopController` and `DotnsNameWhitelist` to authenticate - /// Root-origin dispatches via `ISystem.originIsRoot()`. + /// `DotnsRootGateway`, and only by it, to authenticate a direct Root + /// dispatch via `ISystem.callerIsRoot()`. Every other contract + /// authorises on the gateway's address under `ROOT_GATEWAY`. address internal constant REVIVE_SYSTEM = address(0x0900); /// @notice Address of the Proof-of-Personhood precompile backed by the @@ -189,4 +190,21 @@ library DotnsConstants { /// controller does not consult it. /// forge-lint: disable-next-line(unsafe-typecast) bytes32 internal constant NAME_WHITELIST = bytes32("nameWhitelist"); + + /// @notice Well-known key for the non-upgradeable gateway fronting every governance entry + /// point. + /// @dev Role: sole authorised caller of the governance surface on the registrar controller, + /// `PopRules`, the name whitelist and the PoP controller. Root dispatches to this address + /// directly; it proves Root in its own frame with `ISystem.callerIsRoot` and forwards by + /// regular `CALL`, so each gated contract authorises on `msg.sender`. The indirection + /// exists because neither revive primitive is usable from a UUPS implementation on its + /// own: `callerIsRoot` reads false through the proxy's delegatecall frame, and + /// `originIsRoot` is true in every frame of the transaction and so admits far more than + /// the caller. See @custom:contract GovernanceAuth. + /// + /// The contract behind this key must never sit behind a proxy and must never be made + /// upgradeable: a delegatecall frame breaks `callerIsRoot` and bricks the whole + /// governance surface. Rotate it by pointing this key at a new deployment. + /// forge-lint: disable-next-line(unsafe-typecast) + bytes32 internal constant ROOT_GATEWAY = bytes32("rootGateway"); } diff --git a/contracts/utils/GovernanceAuth.sol b/contracts/utils/GovernanceAuth.sol new file mode 100644 index 000000000..8c78a69e9 --- /dev/null +++ b/contracts/utils/GovernanceAuth.sol @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.34; + +import {IDotnsProtocolRegistry} from "../registry/IDotnsProtocolRegistry.sol"; +import {DotnsConstants} from "./DotnsConstants.sol"; + +/// @title GovernanceAuth +/// @notice Decides which caller is governance for the protocol's admin surfaces. +/// @dev Authority is an address, never a property of the transaction. The one accepted caller is +/// whatever `DotnsConstants.ROOT_GATEWAY` resolves to: a non-upgradeable contract that +/// substrate Root calls directly, which proves Root in its own frame and forwards by regular +/// `CALL` so its callees see it as `msg.sender`. +/// @dev Do NOT gate an admin surface on `ISystem.originIsRoot`. That predicate is transaction +/// scoped rather than frame scoped: it is true in every frame of a Root-origin transaction, +/// so any contract that such a transaction happens to reach would pass, including code +/// governance never chose to trust. `ISystem.callerIsRoot` asks the right question but cannot +/// answer it from behind a proxy, because a `delegatecall` occupies a frame of its own and +/// displaces the one the precompile inspects. The gateway exists to resolve that, and every +/// other contract compares `msg.sender` against it through here. +/// @dev Reading `msg.sender` is safe on this path and only on this path. Under a Root origin there +/// is no account behind the caller and the PVM `caller` syscall traps, so a contract Root +/// calls directly must not read it. Everything downstream of the gateway is called by an +/// ordinary address. +/// @custom:security-contact admin@parity.io +library GovernanceAuth { + /// @notice Whether `caller` is the protocol's governance entry point. + /// @dev Resolved on every call rather than stored, so rotating the gateway is a single + /// `protocolRegistry.set` with no upgrade of the contracts that gate on it. + /// @param protocolRegistry The canonical DotNS protocol registry. + /// @param caller The address being checked, normally `msg.sender`. + /// @return authorised True if `caller` is the currently registered Root gateway. + function isGovernance( + IDotnsProtocolRegistry protocolRegistry, + address caller + ) + internal + view + returns (bool authorised) + { + address gateway = protocolRegistry.get(DotnsConstants.ROOT_GATEWAY); + // An unset key reads as the zero address. Without this guard a caller of `address(0)` + // would match it and a partially wired deployment would sit open rather than closed. + return gateway != address(0) && caller == gateway; + } +} diff --git a/contracts/utils/SystemUtils.sol b/contracts/utils/SystemUtils.sol index e7e191897..4d4233986 100644 --- a/contracts/utils/SystemUtils.sol +++ b/contracts/utils/SystemUtils.sol @@ -10,12 +10,21 @@ import {DotnsConstants} from "./DotnsConstants.sol"; /// address and interface are wired in one place rather than duplicated per consumer. /// @custom:security-contact admin@parity.io library SystemUtils { - /// @notice Returns whether the transaction-level origin is substrate Root. - /// @dev Reads the stack origin through `ISystem.originIsRoot`, which holds through a UUPS - /// proxy's delegatecall frame where `callerIsRoot` returns false, and returns false - /// rather than reverting on a non-Root origin. - /// @return root True when the transaction origin is Root. - function originIsRoot() internal view returns (bool root) { - return ISystem(DotnsConstants.REVIVE_SYSTEM).originIsRoot(); + /// @notice Returns whether substrate Root is the immediate caller of the calling contract. + /// @dev Reads `ISystem.callerIsRoot`, which resolves the caller two frames below the + /// precompile. A `delegatecall` occupies a frame of its own, so this returns false inside + /// a UUPS implementation even on a direct one-hop Root dispatch. It is usable only from a + /// non-proxy contract that Root calls directly, which is what + /// @custom:contract DotnsRootGateway is for. Every other DotNS contract authorises on + /// `msg.sender == protocolRegistry.get(DotnsConstants.ROOT_GATEWAY)` instead. + /// + /// `ISystem.originIsRoot` is deliberately not wrapped here, and must not be added. It is + /// transaction scoped: true in every frame of a Root-origin transaction, so every + /// contract such a transaction reaches passes it. Gating an admin surface on it makes all + /// of them governance principals. Authorisation belongs in + /// @custom:contract GovernanceAuth. + /// @return root True when the calling contract's immediate caller is Root. + function callerIsRoot() internal view returns (bool root) { + return ISystem(DotnsConstants.REVIVE_SYSTEM).callerIsRoot(); } } diff --git a/contracts/whitelist/DotnsNameWhitelist.sol b/contracts/whitelist/DotnsNameWhitelist.sol index 4c4e10f09..de56f3cd7 100644 --- a/contracts/whitelist/DotnsNameWhitelist.sol +++ b/contracts/whitelist/DotnsNameWhitelist.sol @@ -16,7 +16,7 @@ import {IDotnsProtocolRegistry} from "../registry/IDotnsProtocolRegistry.sol"; import {LabelUtils} from "../utils/LabelUtils.sol"; import {StringUtils} from "../utils/StringUtils.sol"; import {DotnsConstants} from "../utils/DotnsConstants.sol"; -import {SystemUtils} from "../utils/SystemUtils.sol"; +import {GovernanceAuth} from "../utils/GovernanceAuth.sol"; /// @title DotnsNameWhitelist /// @notice Pre-launch name whitelist. A name is Open until governance reserves it or a claim is @@ -30,10 +30,11 @@ import {SystemUtils} from "../utils/SystemUtils.sol"; /// no event indexing is required. A name holds at most `maxClaimants` live claims, which /// bounds the loop that clears them on resolution. Resolving a name deletes its claims, /// refunding their storage deposit, so only reserved or won names persist. The entire admin -/// surface is substrate Root: `SystemUtils.originIsRoot` is true through the proxy's -/// delegatecall frame, and no gate reads `msg.sender`, so Root's lack of an address is not a -/// problem. No signed account grants, revokes, reserves, or retunes a cap; the owner's -/// authority is upgrade only. The public and PoP controllers hold only the `consume` hook. +/// surface is governance: every gate requires `msg.sender` to be the `ROOT_GATEWAY`, the +/// non-upgradeable contract Root dispatches to, which proves Root in its own frame and +/// forwards by regular `CALL`. No signed account grants, revokes, reserves, or retunes a cap; +/// the owner's authority is upgrade only. The public and PoP controllers hold only the +/// `consume` hook. /// Entries are keyed by the node under the active TLD, which the deployment holds immutable /// for the whitelist's lifetime. /// @custom:security-contact admin@parity.io @@ -84,11 +85,10 @@ contract DotnsNameWhitelist is /// @dev Reserved storage space to allow for layout changes in the future. uint256[50] private __gap; - /// @notice Restricts a call to a substrate Root dispatch. + /// @notice Restricts a call to a Root dispatch arriving through the Root gateway. /// @dev The whole admin surface is governance-only: no key grants, revokes, reserves, or /// retunes a cap. The owner's authority is deployment and upgrade, not allocation, so no - /// signed account can hand out a name. `msg.sender` is never read here, which is also what - /// keeps every gated entry point callable under a Root origin, since Root has no account. + /// signed account can hand out a name. modifier onlyGovernance() { _onlyGovernance(); _; @@ -104,9 +104,13 @@ contract DotnsNameWhitelist is _; } - /// @notice Internal check enforcing the substrate Root gate. + /// @notice Internal check enforcing the governance gate. + /// @dev Authority is the Root gateway's address, resolved through the protocol registry on + /// every call. See @custom:contract GovernanceAuth for why the gate is an address rather + /// than a check on the transaction's origin, and why an origin check must not be + /// substituted for it. function _onlyGovernance() internal view { - require(SystemUtils.originIsRoot(), NotGovernance()); + require(GovernanceAuth.isGovernance(protocolRegistry, msg.sender), NotGovernance()); } /// @custom:oz-upgrades-unsafe-allow constructor diff --git a/contracts/whitelist/IDotnsNameWhitelist.sol b/contracts/whitelist/IDotnsNameWhitelist.sol index 1923467b0..e694c8a3d 100644 --- a/contracts/whitelist/IDotnsNameWhitelist.sol +++ b/contracts/whitelist/IDotnsNameWhitelist.sol @@ -10,10 +10,10 @@ pragma solidity ^0.8.34; /// mismatched hash. Claims are keyed by the beneficiary `user`, not the submitter, so a /// relayer or a cross-chain sovereign account can submit a claim on a user's behalf and the /// name still binds to that user. All state is on-chain and queryable through views; no event -/// indexing is required. The entire admin surface is substrate Root: the gates check -/// `originIsRoot` and read no `msg.sender`, so Root's lack of an address is not a problem, and -/// no signed account grants, revokes, reserves, or retunes a cap. The controllers hold only -/// the `consume` hook. +/// indexing is required. The entire admin surface is governance: the gates require +/// `msg.sender` to be the `ROOT_GATEWAY`, the non-upgradeable contract Root dispatches to, +/// so no signed account grants, revokes, reserves, or retunes a cap. The controllers hold +/// only the `consume` hook. /// @custom:security-contact admin@parity.io interface IDotnsNameWhitelist { /// @notice Status of a name. @@ -160,9 +160,10 @@ interface IDotnsNameWhitelist { /// @param node Namehash of the label under the active TLD. error NothingToRevoke(bytes32 node); - /// @notice Thrown when a governance-gated call is not a substrate Root dispatch. - /// @dev The whitelist's whole admin surface is Root-only: no signed account, owner included, - /// grants, revokes, reserves, or retunes a cap. + /// @notice Thrown when a governance-gated call did not arrive through the Root gateway. + /// @dev The whitelist's whole admin surface is governance-only: no signed account, owner + /// included, grants, revokes, reserves, or retunes a cap. Also thrown when the + /// `ROOT_GATEWAY` key is unset, so an unwired deployment fails closed. error NotGovernance(); /// @notice Thrown when `consume` is called by any address other than a registrar controller. @@ -197,8 +198,9 @@ interface IDotnsNameWhitelist { function requestName(string calldata label, string calldata reason, address user) external; /// @notice Accepts `user`'s claim as the winner of `label`. - /// @dev Restricted to a substrate Root dispatch. Requires `user`'s claim `Requested`. - /// Sets the name `Claimed` with `user` the winner and clears every claim on the name, rejecting + /// @dev Restricted to governance: the call must arrive through the Root gateway. Requires + /// `user`'s claim `Requested`. Sets the name `Claimed` with `user` the winner and clears every + /// claim on the name, rejecting /// the losers. @custom:reverts NotRequested. @custom:emits NameAccepted for the winner and /// @custom:emits NameRejected for each loser. /// @param label Bare label to resolve. @@ -206,15 +208,16 @@ interface IDotnsNameWhitelist { function accept(string calldata label, address user) external; /// @notice Rejects `user`'s pending claim on `label` without resolving the name. - /// @dev Restricted to a substrate Root dispatch. Requires the claim `Requested`. - /// @custom:reverts NotRequested. @custom:emits NameRejected. + /// @dev Restricted to governance: the call must arrive through the Root gateway. Requires the + /// claim `Requested`. @custom:reverts NotRequested. @custom:emits NameRejected. /// @param label Bare label. /// @param user Beneficiary whose claim is rejected. function reject(string calldata label, address user) external; /// @notice Grants `label` to `user` directly, without a prior claim. - /// @dev Restricted to a substrate Root dispatch. Requires the name Open, `user` non-zero - /// and a canonical label. Sets the name `Claimed` with `user` the winner and clears any pending + /// @dev Restricted to governance: the call must arrive through the Root gateway. Requires the + /// name Open, `user` non-zero and a canonical label. Sets the name `Claimed` with `user` the + /// winner and clears any pending /// claims. @custom:reverts NameNotOpen, @custom:reverts ZeroUser or /// @custom:reverts InvalidLabel. @custom:emits NameAccepted, and /// @custom:emits NameRejected for each cleared claim. @@ -223,24 +226,26 @@ interface IDotnsNameWhitelist { function grantName(string calldata label, address user) external; /// @notice Grants several labels to one `user` directly. - /// @dev Restricted to a substrate Root dispatch. Applies @custom:function grantName to - /// each, at most `maxGrantBatch` labels per call. + /// @dev Restricted to governance: the call must arrive through the Root gateway. Applies + /// @custom:function grantName to each, at most `maxGrantBatch` labels per call. /// @custom:reverts TooManyLabels when `labels` exceeds the batch cap. /// @param labels Bare labels to grant. /// @param user Beneficiary each name binds to. function grantNames(string[] calldata labels, address user) external; /// @notice Resets `label` to Open, clearing any winner and claims. - /// @dev Restricted to a substrate Root dispatch. Resolves a Claimed or claim-holding - /// name; a Reserved name is released through @custom:function setReserved, not here. + /// @dev Restricted to governance: the call must arrive through the Root gateway. Resolves a + /// Claimed or claim-holding name; a Reserved name is released through @custom:function + /// setReserved, not here. /// @custom:reverts NothingToRevoke when the name is not Claimed and holds no claims. /// @custom:emits NameRevoked, and @custom:emits NameRejected for each cleared claim. /// @param label Bare label to reset. function revokeName(string calldata label) external; /// @notice Reserves or releases `label`. - /// @dev Restricted to a substrate Root dispatch. Reserving requires the name Open and clears - /// any pending claims, rejecting each; releasing requires it `Reserved`. @custom:reverts + /// @dev Restricted to governance: the call must arrive through the Root gateway. Reserving + /// requires the name Open and clears any pending claims, rejecting each; releasing requires it + /// `Reserved`. @custom:reverts /// NameNotOpen or @custom:reverts NotReserved. @custom:emits NameReserved or @custom:emits /// NameUnreserved. @param label Bare label. /// @param reserved True to reserve, false to release. @@ -256,29 +261,31 @@ interface IDotnsNameWhitelist { function consume(string calldata label, address registrant) external; /// @notice Sets the request window relative to the current time. - /// @dev Restricted to a substrate Root dispatch. Opens at `block.timestamp + startsIn` for - /// `duration`. @custom:reverts BadWindow when `duration` is zero. @custom:emits WindowSet. + /// @dev Restricted to governance: the call must arrive through the Root gateway. Opens at + /// `block.timestamp + startsIn` for `duration`. @custom:reverts BadWindow when `duration` is + /// zero. @custom:emits WindowSet. /// @param startsIn Seconds from now until requests start being accepted. /// @param duration Seconds the window stays open. function setWindow(uint64 startsIn, uint64 duration) external; /// @notice Sets the live-claim cap per name. - /// @dev Restricted to a substrate Root dispatch. The cap is bounded by - /// `DotnsConstants.WHITELIST_MAX_CLAIMANTS_LIMIT`, which bounds the resolution clear-loop. + /// @dev Restricted to governance: the call must arrive through the Root gateway. The cap is + /// bounded by `DotnsConstants.WHITELIST_MAX_CLAIMANTS_LIMIT`, which bounds the resolution + /// clear-loop. /// @custom:reverts MaxClaimantsOutOfRange when `newMax` is zero or above the ceiling. /// @custom:emits MaxClaimantsSet. /// @param newMax New per-name claim cap. function setMaxClaimants(uint16 newMax) external; /// @notice Sets the reason byte cap. - /// @dev Restricted to a substrate Root dispatch, bounded by + /// @dev Restricted to governance through the Root gateway, bounded by /// `DotnsConstants.WHITELIST_MAX_REASON_LIMIT`. @custom:reverts MaxReasonBytesOutOfRange when /// `newMax` is zero or above the ceiling. @custom:emits MaxReasonBytesSet. /// @param newMax New reason byte cap. function setMaxReasonBytes(uint256 newMax) external; /// @notice Sets the cap on labels per `grantNames` call. - /// @dev Restricted to a substrate Root dispatch, bounded by + /// @dev Restricted to governance through the Root gateway, bounded by /// `DotnsConstants.WHITELIST_MAX_GRANT_BATCH_LIMIT`. @custom:reverts MaxGrantBatchOutOfRange /// when `newMax` is zero or above the ceiling. @custom:emits MaxGrantBatchSet. /// @param newMax New batch cap. diff --git a/scripts/deploy/DeployPolicy.s.sol b/scripts/deploy/DeployPolicy.s.sol index 3a642b804..345711382 100644 --- a/scripts/deploy/DeployPolicy.s.sol +++ b/scripts/deploy/DeployPolicy.s.sol @@ -7,13 +7,16 @@ import {BaseDeployer} from "./BaseDeployer.s.sol"; import {DotnsRegistrarController} from "../../contracts/registrars/DotnsRegistrarController.sol"; import {DotnsNameEscrow} from "../../contracts/escrow/DotnsNameEscrow.sol"; import {DotnsNameWhitelist} from "../../contracts/whitelist/DotnsNameWhitelist.sol"; +import {DotnsRootGateway} from "../../contracts/governance/DotnsRootGateway.sol"; import {IDotnsProtocolRegistry} from "../../contracts/registry/IDotnsProtocolRegistry.sol"; import {DotnsConstants} from "../../contracts/utils/DotnsConstants.sol"; /// @title DeployPolicy -/// @notice Third stage. Deploys the name escrow, the pre-launch name whitelist, and the -/// commit-reveal controller, all of which bind to the protocol registry populated -/// by `DeployCore`. +/// @notice Third stage. Deploys the name escrow, the pre-launch name whitelist, the +/// commit-reveal controller, and the Root gateway, all of which bind to the protocol +/// registry populated by `DeployCore`. +/// @dev The gateway is the only contract in this stage that is NOT a UUPS proxy. See +/// `_deployRootGateway`. /// @custom:security-contact admin@parity.io contract DeployPolicy is BaseDeployer { uint64 public constant MIN_COMMITMENT_AGE = 6 seconds; @@ -29,6 +32,7 @@ contract DeployPolicy is BaseDeployer { _deployNameEscrow(owner, protocolRegistry); _deployNameWhitelist(owner, protocolRegistry); _deployRegistrarController(owner, protocolRegistry); + _deployRootGateway(owner, protocolRegistry); saveDeployments(); @@ -97,4 +101,33 @@ contract DeployPolicy is BaseDeployer { "DotnsNameWhitelist" ); } + + /// @notice Deploys the non-upgradeable Root gateway. + /// @dev Deliberately `_broadcastDeployCreate3`, not `_broadcastDeployUups`. + /// `ISystem.callerIsRoot` resolves the caller two frames below the precompile and a + /// delegatecall occupies a frame of its own, so behind a proxy the check reads false even + /// on a direct Root dispatch. Deploying this as a UUPS proxy would compile, deploy, and + /// silently brick every governance entry point in the protocol. + /// + /// The gateway has no initialiser: it holds the registry in an `immutable` set by its + /// constructor, and no other state at all. That is the one sanctioned exception to the + /// "no immutable peer addresses" rule in CONTRIBUTING.md, because the immutable is the + /// registry itself and this contract cannot be upgraded to rewire it. + /// @param owner Broadcasting account. The gateway itself has no owner. + /// @param protocolRegistry Registry the gateway validates forwarding targets against. + /// @return deployed Address of the deployed gateway. + function _deployRootGateway( + address owner, + address protocolRegistry + ) + internal + returns (address deployed) + { + deployed = _broadcastDeployCreate3( + owner, + "DotnsRootGateway.sol:DotnsRootGateway", + abi.encode(IDotnsProtocolRegistry(protocolRegistry)), + "DotnsRootGateway" + ); + } } diff --git a/scripts/deploy/DotnsDeployer.s.sol b/scripts/deploy/DotnsDeployer.s.sol index e7ff5a795..bba64786e 100644 --- a/scripts/deploy/DotnsDeployer.s.sol +++ b/scripts/deploy/DotnsDeployer.s.sol @@ -13,6 +13,7 @@ import {DotnsRegistrar} from "../../contracts/registrars/DotnsRegistrar.sol"; import {DotnsRegistrarController} from "../../contracts/registrars/DotnsRegistrarController.sol"; import {DotnsPopController} from "../../contracts/registrars/DotnsPopController.sol"; import {DotnsNameWhitelist} from "../../contracts/whitelist/DotnsNameWhitelist.sol"; +import {DotnsRootGateway} from "../../contracts/governance/DotnsRootGateway.sol"; import {DotnsNameEscrow} from "../../contracts/escrow/DotnsNameEscrow.sol"; import {IDotnsController} from "../../contracts/registrars/IDotnsController.sol"; import {DotnsRegistry} from "../../contracts/registry/DotnsRegistry.sol"; @@ -67,6 +68,7 @@ contract DotnsDeployer is BaseDeployer { DotnsRegistrarController public dotnsRegistrarController; DotnsPopController public dotnsPopController; DotnsNameWhitelist public dotnsNameWhitelist; + DotnsRootGateway public dotnsRootGateway; DotnsNameEscrow public dotnsNameEscrow; DotnsProtocolRegistry public protocolRegistry; @@ -90,6 +92,7 @@ contract DotnsDeployer is BaseDeployer { address popController; address popLens; address nameWhitelist; + address rootGateway; } /// @notice Deploys the full DotNS contract set, wires the protocol registry, @@ -136,6 +139,7 @@ contract DotnsDeployer is BaseDeployer { deployment.popController = _deployPopController(OWNER, deployment.protocolRegistry); deployment.popLens = _deployPopLens(OWNER, deployment.protocolRegistry); deployment.nameWhitelist = _deployNameWhitelist(OWNER, deployment.protocolRegistry); + deployment.rootGateway = _deployRootGateway(OWNER, deployment.protocolRegistry); _authoriseControllers(OWNER, deployment); _wireProtocolRegistryKeys(OWNER, deployment); @@ -456,6 +460,11 @@ contract DotnsDeployer is BaseDeployer { protocolRegistry.set(DotnsConstants.POP_RESOLVER, deployment.popResolver); protocolRegistry.set(DotnsConstants.POP_LENS, deployment.popLens); protocolRegistry.set(DotnsConstants.NAME_WHITELIST, deployment.nameWhitelist); + // The governance gate on the controller, PopRules, the whitelist and the PoP controller + // resolves this key on every call and fails closed while it is unset. On an upgrade of an + // existing deployment it must therefore be set BEFORE the new implementations go live, + // or the whole governance surface reverts until it is. + protocolRegistry.set(DotnsConstants.ROOT_GATEWAY, deployment.rootGateway); vm.stopBroadcast(); console.log("Protocol registry keys set"); } @@ -550,6 +559,31 @@ contract DotnsDeployer is BaseDeployer { _assertKey(DotnsConstants.POP_CONTROLLER, deployment.popController, "Key: popController"); _assertKey(DotnsConstants.POP_RESOLVER, deployment.popResolver, "Key: popResolver"); _assertKey(DotnsConstants.POP_LENS, deployment.popLens, "Key: popLens"); + _assertKey(DotnsConstants.ROOT_GATEWAY, deployment.rootGateway, "Key: rootGateway"); + } + + /// @notice Deploys the non-upgradeable Root gateway. + /// @dev Deliberately NOT a UUPS proxy. `ISystem.callerIsRoot` resolves the caller two frames + /// below the precompile and a delegatecall occupies a frame of its own, so behind a proxy + /// the check reads false even on a direct Root dispatch. Deploying this through + /// `_broadcastDeployUups` would silently brick the entire governance surface. + /// @param owner Broadcasting account. The gateway itself has no owner and no storage. + /// @param protocolRegistryProxy Protocol registry the gateway validates targets against. + /// @return deployed Address of the deployed gateway. + function _deployRootGateway( + address owner, + address protocolRegistryProxy + ) + internal + returns (address deployed) + { + deployed = _broadcastDeployCreate3( + owner, + "DotnsRootGateway.sol:DotnsRootGateway", + abi.encode(IDotnsProtocolRegistry(protocolRegistryProxy)), + "DotnsRootGateway" + ); + dotnsRootGateway = DotnsRootGateway(deployed); } function _assertKey(bytes32 key, address expected, string memory label) internal view { diff --git a/scripts/deploy/WireDeployments.s.sol b/scripts/deploy/WireDeployments.s.sol index 76761c706..79ee6648b 100644 --- a/scripts/deploy/WireDeployments.s.sol +++ b/scripts/deploy/WireDeployments.s.sol @@ -19,6 +19,7 @@ import {DotnsPopResolver} from "../../contracts/resolvers/DotnsPopResolver.sol"; import {PopRules} from "../../contracts/pop/PopRules.sol"; import {StoreFactory} from "../../contracts/store/StoreFactory.sol"; import {DotnsConstants} from "../../contracts/utils/DotnsConstants.sol"; +import {DotnsRootGateway} from "../../contracts/governance/DotnsRootGateway.sol"; /// @title WireDeployments /// @notice Final stage. Runs no proxy deployments; reads every address the @@ -48,6 +49,7 @@ contract WireDeployments is BaseDeployer { address popResolver; address popController; address popLens; + address rootGateway; } function run() external { @@ -83,6 +85,7 @@ contract WireDeployments is BaseDeployer { addr.popResolver = _readAddress("DotnsPopResolver"); addr.popController = _readAddress("DotnsPopController"); addr.popLens = _readAddress("DotnsPopLens"); + addr.rootGateway = _readAddress("DotnsRootGateway"); } function _authoriseControllers(address owner, Addresses memory addr) internal { @@ -116,6 +119,9 @@ contract WireDeployments is BaseDeployer { registry.set(DotnsConstants.POP_CONTROLLER, addr.popController); registry.set(DotnsConstants.POP_RESOLVER, addr.popResolver); registry.set(DotnsConstants.POP_LENS, addr.popLens); + // Governance gates resolve this key on every call and fail closed while it is unset, so on + // an upgrade it must be set before the new implementations go live. + registry.set(DotnsConstants.ROOT_GATEWAY, addr.rootGateway); vm.stopBroadcast(); console.log("Protocol registry keys set"); } @@ -191,6 +197,7 @@ contract WireDeployments is BaseDeployer { ); require(registry.get(DotnsConstants.POP_RESOLVER) == addr.popResolver, "Key: popResolver"); require(registry.get(DotnsConstants.POP_LENS) == addr.popLens, "Key: popLens"); + require(registry.get(DotnsConstants.ROOT_GATEWAY) == addr.rootGateway, "Key: rootGateway"); require( DotnsRegistrar(addr.registrar).controllers(IDotnsController(addr.registrarController)), diff --git a/test/base/BaseDotns.t.sol b/test/base/BaseDotns.t.sol index d77a6c6a1..d5aa65404 100644 --- a/test/base/BaseDotns.t.sol +++ b/test/base/BaseDotns.t.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.34; import {Test} from "forge-std/Test.sol"; +import {VmSafe} from "forge-std/Vm.sol"; import {PopRules, IPopRules} from "../../contracts/pop/PopRules.sol"; import {DotnsFlatPricing} from "../../contracts/pop/DotnsFlatPricing.sol"; @@ -35,7 +36,6 @@ import {DotnsNameWhitelist} from "../../contracts/whitelist/DotnsNameWhitelist.s import {DotnsConstants} from "../../contracts/utils/DotnsConstants.sol"; import {LabelUtils} from "../../contracts/utils/LabelUtils.sol"; import {StringUtils} from "../../contracts/utils/StringUtils.sol"; -import {ISystem} from "../../contracts/external/revive/ISystem.sol"; import {IPersonhood} from "../../contracts/external/personhood/IPersonhood.sol"; import {Upgrades} from "openzeppelin-foundry-upgrades/Upgrades.sol"; @@ -273,8 +273,8 @@ abstract contract BaseDotns is Test { popRules = PopRules(popRulesAddress); vm.label(popRulesAddress, "PopRules"); // Open the short-name market so the band and registration suites exercise names below nine - // characters. Toggled as a Root origin because the setter is Root-gated. The default-closed - // state is covered directly in PopRules unit tests. + // characters. Toggled as governance because the setter is governance-gated. The + // default-closed state is covered directly in PopRules unit tests. _setShortNames(true); address dotnsResolverAddress = Upgrades.deployUUPSProxy( @@ -358,31 +358,70 @@ abstract contract BaseDotns is Test { abi.encodeWithSelector(IPersonhood.personhoodStatus.selector), abi.encode(IPersonhood.PersonhoodInfo({status: 0, contextAlias: bytes32(0)})) ); - // Default the revive System precompile to a non-Root origin. Every governance-gated path - // reads it (`DotnsNameWhitelist.onlyGovernance`, `DotnsRegistrarController`'s reserved - // path), and there is no code at the precompile address under forge, so an unmocked read - // decodes empty returndata and reverts. Tests exercising the Root branch override with - // `_mockOriginIsRoot(true)`. - _mockOriginIsRoot(false); - } - - /// @notice Mocks revive's System precompile originIsRoot result. - /// @param returnValue Value to return from `originIsRoot`. - function _mockOriginIsRoot(bool returnValue) internal { + // Default the governance gate to closed. Every governance-gated path resolves the + // `ROOT_GATEWAY` key (`DotnsNameWhitelist.onlyGovernance`, `PopRules`, the PoP + // controller, and `DotnsRegistrarController`'s reserved path), and an unwired key would + // read as the zero address, which those gates reject anyway. Tests exercising the + // governance branch open it with `_actAsGovernance(true)`. + _actAsGovernance(false); + } + + /// @notice Stand-in address for the Root gateway in tests that do not route through a real one. + /// @dev Any address that no test account uses. `_actAsGovernance(false)` points the + /// `ROOT_GATEWAY` key here so the gate is closed for every caller. + address internal constant ROOT_GATEWAY_STUB = address(uint160(0x600D6A7E)); + + /// @notice Opens or closes the governance gate for calls made from this contract. + /// @dev Governance gates authorise on `msg.sender == protocolRegistry.get(ROOT_GATEWAY)`, so + /// this points that key at the caller to open the gate and at an unrelated stub to close + /// it. Ambient rather than per-call, so a suite can open the gate once and make several + /// governance calls under it. + /// + /// Deliberately a registry mock rather than a prank: `vm.startPrank` would collide with + /// the per-call `vm.prank`s the suites already use, and the gate only ever inspects + /// `msg.sender`, so pointing the key at the caller exercises exactly the branch under + /// test. The real dispatch path, Root to the gateway to the target, is covered end to end + /// in `test/unit/governance/DotnsRootGateway.t.sol`. + /// @param enabled True to admit calls from this contract, false to reject every caller. + function _actAsGovernance(bool enabled) internal { + // Whoever the next call will come from: `address(this)` normally, or the pranked address + // when the suite is inside a `vm.startPrank`, as the shared fixture is while it wires the + // protocol up. Pointing the key at the wrong one closes the gate on the fixture itself. + // + // `readCallers` reports the transaction-level sender, which is forge's default sender and + // NOT this contract when no prank is active, so its answer is only usable in prank modes. + (VmSafe.CallerMode mode, address pranked,) = vm.readCallers(); + address effectiveSender = (mode == VmSafe.CallerMode.Prank + || mode == VmSafe.CallerMode.RecurrentPrank) + ? pranked + : address(this); + _actAsGovernanceFor(enabled ? effectiveSender : ROOT_GATEWAY_STUB); + } + + /// @notice Treats `caller` as the Root gateway for subsequent calls. + /// @dev For the tests that drive a governance action through a specific pranked submitter, + /// where the ambient form above cannot see who that will be. The gates compare + /// `msg.sender` against the `ROOT_GATEWAY` key, so pointing the key at that submitter is + /// equivalent to routing its call through the real gateway, without standing the gateway + /// fixture up. `DotnsRootGateway.t.sol` covers the real contract. + /// @param caller Address the gates should accept as governance. + function _actAsGovernanceFor(address caller) internal { vm.mockCall( - DotnsConstants.REVIVE_SYSTEM, - abi.encodeWithSelector(ISystem.originIsRoot.selector), - abi.encode(returnValue) + address(protocolRegistry), + abi.encodeWithSelector( + IDotnsProtocolRegistry.get.selector, DotnsConstants.ROOT_GATEWAY + ), + abi.encode(caller) ); } - /// @notice Toggles the short-name market as a substrate Root origin. - /// @dev `setShortNamesEnabled` is Root-gated, so mock the System precompile's `originIsRoot` - /// true for the call, then restore false so the default test origin stays non-Root. + /// @notice Toggles the short-name market as governance. + /// @dev `setShortNamesEnabled` is gated on the Root gateway, so open the gate for the call and + /// close it afterwards so the default test caller stays unprivileged. function _setShortNames(bool enabled) internal { - _mockOriginIsRoot(true); + _actAsGovernance(true); popRules.setShortNamesEnabled(enabled); - _mockOriginIsRoot(false); + _actAsGovernance(false); } /// @notice Computes the namehash of `parent` and `labelhash`. @@ -497,22 +536,22 @@ abstract contract BaseDotns is Test { protocolRegistry.set(DotnsConstants.NAME_WHITELIST, address(dotnsNameWhitelist)); } - /// @notice Grants `label` to `user` on the name whitelist under a mocked Root origin. - /// @dev The whitelist's admin surface is Root-only, so this mocks `originIsRoot` for the call - /// and restores the default afterwards. The reserved registration path requires a grant - /// naming the intended owner, so this is the setup step every `registerReserved` test - /// needs. Restoring matters: `registerReserved` reads `originIsRoot` too, and a sticky - /// `true` would put the registration itself on the Root branch. + /// @notice Grants `label` to `user` on the name whitelist as governance. + /// @dev The whitelist's admin surface is governance-only, so this opens the gate for the call + /// and closes it afterwards. The reserved registration path requires a grant naming the + /// intended owner, so this is the setup step every `registerReserved` test needs. Closing + /// matters: `registerReserved` reads the same key, and a sticky open gate would put the + /// registration itself on the governance branch, skipping the grant check and consume. function _grantName(string memory label, address user) internal { - _mockOriginIsRoot(true); + _actAsGovernance(true); dotnsNameWhitelist.grantName(label, user); - _mockOriginIsRoot(false); + _actAsGovernance(false); } - /// @notice Drives a PoP reservation under a Root origin and settles the resulting - /// pending claim from the user's signed origin. - /// @dev Single canonical helper for PoP-gateway reservations across unit and fuzz - /// test suites. Calls the controller under a mocked Root origin. + /// @notice Drives a PoP reservation as governance and settles the resulting pending claim + /// from the user's own account. + /// @dev Single canonical helper for PoP reservations across unit and fuzz test suites. Calls + /// the controller with the governance gate open. /// The auto-settle deploys the user's `LabelStore` and writes the stashed label so /// subsequent gateway mints for the same user take the warm path and assertions /// against the resolver and store hold. Chat keys are persisted eagerly on the PoP @@ -542,42 +581,42 @@ abstract contract BaseDotns is Test { } } - /// @notice Dispatches the typed `reserveLiteName` call under a mocked Root origin. + /// @notice Dispatches the typed `reserveLiteName` call as governance. function _rootReserveLiteName(IDotnsPopController.LiteRegistration memory params) internal { _dispatchFromRoot(abi.encodeWithSelector(SELECTOR_RESERVE_LITE_TYPED, params)); } - /// @notice Dispatches the typed `reserveBaseName` call under a mocked Root origin. + /// @notice Dispatches the typed `reserveBaseName` call as governance. function _rootReserveBaseName(IDotnsPopController.BaseReservation memory params) internal { _dispatchFromRoot(abi.encodeWithSelector(SELECTOR_RESERVE_BASE_TYPED, params)); } - /// @notice Dispatches the typed `reserveBaseNameOnly` call under a mocked Root origin. + /// @notice Dispatches the typed `reserveBaseNameOnly` call as governance. function _rootReserveBaseNameOnly(IDotnsPopController.BaseNameReservation memory params) internal { _dispatchFromRoot(abi.encodeWithSelector(SELECTOR_RESERVE_BASE_ONLY_TYPED, params)); } - /// @notice Dispatches the typed `registerBaseName` call under a mocked Root origin. + /// @notice Dispatches the typed `registerBaseName` call as governance. /// @dev Dispatches the label as written. The gateway sends what People Chain holds, so a /// helper that reshaped it would hide whether a fixture is a valid lite label. function _rootRegisterBaseName(IDotnsPopController.FullRegistration memory params) internal { _dispatchFromRoot(abi.encodeWithSelector(SELECTOR_REGISTER_BASE_TYPED, params)); } - /// @notice Calls `payload` on the PoP controller under a mocked Root origin. + /// @notice Calls `payload` on the PoP controller with the governance gate open. /// @dev Reverts with the inner error data when the call fails, so /// `vm.expectRevert` assertions remain meaningful at the test level. function _dispatchFromRoot(bytes memory payload) internal returns (bytes memory ret) { - _mockOriginIsRoot(true); + _actAsGovernance(true); (bool ok, bytes memory data) = address(dotnsPopController).call(payload); - // Restore the default before unwinding, on both the success and revert paths. - // `DotnsRegistrarController.registerReserved` reads `originIsRoot` too, so a sticky - // `true` here would silently put a later reserved registration on the Root branch, + // Close the gate before unwinding, on both the success and revert paths. + // `DotnsRegistrarController.registerReserved` resolves the same key, so a sticky open + // gate here would silently put a later reserved registration on the governance branch, // skipping the grant check and the consume. - _mockOriginIsRoot(false); + _actAsGovernance(false); if (!ok) { assembly { revert(add(data, 32), mload(data)) diff --git a/test/fuzz/registrar/DotnsRegistrarControllerFuzz.t.sol b/test/fuzz/registrar/DotnsRegistrarControllerFuzz.t.sol index e43eb3d50..2d598e493 100644 --- a/test/fuzz/registrar/DotnsRegistrarControllerFuzz.t.sol +++ b/test/fuzz/registrar/DotnsRegistrarControllerFuzz.t.sol @@ -494,9 +494,10 @@ contract DotnsRegistrarControllerFuzzTest is BaseDotns { function testFuzz_root_mints_without_a_grant(uint256 salt) public { string memory nameLabel = _grantLabel(salt); - _mockOriginIsRoot(true); + // `tiago` submits, so `tiago` is the gateway for this call. + _actAsGovernanceFor(tiago); _revealReserved(_reservedFor(nameLabel, ed), tiago); - _mockOriginIsRoot(false); + _actAsGovernance(false); assertEq(dotnsRegistrar.ownerOf(_tokenIdForLabel(nameLabel)), ed); } diff --git a/test/fuzz/whitelist/DotnsNameWhitelistFuzz.t.sol b/test/fuzz/whitelist/DotnsNameWhitelistFuzz.t.sol index 7211c265e..b54c9e043 100644 --- a/test/fuzz/whitelist/DotnsNameWhitelistFuzz.t.sol +++ b/test/fuzz/whitelist/DotnsNameWhitelistFuzz.t.sol @@ -28,11 +28,9 @@ contract DotnsNameWhitelistFuzz is BaseDotns { ); vm.stopPrank(); - // Configuration is Root-only, and the suite's own actions are governance calls, so it - // runs under a Root origin throughout. - // The suite calls governance entry points directly, so it runs under a Root origin - // throughout rather than mocking per call. - _mockOriginIsRoot(true); + // The suite calls governance entry points directly, so it holds the gate open throughout + // rather than opening it per call. Tests that need a rejection close it themselves. + _actAsGovernance(true); whitelist.setWindow(0, 365 days); } @@ -86,6 +84,7 @@ contract DotnsNameWhitelistFuzz is BaseDotns { vm.prank(second); whitelist.requestName(label, "second", second); + _actAsGovernanceFor(owner); vm.prank(owner); whitelist.accept(label, first); @@ -99,6 +98,7 @@ contract DotnsNameWhitelistFuzz is BaseDotns { string memory label = _label(seed); vm.prank(user); whitelist.requestName(label, "r", user); + _actAsGovernanceFor(owner); vm.prank(owner); whitelist.reject(label, user); @@ -130,6 +130,7 @@ contract DotnsNameWhitelistFuzz is BaseDotns { { duration = uint64(bound(uint256(duration), 1, 3650 days)); string memory label = _label(seed); + _actAsGovernanceFor(owner); vm.prank(owner); whitelist.setWindow(0, duration); vm.warp(block.timestamp + duration); diff --git a/test/invariant/registrar/PopControllerHandler.t.sol b/test/invariant/registrar/PopControllerHandler.t.sol index 32819c7c3..8568760b7 100644 --- a/test/invariant/registrar/PopControllerHandler.t.sol +++ b/test/invariant/registrar/PopControllerHandler.t.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.34; import {Test} from "forge-std/Test.sol"; +import {VmSafe} from "forge-std/Vm.sol"; import { DotnsPopController, IDotnsPopController @@ -17,7 +18,7 @@ import {IPopRules} from "../../../contracts/pop/IPopRules.sol"; import {DotnsConstants} from "../../../contracts/utils/DotnsConstants.sol"; import {LabelUtils} from "../../../contracts/utils/LabelUtils.sol"; import {SubnodeUtils} from "../../../contracts/utils/SubnodeUtils.sol"; -import {ISystem} from "../../../contracts/external/revive/ISystem.sol"; +import {IDotnsProtocolRegistry} from "../../../contracts/registry/IDotnsProtocolRegistry.sol"; import {IPersonhood} from "../../../contracts/external/personhood/IPersonhood.sol"; /// @title PopControllerHandler @@ -555,16 +556,16 @@ contract PopControllerHandler is Test { internal returns (bool ok) { - _mockOriginIsRoot(true); + _actAsGovernance(true); try CONTROLLER.reserveBaseName(params) { ok = true; } catch { ok = false; } - // Restore before returning. The mock is campaign-scoped in an invariant run, and - // `DotnsRegistrarController.registerReserved` reads `originIsRoot` too, so leaving it - // `true` would put any later reserved registration on the Root branch. - _mockOriginIsRoot(false); + // Close the gate before returning. The mock is campaign-scoped in an invariant run, and + // `DotnsRegistrarController.registerReserved` resolves the same key, so leaving it open + // would put any later reserved registration on the governance branch. + _actAsGovernance(false); } /// @notice Mirror of `_callReserveBaseName` for `registerBaseName`. @@ -573,21 +574,42 @@ contract PopControllerHandler is Test { internal returns (bool ok) { - _mockOriginIsRoot(true); + _actAsGovernance(true); try CONTROLLER.registerBaseName(params) { ok = true; } catch { ok = false; } - _mockOriginIsRoot(false); - } - - /// @notice Mocks the revive `originIsRoot()` query to return `returnValue`. - function _mockOriginIsRoot(bool returnValue) internal { + _actAsGovernance(false); + } + + /// @notice Stand-in address for the Root gateway, so the closed gate rejects every caller. + address internal constant ROOT_GATEWAY_STUB = address(uint160(0x600D6A7E)); + + /// @notice Opens or closes the governance gate for calls made from this handler. + /// @dev The gates authorise on `msg.sender == protocolRegistry.get(ROOT_GATEWAY)`, so this + /// points that key at the handler to open the gate and at an unrelated stub to close it. + /// A registry mock rather than a prank, so it does not collide with the per-call pranks + /// the handler already makes. + /// @param enabled True to admit calls from this handler, false to reject every caller. + function _actAsGovernance(bool enabled) internal { + // Whoever the next call will come from: `address(this)` normally, or the pranked address + // when the suite is inside a `vm.startPrank`, as the shared fixture is while it wires the + // protocol up. Pointing the key at the wrong one closes the gate on the fixture itself. + // + // `readCallers` reports the transaction-level sender, which is forge's default sender and + // NOT this contract when no prank is active, so its answer is only usable in prank modes. + (VmSafe.CallerMode mode, address pranked,) = vm.readCallers(); + address effectiveSender = (mode == VmSafe.CallerMode.Prank + || mode == VmSafe.CallerMode.RecurrentPrank) + ? pranked + : address(this); vm.mockCall( - DotnsConstants.REVIVE_SYSTEM, - abi.encodeWithSelector(ISystem.originIsRoot.selector), - abi.encode(returnValue) + address(CONTROLLER.protocolRegistry()), + abi.encodeWithSelector( + IDotnsProtocolRegistry.get.selector, DotnsConstants.ROOT_GATEWAY + ), + abi.encode(enabled ? effectiveSender : ROOT_GATEWAY_STUB) ); } diff --git a/test/invariant/registrar/ReservedGrantHandler.t.sol b/test/invariant/registrar/ReservedGrantHandler.t.sol index 3bf0bf002..9e98dee64 100644 --- a/test/invariant/registrar/ReservedGrantHandler.t.sol +++ b/test/invariant/registrar/ReservedGrantHandler.t.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.34; import {Test} from "forge-std/Test.sol"; +import {VmSafe} from "forge-std/Vm.sol"; import { IDotnsRegistrarController, DotnsRegistrarController @@ -11,7 +12,7 @@ import {IDotnsReverseResolver} from "../../../contracts/resolvers/IDotnsReverseR import {IPopRules} from "../../../contracts/pop/IPopRules.sol"; import {DotnsRegistrar} from "../../../contracts/registrars/DotnsRegistrar.sol"; import {DotnsConstants} from "../../../contracts/utils/DotnsConstants.sol"; -import {ISystem} from "../../../contracts/external/revive/ISystem.sol"; +import {IDotnsProtocolRegistry} from "../../../contracts/registry/IDotnsProtocolRegistry.sol"; /// @title ReservedGrantHandler /// @notice Bounded random-action handler for the grant-gated reserved registration path. @@ -89,11 +90,11 @@ contract ReservedGrantHandler is Test { address beneficiary = actors[actorSeed % actors.length]; string memory label = _freshLabel(); - // The whitelist is Root-only. Restore the default afterwards: `registerReserved` reads - // `originIsRoot` too, and a sticky `true` would skip the grant check and the consume. - _mockOriginIsRoot(true); + // The whitelist admits governance only. Close the gate afterwards: `registerReserved` + // resolves the same key, and a sticky open gate would skip the grant check and consume. + _actAsGovernance(true); WHITELIST.grantName(label, beneficiary); - _mockOriginIsRoot(false); + _actAsGovernance(false); pendingLabels.push(label); grantedTo[label] = beneficiary; @@ -216,12 +217,33 @@ contract ReservedGrantHandler is Test { beneficiaries.push(account); } - /// @notice Mocks the revive `originIsRoot()` query for the next call. - function _mockOriginIsRoot(bool returnValue) internal { + /// @notice Stand-in address for the Root gateway, so the closed gate rejects every caller. + address internal constant ROOT_GATEWAY_STUB = address(uint160(0x600D6A7E)); + + /// @notice Opens or closes the governance gate for calls made from this handler. + /// @dev The gates authorise on `msg.sender == protocolRegistry.get(ROOT_GATEWAY)`, so this + /// points that key at the handler to open the gate and at an unrelated stub to close it. + /// A registry mock rather than a prank, so it does not collide with the per-call pranks + /// the handler already makes. + /// @param enabled True to admit calls from this handler, false to reject every caller. + function _actAsGovernance(bool enabled) internal { + // Whoever the next call will come from: `address(this)` normally, or the pranked address + // when the suite is inside a `vm.startPrank`, as the shared fixture is while it wires the + // protocol up. Pointing the key at the wrong one closes the gate on the fixture itself. + // + // `readCallers` reports the transaction-level sender, which is forge's default sender and + // NOT this contract when no prank is active, so its answer is only usable in prank modes. + (VmSafe.CallerMode mode, address pranked,) = vm.readCallers(); + address effectiveSender = (mode == VmSafe.CallerMode.Prank + || mode == VmSafe.CallerMode.RecurrentPrank) + ? pranked + : address(this); vm.mockCall( - DotnsConstants.REVIVE_SYSTEM, - abi.encodeWithSelector(ISystem.originIsRoot.selector), - abi.encode(returnValue) + address(CONTROLLER.protocolRegistry()), + abi.encodeWithSelector( + IDotnsProtocolRegistry.get.selector, DotnsConstants.ROOT_GATEWAY + ), + abi.encode(enabled ? effectiveSender : ROOT_GATEWAY_STUB) ); } diff --git a/test/invariant/whitelist/DotnsNameWhitelistInvariant.t.sol b/test/invariant/whitelist/DotnsNameWhitelistInvariant.t.sol index e8798f2b5..54daca70d 100644 --- a/test/invariant/whitelist/DotnsNameWhitelistInvariant.t.sol +++ b/test/invariant/whitelist/DotnsNameWhitelistInvariant.t.sol @@ -29,9 +29,9 @@ contract DotnsNameWhitelistInvariant is BaseDotns { ) ) ); - _mockOriginIsRoot(true); + _actAsGovernance(true); whitelist.setWindow(0, 3650 days); - _mockOriginIsRoot(false); + _actAsGovernance(false); vm.stopPrank(); address[] memory actors = new address[](4); diff --git a/test/invariant/whitelist/WhitelistHandler.t.sol b/test/invariant/whitelist/WhitelistHandler.t.sol index 50e8b738c..7a583245d 100644 --- a/test/invariant/whitelist/WhitelistHandler.t.sol +++ b/test/invariant/whitelist/WhitelistHandler.t.sol @@ -2,10 +2,11 @@ pragma solidity ^0.8.34; import {Test} from "forge-std/Test.sol"; +import {VmSafe} from "forge-std/Vm.sol"; import {DotnsNameWhitelist} from "../../../contracts/whitelist/DotnsNameWhitelist.sol"; import {IDotnsNameWhitelist} from "../../../contracts/whitelist/IDotnsNameWhitelist.sol"; import {DotnsConstants} from "../../../contracts/utils/DotnsConstants.sol"; -import {ISystem} from "../../../contracts/external/revive/ISystem.sol"; +import {IDotnsProtocolRegistry} from "../../../contracts/registry/IDotnsProtocolRegistry.sol"; /// @title WhitelistHandler /// @notice Drives the whitelist through its lifecycle for the invariant suite, cycling a fixed @@ -116,26 +117,47 @@ contract WhitelistHandler is Test { return _labels[seed % _labels.length]; } - /// @notice Puts the next call under a substrate Root origin. The whitelist's admin surface is - /// Root-only, so every governance action here needs it; a plain prank produces a Signed origin - /// and would revert into the catch, leaving the campaign to exercise only the permissionless - /// entry points. + /// @notice Opens the governance gate for the next call. The whitelist's admin surface admits + /// the Root gateway alone, so every governance action here needs it; without it the call + /// reverts into the catch and the campaign exercises only the permissionless entry points. function _asRoot() internal { - _mockOriginIsRoot(true); + _actAsGovernance(true); } - /// @notice Restores the default. `DotnsRegistrarController.registerReserved` reads - /// `originIsRoot` too, and handler state is campaign-scoped, so a sticky `true` would put a - /// later reserved registration on the Root branch. + /// @notice Closes the gate again. `DotnsRegistrarController.registerReserved` resolves the + /// same key, and handler state is campaign-scoped, so a sticky open gate would put a later + /// reserved registration on the governance branch. function _asSigned() internal { - _mockOriginIsRoot(false); - } - - function _mockOriginIsRoot(bool returnValue) internal { + _actAsGovernance(false); + } + + /// @notice Stand-in address for the Root gateway, so the closed gate rejects every caller. + address internal constant ROOT_GATEWAY_STUB = address(uint160(0x600D6A7E)); + + /// @notice Opens or closes the governance gate for calls made from this handler. + /// @dev The gates authorise on `msg.sender == protocolRegistry.get(ROOT_GATEWAY)`, so this + /// points that key at the handler to open the gate and at an unrelated stub to close it. + /// A registry mock rather than a prank, so it does not collide with the per-call pranks + /// the handler already makes. + /// @param enabled True to admit calls from this handler, false to reject every caller. + function _actAsGovernance(bool enabled) internal { + // Whoever the next call will come from: `address(this)` normally, or the pranked address + // when the suite is inside a `vm.startPrank`, as the shared fixture is while it wires the + // protocol up. Pointing the key at the wrong one closes the gate on the fixture itself. + // + // `readCallers` reports the transaction-level sender, which is forge's default sender and + // NOT this contract when no prank is active, so its answer is only usable in prank modes. + (VmSafe.CallerMode mode, address pranked,) = vm.readCallers(); + address effectiveSender = (mode == VmSafe.CallerMode.Prank + || mode == VmSafe.CallerMode.RecurrentPrank) + ? pranked + : address(this); vm.mockCall( - DotnsConstants.REVIVE_SYSTEM, - abi.encodeWithSelector(ISystem.originIsRoot.selector), - abi.encode(returnValue) + address(WHITELIST.protocolRegistry()), + abi.encodeWithSelector( + IDotnsProtocolRegistry.get.selector, DotnsConstants.ROOT_GATEWAY + ), + abi.encode(enabled ? effectiveSender : ROOT_GATEWAY_STUB) ); } } diff --git a/test/unit/governance/DotnsRootGateway.t.sol b/test/unit/governance/DotnsRootGateway.t.sol new file mode 100644 index 000000000..b74b1a1f1 --- /dev/null +++ b/test/unit/governance/DotnsRootGateway.t.sol @@ -0,0 +1,160 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.34; + +import {Test} from "forge-std/Test.sol"; + +import {DotnsRootGateway} from "../../../contracts/governance/DotnsRootGateway.sol"; +import {IDotnsRootGateway} from "../../../contracts/governance/IDotnsRootGateway.sol"; +import {IDotnsProtocolRegistry} from "../../../contracts/registry/IDotnsProtocolRegistry.sol"; +import {ISystem} from "../../../contracts/external/revive/ISystem.sol"; +import {DotnsConstants} from "../../../contracts/utils/DotnsConstants.sol"; + +/// @notice Minimal forwarding target that records what the gateway looked like as a caller. +contract TargetStub { + address public lastCaller; + uint256 public lastValue; + bool public shouldRevert; + + error TargetReverted(uint256 marker); + + function ping(uint256 value) external { + if (shouldRevert) revert TargetReverted(value); + lastCaller = msg.sender; + lastValue = value; + } + + function setShouldRevert(bool value) external { + shouldRevert = value; + } +} + +/// @title DotnsRootGatewayTest +/// @notice Covers the contract that carries the whole governance trust boundary. +/// @dev The registry is mocked rather than deployed: these tests are about the gateway's own +/// authorisation and forwarding, and a real registry would couple them to the full protocol +/// fixture. The gates that consume the gateway's identity are tested where they live. +contract DotnsRootGatewayTest is Test { + DotnsRootGateway internal gateway; + TargetStub internal target; + + address internal constant REGISTRY = address(uint160(0x9E6157A7)); + address internal constant OUTSIDER = address(uint160(0x0075106E)); + + function setUp() public { + gateway = new DotnsRootGateway(IDotnsProtocolRegistry(REGISTRY)); + target = new TargetStub(); + + vm.label(address(gateway), "DotnsRootGateway"); + vm.label(address(target), "TargetStub"); + + _mockCallerIsRoot(false); + _mockRegistered(address(target), true); + _mockRegistered(OUTSIDER, false); + } + + function test_constructor_rejects_a_zero_registry() public { + vm.expectRevert(IDotnsRootGateway.InvalidRegistry.selector); + new DotnsRootGateway(IDotnsProtocolRegistry(address(0))); + } + + /// @notice The gate is `callerIsRoot`, not the transaction origin. + /// @dev This is the whole point of the contract. A signed caller is rejected even though it + /// can reach the function, and nothing about the transaction it sits in changes that. + function test_execute_rejects_a_non_root_caller() public { + _mockCallerIsRoot(false); + vm.expectRevert(IDotnsRootGateway.NotRoot.selector); + gateway.execute(_one(address(target)), _one(abi.encodeCall(TargetStub.ping, (1)))); + } + + function test_execute_forwards_and_the_target_sees_the_gateway() public { + _mockCallerIsRoot(true); + gateway.execute(_one(address(target)), _one(abi.encodeCall(TargetStub.ping, (42)))); + + // The entire authorisation mechanism downstream: a regular CALL, so the callee observes + // the gateway as `msg.sender` and can compare it against the registry key. + assertEq(target.lastCaller(), address(gateway)); + assertEq(target.lastValue(), 42); + } + + function test_execute_rejects_a_target_outside_the_protocol() public { + _mockCallerIsRoot(true); + vm.expectRevert( + abi.encodeWithSelector(IDotnsRootGateway.TargetNotProtocol.selector, OUTSIDER) + ); + gateway.execute(_one(OUTSIDER), _one(abi.encodeCall(TargetStub.ping, (1)))); + } + + function test_execute_rejects_mismatched_lengths() public { + _mockCallerIsRoot(true); + address[] memory targets = new address[](2); + targets[0] = address(target); + targets[1] = address(target); + vm.expectRevert(IDotnsRootGateway.LengthMismatch.selector); + gateway.execute(targets, _one(abi.encodeCall(TargetStub.ping, (1)))); + } + + function test_execute_rejects_an_empty_batch() public { + _mockCallerIsRoot(true); + vm.expectRevert(IDotnsRootGateway.EmptyBatch.selector); + gateway.execute(new address[](0), new bytes[](0)); + } + + /// @notice A failing call takes the whole batch with it, and the callee's error survives. + /// @dev Governance reads the real revert reason rather than a generic gateway failure, and a + /// partially applied batch is never observable. + function test_execute_bubbles_the_callee_revert_and_applies_nothing() public { + _mockCallerIsRoot(true); + target.setShouldRevert(true); + + address[] memory targets = new address[](2); + targets[0] = address(target); + targets[1] = address(target); + bytes[] memory payloads = new bytes[](2); + payloads[0] = abi.encodeCall(TargetStub.ping, (7)); + payloads[1] = abi.encodeCall(TargetStub.ping, (8)); + + vm.expectRevert(abi.encodeWithSelector(TargetStub.TargetReverted.selector, uint256(7))); + gateway.execute(targets, payloads); + + assertEq(target.lastCaller(), address(0)); + } + + function test_execute_applies_a_batch_in_order() public { + _mockCallerIsRoot(true); + address[] memory targets = new address[](2); + targets[0] = address(target); + targets[1] = address(target); + bytes[] memory payloads = new bytes[](2); + payloads[0] = abi.encodeCall(TargetStub.ping, (1)); + payloads[1] = abi.encodeCall(TargetStub.ping, (2)); + + gateway.execute(targets, payloads); + assertEq(target.lastValue(), 2); + } + + function _mockCallerIsRoot(bool value) private { + vm.mockCall( + DotnsConstants.REVIVE_SYSTEM, + abi.encodeWithSelector(ISystem.callerIsRoot.selector), + abi.encode(value) + ); + } + + function _mockRegistered(address addr, bool value) private { + vm.mockCall( + REGISTRY, + abi.encodeWithSelector(IDotnsProtocolRegistry.isRegisteredAddress.selector, addr), + abi.encode(value) + ); + } + + function _one(address addr) private pure returns (address[] memory list) { + list = new address[](1); + list[0] = addr; + } + + function _one(bytes memory payload) private pure returns (bytes[] memory list) { + list = new bytes[](1); + list[0] = payload; + } +} diff --git a/test/unit/pop/PopRules.t.sol b/test/unit/pop/PopRules.t.sol index ee8599a6b..296c135fa 100644 --- a/test/unit/pop/PopRules.t.sol +++ b/test/unit/pop/PopRules.t.sol @@ -224,32 +224,32 @@ contract PopRulesTests is BaseDotns { } function test_setShortNamesEnabled_emits() public { - _mockOriginIsRoot(true); + _actAsGovernance(true); vm.expectEmit(address(popRules)); emit IPopRules.ShortNamesEnabledUpdated(false); popRules.setShortNamesEnabled(false); } function test_setShortNamesEnabled_requires_root() public { - // A non-Root origin: originIsRoot returns false, so the setter reverts NotRoot regardless - // of the caller (owner included). - _mockOriginIsRoot(false); + // Gate closed: the `ROOT_GATEWAY` key points somewhere else, so the setter reverts NotRoot + // regardless of the caller (owner included). + _actAsGovernance(false); vm.expectRevert(IPopRules.NotRoot.selector); vm.prank(ed); popRules.setShortNamesEnabled(false); } function test_setShortNamesEnabled_owner_reverts() public { - // The owner used to hold this switch. It is now Root-only, so the owner reverts NotRoot - // too. - _mockOriginIsRoot(false); + // The owner used to hold this switch. It is now governance-only, so the owner reverts + // NotRoot too. + _actAsGovernance(false); vm.expectRevert(IPopRules.NotRoot.selector); vm.prank(owner); popRules.setShortNamesEnabled(false); } function test_setShortNamesEnabled_root_succeeds() public { - _mockOriginIsRoot(true); + _actAsGovernance(true); popRules.setShortNamesEnabled(true); assertTrue(popRules.shortNamesEnabled()); } diff --git a/test/unit/registrar/DotnsPopController.t.sol b/test/unit/registrar/DotnsPopController.t.sol index b17a55442..75f3f4d3d 100644 --- a/test/unit/registrar/DotnsPopController.t.sol +++ b/test/unit/registrar/DotnsPopController.t.sol @@ -71,7 +71,7 @@ contract DotnsPopControllerTests is BaseDotns { } function test_reserveBaseName_reverts_when_origin_is_not_root() public { - _mockOriginIsRoot(false); + _actAsGovernance(false); vm.expectRevert(IDotnsPopController.NotRoot.selector); dotnsPopController.reserveBaseName( IDotnsPopController.BaseReservation({ @@ -213,7 +213,7 @@ contract DotnsPopControllerTests is BaseDotns { function test_registerBaseName_reverts_when_origin_is_not_root() public { IDotnsPopController.Link memory link = _linkFresh(_validChatKey(0xaa)); - _mockOriginIsRoot(false); + _actAsGovernance(false); vm.expectRevert(IDotnsPopController.NotRoot.selector); dotnsPopController.registerBaseName( IDotnsPopController.FullRegistration({label: BASE_LABEL_A, user: ed, link: link}) @@ -757,7 +757,7 @@ contract DotnsPopControllerTests is BaseDotns { function test_registered_controller_without_root_origin_cannot_enter_pop_flow() public { // The public commit-reveal controller is already a registered controller. // Even from that origin, the Root-gate must reject the call. - _mockOriginIsRoot(false); + _actAsGovernance(false); vm.prank(address(dotnsRegistrarController)); vm.expectRevert(IDotnsPopController.NotRoot.selector); dotnsPopController.reserveBaseName( @@ -1149,7 +1149,7 @@ contract DotnsPopControllerTests is BaseDotns { } function test_reserveLiteName_reverts_when_origin_is_not_root() public { - _mockOriginIsRoot(false); + _actAsGovernance(false); vm.expectRevert(IDotnsPopController.NotRoot.selector); dotnsPopController.reserveLiteName( IDotnsPopController.LiteRegistration({ @@ -1200,7 +1200,7 @@ contract DotnsPopControllerTests is BaseDotns { } function test_reserveBaseNameOnly_reverts_when_origin_is_not_root() public { - _mockOriginIsRoot(false); + _actAsGovernance(false); vm.expectRevert(IDotnsPopController.NotRoot.selector); dotnsPopController.reserveBaseNameOnly( IDotnsPopController.BaseNameReservation({user: ed, reservedBaseLabel: BASE_LABEL_A}) diff --git a/test/unit/registrar/DotnsRegistrarController.t.sol b/test/unit/registrar/DotnsRegistrarController.t.sol index 0fa85c112..8d527b421 100644 --- a/test/unit/registrar/DotnsRegistrarController.t.sol +++ b/test/unit/registrar/DotnsRegistrarController.t.sol @@ -370,8 +370,8 @@ contract DotnsRegistrarControllerTest is BaseDotns { } /// @dev The controller exposes no role surface at all: reserved registration reads grants - /// from `DotnsNameWhitelist`, whose own admin surface is Root-only, so there is no role to - /// hold and nothing to advertise. + /// from `DotnsNameWhitelist`, whose own admin surface is governance-only, so there is no role + /// to hold and nothing to advertise. function test_controller_advertises_no_role_interface() public view { assertTrue( dotnsRegistrarController.supportsInterface(type(IDotnsRegistrarController).interfaceId) @@ -453,9 +453,9 @@ contract DotnsRegistrarControllerTest is BaseDotns { address nameOwner = ed; _grantName(nameLabel, nameOwner); - _mockOriginIsRoot(true); + _actAsGovernance(true); dotnsNameWhitelist.revokeName(nameLabel); - _mockOriginIsRoot(false); + _actAsGovernance(false); assertFalse(dotnsNameWhitelist.isGrantedTo(nameLabel, nameOwner)); @@ -985,14 +985,16 @@ contract DotnsRegistrarControllerTest is BaseDotns { // survive; an unrelated label could not detect consumption of the one being minted. _grantName(nameLabel, ed); - _mockOriginIsRoot(true); + // `tiago` submits, so `tiago` is the gateway for this call: the governance branch is + // selected by `msg.sender`, not by anything about the transaction it sits in. + _actAsGovernanceFor(tiago); _revealReserved(nameLabel, ed, tiago); - _mockOriginIsRoot(false); + _actAsGovernance(false); assertEq(dotnsRegistrar.ownerOf(_tokenIdForLabel(nameLabel)), ed); assertTrue( dotnsNameWhitelist.isGrantedTo(nameLabel, ed), - "a Root mint consumed the grant on the label it minted" + "a governance mint consumed the grant on the label it minted" ); } @@ -1085,10 +1087,10 @@ contract DotnsRegistrarControllerTest is BaseDotns { IDotnsRegistrarController.Registration memory registration = _commitReserved(nameLabel, ed, ed); - _mockOriginIsRoot(true); + _actAsGovernanceFor(ed); vm.prank(ed); dotnsRegistrarController.registerReserved(registration); - _mockOriginIsRoot(false); + _actAsGovernance(false); assertEq(dotnsRegistrar.ownerOf(_tokenIdForLabel(nameLabel)), ed); } diff --git a/test/unit/whitelist/DotnsNameWhitelist.t.sol b/test/unit/whitelist/DotnsNameWhitelist.t.sol index 95348ffec..d783ffcdd 100644 --- a/test/unit/whitelist/DotnsNameWhitelist.t.sol +++ b/test/unit/whitelist/DotnsNameWhitelist.t.sol @@ -12,10 +12,11 @@ import {Upgrades} from "openzeppelin-foundry-upgrades/Upgrades.sol"; /// @title DotnsNameWhitelist unit tests /// @notice Covers claiming, competing claims and resolution, the request window, reservation, the /// controller-only consume hook, and the review views. -/// @dev No test here pranks a caller for a governance action. The admin surface is Root-only and -/// the gates read no `msg.sender`, so who submits is irrelevant and a prank would imply an -/// owner authority that does not exist. The negative cases mock a signed origin instead and -/// assert `NotGovernance`. +/// @dev The admin surface admits exactly one caller, the Root gateway, so who submits is the +/// whole question. Positive cases open the gate for this contract with +/// `_actAsGovernance(true)` rather than pranking an account, since pranking the owner would +/// imply an owner authority that does not exist. Negative cases close the gate, or call from +/// a third party or an intermediate contract, and assert `NotGovernance`. contract DotnsNameWhitelistTests is BaseDotns { DotnsNameWhitelist internal whitelist; @@ -36,9 +37,10 @@ contract DotnsNameWhitelistTests is BaseDotns { ); vm.stopPrank(); - // The whitelist's admin surface is Root-only, so the suite runs under a Root origin and - // the unauthorised-caller tests flip it back with `_mockOriginIsRoot(false)`. - _mockOriginIsRoot(true); + // The whitelist's admin surface is governance-only, so the suite runs with the gate open + // for this contract and the unauthorised-caller tests close it with + // `_actAsGovernance(false)`. + _actAsGovernance(true); whitelist.setWindow(0, 30 days); vm.label(address(whitelist), "DotnsNameWhitelist"); @@ -165,7 +167,7 @@ contract DotnsNameWhitelistTests is BaseDotns { function test_accept_reverts_for_a_signed_caller() public { _request(ed, BASE_LABEL_A); - _mockOriginIsRoot(false); + _actAsGovernance(false); vm.expectRevert(IDotnsNameWhitelist.NotGovernance.selector); vm.prank(tiago); whitelist.accept(BASE_LABEL_A, ed); @@ -226,7 +228,7 @@ contract DotnsNameWhitelistTests is BaseDotns { /// method that later lost its gate, which is how the four spot checks below left `reject`, /// `grantNames`, `setWindow` and two caps uncovered. function test_every_admin_entry_point_requires_root() public { - _mockOriginIsRoot(false); + _actAsGovernance(false); string[] memory batch = new string[](1); batch[0] = BASE_LABEL_B; @@ -323,10 +325,10 @@ contract DotnsNameWhitelistTests is BaseDotns { assertEq(whitelist.nameCount(), 0); } - /// @dev Not even the owner revokes: the admin surface is Root-only. + /// @dev Not even the owner revokes: the admin surface admits the Root gateway alone. function test_revokeName_rejects_the_owner() public { _grant(ed, BASE_LABEL_A); - _mockOriginIsRoot(false); + _actAsGovernance(false); vm.expectRevert(IDotnsNameWhitelist.NotGovernance.selector); whitelist.revokeName(BASE_LABEL_A); } @@ -396,7 +398,7 @@ contract DotnsNameWhitelistTests is BaseDotns { } function test_setReserved_reverts_for_a_signed_caller() public { - _mockOriginIsRoot(false); + _actAsGovernance(false); vm.expectRevert(IDotnsNameWhitelist.NotGovernance.selector); vm.prank(tiago); whitelist.setReserved(BASE_LABEL_A, true); @@ -499,10 +501,10 @@ contract DotnsNameWhitelistTests is BaseDotns { } function test_names_pagination_lists_active() public { - vm.startPrank(owner); + // No prank: governance is the gateway, and pranking the owner here would imply an owner + // authority over the admin surface that does not exist. whitelist.grantName(BASE_LABEL_A, ed); whitelist.setReserved(BASE_LABEL_B, true); - vm.stopPrank(); assertEq(whitelist.nameCount(), 2); IDotnsNameWhitelist.NameView[] memory page = whitelist.names(0, 10); @@ -585,33 +587,76 @@ contract DotnsNameWhitelistTests is BaseDotns { whitelist.setMaxGrantBatch(aboveLimit); } - /// @dev Caps are configuration, and configuration is Root-only too: the owner cannot retune. + /// @dev Caps are configuration, and configuration is governance-only too: the owner cannot + /// retune them. function test_setMaxClaimants_reverts_for_a_signed_caller() public { - _mockOriginIsRoot(false); + _actAsGovernance(false); vm.expectRevert(IDotnsNameWhitelist.NotGovernance.selector); whitelist.setMaxClaimants(10); } - function test_governance_root_grants_from_any_caller() public { - // Root has no address, so the gate admits the call regardless of who submits it. - _mockOriginIsRoot(true); + /// @notice The admin surface names one caller and rejects every other. + /// @dev Regression cover for a confused deputy. If the gate is ever widened to a property of + /// the transaction rather than an address, for example `ISystem.originIsRoot`, these pass + /// for any caller and the whole admin surface belongs to whoever a governance dispatch + /// happens to reach. They are meant to fail loudly if that is attempted. + function test_governance_rejects_a_third_party_caller_on_grant() public { + _actAsGovernance(true); + vm.expectRevert(IDotnsNameWhitelist.NotGovernance.selector); vm.prank(leonardo); whitelist.grantName(BASE_LABEL_A, ed); - assertEq(whitelist.granteeOf(BASE_LABEL_A), ed); } - function test_governance_root_reserves_from_any_caller() public { - _mockOriginIsRoot(true); + function test_governance_rejects_a_third_party_caller_on_reserve() public { + _actAsGovernance(true); + vm.expectRevert(IDotnsNameWhitelist.NotGovernance.selector); vm.prank(leonardo); whitelist.setReserved(BASE_LABEL_A, true); - assertTrue(whitelist.isReserved(BASE_LABEL_A)); } - /// @dev Configuration is Root-only as well, so a referendum retunes the caps. - function test_governance_root_sets_a_cap_from_any_caller() public { - _mockOriginIsRoot(true); + function test_governance_rejects_a_third_party_caller_on_a_cap() public { + _actAsGovernance(true); + vm.expectRevert(IDotnsNameWhitelist.NotGovernance.selector); vm.prank(leonardo); whitelist.setMaxClaimants(7); - assertEq(whitelist.maxClaimants(), 7); + } + + /// @notice A contract reached during a governance transaction holds no authority. + /// @dev The gate is open for this test contract, and the intruder is called from inside the + /// same transaction. It must still be rejected: authority is an address, not a property + /// the whole call stack shares. + /// + /// Exercised through a nested call on purpose. A direct call cannot distinguish an + /// address gate from a transaction-scoped one; only an intermediate frame can. + function test_governance_rejects_an_intermediate_contract() public { + Intruder intruder = new Intruder(); + _actAsGovernance(true); + + vm.expectRevert(IDotnsNameWhitelist.NotGovernance.selector); + intruder.grant(IDotnsNameWhitelist(address(whitelist)), BASE_LABEL_A, ed); + } + + /// @notice The gate fails closed when the `ROOT_GATEWAY` key has never been wired. + /// @dev An unset key reads as the zero address. Without the explicit non-zero guard a caller + /// of `address(0)` would match it, and a partially wired deployment would sit open. + function test_governance_rejects_when_the_gateway_key_is_unset() public { + vm.mockCall( + address(protocolRegistry), + abi.encodeWithSelector( + IDotnsProtocolRegistry.get.selector, DotnsConstants.ROOT_GATEWAY + ), + abi.encode(address(0)) + ); + vm.expectRevert(IDotnsNameWhitelist.NotGovernance.selector); + whitelist.setMaxClaimants(7); + } +} + +/// @notice Stands in for any contract a governance transaction happens to reach. +/// @dev Holds no DotNS key and is named by nothing. Under a transaction-scoped gate that is +/// enough to hold the whole admin surface, which is why the gate is an address. +contract Intruder { + function grant(IDotnsNameWhitelist whitelist, string memory label, address user) external { + whitelist.grantName(label, user); } }