From 9f49e2736d28be0f9d02439d346aa72ef6554d85 Mon Sep 17 00:00:00 2001 From: Leslie Fung Date: Mon, 22 Apr 2024 22:22:51 +1000 Subject: [PATCH] Add zone manager role management --- ...4-threat-model-immutable-signed-zone-v2.md | 44 ++-- .../v2/ImmutableSignedZoneV2.sol | 24 +-- .../v2/ZoneAccessControl.sol | 51 +++++ .../ZoneAccessControlEventsAndErrors.sol | 16 ++ ...utableSeaportSignedZoneV2Integration.t.sol | 5 + .../v2/IImmutableSignedZoneV2Harness.t.sol | 6 + .../v2/ImmutableSignedZoneV2.t.sol | 199 +++++++++++++++--- .../zones/immutable-signed-zone/v2/README.md | 36 ++-- 8 files changed, 309 insertions(+), 72 deletions(-) create mode 100644 contracts/trading/seaport/zones/immutable-signed-zone/v2/ZoneAccessControl.sol create mode 100644 contracts/trading/seaport/zones/immutable-signed-zone/v2/interfaces/ZoneAccessControlEventsAndErrors.sol diff --git a/audits/trading/202404-threat-model-immutable-signed-zone-v2.md b/audits/trading/202404-threat-model-immutable-signed-zone-v2.md index a7e444c1..2ffd2862 100644 --- a/audits/trading/202404-threat-model-immutable-signed-zone-v2.md +++ b/audits/trading/202404-threat-model-immutable-signed-zone-v2.md @@ -116,21 +116,22 @@ forge inspect ImmutableSignedZoneV2 --pretty methods Functions that *change* state: -| Name | Function Selector | Access Control | -| -------------------------------- | ----------------- | --------------- | -| `addSigner(address)` | eb12d61e | `DEFAULT_ADMIN` | -| `grantRole(bytes32,address)` | 2f2ff15d | Role admin | -| `removeSigner(address)` | 0e316ab7 | `DEFAULT_ADMIN` | -| `renounceRole(bytes32,address)` | 36568abe | `msg.sender` | -| `revokeRole(bytes32,address)` | d547741f | Role admin | -| `updateAPIEndpoint(string)` | 297234d7 | `DEFAULT_ADMIN` | -| `updateDocumentationURI(string)` | 0a904f08 | `DEFAULT_ADMIN` | +| Name | Function Selector | Access Control | +| -------------------------------- | ----------------- | ------------------- | +| `addSigner(address)` | eb12d61e | `ZONE_MANAGER_ROLE` | +| `grantRole(bytes32,address)` | 2f2ff15d | Role admin | +| `removeSigner(address)` | 0e316ab7 | `ZONE_MANAGER_ROLE` | +| `renounceRole(bytes32,address)` | 36568abe | `msg.sender` | +| `revokeRole(bytes32,address)` | d547741f | Role admin | +| `updateAPIEndpoint(string)` | 297234d7 | `ZONE_MANAGER_ROLE` | +| `updateDocumentationURI(string)` | 0a904f08 | `ZONE_MANAGER_ROLE` | Functions that *do not change* state: | Name | Function Selector | | -------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------- | | `DEFAULT_ADMIN_ROLE()` | a217fddf | +| `ZONE_MANAGER_ROLE()` | c6e95ae7 | | `getRoleAdmin(bytes32)` | 248a9ca3 | | `getRoleMember(bytes32,uint256)` | 9010d07c | | `getRoleMemberCount(bytes32)` | ca15c873 | @@ -148,13 +149,22 @@ Accounts with administrative privileges could be used by attackers to facilitate This role is granted to the `owner` specified in the `constructor` of the contract. Accounts with the `DEFAULT_ADMIN` account can: -* Grant can grant administrator roles to any account, including the `DEFAULT_ADMIN` role -* Revoke `DEFAULT_ADMIN` role from any account -* Renounce the `DEFAULT_ADMIN` role for itself, possibly leading to no administrators and loss of control of the contract +* Grant administrator roles to any account, including the `DEFAULT_ADMIN` role +* Revoke administrator roles from any account, including the `DEFAULT_ADMIN` role + * The `DEFAULT_ADMIN` role cannot be revoked from an account if it the only account with the `DEFAULT_ADMIN` role +* Renounce the `DEFAULT_ADMIN` role for itself, unless it is the only account with the `DEFAULT_ADMIN` role + +Exploiting this attack surface requires compromising an account with `DEFAULT_ADMIN` role. + +#### Accounts with `ZONE_MANAGER` role on ImmutableSignedZoneV2 contract + +An account with `ZONE_MANAGER` role can: + * Update API endpoint and documentation URI (no impact to Immutable system as these values are not utilised) * Add and remove SIP-7 signers, letting them control the result of order validation +* Renounce the `ZONE_MANAGER` role for itself -Exploiting this attack surface requires compromising an account with `DEFAULT_ADMIN` role. +Exploiting this attack surface requires compromising an account with `ZONE_MANAGER` role. ### SIP-7 Signers on the ImmutableSignedZoneV2 contract @@ -196,7 +206,13 @@ This section outlines possible attacks against the attack surfaces by the attack ### `DEFAULT_ADMIN` Role Account Compromise -**Detection:** Monitoring role change events and SIP-7 signer events. +**Detection:** Monitoring role change events. + +The mitigation is to assume that the role will be operated by multi-signature addresses such that an attacker would need to compromise multiple signers simultaneously. As such, even if some keys are compromised due to the Spear Phishing Attacker or the Insider Attacker, the administrative actions will not be able to be executed as a threshold number of keys will not be available. + +### `ZONE_MANAGER` Role Account Compromise + +**Detection:** Monitoring SIP-7 signer events. The mitigation is to assume that the role will be operated by multi-signature addresses such that an attacker would need to compromise multiple signers simultaneously. As such, even if some keys are compromised due to the Spear Phishing Attacker or the Insider Attacker, the administrative actions will not be able to be executed as a threshold number of keys will not be available. diff --git a/contracts/trading/seaport/zones/immutable-signed-zone/v2/ImmutableSignedZoneV2.sol b/contracts/trading/seaport/zones/immutable-signed-zone/v2/ImmutableSignedZoneV2.sol index a49906f0..b22dd795 100644 --- a/contracts/trading/seaport/zones/immutable-signed-zone/v2/ImmutableSignedZoneV2.sol +++ b/contracts/trading/seaport/zones/immutable-signed-zone/v2/ImmutableSignedZoneV2.sol @@ -4,13 +4,14 @@ // solhint-disable-next-line compiler-version pragma solidity ^0.8.20; -import {ZoneInterface} from "seaport/contracts/interfaces/ZoneInterface.sol"; -import {ZoneParameters, Schema, ReceivedItem} from "seaport-types/src/lib/ConsiderationStructs.sol"; import {AccessControlEnumerable} from "openzeppelin-contracts-5.0.2/access/extensions/AccessControlEnumerable.sol"; import {ECDSA} from "openzeppelin-contracts-5.0.2/utils/cryptography/ECDSA.sol"; import {MessageHashUtils} from "openzeppelin-contracts-5.0.2/utils/cryptography/MessageHashUtils.sol"; import {ERC165} from "openzeppelin-contracts-5.0.2/utils/introspection/ERC165.sol"; import {Math} from "openzeppelin-contracts-5.0.2/utils/math/Math.sol"; +import {ZoneInterface} from "seaport/contracts/interfaces/ZoneInterface.sol"; +import {ZoneParameters, Schema, ReceivedItem} from "seaport-types/src/lib/ConsiderationStructs.sol"; +import {ZoneAccessControl} from "./ZoneAccessControl.sol"; import {SIP5Interface} from "./interfaces/SIP5Interface.sol"; import {SIP6Interface} from "./interfaces/SIP6Interface.sol"; import {SIP7Interface} from "./interfaces/SIP7Interface.sol"; @@ -24,11 +25,11 @@ import {SIP7Interface} from "./interfaces/SIP7Interface.sol"; */ contract ImmutableSignedZoneV2 is ERC165, + ZoneAccessControl, ZoneInterface, SIP5Interface, SIP6Interface, - SIP7Interface, - AccessControlEnumerable + SIP7Interface { /// @dev The EIP-712 domain type hash. bytes32 private constant _EIP_712_DOMAIN_TYPEHASH = keccak256( @@ -82,7 +83,9 @@ contract ImmutableSignedZoneV2 is * @param owner The address of the owner of this contract. Specified in the * constructor to be CREATE2 / CREATE3 compatible. */ - constructor(string memory zoneName, string memory apiEndpoint, string memory documentationURI, address owner) { + constructor(string memory zoneName, string memory apiEndpoint, string memory documentationURI, address owner) + ZoneAccessControl(owner) + { // Set the zone name. _ZONE_NAME = zoneName; @@ -100,9 +103,6 @@ contract ImmutableSignedZoneV2 is // Emit an event to signal a SIP-5 contract has been deployed. emit SeaportCompatibleContractDeployed(); - - // Grant admin role to the specified owner. - _grantRole(DEFAULT_ADMIN_ROLE, owner); } /** @@ -110,7 +110,7 @@ contract ImmutableSignedZoneV2 is * * @param signer The new signer address to add. */ - function addSigner(address signer) external override onlyRole(DEFAULT_ADMIN_ROLE) { + function addSigner(address signer) external override onlyRole(ZONE_MANAGER_ROLE) { // Do not allow the zero address to be added as a signer. if (signer == address(0)) { revert SignerCannotBeZeroAddress(); @@ -140,7 +140,7 @@ contract ImmutableSignedZoneV2 is * * @param signer The signer address to remove. */ - function removeSigner(address signer) external override onlyRole(DEFAULT_ADMIN_ROLE) { + function removeSigner(address signer) external override onlyRole(ZONE_MANAGER_ROLE) { // Revert if the signer is not active. if (!_signers[signer].active) { revert SignerNotActive(signer); @@ -158,7 +158,7 @@ contract ImmutableSignedZoneV2 is * * @param newApiEndpoint The new API endpoint. */ - function updateAPIEndpoint(string calldata newApiEndpoint) external override onlyRole(DEFAULT_ADMIN_ROLE) { + function updateAPIEndpoint(string calldata newApiEndpoint) external override onlyRole(ZONE_MANAGER_ROLE) { _apiEndpoint = newApiEndpoint; } @@ -170,7 +170,7 @@ contract ImmutableSignedZoneV2 is function updateDocumentationURI(string calldata newDocumentationURI) external override - onlyRole(DEFAULT_ADMIN_ROLE) + onlyRole(ZONE_MANAGER_ROLE) { _documentationURI = newDocumentationURI; } diff --git a/contracts/trading/seaport/zones/immutable-signed-zone/v2/ZoneAccessControl.sol b/contracts/trading/seaport/zones/immutable-signed-zone/v2/ZoneAccessControl.sol new file mode 100644 index 00000000..19a5c50b --- /dev/null +++ b/contracts/trading/seaport/zones/immutable-signed-zone/v2/ZoneAccessControl.sol @@ -0,0 +1,51 @@ +// Copyright (c) Immutable Pty Ltd 2018 - 2024 +// SPDX-License-Identifier: Apache-2 + +// solhint-disable-next-line compiler-version +pragma solidity ^0.8.20; + +import {AccessControl} from "openzeppelin-contracts-5.0.2/access/AccessControl.sol"; +import {IAccessControl} from "openzeppelin-contracts-5.0.2/access/IAccessControl.sol"; +import {AccessControlEnumerable} from "openzeppelin-contracts-5.0.2/access/extensions/AccessControlEnumerable.sol"; +import {ZoneAccessControlEventsAndErrors} from + "../../../../../../contracts/trading/seaport/zones/immutable-signed-zone/v2/interfaces/ZoneAccessControlEventsAndErrors.sol"; + +/** + * @notice ZoneAccessControl encapsulates access control functionality for the zone. + */ +abstract contract ZoneAccessControl is AccessControlEnumerable, ZoneAccessControlEventsAndErrors { + /// @dev Zone manager manages the zone. + bytes32 public constant ZONE_MANAGER_ROLE = bytes32("ZONE_MANAGER"); + + /** + * @notice Constructor to setup initial default admin. + * + * @param owner The address to assign the DEFAULT_ADMIN_ROLE. + */ + constructor(address owner) { + // Grant admin role to the specified owner. + _grantRole(DEFAULT_ADMIN_ROLE, owner); + } + + /** + * @inheritdoc AccessControl + */ + function revokeRole(bytes32 role, address account) public override(AccessControl, IAccessControl) onlyRole(getRoleAdmin(role)) { + super.revokeRole(role, account); + + if (role == DEFAULT_ADMIN_ROLE && super.getRoleMemberCount(DEFAULT_ADMIN_ROLE) == 0) { + revert LastDefaultAdminRole(account); + } + } + + /** + * @inheritdoc AccessControl + */ + function renounceRole(bytes32 role, address callerConfirmation) public override(AccessControl, IAccessControl) { + super.renounceRole(role, callerConfirmation); + + if (role == DEFAULT_ADMIN_ROLE && super.getRoleMemberCount(DEFAULT_ADMIN_ROLE) == 0) { + revert LastDefaultAdminRole(callerConfirmation); + } + } +} diff --git a/contracts/trading/seaport/zones/immutable-signed-zone/v2/interfaces/ZoneAccessControlEventsAndErrors.sol b/contracts/trading/seaport/zones/immutable-signed-zone/v2/interfaces/ZoneAccessControlEventsAndErrors.sol new file mode 100644 index 00000000..cf86ab3b --- /dev/null +++ b/contracts/trading/seaport/zones/immutable-signed-zone/v2/interfaces/ZoneAccessControlEventsAndErrors.sol @@ -0,0 +1,16 @@ +// Copyright (c) Immutable Pty Ltd 2018 - 2024 +// SPDX-License-Identifier: Apache-2 + +// solhint-disable compiler-version +pragma solidity ^0.8.17; + +/** + * @notice ZoneAccessControlEventsAndErrors contains errors and events + * related to zone access control. + */ +interface ZoneAccessControlEventsAndErrors { + /** + * @dev Revert with an error if revoking last DEFAULT_ADMIN_ROLE. + */ + error LastDefaultAdminRole(address account); +} diff --git a/test/trading/seaport/ImmutableSeaportSignedZoneV2Integration.t.sol b/test/trading/seaport/ImmutableSeaportSignedZoneV2Integration.t.sol index aa65560c..923560d9 100644 --- a/test/trading/seaport/ImmutableSeaportSignedZoneV2Integration.t.sol +++ b/test/trading/seaport/ImmutableSeaportSignedZoneV2Integration.t.sol @@ -41,6 +41,7 @@ contract ImmutableSeaportSignedZoneV2IntegrationTest is Test, SigningTestHelper "./foundry-out/ImmutableSignedZoneV2Harness.t.sol/ImmutableSignedZoneV2Harness.json"; address private immutable OWNER = makeAddr("owner"); + address private immutable ZONE_MANAGER = makeAddr("zone_manager"); address private immutable SIGNER; uint256 private immutable SIGNER_PRIVATE_KEY; address private immutable FULFILLER = makeAddr("fulfiller"); @@ -98,6 +99,10 @@ contract ImmutableSeaportSignedZoneV2IntegrationTest is Test, SigningTestHelper ) ); vm.prank(OWNER); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + vm.prank(OWNER); + zone.grantRole(managerRole, ZONE_MANAGER); + vm.prank(ZONE_MANAGER); zone.addSigner(SIGNER); // seaport diff --git a/test/trading/seaport/zones/immutable-signed-zone/v2/IImmutableSignedZoneV2Harness.t.sol b/test/trading/seaport/zones/immutable-signed-zone/v2/IImmutableSignedZoneV2Harness.t.sol index 8a2fca35..85b5e699 100644 --- a/test/trading/seaport/zones/immutable-signed-zone/v2/IImmutableSignedZoneV2Harness.t.sol +++ b/test/trading/seaport/zones/immutable-signed-zone/v2/IImmutableSignedZoneV2Harness.t.sol @@ -12,6 +12,12 @@ import {SIP7Interface} from // solhint-disable func-name-mixedcase interface IImmutableSignedZoneV2Harness is ZoneInterface, SIP7Interface { + function grantRole(bytes32 role, address account) external; + + function DEFAULT_ADMIN_ROLE() external view returns (bytes32); + + function ZONE_MANAGER_ROLE() external view returns (bytes32); + function exposed_domainSeparator() external view returns (bytes32); function exposed_deriveDomainSeparator() external view returns (bytes32 domainSeparator); diff --git a/test/trading/seaport/zones/immutable-signed-zone/v2/ImmutableSignedZoneV2.t.sol b/test/trading/seaport/zones/immutable-signed-zone/v2/ImmutableSignedZoneV2.t.sol index 0dd27a8d..2cb42bf2 100644 --- a/test/trading/seaport/zones/immutable-signed-zone/v2/ImmutableSignedZoneV2.t.sol +++ b/test/trading/seaport/zones/immutable-signed-zone/v2/ImmutableSignedZoneV2.t.sol @@ -17,6 +17,8 @@ import {SIP6EventsAndErrors} from "../../../../../../contracts/trading/seaport/zones/immutable-signed-zone/v2/interfaces/SIP6EventsAndErrors.sol"; import {SIP7EventsAndErrors} from "../../../../../../contracts/trading/seaport/zones/immutable-signed-zone/v2/interfaces/SIP7EventsAndErrors.sol"; +import {ZoneAccessControlEventsAndErrors} from + "../../../../../../contracts/trading/seaport/zones/immutable-signed-zone/v2/interfaces/ZoneAccessControlEventsAndErrors.sol"; import {SigningTestHelper} from "../../../utils/SigningTestHelper.t.sol"; import {ImmutableSignedZoneV2Harness} from "./ImmutableSignedZoneV2Harness.t.sol"; @@ -25,6 +27,7 @@ import {ImmutableSignedZoneV2Harness} from "./ImmutableSignedZoneV2Harness.t.sol contract ImmutableSignedZoneV2Test is Test, SigningTestHelper, + ZoneAccessControlEventsAndErrors, SIP5EventsAndErrors, SIP6EventsAndErrors, SIP7EventsAndErrors @@ -39,6 +42,7 @@ contract ImmutableSignedZoneV2Test is // OpenZeppelin v5 access/IAccessControl.sol error AccessControlUnauthorizedAccount(address account, bytes32 neededRole); + error AccessControlBadConfirmation(); constructor() { (SIGNER, SIGNER_PRIVATE_KEY) = makeAddrAndKey("signer"); @@ -69,24 +73,142 @@ contract ImmutableSignedZoneV2Test is ); } + /* grantRole */ + + function test_grantRole_revertsIfCalledByNonAdminRole() public { + ImmutableSignedZoneV2 zone = _newZone(OWNER); + address nonAdmin = makeAddr("non_admin"); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + vm.expectRevert( + abi.encodeWithSelector(AccessControlUnauthorizedAccount.selector, nonAdmin, zone.DEFAULT_ADMIN_ROLE()) + ); + vm.prank(nonAdmin); + zone.grantRole(managerRole, OWNER); + } + + function test_grantRole_grantsIfCalledByAdminRole() public { + ImmutableSignedZoneV2 zone = _newZone(OWNER); + address newManager = makeAddr("new_manager"); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + vm.prank(OWNER); + zone.grantRole(managerRole, newManager); + bool newManagerHasManagerRole = zone.hasRole(managerRole, newManager); + assertTrue(newManagerHasManagerRole); + } + + /* revokeRole */ + + function test_revokeRole_revertsIfCalledByNonAdminRole() public { + ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + address managerOne = makeAddr("manager_one"); + address managerTwo = makeAddr("manager_two"); + vm.prank(OWNER); + zone.grantRole(managerRole, managerOne); + vm.prank(OWNER); + zone.grantRole(managerRole, managerTwo); + vm.expectRevert( + abi.encodeWithSelector(AccessControlUnauthorizedAccount.selector, managerOne, zone.DEFAULT_ADMIN_ROLE()) + ); + vm.prank(managerOne); + zone.revokeRole(managerRole, managerTwo); + } + + function test_revokeRole_revertsIfRevokingLastDefaultAdminRole() public { + ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 adminRole = zone.DEFAULT_ADMIN_ROLE(); + vm.expectRevert(abi.encodeWithSelector(LastDefaultAdminRole.selector, OWNER)); + vm.prank(OWNER); + zone.revokeRole(adminRole, OWNER); + } + + function test_revokeRole_revokesIfRevokingNonLastDefaultAdminRole() public { + ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 adminRole = zone.DEFAULT_ADMIN_ROLE(); + address newAdmin = makeAddr("new_admin"); + vm.prank(OWNER); + zone.grantRole(adminRole, newAdmin); + vm.prank(OWNER); + zone.revokeRole(adminRole, OWNER); + bool ownerHasAdminRole = zone.hasRole(adminRole, OWNER); + assertFalse(ownerHasAdminRole); + } + + function test_revokeRole_revokesIfRevokingLastNonDefaultAdminRole() public { + ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + vm.prank(OWNER); + zone.grantRole(managerRole, OWNER); + vm.prank(OWNER); + zone.revokeRole(managerRole, OWNER); + bool ownerHasManagerRole = zone.hasRole(managerRole, OWNER); + uint256 managerCount = zone.getRoleMemberCount(managerRole); + assertFalse(ownerHasManagerRole); + assertEq(managerCount, 0); + } + + /* renounceRole */ + + function test_renounceRole_revertsIfCallerDoesNotMatchCallerConfirmationAddress() public { + ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + address newManager = makeAddr("new_manager"); + vm.prank(OWNER); + zone.grantRole(managerRole, newManager); + vm.expectRevert(abi.encodeWithSelector(AccessControlBadConfirmation.selector)); + vm.prank(newManager); + zone.renounceRole(managerRole, makeAddr("random")); + } + + function test_renounceRole_revertsIfRenouncingLastDefaultAdminRole() public { + ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 adminRole = zone.DEFAULT_ADMIN_ROLE(); + vm.expectRevert(abi.encodeWithSelector(LastDefaultAdminRole.selector, OWNER)); + vm.prank(OWNER); + zone.renounceRole(adminRole, OWNER); + } + + function test_renounceRole_revokesIfRenouncingNonLastDefaultAdminRole() public { + ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 adminRole = zone.DEFAULT_ADMIN_ROLE(); + address newAdmin = makeAddr("new_admin"); + vm.prank(OWNER); + zone.grantRole(adminRole, newAdmin); + vm.prank(OWNER); + zone.renounceRole(adminRole, OWNER); + bool ownerHasAdminRole = zone.hasRole(adminRole, OWNER); + assertFalse(ownerHasAdminRole); + } + + function test_renounceRole_revokesIfRenouncingLastNonDefaultAdminRole() public { + ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + vm.prank(OWNER); + zone.grantRole(managerRole, OWNER); + vm.prank(OWNER); + zone.renounceRole(managerRole, OWNER); + bool ownerHasManagerRole = zone.hasRole(managerRole, OWNER); + uint256 managerCount = zone.getRoleMemberCount(managerRole); + assertFalse(ownerHasManagerRole); + assertEq(managerCount, 0); + } + /* addSigner */ - function test_addSigner_revertsIfCalledByNonAdminRole() public { + function test_addSigner_revertsIfCalledByNonZoneManagerRole() public { ImmutableSignedZoneV2 zone = _newZone(OWNER); - address nonAdminAccount = makeAddr("non_admin"); vm.expectRevert( - abi.encodeWithSelector( - AccessControlUnauthorizedAccount.selector, - nonAdminAccount, - zone.DEFAULT_ADMIN_ROLE() - ) + abi.encodeWithSelector(AccessControlUnauthorizedAccount.selector, OWNER, zone.ZONE_MANAGER_ROLE()) ); - vm.prank(nonAdminAccount); + vm.prank(OWNER); zone.addSigner(makeAddr("signer_to_add")); } function test_addSigner_revertsIfSignerIsTheZeroAddress() public { ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + vm.prank(OWNER); + zone.grantRole(managerRole, OWNER); vm.expectRevert(abi.encodeWithSelector(SignerCannotBeZeroAddress.selector)); vm.prank(OWNER); zone.addSigner(address(0)); @@ -95,6 +217,9 @@ contract ImmutableSignedZoneV2Test is function test_addSigner_emitsSignerAddedEvent() public { address signerToAdd = makeAddr("signer_to_add"); ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + vm.prank(OWNER); + zone.grantRole(managerRole, OWNER); vm.expectEmit(address(zone)); emit SignerAdded(signerToAdd); vm.prank(OWNER); @@ -104,6 +229,9 @@ contract ImmutableSignedZoneV2Test is function test_addSigner_revertsIfSignerAlreadyActive() public { address signerToAdd = makeAddr("signer_to_add"); ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + vm.prank(OWNER); + zone.grantRole(managerRole, OWNER); vm.prank(OWNER); zone.addSigner(signerToAdd); vm.expectRevert(abi.encodeWithSelector(SignerAlreadyActive.selector, signerToAdd)); @@ -114,6 +242,9 @@ contract ImmutableSignedZoneV2Test is function test_addSigner_revertsIfSignerWasPreviouslyActive() public { address signerToAdd = makeAddr("signer_to_add"); ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + vm.prank(OWNER); + zone.grantRole(managerRole, OWNER); vm.prank(OWNER); zone.addSigner(signerToAdd); vm.prank(OWNER); @@ -125,23 +256,21 @@ contract ImmutableSignedZoneV2Test is /* removeSigner */ - function test_removeSigner_revertsIfCalledByNonAdminRole() public { + function test_removeSigner_revertsIfCalledByNonZoneManagerRole() public { ImmutableSignedZoneV2 zone = _newZone(OWNER); - address nonAdminAccount = makeAddr("non_admin"); vm.expectRevert( - abi.encodeWithSelector( - AccessControlUnauthorizedAccount.selector, - nonAdminAccount, - zone.DEFAULT_ADMIN_ROLE() - ) + abi.encodeWithSelector(AccessControlUnauthorizedAccount.selector, OWNER, zone.ZONE_MANAGER_ROLE()) ); - vm.prank(nonAdminAccount); + vm.prank(OWNER); zone.removeSigner(makeAddr("signer_to_remove")); } function test_removeSigner_revertsIfSignerNotActive() public { address signerToRemove = makeAddr("signer_to_remove"); ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + vm.prank(OWNER); + zone.grantRole(managerRole, OWNER); vm.expectRevert(abi.encodeWithSelector(SignerNotActive.selector, signerToRemove)); vm.prank(OWNER); zone.removeSigner(signerToRemove); @@ -150,6 +279,9 @@ contract ImmutableSignedZoneV2Test is function test_removeSigner_emitsSignerRemovedEvent() public { address signerToRemove = makeAddr("signer_to_remove"); ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + vm.prank(OWNER); + zone.grantRole(managerRole, OWNER); vm.prank(OWNER); zone.addSigner(signerToRemove); vm.expectEmit(address(zone)); @@ -160,22 +292,20 @@ contract ImmutableSignedZoneV2Test is /* updateAPIEndpoint */ - function test_updateAPIEndpoint_revertsIfCalledByNonAdminRole() public { + function test_updateAPIEndpoint_revertsIfCalledByNonZoneManagerRole() public { ImmutableSignedZoneV2 zone = _newZone(OWNER); - address nonAdminAccount = makeAddr("non_admin"); vm.expectRevert( - abi.encodeWithSelector( - AccessControlUnauthorizedAccount.selector, - nonAdminAccount, - zone.DEFAULT_ADMIN_ROLE() - ) + abi.encodeWithSelector(AccessControlUnauthorizedAccount.selector, OWNER, zone.ZONE_MANAGER_ROLE()) ); - vm.prank(nonAdminAccount); + vm.prank(OWNER); zone.updateAPIEndpoint("https://www.new-immutable.com"); } - function test_updateAPIEndpoint_updatesAPIEndpointIfCalledByAdminRole() public { + function test_updateAPIEndpoint_updatesAPIEndpointIfCalledByZoneManagerRole() public { ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + vm.prank(OWNER); + zone.grantRole(managerRole, OWNER); string memory expectedApiEndpoint = "https://www.new-immutable.com"; vm.prank(OWNER); zone.updateAPIEndpoint(expectedApiEndpoint); @@ -186,22 +316,20 @@ contract ImmutableSignedZoneV2Test is /* updateDocumentationURI */ - function test_updateDocumentationURI_revertsIfCalledByNonAdminRole() public { + function test_updateDocumentationURI_revertsIfCalledByNonZoneManagerRole() public { ImmutableSignedZoneV2 zone = _newZone(OWNER); - address nonAdminAccount = makeAddr("non_admin"); vm.expectRevert( - abi.encodeWithSelector( - AccessControlUnauthorizedAccount.selector, - nonAdminAccount, - zone.DEFAULT_ADMIN_ROLE() - ) + abi.encodeWithSelector(AccessControlUnauthorizedAccount.selector, OWNER, zone.ZONE_MANAGER_ROLE()) ); - vm.prank(nonAdminAccount); + vm.prank(OWNER); zone.updateDocumentationURI("https://www.new-immutable.com/docs"); } - function test_updateDocumentationURI_updatesDocumentationURIIfCalledByAdminRole() public { + function test_updateDocumentationURI_updatesDocumentationURIIfCalledByZoneManagerRole() public { ImmutableSignedZoneV2 zone = _newZone(OWNER); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + vm.prank(OWNER); + zone.grantRole(managerRole, OWNER); string memory expectedDocumentationURI = "https://www.new-immutable.com/docs"; vm.prank(OWNER); zone.updateDocumentationURI(expectedDocumentationURI); @@ -430,6 +558,9 @@ contract ImmutableSignedZoneV2Test is function test_validateOrder_returnsMagicValueOnSuccessfulValidation() public { ImmutableSignedZoneV2Harness zone = _newZoneHarness(OWNER); + bytes32 managerRole = zone.ZONE_MANAGER_ROLE(); + vm.prank(OWNER); + zone.grantRole(managerRole, OWNER); vm.prank(OWNER); zone.addSigner(SIGNER); diff --git a/test/trading/seaport/zones/immutable-signed-zone/v2/README.md b/test/trading/seaport/zones/immutable-signed-zone/v2/README.md index 2096e427..a5dc1cc8 100644 --- a/test/trading/seaport/zones/immutable-signed-zone/v2/README.md +++ b/test/trading/seaport/zones/immutable-signed-zone/v2/README.md @@ -11,18 +11,30 @@ Constructor tests: Control function tests: -| Test name | Description | Happy Case | Implemented | -| -------------------------------------------------------------- | ------------------------------------------ | ---------- | ----------- | -| `test_addSigner_revertsIfCalledByNonAdminRole` | Add signer without authorization. | No | Yes | -| `test_addSigner_revertsIfSignerIsTheZeroAddress` | Add zero address as signer. | No | Yes | -| `test_addSigner_emitsSignerAddedEvent` | Emits `SignerAdded` event. | Yes | Yes | -| `test_addSigner_revertsIfSignerAlreadyActive` | Add an already active signer. | No | Yes | -| `test_addSigner_revertsIfSignerWasPreviouslyActive` | Add a previously active signer. | No | Yes | -| `test_removeSigner_revertsIfCalledByNonAdminRole` | Remove signer without authorization. | Yes | Yes | -| `test_removeSigner_revertsIfSignerNotActive` | Remove a signer that is not active. | No | Yes | -| `test_removeSigner_emitsSignerRemovedEvent` | Emits `SignerRemoved` event. | Yes | Yes | -| `test_updateAPIEndpoint_revertsIfCalledByNonAdminRole` | Update API endpoint without authorization. | No | Yes | -| `test_updateAPIEndpoint_updatesAPIEndpointIfCalledByAdminRole` | Update API endpoint with authorization. | Yes | Yes | +| Test name | Description | Happy Case | Implemented | +| ------------------------------------------------------------------------------ | ----------------------------------------------- | ---------- | ----------- | +| `test_grantRole_revertsIfCalledByNonAdminRole` | Grant role without authorization | No | Yes | +| `test_grantRole_grantsIfCalledByAdminRole` | Grant role with authorization | Yes | Yes | +| `test_revokeRole_revertsIfCalledByNonAdminRole` | Revoke role without authorization | No | Yes | +| `test_revokeRole_revertsIfRevokingLastDefaultAdminRole` | Revoke last `DEFAULT_ADMIN_ROLE`. | No | Yes | +| `test_revokeRole_revokesIfRevokingNonLastDefaultAdminRole` | Revoke non-last `DEFAULT_ADMIN_ROLE`. | Yes | Yes | +| `test_revokeRole_revokesIfRevokingLastNonDefaultAdminRole` | Revoke last non-`DEFAULT_ADMIN_ROLE`. | Yes | Yes | +| `test_renounceRole_revertsIfCallerDoesNotMatchCallerConfirmationAddress` | Renounce role without authorization | No | Yes | +| `test_renounceRole_revertsIfRenouncingLastDefaultAdminRole` | Renounce last `DEFAULT_ADMIN_ROLE`. | No | Yes | +| `test_renounceRole_revokesIfRenouncingNonLastDefaultAdminRole` | Renounce non-last `DEFAULT_ADMIN_ROLE`. | Yes | Yes | +| `test_renounceRole_revokesIfRenouncingLastNonDefaultAdminRole` | Renounce last non-`DEFAULT_ADMIN_ROLE`. | Yes | Yes | +| `test_addSigner_revertsIfCalledByNonZoneManagerRole` | Add signer without authorization. | No | Yes | +| `test_addSigner_revertsIfSignerIsTheZeroAddress` | Add zero address as signer. | No | Yes | +| `test_addSigner_emitsSignerAddedEvent` | Emits `SignerAdded` event. | Yes | Yes | +| `test_addSigner_revertsIfSignerAlreadyActive` | Add an already active signer. | No | Yes | +| `test_addSigner_revertsIfSignerWasPreviouslyActive` | Add a previously active signer. | No | Yes | +| `test_removeSigner_revertsIfCalledByNonZoneManagerRole` | Remove signer without authorization. | Yes | Yes | +| `test_removeSigner_revertsIfSignerNotActive` | Remove a signer that is not active. | No | Yes | +| `test_removeSigner_emitsSignerRemovedEvent` | Emits `SignerRemoved` event. | Yes | Yes | +| `test_updateAPIEndpoint_revertsIfCalledByNonZoneManagerRole` | Update API endpoint without authorization. | No | Yes | +| `test_updateAPIEndpoint_updatesAPIEndpointIfCalledByZoneManagerRole` | Update API endpoint with authorization. | Yes | Yes | +| `test_updateDocumentationURI_revertsIfCalledByNonZoneManagerRole` | Update documentation URI without authorization. | No | Yes | +| `test_updateDocumentationURI_updatesDocumentationURIIfCalledByZoneManagerRole` | Update documentation URI with authorization. | Yes | Yes | Operational function tests: