From e04439e0091a91d78d8c0e8335e40349e44d9747 Mon Sep 17 00:00:00 2001 From: Satyajeet Kolhapure <77279246+satyajeetkolhapure@users.noreply.github.com> Date: Wed, 25 Oct 2023 17:03:56 +0100 Subject: [PATCH] fix: As an issuer, I can update another issuer's schema context (#319) Co-authored-by: Alain Nicolas Co-authored-by: Satyajeet Kolhapure --- contracts/src/PortalRegistry.sol | 2 + contracts/src/SchemaRegistry.sol | 54 ++++++++++++++++++++- contracts/test/PortalRegistry.t.sol | 4 ++ contracts/test/SchemaRegistry.t.sol | 54 ++++++++++++++++++--- contracts/test/mocks/SchemaRegistryMock.sol | 2 + 5 files changed, 107 insertions(+), 9 deletions(-) diff --git a/contracts/src/PortalRegistry.sol b/contracts/src/PortalRegistry.sol index d0d008aa..78f3c33d 100644 --- a/contracts/src/PortalRegistry.sol +++ b/contracts/src/PortalRegistry.sol @@ -6,6 +6,7 @@ import { OwnableUpgradeable } from "openzeppelin-contracts-upgradeable/contracts import { ERC165CheckerUpgradeable } from "openzeppelin-contracts-upgradeable/contracts/utils/introspection/ERC165CheckerUpgradeable.sol"; import { AbstractPortal } from "./interface/AbstractPortal.sol"; import { DefaultPortal } from "./DefaultPortal.sol"; +import { SchemaRegistry } from "./SchemaRegistry.sol"; import { Portal } from "./types/Structs.sol"; import { IRouter } from "./interface/IRouter.sol"; import { IPortal } from "./interface/IPortal.sol"; @@ -81,6 +82,7 @@ contract PortalRegistry is OwnableUpgradeable { */ function removeIssuer(address issuer) public onlyOwner { issuers[issuer] = false; + SchemaRegistry(router.getSchemaRegistry()).updateMatchingSchemaIssuers(issuer, msg.sender); } /** diff --git a/contracts/src/SchemaRegistry.sol b/contracts/src/SchemaRegistry.sol index 22a502c2..5e118b84 100644 --- a/contracts/src/SchemaRegistry.sol +++ b/contracts/src/SchemaRegistry.sol @@ -15,6 +15,8 @@ contract SchemaRegistry is OwnableUpgradeable { IRouter public router; /// @dev The list of Schemas, accessed by their ID mapping(bytes32 id => Schema schema) private schemas; + /// @dev Associates a Schema ID with the address of the Issuer who created it + mapping(bytes32 id => address issuer) private schemasIssuers; /// @dev The list of Schema IDs bytes32[] public schemaIds; @@ -22,6 +24,12 @@ contract SchemaRegistry is OwnableUpgradeable { error RouterInvalid(); /// @notice Error thrown when a non-issuer tries to call a method that can only be called by an issuer error OnlyIssuer(); + /// @notice Error thrown when any address which is not a portal registry tries to call a method + error OnlyPortalRegistry(); + /// @notice Error thrown when a non-assigned issuer tries to call a method that can only be called by an assigned issuer + error OnlyAssignedIssuer(); + /// @notice Error thrown when an invalid Issuer address is given + error IssuerInvalid(); /// @notice Error thrown when an identical Schema was already registered error SchemaAlreadyExists(); /// @notice Error thrown when attempting to add a Schema without a name @@ -56,6 +64,16 @@ contract SchemaRegistry is OwnableUpgradeable { _; } + /** + * @notice Checks if the caller is the portal registry. + * @param caller the caller address + */ + modifier onlyPortalRegistry(address caller) { + bool isCallerPortalRegistry = router.getPortalRegistry() == caller; + if (!isCallerPortalRegistry) revert OnlyPortalRegistry(); + _; + } + /** * @notice Changes the address for the Router * @dev Only the registry owner can call this method @@ -65,6 +83,34 @@ contract SchemaRegistry is OwnableUpgradeable { router = IRouter(_router); } + /** + * @notice Updates a given Schema's Issuer + * @param schemaId the Schema's ID + * @param issuer the address of the issuer who created the given Schema + * @dev Updates issuer for the given schemaId in the `schemaIssuers` mapping + * The issuer must already be registered as an Issuer via the `PortalRegistry` + */ + function updateSchemaIssuer(bytes32 schemaId, address issuer) public onlyOwner { + if (!isRegistered(schemaId)) revert SchemaNotRegistered(); + if (issuer == address(0)) revert IssuerInvalid(); + schemasIssuers[schemaId] = issuer; + } + + /** + * @notice Updates issuer address for all schemas associated with old issuer address + * @param oldIssuer the address of old issuer + * @param newIssuer the address of new issuer + * @dev Finds all the schemaIds associated with old issuer and updates the mapping `schemasIssuers` + * for schemaIds found with new issuer + */ + function updateMatchingSchemaIssuers(address oldIssuer, address newIssuer) public onlyPortalRegistry(msg.sender) { + for (uint256 i = 0; i < schemaIds.length; i++) { + if (schemasIssuers[schemaIds[i]] == oldIssuer) { + schemasIssuers[schemaIds[i]] = newIssuer; + } + } + } + /** * Generate an ID for a given schema * @param schema the string defining a schema @@ -84,7 +130,8 @@ contract SchemaRegistry is OwnableUpgradeable { * @param description the Schema description * @param context the Schema context * @param schemaString the string defining a Schema - * @dev the Schema is stored in a mapping, its ID is added to an array of IDs and an event is emitted + * @dev The Schema is stored in the `schemas` mapping, its ID is added to an array of IDs and an event is emitted + * The caller is assigned as the creator of the Schema, via the `schemasIssuers` mapping */ function createSchema( string memory name, @@ -103,6 +150,7 @@ contract SchemaRegistry is OwnableUpgradeable { schemas[schemaId] = Schema(name, description, context, schemaString); schemaIds.push(schemaId); + schemasIssuers[schemaId] = msg.sender; emit SchemaCreated(schemaId, name, description, context, schemaString); } @@ -111,9 +159,11 @@ contract SchemaRegistry is OwnableUpgradeable { * @param schemaId the schema ID * @param context the Schema context * @dev Retrieve the Schema with given ID and update its context with new value + * The caller must be the creator of the given Schema (through the `schemaIssuers` mapping) */ - function updateContext(bytes32 schemaId, string memory context) public onlyIssuers(msg.sender) { + function updateContext(bytes32 schemaId, string memory context) public { if (!isRegistered(schemaId)) revert SchemaNotRegistered(); + if (schemasIssuers[schemaId] != msg.sender) revert OnlyAssignedIssuer(); schemas[schemaId].context = context; } diff --git a/contracts/test/PortalRegistry.t.sol b/contracts/test/PortalRegistry.t.sol index 5c1e2abf..5f3a026e 100644 --- a/contracts/test/PortalRegistry.t.sol +++ b/contracts/test/PortalRegistry.t.sol @@ -7,6 +7,7 @@ import { CorrectModule } from "./mocks/MockModules.sol"; import { Portal } from "../src/types/Structs.sol"; import { Router } from "../src/Router.sol"; import { AttestationRegistryMock } from "./mocks/AttestationRegistryMock.sol"; +import { SchemaRegistryMock } from "./mocks/SchemaRegistryMock.sol"; import { ModuleRegistryMock } from "./mocks/ModuleRegistryMock.sol"; import { ValidPortalMock } from "./mocks/ValidPortalMock.sol"; import { InvalidPortalMock } from "./mocks/InvalidPortalMock.sol"; @@ -18,6 +19,7 @@ contract PortalRegistryTest is Test { PortalRegistry public portalRegistry; address public moduleRegistryAddress; address public attestationRegistryAddress; + address public schemaRegistryAddress; string public expectedName = "Name"; string public expectedDescription = "Description"; string public expectedOwnerName = "Owner Name"; @@ -37,11 +39,13 @@ contract PortalRegistryTest is Test { moduleRegistryAddress = address(new ModuleRegistryMock()); attestationRegistryAddress = address(new AttestationRegistryMock()); + schemaRegistryAddress = address(new SchemaRegistryMock()); vm.prank(address(0)); portalRegistry.updateRouter(address(router)); router.updateModuleRegistry(moduleRegistryAddress); router.updateAttestationRegistry(attestationRegistryAddress); + router.updateSchemaRegistry(schemaRegistryAddress); vm.prank(address(0)); portalRegistry.setIssuer(user); diff --git a/contracts/test/SchemaRegistry.t.sol b/contracts/test/SchemaRegistry.t.sol index 7e645357..2eb250ea 100644 --- a/contracts/test/SchemaRegistry.t.sol +++ b/contracts/test/SchemaRegistry.t.sol @@ -17,6 +17,7 @@ contract SchemaRegistryTest is Test { string private expectedContext = "Context"; string private expectedString = "this is a schema"; address private user = makeAddr("user"); + address private unassignedUser = makeAddr("unassignedUser"); event SchemaCreated(bytes32 indexed id, string name, string description, string context, string schemaString); event Initialized(uint8 version); @@ -32,6 +33,7 @@ contract SchemaRegistryTest is Test { portalRegistryAddress = address(portalRegistryMock); router.updatePortalRegistry(portalRegistryAddress); portalRegistryMock.setIssuer(user); + portalRegistryMock.setIssuer(unassignedUser); } function test_initialize_ContractAlreadyInitialized() public { @@ -56,6 +58,42 @@ contract SchemaRegistryTest is Test { testSchemaRegistry.updateRouter(address(0)); } + function test_updateSchemaIssuer() public { + vm.prank(user); + schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); + vm.prank(address(0)); + schemaRegistry.updateSchemaIssuer(expectedId, address(2)); + } + + function test_updateSchemaIssuer_SchemaNotRegistered() public { + vm.expectRevert(SchemaRegistry.SchemaNotRegistered.selector); + vm.prank(address(0)); + schemaRegistry.updateSchemaIssuer(expectedId, address(0)); + } + + function test_updateSchemaIssuer_IssuerInvalid() public { + vm.prank(user); + schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); + vm.prank(address(0)); + vm.expectRevert(SchemaRegistry.IssuerInvalid.selector); + schemaRegistry.updateSchemaIssuer(expectedId, address(0)); + } + + function test_updateMatchingSchemaIssuers() public { + vm.prank(user); + schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); + vm.prank(portalRegistryAddress); + schemaRegistry.updateMatchingSchemaIssuers(user, address(2)); + } + + function test_updateMatchingSchemaIssuers_OnlyPortalRegistry() public { + vm.prank(user); + schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); + vm.prank(address(0)); + vm.expectRevert(SchemaRegistry.OnlyPortalRegistry.selector); + schemaRegistry.updateMatchingSchemaIssuers(user, address(2)); + } + function test_getIdFromSchemaString() public { bytes32 id = schemaRegistry.getIdFromSchemaString(expectedString); assertEq(id, expectedId); @@ -118,13 +156,6 @@ contract SchemaRegistryTest is Test { vm.stopPrank(); } - function test_updateContext_OnlyIssuer() public { - vm.expectRevert(SchemaRegistry.OnlyIssuer.selector); - vm.startPrank(makeAddr("InvalidIssuer")); - schemaRegistry.updateContext(expectedId, "New context"); - vm.stopPrank(); - } - function test_updateContext_SchemaNotRegistered() public { vm.startPrank(user); vm.expectRevert(SchemaRegistry.SchemaNotRegistered.selector); @@ -132,6 +163,15 @@ contract SchemaRegistryTest is Test { vm.stopPrank(); } + function test_updateContext_OnlyAssignedIssuer() public { + vm.startPrank(user); + schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); + vm.expectRevert(SchemaRegistry.OnlyAssignedIssuer.selector); + vm.startPrank(unassignedUser); + schemaRegistry.updateContext(expectedId, "New context"); + vm.stopPrank(); + } + function test_updateContext_withEmptyContext() public { vm.startPrank(user); schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); diff --git a/contracts/test/mocks/SchemaRegistryMock.sol b/contracts/test/mocks/SchemaRegistryMock.sol index d2582427..e699f695 100644 --- a/contracts/test/mocks/SchemaRegistryMock.sol +++ b/contracts/test/mocks/SchemaRegistryMock.sol @@ -26,4 +26,6 @@ contract SchemaRegistryMock { function isRegistered(bytes32 schemaId) public view returns (bool) { return schemas[schemaId]; } + + function updateMatchingSchemaIssuers(address oldIssuer, address newIssuer) public {} }