From cd6e986f6a9ef87adf1a4848cb6fc275d02fc6d0 Mon Sep 17 00:00:00 2001 From: Alain Nicolas Date: Thu, 28 Nov 2024 12:02:46 +0100 Subject: [PATCH] chore(contracts): Avoid code duplication in AbstractPortal (#826) --- contracts/src/abstracts/AbstractPortal.sol | 33 ++++++++------- contracts/src/abstracts/AbstractPortalV2.sol | 42 +++++++++---------- contracts/src/examples/portals/EASPortal.sol | 3 -- .../test/examples/portals/EASPortal.t.sol | 7 ++++ 4 files changed, 43 insertions(+), 42 deletions(-) diff --git a/contracts/src/abstracts/AbstractPortal.sol b/contracts/src/abstracts/AbstractPortal.sol index ba9e753b..d146064b 100644 --- a/contracts/src/abstracts/AbstractPortal.sol +++ b/contracts/src/abstracts/AbstractPortal.sol @@ -22,7 +22,7 @@ abstract contract AbstractPortal is IPortal, ERC165 { AttestationRegistry public attestationRegistry; PortalRegistry public portalRegistry; - /// @notice Error thrown when someone else than the portal's owner is trying to revoke + /// @notice Error thrown when someone else than the portal's owner is trying to revoke or replace error OnlyPortalOwner(); /// @notice Error thrown when withdrawing funds fails @@ -42,14 +42,19 @@ abstract contract AbstractPortal is IPortal, ERC165 { portalRegistry = PortalRegistry(router.getPortalRegistry()); } + /// @notice Modifier to enforce only the portal owner can perform certain actions + modifier onlyPortalOwner() { + if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner(); + _; + } + /** * @notice Withdraw funds from the Portal * @param to the address to send the funds to * @param amount the amount to withdraw * @dev Only the Portal owner can withdraw funds */ - function withdraw(address payable to, uint256 amount) external virtual { - if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner(); + function withdraw(address payable to, uint256 amount) external virtual onlyPortalOwner { (bool s, ) = to.call{ value: amount }(""); if (!s) revert WithdrawFail(); } @@ -317,15 +322,14 @@ abstract contract AbstractPortal is IPortal, ERC165 { * @param attestationPayload the attestation payload to create attestation and register it * @param attester the address of the attester * @param value the value sent with the attestation + * @dev This method now uses the `onlyPortalOwner` modifier to enforce ownership rules */ function _onReplace( bytes32 attestationId, AttestationPayload memory attestationPayload, address attester, uint256 value - ) internal virtual { - if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner(); - } + ) internal virtual onlyPortalOwner {} /** * @notice Optional method run when attesting a batch of payloads @@ -343,28 +347,25 @@ abstract contract AbstractPortal is IPortal, ERC165 { * @param attestationIds the IDs of the attestations being replaced * @param attestationsPayloads the payloads to replace * @param validationPayloads the payloads to validate in order to replace the attestations + * @dev This method now uses the `onlyPortalOwner` modifier to enforce ownership rules */ function _onBulkReplace( bytes32[] memory attestationIds, AttestationPayload[] memory attestationsPayloads, bytes[][] memory validationPayloads - ) internal virtual { - if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner(); - } + ) internal virtual onlyPortalOwner {} /** - * @notice Optional method run when an attestation is revoked or replaced + * @notice Optional method run when an attestation is revoked * @dev IMPORTANT NOTE: By default, revocation is only possible by the portal owner + * @dev This method now uses the `onlyPortalOwner` modifier to enforce ownership rules */ - function _onRevoke(bytes32 /*attestationId*/) internal virtual { - if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner(); - } + function _onRevoke(bytes32 attestationId) internal virtual onlyPortalOwner {} /** * @notice Optional method run when a batch of attestations are revoked or replaced * @dev IMPORTANT NOTE: By default, revocation is only possible by the portal owner + * @dev This method now uses the `onlyPortalOwner` modifier to enforce ownership rules */ - function _onBulkRevoke(bytes32[] memory /*attestationIds*/) internal virtual { - if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner(); - } + function _onBulkRevoke(bytes32[] memory attestationIds) internal virtual onlyPortalOwner {} } diff --git a/contracts/src/abstracts/AbstractPortalV2.sol b/contracts/src/abstracts/AbstractPortalV2.sol index cce873da..eb79a52c 100644 --- a/contracts/src/abstracts/AbstractPortalV2.sol +++ b/contracts/src/abstracts/AbstractPortalV2.sol @@ -44,14 +44,21 @@ abstract contract AbstractPortalV2 is IPortal, ERC165 { portalRegistry = PortalRegistry(router.getPortalRegistry()); } + /** + * @notice Modifier to enforce only the portal owner can perform certain actions. + */ + modifier onlyPortalOwner() { + if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner(); + _; + } + /** * @notice Withdraw funds from the Portal * @param to the address to send the funds to * @param amount the amount to withdraw * @dev Only the Portal owner can withdraw funds */ - function withdraw(address payable to, uint256 amount) external virtual { - if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner(); + function withdraw(address payable to, uint256 amount) external virtual onlyPortalOwner { (bool s, ) = to.call{ value: amount }(""); if (!s) revert WithdrawFail(); } @@ -112,7 +119,7 @@ abstract contract AbstractPortalV2 is IPortal, ERC165 { bytes32 attestationId, AttestationPayload memory attestationPayload, bytes[] memory validationPayloads - ) public payable { + ) public payable onlyPortalOwner { moduleRegistry.runModulesV2( modules, attestationPayload, @@ -141,7 +148,7 @@ abstract contract AbstractPortalV2 is IPortal, ERC165 { bytes32[] memory attestationIds, AttestationPayload[] memory attestationsPayloads, bytes[][] memory validationPayloads - ) public { + ) public onlyPortalOwner { moduleRegistry.bulkRunModulesV2( modules, attestationsPayloads, @@ -162,7 +169,7 @@ abstract contract AbstractPortalV2 is IPortal, ERC165 { * @dev By default, revocation is only possible by the portal owner * We strongly encourage implementing such a rule in your Portal if you intend on overriding this method */ - function revoke(bytes32 attestationId) public { + function revoke(bytes32 attestationId) public onlyPortalOwner { _onRevoke(attestationId); attestationRegistry.revoke(attestationId); @@ -172,7 +179,7 @@ abstract contract AbstractPortalV2 is IPortal, ERC165 { * @notice Bulk revokes a list of attestations for the given identifiers * @param attestationIds the IDs of the attestations to revoke */ - function bulkRevoke(bytes32[] memory attestationIds) public { + function bulkRevoke(bytes32[] memory attestationIds) public onlyPortalOwner { _onBulkRevoke(attestationIds); attestationRegistry.bulkRevoke(attestationIds); @@ -189,7 +196,7 @@ abstract contract AbstractPortalV2 is IPortal, ERC165 { /** * @notice Verifies that a specific interface is implemented by the Portal, following ERC-165 specification * @param interfaceID the interface identifier checked in this call - * @return The list of modules addresses linked to the Portal + * @return True if the interface is supported, false otherwise */ function supportsInterface(bytes4 interfaceID) public view virtual override returns (bool) { return @@ -207,10 +214,7 @@ abstract contract AbstractPortalV2 is IPortal, ERC165 { } /** - * @notice Optional method run before a payload is attested - * @param attestationPayload the attestation payload to attest - * @param validationPayloads the payloads to validate via the modules - * @param value the value sent with the attestation + * @notice Optional hooks for specific operations */ function _onAttest( AttestationPayload memory attestationPayload, @@ -231,9 +235,7 @@ abstract contract AbstractPortalV2 is IPortal, ERC165 { AttestationPayload memory attestationPayload, address attester, uint256 value - ) internal virtual { - if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner(); - } + ) internal virtual {} /** * @notice Optional method run when attesting a batch of payloads @@ -256,23 +258,17 @@ abstract contract AbstractPortalV2 is IPortal, ERC165 { bytes32[] memory attestationIds, AttestationPayload[] memory attestationsPayloads, bytes[][] memory validationPayloads - ) internal virtual { - if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner(); - } + ) internal virtual {} /** * @notice Optional method run when an attestation is revoked or replaced * @dev IMPORTANT NOTE: By default, revocation is only possible by the portal owner */ - function _onRevoke(bytes32 /*attestationId*/) internal virtual { - if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner(); - } + function _onRevoke(bytes32 /*attestationId*/) internal virtual {} /** * @notice Optional method run when a batch of attestations are revoked or replaced * @dev IMPORTANT NOTE: By default, revocation is only possible by the portal owner */ - function _onBulkRevoke(bytes32[] memory /*attestationIds*/) internal virtual { - if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner(); - } + function _onBulkRevoke(bytes32[] memory /*attestationIds*/) internal virtual {} } diff --git a/contracts/src/examples/portals/EASPortal.sol b/contracts/src/examples/portals/EASPortal.sol index 56bd8c57..44b815b8 100644 --- a/contracts/src/examples/portals/EASPortal.sol +++ b/contracts/src/examples/portals/EASPortal.sol @@ -48,9 +48,6 @@ contract EASPortal is AbstractPortalV2 { */ constructor(address[] memory modules, address router) AbstractPortalV2(modules, router) {} - /// @inheritdoc AbstractPortalV2 - function withdraw(address payable to, uint256 amount) external override {} - /** * @notice Issues a Verax attestation based on an EAS attestation * @param attestationRequest the EAS payload to attest diff --git a/contracts/test/examples/portals/EASPortal.t.sol b/contracts/test/examples/portals/EASPortal.t.sol index 771191e4..9c67ae5b 100644 --- a/contracts/test/examples/portals/EASPortal.t.sol +++ b/contracts/test/examples/portals/EASPortal.t.sol @@ -7,6 +7,7 @@ import { Router } from "../../../src/Router.sol"; import { AbstractPortalV2 } from "../../../src/abstracts/AbstractPortalV2.sol"; import { AttestationRegistryMock } from "../../mocks/AttestationRegistryMock.sol"; import { ModuleRegistryMock } from "../../mocks/ModuleRegistryMock.sol"; +import { PortalRegistryMock } from "../../mocks/PortalRegistryMock.sol"; import { ERC165Upgradeable } from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; contract EASPortalTest is Test { @@ -14,6 +15,7 @@ contract EASPortalTest is Test { EASPortal public easPortal; address[] public modules = new address[](0); ModuleRegistryMock public moduleRegistryMock = new ModuleRegistryMock(); + PortalRegistryMock public portalRegistryMock = new PortalRegistryMock(); AttestationRegistryMock public attestationRegistryMock = new AttestationRegistryMock(); Router public router = new Router(); @@ -24,9 +26,12 @@ contract EASPortalTest is Test { function setUp() public { router.initialize(); router.updateModuleRegistry(address(moduleRegistryMock)); + router.updatePortalRegistry(address(portalRegistryMock)); router.updateAttestationRegistry(address(attestationRegistryMock)); easPortal = new EASPortal(modules, address(router)); + + portalRegistryMock.register(address(easPortal), "EASPortal", "EASPortal description", false, "EAS"); } function test_attest() public { @@ -134,6 +139,7 @@ contract EASPortalTest is Test { function test_revoke() public { vm.expectRevert(EASPortal.NoRevocation.selector); + vm.prank(address(this)); easPortal.revoke(bytes32(uint256(1))); } @@ -147,6 +153,7 @@ contract EASPortalTest is Test { replacingAttestations[1] = bytes32(uint256(3)); vm.expectRevert(EASPortal.NoBulkRevocation.selector); + vm.prank(address(this)); easPortal.bulkRevoke(attestationsToRevoke); }