From ba69bac7b1d52eef64acd67566945d2597ab62b9 Mon Sep 17 00:00:00 2001 From: Satyajeet Kolhapure <77279246+satyajeetkolhapure@users.noreply.github.com> Date: Wed, 27 Nov 2024 19:54:25 +0000 Subject: [PATCH] chore(contracts): Revert when no change happens in a state-changing function (#834) --- contracts/src/AttestationReader.sol | 8 +++++ contracts/src/AttestationRegistry.sol | 8 +++++ contracts/src/ModuleRegistry.sol | 4 +++ contracts/src/PortalRegistry.sol | 11 +++++++ contracts/src/SchemaRegistry.sol | 14 +++++++++ contracts/test/AttestationReader.t.sol | 24 +++++++++++++++ contracts/test/AttestationRegistry.t.sol | 31 +++++++++++++++++-- contracts/test/ModuleRegistry.t.sol | 12 ++++++++ contracts/test/PortalRegistry.t.sol | 32 +++++++++++++++++++ contracts/test/SchemaRegistry.t.sol | 39 ++++++++++++++++++++++++ 10 files changed, 180 insertions(+), 3 deletions(-) diff --git a/contracts/src/AttestationReader.sol b/contracts/src/AttestationReader.sol index f0a26755..81d62708 100644 --- a/contracts/src/AttestationReader.sol +++ b/contracts/src/AttestationReader.sol @@ -17,8 +17,12 @@ contract AttestationReader is RouterManager { IRouter public router; IEAS public easRegistry; + /// @notice Error thrown when the Router address remains unchanged + error RouterAlreadyUpdated(); /// @notice Error thrown when an invalid EAS registry address is given error EASAddressInvalid(); + /// @notice Error thrown when the EAS registry address remains unchanged + error EASRegistryAddressAlreadyUpdated(); /// @notice Event emitted when the EAS registry address is updated event EASRegistryAddressUpdated(address easRegistryAddress); @@ -40,6 +44,8 @@ contract AttestationReader is RouterManager { * @param _router the new Router address */ function _setRouter(address _router) internal override { + if (_router == address(router)) revert RouterAlreadyUpdated(); + router = IRouter(_router); } @@ -50,6 +56,8 @@ contract AttestationReader is RouterManager { */ function updateEASRegistryAddress(address _easRegistryAddress) public onlyOwner { if (_easRegistryAddress == address(0)) revert EASAddressInvalid(); + if (_easRegistryAddress == address(easRegistry)) revert EASRegistryAddressAlreadyUpdated(); + easRegistry = IEAS(_easRegistryAddress); emit EASRegistryAddressUpdated(_easRegistryAddress); } diff --git a/contracts/src/AttestationRegistry.sol b/contracts/src/AttestationRegistry.sol index be65fa94..3e5d028f 100644 --- a/contracts/src/AttestationRegistry.sol +++ b/contracts/src/AttestationRegistry.sol @@ -23,6 +23,10 @@ contract AttestationRegistry is RouterManager { uint256 private chainPrefix; + /// @notice Error thrown when the Router address remains unchanged + error RouterAlreadyUpdated(); + /// @notice Error thrown when the chain prefix remains unchanged + error ChainPrefixAlreadyUpdated(); /// @notice Error thrown when a non-portal tries to call a method that can only be called by a portal error OnlyPortal(); /// @notice Error thrown when an attestation is not registered in the AttestationRegistry @@ -80,6 +84,8 @@ contract AttestationRegistry is RouterManager { * @param _router the new Router address */ function _setRouter(address _router) internal override { + if (_router == address(router)) revert RouterAlreadyUpdated(); + router = IRouter(_router); } @@ -88,6 +94,8 @@ contract AttestationRegistry is RouterManager { * @dev Only the registry owner can call this method */ function updateChainPrefix(uint256 _chainPrefix) public onlyOwner { + if (_chainPrefix == chainPrefix) revert ChainPrefixAlreadyUpdated(); + chainPrefix = _chainPrefix; emit ChainPrefixUpdated(_chainPrefix); } diff --git a/contracts/src/ModuleRegistry.sol b/contracts/src/ModuleRegistry.sol index c06dcf0e..f8b18e17 100644 --- a/contracts/src/ModuleRegistry.sol +++ b/contracts/src/ModuleRegistry.sol @@ -24,6 +24,8 @@ contract ModuleRegistry is RouterManager { /// @dev Deprecated: The `moduleAddresses` variable is no longer used. It was used to store the modules addresses. address[] public moduleAddresses; + /// @notice Error thrown when the Router address remains unchanged + error RouterAlreadyUpdated(); /// @notice Error thrown when a non-allowlisted user tries to call a forbidden method error OnlyAllowlisted(); /// @notice Error thrown when an identical Module was already registered @@ -70,6 +72,8 @@ contract ModuleRegistry is RouterManager { * @param _router the new Router address */ function _setRouter(address _router) internal override { + if (_router == address(router)) revert RouterAlreadyUpdated(); + router = IRouter(_router); } diff --git a/contracts/src/PortalRegistry.sol b/contracts/src/PortalRegistry.sol index a9dbd03b..b8eb2d97 100644 --- a/contracts/src/PortalRegistry.sol +++ b/contracts/src/PortalRegistry.sol @@ -27,6 +27,12 @@ contract PortalRegistry is RouterManager { bool private isTestnet; + /// @notice Error thrown when the Router address remains unchanged + error RouterAlreadyUpdated(); + /// @notice Error thrown when attempting to set an issuer that is already set + error IssuerAlreadySet(); + /// @notice Error thrown when the testnet flag remains unchanged + error TestnetStatusAlreadyUpdated(); /// @notice Error thrown when a non-allowlisted user tries to call a forbidden method error OnlyAllowlisted(); /// @notice Error thrown when attempting to register a Portal twice @@ -75,6 +81,8 @@ contract PortalRegistry is RouterManager { * @param _router the new Router address */ function _setRouter(address _router) internal override { + if (_router == address(router)) revert RouterAlreadyUpdated(); + router = IRouter(_router); } @@ -84,6 +92,7 @@ contract PortalRegistry is RouterManager { */ function setIssuer(address issuer) public onlyOwner { if (issuer == address(0)) revert AddressInvalid(); + if (issuers[issuer]) revert IssuerAlreadySet(); issuers[issuer] = true; emit IssuerAdded(issuer); @@ -94,6 +103,8 @@ contract PortalRegistry is RouterManager { * @param _isTestnet the flag defining the testnet status */ function setIsTestnet(bool _isTestnet) public onlyOwner { + if (isTestnet == _isTestnet) revert TestnetStatusAlreadyUpdated(); + isTestnet = _isTestnet; emit IsTestnetUpdated(_isTestnet); } diff --git a/contracts/src/SchemaRegistry.sol b/contracts/src/SchemaRegistry.sol index da96488b..f474bbef 100644 --- a/contracts/src/SchemaRegistry.sol +++ b/contracts/src/SchemaRegistry.sol @@ -21,6 +21,12 @@ contract SchemaRegistry is RouterManager { /// @dev Associates a Schema ID with the address of the Issuer who created it mapping(bytes32 id => address issuer) private schemasIssuers; + /// @notice Error thrown when the Router address remains unchanged + error RouterAlreadyUpdated(); + /// @notice Error thrown when attempting to set a schema issuer that is already set + error SchemaIssuerAlreadySet(); + /// @notice Error thrown when the schema context remains unchanged + error SchemaContextAlreadyUpdated(); /// @notice Error thrown when a non-allowlisted user tries to call a forbidden method error OnlyAllowlisted(); /// @notice Error thrown when any address which is not a portal registry tries to call a method @@ -81,6 +87,8 @@ contract SchemaRegistry is RouterManager { * @param _router the new Router address */ function _setRouter(address _router) internal override { + if (_router == address(router)) revert RouterAlreadyUpdated(); + router = IRouter(_router); } @@ -94,6 +102,8 @@ contract SchemaRegistry is RouterManager { function updateSchemaIssuer(bytes32 schemaId, address issuer) public onlyOwner { if (!isRegistered(schemaId)) revert SchemaNotRegistered(); if (issuer == address(0)) revert IssuerInvalid(); + if (schemasIssuers[schemaId] == issuer) revert SchemaIssuerAlreadySet(); + schemasIssuers[schemaId] = issuer; emit SchemaIssuerUpdated(schemaId, issuer); } @@ -163,6 +173,10 @@ contract SchemaRegistry is RouterManager { function updateContext(bytes32 schemaId, string memory context) public { if (!isRegistered(schemaId)) revert SchemaNotRegistered(); if (schemasIssuers[schemaId] != msg.sender) revert OnlyAssignedIssuer(); + if (keccak256(abi.encodePacked(schemas[schemaId].context)) == keccak256(abi.encodePacked(context))) { + revert SchemaContextAlreadyUpdated(); + } + schemas[schemaId].context = context; emit SchemaContextUpdated(schemaId); } diff --git a/contracts/test/AttestationReader.t.sol b/contracts/test/AttestationReader.t.sol index 95637b7c..7f64d108 100644 --- a/contracts/test/AttestationReader.t.sol +++ b/contracts/test/AttestationReader.t.sol @@ -66,6 +66,18 @@ contract AttestationReaderTest is Test { testAttestationReader.updateRouter(address(0)); } + function test_updateRouter_RouterAlreadyUpdated() public { + AttestationReader testAttestationReader = new AttestationReader(); + vm.expectEmit(true, true, true, true); + emit RouterUpdated(address(1)); + vm.prank(address(0)); + testAttestationReader.updateRouter(address(1)); + + vm.expectRevert(AttestationReader.RouterAlreadyUpdated.selector); + vm.prank(address(0)); + testAttestationReader.updateRouter(address(1)); + } + function test_updateEASRegistryAddress() public { AttestationReader testAttestationReader = new AttestationReader(); @@ -85,6 +97,18 @@ contract AttestationReaderTest is Test { testAttestationReader.updateEASRegistryAddress(address(0)); } + function test_updateEASRegistryAddress_EASRegistryAddressAlreadyUpdated() public { + AttestationReader testAttestationReader = new AttestationReader(); + vm.expectEmit(true, true, true, true); + emit EASRegistryAddressUpdated(address(1)); + vm.prank(address(0)); + testAttestationReader.updateEASRegistryAddress(address(1)); + + vm.expectRevert(AttestationReader.EASRegistryAddressAlreadyUpdated.selector); + vm.prank(address(0)); + testAttestationReader.updateEASRegistryAddress(address(1)); + } + function test_getAttestation_fromEAS() public { EASAttestation memory easAttestation = EASAttestation({ uid: keccak256(abi.encodePacked("uniqueID")), diff --git a/contracts/test/AttestationRegistry.t.sol b/contracts/test/AttestationRegistry.t.sol index a3f91c9a..9c2cfe6e 100644 --- a/contracts/test/AttestationRegistry.t.sol +++ b/contracts/test/AttestationRegistry.t.sol @@ -97,6 +97,26 @@ contract AttestationRegistryTest is Test { assertEq(routerAddress, address(1)); } + function test_updateRouter_InvalidParameter() public { + AttestationRegistry testAttestationRegistry = new AttestationRegistry(); + + vm.expectRevert(RouterManager.RouterInvalid.selector); + vm.prank(address(0)); + testAttestationRegistry.updateRouter(address(0)); + } + + function test_updateRouter_RouterAlreadyUpdated() public { + AttestationRegistry testAttestationRegistry = new AttestationRegistry(); + vm.expectEmit(true, true, true, true); + emit RouterUpdated(address(1)); + vm.prank(address(0)); + testAttestationRegistry.updateRouter(address(1)); + + vm.expectRevert(AttestationRegistry.RouterAlreadyUpdated.selector); + vm.prank(address(0)); + testAttestationRegistry.updateRouter(address(1)); + } + function test_updateChainPrefix() public { AttestationRegistry testAttestationRegistry = new AttestationRegistry(); @@ -117,12 +137,17 @@ contract AttestationRegistryTest is Test { assertEq(chainPrefix, 0x0001000000000000000000000000000000000000000000000000000000000000); } - function test_updateRouter_InvalidParameter() public { + function test_updateChainPrefix_ChainPrefixAlreadyUpdated() public { AttestationRegistry testAttestationRegistry = new AttestationRegistry(); - vm.expectRevert(RouterManager.RouterInvalid.selector); + vm.expectEmit(true, true, true, true); + emit ChainPrefixUpdated(initialChainPrefix); vm.prank(address(0)); - testAttestationRegistry.updateRouter(address(0)); + testAttestationRegistry.updateChainPrefix(initialChainPrefix); + + vm.expectRevert(AttestationRegistry.ChainPrefixAlreadyUpdated.selector); + vm.prank(address(0)); + testAttestationRegistry.updateChainPrefix(initialChainPrefix); } function test_attest(AttestationPayload memory attestationPayload) public { diff --git a/contracts/test/ModuleRegistry.t.sol b/contracts/test/ModuleRegistry.t.sol index 9ab9eba2..c0e6a33e 100644 --- a/contracts/test/ModuleRegistry.t.sol +++ b/contracts/test/ModuleRegistry.t.sol @@ -71,6 +71,18 @@ contract ModuleRegistryTest is Test { testModuleRegistry.updateRouter(address(0)); } + function test_updateRouter_RouterAlreadyUpdated() public { + ModuleRegistry testModuleRegistry = new ModuleRegistry(); + vm.expectEmit(true, true, true, true); + emit RouterUpdated(address(1)); + vm.prank(address(0)); + testModuleRegistry.updateRouter(address(1)); + + vm.expectRevert(ModuleRegistry.RouterAlreadyUpdated.selector); + vm.prank(address(0)); + testModuleRegistry.updateRouter(address(1)); + } + function test_isContractAddress() public view { // isContractAddress should return false for EOA address address eoaAddress = vm.addr(1); diff --git a/contracts/test/PortalRegistry.t.sol b/contracts/test/PortalRegistry.t.sol index bf76eac7..d9895ec9 100644 --- a/contracts/test/PortalRegistry.t.sol +++ b/contracts/test/PortalRegistry.t.sol @@ -83,6 +83,18 @@ contract PortalRegistryTest is Test { testPortalRegistry.updateRouter(address(0)); } + function test_updateRouter_RouterAlreadyUpdated() public { + PortalRegistry testPortalRegistry = new PortalRegistry(false); + vm.expectEmit(true, true, true, true); + emit RouterUpdated(address(1)); + vm.prank(address(0)); + testPortalRegistry.updateRouter(address(1)); + + vm.expectRevert(PortalRegistry.RouterAlreadyUpdated.selector); + vm.prank(address(0)); + testPortalRegistry.updateRouter(address(1)); + } + function test_setIssuer() public { vm.startPrank(address(0)); address issuerAddress = makeAddr("Issuer"); @@ -115,6 +127,17 @@ contract PortalRegistryTest is Test { assertEq(isIssuer, false); } + function test_setIssuer_IssuerAlreadySet() public { + vm.startPrank(address(0)); + address issuerAddress = makeAddr("Issuer"); + vm.expectEmit(); + emit IssuerAdded(issuerAddress); + portalRegistry.setIssuer(issuerAddress); + + vm.expectRevert(PortalRegistry.IssuerAlreadySet.selector); + portalRegistry.setIssuer(issuerAddress); + } + function test_setIsTestnet_true() public { bool isTestnet = portalRegistry.getIsTestnet(); assertEq(isTestnet, false); @@ -156,6 +179,15 @@ contract PortalRegistryTest is Test { assertEq(isTestnet, false); } + function test_setIsTestnet_TestnetStatusAlreadyUpdated() public { + bool isTestnet = portalRegistry.getIsTestnet(); + assertEq(isTestnet, false); + + vm.prank(address(0)); + vm.expectRevert(PortalRegistry.TestnetStatusAlreadyUpdated.selector); + portalRegistry.setIsTestnet(false); + } + function test_removeIssuer() public { vm.startPrank(address(0)); address issuerAddress = makeAddr("Issuer"); diff --git a/contracts/test/SchemaRegistry.t.sol b/contracts/test/SchemaRegistry.t.sol index 4e36ac08..24414dd6 100644 --- a/contracts/test/SchemaRegistry.t.sol +++ b/contracts/test/SchemaRegistry.t.sol @@ -67,6 +67,19 @@ contract SchemaRegistryTest is Test { testSchemaRegistry.updateRouter(address(0)); } + function test_updateRouter_RouterAlreadyUpdated() public { + SchemaRegistry testSchemaRegistry = new SchemaRegistry(); + + vm.expectEmit(true, true, true, true); + emit RouterUpdated(address(1)); + vm.prank(address(0)); + testSchemaRegistry.updateRouter(address(1)); + + vm.expectRevert(SchemaRegistry.RouterAlreadyUpdated.selector); + vm.prank(address(0)); + testSchemaRegistry.updateRouter(address(1)); + } + function test_updateSchemaIssuer() public { vm.prank(user); schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); @@ -90,6 +103,19 @@ contract SchemaRegistryTest is Test { schemaRegistry.updateSchemaIssuer(expectedId, address(0)); } + function test_updateSchemaIssuer_SchemaIssuerAlreadySet() public { + vm.prank(user); + schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); + vm.expectEmit(true, true, true, true); + emit SchemaIssuerUpdated(expectedId, address(2)); + vm.prank(address(0)); + schemaRegistry.updateSchemaIssuer(expectedId, address(2)); + + vm.prank(address(0)); + vm.expectRevert(SchemaRegistry.SchemaIssuerAlreadySet.selector); + schemaRegistry.updateSchemaIssuer(expectedId, address(2)); + } + function test_bulkUpdateSchemasIssuers() public { vm.prank(user); schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); @@ -205,6 +231,19 @@ contract SchemaRegistryTest is Test { vm.stopPrank(); } + function test_updateContext_SchemaContextAlreadyUpdated() public { + vm.expectEmit(); + emit SchemaCreated(expectedId, expectedName, expectedDescription, expectedContext, expectedString); + vm.startPrank(user); + // create a schema + schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); + + vm.expectRevert(SchemaRegistry.SchemaContextAlreadyUpdated.selector); + // update the context with the same value + schemaRegistry.updateContext(expectedId, expectedContext); + vm.stopPrank(); + } + function test_getSchema() public { vm.startPrank(user); schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString);