diff --git a/src/interfaces/IPolicyRegistry.sol b/src/interfaces/IPolicyRegistry.sol index 454fd84f..3495b8b0 100644 --- a/src/interfaces/IPolicyRegistry.sol +++ b/src/interfaces/IPolicyRegistry.sol @@ -224,13 +224,16 @@ interface IPolicyRegistry { AUTHORIZATION QUERIES //////////////////////////////////////////////////////////////*/ - /// @notice Returns whether `account` is authorized under `policyId`. Never reverts; unknown - /// or malformed IDs collapse to empty-member-set semantics (ALLOWLIST -> false, - /// BLOCKLIST -> true). + /// @notice Returns whether `account` is authorized under `policyId`. Never reverts. /// + /// @dev A malformed ID returns false. The invert flag (bit 63) is cleared before the type + /// check, so an ID is malformed when the remaining top byte is outside `PolicyType`. + /// @dev A well-formed but unknown ID is an empty set: ALLOWLIST and UNION return false; + /// BLOCKLIST and INTERSECT return true. /// @dev Callers that store policy IDs MUST validate `policyExists(policyId)` at write time. /// @dev Invert: `isAuthorized(invertedPolicyId(id), account)` returns the negated - /// result of the base. Applies to every policy type. + /// result of the base when that base exists. Applies to every policy type. An + /// inverted unknown or malformed base returns false. /// /// @param policyId Policy to query. /// @param account Account to check. diff --git a/test/lib/BaseTest.sol b/test/lib/BaseTest.sol index f56a3c72..eeb43b0d 100644 --- a/test/lib/BaseTest.sol +++ b/test/lib/BaseTest.sol @@ -193,6 +193,9 @@ abstract contract BaseTest is Test { /// forced above the `PolicyType` enum range. View queries /// short-circuit these to the "absent" value; mutating calls /// reject via `PolicyNotFound`. + /// @dev The top byte may have bit 63 set, so clearing the invert flag can + /// leave a well-formed type. Use `_malformedBasePolicyId` when the + /// type byte must stay outside `PolicyType` after that clear. function _malformedPolicyId(uint64 seed) internal pure returns (uint64) { uint8 maxValidType = uint8(type(IPolicyRegistry.PolicyType).max); uint8 invalidRange = type(uint8).max - maxValidType; @@ -200,6 +203,16 @@ abstract contract BaseTest is Test { return (uint64(typeByte) << 56) | uint64(seed & ((1 << 56) - 1)); } + /// @notice Maps a fuzz seed to a malformed policy ID whose type byte is outside + /// `PolicyType` even after the invert flag (bit 63) is cleared. + function _malformedBasePolicyId(uint64 seed) internal pure returns (uint64) { + uint8 maxValidType = uint8(type(IPolicyRegistry.PolicyType).max); + // Type bytes 4..127: outside PolicyType, invert flag clear. + uint8 span = 127 - maxValidType; + uint8 typeByte = maxValidType + 1 + uint8(seed % span); + return (uint64(typeByte) << 56) | (seed & ((uint64(1) << 56) - 1)); + } + // ============================================================ // STRING SLOT ENCODING HELPER // ============================================================ diff --git a/test/unit/PolicyRegistry/isAuthorized.t.sol b/test/unit/PolicyRegistry/isAuthorized.t.sol index 0d037624..9a2bc5ff 100644 --- a/test/unit/PolicyRegistry/isAuthorized.t.sol +++ b/test/unit/PolicyRegistry/isAuthorized.t.sol @@ -24,11 +24,11 @@ contract PolicyRegistryIsAuthorizedTest is PolicyRegistryTest { assertTrue(policyRegistry.isAuthorized(policyId, account)); } - /// @notice Verifies isAuthorized returns false for any id whose top byte - /// is outside the PolicyType enum range. + /// @notice Verifies isAuthorized returns false for an ID whose type byte is outside + /// PolicyType after the invert flag is cleared. /// @dev Malformed-ID short-circuit returns false rather than reverting. function test_isAuthorized_success_falseForMalformedId(uint64 seed, address account) public view { - uint64 policyId = _malformedPolicyId(seed); + uint64 policyId = _malformedBasePolicyId(seed); assertFalse(policyRegistry.isAuthorized(policyId, account)); } diff --git a/test/unit/PolicyRegistry/isAuthorizedInvert.t.sol b/test/unit/PolicyRegistry/isAuthorizedInvert.t.sol index b1e251bb..0b6f8473 100644 --- a/test/unit/PolicyRegistry/isAuthorizedInvert.t.sol +++ b/test/unit/PolicyRegistry/isAuthorizedInvert.t.sol @@ -28,26 +28,43 @@ contract PolicyRegistryIsAuthorizedInvertTest is PolicyRegistryTest { // FAIL-CLOSED INVARIANT (the point of 2a) // ============================================================ - /// @notice Inverting an uncreated (unknown) base denies rather than allowing everyone. + /// @notice An uncreated ALLOWLIST denies, and its inverse denies too. function test_isAuthorized_success_invertUnknownAllowlistBaseDenies(uint56 counter, address account) public view { vm.assume(counter > 1); uint64 base = (uint64(uint8(IPolicyRegistry.PolicyType.ALLOWLIST)) << 56) | uint64(counter); + assertFalse(policyRegistry.isAuthorized(base, account)); assertFalse(policyRegistry.isAuthorized(base | INVERTED_POLICY_BIT, account)); } - /// @notice Inverting an uncreated BLOCKLIST base also denies (fail-closed) + /// @notice An uncreated BLOCKLIST authorizes, and its inverse denies. function test_isAuthorized_success_invertUnknownBlocklistBaseDenies(uint56 counter, address account) public view { vm.assume(counter > 1); uint64 base = (uint64(uint8(IPolicyRegistry.PolicyType.BLOCKLIST)) << 56) | uint64(counter); - // Sanity: the plain unknown blocklist authorizes (empty-member-set semantics)... assertTrue(policyRegistry.isAuthorized(base, account)); - // ...but its inverse must NOT become allow-everyone; the base does not exist. assertFalse(policyRegistry.isAuthorized(base | INVERTED_POLICY_BIT, account)); } - /// @notice Inverting a malformed base denies. + /// @notice An uncreated UNION denies, and its inverse denies too. + function test_isAuthorized_success_invertUnknownUnionBaseDenies(uint56 counter, address account) public view { + vm.assume(counter > 1); + uint64 base = (uint64(uint8(IPolicyRegistry.PolicyType.UNION)) << 56) | uint64(counter); + assertFalse(policyRegistry.isAuthorized(base, account)); + assertFalse(policyRegistry.isAuthorized(base | INVERTED_POLICY_BIT, account)); + } + + /// @notice An uncreated INTERSECT authorizes, and its inverse denies. + function test_isAuthorized_success_invertUnknownIntersectBaseDenies(uint56 counter, address account) public view { + vm.assume(counter > 1); + uint64 base = (uint64(uint8(IPolicyRegistry.PolicyType.INTERSECT)) << 56) | uint64(counter); + assertTrue(policyRegistry.isAuthorized(base, account)); + assertFalse(policyRegistry.isAuthorized(base | INVERTED_POLICY_BIT, account)); + } + + /// @notice A malformed base denies, and inverting it denies too. The type byte stays + /// outside PolicyType after the invert flag is cleared. function test_isAuthorized_success_invertMalformedBaseDenies(uint64 seed, address account) public view { - uint64 base = _malformedPolicyId(seed) & ~INVERTED_POLICY_BIT; + uint64 base = _malformedBasePolicyId(seed); + assertFalse(policyRegistry.isAuthorized(base, account)); assertFalse(policyRegistry.isAuthorized(base | INVERTED_POLICY_BIT, account)); }