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 14 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
15 changes: 15 additions & 0 deletions 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_CONTRACT_CALL_NOT_ALLOWED = "APP_CONTRACT_CALL_NOT_ALLOWED";

modifier auth(bytes32 _role) {
require(canPerform(msg.sender, _role, new uint256[](0)), ERROR_AUTH_FAILED);
Expand All @@ -31,6 +32,12 @@ contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, ReentrancyGua
_;
}

modifier killSwitched {
Copy link
Contributor

@sohkai sohkai May 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we should directly bundle this into auth() or something similar as well. It may be tedious for developers to place this on every external, state-changing call (similar to isInitialized(); this is my envisioned recommendation for apps but correct me if you think devs should have more granularity).

We may also want to find a better name, e.g. killSwitchProtected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may make sense but before coupling them we can keep working on other details and leave that decision for later. We will probably have a better understanding of how the whole kill-switch thing interacts with a DAO once we have finished some of the pending stuff.

OTOH, I like the new name you propose :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, makes sense, although I wonder if, given the gas overhead, it could be useful to have that granularity to only apply the killSwitch to very critical functions.
Nevertheless, one could make the argue that if the function has an ACL protection is likely critical. 🤔

bool _shouldDenyCall = kernel().killSwitch().shouldDenyCallingContract(_baseApp());
require(!_shouldDenyCall, 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
Expand Down Expand Up @@ -65,4 +72,12 @@ contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, ReentrancyGua
// Funds recovery via a vault is only available when used with a kernel
return kernel().getRecoveryVault(); // if kernel is not set, it will revert
}

/**
* @dev Get the address of the base implementation for the current app
* @return Address of the base implementation
*/
function _baseApp() internal view returns (address) {
return kernel().getApp(KERNEL_APP_BASES_NAMESPACE, appId());
}
}
79 changes: 58 additions & 21 deletions contracts/factory/DAOFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ pragma solidity 0.4.24;
import "../kernel/IKernel.sol";
import "../kernel/Kernel.sol";
import "../kernel/KernelProxy.sol";
import "../kill_switch/IKillSwitch.sol";
import "../kill_switch/IIssuesRegistry.sol";

import "../acl/IACL.sol";
import "../acl/ACL.sol";
Expand All @@ -13,6 +15,7 @@ import "./EVMScriptRegistryFactory.sol";
contract DAOFactory {
IKernel public baseKernel;
IACL public baseACL;
IKillSwitch public baseKillSwitch;
EVMScriptRegistryFactory public regFactory;

event DeployDAO(address dao);
Expand All @@ -24,14 +27,22 @@ contract DAOFactory {
* @param _baseACL Base ACL
* @param _regFactory EVMScriptRegistry factory
*/
constructor(IKernel _baseKernel, IACL _baseACL, EVMScriptRegistryFactory _regFactory) public {
constructor(
IKernel _baseKernel,
IACL _baseACL,
IKillSwitch _baseKillSwitch,
EVMScriptRegistryFactory _regFactory
)
public
{
// No need to init as it cannot be killed by devops199
if (address(_regFactory) != address(0)) {
regFactory = _regFactory;
}

baseKernel = _baseKernel;
baseACL = _baseACL;
baseKillSwitch = _baseKillSwitch;
}

/**
Expand All @@ -40,38 +51,64 @@ contract DAOFactory {
* @return Newly created DAO
*/
function newDAO(address _root) public returns (Kernel) {
Kernel dao = Kernel(new KernelProxy(baseKernel));
Kernel dao = _newDAO();

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

ACL acl = ACL(dao.acl());
bytes32 permRole = acl.CREATE_PERMISSIONS_ROLE();
bytes32 appManagerRole = dao.APP_MANAGER_ROLE();
return dao;
}

acl.grantPermission(regFactory, acl, permRole);
/**
* @notice Create a new DAO with `_root` set as the initial admin and `_issuesRegistry` as the source of truth for kill-switch purpose
* @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
* @return Newly created DAO
*/
function newDAOWithKillSwitch(address _root, IIssuesRegistry _issuesRegistry) public returns (Kernel) {
Kernel dao = _newDAO();

acl.createPermission(regFactory, dao, appManagerRole, this);
if (address(regFactory) == address(0)) {
dao.initializeWithKillSwitch(baseACL, _root, baseKillSwitch, _issuesRegistry);
} else {
dao.initializeWithKillSwitch(baseACL, address(this), baseKillSwitch, _issuesRegistry);
_setupNewDaoPermissions(_root, Kernel(dao));
}

EVMScriptRegistry reg = regFactory.newEVMScriptRegistry(dao);
emit DeployEVMScriptRegistry(address(reg));
return dao;
}

// Clean up permissions
// First, completely reset the APP_MANAGER_ROLE
acl.revokePermission(regFactory, dao, appManagerRole);
acl.removePermissionManager(dao, appManagerRole);
function _newDAO() internal returns (Kernel) {
Kernel dao = Kernel(new KernelProxy(baseKernel));
emit DeployDAO(address(dao));
return dao;
}

// 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);
}
function _setupNewDaoPermissions(address _root, Kernel _dao) internal {
ACL acl = ACL(_dao.acl());
bytes32 permRole = acl.CREATE_PERMISSIONS_ROLE();
bytes32 appManagerRole = _dao.APP_MANAGER_ROLE();

emit DeployDAO(address(dao));
acl.grantPermission(regFactory, acl, permRole);

return dao;
acl.createPermission(regFactory, _dao, appManagerRole, this);

EVMScriptRegistry reg = regFactory.newEVMScriptRegistry(_dao);
emit DeployEVMScriptRegistry(address(reg));

// Clean up permissions
// First, completely reset the APP_MANAGER_ROLE
acl.revokePermission(regFactory, _dao, appManagerRole);
acl.removePermissionManager(_dao, appManagerRole);

// 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);
}
}
2 changes: 2 additions & 0 deletions contracts/kernel/IKernel.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
pragma solidity ^0.4.24;

import "../acl/IACL.sol";
import "../kill_switch/IKillSwitch.sol";
import "../common/IVaultRecoverable.sol";


Expand All @@ -16,6 +17,7 @@ interface IKernelEvents {
// This should be an interface, but interfaces can't inherit yet :(
contract IKernel is IKernelEvents, IVaultRecoverable {
function acl() public view returns (IACL);
function killSwitch() public view returns (IKillSwitch);
function hasPermission(address who, address where, bytes32 what, bytes how) public view returns (bool);

function setApp(bytes32 namespace, bytes32 appId, address app) public;
Expand Down
32 changes: 32 additions & 0 deletions contracts/kernel/Kernel.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import "../common/IsContract.sol";
import "../common/Petrifiable.sol";
import "../common/VaultRecoverable.sol";
import "../factory/AppProxyFactory.sol";
import "../kill_switch/IKillSwitch.sol";
import "../kill_switch/IIssuesRegistry.sol";
import "../lib/misc/ERCProxy.sol";


Expand Down Expand Up @@ -54,6 +56,27 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant
recoveryVaultAppId = KERNEL_DEFAULT_VAULT_APP_ID;
}

/**
* @dev Initialize can only be called once. It saves the block number in which it was initialized.
* @notice Initialize this kernel instance, its ACL setting `_permissionsCreator` as the entity that can create other permissions, and a KillSwitch instance setting `_issuesRegistry
* @param _baseAcl Address of base ACL app
* @param _permissionsCreator Entity that will be given permission over createPermission
* @param _baseKillSwitch Address of base KillSwitch app
* @param _issuesRegistry Issues registry that will act as the default source of truth to provide info about applications issues
*/
function initializeWithKillSwitch(IACL _baseAcl, address _permissionsCreator, IKillSwitch _baseKillSwitch, IIssuesRegistry _issuesRegistry)
public onlyInit
{
// Set and create ACL app
initialize(_baseAcl, _permissionsCreator);

// Set and create KillSwitch app
_setApp(KERNEL_APP_BASES_NAMESPACE, KERNEL_DEFAULT_KILL_SWITCH_APP_ID, _baseKillSwitch);
IKillSwitch killSwitch = IKillSwitch(newAppProxy(this, KERNEL_DEFAULT_KILL_SWITCH_APP_ID));
killSwitch.initialize(_issuesRegistry);
_setApp(KERNEL_APP_ADDR_NAMESPACE, KERNEL_DEFAULT_KILL_SWITCH_APP_ID, killSwitch);
}

/**
* @dev Create a new instance of an app linked to this kernel
* @notice Create a new upgradeable instance of `_appId` app linked to the Kernel, setting its code to `_appBase`
Expand Down Expand Up @@ -169,6 +192,7 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant
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 Down Expand Up @@ -197,6 +221,14 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant
return IACL(getApp(KERNEL_APP_ADDR_NAMESPACE, KERNEL_DEFAULT_ACL_APP_ID));
}

/**
* @dev Get the 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));
}

/**
* @dev Function called by apps to check ACL on kernel or to check permission status
* @param _who Sender of the original call
Expand Down
2 changes: 2 additions & 0 deletions contracts/kernel/KernelConstants.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@ contract KernelAppIds {
bytes32 internal constant KERNEL_CORE_APP_ID = apmNamehash("kernel");
bytes32 internal constant KERNEL_DEFAULT_ACL_APP_ID = apmNamehash("acl");
bytes32 internal constant KERNEL_DEFAULT_VAULT_APP_ID = apmNamehash("vault");
bytes32 internal constant KERNEL_DEFAULT_KILL_SWITCH_APP_ID = apmNamehash("killSwitch");
*/
bytes32 internal constant KERNEL_CORE_APP_ID = 0x3b4bf6bf3ad5000ecf0f989d5befde585c6860fea3e574a4fab4c49d1c177d9c;
bytes32 internal constant KERNEL_DEFAULT_ACL_APP_ID = 0xe3262375f45a6e2026b7e7b18c2b807434f2508fe1a2a3dfb493c7df8f4aad6a;
bytes32 internal constant KERNEL_DEFAULT_VAULT_APP_ID = 0x7e852e0fcfce6551c13800f1e7476f982525c2b5277ba14b24339c68416336d1;
bytes32 internal constant KERNEL_DEFAULT_KILL_SWITCH_APP_ID = 0x05b6cbc146cecc3a8014843768ab6e17332ef00418da7f6babf4ea94c76ab6a1;
}


Expand Down
16 changes: 16 additions & 0 deletions contracts/kill_switch/IIssuesRegistry.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
pragma solidity 0.4.24;


contract IIssuesRegistry {
enum Severity { None, Low, Mid, High, Critical }

event SeveritySet(address indexed entry, Severity severity, address sender);

function initialize() external;

function setSeverityFor(address entry, Severity severity) external;

function isSeverityFor(address entry) public view returns (bool);

function getSeverityFor(address entry) public view returns (Severity);
}
10 changes: 10 additions & 0 deletions contracts/kill_switch/IKillSwitch.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
pragma solidity 0.4.24;

import "./IIssuesRegistry.sol";


contract IKillSwitch {
function initialize(IIssuesRegistry _issuesRegistry) external;

function shouldDenyCallingContract(address _contract) external returns (bool);
}
28 changes: 28 additions & 0 deletions contracts/kill_switch/IssuesRegistry.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
pragma solidity 0.4.24;

import "../apps/AragonApp.sol";
import "./IIssuesRegistry.sol";


contract IssuesRegistry is IIssuesRegistry, AragonApp {
bytes32 constant public SET_ENTRY_SEVERITY_ROLE = keccak256("SET_ENTRY_SEVERITY_ROLE");

mapping (address => Severity) internal issuesSeverity;

function initialize() external onlyInit {
initialized();
}

function setSeverityFor(address entry, Severity severity) external authP(SET_ENTRY_SEVERITY_ROLE, arr(entry, msg.sender)) {
issuesSeverity[entry] = severity;
emit SeveritySet(entry, severity, msg.sender);
}

function isSeverityFor(address entry) public view isInitialized returns (bool) {
return issuesSeverity[entry] != Severity.None;
}

function getSeverityFor(address entry) public view isInitialized returns (Severity) {
return issuesSeverity[entry];
}
}
Loading