Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kill switch: Polish and integrate MVP version #518

Draft
wants to merge 38 commits into
base: next
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
383dd79
contracts: base kill switch implementation
facuspagnuolo Apr 24, 2019
9d7a118
contracts: binary kill switch implementation
facuspagnuolo Apr 24, 2019
c3f78fd
contracts: severities kill switch implementation
facuspagnuolo Apr 24, 2019
4d9ce01
contracts: application level kill switch
facuspagnuolo Apr 24, 2019
51127c4
contracts: kernel level kill switch
facuspagnuolo Apr 24, 2019
b75fdca
tests: add kill switch unit tests
facuspagnuolo Apr 29, 2019
4f4421e
contracts: use ternary ignoration for kill-switch settings
facuspagnuolo Apr 29, 2019
00b76b9
contracts: fix linting issues
facuspagnuolo Apr 29, 2019
df3568f
kill-switch: cleanup models leaving only app level
facuspagnuolo Apr 30, 2019
b6c5531
kill-switch: use `highest` instead of `lowest` allowed severities
facuspagnuolo Apr 30, 2019
c20c608
kill-switch: Support one issues registry per contract
facuspagnuolo Apr 30, 2019
1f17dcb
kill-switch: drop custom programmatic handling
facuspagnuolo Apr 30, 2019
130c343
kill-switch: integrate with aragonOS components
facuspagnuolo May 1, 2019
3c9eb50
kill-switch: place mocks and test files in corresponding dirs
facuspagnuolo May 2, 2019
ffff170
kill-switch: minor tweaks based on @bingen's feedback
facuspagnuolo May 3, 2019
76f8521
kill-switch: skip gas costs test for coverage
facuspagnuolo May 3, 2019
2db3e46
kill-switch: rename 'ignore' action to 'allow'
facuspagnuolo May 3, 2019
d28cfee
kill-switch: rename issues registry `entry` by `implementation`
facuspagnuolo May 7, 2019
dba7e42
kill-switch: rename test files to follow naming convention
facuspagnuolo May 7, 2019
60268b2
kill-switch: improve authentication params
facuspagnuolo May 7, 2019
8f91fec
kill-switch: fix app id constant value
facuspagnuolo May 7, 2019
1ce842d
kill-switch: improve gas costs and support current kernel versions
facuspagnuolo May 7, 2019
a13d491
kill-switch: move kernel initialization logic to DAO factory
facuspagnuolo May 7, 2019
77b8b2e
kill-switch: rename issues registry `isSeverityFor` to `hasSeverity`
facuspagnuolo May 7, 2019
d499267
kill-switch: rename `killSwitched` modifier to `killSwitchProtected`
facuspagnuolo May 7, 2019
17922cc
kill-switch: improve settings to customize different scenarios
facuspagnuolo May 8, 2019
0f288ce
kill-switch: add `DAOFactory` tests
facuspagnuolo May 8, 2019
a5f6bfe
kill-switch: test non compliant kernel versions
facuspagnuolo May 8, 2019
7f73ac3
kill-switch: allow core instances by default
facuspagnuolo May 8, 2019
5230a86
kill-switch: optimize tests
facuspagnuolo May 8, 2019
fd6348c
kill-switch: parameterize instance being called in kernel
facuspagnuolo May 9, 2019
6ab5f69
Merge branch 'dev' of github.com:aragon/aragonOS into application_kil…
facuspagnuolo May 9, 2019
ae617f6
kill-switch: apply suggestions from @izqui
facuspagnuolo May 17, 2019
ce7a29b
kill-switch: extract `killSwitchProtected` modifier to function
facuspagnuolo May 17, 2019
08f1a8b
kill-switch: address feedback from @sohkai
facuspagnuolo May 20, 2019
5817661
kill-switch: small tweaks and optimizations
facuspagnuolo May 21, 2019
3113233
kill-switch: split unit and integration tests
facuspagnuolo May 21, 2019
c057f0d
kill-switch: rename mocks dir and improve inline doc
facuspagnuolo Jun 10, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions contracts/acl/ACLSyntaxSugar.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
}
}


Expand Down
42 changes: 41 additions & 1 deletion contracts/apps/AragonApp.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ 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_UNEXPECTED_KERNEL_RESPONSE = "APP_UNEXPECTED_KERNEL_RESPONSE";

modifier auth(bytes32 _role) {
require(canPerform(msg.sender, _role, new uint256[](0)), ERROR_AUTH_FAILED);
Expand All @@ -40,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;
}
facuspagnuolo marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -57,6 +58,45 @@ 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 isCallEnabled() public view returns (bool) {
if (!hasInitialized()) {
return false;
}

IKernel _kernel = kernel();
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.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 not, first ensure the returned value is 32 bytes length
uint256 _outputLength;
assembly { _outputLength := returndatasize }
require(_outputLength == 32, ERROR_UNEXPECTED_KERNEL_RESPONSE);
facuspagnuolo marked this conversation as resolved.
Show resolved Hide resolved

// forward returned value
bool _shouldDenyCall;
assembly {
let ptr := mload(0x40) // get next free memory pointer
mstore(0x40, add(ptr, 0x20)) // set next free memory pointer
returndatacopy(ptr, 0, 0x20) // copy call return value
_shouldDenyCall := mload(ptr) // read data
}
return !_shouldDenyCall;
}

/**
* @dev Get the recovery vault for the app
* @return Recovery vault address for the app
Expand Down
147 changes: 115 additions & 32 deletions contracts/factory/DAOFactory.sol
Original file line number Diff line number Diff line change
@@ -1,33 +1,48 @@
pragma solidity 0.4.24;

import "../acl/IACL.sol";
import "../acl/ACL.sol";
import "../kernel/IKernel.sol";
import "../kernel/Kernel.sol";
import "../kernel/KernelProxy.sol";

import "../acl/IACL.sol";
import "../acl/ACL.sol";

import "../kill-switch/KillSwitch.sol";
import "../kill-switch/IssuesRegistry.sol";
import "./EVMScriptRegistryFactory.sol";


contract DAOFactory {
string private constant ERROR_MISSING_BASE_KILL_SWITCH = "DF_MISSING_BASE_KILL_SWITCH";

IKernel public baseKernel;
IACL public baseACL;
KillSwitch public baseKillSwitch;
EVMScriptRegistryFactory public regFactory;

event DeployDAO(address dao);
event DeployEVMScriptRegistry(address reg);
event DeployKillSwitch(address killSwitch);
event DeployEVMScriptRegistry(address registry);
facuspagnuolo marked this conversation as resolved.
Show resolved Hide resolved

/**
* @notice Create a new DAOFactory, creating DAOs with Kernels proxied to `_baseKernel`, ACLs proxied to `_baseACL`, and new EVMScriptRegistries created from `_regFactory`.
* @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 _regFactory EVMScriptRegistry factory
* @param _baseKillSwitch Base KillSwitch
* @param _scriptsRegistryFactory EVMScriptRegistry factory
facuspagnuolo marked this conversation as resolved.
Show resolved Hide resolved
*/
constructor(IKernel _baseKernel, IACL _baseACL, EVMScriptRegistryFactory _regFactory) public {
constructor(
IKernel _baseKernel,
IACL _baseACL,
KillSwitch _baseKillSwitch,
EVMScriptRegistryFactory _scriptsRegistryFactory
)
public
{
// No need to init as it cannot be killed by devops199
if (address(_regFactory) != address(0)) {
regFactory = _regFactory;
if (address(_scriptsRegistryFactory) != address(0)) {
regFactory = _scriptsRegistryFactory;
}
if (address(_baseKillSwitch) != address(0)) {
baseKillSwitch = _baseKillSwitch;
}

baseKernel = _baseKernel;
facuspagnuolo marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -40,38 +55,106 @@ contract DAOFactory {
* @return Newly created DAO
*/
function newDAO(address _root) public returns (Kernel) {
Kernel dao = Kernel(new KernelProxy(baseKernel));

if (address(regFactory) == address(0)) {
dao.initialize(baseACL, _root);
} else {
dao.initialize(baseACL, this);
return _createDAO(_root);
}

ACL acl = ACL(dao.acl());
bytes32 permRole = acl.CREATE_PERMISSIONS_ROLE();
bytes32 appManagerRole = dao.APP_MANAGER_ROLE();
Kernel dao = _createDAO(address(this));
ACL acl = ACL(dao.acl());

acl.grantPermission(regFactory, acl, permRole);
// load roles
bytes32 appManagerRole = dao.APP_MANAGER_ROLE();
bytes32 createPermissionsRole = acl.CREATE_PERMISSIONS_ROLE();

acl.createPermission(regFactory, dao, appManagerRole, this);
// grant app manager permissions to factory and deploy EVM scripts registry
acl.createPermission(regFactory, dao, appManagerRole, address(this));
_createEVMScriptRegistry(dao, acl, createPermissionsRole);

EVMScriptRegistry reg = regFactory.newEVMScriptRegistry(dao);
emit DeployEVMScriptRegistry(address(reg));
// roll back app manager permissions
acl.revokePermission(regFactory, dao, appManagerRole);
acl.removePermissionManager(dao, appManagerRole);

// Clean up permissions
// First, completely reset the APP_MANAGER_ROLE
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 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 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);

// Then, make root the only holder and manager of CREATE_PERMISSIONS_ROLE
acl.revokePermission(regFactory, acl, permRole);
acl.revokePermission(this, acl, permRole);
acl.grantPermission(_root, acl, permRole);
acl.setPermissionManager(_root, acl, permRole);
Kernel dao = _createDAO(address(this));
ACL acl = ACL(dao.acl());

// 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);
}

emit DeployDAO(address(dao));
// 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;
}

function _createDAO(address _permissionsCreator) internal returns (Kernel) {
Kernel dao = Kernel(new KernelProxy(baseKernel));
dao.initialize(baseACL, _permissionsCreator);
emit DeployDAO(address(dao));
return dao;
}

function _createEVMScriptRegistry(Kernel _dao, ACL _acl, bytes32 _createPermissionsRole) internal {
_acl.grantPermission(regFactory, _acl, _createPermissionsRole);
EVMScriptRegistry scriptsRegistry = regFactory.newEVMScriptRegistry(_dao);
emit DeployEVMScriptRegistry(address(scriptsRegistry));
_acl.revokePermission(regFactory, _acl, _createPermissionsRole);
}

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 _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));

// whitelist core instances: kill switch, acl and kernel
_killSwitch.setWhitelistedInstance(address(_dao), true);
_killSwitch.setWhitelistedInstance(address(_acl), true);
_killSwitch.setWhitelistedInstance(address(_killSwitch), true);

// revoke and remove change whitelisted instances role from this
_acl.revokePermission(address(this), _killSwitch, changeWhitelistedInstancesRole);
_acl.removePermissionManager(_killSwitch, changeWhitelistedInstancesRole);
}
facuspagnuolo marked this conversation as resolved.
Show resolved Hide resolved
}
1 change: 1 addition & 0 deletions contracts/kernel/IKernel.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +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 isAppDisabled(bytes32 appId, address _instance) public view returns (bool);
}
31 changes: 29 additions & 2 deletions contracts/kernel/Kernel.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import "../common/IsContract.sol";
import "../common/Petrifiable.sol";
import "../common/VaultRecoverable.sol";
import "../factory/AppProxyFactory.sol";
import "../kill-switch/IKillSwitch.sol";
import "../lib/misc/ERCProxy.sol";


Expand All @@ -20,9 +21,9 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant
*/
bytes32 public constant APP_MANAGER_ROLE = 0xb6d92708f3d4817afc106147d969e229ced5c46e65e0a5002a0d391287762bd0;

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_AUTH_FAILED = "KERNEL_AUTH_FAILED";

/**
* @dev Constructor that allows the deployer to choose if the base instance should be petrified immediately.
Expand Down Expand Up @@ -162,13 +163,31 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant
recoveryVaultAppId = _recoveryVaultAppId;
}

/**
* @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.
facuspagnuolo marked this conversation as resolved.
Show resolved Hide resolved
* @param _appId Identifier for app to be checked
* @return True if the given call should be denied, false otherwise
*/
function isAppDisabled(bytes32 _appId, address _instance) public view returns (bool) {
IKillSwitch _killSwitch = killSwitch();
if (address(_killSwitch) == address(0)) {
return false;
}

address _baseApp = getApp(KERNEL_APP_BASES_NAMESPACE, _appId);
return _killSwitch.shouldDenyCallingApp(_appId, _baseApp, _instance);
}

// External access to default app id and namespace constants to mimic default getters for constants
/* solium-disable function-order, mixedcase */
function CORE_NAMESPACE() external pure returns (bytes32) { return KERNEL_CORE_NAMESPACE; }
function APP_BASES_NAMESPACE() external pure returns (bytes32) { return KERNEL_APP_BASES_NAMESPACE; }
function APP_ADDR_NAMESPACE() external pure returns (bytes32) { return KERNEL_APP_ADDR_NAMESPACE; }
function KERNEL_APP_ID() external pure returns (bytes32) { return KERNEL_CORE_APP_ID; }
function DEFAULT_ACL_APP_ID() external pure returns (bytes32) { return KERNEL_DEFAULT_ACL_APP_ID; }
function DEFAULT_KILL_SWITCH_APP_ID() external pure returns (bytes32) { return KERNEL_DEFAULT_KILL_SWITCH_APP_ID; }
/* solium-enable function-order, mixedcase */

/**
Expand All @@ -190,13 +209,21 @@ 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 default KillSwitch app
* @return KillSwitch app
*/
function killSwitch() public view returns (IKillSwitch) {
return IKillSwitch(getApp(KERNEL_APP_ADDR_NAMESPACE, KERNEL_DEFAULT_KILL_SWITCH_APP_ID));
}

/**
* @dev Function called by apps to check ACL on kernel or to check permission status
* @param _who Sender of the original call
Expand Down
Loading