chore(contracts): Avoid code duplication in AbstractPortal (#826)

Consensys · Nov 28, 2024 · cd6e986 · cd6e986
1 parent f854191
commit cd6e986
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 42 deletions.
diff --git 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
@@ -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
@@ -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
@@ -7,13 +7,15 @@ 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 {
   address public attester = makeAddr("attester");
   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);
   }