Kill switch: Polish and integrate MVP version

contracts/acl/ACLSyntaxSugar.sol

@@ -18,14 +18,42 @@ contract ACLSyntaxSugar {
         return arr(uint256(_a), uint256(_b));
     }
 
+    function arr(bytes32 _a, address _b) internal pure returns (uint256[] r) {
+        return arr(uint256(_a), uint256(_b));
+    }
+
+    function arr(bytes32 _a, address _b, address _c) internal pure returns (uint256[] r) {
+        return arr(uint256(_a), uint256(_b), uint256(_c));
+    }
+
+    function arr(bytes32 _a, uint256 _b) internal pure returns (uint256[] r) {
+        return arr(uint256(_a), _b);
+    }
+
+    function arr(bytes32 _a, uint256 _b, uint256 _c) internal pure returns (uint256[] r) {
+        return arr(uint256(_a), _b, _c);
+    }
+
     function arr(address _a) internal pure returns (uint256[] r) {
         return arr(uint256(_a));
     }
 
+    function arr(address _a, bool _b) internal pure returns (uint256[] r) {
+        return arr(uint256(_a), _toUint(_b));
+    }
+
+    function arr(address _a, bool _b, bool _c) internal pure returns (uint256[] r) {
+        return arr(uint256(_a), _toUint(_b), _toUint(_c));
+    }
+
     function arr(address _a, address _b) internal pure returns (uint256[] r) {
         return arr(uint256(_a), uint256(_b));
     }
 
+    function arr(address _a, uint256 _b) internal pure returns (uint256[] r) {
+        return arr(uint256(_a), _b);
+    }
+
     function arr(address _a, uint256 _b, uint256 _c) internal pure returns (uint256[] r) {
         return arr(uint256(_a), _b, _c);
     }
@@ -34,10 +62,6 @@ contract ACLSyntaxSugar {
         return arr(uint256(_a), _b, _c, _d);
     }
 
-    function arr(address _a, uint256 _b) internal pure returns (uint256[] r) {
-        return arr(uint256(_a), uint256(_b));
-    }
-
     function arr(address _a, address _b, uint256 _c, uint256 _d, uint256 _e) internal pure returns (uint256[] r) {
         return arr(uint256(_a), uint256(_b), _c, _d, _e);
     }
@@ -84,6 +108,10 @@ contract ACLSyntaxSugar {
         r[3] = _d;
         r[4] = _e;
     }
+
+    function _toUint(bool _a) private pure returns (uint256) {
+        return _a ? uint256(1) : uint256(0);
+    }
 }
 
 

contracts/apps/AragonApp.sol

@@ -20,7 +20,6 @@ import "../evmscript/EVMScriptRunner.sol";
 // are included so that they are automatically usable by subclassing contracts
 contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, ReentrancyGuard, EVMScriptRunner, ACLSyntaxSugar {
     string private constant ERROR_AUTH_FAILED = "APP_AUTH_FAILED";
-    string private constant ERROR_CONTRACT_CALL_NOT_ALLOWED = "APP_CONTRACT_CALL_NOT_ALLOWED";
     string private constant ERROR_UNEXPECTED_KERNEL_RESPONSE = "APP_UNEXPECTED_KERNEL_RESPONSE";
 
     modifier auth(bytes32 _role) {
@@ -33,11 +32,6 @@ contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, ReentrancyGua
         _;
     }
 
-    modifier killSwitchProtected {
-        require(canExecuteCall(), ERROR_CONTRACT_CALL_NOT_ALLOWED);
-        _;
-    }
-
     /**
     * @dev Check whether an action can be performed by a sender for a particular role on this app
     * @param _sender Sender of the call
@@ -47,7 +41,7 @@ contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, ReentrancyGua
     *         Always returns false if the app hasn't been initialized yet.
     */
     function canPerform(address _sender, bytes32 _role, uint256[] _params) public view returns (bool) {
-        if (!hasInitialized()) {
+        if (!isCallEnabled()) {
             return false;
         }
 
@@ -68,15 +62,24 @@ contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, ReentrancyGua
     * @dev Check whether a call to the current app can be executed or not based on the kill-switch settings
     * @return Boolean indicating whether the call could be executed or not
     */
-    function canExecuteCall() public view returns (bool) {
+    function isCallEnabled() public view returns (bool) {
+        if (!hasInitialized()) {
+            return false;
+        }
+
         IKernel _kernel = kernel();
-        bytes4 selector = _kernel.shouldDenyCallingContract.selector;
-        bytes memory callData = abi.encodeWithSelector(selector, appId(), address(this));
-        bool success = address(_kernel).call(callData);
+        bytes4 selector = _kernel.isAppDisabled.selector;
+        bytes memory isAppDisabledCalldata = abi.encodeWithSelector(selector, appId(), address(this));
+        bool success;
+        assembly {
+            success := staticcall(gas, _kernel, add(isAppDisabledCalldata, 0x20), mload(isAppDisabledCalldata), 0, 0)
+        }
 
-        // if the call to `kernel.shouldDenyCallingApp` reverts (using an old Kernel) we consider that
+        // If the call to `kernel.isAppDisabled()` reverts (using an old or non-existent Kernel) we consider that
         // there is no kill switch and the call can be executed be allowed to continue
-        if (!success) return true;
+        if (!success) {
+            return true;
+        }
 
         // if not, first ensure the returned value is 32 bytes length
         uint256 _outputLength;

contracts/factory/DAOFactory.sol

@@ -5,8 +5,8 @@ import "../acl/ACL.sol";
 import "../kernel/IKernel.sol";
 import "../kernel/Kernel.sol";
 import "../kernel/KernelProxy.sol";
-import "../kill_switch/KillSwitch.sol";
-import "../kill_switch/IssuesRegistry.sol";
+import "../kill-switch/KillSwitch.sol";
+import "../kill-switch/IssuesRegistry.sol";
 import "./EVMScriptRegistryFactory.sol";
 
 
@@ -16,15 +16,17 @@ contract DAOFactory {
     IKernel public baseKernel;
     IACL public baseACL;
     KillSwitch public baseKillSwitch;
-    EVMScriptRegistryFactory public scriptsRegistryFactory;
+    EVMScriptRegistryFactory public regFactory;
 
     event DeployDAO(address dao);
+    event DeployKillSwitch(address killSwitch);
     event DeployEVMScriptRegistry(address registry);
 
     /**
     * @notice Create a new DAOFactory, creating DAOs with Kernels proxied to `_baseKernel`, ACLs proxied to `_baseACL`, and new EVMScriptRegistries created from `_scriptsRegistryFactory`.
     * @param _baseKernel Base Kernel
     * @param _baseACL Base ACL
+    * @param _baseKillSwitch Base KillSwitch
     * @param _scriptsRegistryFactory EVMScriptRegistry factory
     */
     constructor(
@@ -37,7 +39,7 @@ contract DAOFactory {
     {
         // No need to init as it cannot be killed by devops199
         if (address(_scriptsRegistryFactory) != address(0)) {
-            scriptsRegistryFactory = _scriptsRegistryFactory;
+            regFactory = _scriptsRegistryFactory;
         }
         if (address(_baseKillSwitch) != address(0)) {
             baseKillSwitch = _baseKillSwitch;
@@ -53,35 +55,69 @@ contract DAOFactory {
     * @return Newly created DAO
     */
     function newDAO(address _root) public returns (Kernel) {
-        if (address(scriptsRegistryFactory) == address(0)) {
+        if (address(regFactory) == address(0)) {
             return _createDAO(_root);
         }
 
         Kernel dao = _createDAO(address(this));
         ACL acl = ACL(dao.acl());
-        _setupEVMScriptRegistry(dao, acl, _root);
+
+        // load roles
+        bytes32 appManagerRole = dao.APP_MANAGER_ROLE();
+        bytes32 createPermissionsRole = acl.CREATE_PERMISSIONS_ROLE();
+
+        // grant app manager permissions to factory and deploy EVM scripts registry
+        acl.createPermission(regFactory, dao, appManagerRole, address(this));
+        _createEVMScriptRegistry(dao, acl, createPermissionsRole);
+
+        // roll back app manager permissions
+        acl.revokePermission(regFactory, dao, appManagerRole);
+        acl.removePermissionManager(dao, appManagerRole);
+
+        // transfer create permissions roles to root address
+        acl.revokePermission(address(this), acl, createPermissionsRole);
+        acl.grantPermission(_root, acl, createPermissionsRole);
+        acl.setPermissionManager(_root, acl, createPermissionsRole);
+
         return dao;
     }
 
     /**
-    * @notice Create a new DAO with `_root` set as the initial admin and `_issuesRegistry` as the source of truth for kill-switch purpose
+    * @notice Create a new DAO with `_root` set as the initial admin and `_issuesRegistry` as the source of truth for kill-switch purposes
     * @param _root Address that will be granted control to setup DAO permissions
-    * @param _issuesRegistry Address of the registry of issues that will be used in case of critical situations by the kill switch
+    * @param _issuesRegistry Address of the registry of issues that will be used to detect critical situations by the kill switch
     * @return Newly created DAO
     */
     function newDAOWithKillSwitch(address _root, IssuesRegistry _issuesRegistry) public returns (Kernel) {
         require(address(baseKillSwitch) != address(0), ERROR_MISSING_BASE_KILL_SWITCH);
 
         Kernel dao = _createDAO(address(this));
         ACL acl = ACL(dao.acl());
-        _createKillSwitch(dao, acl, _issuesRegistry);
 
-        if (address(scriptsRegistryFactory) == address(0)) {
-            _transferCreatePermissionsRole(dao, acl, address(this), _root);
-        } else {
-            _setupEVMScriptRegistry(dao, acl, _root);
+        // load roles
+        bytes32 appManagerRole = dao.APP_MANAGER_ROLE();
+        bytes32 createPermissionsRole = acl.CREATE_PERMISSIONS_ROLE();
+
+        // grant app manager permissions to this and deploy kill switch
+        acl.createPermission(address(this), dao, appManagerRole, address(this));
+        _createKillSwitch(dao, acl, _issuesRegistry, appManagerRole);
+
+        // deploy EVM scripts registry if required
+        if (address(regFactory) != address(0)) {
+            acl.grantPermission(regFactory, dao, appManagerRole);
+            _createEVMScriptRegistry(dao, acl, createPermissionsRole);
+            acl.revokePermission(regFactory, dao, appManagerRole);
         }
 
+        // roll back app manager permissions
+        acl.revokePermission(address(this), dao, appManagerRole);
+        acl.removePermissionManager(dao, appManagerRole);
+
+        // transfer create permissions roles to root address
+        acl.revokePermission(address(this), acl, createPermissionsRole);
+        acl.grantPermission(_root, acl, createPermissionsRole);
+        acl.setPermissionManager(_root, acl, createPermissionsRole);
+
         return dao;
     }
 
@@ -92,75 +128,33 @@ contract DAOFactory {
         return dao;
     }
 
-    function _createKillSwitch(Kernel _dao, ACL _acl, IssuesRegistry _issuesRegistry) internal {
-        // create app manager role for this
-        _createAppManagerRole(_dao, _acl, address(this));
-
-        // create kill switch instance and set it as default
-        bytes32 killSwitchAppID = _dao.DEFAULT_KILL_SWITCH_APP_ID();
-        bytes memory initializeData = abi.encodeWithSelector(baseKillSwitch.initialize.selector, _issuesRegistry);
-        _dao.newAppInstance(killSwitchAppID, baseKillSwitch, initializeData, true);
-        _allowKillSwitchCoreInstances(_dao, _acl);
-
-        // remove app manager role permissions from this
-        _removeAppManagerRole(_dao, _acl, address(this));
-    }
-
-    function _allowKillSwitchCoreInstances(Kernel _dao, ACL _acl) internal {
-        KillSwitch killSwitch = KillSwitch(_dao.killSwitch());
-
-        // create allow instances role for this
-        bytes32 setAllowedInstancesRole = killSwitch.SET_ALLOWED_INSTANCES_ROLE();
-        _acl.createPermission(address(this), killSwitch, setAllowedInstancesRole, address(this));
-
-        // allow calls to core instances: kill switch, acl and kernel
-        killSwitch.setAllowedInstance(address(_dao), true);
-        killSwitch.setAllowedInstance(address(_acl), true);
-        killSwitch.setAllowedInstance(address(killSwitch), true);
-
-        // remove allow instances role from this
-        _acl.revokePermission(address(this), killSwitch, setAllowedInstancesRole);
-        _acl.removePermissionManager(killSwitch, setAllowedInstancesRole);
-    }
-
-    function _setupEVMScriptRegistry(Kernel _dao, ACL _acl, address _root) internal {
-        // grant permissions to script registry factory
-        _grantCreatePermissionsRole(_dao, _acl, scriptsRegistryFactory);
-        _createAppManagerRole(_dao, _acl, scriptsRegistryFactory);
-
-        // create evm scripts registry
-        EVMScriptRegistry scriptsRegistry = scriptsRegistryFactory.newEVMScriptRegistry(_dao);
+    function _createEVMScriptRegistry(Kernel _dao, ACL _acl, bytes32 _createPermissionsRole) internal {
+        _acl.grantPermission(regFactory, _acl, _createPermissionsRole);
+        EVMScriptRegistry scriptsRegistry = regFactory.newEVMScriptRegistry(_dao);
         emit DeployEVMScriptRegistry(address(scriptsRegistry));
-
-        // remove permissions from scripts registry factory and transfer to root address
-        _removeAppManagerRole(_dao, _acl, scriptsRegistryFactory);
-        _transferCreatePermissionsRole(_dao, _acl, scriptsRegistryFactory, _root);
-    }
-
-    function _grantCreatePermissionsRole(Kernel _dao, ACL _acl, address _to) internal {
-        bytes32 createPermissionsRole = _acl.CREATE_PERMISSIONS_ROLE();
-        _acl.grantPermission(_to, _acl, createPermissionsRole);
+        _acl.revokePermission(regFactory, _acl, _createPermissionsRole);
     }
 
-    function _createAppManagerRole(Kernel _dao, ACL _acl, address _to) internal {
-        bytes32 appManagerRole = _dao.APP_MANAGER_ROLE();
-        _acl.createPermission(_to, _dao, appManagerRole, address(this));
+    function _createKillSwitch(Kernel _dao, ACL _acl, IssuesRegistry _issuesRegistry, bytes32 _appManagerRole) internal {
+        bytes32 killSwitchAppID = _dao.DEFAULT_KILL_SWITCH_APP_ID();
+        bytes memory initializeData = abi.encodeWithSelector(baseKillSwitch.initialize.selector, _issuesRegistry);
+        KillSwitch killSwitch = KillSwitch(_dao.newAppInstance(killSwitchAppID, baseKillSwitch, initializeData, true));
+        _allowKillSwitchCoreInstances(_dao, _acl, killSwitch);
+        emit DeployKillSwitch(address(killSwitch));
     }
 
-    function _removeAppManagerRole(Kernel _dao, ACL _acl, address _from) internal {
-        bytes32 appManagerRole = _dao.APP_MANAGER_ROLE();
-        _acl.revokePermission(_from, _dao, appManagerRole);
-        _acl.removePermissionManager(_dao, appManagerRole);
-    }
+    function _allowKillSwitchCoreInstances(Kernel _dao, ACL _acl, KillSwitch _killSwitch) internal {
+        // create change whitelisted instances role for this
+        bytes32 changeWhitelistedInstancesRole = _killSwitch.CHANGE_WHITELISTED_INSTANCES_ROLE();
+        _acl.createPermission(address(this), _killSwitch, changeWhitelistedInstancesRole, address(this));
 
-    function _transferCreatePermissionsRole(Kernel _dao, ACL _acl, address _from, address _to) internal {
-        bytes32 createPermissionsRole = _acl.CREATE_PERMISSIONS_ROLE();
-        _acl.revokePermission(_from, _acl, createPermissionsRole);
-        if (_from != address(this)) {
-            _acl.revokePermission(address(this), _acl, createPermissionsRole);
-        }
+        // whitelist core instances: kill switch, acl and kernel
+        _killSwitch.setWhitelistedInstance(address(_dao), true);
+        _killSwitch.setWhitelistedInstance(address(_acl), true);
+        _killSwitch.setWhitelistedInstance(address(_killSwitch), true);
 
-        _acl.grantPermission(_to, _acl, createPermissionsRole);
-        _acl.setPermissionManager(_to, _acl, createPermissionsRole);
+        // revoke and remove change whitelisted instances role from this
+        _acl.revokePermission(address(this), _killSwitch, changeWhitelistedInstancesRole);
+        _acl.removePermissionManager(_killSwitch, changeWhitelistedInstancesRole);
     }
 }

contracts/kernel/IKernel.sol

@@ -20,5 +20,5 @@ contract IKernel is IKernelEvents, IVaultRecoverable {
 
     function setApp(bytes32 namespace, bytes32 appId, address app) public;
     function getApp(bytes32 namespace, bytes32 appId) public view returns (address);
-    function shouldDenyCallingContract(bytes32 appId, address _instance) public returns (bool);
+    function isAppDisabled(bytes32 appId, address _instance) public view returns (bool);
 }

contracts/kernel/Kernel.sol

@@ -10,7 +10,7 @@ import "../common/IsContract.sol";
 import "../common/Petrifiable.sol";
 import "../common/VaultRecoverable.sol";
 import "../factory/AppProxyFactory.sol";
-import "../kill_switch/IKillSwitch.sol";
+import "../kill-switch/IKillSwitch.sol";
 import "../lib/misc/ERCProxy.sol";
 
 
@@ -164,11 +164,13 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant
     }
 
     /**
-    * @dev Tells whether a call to an instance of an app should be denied or not based on the kill-switch settings
+    * @dev Tells whether a call to an instance of an app should be denied or not based on the kill-switch settings.
+    *      Note that we don't need to perform an initialization check since having or not a kill-switch installed
+    *      implicitly means that.
     * @param _appId Identifier for app to be checked
-    * @return True is the given call should be denied, false otherwise
+    * @return True if the given call should be denied, false otherwise
     */
-    function shouldDenyCallingContract(bytes32 _appId, address _instance) public returns (bool) {
+    function isAppDisabled(bytes32 _appId, address _instance) public view returns (bool) {
         IKillSwitch _killSwitch = killSwitch();
         if (address(_killSwitch) == address(0)) {
             return false;
@@ -207,15 +209,15 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant
     }
 
     /**
-    * @dev Get the installed ACL app
+    * @dev Get the default ACL app
     * @return ACL app
     */
     function acl() public view returns (IACL) {
         return IACL(getApp(KERNEL_APP_ADDR_NAMESPACE, KERNEL_DEFAULT_ACL_APP_ID));
     }
 
     /**
-    * @dev Get the installed KillSwitch app
+    * @dev Get the default KillSwitch app
     * @return KillSwitch app
     */
     function killSwitch() public view returns (IKillSwitch) {

contracts/kill_switch/IIssuesRegistry.sol → contracts/kill-switch/IIssuesRegistry.sol

@@ -4,7 +4,7 @@ pragma solidity 0.4.24;
 contract IIssuesRegistry {
     enum Severity { None, Low, Mid, High, Critical }
 
-    event SeveritySet(address indexed implementation, Severity severity, address indexed sender);
+    event ChangeSeverity(address indexed implementation, Severity severity, address indexed sender);
 
     function setSeverityFor(address implementation, Severity severity) external;