From 9f5db4333301635cdb320289745245c9b3f99adc Mon Sep 17 00:00:00 2001 From: Eric Shenghsiung Liu Date: Mon, 1 Jun 2026 18:38:40 -0400 Subject: [PATCH 1/3] test(PolicyRegistry): add revertOrder tests for all mutating functions BOP-221 --- ...createPolicyWithAccounts_revertOrder.t.sol | 37 +++++ .../createPolicy_revertOrder.t.sol | 16 ++ .../finalizeUpdateAdmin_revertOrder.t.sol | 73 +++++++++ .../renounceAdmin_revertOrder.t.sol | 30 ++++ .../stageUpdateAdmin_revertOrder.t.sol | 34 +++++ .../updateAllowlist_revertOrder.t.sol | 144 ++++++++++++++++++ .../updateBlocklist_revertOrder.t.sol | 143 +++++++++++++++++ 7 files changed, 477 insertions(+) create mode 100644 test/unit/PolicyRegistry/createPolicyWithAccounts_revertOrder.t.sol create mode 100644 test/unit/PolicyRegistry/createPolicy_revertOrder.t.sol create mode 100644 test/unit/PolicyRegistry/finalizeUpdateAdmin_revertOrder.t.sol create mode 100644 test/unit/PolicyRegistry/renounceAdmin_revertOrder.t.sol create mode 100644 test/unit/PolicyRegistry/stageUpdateAdmin_revertOrder.t.sol create mode 100644 test/unit/PolicyRegistry/updateAllowlist_revertOrder.t.sol create mode 100644 test/unit/PolicyRegistry/updateBlocklist_revertOrder.t.sol diff --git a/test/unit/PolicyRegistry/createPolicyWithAccounts_revertOrder.t.sol b/test/unit/PolicyRegistry/createPolicyWithAccounts_revertOrder.t.sol new file mode 100644 index 0000000..4f16a52 --- /dev/null +++ b/test/unit/PolicyRegistry/createPolicyWithAccounts_revertOrder.t.sol @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {IPolicyRegistry} from "src/interfaces/IPolicyRegistry.sol"; + +import {PolicyRegistryTest} from "test/lib/PolicyRegistryTest.sol"; + +/// @title Differential check-order tests for `createPolicyWithAccounts`. +/// +/// @notice **Canonical order (Solidity reference entry-point, pre-`_create`):** +/// 1. ZERO-ADMIN (`admin == address(0)`) → `ZeroAddress` +/// 2. BATCH-SIZE (`accounts.length > MAX_BATCH_SIZE`) → `BatchSizeTooLarge` +/// +/// C(2, 2) = 1 pair. +/// +/// The natspec on `createPolicyWithAccounts` explicitly annotates this ordering: +/// "Reverts with `ZeroAddress` … Takes precedence over `BatchSizeTooLarge`." +/// This test pins that annotation against the Solidity reference implementation. +contract PolicyRegistryCreatePolicyWithAccountsRevertOrderTest is PolicyRegistryTest { + /// @notice ZERO-ADMIN beats BATCH-SIZE. + /// @dev admin is address(0) AND accounts.length exceeds MAX_BATCH_SIZE. + /// The zero-admin guard is checked before the batch-size guard. + function test_createPolicyWithAccounts_revertOrder_zeroAddress_beats_batchSizeTooLarge( + address caller, + uint8 typeIdx, + uint8 overflow + ) public { + _assumeValidCaller(caller); + IPolicyRegistry.PolicyType pt = _creatablePolicyType(typeIdx); + uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); + address[] memory accounts = _makeAccounts(n); + + vm.prank(caller); + vm.expectRevert(IPolicyRegistry.ZeroAddress.selector); + policyRegistry.createPolicyWithAccounts(address(0), pt, accounts); + } +} diff --git a/test/unit/PolicyRegistry/createPolicy_revertOrder.t.sol b/test/unit/PolicyRegistry/createPolicy_revertOrder.t.sol new file mode 100644 index 0000000..8a0c6f4 --- /dev/null +++ b/test/unit/PolicyRegistry/createPolicy_revertOrder.t.sol @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {PolicyRegistryTest} from "test/lib/PolicyRegistryTest.sol"; + +/// @title Differential check-order tests for `createPolicy`. +/// +/// @notice **Canonical order (Solidity reference `_create`):** +/// 1. ZERO-ADMIN (`admin == address(0)`) → `ZeroAddress` +/// +/// C(1, 2) = 0 pairs. `createPolicy` has only a single revert +/// condition so there is no pair-wise ordering to pin. This file +/// exists for coverage bookkeeping only; the individual revert is +/// tested in `createPolicy.t.sol`. +// solhint-disable-next-line no-empty-blocks +contract PolicyRegistryCreatePolicyRevertOrderTest is PolicyRegistryTest {} diff --git a/test/unit/PolicyRegistry/finalizeUpdateAdmin_revertOrder.t.sol b/test/unit/PolicyRegistry/finalizeUpdateAdmin_revertOrder.t.sol new file mode 100644 index 0000000..d48f631 --- /dev/null +++ b/test/unit/PolicyRegistry/finalizeUpdateAdmin_revertOrder.t.sol @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {IPolicyRegistry} from "src/interfaces/IPolicyRegistry.sol"; + +import {PolicyRegistryTest} from "test/lib/PolicyRegistryTest.sol"; + +/// @title Differential check-order tests for `finalizeUpdateAdmin`. +/// +/// @notice **Canonical order (Solidity reference):** +/// 1. POLICY-NOT-FOUND (`policies[policyId] == 0`) → `PolicyNotFound` +/// 2. NO-PENDING-ADMIN (`pendingAdmins[policyId] == address(0)`) → `NoPendingAdmin` +/// 3. UNAUTHORIZED (`pendingAdmins[policyId] != msg.sender`) → `Unauthorized` +/// +/// C(3, 2) = 3 pairs. +contract PolicyRegistryFinalizeUpdateAdminRevertOrderTest is PolicyRegistryTest { + // --------------------------------------------------------------- + // Pairs where POLICY-NOT-FOUND wins + // --------------------------------------------------------------- + + /// @notice POLICY-NOT-FOUND beats NO-PENDING-ADMIN. + /// @dev policyId does not exist (packed == 0) AND no pending admin is staged. + /// PolicyNotFound fires before the pending-admin slot is read. + function test_finalizeUpdateAdmin_revertOrder_policyNotFound_beats_noPendingAdmin(uint64 seed) public { + uint64 policyId = _wellFormedUncreatedPolicyId(seed); + + // Both conditions apply independently: + // - PolicyNotFound: policyId has never been created. + // - NoPendingAdmin: pendingAdmins[policyId] == address(0) (zero storage). + vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + policyRegistry.finalizeUpdateAdmin(policyId); + } + + /// @notice POLICY-NOT-FOUND beats UNAUTHORIZED. + /// @dev policyId does not exist AND caller is not address(0), so the pending-admin + /// comparison (address(0) != caller) would trigger Unauthorized if reached. + /// PolicyNotFound fires before the pending-admin slot or caller comparison runs. + function test_finalizeUpdateAdmin_revertOrder_policyNotFound_beats_unauthorized(address caller, uint64 seed) + public + { + _assumeValidCaller(caller); + uint64 policyId = _wellFormedUncreatedPolicyId(seed); + + // Both conditions apply independently: + // - PolicyNotFound: policyId has never been created (packed == 0). + // - Unauthorized: pendingAdmins[policyId] == address(0) != caller. + // (NoPendingAdmin also applies; it would fire second if PolicyNotFound didn't.) + vm.prank(caller); + vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + policyRegistry.finalizeUpdateAdmin(policyId); + } + + // --------------------------------------------------------------- + // Pairs where NO-PENDING-ADMIN wins + // --------------------------------------------------------------- + + /// @notice NO-PENDING-ADMIN beats UNAUTHORIZED. + /// @dev Policy exists but has no pending admin staged. Caller is a non-zero address, + /// so `pendingAdmins[policyId] (== address(0)) != caller` would trigger + /// Unauthorized if the NoPendingAdmin guard were absent. + function test_finalizeUpdateAdmin_revertOrder_noPendingAdmin_beats_unauthorized(address caller) public { + _assumeValidCaller(caller); + // Create a policy; no stageUpdateAdmin call follows, so pending == address(0). + uint64 policyId = policyRegistry.createPolicy(admin, IPolicyRegistry.PolicyType.ALLOWLIST); + + // Both conditions apply independently: + // - NoPendingAdmin: pendingAdmins[policyId] == address(0). + // - Unauthorized: address(0) != caller (caller is non-zero). + vm.prank(caller); + vm.expectRevert(IPolicyRegistry.NoPendingAdmin.selector); + policyRegistry.finalizeUpdateAdmin(policyId); + } +} diff --git a/test/unit/PolicyRegistry/renounceAdmin_revertOrder.t.sol b/test/unit/PolicyRegistry/renounceAdmin_revertOrder.t.sol new file mode 100644 index 0000000..d7dfac8 --- /dev/null +++ b/test/unit/PolicyRegistry/renounceAdmin_revertOrder.t.sol @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {IPolicyRegistry} from "src/interfaces/IPolicyRegistry.sol"; + +import {PolicyRegistryTest} from "test/lib/PolicyRegistryTest.sol"; + +/// @title Differential check-order tests for `renounceAdmin`. +/// +/// @notice **Canonical order (Solidity reference):** +/// 1. POLICY-NOT-FOUND (`policies[policyId] == 0`) → `PolicyNotFound` +/// 2. UNAUTHORIZED (`_decodeAdmin(packed) != msg.sender`) → `Unauthorized` +/// +/// C(2, 2) = 1 pair. +contract PolicyRegistryRenounceAdminRevertOrderTest is PolicyRegistryTest { + /// @notice POLICY-NOT-FOUND beats UNAUTHORIZED. + /// @dev policyId does not exist (packed == 0) AND caller is not the policy admin. + /// PolicyNotFound fires before the admin comparison runs. + function test_renounceAdmin_revertOrder_policyNotFound_beats_unauthorized(address caller, uint64 seed) public { + _assumeValidCaller(caller); + uint64 policyId = _wellFormedUncreatedPolicyId(seed); + + // Both conditions apply independently: + // - PolicyNotFound: policyId has never been created (policies[policyId] == 0). + // - Unauthorized: _decodeAdmin(0) == address(0) != caller (caller is non-zero). + vm.prank(caller); + vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + policyRegistry.renounceAdmin(policyId); + } +} diff --git a/test/unit/PolicyRegistry/stageUpdateAdmin_revertOrder.t.sol b/test/unit/PolicyRegistry/stageUpdateAdmin_revertOrder.t.sol new file mode 100644 index 0000000..0f61a0a --- /dev/null +++ b/test/unit/PolicyRegistry/stageUpdateAdmin_revertOrder.t.sol @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {IPolicyRegistry} from "src/interfaces/IPolicyRegistry.sol"; + +import {PolicyRegistryTest} from "test/lib/PolicyRegistryTest.sol"; + +/// @title Differential check-order tests for `stageUpdateAdmin`. +/// +/// @notice **Canonical order (Solidity reference):** +/// 1. POLICY-NOT-FOUND (`_requireCustom`: packed == 0) → `PolicyNotFound` +/// 2. UNAUTHORIZED (`_decodeAdmin(packed) != msg.sender`) → `Unauthorized` +/// +/// C(2, 2) = 1 pair. +contract PolicyRegistryStageUpdateAdminRevertOrderTest is PolicyRegistryTest { + /// @notice POLICY-NOT-FOUND beats UNAUTHORIZED. + /// @dev policyId does not exist (packed == 0) AND caller is not the policy admin. + /// `_requireCustom` reverts with `PolicyNotFound` before the admin check runs. + function test_stageUpdateAdmin_revertOrder_policyNotFound_beats_unauthorized( + address caller, + uint64 seed, + address newAdmin + ) public { + _assumeValidCaller(caller); + uint64 policyId = _wellFormedUncreatedPolicyId(seed); + + // Both conditions apply independently: + // - PolicyNotFound: policyId has never been created (policies[policyId] == 0). + // - Unauthorized: _decodeAdmin(0) == address(0) != caller (caller is non-zero). + vm.prank(caller); + vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + policyRegistry.stageUpdateAdmin(policyId, newAdmin); + } +} diff --git a/test/unit/PolicyRegistry/updateAllowlist_revertOrder.t.sol b/test/unit/PolicyRegistry/updateAllowlist_revertOrder.t.sol new file mode 100644 index 0000000..2be3143 --- /dev/null +++ b/test/unit/PolicyRegistry/updateAllowlist_revertOrder.t.sol @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {IPolicyRegistry} from "src/interfaces/IPolicyRegistry.sol"; + +import {PolicyRegistryTest} from "test/lib/PolicyRegistryTest.sol"; + +/// @title Differential check-order tests for `updateAllowlist`. +/// +/// @notice **Canonical order (Solidity reference):** +/// 1. POLICY-NOT-FOUND (`_requireCustom`: packed == 0) → `PolicyNotFound` +/// 2. INCOMPATIBLE-TYPE (`_typeOf(policyId) != ALLOWLIST`) → `IncompatiblePolicyType` +/// 3. UNAUTHORIZED (`_decodeAdmin(packed) != msg.sender`) → `Unauthorized` +/// 4. BATCH-SIZE (inside `_batchSetMembers`) → `BatchSizeTooLarge` +/// +/// C(4, 2) = 6 pairs. +contract PolicyRegistryUpdateAllowlistRevertOrderTest is PolicyRegistryTest { + // --------------------------------------------------------------- + // Pairs where POLICY-NOT-FOUND wins + // --------------------------------------------------------------- + + /// @notice POLICY-NOT-FOUND beats INCOMPATIBLE-TYPE. + /// @dev policyId encodes as BLOCKLIST (top byte 0, incompatible for updateAllowlist) + /// AND the policy has never been created (packed == 0). + /// PolicyNotFound fires before the type discriminator is examined. + function test_updateAllowlist_revertOrder_policyNotFound_beats_incompatiblePolicyType(uint56 counter) public { + counter = uint56(bound(uint256(counter), 2, type(uint56).max)); + // top byte 0 = BLOCKLIST; would trigger IncompatiblePolicyType if the policy existed. + uint64 policyId = uint64(counter); + address[] memory empty = new address[](0); + + // Both conditions apply independently: + // - PolicyNotFound: policyId has never been created. + // - IncompatiblePolicyType: policyId encodes as BLOCKLIST, not ALLOWLIST. + vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + policyRegistry.updateAllowlist(policyId, true, empty); + } + + /// @notice POLICY-NOT-FOUND beats UNAUTHORIZED. + /// @dev policyId encodes as ALLOWLIST (so IncompatiblePolicyType does not apply) + /// AND has never been created (PolicyNotFound fires), AND caller is non-zero + /// (Unauthorized would fire if policyId existed as ALLOWLIST). + function test_updateAllowlist_revertOrder_policyNotFound_beats_unauthorized(address caller, uint56 counter) + public + { + _assumeValidCaller(caller); + counter = uint56(bound(uint256(counter), 2, type(uint56).max)); + // top byte 1 = ALLOWLIST; IncompatiblePolicyType would not apply if the policy existed. + uint64 policyId = (uint64(1) << 56) | uint64(counter); + address[] memory empty = new address[](0); + + // Both conditions apply independently: + // - PolicyNotFound: policyId has never been created (packed == 0). + // - Unauthorized: _decodeAdmin(0) == address(0) != caller. + vm.prank(caller); + vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + policyRegistry.updateAllowlist(policyId, true, empty); + } + + /// @notice POLICY-NOT-FOUND beats BATCH-SIZE. + /// @dev policyId has never been created (PolicyNotFound fires) AND accounts.length + /// exceeds MAX_BATCH_SIZE (BatchSizeTooLarge would fire inside _batchSetMembers + /// if the policy existed and all earlier checks passed). + function test_updateAllowlist_revertOrder_policyNotFound_beats_batchSizeTooLarge(uint56 counter, uint8 overflow) + public + { + counter = uint56(bound(uint256(counter), 2, type(uint56).max)); + uint64 policyId = (uint64(1) << 56) | uint64(counter); // ALLOWLIST type, not created + uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); + address[] memory accounts = _makeAccounts(n); + + // Both conditions apply independently: + // - PolicyNotFound: policyId has never been created. + // - BatchSizeTooLarge: accounts.length > MAX_BATCH_SIZE. + vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + policyRegistry.updateAllowlist(policyId, true, accounts); + } + + // --------------------------------------------------------------- + // Pairs where INCOMPATIBLE-TYPE wins + // --------------------------------------------------------------- + + /// @notice INCOMPATIBLE-TYPE beats UNAUTHORIZED. + /// @dev Policy exists as BLOCKLIST (incompatible for updateAllowlist) AND caller + /// is not the policy admin (Unauthorized would fire if the type check were absent). + function test_updateAllowlist_revertOrder_incompatiblePolicyType_beats_unauthorized(address caller) public { + _assumeValidCaller(caller); + // Create a BLOCKLIST policy with `alice` as admin. + uint64 policyId = _createBlocklist(admin, alice); + vm.assume(caller != alice); + address[] memory empty = new address[](0); + + // Both conditions apply independently: + // - IncompatiblePolicyType: policy is BLOCKLIST, updateAllowlist requires ALLOWLIST. + // - Unauthorized: caller is not alice (the policy admin). + vm.prank(caller); + vm.expectRevert(IPolicyRegistry.IncompatiblePolicyType.selector); + policyRegistry.updateAllowlist(policyId, true, empty); + } + + /// @notice INCOMPATIBLE-TYPE beats BATCH-SIZE. + /// @dev Policy exists as BLOCKLIST AND accounts.length exceeds MAX_BATCH_SIZE. + /// Caller is the policy admin so Unauthorized does not apply; only type and + /// batch-size conditions compete. + function test_updateAllowlist_revertOrder_incompatiblePolicyType_beats_batchSizeTooLarge(uint8 overflow) public { + // Create a BLOCKLIST policy with alice as admin; call as alice (no Unauthorized). + uint64 policyId = _createBlocklist(admin, alice); + uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); + address[] memory accounts = _makeAccounts(n); + + // Both conditions apply independently: + // - IncompatiblePolicyType: policy is BLOCKLIST, updateAllowlist requires ALLOWLIST. + // - BatchSizeTooLarge: accounts.length > MAX_BATCH_SIZE. + vm.prank(alice); + vm.expectRevert(IPolicyRegistry.IncompatiblePolicyType.selector); + policyRegistry.updateAllowlist(policyId, true, accounts); + } + + // --------------------------------------------------------------- + // Pairs where UNAUTHORIZED wins + // --------------------------------------------------------------- + + /// @notice UNAUTHORIZED beats BATCH-SIZE. + /// @dev Policy exists as ALLOWLIST (IncompatiblePolicyType does not apply) AND + /// caller is not the policy admin AND accounts.length exceeds MAX_BATCH_SIZE. + /// Unauthorized fires before _batchSetMembers runs its batch-size guard. + function test_updateAllowlist_revertOrder_unauthorized_beats_batchSizeTooLarge(address caller, uint8 overflow) + public + { + _assumeValidCaller(caller); + // Create an ALLOWLIST policy with alice as admin; caller is not alice. + uint64 policyId = _createAllowlist(admin, alice); + vm.assume(caller != alice); + uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); + address[] memory accounts = _makeAccounts(n); + + // Both conditions apply independently: + // - Unauthorized: caller is not alice (the policy admin). + // - BatchSizeTooLarge: accounts.length > MAX_BATCH_SIZE. + vm.prank(caller); + vm.expectRevert(IPolicyRegistry.Unauthorized.selector); + policyRegistry.updateAllowlist(policyId, true, accounts); + } +} diff --git a/test/unit/PolicyRegistry/updateBlocklist_revertOrder.t.sol b/test/unit/PolicyRegistry/updateBlocklist_revertOrder.t.sol new file mode 100644 index 0000000..e1b3da2 --- /dev/null +++ b/test/unit/PolicyRegistry/updateBlocklist_revertOrder.t.sol @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {IPolicyRegistry} from "src/interfaces/IPolicyRegistry.sol"; + +import {PolicyRegistryTest} from "test/lib/PolicyRegistryTest.sol"; + +/// @title Differential check-order tests for `updateBlocklist`. +/// +/// @notice **Canonical order (Solidity reference):** +/// 1. POLICY-NOT-FOUND (`_requireCustom`: packed == 0) → `PolicyNotFound` +/// 2. INCOMPATIBLE-TYPE (`_typeOf(policyId) != BLOCKLIST`) → `IncompatiblePolicyType` +/// 3. UNAUTHORIZED (`_decodeAdmin(packed) != msg.sender`) → `Unauthorized` +/// 4. BATCH-SIZE (inside `_batchSetMembers`) → `BatchSizeTooLarge` +/// +/// C(4, 2) = 6 pairs. Mirror of `updateAllowlist_revertOrder.t.sol` with +/// ALLOWLIST and BLOCKLIST roles swapped. +contract PolicyRegistryUpdateBlocklistRevertOrderTest is PolicyRegistryTest { + // --------------------------------------------------------------- + // Pairs where POLICY-NOT-FOUND wins + // --------------------------------------------------------------- + + /// @notice POLICY-NOT-FOUND beats INCOMPATIBLE-TYPE. + /// @dev policyId encodes as ALLOWLIST (top byte 1, incompatible for updateBlocklist) + /// AND the policy has never been created (packed == 0). + /// PolicyNotFound fires before the type discriminator is examined. + function test_updateBlocklist_revertOrder_policyNotFound_beats_incompatiblePolicyType(uint56 counter) public { + counter = uint56(bound(uint256(counter), 2, type(uint56).max)); + // top byte 1 = ALLOWLIST; would trigger IncompatiblePolicyType if the policy existed. + uint64 policyId = (uint64(1) << 56) | uint64(counter); + address[] memory empty = new address[](0); + + // Both conditions apply independently: + // - PolicyNotFound: policyId has never been created. + // - IncompatiblePolicyType: policyId encodes as ALLOWLIST, not BLOCKLIST. + vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + policyRegistry.updateBlocklist(policyId, true, empty); + } + + /// @notice POLICY-NOT-FOUND beats UNAUTHORIZED. + /// @dev policyId encodes as BLOCKLIST (so IncompatiblePolicyType does not apply) + /// AND has never been created (PolicyNotFound fires), AND caller is non-zero + /// (Unauthorized would fire if policyId existed as BLOCKLIST). + function test_updateBlocklist_revertOrder_policyNotFound_beats_unauthorized(address caller, uint56 counter) public { + _assumeValidCaller(caller); + counter = uint56(bound(uint256(counter), 2, type(uint56).max)); + // top byte 0 = BLOCKLIST; IncompatiblePolicyType would not apply if the policy existed. + uint64 policyId = uint64(counter); + address[] memory empty = new address[](0); + + // Both conditions apply independently: + // - PolicyNotFound: policyId has never been created (packed == 0). + // - Unauthorized: _decodeAdmin(0) == address(0) != caller. + vm.prank(caller); + vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + policyRegistry.updateBlocklist(policyId, true, empty); + } + + /// @notice POLICY-NOT-FOUND beats BATCH-SIZE. + /// @dev policyId has never been created (PolicyNotFound fires) AND accounts.length + /// exceeds MAX_BATCH_SIZE (BatchSizeTooLarge would fire inside _batchSetMembers + /// if the policy existed and all earlier checks passed). + function test_updateBlocklist_revertOrder_policyNotFound_beats_batchSizeTooLarge(uint56 counter, uint8 overflow) + public + { + counter = uint56(bound(uint256(counter), 2, type(uint56).max)); + uint64 policyId = uint64(counter); // BLOCKLIST type, not created + uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); + address[] memory accounts = _makeAccounts(n); + + // Both conditions apply independently: + // - PolicyNotFound: policyId has never been created. + // - BatchSizeTooLarge: accounts.length > MAX_BATCH_SIZE. + vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + policyRegistry.updateBlocklist(policyId, true, accounts); + } + + // --------------------------------------------------------------- + // Pairs where INCOMPATIBLE-TYPE wins + // --------------------------------------------------------------- + + /// @notice INCOMPATIBLE-TYPE beats UNAUTHORIZED. + /// @dev Policy exists as ALLOWLIST (incompatible for updateBlocklist) AND caller + /// is not the policy admin (Unauthorized would fire if the type check were absent). + function test_updateBlocklist_revertOrder_incompatiblePolicyType_beats_unauthorized(address caller) public { + _assumeValidCaller(caller); + // Create an ALLOWLIST policy with `alice` as admin. + uint64 policyId = _createAllowlist(admin, alice); + vm.assume(caller != alice); + address[] memory empty = new address[](0); + + // Both conditions apply independently: + // - IncompatiblePolicyType: policy is ALLOWLIST, updateBlocklist requires BLOCKLIST. + // - Unauthorized: caller is not alice (the policy admin). + vm.prank(caller); + vm.expectRevert(IPolicyRegistry.IncompatiblePolicyType.selector); + policyRegistry.updateBlocklist(policyId, true, empty); + } + + /// @notice INCOMPATIBLE-TYPE beats BATCH-SIZE. + /// @dev Policy exists as ALLOWLIST AND accounts.length exceeds MAX_BATCH_SIZE. + /// Caller is the policy admin so Unauthorized does not apply; only type and + /// batch-size conditions compete. + function test_updateBlocklist_revertOrder_incompatiblePolicyType_beats_batchSizeTooLarge(uint8 overflow) public { + // Create an ALLOWLIST policy with alice as admin; call as alice (no Unauthorized). + uint64 policyId = _createAllowlist(admin, alice); + uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); + address[] memory accounts = _makeAccounts(n); + + // Both conditions apply independently: + // - IncompatiblePolicyType: policy is ALLOWLIST, updateBlocklist requires BLOCKLIST. + // - BatchSizeTooLarge: accounts.length > MAX_BATCH_SIZE. + vm.prank(alice); + vm.expectRevert(IPolicyRegistry.IncompatiblePolicyType.selector); + policyRegistry.updateBlocklist(policyId, true, accounts); + } + + // --------------------------------------------------------------- + // Pairs where UNAUTHORIZED wins + // --------------------------------------------------------------- + + /// @notice UNAUTHORIZED beats BATCH-SIZE. + /// @dev Policy exists as BLOCKLIST (IncompatiblePolicyType does not apply) AND + /// caller is not the policy admin AND accounts.length exceeds MAX_BATCH_SIZE. + /// Unauthorized fires before _batchSetMembers runs its batch-size guard. + function test_updateBlocklist_revertOrder_unauthorized_beats_batchSizeTooLarge(address caller, uint8 overflow) + public + { + _assumeValidCaller(caller); + // Create a BLOCKLIST policy with alice as admin; caller is not alice. + uint64 policyId = _createBlocklist(admin, alice); + vm.assume(caller != alice); + uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); + address[] memory accounts = _makeAccounts(n); + + // Both conditions apply independently: + // - Unauthorized: caller is not alice (the policy admin). + // - BatchSizeTooLarge: accounts.length > MAX_BATCH_SIZE. + vm.prank(caller); + vm.expectRevert(IPolicyRegistry.Unauthorized.selector); + policyRegistry.updateBlocklist(policyId, true, accounts); + } +} From 1c78d73cef19c7822a749975d6d0b8d0e0b99913 Mon Sep 17 00:00:00 2001 From: Eric Shenghsiung Liu Date: Mon, 1 Jun 2026 19:09:27 -0400 Subject: [PATCH 2/3] chore: forge fmt --- test/unit/PolicyRegistry/updateAllowlist_revertOrder.t.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/unit/PolicyRegistry/updateAllowlist_revertOrder.t.sol b/test/unit/PolicyRegistry/updateAllowlist_revertOrder.t.sol index 2be3143..d4bd3d6 100644 --- a/test/unit/PolicyRegistry/updateAllowlist_revertOrder.t.sol +++ b/test/unit/PolicyRegistry/updateAllowlist_revertOrder.t.sol @@ -40,9 +40,7 @@ contract PolicyRegistryUpdateAllowlistRevertOrderTest is PolicyRegistryTest { /// @dev policyId encodes as ALLOWLIST (so IncompatiblePolicyType does not apply) /// AND has never been created (PolicyNotFound fires), AND caller is non-zero /// (Unauthorized would fire if policyId existed as ALLOWLIST). - function test_updateAllowlist_revertOrder_policyNotFound_beats_unauthorized(address caller, uint56 counter) - public - { + function test_updateAllowlist_revertOrder_policyNotFound_beats_unauthorized(address caller, uint56 counter) public { _assumeValidCaller(caller); counter = uint56(bound(uint256(counter), 2, type(uint56).max)); // top byte 1 = ALLOWLIST; IncompatiblePolicyType would not apply if the policy existed. From 6d7aa076fa91f129ea6275f5fb737fe482b51ff9 Mon Sep 17 00:00:00 2001 From: Eric Shenghsiung Liu Date: Mon, 1 Jun 2026 20:04:49 -0400 Subject: [PATCH 3/3] test(PolicyRegistry): rewrite revertOrder tests to sequential walk-through style Replace multiple pairwise tests with single sequential tests that walk through all revert conditions in order, fixing one per step until success. Implement the previously-empty createPolicy_revertOrder.t.sol. --- ...createPolicyWithAccounts_revertOrder.t.sol | 42 +++-- .../createPolicy_revertOrder.t.sol | 32 +++- .../finalizeUpdateAdmin_revertOrder.t.sol | 78 ++++----- .../renounceAdmin_revertOrder.t.sol | 39 +++-- .../stageUpdateAdmin_revertOrder.t.sol | 45 +++--- .../updateAllowlist_revertOrder.t.sol | 151 +++++------------ .../updateBlocklist_revertOrder.t.sol | 153 +++++------------- 7 files changed, 202 insertions(+), 338 deletions(-) diff --git a/test/unit/PolicyRegistry/createPolicyWithAccounts_revertOrder.t.sol b/test/unit/PolicyRegistry/createPolicyWithAccounts_revertOrder.t.sol index 4f16a52..75d1678 100644 --- a/test/unit/PolicyRegistry/createPolicyWithAccounts_revertOrder.t.sol +++ b/test/unit/PolicyRegistry/createPolicyWithAccounts_revertOrder.t.sol @@ -5,33 +5,41 @@ import {IPolicyRegistry} from "src/interfaces/IPolicyRegistry.sol"; import {PolicyRegistryTest} from "test/lib/PolicyRegistryTest.sol"; -/// @title Differential check-order tests for `createPolicyWithAccounts`. +/// @title Sequential revert-order test for `createPolicyWithAccounts`. /// -/// @notice **Canonical order (Solidity reference entry-point, pre-`_create`):** +/// @notice **Canonical order:** /// 1. ZERO-ADMIN (`admin == address(0)`) → `ZeroAddress` /// 2. BATCH-SIZE (`accounts.length > MAX_BATCH_SIZE`) → `BatchSizeTooLarge` /// -/// C(2, 2) = 1 pair. -/// -/// The natspec on `createPolicyWithAccounts` explicitly annotates this ordering: -/// "Reverts with `ZeroAddress` … Takes precedence over `BatchSizeTooLarge`." -/// This test pins that annotation against the Solidity reference implementation. +/// Walks from all conditions broken to success, fixing one per step. contract PolicyRegistryCreatePolicyWithAccountsRevertOrderTest is PolicyRegistryTest { - /// @notice ZERO-ADMIN beats BATCH-SIZE. - /// @dev admin is address(0) AND accounts.length exceeds MAX_BATCH_SIZE. - /// The zero-admin guard is checked before the batch-size guard. - function test_createPolicyWithAccounts_revertOrder_zeroAddress_beats_batchSizeTooLarge( - address caller, - uint8 typeIdx, - uint8 overflow - ) public { + /// @notice Walks through every revert in canonical order, fixing one per step, ending at success. + function test_createPolicyWithAccounts_revertOrder(address caller, address admin_, uint8 typeIdx, uint8 overflow) + public + { _assumeValidCaller(caller); + vm.assume(admin_ != address(0)); IPolicyRegistry.PolicyType pt = _creatablePolicyType(typeIdx); uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); - address[] memory accounts = _makeAccounts(n); + address[] memory tooMany = _makeAccounts(n); + address[] memory valid = new address[](0); + // 1. ZERO-ADMIN: admin==address(0) AND batch oversized → ZeroAddress fires first. vm.prank(caller); vm.expectRevert(IPolicyRegistry.ZeroAddress.selector); - policyRegistry.createPolicyWithAccounts(address(0), pt, accounts); + policyRegistry.createPolicyWithAccounts(address(0), pt, tooMany); + + // Fix: use a non-zero admin. + + // 2. BATCH-SIZE: valid admin, but accounts.length > MAX_BATCH_SIZE → BatchSizeTooLarge. + vm.prank(caller); + vm.expectRevert(abi.encodeWithSelector(IPolicyRegistry.BatchSizeTooLarge.selector, MAX_BATCH_SIZE)); + policyRegistry.createPolicyWithAccounts(admin_, pt, tooMany); + + // Fix: use an empty accounts array. + + // Success + vm.prank(caller); + policyRegistry.createPolicyWithAccounts(admin_, pt, valid); } } diff --git a/test/unit/PolicyRegistry/createPolicy_revertOrder.t.sol b/test/unit/PolicyRegistry/createPolicy_revertOrder.t.sol index 8a0c6f4..7dc9c8a 100644 --- a/test/unit/PolicyRegistry/createPolicy_revertOrder.t.sol +++ b/test/unit/PolicyRegistry/createPolicy_revertOrder.t.sol @@ -1,16 +1,32 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.20; +import {IPolicyRegistry} from "src/interfaces/IPolicyRegistry.sol"; + import {PolicyRegistryTest} from "test/lib/PolicyRegistryTest.sol"; -/// @title Differential check-order tests for `createPolicy`. +/// @title Sequential revert-order test for `createPolicy`. /// -/// @notice **Canonical order (Solidity reference `_create`):** +/// @notice **Canonical order:** /// 1. ZERO-ADMIN (`admin == address(0)`) → `ZeroAddress` /// -/// C(1, 2) = 0 pairs. `createPolicy` has only a single revert -/// condition so there is no pair-wise ordering to pin. This file -/// exists for coverage bookkeeping only; the individual revert is -/// tested in `createPolicy.t.sol`. -// solhint-disable-next-line no-empty-blocks -contract PolicyRegistryCreatePolicyRevertOrderTest is PolicyRegistryTest {} +/// Single revert condition; walks from that condition to success. +contract PolicyRegistryCreatePolicyRevertOrderTest is PolicyRegistryTest { + /// @notice Walks through every revert in canonical order, fixing one per step, ending at success. + function test_createPolicy_revertOrder(address caller, address admin_, uint8 typeIdx) public { + _assumeValidCaller(caller); + vm.assume(admin_ != address(0)); + IPolicyRegistry.PolicyType pt = _creatablePolicyType(typeIdx); + + // 1. ZERO-ADMIN: admin == address(0) → ZeroAddress + vm.prank(caller); + vm.expectRevert(IPolicyRegistry.ZeroAddress.selector); + policyRegistry.createPolicy(address(0), pt); + + // Fix: use a non-zero admin. + + // Success + vm.prank(caller); + policyRegistry.createPolicy(admin_, pt); + } +} diff --git a/test/unit/PolicyRegistry/finalizeUpdateAdmin_revertOrder.t.sol b/test/unit/PolicyRegistry/finalizeUpdateAdmin_revertOrder.t.sol index d48f631..26c5d04 100644 --- a/test/unit/PolicyRegistry/finalizeUpdateAdmin_revertOrder.t.sol +++ b/test/unit/PolicyRegistry/finalizeUpdateAdmin_revertOrder.t.sol @@ -5,69 +5,49 @@ import {IPolicyRegistry} from "src/interfaces/IPolicyRegistry.sol"; import {PolicyRegistryTest} from "test/lib/PolicyRegistryTest.sol"; -/// @title Differential check-order tests for `finalizeUpdateAdmin`. +/// @title Sequential revert-order test for `finalizeUpdateAdmin`. /// -/// @notice **Canonical order (Solidity reference):** +/// @notice **Canonical order:** /// 1. POLICY-NOT-FOUND (`policies[policyId] == 0`) → `PolicyNotFound` /// 2. NO-PENDING-ADMIN (`pendingAdmins[policyId] == address(0)`) → `NoPendingAdmin` /// 3. UNAUTHORIZED (`pendingAdmins[policyId] != msg.sender`) → `Unauthorized` /// -/// C(3, 2) = 3 pairs. +/// Walks from all conditions broken to success, fixing one per step. contract PolicyRegistryFinalizeUpdateAdminRevertOrderTest is PolicyRegistryTest { - // --------------------------------------------------------------- - // Pairs where POLICY-NOT-FOUND wins - // --------------------------------------------------------------- + /// @notice Walks through every revert in canonical order, fixing one per step, ending at success. + function test_finalizeUpdateAdmin_revertOrder() public { + // ghostId is a well-formed policyId that has never been created. + uint64 ghostId = _wellFormedUncreatedPolicyId(type(uint64).max); + + // 1. POLICY-NOT-FOUND: policyId has never been created AND no pending admin is + // staged (NoPendingAdmin and Unauthorized would also apply once a policy exists). + vm.prank(attacker); + vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + policyRegistry.finalizeUpdateAdmin(ghostId); - /// @notice POLICY-NOT-FOUND beats NO-PENDING-ADMIN. - /// @dev policyId does not exist (packed == 0) AND no pending admin is staged. - /// PolicyNotFound fires before the pending-admin slot is read. - function test_finalizeUpdateAdmin_revertOrder_policyNotFound_beats_noPendingAdmin(uint64 seed) public { - uint64 policyId = _wellFormedUncreatedPolicyId(seed); + // Fix: create an allowlist policy with alice as admin. + uint64 policyId = policyRegistry.createPolicy(alice, IPolicyRegistry.PolicyType.ALLOWLIST); - // Both conditions apply independently: - // - PolicyNotFound: policyId has never been created. - // - NoPendingAdmin: pendingAdmins[policyId] == address(0) (zero storage). - vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + // 2. NO-PENDING-ADMIN: policy exists but no pending admin has been staged. + // (Unauthorized would also fire if NoPendingAdmin were absent, since + // pendingAdmins[policyId] == address(0) != attacker.) + vm.prank(attacker); + vm.expectRevert(IPolicyRegistry.NoPendingAdmin.selector); policyRegistry.finalizeUpdateAdmin(policyId); - } - /// @notice POLICY-NOT-FOUND beats UNAUTHORIZED. - /// @dev policyId does not exist AND caller is not address(0), so the pending-admin - /// comparison (address(0) != caller) would trigger Unauthorized if reached. - /// PolicyNotFound fires before the pending-admin slot or caller comparison runs. - function test_finalizeUpdateAdmin_revertOrder_policyNotFound_beats_unauthorized(address caller, uint64 seed) - public - { - _assumeValidCaller(caller); - uint64 policyId = _wellFormedUncreatedPolicyId(seed); + // Fix: stage bob as the pending admin. + vm.prank(alice); + policyRegistry.stageUpdateAdmin(policyId, bob); - // Both conditions apply independently: - // - PolicyNotFound: policyId has never been created (packed == 0). - // - Unauthorized: pendingAdmins[policyId] == address(0) != caller. - // (NoPendingAdmin also applies; it would fire second if PolicyNotFound didn't.) - vm.prank(caller); - vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + // 3. UNAUTHORIZED: bob is the staged pending admin, but attacker is calling. + vm.prank(attacker); + vm.expectRevert(IPolicyRegistry.Unauthorized.selector); policyRegistry.finalizeUpdateAdmin(policyId); - } - // --------------------------------------------------------------- - // Pairs where NO-PENDING-ADMIN wins - // --------------------------------------------------------------- + // Fix: call as bob (the staged pending admin). - /// @notice NO-PENDING-ADMIN beats UNAUTHORIZED. - /// @dev Policy exists but has no pending admin staged. Caller is a non-zero address, - /// so `pendingAdmins[policyId] (== address(0)) != caller` would trigger - /// Unauthorized if the NoPendingAdmin guard were absent. - function test_finalizeUpdateAdmin_revertOrder_noPendingAdmin_beats_unauthorized(address caller) public { - _assumeValidCaller(caller); - // Create a policy; no stageUpdateAdmin call follows, so pending == address(0). - uint64 policyId = policyRegistry.createPolicy(admin, IPolicyRegistry.PolicyType.ALLOWLIST); - - // Both conditions apply independently: - // - NoPendingAdmin: pendingAdmins[policyId] == address(0). - // - Unauthorized: address(0) != caller (caller is non-zero). - vm.prank(caller); - vm.expectRevert(IPolicyRegistry.NoPendingAdmin.selector); + // Success + vm.prank(bob); policyRegistry.finalizeUpdateAdmin(policyId); } } diff --git a/test/unit/PolicyRegistry/renounceAdmin_revertOrder.t.sol b/test/unit/PolicyRegistry/renounceAdmin_revertOrder.t.sol index d7dfac8..1d33513 100644 --- a/test/unit/PolicyRegistry/renounceAdmin_revertOrder.t.sol +++ b/test/unit/PolicyRegistry/renounceAdmin_revertOrder.t.sol @@ -5,26 +5,37 @@ import {IPolicyRegistry} from "src/interfaces/IPolicyRegistry.sol"; import {PolicyRegistryTest} from "test/lib/PolicyRegistryTest.sol"; -/// @title Differential check-order tests for `renounceAdmin`. +/// @title Sequential revert-order test for `renounceAdmin`. /// -/// @notice **Canonical order (Solidity reference):** +/// @notice **Canonical order:** /// 1. POLICY-NOT-FOUND (`policies[policyId] == 0`) → `PolicyNotFound` /// 2. UNAUTHORIZED (`_decodeAdmin(packed) != msg.sender`) → `Unauthorized` /// -/// C(2, 2) = 1 pair. +/// Walks from all conditions broken to success, fixing one per step. contract PolicyRegistryRenounceAdminRevertOrderTest is PolicyRegistryTest { - /// @notice POLICY-NOT-FOUND beats UNAUTHORIZED. - /// @dev policyId does not exist (packed == 0) AND caller is not the policy admin. - /// PolicyNotFound fires before the admin comparison runs. - function test_renounceAdmin_revertOrder_policyNotFound_beats_unauthorized(address caller, uint64 seed) public { - _assumeValidCaller(caller); - uint64 policyId = _wellFormedUncreatedPolicyId(seed); - - // Both conditions apply independently: - // - PolicyNotFound: policyId has never been created (policies[policyId] == 0). - // - Unauthorized: _decodeAdmin(0) == address(0) != caller (caller is non-zero). - vm.prank(caller); + /// @notice Walks through every revert in canonical order, fixing one per step, ending at success. + function test_renounceAdmin_revertOrder() public { + // ghostId is a well-formed policyId that has never been created. + uint64 ghostId = _wellFormedUncreatedPolicyId(type(uint64).max); + + // 1. POLICY-NOT-FOUND: policyId has never been created AND attacker is not the + // policy admin (Unauthorized would also apply if the policy existed). + vm.prank(attacker); vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + policyRegistry.renounceAdmin(ghostId); + + // Fix: create an allowlist policy with alice as admin. + uint64 policyId = policyRegistry.createPolicy(alice, IPolicyRegistry.PolicyType.ALLOWLIST); + + // 2. UNAUTHORIZED: attacker is not the policy admin (alice). + vm.prank(attacker); + vm.expectRevert(IPolicyRegistry.Unauthorized.selector); + policyRegistry.renounceAdmin(policyId); + + // Fix: call as alice (the policy admin). + + // Success + vm.prank(alice); policyRegistry.renounceAdmin(policyId); } } diff --git a/test/unit/PolicyRegistry/stageUpdateAdmin_revertOrder.t.sol b/test/unit/PolicyRegistry/stageUpdateAdmin_revertOrder.t.sol index 0f61a0a..62e0178 100644 --- a/test/unit/PolicyRegistry/stageUpdateAdmin_revertOrder.t.sol +++ b/test/unit/PolicyRegistry/stageUpdateAdmin_revertOrder.t.sol @@ -5,30 +5,37 @@ import {IPolicyRegistry} from "src/interfaces/IPolicyRegistry.sol"; import {PolicyRegistryTest} from "test/lib/PolicyRegistryTest.sol"; -/// @title Differential check-order tests for `stageUpdateAdmin`. +/// @title Sequential revert-order test for `stageUpdateAdmin`. /// -/// @notice **Canonical order (Solidity reference):** -/// 1. POLICY-NOT-FOUND (`_requireCustom`: packed == 0) → `PolicyNotFound` +/// @notice **Canonical order:** +/// 1. POLICY-NOT-FOUND (`policies[policyId] == 0`) → `PolicyNotFound` /// 2. UNAUTHORIZED (`_decodeAdmin(packed) != msg.sender`) → `Unauthorized` /// -/// C(2, 2) = 1 pair. +/// Walks from all conditions broken to success, fixing one per step. contract PolicyRegistryStageUpdateAdminRevertOrderTest is PolicyRegistryTest { - /// @notice POLICY-NOT-FOUND beats UNAUTHORIZED. - /// @dev policyId does not exist (packed == 0) AND caller is not the policy admin. - /// `_requireCustom` reverts with `PolicyNotFound` before the admin check runs. - function test_stageUpdateAdmin_revertOrder_policyNotFound_beats_unauthorized( - address caller, - uint64 seed, - address newAdmin - ) public { - _assumeValidCaller(caller); - uint64 policyId = _wellFormedUncreatedPolicyId(seed); - - // Both conditions apply independently: - // - PolicyNotFound: policyId has never been created (policies[policyId] == 0). - // - Unauthorized: _decodeAdmin(0) == address(0) != caller (caller is non-zero). - vm.prank(caller); + /// @notice Walks through every revert in canonical order, fixing one per step, ending at success. + function test_stageUpdateAdmin_revertOrder(address newAdmin) public { + // ghostId is a well-formed policyId that has never been created. + uint64 ghostId = _wellFormedUncreatedPolicyId(type(uint64).max); + + // 1. POLICY-NOT-FOUND: policyId has never been created AND attacker is not the + // policy admin (Unauthorized would also apply if the policy existed). + vm.prank(attacker); vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); + policyRegistry.stageUpdateAdmin(ghostId, newAdmin); + + // Fix: create an allowlist policy with alice as admin. + uint64 policyId = policyRegistry.createPolicy(alice, IPolicyRegistry.PolicyType.ALLOWLIST); + + // 2. UNAUTHORIZED: attacker is not the policy admin (alice). + vm.prank(attacker); + vm.expectRevert(IPolicyRegistry.Unauthorized.selector); + policyRegistry.stageUpdateAdmin(policyId, newAdmin); + + // Fix: call as alice (the policy admin). + + // Success + vm.prank(alice); policyRegistry.stageUpdateAdmin(policyId, newAdmin); } } diff --git a/test/unit/PolicyRegistry/updateAllowlist_revertOrder.t.sol b/test/unit/PolicyRegistry/updateAllowlist_revertOrder.t.sol index d4bd3d6..701a03e 100644 --- a/test/unit/PolicyRegistry/updateAllowlist_revertOrder.t.sol +++ b/test/unit/PolicyRegistry/updateAllowlist_revertOrder.t.sol @@ -5,138 +5,59 @@ import {IPolicyRegistry} from "src/interfaces/IPolicyRegistry.sol"; import {PolicyRegistryTest} from "test/lib/PolicyRegistryTest.sol"; -/// @title Differential check-order tests for `updateAllowlist`. +/// @title Sequential revert-order test for `updateAllowlist`. /// -/// @notice **Canonical order (Solidity reference):** -/// 1. POLICY-NOT-FOUND (`_requireCustom`: packed == 0) → `PolicyNotFound` +/// @notice **Canonical order:** +/// 1. POLICY-NOT-FOUND (`policies[policyId] == 0`) → `PolicyNotFound` /// 2. INCOMPATIBLE-TYPE (`_typeOf(policyId) != ALLOWLIST`) → `IncompatiblePolicyType` /// 3. UNAUTHORIZED (`_decodeAdmin(packed) != msg.sender`) → `Unauthorized` /// 4. BATCH-SIZE (inside `_batchSetMembers`) → `BatchSizeTooLarge` /// -/// C(4, 2) = 6 pairs. +/// Walks from all conditions broken to success, fixing one per step. contract PolicyRegistryUpdateAllowlistRevertOrderTest is PolicyRegistryTest { - // --------------------------------------------------------------- - // Pairs where POLICY-NOT-FOUND wins - // --------------------------------------------------------------- - - /// @notice POLICY-NOT-FOUND beats INCOMPATIBLE-TYPE. - /// @dev policyId encodes as BLOCKLIST (top byte 0, incompatible for updateAllowlist) - /// AND the policy has never been created (packed == 0). - /// PolicyNotFound fires before the type discriminator is examined. - function test_updateAllowlist_revertOrder_policyNotFound_beats_incompatiblePolicyType(uint56 counter) public { - counter = uint56(bound(uint256(counter), 2, type(uint56).max)); - // top byte 0 = BLOCKLIST; would trigger IncompatiblePolicyType if the policy existed. - uint64 policyId = uint64(counter); + /// @notice Walks through every revert in canonical order, fixing one per step, ending at success. + function test_updateAllowlist_revertOrder(uint8 overflow) public { + uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); + address[] memory tooMany = _makeAccounts(n); address[] memory empty = new address[](0); + // ghostId is a well-formed policyId that has never been created. + uint64 ghostId = _wellFormedUncreatedPolicyId(type(uint64).max); - // Both conditions apply independently: - // - PolicyNotFound: policyId has never been created. - // - IncompatiblePolicyType: policyId encodes as BLOCKLIST, not ALLOWLIST. + // 1. POLICY-NOT-FOUND: policyId has never been created AND all later conditions + // would also apply (wrong type, unauthorized caller, oversized batch). + vm.prank(attacker); vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); - policyRegistry.updateAllowlist(policyId, true, empty); - } + policyRegistry.updateAllowlist(ghostId, true, tooMany); - /// @notice POLICY-NOT-FOUND beats UNAUTHORIZED. - /// @dev policyId encodes as ALLOWLIST (so IncompatiblePolicyType does not apply) - /// AND has never been created (PolicyNotFound fires), AND caller is non-zero - /// (Unauthorized would fire if policyId existed as ALLOWLIST). - function test_updateAllowlist_revertOrder_policyNotFound_beats_unauthorized(address caller, uint56 counter) public { - _assumeValidCaller(caller); - counter = uint56(bound(uint256(counter), 2, type(uint56).max)); - // top byte 1 = ALLOWLIST; IncompatiblePolicyType would not apply if the policy existed. - uint64 policyId = (uint64(1) << 56) | uint64(counter); - address[] memory empty = new address[](0); + // Fix: create a BLOCKLIST policy with alice as admin (exists, but wrong type for updateAllowlist). + uint64 blocklistId = _createBlocklist(admin, alice); - // Both conditions apply independently: - // - PolicyNotFound: policyId has never been created (packed == 0). - // - Unauthorized: _decodeAdmin(0) == address(0) != caller. - vm.prank(caller); - vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); - policyRegistry.updateAllowlist(policyId, true, empty); - } - - /// @notice POLICY-NOT-FOUND beats BATCH-SIZE. - /// @dev policyId has never been created (PolicyNotFound fires) AND accounts.length - /// exceeds MAX_BATCH_SIZE (BatchSizeTooLarge would fire inside _batchSetMembers - /// if the policy existed and all earlier checks passed). - function test_updateAllowlist_revertOrder_policyNotFound_beats_batchSizeTooLarge(uint56 counter, uint8 overflow) - public - { - counter = uint56(bound(uint256(counter), 2, type(uint56).max)); - uint64 policyId = (uint64(1) << 56) | uint64(counter); // ALLOWLIST type, not created - uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); - address[] memory accounts = _makeAccounts(n); - - // Both conditions apply independently: - // - PolicyNotFound: policyId has never been created. - // - BatchSizeTooLarge: accounts.length > MAX_BATCH_SIZE. - vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); - policyRegistry.updateAllowlist(policyId, true, accounts); - } - - // --------------------------------------------------------------- - // Pairs where INCOMPATIBLE-TYPE wins - // --------------------------------------------------------------- + // 2. INCOMPATIBLE-TYPE: policy exists as BLOCKLIST; updateAllowlist requires ALLOWLIST. + // (Unauthorized and BatchSizeTooLarge would also apply.) + vm.prank(attacker); + vm.expectRevert(IPolicyRegistry.IncompatiblePolicyType.selector); + policyRegistry.updateAllowlist(blocklistId, true, tooMany); - /// @notice INCOMPATIBLE-TYPE beats UNAUTHORIZED. - /// @dev Policy exists as BLOCKLIST (incompatible for updateAllowlist) AND caller - /// is not the policy admin (Unauthorized would fire if the type check were absent). - function test_updateAllowlist_revertOrder_incompatiblePolicyType_beats_unauthorized(address caller) public { - _assumeValidCaller(caller); - // Create a BLOCKLIST policy with `alice` as admin. - uint64 policyId = _createBlocklist(admin, alice); - vm.assume(caller != alice); - address[] memory empty = new address[](0); + // Fix: create an ALLOWLIST policy with alice as admin. + uint64 policyId = _createAllowlist(admin, alice); - // Both conditions apply independently: - // - IncompatiblePolicyType: policy is BLOCKLIST, updateAllowlist requires ALLOWLIST. - // - Unauthorized: caller is not alice (the policy admin). - vm.prank(caller); - vm.expectRevert(IPolicyRegistry.IncompatiblePolicyType.selector); - policyRegistry.updateAllowlist(policyId, true, empty); - } + // 3. UNAUTHORIZED: policy is ALLOWLIST but attacker is not the policy admin (alice). + // (BatchSizeTooLarge would also apply.) + vm.prank(attacker); + vm.expectRevert(IPolicyRegistry.Unauthorized.selector); + policyRegistry.updateAllowlist(policyId, true, tooMany); - /// @notice INCOMPATIBLE-TYPE beats BATCH-SIZE. - /// @dev Policy exists as BLOCKLIST AND accounts.length exceeds MAX_BATCH_SIZE. - /// Caller is the policy admin so Unauthorized does not apply; only type and - /// batch-size conditions compete. - function test_updateAllowlist_revertOrder_incompatiblePolicyType_beats_batchSizeTooLarge(uint8 overflow) public { - // Create a BLOCKLIST policy with alice as admin; call as alice (no Unauthorized). - uint64 policyId = _createBlocklist(admin, alice); - uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); - address[] memory accounts = _makeAccounts(n); + // Fix: call as alice (the policy admin). - // Both conditions apply independently: - // - IncompatiblePolicyType: policy is BLOCKLIST, updateAllowlist requires ALLOWLIST. - // - BatchSizeTooLarge: accounts.length > MAX_BATCH_SIZE. + // 4. BATCH-SIZE: alice is the admin, but accounts.length > MAX_BATCH_SIZE. vm.prank(alice); - vm.expectRevert(IPolicyRegistry.IncompatiblePolicyType.selector); - policyRegistry.updateAllowlist(policyId, true, accounts); - } - - // --------------------------------------------------------------- - // Pairs where UNAUTHORIZED wins - // --------------------------------------------------------------- + vm.expectRevert(abi.encodeWithSelector(IPolicyRegistry.BatchSizeTooLarge.selector, MAX_BATCH_SIZE)); + policyRegistry.updateAllowlist(policyId, true, tooMany); - /// @notice UNAUTHORIZED beats BATCH-SIZE. - /// @dev Policy exists as ALLOWLIST (IncompatiblePolicyType does not apply) AND - /// caller is not the policy admin AND accounts.length exceeds MAX_BATCH_SIZE. - /// Unauthorized fires before _batchSetMembers runs its batch-size guard. - function test_updateAllowlist_revertOrder_unauthorized_beats_batchSizeTooLarge(address caller, uint8 overflow) - public - { - _assumeValidCaller(caller); - // Create an ALLOWLIST policy with alice as admin; caller is not alice. - uint64 policyId = _createAllowlist(admin, alice); - vm.assume(caller != alice); - uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); - address[] memory accounts = _makeAccounts(n); + // Fix: use an empty accounts array. - // Both conditions apply independently: - // - Unauthorized: caller is not alice (the policy admin). - // - BatchSizeTooLarge: accounts.length > MAX_BATCH_SIZE. - vm.prank(caller); - vm.expectRevert(IPolicyRegistry.Unauthorized.selector); - policyRegistry.updateAllowlist(policyId, true, accounts); + // Success + vm.prank(alice); + policyRegistry.updateAllowlist(policyId, true, empty); } } diff --git a/test/unit/PolicyRegistry/updateBlocklist_revertOrder.t.sol b/test/unit/PolicyRegistry/updateBlocklist_revertOrder.t.sol index e1b3da2..3433aea 100644 --- a/test/unit/PolicyRegistry/updateBlocklist_revertOrder.t.sol +++ b/test/unit/PolicyRegistry/updateBlocklist_revertOrder.t.sol @@ -5,139 +5,60 @@ import {IPolicyRegistry} from "src/interfaces/IPolicyRegistry.sol"; import {PolicyRegistryTest} from "test/lib/PolicyRegistryTest.sol"; -/// @title Differential check-order tests for `updateBlocklist`. +/// @title Sequential revert-order test for `updateBlocklist`. /// -/// @notice **Canonical order (Solidity reference):** -/// 1. POLICY-NOT-FOUND (`_requireCustom`: packed == 0) → `PolicyNotFound` +/// @notice **Canonical order:** +/// 1. POLICY-NOT-FOUND (`policies[policyId] == 0`) → `PolicyNotFound` /// 2. INCOMPATIBLE-TYPE (`_typeOf(policyId) != BLOCKLIST`) → `IncompatiblePolicyType` /// 3. UNAUTHORIZED (`_decodeAdmin(packed) != msg.sender`) → `Unauthorized` /// 4. BATCH-SIZE (inside `_batchSetMembers`) → `BatchSizeTooLarge` /// -/// C(4, 2) = 6 pairs. Mirror of `updateAllowlist_revertOrder.t.sol` with -/// ALLOWLIST and BLOCKLIST roles swapped. +/// Walks from all conditions broken to success, fixing one per step. +/// Mirror of `updateAllowlist_revertOrder.t.sol` with ALLOWLIST and BLOCKLIST roles swapped. contract PolicyRegistryUpdateBlocklistRevertOrderTest is PolicyRegistryTest { - // --------------------------------------------------------------- - // Pairs where POLICY-NOT-FOUND wins - // --------------------------------------------------------------- - - /// @notice POLICY-NOT-FOUND beats INCOMPATIBLE-TYPE. - /// @dev policyId encodes as ALLOWLIST (top byte 1, incompatible for updateBlocklist) - /// AND the policy has never been created (packed == 0). - /// PolicyNotFound fires before the type discriminator is examined. - function test_updateBlocklist_revertOrder_policyNotFound_beats_incompatiblePolicyType(uint56 counter) public { - counter = uint56(bound(uint256(counter), 2, type(uint56).max)); - // top byte 1 = ALLOWLIST; would trigger IncompatiblePolicyType if the policy existed. - uint64 policyId = (uint64(1) << 56) | uint64(counter); + /// @notice Walks through every revert in canonical order, fixing one per step, ending at success. + function test_updateBlocklist_revertOrder(uint8 overflow) public { + uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); + address[] memory tooMany = _makeAccounts(n); address[] memory empty = new address[](0); + // ghostId is a well-formed policyId that has never been created. + uint64 ghostId = _wellFormedUncreatedPolicyId(type(uint64).max); - // Both conditions apply independently: - // - PolicyNotFound: policyId has never been created. - // - IncompatiblePolicyType: policyId encodes as ALLOWLIST, not BLOCKLIST. + // 1. POLICY-NOT-FOUND: policyId has never been created AND all later conditions + // would also apply (wrong type, unauthorized caller, oversized batch). + vm.prank(attacker); vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); - policyRegistry.updateBlocklist(policyId, true, empty); - } + policyRegistry.updateBlocklist(ghostId, true, tooMany); - /// @notice POLICY-NOT-FOUND beats UNAUTHORIZED. - /// @dev policyId encodes as BLOCKLIST (so IncompatiblePolicyType does not apply) - /// AND has never been created (PolicyNotFound fires), AND caller is non-zero - /// (Unauthorized would fire if policyId existed as BLOCKLIST). - function test_updateBlocklist_revertOrder_policyNotFound_beats_unauthorized(address caller, uint56 counter) public { - _assumeValidCaller(caller); - counter = uint56(bound(uint256(counter), 2, type(uint56).max)); - // top byte 0 = BLOCKLIST; IncompatiblePolicyType would not apply if the policy existed. - uint64 policyId = uint64(counter); - address[] memory empty = new address[](0); + // Fix: create an ALLOWLIST policy with alice as admin (exists, but wrong type for updateBlocklist). + uint64 allowlistId = _createAllowlist(admin, alice); - // Both conditions apply independently: - // - PolicyNotFound: policyId has never been created (packed == 0). - // - Unauthorized: _decodeAdmin(0) == address(0) != caller. - vm.prank(caller); - vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); - policyRegistry.updateBlocklist(policyId, true, empty); - } - - /// @notice POLICY-NOT-FOUND beats BATCH-SIZE. - /// @dev policyId has never been created (PolicyNotFound fires) AND accounts.length - /// exceeds MAX_BATCH_SIZE (BatchSizeTooLarge would fire inside _batchSetMembers - /// if the policy existed and all earlier checks passed). - function test_updateBlocklist_revertOrder_policyNotFound_beats_batchSizeTooLarge(uint56 counter, uint8 overflow) - public - { - counter = uint56(bound(uint256(counter), 2, type(uint56).max)); - uint64 policyId = uint64(counter); // BLOCKLIST type, not created - uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); - address[] memory accounts = _makeAccounts(n); - - // Both conditions apply independently: - // - PolicyNotFound: policyId has never been created. - // - BatchSizeTooLarge: accounts.length > MAX_BATCH_SIZE. - vm.expectRevert(IPolicyRegistry.PolicyNotFound.selector); - policyRegistry.updateBlocklist(policyId, true, accounts); - } - - // --------------------------------------------------------------- - // Pairs where INCOMPATIBLE-TYPE wins - // --------------------------------------------------------------- + // 2. INCOMPATIBLE-TYPE: policy exists as ALLOWLIST; updateBlocklist requires BLOCKLIST. + // (Unauthorized and BatchSizeTooLarge would also apply.) + vm.prank(attacker); + vm.expectRevert(IPolicyRegistry.IncompatiblePolicyType.selector); + policyRegistry.updateBlocklist(allowlistId, true, tooMany); - /// @notice INCOMPATIBLE-TYPE beats UNAUTHORIZED. - /// @dev Policy exists as ALLOWLIST (incompatible for updateBlocklist) AND caller - /// is not the policy admin (Unauthorized would fire if the type check were absent). - function test_updateBlocklist_revertOrder_incompatiblePolicyType_beats_unauthorized(address caller) public { - _assumeValidCaller(caller); - // Create an ALLOWLIST policy with `alice` as admin. - uint64 policyId = _createAllowlist(admin, alice); - vm.assume(caller != alice); - address[] memory empty = new address[](0); + // Fix: create a BLOCKLIST policy with alice as admin. + uint64 policyId = _createBlocklist(admin, alice); - // Both conditions apply independently: - // - IncompatiblePolicyType: policy is ALLOWLIST, updateBlocklist requires BLOCKLIST. - // - Unauthorized: caller is not alice (the policy admin). - vm.prank(caller); - vm.expectRevert(IPolicyRegistry.IncompatiblePolicyType.selector); - policyRegistry.updateBlocklist(policyId, true, empty); - } + // 3. UNAUTHORIZED: policy is BLOCKLIST but attacker is not the policy admin (alice). + // (BatchSizeTooLarge would also apply.) + vm.prank(attacker); + vm.expectRevert(IPolicyRegistry.Unauthorized.selector); + policyRegistry.updateBlocklist(policyId, true, tooMany); - /// @notice INCOMPATIBLE-TYPE beats BATCH-SIZE. - /// @dev Policy exists as ALLOWLIST AND accounts.length exceeds MAX_BATCH_SIZE. - /// Caller is the policy admin so Unauthorized does not apply; only type and - /// batch-size conditions compete. - function test_updateBlocklist_revertOrder_incompatiblePolicyType_beats_batchSizeTooLarge(uint8 overflow) public { - // Create an ALLOWLIST policy with alice as admin; call as alice (no Unauthorized). - uint64 policyId = _createAllowlist(admin, alice); - uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); - address[] memory accounts = _makeAccounts(n); + // Fix: call as alice (the policy admin). - // Both conditions apply independently: - // - IncompatiblePolicyType: policy is ALLOWLIST, updateBlocklist requires BLOCKLIST. - // - BatchSizeTooLarge: accounts.length > MAX_BATCH_SIZE. + // 4. BATCH-SIZE: alice is the admin, but accounts.length > MAX_BATCH_SIZE. vm.prank(alice); - vm.expectRevert(IPolicyRegistry.IncompatiblePolicyType.selector); - policyRegistry.updateBlocklist(policyId, true, accounts); - } - - // --------------------------------------------------------------- - // Pairs where UNAUTHORIZED wins - // --------------------------------------------------------------- + vm.expectRevert(abi.encodeWithSelector(IPolicyRegistry.BatchSizeTooLarge.selector, MAX_BATCH_SIZE)); + policyRegistry.updateBlocklist(policyId, true, tooMany); - /// @notice UNAUTHORIZED beats BATCH-SIZE. - /// @dev Policy exists as BLOCKLIST (IncompatiblePolicyType does not apply) AND - /// caller is not the policy admin AND accounts.length exceeds MAX_BATCH_SIZE. - /// Unauthorized fires before _batchSetMembers runs its batch-size guard. - function test_updateBlocklist_revertOrder_unauthorized_beats_batchSizeTooLarge(address caller, uint8 overflow) - public - { - _assumeValidCaller(caller); - // Create a BLOCKLIST policy with alice as admin; caller is not alice. - uint64 policyId = _createBlocklist(admin, alice); - vm.assume(caller != alice); - uint256 n = MAX_BATCH_SIZE + 1 + (uint256(overflow) % 16); - address[] memory accounts = _makeAccounts(n); + // Fix: use an empty accounts array. - // Both conditions apply independently: - // - Unauthorized: caller is not alice (the policy admin). - // - BatchSizeTooLarge: accounts.length > MAX_BATCH_SIZE. - vm.prank(caller); - vm.expectRevert(IPolicyRegistry.Unauthorized.selector); - policyRegistry.updateBlocklist(policyId, true, accounts); + // Success + vm.prank(alice); + policyRegistry.updateBlocklist(policyId, true, empty); } }