Kill switch: provide emergency upgrade path

contracts/kernel/Kernel.sol

@@ -18,12 +18,16 @@ import "../lib/misc/ERCProxy.sol";
 contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstants, Petrifiable, IsContract, VaultRecoverable, AppProxyFactory, ACLSyntaxSugar {
     /* Hardcoded constants to save gas
     bytes32 public constant APP_MANAGER_ROLE = keccak256("APP_MANAGER_ROLE");
+    bytes32 public constant APP_MANAGER_EMERGENCY_ROLE = keccak256("APP_MANAGER_EMERGENCY_ROLE");
     */
     bytes32 public constant APP_MANAGER_ROLE = 0xb6d92708f3d4817afc106147d969e229ced5c46e65e0a5002a0d391287762bd0;
+    bytes32 public constant APP_MANAGER_EMERGENCY_ROLE = 0xabe3d63c29e614f79a410086a986fb228d44252d5fd75e80688f4621afcf212a;
 
     string private constant ERROR_AUTH_FAILED = "KERNEL_AUTH_FAILED";
     string private constant ERROR_APP_NOT_CONTRACT = "KERNEL_APP_NOT_CONTRACT";
     string private constant ERROR_INVALID_APP_CHANGE = "KERNEL_INVALID_APP_CHANGE";
+    string private constant ERROR_MISSING_KILL_SWITCH = "KERNEL_MISSING_KILL_SWITCH";
+    string private constant ERROR_BAD_EMERGENCY_UPDATE = "KERNEL_BAD_EMERGENCY_UPDATE";
 
     /**
     * @dev Constructor that allows the deployer to choose if the base instance should be petrified immediately.
@@ -152,6 +156,33 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant
         _setApp(_namespace, _appId, _app);
     }
 
+    /**
+    * @notice Set the resolving address of `_appId` in namespace `_namespace` to `_newBaseApp`
+    * @dev Set the resolving address of a base implementation on emergency. This is entry point works as a particular
+    *      case of `setApp(bytes32,bytes32,address)` that is meant to be used only when the base app to be updated is
+    *      compromised based on the kill switch of the current kernel. This function will perform the proposed update
+    *      only when the current base app address is disabled by the kill-switch and the new proposed one is not.
+    *      Note that this function is not expected to be used for the app addresses namespace, since the kill switch is
+    *      supposed to work with a whitelist of app instances but a blacklist of base implementations.
+    * @param _namespace App namespace to use
+    * @param _appId Identifier for app
+    * @param _newBaseApp Address of the new proposed base app implementation
+    */
+    function setAppOnEmergency(bytes32 _namespace, bytes32 _appId, address _newBaseApp)
+        public
+        auth(APP_MANAGER_EMERGENCY_ROLE, arr(_namespace, _appId))
+    {
+        IKillSwitch _killSwitch = killSwitch();
+        require(address(_killSwitch) != address(0), ERROR_MISSING_KILL_SWITCH);
+
+        address _currentBaseApp = _getApp(_namespace, _appId);
+        bool _isCurrentBaseAppEnabled = _killSwitch.shouldDenyCallingBaseApp(_appId, _currentBaseApp);
+        bool _isNewBaseAppEnabled = _killSwitch.shouldDenyCallingBaseApp(_appId, _newBaseApp);
+        require(_isCurrentBaseAppEnabled && !_isNewBaseAppEnabled, ERROR_BAD_EMERGENCY_UPDATE);
+
+        _setApp(_namespace, _appId, _newBaseApp);
+    }
+
     /**
     * @dev Set the default vault id for the escape hatch mechanism
     * @param _recoveryVaultAppId Identifier of the recovery vault app
@@ -167,6 +198,7 @@ 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.
     *      Initialization check is implicitly provided by the KillSwitch's existence, as apps can only be installed after initialization.
     * @param _appId Identifier for app to be checked
+    * @param _instance Address of the app instance to be checked
     * @return True if the given call should be denied, false otherwise
     */
     function isAppDisabled(bytes32 _appId, address _instance) public view returns (bool) {
@@ -196,31 +228,31 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant
     * @return Address of the app
     */
     function getApp(bytes32 _namespace, bytes32 _appId) public view returns (address) {
-        return apps[_namespace][_appId];
+        return _getApp(_namespace, _appId);
     }
 
     /**
     * @dev Get the address of the recovery Vault instance (to recover funds)
     * @return Address of the Vault
     */
     function getRecoveryVault() public view returns (address) {
-        return apps[KERNEL_APP_ADDR_NAMESPACE][recoveryVaultAppId];
+        return _getInstalledApp(recoveryVaultAppId);
     }
 
     /**
-    * @dev Get the default ACL app
+    * @dev Get the default installed ACL app
     * @return ACL app
     */
     function acl() public view returns (IACL) {
-        return IACL(getApp(KERNEL_APP_ADDR_NAMESPACE, KERNEL_DEFAULT_ACL_APP_ID));
+        return IACL(_getInstalledApp(KERNEL_DEFAULT_ACL_APP_ID));
     }
 
     /**
-    * @dev Get the default KillSwitch app
+    * @dev Get the default installed KillSwitch app
     * @return KillSwitch app
     */
     function killSwitch() public view returns (IKillSwitch) {
-        return IKillSwitch(getApp(KERNEL_APP_ADDR_NAMESPACE, KERNEL_DEFAULT_KILL_SWITCH_APP_ID));
+        return IKillSwitch(_getInstalledApp(KERNEL_DEFAULT_KILL_SWITCH_APP_ID));
     }
 
     /**
@@ -238,6 +270,18 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant
             defaultAcl.hasPermission(_who, _where, _what, _how);
     }
 
+    function _getBaseApp(bytes32 _appId) internal view returns (address) {
+        return _getApp(KERNEL_APP_BASES_NAMESPACE, _appId);
+    }
+
+    function _getInstalledApp(bytes32 _appId) internal view returns (address) {
+        return _getApp(KERNEL_APP_ADDR_NAMESPACE, _appId);
+    }
+
+    function _getApp(bytes32 _namespace, bytes32 _appId) internal view returns (address) {
+        return apps[_namespace][_appId];
+    }
+
     function _setApp(bytes32 _namespace, bytes32 _appId, address _app) internal {
         require(isContract(_app), ERROR_APP_NOT_CONTRACT);
         apps[_namespace][_appId] = _app;

contracts/kill-switch/IKillSwitch.sol

@@ -4,5 +4,6 @@ import "./IIssuesRegistry.sol";
 
 
 contract IKillSwitch {
-    function shouldDenyCallingApp(bytes32 _appId, address _base, address _proxy) external view returns (bool);
+    function shouldDenyCallingApp(bytes32 _appId, address _base, address _instance) external view returns (bool);
+    function shouldDenyCallingBaseApp(bytes32 _appId, address _base) public view returns (bool);
 }

contracts/kill-switch/KillSwitch.sol

@@ -91,17 +91,22 @@ contract KillSwitch is IKillSwitch, IsContract, AragonApp {
      *      and we have several tests to make sure its usage is working as expected.
      */
     function shouldDenyCallingApp(bytes32 _appId, address _base, address _instance) external view returns (bool) {
-        // if the instance is the kill switch itself, then allow given call
+        // If the instance is the kill switch itself, then allow given call
         if (_instance == address(this)) {
             return false;
         }
 
-        // if the instance is whitelisted, then allow given call
+        // If the instance is whitelisted, then allow given call
         if (isInstanceWhitelisted(_instance)) {
             return false;
         }
 
-        // if the base implementation is blacklisted, then deny given call
+        // Check if the base implementation is allowed
+        return shouldDenyCallingBaseApp(_appId, _base);
+    }
+
+    function shouldDenyCallingBaseApp(bytes32 _appId, address _base) public view returns (bool) {
+        // If the base implementation is blacklisted, then deny given call
         if (isBaseImplementationBlacklisted(_base)) {
             return true;
         }

contracts/test/mocks/common/KeccakConstants.sol

@@ -18,6 +18,7 @@ contract KeccakConstants {
     bytes32 public constant KERNEL_APP_ADDR_NAMESPACE = keccak256(abi.encodePacked("app"));
 
     bytes32 public constant APP_MANAGER_ROLE = keccak256(abi.encodePacked("APP_MANAGER_ROLE"));
+    bytes32 public constant APP_MANAGER_EMERGENCY_ROLE = keccak256(abi.encodePacked("APP_MANAGER_EMERGENCY_ROLE"));
 
     bytes32 public constant KERNEL_APP_ID = keccak256(abi.encodePacked(APM_NODE, keccak256("kernel")));
     bytes32 public constant DEFAULT_ACL_APP_ID = keccak256(abi.encodePacked(APM_NODE, keccak256("acl")));

contracts/test/mocks/kill-switch/EmergencyAppManagerMock.sol

@@ -0,0 +1,38 @@
+pragma solidity 0.4.24;
+
+import "../../../kernel/Kernel.sol";
+import "../../../apps/AragonApp.sol";
+
+
+/**
+* @title EmergencyAppManagerMock
+* @dev This mock mimics a contract that is allowed to manage contract upgrades in emergency situations
+*/
+contract EmergencyAppManagerMock is AragonApp {
+    bytes32 public constant APP_MANAGER_EMERGENCY_ROLE = keccak256("APP_MANAGER_EMERGENCY_ROLE");
+
+    function initialize() external onlyInit {
+        initialized();
+    }
+
+    function setAppOnEmergency(bytes32 _namespace, bytes32 _appId, address _app) public auth(APP_MANAGER_EMERGENCY_ROLE) {
+        Kernel(kernel()).setAppOnEmergency(_namespace, _appId, _app);
+    }
+}
+
+
+/**
+* @title AppManagerMock
+* @dev This mock mimics a contract that is allowed to manage contract upgrades in non-emergency situations
+*/
+contract AppManagerMock is AragonApp {
+    bytes32 public constant APP_MANAGER_ROLE = keccak256("APP_MANAGER_ROLE");
+
+    function initialize() external onlyInit {
+        initialized();
+    }
+
+    function setApp(bytes32 _namespace, bytes32 _appId, address _app) public auth(APP_MANAGER_ROLE) {
+        kernel().setApp(_namespace, _appId, _app);
+    }
+}

contracts/test/mocks/kill-switch/RevertingKillSwitchMock.sol

@@ -9,4 +9,8 @@ contract RevertingKillSwitchMock is KillSwitch {
     function shouldDenyCallingApp(bytes32 _appId, address _base, address _instance) external view returns (bool) {
         revert(ERROR_MESSAGE);
     }
+
+    function shouldDenyCallingBaseApp(bytes32 _appId, address _base) public view returns (bool) {
+        revert(ERROR_MESSAGE);
+    }
 }

test/contracts/common/keccak_constants.js

@@ -34,6 +34,7 @@ contract('Constants', () => {
 
     const kernel = await getContract('Kernel').new(false)
     assert.equal(await kernel.APP_MANAGER_ROLE(), await keccakConstants.APP_MANAGER_ROLE(), "app manager role doesn't match")
+    assert.equal(await kernel.APP_MANAGER_EMERGENCY_ROLE(), await keccakConstants.APP_MANAGER_EMERGENCY_ROLE(), "app manager emergency role doesn't match")
     assert.equal(await kernel.KERNEL_APP_ID(), await keccakConstants.KERNEL_APP_ID(), "app id doesn't match")
     assert.equal(await kernel.DEFAULT_ACL_APP_ID(), await keccakConstants.DEFAULT_ACL_APP_ID(), "default acl id doesn't match")
     assert.equal(await kernel.DEFAULT_KILL_SWITCH_APP_ID(), await keccakConstants.DEFAULT_KILL_SWITCH_APP_ID(), "default kill switch id doesn't match")

test/contracts/kernel/kernel_lifecycle.js

@@ -23,7 +23,7 @@ contract('Kernel lifecycle', ([root, someone]) => {
     assert.isFalse(await kernel.hasPermission(someone, kernel.address, APP_MANAGER_ROLE, EMPTY_BYTES))
 
     await assertRevert(kernel.newAppInstance(APP_ID, appBase.address, EMPTY_BYTES, false))
-    await assertRevert(kernel.newPinnedAppInstance(APP_ID, appBase.address, EMPTY_BYTES, false))
+    await assertRevert(kernel.newPinnedAppInstance(APP_ID, appBase.address))
     await assertRevert(kernel.setApp(APP_BASES_NAMESPACE, APP_ID, appBase.address))
     await assertRevert(kernel.setRecoveryVaultAppId(VAULT_ID))
   }