From 6624c426ce4fb1fe8ea41aab04f045da836723a2 Mon Sep 17 00:00:00 2001 From: makemake Date: Fri, 24 Jul 2026 13:43:23 +0200 Subject: [PATCH 1/5] feat(safe): add guard lifecycle scripts --- .gitignore | 3 + examples/safe-guard/README.md | 68 +++++- .../script/DeployCredibleSafeGuard.s.sol | 28 +++ .../script/GenerateSafeGuardBatch.s.sol | 35 +++ examples/safe-guard/script/SafeGuardBatch.sol | 154 ++++++++++++++ .../CredibleSafeGuardSafeIntegration.t.sol | 46 ++++ .../test/CredibleSafeGuardScripts.t.sol | 199 ++++++++++++++++++ foundry.toml | 2 + src/protection/safe/CredibleSafeGuard.sol | 84 ++++++-- src/protection/safe/README.md | 13 +- test/protection/safe/CredibleSafeGuard.t.sol | 67 +++++- 11 files changed, 672 insertions(+), 27 deletions(-) create mode 100644 examples/safe-guard/script/DeployCredibleSafeGuard.s.sol create mode 100644 examples/safe-guard/script/GenerateSafeGuardBatch.s.sol create mode 100644 examples/safe-guard/script/SafeGuardBatch.sol create mode 100644 examples/safe-guard/test/CredibleSafeGuardScripts.t.sol diff --git a/.gitignore b/.gitignore index 4982444..3f3886b 100644 --- a/.gitignore +++ b/.gitignore @@ -8,6 +8,9 @@ out/ # Forge broadcast logs at repo root /broadcast/ +# Generated Safe Transaction Builder batches +/safe-guard-output/ + # Docs docs/ diff --git a/examples/safe-guard/README.md b/examples/safe-guard/README.md index 004c98b..88c6dfb 100644 --- a/examples/safe-guard/README.md +++ b/examples/safe-guard/README.md @@ -16,9 +16,75 @@ These integration tests live under their own profile because the Safe contracts - A signed Safe transaction executing in a credible block. - A signed Safe transaction reverting with `NonCredibleBlock` in a non-credible block while the builder set is live. - The fail-open path: a signed Safe transaction executing once the builder set has been silent past the configured window. +- Registry-unavailability fail-open paths, including reverted and malformed registry responses. - An end-to-end builder stall-then-recover sequence. -## Run +## Deploy the guard + +The deployment script reads the registry and fail-open threshold from the environment. Foundry handles the deployer wallet; the script does not read a private key. + +```sh +FOUNDRY_PROFILE=safe-guard \ +CREDIBLE_REGISTRY=0x... \ +FAIL_OPEN_BLOCK_THRESHOLD=75 \ +forge script examples/safe-guard/script/DeployCredibleSafeGuard.s.sol \ + --rpc-url "$RPC_URL" \ + --account \ + --broadcast +``` + +Foundry hardware-wallet flags such as `--ledger` or `--trezor` can be used instead of `--account`. + +## Create a Safe{Wallet} installation transaction + +`GenerateSafeGuardBatch.s.sol` creates an official Safe Transaction Builder JSON batch. It does not sign, broadcast, call `execTransaction`, or submit anything to the Safe Transaction Service. + +```sh +FOUNDRY_PROFILE=safe-guard \ +SAFE_ADDRESS=0x... \ +SAFE_GUARD_ACTION=install \ +CREDIBLE_SAFE_GUARD=0x... \ +forge script examples/safe-guard/script/GenerateSafeGuardBatch.s.sol \ + --rpc-url "$RPC_URL" +``` + +The output is written to: + +```text +safe-guard-output/install.json +``` + +To install the guard: + +1. Open the target Safe in Safe{Wallet} on the same chain used to generate the file. +2. Open **Apps → Transaction Builder**. +3. Import `safe-guard-output/install.json`. +4. Verify that the transaction target is the Safe itself, the value is zero, and the decoded call is `setGuard()`. +5. Create the Safe transaction and complete the Safe's normal owner confirmation and execution flow. + +Safe's `setGuard` function is self-authorized. A Safe owner cannot call it directly; the call must execute through an owner-authorized Safe transaction targeting the Safe itself. The generated batch provides exactly that inner transaction and includes a Transaction Builder checksum so the UI can detect modification. + +## Create a removal transaction + +Removal uses the same Safe self-call with the guard set to the zero address: + +```sh +FOUNDRY_PROFILE=safe-guard \ +SAFE_ADDRESS=0x... \ +SAFE_GUARD_ACTION=remove \ +forge script examples/safe-guard/script/GenerateSafeGuardBatch.s.sol \ + --rpc-url "$RPC_URL" +``` + +Import `safe-guard-output/remove.json` into Transaction Builder and verify that it displays `setGuard(0x0000000000000000000000000000000000000000)` before submitting it. + +If a guard is already installed, that current guard checks a replacement or removal transaction before Safe executes it. `CredibleSafeGuard` therefore requires the transaction to land in a credible block while the builder set is live. Its fail-open behavior still permits recovery if the builder set is stale or a required registry read is unavailable or malformed. + +Importing a batch does not automatically publish it to the Safe transaction queue. The owner creates the proposal from Transaction Builder after reviewing the imported action. Automatically inserting a transaction into the queue would require a separate Safe owner signature and Safe Transaction Service integration. + +Generated files under `safe-guard-output/` are ignored by Git. Always verify the chain, Safe address, action, guard address, and decoded call in Safe{Wallet} before signing. + +## Run tests ```sh FOUNDRY_PROFILE=safe-guard forge test diff --git a/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol b/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol new file mode 100644 index 0000000..006576f --- /dev/null +++ b/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.13; + +import {Script, console2} from "forge-std/Script.sol"; + +import {CredibleSafeGuard} from "credible-std/protection/safe/CredibleSafeGuard.sol"; +import {ICredibleRegistry} from "credible-std/protection/safe/ICredibleRegistry.sol"; + +/// @notice Deploys a Credible Safe guard using Foundry's configured broadcast wallet. +contract DeployCredibleSafeGuard is Script { + function run() external returns (CredibleSafeGuard guard) { + address registry = vm.envAddress("CREDIBLE_REGISTRY"); + uint256 threshold = vm.envUint("FAIL_OPEN_BLOCK_THRESHOLD"); + + vm.startBroadcast(); + guard = deploy(registry, threshold); + vm.stopBroadcast(); + + console2.log("Chain ID:", block.chainid); + console2.log("Credible Safe guard:", address(guard)); + console2.log("Credible Registry:", registry); + console2.log("Fail-open block threshold:", threshold); + } + + function deploy(address registry, uint256 threshold) public returns (CredibleSafeGuard) { + return new CredibleSafeGuard(ICredibleRegistry(registry), threshold); + } +} diff --git a/examples/safe-guard/script/GenerateSafeGuardBatch.s.sol b/examples/safe-guard/script/GenerateSafeGuardBatch.s.sol new file mode 100644 index 0000000..3543480 --- /dev/null +++ b/examples/safe-guard/script/GenerateSafeGuardBatch.s.sol @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.13; + +import {console2} from "forge-std/Script.sol"; + +import {SafeGuardBatch} from "./SafeGuardBatch.sol"; + +/// @notice Generates a Safe Transaction Builder JSON file for installing or removing a guard. +/// @dev This script never broadcasts or signs. Import the generated file into Safe{Wallet}'s +/// Transaction Builder and submit it through the Safe's normal owner-confirmation flow. +contract GenerateSafeGuardBatch is SafeGuardBatch { + function run() external returns (string memory json, string memory path) { + address safe = vm.envAddress("SAFE_ADDRESS"); + Action action = parseAction(vm.envString("SAFE_GUARD_ACTION")); + address guard = vm.envOr("CREDIBLE_SAFE_GUARD", address(0)); + uint256 createdAt = vm.unixTime(); + + if (action == Action.Install) { + json = buildInstallBatch(safe, guard, block.chainid, createdAt); + } else { + json = buildRemoveBatch(safe, block.chainid, createdAt); + } + + path = outputPath(action); + vm.createDir("safe-guard-output", true); + vm.writeFile(path, json); + + console2.log("Safe Transaction Builder file:", path); + console2.log("Chain ID:", block.chainid); + console2.log("Safe:", safe); + console2.log("Action:", action == Action.Install ? "install" : "remove"); + console2.log("Guard:", action == Action.Install ? guard : address(0)); + console2.log("Import this JSON in Safe{Wallet} Transaction Builder and verify it before signing."); + } +} diff --git a/examples/safe-guard/script/SafeGuardBatch.sol b/examples/safe-guard/script/SafeGuardBatch.sol new file mode 100644 index 0000000..f2a153e --- /dev/null +++ b/examples/safe-guard/script/SafeGuardBatch.sol @@ -0,0 +1,154 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.13; + +import {Script} from "forge-std/Script.sol"; + +import {IERC165, ITransactionGuard} from "credible-std/protection/safe/CredibleSafeGuard.sol"; + +/// @notice Builds Safe Transaction Builder batches for installing and removing a Safe guard. +/// @dev The checksum implementation matches Safe Transaction Builder's canonical serializer for +/// the fixed one-transaction schema emitted by this contract. +abstract contract SafeGuardBatch is Script { + enum Action { + Install, + Remove + } + + bytes4 internal constant SAFE_GUARD_INTERFACE_ID = type(ITransactionGuard).interfaceId; + string internal constant TX_BUILDER_VERSION = "2.0.1"; + + error ZeroSafeAddress(); + error SafeHasNoCode(address safe); + error ZeroGuardAddress(); + error GuardHasNoCode(address guard); + error UnsupportedGuardInterface(address guard); + error InvalidAction(string action); + + function buildInstallBatch(address safe, address guard, uint256 chainId, uint256 createdAt) + public + view + returns (string memory) + { + _validateSafe(safe); + _validateGuard(guard); + return _buildBatch(safe, guard, Action.Install, chainId, createdAt); + } + + function buildRemoveBatch(address safe, uint256 chainId, uint256 createdAt) public view returns (string memory) { + _validateSafe(safe); + return _buildBatch(safe, address(0), Action.Remove, chainId, createdAt); + } + + function parseAction(string memory action) public pure returns (Action) { + bytes32 actionHash = keccak256(bytes(action)); + if (actionHash == keccak256("install")) return Action.Install; + if (actionHash == keccak256("remove")) return Action.Remove; + revert InvalidAction(action); + } + + function outputPath(Action action) public pure returns (string memory) { + return action == Action.Install ? "safe-guard-output/install.json" : "safe-guard-output/remove.json"; + } + + function calculateChecksum(address safe, address guard, uint256 chainId, uint256 createdAt) + public + view + returns (bytes32) + { + string memory safeString = vm.toString(safe); + string memory guardString = vm.toString(guard); + string memory chainIdString = vm.toString(chainId); + string memory createdAtString = vm.toString(createdAt); + + string memory canonicalMeta = string.concat( + '{["createdFromOwnerAddress","createdFromSafeAddress","description","name","txBuilderVersion"]', + '"",', + _quote(safeString), + ',"",null,"', + TX_BUILDER_VERSION, + '",}' + ); + + string memory canonicalInput = string.concat('{["internalType","name","type"]', '"address","guard","address",}'); + string memory canonicalMethod = + string.concat('{["inputs","name","payable"]', "[", canonicalInput, "]", ',"setGuard",false,}'); + string memory canonicalInputs = string.concat('{["guard"]', _quote(guardString), ",}"); + string memory canonicalTransaction = string.concat( + '{["contractInputsValues","contractMethod","data","to","value"]', + canonicalInputs, + ",", + canonicalMethod, + ",null,", + _quote(safeString), + ',"0",}' + ); + string memory canonicalRoot = string.concat( + '{["chainId","createdAt","meta","transactions","version"]', + _quote(chainIdString), + ",", + createdAtString, + ",", + canonicalMeta, + ",[", + canonicalTransaction, + '],"1.0",}' + ); + + return keccak256(bytes(canonicalRoot)); + } + + function _buildBatch(address safe, address guard, Action action, uint256 chainId, uint256 createdAt) + internal + view + returns (string memory) + { + string memory safeString = vm.toString(safe); + string memory guardString = vm.toString(guard); + string memory checksum = vm.toString(calculateChecksum(safe, guard, chainId, createdAt)); + string memory name = action == Action.Install ? "Install Credible Safe Guard" : "Remove Credible Safe Guard"; + + return string.concat( + '{"version":"1.0","chainId":', + _quote(vm.toString(chainId)), + ',"createdAt":', + vm.toString(createdAt), + ',"meta":{"name":', + _quote(name), + ',"description":"","txBuilderVersion":"', + TX_BUILDER_VERSION, + '","createdFromSafeAddress":', + _quote(safeString), + ',"createdFromOwnerAddress":"","checksum":', + _quote(checksum), + '},"transactions":[{"to":', + _quote(safeString), + ',"value":"0","data":null,"contractMethod":{"inputs":[{"internalType":"address","name":"guard","type":"address"}],"name":"setGuard","payable":false},"contractInputsValues":{"guard":', + _quote(guardString), + "}}]}" + ); + } + + function _validateSafe(address safe) internal view { + if (safe == address(0)) revert ZeroSafeAddress(); + if (safe.code.length == 0) revert SafeHasNoCode(safe); + } + + function _validateGuard(address guard) internal view { + if (guard == address(0)) revert ZeroGuardAddress(); + if (guard.code.length == 0) revert GuardHasNoCode(guard); + + (bool success, bytes memory result) = + guard.staticcall(abi.encodeCall(IERC165.supportsInterface, (SAFE_GUARD_INTERFACE_ID))); + uint256 supported; + if (success && result.length == 32) { + assembly ("memory-safe") { + supported := mload(add(result, 0x20)) + } + } + if (!success || result.length != 32 || supported != 1) revert UnsupportedGuardInterface(guard); + } + + function _quote(string memory value) internal pure returns (string memory) { + return string.concat('"', value, '"'); + } +} diff --git a/examples/safe-guard/test/CredibleSafeGuardSafeIntegration.t.sol b/examples/safe-guard/test/CredibleSafeGuardSafeIntegration.t.sol index 7b0c1ef..1a41e12 100644 --- a/examples/safe-guard/test/CredibleSafeGuardSafeIntegration.t.sol +++ b/examples/safe-guard/test/CredibleSafeGuardSafeIntegration.t.sol @@ -98,6 +98,52 @@ contract CredibleSafeGuardSafeIntegrationTest is Test { assertEq(safe.nonce(), nonceBefore + 1); } + function test_realSafe_failsOpenWhenCredibilityReadReverts() public { + vm.mockCallRevert( + address(registry), abi.encodeWithSignature("isCredibleBlock(uint256)", block.number), "registry unavailable" + ); + + uint256 nonceBefore = safe.nonce(); + assertTrue(_execSafeTx(owner, 0, "")); + assertEq(safe.nonce(), nonceBefore + 1); + } + + function test_realSafe_canRemoveGuardWhenRegistryReadReverts() public { + vm.mockCallRevert( + address(registry), abi.encodeWithSignature("isCredibleBlock(uint256)", block.number), "registry unavailable" + ); + + bytes memory setGuardData = abi.encodeWithSignature("setGuard(address)", address(0)); + bytes memory sig = _signTx(address(safe), 0, setGuardData); + uint256 nonceBefore = safe.nonce(); + + assertTrue( + safe.execTransaction( + address(safe), 0, setGuardData, Enum.Operation.Call, 0, 0, 0, address(0), payable(address(0)), sig + ) + ); + assertEq(safe.nonce(), nonceBefore + 1); + assertEq(_installedGuard(), address(0)); + } + + function test_realSafe_failsOpenWhenCredibilityReadIsMalformed() public { + vm.mockCall( + address(registry), abi.encodeWithSignature("isCredibleBlock(uint256)", block.number), abi.encode(uint256(2)) + ); + + uint256 nonceBefore = safe.nonce(); + assertTrue(_execSafeTx(owner, 0, "")); + assertEq(safe.nonce(), nonceBefore + 1); + } + + function test_realSafe_failsOpenWhenLastBlockReadReverts() public { + vm.mockCallRevert(address(registry), abi.encodeWithSignature("lastCredibleBlock()"), "registry unavailable"); + + uint256 nonceBefore = safe.nonce(); + assertTrue(_execSafeTx(owner, 0, "")); + assertEq(safe.nonce(), nonceBefore + 1); + } + function test_realSafe_endToEnd_stallThenRecover() public { // Builder healthy: current block is credible -> allowed. registry.markCurrentBlockCredible(); diff --git a/examples/safe-guard/test/CredibleSafeGuardScripts.t.sol b/examples/safe-guard/test/CredibleSafeGuardScripts.t.sol new file mode 100644 index 0000000..c5920dd --- /dev/null +++ b/examples/safe-guard/test/CredibleSafeGuardScripts.t.sol @@ -0,0 +1,199 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.13; + +import {Test} from "forge-std/Test.sol"; + +import {Safe} from "../../../lib/safe-smart-account/contracts/Safe.sol"; +import {SafeProxyFactory} from "../../../lib/safe-smart-account/contracts/proxies/SafeProxyFactory.sol"; +import {Enum} from "../../../lib/safe-smart-account/contracts/common/Enum.sol"; + +import {CredibleSafeGuard} from "credible-std/protection/safe/CredibleSafeGuard.sol"; +import {CredibleRegistryMock} from "../src/CredibleRegistryMock.sol"; +import {DeployCredibleSafeGuard} from "../script/DeployCredibleSafeGuard.s.sol"; +import {GenerateSafeGuardBatch} from "../script/GenerateSafeGuardBatch.s.sol"; +import {SafeGuardBatch} from "../script/SafeGuardBatch.sol"; + +contract UnsupportedGuard {} + +contract CredibleSafeGuardScriptsTest is Test { + bytes32 internal constant GUARD_STORAGE_SLOT = 0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8; + bytes32 internal constant REFERENCE_CHECKSUM = 0x8994ee462d748c24ecd7804083007dd231e36ff84da4b272921c30d1ae7f0df0; + + uint256 internal constant THRESHOLD = 75; + uint256 internal constant CREATED_AT = 1_700_000_000_000; + + uint256 internal ownerPk = uint256(keccak256("safe.owner")); + address internal owner; + + CredibleRegistryMock internal registry; + Safe internal safe; + GenerateSafeGuardBatch internal generator; + DeployCredibleSafeGuard internal deployer; + + function setUp() public { + owner = vm.addr(ownerPk); + registry = new CredibleRegistryMock(); + generator = new GenerateSafeGuardBatch(); + deployer = new DeployCredibleSafeGuard(); + + Safe singleton = new Safe(); + SafeProxyFactory factory = new SafeProxyFactory(); + address[] memory owners = new address[](1); + owners[0] = owner; + bytes memory initializer = + abi.encodeCall(Safe.setup, (owners, 1, address(0), "", address(0), address(0), 0, payable(address(0)))); + safe = Safe(payable(address(factory.createProxyWithNonce(address(singleton), initializer, 0)))); + + vm.roll(1_000_000); + } + + function test_deployHelper_deploysConfiguredGuard() public { + CredibleSafeGuard guard = deployer.deploy(address(registry), THRESHOLD); + + assertEq(address(guard.credibleRegistry()), address(registry)); + assertEq(guard.failOpenBlockThreshold(), THRESHOLD); + } + + function test_deployHelper_preservesConstructorValidation() public { + vm.expectRevert(CredibleSafeGuard.ZeroCredibleRegistryAddress.selector); + deployer.deploy(address(0), THRESHOLD); + + vm.expectRevert(CredibleSafeGuard.ZeroFailOpenBlockThreshold.selector); + deployer.deploy(address(registry), 0); + } + + function test_installBatch_matchesSafeTransactionBuilderSchema() public { + CredibleSafeGuard guard = deployer.deploy(address(registry), THRESHOLD); + string memory json = generator.buildInstallBatch(address(safe), address(guard), block.chainid, CREATED_AT); + + assertEq(vm.parseJsonString(json, ".version"), "1.0"); + assertEq(vm.parseJsonString(json, ".chainId"), vm.toString(block.chainid)); + assertEq(vm.parseJsonUint(json, ".createdAt"), CREATED_AT); + assertEq(vm.parseJsonString(json, ".meta.name"), "Install Credible Safe Guard"); + assertEq(vm.parseJsonAddress(json, ".meta.createdFromSafeAddress"), address(safe)); + assertEq(vm.parseJsonAddress(json, ".transactions[0].to"), address(safe)); + assertEq(vm.parseJsonString(json, ".transactions[0].value"), "0"); + assertEq(vm.parseJsonString(json, ".transactions[0].contractMethod.name"), "setGuard"); + assertFalse(vm.parseJsonBool(json, ".transactions[0].contractMethod.payable")); + assertEq(vm.parseJsonString(json, ".transactions[0].contractMethod.inputs[0].name"), "guard"); + assertEq(vm.parseJsonString(json, ".transactions[0].contractMethod.inputs[0].type"), "address"); + assertEq(vm.parseJsonAddress(json, ".transactions[0].contractInputsValues.guard"), address(guard)); + assertTrue(vm.keyExistsJson(json, ".transactions[0].data")); + + bytes32 expected = generator.calculateChecksum(address(safe), address(guard), block.chainid, CREATED_AT); + assertEq(vm.parseJsonBytes32(json, ".meta.checksum"), expected); + } + + function test_removeBatch_targetsZeroGuard() public { + string memory json = generator.buildRemoveBatch(address(safe), block.chainid, CREATED_AT); + + assertEq(vm.parseJsonString(json, ".meta.name"), "Remove Credible Safe Guard"); + assertEq(vm.parseJsonAddress(json, ".transactions[0].to"), address(safe)); + assertEq(vm.parseJsonString(json, ".transactions[0].contractMethod.name"), "setGuard"); + assertEq(vm.parseJsonAddress(json, ".transactions[0].contractInputsValues.guard"), address(0)); + assertEq( + vm.parseJsonBytes32(json, ".meta.checksum"), + generator.calculateChecksum(address(safe), address(0), block.chainid, CREATED_AT) + ); + } + + function test_checksum_matchesSafeReferenceAlgorithm() public view { + bytes32 checksum = generator.calculateChecksum(address(0x1111), address(0x2222), 1, 1_700_000_000_000); + assertEq(checksum, REFERENCE_CHECKSUM); + } + + function test_generatedInstallBatch_executesThroughRealSafe() public { + CredibleSafeGuard guard = deployer.deploy(address(registry), THRESHOLD); + string memory json = generator.buildInstallBatch(address(safe), address(guard), block.chainid, CREATED_AT); + + _executeGeneratedBatch(json); + assertEq(_installedGuard(), address(guard)); + } + + function test_generatedBatch_replacesAndRemovesGuard() public { + CredibleSafeGuard firstGuard = deployer.deploy(address(registry), THRESHOLD); + CredibleSafeGuard secondGuard = deployer.deploy(address(registry), THRESHOLD + 1); + + _executeGeneratedBatch( + generator.buildInstallBatch(address(safe), address(firstGuard), block.chainid, CREATED_AT) + ); + assertEq(_installedGuard(), address(firstGuard)); + + registry.markCurrentBlockCredible(); + _executeGeneratedBatch( + generator.buildInstallBatch(address(safe), address(secondGuard), block.chainid, CREATED_AT + 1) + ); + assertEq(_installedGuard(), address(secondGuard)); + + registry.markCurrentBlockCredible(); + _executeGeneratedBatch(generator.buildRemoveBatch(address(safe), block.chainid, CREATED_AT + 2)); + assertEq(_installedGuard(), address(0)); + } + + function test_rejectsInvalidSafeAndGuardInputs() public { + CredibleSafeGuard guard = deployer.deploy(address(registry), THRESHOLD); + + vm.expectRevert(SafeGuardBatch.ZeroSafeAddress.selector); + generator.buildInstallBatch(address(0), address(guard), block.chainid, CREATED_AT); + + vm.expectRevert(abi.encodeWithSelector(SafeGuardBatch.SafeHasNoCode.selector, address(0xBEEF))); + generator.buildInstallBatch(address(0xBEEF), address(guard), block.chainid, CREATED_AT); + + vm.expectRevert(SafeGuardBatch.ZeroGuardAddress.selector); + generator.buildInstallBatch(address(safe), address(0), block.chainid, CREATED_AT); + + vm.expectRevert(abi.encodeWithSelector(SafeGuardBatch.GuardHasNoCode.selector, address(0xBEEF))); + generator.buildInstallBatch(address(safe), address(0xBEEF), block.chainid, CREATED_AT); + + UnsupportedGuard unsupported = new UnsupportedGuard(); + vm.expectRevert(abi.encodeWithSelector(SafeGuardBatch.UnsupportedGuardInterface.selector, address(unsupported))); + generator.buildInstallBatch(address(safe), address(unsupported), block.chainid, CREATED_AT); + } + + function test_rejectsUnknownAction() public { + vm.expectRevert(abi.encodeWithSelector(SafeGuardBatch.InvalidAction.selector, "replace")); + generator.parseAction("replace"); + } + + function test_actionPathsAreDistinct() public view { + assertEq(generator.outputPath(SafeGuardBatch.Action.Install), "safe-guard-output/install.json"); + assertEq(generator.outputPath(SafeGuardBatch.Action.Remove), "safe-guard-output/remove.json"); + } + + function test_run_writesImportableInstallBatch() public { + CredibleSafeGuard guard = deployer.deploy(address(registry), THRESHOLD); + vm.setEnv("SAFE_ADDRESS", vm.toString(address(safe))); + vm.setEnv("SAFE_GUARD_ACTION", "install"); + vm.setEnv("CREDIBLE_SAFE_GUARD", vm.toString(address(guard))); + + (string memory json, string memory path) = generator.run(); + + assertEq(path, "safe-guard-output/install.json"); + assertEq(vm.readFile(path), json); + assertEq(vm.parseJsonAddress(json, ".transactions[0].to"), address(safe)); + assertEq(vm.parseJsonAddress(json, ".transactions[0].contractInputsValues.guard"), address(guard)); + vm.removeFile(path); + } + + function _executeGeneratedBatch(string memory json) internal { + address to = vm.parseJsonAddress(json, ".transactions[0].to"); + address guard = vm.parseJsonAddress(json, ".transactions[0].contractInputsValues.guard"); + bytes memory data = abi.encodeWithSignature("setGuard(address)", guard); + bytes memory signatures = _signTx(to, data); + + assertTrue( + safe.execTransaction(to, 0, data, Enum.Operation.Call, 0, 0, 0, address(0), payable(address(0)), signatures) + ); + } + + function _signTx(address to, bytes memory data) internal view returns (bytes memory) { + bytes32 txHash = + safe.getTransactionHash(to, 0, data, Enum.Operation.Call, 0, 0, 0, address(0), address(0), safe.nonce()); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerPk, txHash); + return abi.encodePacked(r, s, v); + } + + function _installedGuard() internal view returns (address) { + return address(uint160(uint256(vm.load(address(safe), GUARD_STORAGE_SLOT)))); + } +} diff --git a/foundry.toml b/foundry.toml index 9df9847..0aa2710 100644 --- a/foundry.toml +++ b/foundry.toml @@ -185,10 +185,12 @@ remappings = ["credible-std/=src/"] [profile.safe-guard] src = "examples/safe-guard/src" test = "examples/safe-guard/test" +script = "examples/safe-guard/script" out = "examples/safe-guard/out" cache_path = "examples/safe-guard/cache" libs = ["lib"] remappings = ["credible-std/=src/"] +fs_permissions = [{ access = "read-write", path = "./safe-guard-output" }] optimizer = true via_ir = false diff --git a/src/protection/safe/CredibleSafeGuard.sol b/src/protection/safe/CredibleSafeGuard.sol index 8ff15db..66ae6e5 100644 --- a/src/protection/safe/CredibleSafeGuard.sol +++ b/src/protection/safe/CredibleSafeGuard.sol @@ -47,8 +47,8 @@ interface ITransactionGuard is IERC165 { /// @author Phylax Systems /// @notice Safe transaction guard that only allows owner/multisig Safe transactions while the /// current block is credible, i.e. built by a Credible Layer builder that enforces -/// assertions. When the credible builder set is offline the guard fails open so the Safe -/// is never bricked. +/// assertions. When the credible builder set or registry is unavailable the guard fails +/// open so the Safe is never bricked by the credibility gate. /// @dev Installed on a Safe via `setGuard(address(thisGuard))`. The Safe calls /// {checkTransaction} before every owner-path execution; a revert blocks that execution. /// @@ -60,12 +60,14 @@ interface ITransactionGuard is IERC165 { /// Credible Layer assertion such as {SafeTxShapeAssertion}. /// /// Decision in {checkTransaction}: -/// 1. If the current block is credible, the transaction is always allowed. -/// 2. Otherwise, if the credible builder set looks offline (the most recent credible block +/// 1. If a required registry read reverts or returns malformed data, FAIL OPEN and allow the +/// transaction. This prevents an unavailable registry from permanently locking the Safe. +/// 2. If the current block is credible, the transaction is always allowed. +/// 3. Otherwise, if the credible builder set looks offline (the most recent credible block /// is more than `failOpenBlockThreshold` blocks behind the current block), FAIL OPEN and /// allow the transaction. This prevents a stalled builder set from permanently locking /// the Safe. -/// 3. Otherwise the builder set is live and the current block is not credible, so the +/// 4. Otherwise the builder set is live and the current block is not credible, so the /// transaction is blocked with {NonCredibleBlock}. /// /// Fail-open window. The product requirement is "fail open after ~15 minutes with no @@ -78,6 +80,10 @@ interface ITransactionGuard is IERC165 { /// Both the registry address and the fail-open threshold are immutable; re-pointing or /// re-tuning means deploying a new guard and calling `setGuard` again. contract CredibleSafeGuard is ITransactionGuard { + /// @dev Bounds the gas and returndata exposure of each registry probe. A registry that cannot + /// answer within this budget is treated as unavailable and the guard fails open. + uint256 internal constant REGISTRY_READ_GAS_LIMIT = 50_000; + /// @notice The on-chain Credible Registry queried for block credibility. ICredibleRegistry public immutable credibleRegistry; @@ -132,31 +138,69 @@ contract CredibleSafeGuard is ITransactionGuard { /// @notice Whether a Safe transaction would currently be allowed by this guard. /// @dev View helper mirroring {checkTransaction}'s decision for off-chain inspection. - /// @return True if the current block is credible or the guard is failing open. + /// @return True if the block is credible, the builder set is offline, or a required registry + /// read is unavailable or malformed. function isCurrentBlockAllowed() external view returns (bool) { - return credibleRegistry.isCredibleBlock(block.number) || _failOpenActive(); + (bool readable, bool credible) = _tryIsCredibleBlock(block.number); + return !readable || credible || _failOpenActive(); } - /// @notice Whether the guard is currently failing open because the builder set looks offline. - /// @return True if the most recent credible block lags the current block beyond the threshold. + /// @notice Whether a registry failure or builder outage currently activates fail-open. + /// @return True if a required registry read is unavailable or malformed, or the latest credible + /// block is too old. function failOpenActive() external view returns (bool) { - return _failOpenActive(); + (bool readable,) = _tryIsCredibleBlock(block.number); + return !readable || _failOpenActive(); } - /// @dev Core gate: allow credible blocks, otherwise fail open only when the builder set is - /// offline. The credible-block check runs first so the expected hot path (credible block, - /// live builder set) costs a single registry call. + /// @dev Core gate: allow credible blocks, otherwise fail open when a required registry read + /// fails or the builder set is offline. The credibility check runs first so the expected + /// hot path (credible block, live builder set) costs a single registry call. function _checkCredibleBlock() internal view { - if (credibleRegistry.isCredibleBlock(block.number)) return; - if (!_failOpenActive()) revert NonCredibleBlock(); + (bool readable, bool credible) = _tryIsCredibleBlock(block.number); + if (!readable || credible || _failOpenActive()) return; + revert NonCredibleBlock(); } - /// @dev Fail-open is active when the current block is strictly more than - /// `failOpenBlockThreshold` blocks ahead of the last credible block. The - /// `block.number > lastCredibleBlock_` guard avoids underflow if the registry ever - /// reports a last credible block at or beyond the current block. + /// @dev Fail-open is active when the last-block read fails or the current block is strictly + /// more than `failOpenBlockThreshold` blocks ahead of the last credible block. The + /// `block.number > lastCredibleBlock_` guard avoids underflow if the registry reports a + /// last credible block at or beyond the current block. function _failOpenActive() internal view returns (bool) { - uint256 lastCredibleBlock_ = credibleRegistry.lastCredibleBlock(); + (bool readable, uint256 lastCredibleBlock_) = _tryLastCredibleBlock(); + if (!readable) return true; return block.number > lastCredibleBlock_ && block.number - lastCredibleBlock_ > failOpenBlockThreshold; } + + /// @dev Probes `isCredibleBlock` without allowing a revert, malformed boolean, or returndata + /// bomb from the registry to bubble into the Safe transaction. + function _tryIsCredibleBlock(uint256 blockNumber) internal view returns (bool readable, bool credible) { + address registry = address(credibleRegistry); + uint256 selector = uint32(ICredibleRegistry.isCredibleBlock.selector); + uint256 value; + + assembly ("memory-safe") { + mstore(0x00, shl(224, selector)) + mstore(0x04, blockNumber) + readable := staticcall(REGISTRY_READ_GAS_LIMIT, registry, 0x00, 0x24, 0x00, 0x20) + readable := and(readable, eq(returndatasize(), 0x20)) + value := mload(0x00) + } + + if (!readable || value > 1) return (false, false); + return (true, value == 1); + } + + /// @dev Probes `lastCredibleBlock` while copying at most one word of returndata. + function _tryLastCredibleBlock() internal view returns (bool readable, uint256 lastCredibleBlock_) { + address registry = address(credibleRegistry); + uint256 selector = uint32(ICredibleRegistry.lastCredibleBlock.selector); + + assembly ("memory-safe") { + mstore(0x00, shl(224, selector)) + readable := staticcall(REGISTRY_READ_GAS_LIMIT, registry, 0x00, 0x04, 0x00, 0x20) + readable := and(readable, eq(returndatasize(), 0x20)) + lastCredibleBlock_ := mload(0x00) + } + } } diff --git a/src/protection/safe/README.md b/src/protection/safe/README.md index a052c79..d66946a 100644 --- a/src/protection/safe/README.md +++ b/src/protection/safe/README.md @@ -12,15 +12,18 @@ The two `*Assertion` contracts run inside the PhEvm. `CredibleSafeGuard` is a pl `CredibleSafeGuard` is a Gnosis Safe transaction guard that gates every owner-path Safe execution on block credibility, as reported by the on-chain [Credible Registry](https://github.com/phylaxsystems/credible-registry). -Install it on a Safe with `setGuard(address(guard))`. Safe then calls `checkTransaction` before each `execTransaction`, and the guard reverts to block the execution. +Install it through an owner-authorized Safe self-transaction whose calldata is `setGuard(address(guard))`; a direct owner call is not authorized. Safe then calls `checkTransaction` before each `execTransaction`, and the guard reverts to block the execution. The scripts and Safe Transaction Builder import workflow are documented in [`examples/safe-guard/README.md`](../../../examples/safe-guard/README.md). ### What It Checks On every Safe transaction the guard reads the Credible Registry and decides: -1. If the current block is credible, the transaction is allowed. -2. Otherwise, if the most recent credible block lags the current block by more than `failOpenBlockThreshold` blocks, the credible builder set is treated as offline, so the guard fails open and allows the transaction. This keeps a stalled builder set from locking the Safe. -3. Otherwise the builder set is live and the current block is not credible, so the transaction reverts with `NonCredibleBlock`. +1. If a required registry read reverts, exceeds its gas budget, or returns malformed data, the registry is treated as unavailable, so the guard fails open and allows the transaction. This prevents a broken registry from locking the Safe. +2. If the current block is credible, the transaction is allowed. +3. Otherwise, if the most recent credible block lags the current block by more than `failOpenBlockThreshold` blocks, the credible builder set is treated as offline, so the guard fails open and allows the transaction. This keeps a stalled builder set from locking the Safe. +4. Otherwise the builder set is live and the current block is not credible, so the transaction reverts with `NonCredibleBlock`. + +Registry probes are limited to 50,000 gas and copy exactly one 32-byte return word. A non-canonical boolean, short or oversized response, codeless registry address, or failed call activates fail-open instead of bubbling a failure through the Safe guard hook. This also preserves the owner path for replacing or removing the guard if the registry breaks. ### Config Options @@ -42,7 +45,7 @@ The target is to fail open after roughly 15 minutes with no credible blocks. The ### Material Effect - While the credible builder set is live, a Safe transaction cannot land in a non-credible block, such as one built by a builder that does not enforce assertions. -- If the credible builder set goes offline for longer than the configured window, the Safe keeps working. +- If the credible builder set goes offline for longer than the configured window, or the registry becomes unreadable, the Safe keeps working. - The guard gates only on block credibility and does not inspect the transaction target, value, calldata, or signatures. Pair it with `SafeConfigLockAssertion`, `SafeTxShapeAssertion`, or other assertions to constrain what the Safe may do within a credible block. ### Scope diff --git a/test/protection/safe/CredibleSafeGuard.t.sol b/test/protection/safe/CredibleSafeGuard.t.sol index 6722f01..d1dc7dc 100644 --- a/test/protection/safe/CredibleSafeGuard.t.sol +++ b/test/protection/safe/CredibleSafeGuard.t.sol @@ -71,7 +71,11 @@ contract CredibleSafeGuardTest is Test { /// @dev Calls the guard with a representative full Safe transaction tuple. function _check() internal view { - guard.checkTransaction( + _checkGuard(guard); + } + + function _checkGuard(CredibleSafeGuard guard_) internal view { + guard_.checkTransaction( address(0xBEEF), 1 ether, hex"abcdef", @@ -206,6 +210,67 @@ contract CredibleSafeGuardTest is Test { _check(); } + // --------------------------------------------------------------------- + // Fail-open path: registry unavailable or malformed + // --------------------------------------------------------------------- + + function test_failsOpen_whenCredibilityReadReverts() public { + vm.mockCallRevert( + address(registry), abi.encodeCall(ICredibleRegistry.isCredibleBlock, (block.number)), "registry unavailable" + ); + + assertTrue(guard.isCurrentBlockAllowed()); + assertTrue(guard.failOpenActive()); + _check(); + } + + function test_failsOpen_whenCredibilityReadReturnsShortData() public { + vm.mockCall( + address(registry), + abi.encodeCall(ICredibleRegistry.isCredibleBlock, (block.number)), + abi.encodePacked(uint8(1)) + ); + + assertTrue(guard.isCurrentBlockAllowed()); + _check(); + } + + function test_failsOpen_whenCredibilityReadReturnsNonCanonicalBool() public { + vm.mockCall( + address(registry), abi.encodeCall(ICredibleRegistry.isCredibleBlock, (block.number)), abi.encode(uint256(2)) + ); + + assertTrue(guard.isCurrentBlockAllowed()); + _check(); + } + + function test_failsOpen_whenLastCredibleBlockReadReverts() public { + registry.setCredibleBlock(block.number, false); + vm.mockCallRevert( + address(registry), abi.encodeCall(ICredibleRegistry.lastCredibleBlock, ()), "registry unavailable" + ); + + assertTrue(guard.failOpenActive()); + assertTrue(guard.isCurrentBlockAllowed()); + _check(); + } + + function test_failsOpen_whenRegistryHasNoCode() public { + CredibleSafeGuard codelessRegistryGuard = new CredibleSafeGuard(ICredibleRegistry(address(0xBEEF)), THRESHOLD); + + assertTrue(codelessRegistryGuard.isCurrentBlockAllowed()); + _checkGuard(codelessRegistryGuard); + } + + function test_credibleHotPath_doesNotRequireLastBlockRead() public { + registry.markCurrentBlockCredible(); + vm.mockCallRevert( + address(registry), abi.encodeCall(ICredibleRegistry.lastCredibleBlock, ()), "registry unavailable" + ); + + _check(); + } + // --------------------------------------------------------------------- // Defensive: registry reports a last credible block at/after current block // --------------------------------------------------------------------- From e23f54dd520275156d5c24acd1eba8ad42ad43a1 Mon Sep 17 00:00:00 2001 From: makemake Date: Fri, 24 Jul 2026 17:14:06 +0200 Subject: [PATCH 2/5] fix(safe): validate registry before deploy + cover adversarial registry probes Address review feedback on the guard lifecycle scripts: - DeployCredibleSafeGuard now validates the registry before broadcasting: reverts RegistryHasNoCode for a codeless address (e.g. a typo resolving to an EOA, which would otherwise deploy a permanently-fail-open guard) and RegistryReadFailed unless both isCredibleBlock and lastCredibleBlock return a well-formed 32-byte word. Kept out of deploy() so constructor validation is preserved for in-memory mocks. - CredibleSafeGuard tests gain two adversarial registry doubles: a gas guzzler whose reads burn far more than the 50k probe budget, and a returndata bomb returning >32 bytes with a leading "credible" word. New tests prove the bounded staticcall fails open gracefully on both reads without propagating OOG or copying unbounded returndata. --- .../script/DeployCredibleSafeGuard.s.sol | 29 ++++++ .../test/CredibleSafeGuardScripts.t.sol | 23 +++++ test/protection/safe/CredibleSafeGuard.t.sol | 90 +++++++++++++++++++ 3 files changed, 142 insertions(+) diff --git a/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol b/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol index 006576f..585cd29 100644 --- a/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol +++ b/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol @@ -8,10 +8,23 @@ import {ICredibleRegistry} from "credible-std/protection/safe/ICredibleRegistry. /// @notice Deploys a Credible Safe guard using Foundry's configured broadcast wallet. contract DeployCredibleSafeGuard is Script { + /// @notice Thrown when the configured registry address has no deployed code. + /// @dev Catches the common footgun of a typo that resolves to an EOA: a codeless registry + /// makes every credibility probe fail open, so the guard would allow every transaction. + error RegistryHasNoCode(address registry); + /// @notice Thrown when a required registry read (isCredibleBlock / lastCredibleBlock) does not + /// return a single, well-formed 32-byte word. + error RegistryReadFailed(address registry, string read); + function run() external returns (CredibleSafeGuard guard) { address registry = vm.envAddress("CREDIBLE_REGISTRY"); uint256 threshold = vm.envUint("FAIL_OPEN_BLOCK_THRESHOLD"); + // Validate the registry before broadcasting so a misconfigured (codeless or + // non-responsive) registry is caught up front rather than silently deploying a + // permanently-fail-open guard. + validateRegistry(registry); + vm.startBroadcast(); guard = deploy(registry, threshold); vm.stopBroadcast(); @@ -25,4 +38,20 @@ contract DeployCredibleSafeGuard is Script { function deploy(address registry, uint256 threshold) public returns (CredibleSafeGuard) { return new CredibleSafeGuard(ICredibleRegistry(registry), threshold); } + + /// @notice Asserts the registry has code and answers both reads the guard depends on. + /// @dev Reverts with a descriptive error otherwise. Kept separate from {deploy} so the + /// constructor's own zero-address / zero-threshold validation is preserved for callers + /// that deploy against an in-memory mock. + function validateRegistry(address registry) public view { + if (registry.code.length == 0) revert RegistryHasNoCode(registry); + + (bool credibleOk, bytes memory credibleData) = + registry.staticcall(abi.encodeCall(ICredibleRegistry.isCredibleBlock, (block.number))); + if (!credibleOk || credibleData.length != 32) revert RegistryReadFailed(registry, "isCredibleBlock"); + + (bool lastOk, bytes memory lastData) = + registry.staticcall(abi.encodeCall(ICredibleRegistry.lastCredibleBlock, ())); + if (!lastOk || lastData.length != 32) revert RegistryReadFailed(registry, "lastCredibleBlock"); + } } diff --git a/examples/safe-guard/test/CredibleSafeGuardScripts.t.sol b/examples/safe-guard/test/CredibleSafeGuardScripts.t.sol index c5920dd..06d24b9 100644 --- a/examples/safe-guard/test/CredibleSafeGuardScripts.t.sol +++ b/examples/safe-guard/test/CredibleSafeGuardScripts.t.sol @@ -62,6 +62,29 @@ contract CredibleSafeGuardScriptsTest is Test { deployer.deploy(address(registry), 0); } + function test_validateRegistry_acceptsResponsiveRegistry() public view { + // A live mock registry with code and both reads answering must validate cleanly. + deployer.validateRegistry(address(registry)); + } + + function test_validateRegistry_rejectsCodelessRegistry() public { + // A typo that resolves to an EOA has no code and would fail every credibility probe open. + vm.expectRevert(abi.encodeWithSelector(DeployCredibleSafeGuard.RegistryHasNoCode.selector, address(0xBEEF))); + deployer.validateRegistry(address(0xBEEF)); + } + + function test_validateRegistry_rejectsUnresponsiveRegistry() public { + // A contract that exists but does not implement the registry reads must be rejected before + // broadcasting, rather than deploying a permanently-fail-open guard. + UnsupportedGuard notARegistry = new UnsupportedGuard(); + vm.expectRevert( + abi.encodeWithSelector( + DeployCredibleSafeGuard.RegistryReadFailed.selector, address(notARegistry), "isCredibleBlock" + ) + ); + deployer.validateRegistry(address(notARegistry)); + } + function test_installBatch_matchesSafeTransactionBuilderSchema() public { CredibleSafeGuard guard = deployer.deploy(address(registry), THRESHOLD); string memory json = generator.buildInstallBatch(address(safe), address(guard), block.chainid, CREATED_AT); diff --git a/test/protection/safe/CredibleSafeGuard.t.sol b/test/protection/safe/CredibleSafeGuard.t.sol index d1dc7dc..bd131b6 100644 --- a/test/protection/safe/CredibleSafeGuard.t.sol +++ b/test/protection/safe/CredibleSafeGuard.t.sol @@ -35,6 +35,61 @@ contract MockCredibleRegistry is ICredibleRegistry { } } +/// @notice Adversarial registry whose probed reads burn far more than the guard's +/// `REGISTRY_READ_GAS_LIMIT` (50_000) gas budget via an unbounded hashing loop. +/// @dev Proves the bounded staticcall lets the sub-call run out of its capped gas and fail open, +/// instead of letting the OOG propagate into (and revert) the Safe transaction. `guzzleIsCredible` +/// toggles which read guzzles, so both bounded calls can be exercised independently. +contract GasGuzzlingRegistry is ICredibleRegistry { + bool public guzzleIsCredible = true; + + function setGuzzleIsCredible(bool value) external { + guzzleIsCredible = value; + } + + function isCredibleBlock(uint256) external view returns (bool) { + if (guzzleIsCredible) _burnGas(); + // When not guzzling, return a well-formed `false` so the gate advances to lastCredibleBlock. + return false; + } + + function lastCredibleBlock() external view returns (uint256) { + _burnGas(); + return 0; + } + + /// @dev Consumes millions of gas; capped at 50_000 by the guard it OOGs almost immediately. + function _burnGas() private view { + uint256 acc = uint256(blockhash(block.number - 1)); + for (uint256 i = 0; i < 100_000; ++i) { + acc = uint256(keccak256(abi.encode(acc, i))); + } + // Consume `acc` so the loop cannot be optimized away. + require(acc != 1, "unreachable"); + } +} + +/// @notice Adversarial registry that returns far more than 32 bytes of data from each read, with a +/// leading word that would decode to a "credible" answer. +/// @dev Proves the bounded staticcall (retSize 0x20 + `returndatasize() == 0x20` check) rejects the +/// over-long return, copies at most one word, and fails open rather than trusting the payload. +contract ReturnDataBombRegistry is ICredibleRegistry { + function isCredibleBlock(uint256) external pure returns (bool) { + assembly ("memory-safe") { + // Leading word == 1 (would read as `true`), followed by 99 more words of garbage. + mstore(0x00, 1) + return(0x00, 3200) + } + } + + function lastCredibleBlock() external pure returns (uint256) { + assembly ("memory-safe") { + mstore(0x00, 1) + return(0x00, 3200) + } + } +} + /// @notice Minimal Safe stand-in that calls the guard before "executing", mimicking the /// relevant slice of `Safe.execTransaction`. contract MockSafe { @@ -262,6 +317,41 @@ contract CredibleSafeGuardTest is Test { _checkGuard(codelessRegistryGuard); } + function test_failsOpen_whenCredibilityReadExceedsGasBudget() public { + // isCredibleBlock burns >> 50k gas; the capped staticcall OOGs and fails open without + // reverting the outer (Safe) transaction. + GasGuzzlingRegistry guzzler = new GasGuzzlingRegistry(); + CredibleSafeGuard guzzlerGuard = new CredibleSafeGuard(guzzler, THRESHOLD); + + assertTrue(guzzlerGuard.isCurrentBlockAllowed()); + assertTrue(guzzlerGuard.failOpenActive()); + _checkGuard(guzzlerGuard); + } + + function test_failsOpen_whenLastCredibleBlockReadExceedsGasBudget() public { + // isCredibleBlock answers a clean `false`, so the gate advances to lastCredibleBlock, which + // burns >> 50k gas. The capped staticcall on that read OOGs and fails open too. + GasGuzzlingRegistry guzzler = new GasGuzzlingRegistry(); + guzzler.setGuzzleIsCredible(false); + CredibleSafeGuard guzzlerGuard = new CredibleSafeGuard(guzzler, THRESHOLD); + + assertTrue(guzzlerGuard.isCurrentBlockAllowed()); + assertTrue(guzzlerGuard.failOpenActive()); + _checkGuard(guzzlerGuard); + } + + function test_failsOpen_whenRegistryReturnsMoreThan32Bytes() public { + // Both reads return 3200 bytes with a leading "credible/true" word; the bounded staticcall + // copies at most one word and rejects the over-long returndata, so the guard fails open + // rather than trusting the payload or copying it unbounded. + ReturnDataBombRegistry bomb = new ReturnDataBombRegistry(); + CredibleSafeGuard bombGuard = new CredibleSafeGuard(bomb, THRESHOLD); + + assertTrue(bombGuard.isCurrentBlockAllowed()); + assertTrue(bombGuard.failOpenActive()); + _checkGuard(bombGuard); + } + function test_credibleHotPath_doesNotRequireLastBlockRead() public { registry.markCurrentBlockCredible(); vm.mockCallRevert( From 05213a92c06bcbf0d0f37f387918e5f144f32189 Mon Sep 17 00:00:00 2001 From: makemake Date: Fri, 24 Jul 2026 17:20:32 +0200 Subject: [PATCH 3/5] docs(safe): document fail-open-on-registry-failure as an intentional decision Per repo-owner sign-off on the enforcement-boundary review comment: keep the existing fail-open behavior exactly as-is (no control-flow change) but make it an explicit, documented product decision rather than an implicit side effect. Expand the contract-level NatSpec and annotate the enforcement decision points (_checkCredibleBlock, _tryIsCredibleBlock, _tryLastCredibleBlock) to state: - the guard fails OPEN when a registry read reverts / has no code / returns malformed or non-32-byte data / exceeds the 50k gas budget; - the rationale: never permanently brick the Safe's owner path on an unavailable/broken/misconfigured registry, degrading to no protection; - the security tradeoff plainly: a broken/absent/malicious registry allows all owner transactions through; - that the deploy-time validateRegistry check (codeless/EOA rejection + read verification before broadcast) is the mitigation, since the registry address is immutable. --- src/protection/safe/CredibleSafeGuard.sol | 42 +++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/src/protection/safe/CredibleSafeGuard.sol b/src/protection/safe/CredibleSafeGuard.sol index 66ae6e5..3cc8147 100644 --- a/src/protection/safe/CredibleSafeGuard.sol +++ b/src/protection/safe/CredibleSafeGuard.sol @@ -70,6 +70,28 @@ interface ITransactionGuard is IERC165 { /// 4. Otherwise the builder set is live and the current block is not credible, so the /// transaction is blocked with {NonCredibleBlock}. /// +/// Fail-open on registry failure (intentional, signed-off product decision). +/// Every registry probe is a bounded staticcall (see {_tryIsCredibleBlock} / +/// {_tryLastCredibleBlock}). The guard treats the registry as UNAVAILABLE and FAILS OPEN, +/// allowing the owner transaction, whenever a probe: +/// - reverts, or +/// - hits an address with no deployed code, or +/// - returns malformed data (a non-canonical boolean, or fewer/more than exactly 32 bytes, +/// so an over-long "returndata bomb" cannot be trusted or copied unbounded), or +/// - exceeds the {REGISTRY_READ_GAS_LIMIT} (50k) gas budget and runs out of gas. +/// Rationale: this guard must NEVER permanently brick the Safe's owner-transaction path +/// because of an unavailable, broken, or misconfigured registry. Registry unavailability +/// therefore degrades to "no credibility protection" rather than a frozen Safe. This is a +/// deliberate, reviewed decision, not an accidental side effect of the bounded-call +/// hardening. +/// Security tradeoff (stated plainly): a broken, absent, or malicious registry that fails +/// any of the above ways lets ALL owner transactions through unchecked. The mitigation is at +/// DEPLOY TIME, not runtime: the deployment script's `validateRegistry` step rejects a +/// codeless / EOA registry address and verifies that both `isCredibleBlock` and +/// `lastCredibleBlock` return well-formed data before broadcasting, so a guard is never +/// deployed already pointed at a permanently-fail-open registry. The registry address is +/// immutable, so this deploy-time check is the enforcement point for a sound registry. +/// /// Fail-open window. The product requirement is "fail open after ~15 minutes with no /// credible blocks". The {ICredibleRegistry} records credibility by block number and does /// not expose timestamps, so the window is expressed as a block count that the credible @@ -156,6 +178,15 @@ contract CredibleSafeGuard is ITransactionGuard { /// @dev Core gate: allow credible blocks, otherwise fail open when a required registry read /// fails or the builder set is offline. The credibility check runs first so the expected /// hot path (credible block, live builder set) costs a single registry call. + /// + /// The `!readable` disjunct is the intentional, signed-off fail-open-on-registry-failure + /// decision (see the contract-level NatSpec): when the bounded probe cannot get a + /// well-formed answer within its gas/returndata budget the transaction is ALLOWED rather + /// than blocked, so an unavailable/broken/misconfigured registry can never permanently + /// brick the Safe's owner path. Security tradeoff: such a registry lets all owner + /// transactions through; this is mitigated at deploy time by the deployment script's + /// `validateRegistry` check, not here. Only reachable if the builder set is live and the + /// current block is genuinely not credible do we `revert NonCredibleBlock()`. function _checkCredibleBlock() internal view { (bool readable, bool credible) = _tryIsCredibleBlock(block.number); if (!readable || credible || _failOpenActive()) return; @@ -173,7 +204,11 @@ contract CredibleSafeGuard is ITransactionGuard { } /// @dev Probes `isCredibleBlock` without allowing a revert, malformed boolean, or returndata - /// bomb from the registry to bubble into the Safe transaction. + /// bomb from the registry to bubble into the Safe transaction. Returns `readable == false` + /// (the intentional fail-open signal, see {_checkCredibleBlock}) when the staticcall + /// reverts, the registry has no code, the call exceeds {REGISTRY_READ_GAS_LIMIT} and runs + /// out of gas, the returndata is not exactly 32 bytes (rejecting an over-long returndata + /// bomb while copying at most one word), or the word is a non-canonical boolean (> 1). function _tryIsCredibleBlock(uint256 blockNumber) internal view returns (bool readable, bool credible) { address registry = address(credibleRegistry); uint256 selector = uint32(ICredibleRegistry.isCredibleBlock.selector); @@ -191,7 +226,10 @@ contract CredibleSafeGuard is ITransactionGuard { return (true, value == 1); } - /// @dev Probes `lastCredibleBlock` while copying at most one word of returndata. + /// @dev Probes `lastCredibleBlock` while copying at most one word of returndata. Returns + /// `readable == false` on the same failure set as {_tryIsCredibleBlock} (revert, no code, + /// >50k gas / OOG, or returndata not exactly 32 bytes); {_failOpenActive} maps that to + /// fail-open, consistent with the intentional signed-off registry-failure policy. function _tryLastCredibleBlock() internal view returns (bool readable, uint256 lastCredibleBlock_) { address registry = address(credibleRegistry); uint256 selector = uint32(ICredibleRegistry.lastCredibleBlock.selector); From bb3d6c5dedc721e9e667ce6a5dc51131383ac4dd Mon Sep 17 00:00:00 2001 From: makemake Date: Fri, 24 Jul 2026 18:13:59 +0200 Subject: [PATCH 4/5] fix(safe-guard): reject non-canonical bool from registry in validateRegistry The deploy script's validateRegistry only checked that isCredibleBlock returned exactly 32 bytes, so a registry answering abi.encode(uint256(2)) passed deploy-time validation while the guard's runtime decode (_tryIsCredibleBlock, value > 1) rejects it as unreadable -- silently deploying a permanently-fail-open guard. Decode the word as a canonical bool (reject > 1) up front, mirroring the runtime check, and add a regression test with a mock registry returning uint256(2). --- .../script/DeployCredibleSafeGuard.s.sol | 9 +++++- .../test/CredibleSafeGuardScripts.t.sol | 32 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol b/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol index 585cd29..be51bf0 100644 --- a/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol +++ b/examples/safe-guard/script/DeployCredibleSafeGuard.s.sol @@ -46,9 +46,16 @@ contract DeployCredibleSafeGuard is Script { function validateRegistry(address registry) public view { if (registry.code.length == 0) revert RegistryHasNoCode(registry); + // `isCredibleBlock` is a Solidity `bool`, so a well-formed answer is a canonical boolean + // word (0 or 1). Reject any other 32-byte value (e.g. `abi.encode(uint256(2))`) here: + // it passes the length check but the guard's runtime decode treats it as unreadable + // (see CredibleSafeGuard._tryIsCredibleBlock's `value > 1` branch), which would otherwise + // let a registry silently deploy a permanently-fail-open guard. (bool credibleOk, bytes memory credibleData) = registry.staticcall(abi.encodeCall(ICredibleRegistry.isCredibleBlock, (block.number))); - if (!credibleOk || credibleData.length != 32) revert RegistryReadFailed(registry, "isCredibleBlock"); + if (!credibleOk || credibleData.length != 32 || abi.decode(credibleData, (uint256)) > 1) { + revert RegistryReadFailed(registry, "isCredibleBlock"); + } (bool lastOk, bytes memory lastData) = registry.staticcall(abi.encodeCall(ICredibleRegistry.lastCredibleBlock, ())); diff --git a/examples/safe-guard/test/CredibleSafeGuardScripts.t.sol b/examples/safe-guard/test/CredibleSafeGuardScripts.t.sol index 06d24b9..e60ddff 100644 --- a/examples/safe-guard/test/CredibleSafeGuardScripts.t.sol +++ b/examples/safe-guard/test/CredibleSafeGuardScripts.t.sol @@ -15,6 +15,24 @@ import {SafeGuardBatch} from "../script/SafeGuardBatch.sol"; contract UnsupportedGuard {} +/// @dev Registry whose `isCredibleBlock` returns a full 32-byte word that is not a canonical +/// boolean (`abi.encode(uint256(2))`). It passes a naive length check but the guard's runtime +/// decode treats a word `> 1` as unreadable, so a guard deployed against it would silently +/// fail open. Used to regression-test that {DeployCredibleSafeGuard.validateRegistry} rejects +/// it before broadcasting. +contract NonCanonicalBoolRegistry { + function isCredibleBlock(uint256) external pure returns (bool) { + assembly { + mstore(0x00, 2) + return(0x00, 0x20) + } + } + + function lastCredibleBlock() external pure returns (uint256) { + return 0; + } +} + contract CredibleSafeGuardScriptsTest is Test { bytes32 internal constant GUARD_STORAGE_SLOT = 0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8; bytes32 internal constant REFERENCE_CHECKSUM = 0x8994ee462d748c24ecd7804083007dd231e36ff84da4b272921c30d1ae7f0df0; @@ -85,6 +103,20 @@ contract CredibleSafeGuardScriptsTest is Test { deployer.validateRegistry(address(notARegistry)); } + function test_validateRegistry_rejectsNonCanonicalBoolRegistry() public { + // A registry whose isCredibleBlock returns abi.encode(uint256(2)) answers with a full + // 32-byte word, so the length check alone passes. The guard's runtime decode rejects any + // word > 1 as unreadable, which would fail the guard open. validateRegistry must catch it + // up front rather than broadcasting a permanently-fail-open guard. + NonCanonicalBoolRegistry badRegistry = new NonCanonicalBoolRegistry(); + vm.expectRevert( + abi.encodeWithSelector( + DeployCredibleSafeGuard.RegistryReadFailed.selector, address(badRegistry), "isCredibleBlock" + ) + ); + deployer.validateRegistry(address(badRegistry)); + } + function test_installBatch_matchesSafeTransactionBuilderSchema() public { CredibleSafeGuard guard = deployer.deploy(address(registry), THRESHOLD); string memory json = generator.buildInstallBatch(address(safe), address(guard), block.chainid, CREATED_AT); From 22434a1ca4150f377e354827a01b44c594e6945f Mon Sep 17 00:00:00 2001 From: makemake Date: Fri, 24 Jul 2026 18:53:53 +0200 Subject: [PATCH 5/5] fix(safe): fail open when registry reports a future lastCredibleBlock A broken registry returning a lastCredibleBlock beyond the current block (e.g. type(uint256).max) previously kept fail-open disabled and reverted every owner transaction until the chain reached that height, effectively bricking the Safe. Such a value is impossible for a sound registry, so treat it as an unreadable response and fail open, consistent with the guard's registry-failure policy. Updates the unit test that previously preserved the bricking behavior, adds max-uint and equal-to-current cases, and adds a real-Safe E2E fail-open test for a future reported height. --- .../CredibleSafeGuardSafeIntegration.t.sol | 10 +++++++ src/protection/safe/CredibleSafeGuard.sol | 24 +++++++++++----- test/protection/safe/CredibleSafeGuard.t.sol | 28 +++++++++++++++++-- 3 files changed, 53 insertions(+), 9 deletions(-) diff --git a/examples/safe-guard/test/CredibleSafeGuardSafeIntegration.t.sol b/examples/safe-guard/test/CredibleSafeGuardSafeIntegration.t.sol index 1a41e12..9cea8ed 100644 --- a/examples/safe-guard/test/CredibleSafeGuardSafeIntegration.t.sol +++ b/examples/safe-guard/test/CredibleSafeGuardSafeIntegration.t.sol @@ -144,6 +144,16 @@ contract CredibleSafeGuardSafeIntegrationTest is Test { assertEq(safe.nonce(), nonceBefore + 1); } + function test_realSafe_failsOpenWhenLastBlockReportedInFuture() public { + // A broken registry reporting a last credible block beyond the chain head must not brick the + // Safe: it is treated as an unreadable response and fails open. + registry.setLastCredibleBlock(type(uint256).max); + + uint256 nonceBefore = safe.nonce(); + assertTrue(_execSafeTx(owner, 0, "")); + assertEq(safe.nonce(), nonceBefore + 1); + } + function test_realSafe_endToEnd_stallThenRecover() public { // Builder healthy: current block is credible -> allowed. registry.markCurrentBlockCredible(); diff --git a/src/protection/safe/CredibleSafeGuard.sol b/src/protection/safe/CredibleSafeGuard.sol index 3cc8147..f5c6c89 100644 --- a/src/protection/safe/CredibleSafeGuard.sol +++ b/src/protection/safe/CredibleSafeGuard.sol @@ -66,7 +66,9 @@ interface ITransactionGuard is IERC165 { /// 3. Otherwise, if the credible builder set looks offline (the most recent credible block /// is more than `failOpenBlockThreshold` blocks behind the current block), FAIL OPEN and /// allow the transaction. This prevents a stalled builder set from permanently locking -/// the Safe. +/// the Safe. A `lastCredibleBlock` reported beyond the current block is impossible for a +/// sound registry, so it is treated as a broken read and also fails open (see +/// {_failOpenActive}) rather than blocking every transaction until that height is reached. /// 4. Otherwise the builder set is live and the current block is not credible, so the /// transaction is blocked with {NonCredibleBlock}. /// @@ -193,14 +195,22 @@ contract CredibleSafeGuard is ITransactionGuard { revert NonCredibleBlock(); } - /// @dev Fail-open is active when the last-block read fails or the current block is strictly - /// more than `failOpenBlockThreshold` blocks ahead of the last credible block. The - /// `block.number > lastCredibleBlock_` guard avoids underflow if the registry reports a - /// last credible block at or beyond the current block. + /// @dev Fail-open is active when the last-block read fails, the registry reports a last credible + /// block beyond the current block, or the current block is strictly more than + /// `failOpenBlockThreshold` blocks ahead of the last credible block. + /// + /// A `lastCredibleBlock_ > block.number` reading is impossible for a sound registry (the + /// registry defines this value as the highest block marked credible so far, which cannot + /// exceed the chain head), so it is treated as an unreadable/broken response and fails open. + /// Otherwise a broken registry reporting a far-future height with the current block not + /// credible would keep fail-open disabled and revert every owner transaction until the chain + /// reached that height — effectively bricking the Safe, the exact outcome this guard must + /// never produce on registry failure. Rejecting the future height also removes the + /// subtraction underflow it would otherwise cause. function _failOpenActive() internal view returns (bool) { (bool readable, uint256 lastCredibleBlock_) = _tryLastCredibleBlock(); - if (!readable) return true; - return block.number > lastCredibleBlock_ && block.number - lastCredibleBlock_ > failOpenBlockThreshold; + if (!readable || lastCredibleBlock_ > block.number) return true; + return block.number - lastCredibleBlock_ > failOpenBlockThreshold; } /// @dev Probes `isCredibleBlock` without allowing a revert, malformed boolean, or returndata diff --git a/test/protection/safe/CredibleSafeGuard.t.sol b/test/protection/safe/CredibleSafeGuard.t.sol index bd131b6..92677f8 100644 --- a/test/protection/safe/CredibleSafeGuard.t.sol +++ b/test/protection/safe/CredibleSafeGuard.t.sol @@ -365,9 +365,33 @@ contract CredibleSafeGuardTest is Test { // Defensive: registry reports a last credible block at/after current block // --------------------------------------------------------------------- - function test_noUnderflow_whenLastCredibleBlockInFuture() public { + function test_failsOpen_whenLastCredibleBlockInFuture() public { + // A last credible block beyond the current block is impossible for a sound registry, so it + // is treated as a broken read and fails open. Blocking here would revert every owner + // transaction until the chain reached that height, effectively bricking the Safe. registry.setLastCredibleBlock(block.number + 5); - // Not fail open, current block not credible -> clean NonCredibleBlock revert, no panic. + + assertTrue(guard.failOpenActive()); + assertTrue(guard.isCurrentBlockAllowed()); + _check(); + } + + function test_failsOpen_whenLastCredibleBlockIsMaxUint() public { + // The pathological far-future value from the review: a broken registry returning + // type(uint256).max must not brick the Safe. + registry.setLastCredibleBlock(type(uint256).max); + + assertTrue(guard.failOpenActive()); + assertTrue(guard.isCurrentBlockAllowed()); + _check(); + } + + function test_reverts_whenLastCredibleBlockEqualsCurrent_andNotCredible() public { + // Equal (not future) is a valid gap of 0: builder set is live, so a non-credible current + // block is still blocked rather than failing open. + registry.setLastCredibleBlock(block.number); + + assertFalse(guard.failOpenActive()); vm.expectRevert(CredibleSafeGuard.NonCredibleBlock.selector); _check(); }