Skip to content

Commit

Permalink
fix: Portal should not be initializable/upgradeable (#199)
Browse files Browse the repository at this point in the history
  • Loading branch information
alainncls authored Sep 19, 2023
1 parent c67e045 commit cbd2142
Show file tree
Hide file tree
Showing 9 changed files with 19 additions and 33 deletions.
3 changes: 1 addition & 2 deletions src/PortalRegistry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,7 @@ contract PortalRegistry is OwnableUpgradeable {
bool isRevocable,
string memory ownerName
) external onlyIssuers(msg.sender) {
DefaultPortal defaultPortal = new DefaultPortal();
defaultPortal.initialize(modules, address(router));
DefaultPortal defaultPortal = new DefaultPortal(modules, address(router));
register(address(defaultPortal), name, description, isRevocable, ownerName);
}

Expand Down
2 changes: 2 additions & 0 deletions src/example/EASPortal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ contract EASPortal is AbstractPortal {
/// @notice Error thrown when trying to bulk revoke attestations
error NoBulkRevocation();

constructor(address[] memory modules, address router) AbstractPortal(modules, router) {}

function withdraw(address payable to, uint256 amount) external override {}

function attest(AttestationRequest memory attestationRequest) public payable {
Expand Down
14 changes: 6 additions & 8 deletions src/interface/AbstractPortal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,10 @@ import { AttestationRegistry } from "../AttestationRegistry.sol";
import { ModuleRegistry } from "../ModuleRegistry.sol";
import { PortalRegistry } from "../PortalRegistry.sol";
import { AttestationPayload } from "../types/Structs.sol";
// solhint-disable-next-line max-line-length
import { IERC165Upgradeable } from "openzeppelin-contracts-upgradeable/contracts/utils/introspection/ERC165Upgradeable.sol";
import { Initializable } from "openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initializable.sol";
import { IERC165 } from "openzeppelin-contracts/contracts/utils/introspection/ERC165.sol";
import { IRouter } from "../interface/IRouter.sol";

abstract contract AbstractPortal is Initializable, IERC165Upgradeable {
abstract contract AbstractPortal is IERC165 {
IRouter public router;
address[] public modules;
ModuleRegistry public moduleRegistry;
Expand All @@ -21,12 +19,12 @@ abstract contract AbstractPortal is Initializable, IERC165Upgradeable {
error OnlyPortalOwner();

/**
* @notice Contract initialization
* @notice Contract constructor
* @param _modules list of modules to use for the portal (can be empty)
* @param _router Router's address
* @dev This sets the addresses for the AttestationRegistry, ModuleRegistry and PortalRegistry
*/
function initialize(address[] calldata _modules, address _router) public virtual initializer {
// Store addresses for linked modules, ModuleRegistry and AttestationRegistry
constructor(address[] memory _modules, address _router) {
modules = _modules;
router = IRouter(_router);
attestationRegistry = AttestationRegistry(router.getAttestationRegistry());
Expand Down Expand Up @@ -144,7 +142,7 @@ abstract contract AbstractPortal is Initializable, IERC165Upgradeable {
* @return The list of modules addresses linked to the Portal
*/
function supportsInterface(bytes4 interfaceID) public pure virtual override returns (bool) {
return interfaceID == type(AbstractPortal).interfaceId || interfaceID == type(IERC165Upgradeable).interfaceId;
return interfaceID == type(AbstractPortal).interfaceId || interfaceID == type(IERC165).interfaceId;
}

/**
Expand Down
2 changes: 2 additions & 0 deletions src/portal/DefaultPortal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,7 @@ import { AbstractPortal } from "../interface/AbstractPortal.sol";
* @dev This Portal does not add any logic to the AbstractPortal
*/
contract DefaultPortal is AbstractPortal {
constructor(address[] memory modules, address router) AbstractPortal(modules, router) {}

function withdraw(address payable to, uint256 amount) external override {}
}
2 changes: 1 addition & 1 deletion test/PortalRegistry.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ contract PortalRegistryTest is Test {
vm.prank(address(0));
portalRegistry.setIssuer(user);

validPortalMock = new ValidPortalMock();
validPortalMock = new ValidPortalMock(new address[](0), address(router));
}

function test_alreadyInitialized() public {
Expand Down
7 changes: 1 addition & 6 deletions test/example/EASPortal.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,11 @@ contract EASPortalTest is Test {
event BulkAttestationsRegistered();

function setUp() public {
easPortal = new EASPortal();
router.initialize();
router.updateModuleRegistry(address(moduleRegistryMock));
router.updateAttestationRegistry(address(attestationRegistryMock));
easPortal.initialize(modules, address(router));
}

function test_initialize() public {
vm.expectRevert("Initializable: contract is already initialized");
easPortal.initialize(modules, address(router));
easPortal = new EASPortal(modules, address(router));
}

function test_attest() public {
Expand Down
3 changes: 1 addition & 2 deletions test/integration/AttestationRegistryMass.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,7 @@ contract AttestationRegistryMassTest is Test {
portalRegistry.setIssuer(portalOwner);
vm.prank(portalOwner);
address[] memory modules = new address[](0);
defaultPortal = new DefaultPortal();
defaultPortal.initialize(modules, address(router));
defaultPortal = new DefaultPortal(modules, address(router));

vm.prank(portalOwner);
portalRegistry.register(address(defaultPortal), "Name", "Description", true, "Linea");
Expand Down
2 changes: 2 additions & 0 deletions test/mocks/ValidPortalMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ pragma solidity 0.8.21;
import { AbstractPortal } from "../../src/interface/AbstractPortal.sol";

contract ValidPortalMock is AbstractPortal {
constructor(address[] memory modules, address router) AbstractPortal(modules, router) {}

function test() public {}

function withdraw(address payable to, uint256 amount) external override {}
Expand Down
17 changes: 3 additions & 14 deletions test/portal/DefaultPortal.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,16 @@ contract DefaultPortalTest is Test {
event BulkAttestationsRevoked(bytes32[] attestationId);

function setUp() public {
modules.push(address(correctModule));
defaultPortal = new DefaultPortal();
router.initialize();
router.updateModuleRegistry(address(moduleRegistryMock));
router.updateAttestationRegistry(address(attestationRegistryMock));
router.updatePortalRegistry(address(portalRegistryMock));

modules.push(address(correctModule));
defaultPortal = new DefaultPortal(modules, address(router));

vm.prank(portalOwner);
portalRegistryMock.register(address(defaultPortal), "Name", "Description", true, "Owner name");

defaultPortal.initialize(modules, address(router));
}

function test_setup() public {
Expand All @@ -53,16 +52,6 @@ contract DefaultPortalTest is Test {
assertEq(portalRegistryMock.getPortalByAddress(address(defaultPortal)).ownerAddress, portalOwner);
}

function test_initialize() public {
DefaultPortal defaultPortalTest = new DefaultPortal();
vm.expectEmit();
emit Initialized(1);
defaultPortalTest.initialize(modules, address(router));

vm.expectRevert("Initializable: contract is already initialized");
defaultPortalTest.initialize(modules, address(router));
}

function test_getModules() public {
address[] memory _modules = defaultPortal.getModules();
assertEq(_modules, modules);
Expand Down

0 comments on commit cbd2142

Please sign in to comment.