Merge pull request #29 from etherisc/bug/fix-role-creation

fix role creation, add test cases

brownie-config.yaml

@@ -22,7 +22,7 @@ compiler:
         remappings:
           - "@openzeppelin=OpenZeppelin/openzeppelin-contracts@4.7.0"
           - "@chainlink=smartcontractkit/chainlink@1.6.0"
-          - "@etherisc/gif-interface=etherisc/gif-interface@d954cef"
+          - "@etherisc/gif-interface=etherisc/gif-interface@d154067"
 
 # packages below will be added to brownie
 # you may use 'brownie pm list' after 'brownie compile' 
@@ -31,7 +31,7 @@ dependencies:
     # github dependency format: <owner>/<repository>@<release>
     - OpenZeppelin/openzeppelin-contracts@4.7.0
     - smartcontractkit/chainlink@1.6.0
-    - etherisc/gif-interface@d954cef
+    - etherisc/gif-interface@d154067
 
 # exclude open zeppeling contracts  when calculating test coverage
 # https://eth-brownie.readthedocs.io/en/v1.10.3/config.html#exclude_paths

contracts/modules/AccessController.sol

@@ -26,23 +26,26 @@ contract AccessController is
 
     mapping(bytes32 => bool) public validRole;
 
-    bool defaultAdminSet;
+    bool private _defaultAdminSet;
 
     function _afterInitialize() internal override {
+        // add product owner, oracle provider and riskpool keeper roles
         _populateValidRoles();
     }
 
     function _getName() internal override pure returns(bytes32) { return "Access"; }
 
-    // IMPORTANT this method must be called during initial setup of the GIF instance
-    // otherwise any caller might set the default role admin to any addressS
+    // IMPORTANT check the setting of the default admin role
+    // after the deployment of a GIF instance.
+    // this method is called in the deployment of
+    // the instance operator proxy/controller 
     function setDefaultAdminRole(address defaultAdmin) 
-        public 
+        external 
     {
-        require(!defaultAdminSet, "ERROR:ACL-001:ADMIN_ROLE_ALREADY_SET");
-        defaultAdminSet = true;
+        require(!_defaultAdminSet, "ERROR:ACL-001:ADMIN_ROLE_ALREADY_SET");
+        _defaultAdminSet = true;
 
-        _setupRole(DEFAULT_ADMIN_ROLE, defaultAdmin);
+        _grantRole(DEFAULT_ADMIN_ROLE, defaultAdmin);
     }
 
     //--- manage role ownership ---------------------------------------------//
@@ -75,7 +78,7 @@ contract AccessController is
         public override
         onlyInstanceOperator 
     {
-        require(validRole[role], "ERROR:ACL-003:ROLE_EXISTING_AND_VALID");
+        require(!validRole[role], "ERROR:ACL-003:ROLE_EXISTING_AND_VALID");
         validRole[role] = true;
     }
 
@@ -95,15 +98,19 @@ contract AccessController is
         return super.hasRole(role, principal);
     }
 
-    function productOwnerRole() public view override returns(bytes32) {
+    function getDefaultAdminRole() public view override returns(bytes32) {
+        return DEFAULT_ADMIN_ROLE;
+    }
+
+    function getProductOwnerRole() public view override returns(bytes32) {
         return PRODUCT_OWNER_ROLE;
     }
 
-    function oracleProviderRole() public view override returns(bytes32) {
+    function getOracleProviderRole() public view override returns(bytes32) {
         return ORACLE_PROVIDER_ROLE;
     }
 
-    function riskpoolKeeperRole() public view override returns(bytes32) {
+    function getRiskpoolKeeperRole() public view override returns(bytes32) {
         return RISKPOOL_KEEPER_ROLE;
     }
 

contracts/modules/ComponentController.sol

@@ -203,9 +203,9 @@ contract ComponentController is
     }
 
     function getRequiredRole(IComponent.ComponentType componentType) external returns (bytes32) {
-        if (componentType == IComponent.ComponentType.Product) { return _access.productOwnerRole(); }
-        else if (componentType == IComponent.ComponentType.Oracle) { return _access.oracleProviderRole(); }
-        else if (componentType == IComponent.ComponentType.Riskpool) { return _access.riskpoolKeeperRole(); }
+        if (componentType == IComponent.ComponentType.Product) { return _access.getProductOwnerRole(); }
+        else if (componentType == IComponent.ComponentType.Oracle) { return _access.getOracleProviderRole(); }
+        else if (componentType == IComponent.ComponentType.Riskpool) { return _access.getRiskpoolKeeperRole(); }
         else { revert("ERROR:CCR-008:COMPONENT_TYPE_UNKNOWN"); }
     }
 

contracts/services/InstanceOperatorService.sol

@@ -1,11 +1,14 @@
 // SPDX-License-Identifier: Apache-2.0
 pragma solidity ^0.8.0;
 
+import "../modules/AccessController.sol";
+import "../modules/BundleController.sol";
 import "../modules/ComponentController.sol";
 import "../modules/PoolController.sol";
 import "../modules/TreasuryModule.sol";
 import "../shared/CoreController.sol";
 import "../test/TestProduct.sol";
+import "../tokens/BundleToken.sol";
 
 import "@etherisc/gif-interface/contracts/components/IComponent.sol";
 import "@etherisc/gif-interface/contracts/components/IProduct.sol";
@@ -35,6 +38,19 @@ contract InstanceOperatorService is
         _treasury = TreasuryModule(_getContractAddress("Treasury"));
 
         _transferOwnership(_msgSender());
+        _linkBundleModuleToBundleToken();
+        _setDefaultAdminRole();
+    }
+
+    function _setDefaultAdminRole() private {
+        AccessController access = AccessController(_getContractAddress("Access"));
+        access.setDefaultAdminRole(address(this));
+    }
+
+    function _linkBundleModuleToBundleToken() private {
+        BundleToken token = BundleToken(_getContractAddress("BundleToken"));
+        address bundleAddress = _getContractAddress("Bundle");
+        token.setBundleModule(bundleAddress);
     }
 
     /* registry */
@@ -85,6 +101,13 @@ contract InstanceOperatorService is
         _access.addRole(_role);
     }
 
+    function invalidateRole(bytes32 _role) 
+        external
+        onlyInstanceOperatorAddress 
+    {
+        _access.invalidateRole(_role);
+    }
+
     function grantRole(bytes32 role, address principal)
         external override
         onlyInstanceOperatorAddress

contracts/services/InstanceService.sol

@@ -97,16 +97,20 @@ contract InstanceService is
     }
 
     /* access */
+    function getDefaultAdminRole() external view returns(bytes32) {
+        return _access.getDefaultAdminRole();
+    }
+
     function getProductOwnerRole() external override view returns(bytes32) {
-        return _access.productOwnerRole();
+        return _access.getProductOwnerRole();
     }
 
     function getOracleProviderRole() external override view returns(bytes32) {
-        return _access.oracleProviderRole();
+        return _access.getOracleProviderRole();
     }
 
     function getRiskpoolKeeperRole() external override view returns(bytes32) {
-        return _access.riskpoolKeeperRole();
+        return _access.getRiskpoolKeeperRole();
     }
 
     function hasRole(bytes32 role, address principal)

contracts/tokens/BundleToken.sol

@@ -29,7 +29,6 @@ contract BundleToken is
 
     function setBundleModule(address bundleModule)
         external
-        onlyOwner
     {
         require(_bundleModule == address(0), "ERROR:BTK-003:BUNDLE_MODULE_ALREADY_DEFINED");
         require(bundleModule != address(0), "ERROR:BTK-004:INVALID_BUNDLE_MODULE_ADDRESS");
@@ -70,7 +69,9 @@ contract BundleToken is
         isBurned = tokenId <= _tokens && !_exists(tokenId);
     }
 
-    function exists(uint256 tokenId) external override view returns(bool) { return tokenId <= _tokens; }
     function getBundleId(uint256 tokenId) external override view returns(uint256) { return _bundleId[tokenId]; }
+    function getBundleModuleAddress() external view returns(address) { return _bundleModule; }
+
+    function exists(uint256 tokenId) external override view returns(bool) { return tokenId <= _tokens; }
     function tokens() external override view returns(uint256 tokenCount) { return _tokens; }
 }

scripts/instance.py

@@ -156,13 +156,13 @@ def deployWithRegistry(
         # TODO these contracts do not work with proxy pattern
         self.productService = deployGifService(ProductService, registry, owner, publishSource)
 
-        # needs to be the last module to register as it will change
-        # the address of the instance operator service to its true address
+        # needs to be the last module to register as it will 
+        # perform some post deploy wirings and changes the address 
+        # of the instance operator service to its true address
         self.instanceOperatorService = deployGifModuleV2("InstanceOperatorService", InstanceOperatorService, registry, owner, publishSource)
 
         # post deploy wiring steps
-        self.bundleToken.setBundleModule(self.bundle)
-        self.access.setDefaultAdminRole(self.instanceOperatorService.address, {'from': owner})
+        # self.bundleToken.setBundleModule(self.bundle)
 
 
     def fromRegistryAddress(self, registry_address):

scripts/util.py

@@ -30,6 +30,9 @@ def s2b(text:str):
 def b2s(b32: bytes):
     return b322s(b32)
 
+def keccak256(text:str):
+    return Web3.solidityKeccak(['string'], [text]).hex()
+
 def get_account(mnemonic: str, account_offset: int) -> Account:
     return accounts.from_mnemonic(
         mnemonic,

tests/test_access_module.py

@@ -0,0 +1,175 @@
+import brownie
+import pytest
+
+from brownie import AccessController
+
+from scripts.util import (
+    keccak256, 
+    s2b32,
+    contractFromAddress
+)
+
+# enforce function isolation for tests below
+@pytest.fixture(autouse=True)
+def isolation(fn_isolation):
+    pass
+
+def test_initial_setup(
+    instance,
+    owner,
+    productOwner,
+    oracleProvider,
+    riskpoolKeeper,
+):
+    instanceService = instance.getInstanceService()
+    instanceOperatorService = instance.getInstanceOperatorService()
+    (poRole, opRole, rkRole, daRole) = getRoles(instanceService)
+
+    assert poRole != opRole
+    assert poRole != rkRole
+    assert poRole != daRole
+    assert opRole != rkRole
+    assert opRole != daRole
+    assert rkRole != daRole
+
+    assert poRole == keccak256('PRODUCT_OWNER_ROLE')
+    assert opRole == keccak256('ORACLE_PROVIDER_ROLE')
+    assert rkRole == keccak256('RISKPOOL_KEEPER_ROLE')
+
+    # check component owners against their roles
+    assert not instanceService.hasRole(poRole, productOwner)
+    assert not instanceService.hasRole(opRole, oracleProvider)
+    assert not instanceService.hasRole(rkRole, riskpoolKeeper)
+
+    # check default role assignemnt
+    assert instanceService.hasRole(daRole, instanceOperatorService.address)
+
+
+def test_default_admin_role(
+    instance,
+    owner,
+    productOwner,
+    oracleProvider,
+    riskpoolKeeper,
+):
+    instanceOperatorService = instance.getInstanceOperatorService()
+    instanceService = instance.getInstanceService()
+    ioDict = {'from': owner}
+
+    (poRole, opRole, rkRole, daRole) = getRoles(instanceService)
+
+    # check default admin role assignemnt to instance operator service
+    assert instanceService.hasRole(daRole, instanceOperatorService.address)
+
+    registry = instance.getRegistry()
+    access = contractFromAddress(AccessController, registry.getContract(s2b32('Access')))
+
+    # check that 'random' accaounts can't re-assign the admin role
+    with brownie.reverts('ERROR:ACL-001:ADMIN_ROLE_ALREADY_SET'):
+        access.setDefaultAdminRole(productOwner, {'from': productOwner})
+
+    # check that not even the instance operator can change the role assignment
+    with brownie.reverts('ERROR:ACL-001:ADMIN_ROLE_ALREADY_SET'):
+        access.setDefaultAdminRole(productOwner, ioDict)
+
+
+def test_role_assignment(
+    instance,
+    owner,
+    productOwner,
+    oracleProvider,
+    riskpoolKeeper,
+):
+    instanceOperatorService = instance.getInstanceOperatorService()
+    instanceService = instance.getInstanceService()
+    ioDict = {'from': owner}
+
+    (poRole, opRole, rkRole, daRole) = getRoles(instanceService)
+
+    instanceOperatorService.grantRole(poRole, productOwner, ioDict)
+    instanceOperatorService.grantRole(opRole, oracleProvider, ioDict)
+    instanceOperatorService.grantRole(rkRole, riskpoolKeeper, ioDict)
+
+    assert instanceService.hasRole(poRole, productOwner)
+    assert instanceService.hasRole(opRole, oracleProvider)
+    assert instanceService.hasRole(rkRole, riskpoolKeeper)
+
+    instanceOperatorService.revokeRole(poRole, productOwner, ioDict)
+    instanceOperatorService.revokeRole(opRole, oracleProvider, ioDict)
+    instanceOperatorService.revokeRole(rkRole, riskpoolKeeper, ioDict)
+
+    assert not instanceService.hasRole(poRole, productOwner)
+    assert not instanceService.hasRole(opRole, oracleProvider)
+    assert not instanceService.hasRole(rkRole, riskpoolKeeper)
+
+
+def test_role_creation(
+    instance,
+    owner,
+    productOwner,
+    oracleProvider,
+    riskpoolKeeper,
+):
+    instanceOperatorService = instance.getInstanceOperatorService()
+    instanceService = instance.getInstanceService()
+    ioDict = {'from': owner}
+
+    NEW_ROLE = keccak256('NEW_ROLE')
+
+    # check that unknown roles cannot be granted
+    with brownie.reverts('ERROR:ACL-002:ROLE_UNKNOWN_OR_INVALID'):
+        instanceOperatorService.grantRole(NEW_ROLE, productOwner, ioDict)
+
+    # check that a non instance operator cannot create new role
+    with brownie.reverts('ERROR:IOS-001:NOT_INSTANCE_OPERATOR'):
+        instanceOperatorService.createRole(NEW_ROLE, {'from': productOwner})
+
+    # role creation
+    instanceOperatorService.createRole(NEW_ROLE, ioDict)
+
+    assert not instanceService.hasRole(NEW_ROLE, productOwner)
+
+    # grant newly created role
+    instanceOperatorService.grantRole(NEW_ROLE, productOwner, ioDict)
+
+    # check granting
+    assert instanceService.hasRole(NEW_ROLE, productOwner)
+
+
+def test_role_invalidation(
+    instance,
+    owner,
+    productOwner,
+    oracleProvider,
+    riskpoolKeeper,
+):
+    instanceOperatorService = instance.getInstanceOperatorService()
+    instanceService = instance.getInstanceService()
+    ioDict = {'from': owner}
+
+    (poRole, opRole, rkRole, daRole) = getRoles(instanceService)
+
+    instanceOperatorService.grantRole(poRole, productOwner, ioDict)
+
+    # check that a non instance operator cannot invalidate a role
+    with brownie.reverts('ERROR:IOS-001:NOT_INSTANCE_OPERATOR'):
+        instanceOperatorService.invalidateRole(poRole, {'from': productOwner})
+
+    NEW_ROLE = keccak256('NEW_ROLE')
+
+    with brownie.reverts('ERROR:ACL-004:ROLE_UNKNOWN_OR_INVALID'):
+        instanceOperatorService.invalidateRole(NEW_ROLE, ioDict)
+
+    instanceOperatorService.invalidateRole(poRole, ioDict)
+
+    with brownie.reverts('ERROR:ACL-002:ROLE_UNKNOWN_OR_INVALID'):
+        instanceOperatorService.grantRole(poRole, oracleProvider, ioDict)
+
+
+def getRoles(instanceService):
+    poRole = instanceService.getProductOwnerRole()
+    opRole = instanceService.getOracleProviderRole()
+    rkRole = instanceService.getRiskpoolKeeperRole()
+    daRole = instanceService.getDefaultAdminRole()
+
+    return (poRole, opRole, rkRole, daRole)