From 16e236def19c4a16e3e8ac7c0dd0020399c1c760 Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Wed, 24 Apr 2019 11:10:37 -0300 Subject: [PATCH 1/8] contracts: base kill switch implementation --- contracts/kill_switch/base/IssuesRegistry.sol | 31 ++++++++++++ contracts/kill_switch/base/KillSwitch.sol | 48 +++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 contracts/kill_switch/base/IssuesRegistry.sol create mode 100644 contracts/kill_switch/base/KillSwitch.sol diff --git a/contracts/kill_switch/base/IssuesRegistry.sol b/contracts/kill_switch/base/IssuesRegistry.sol new file mode 100644 index 000000000..1cad627dd --- /dev/null +++ b/contracts/kill_switch/base/IssuesRegistry.sol @@ -0,0 +1,31 @@ +pragma solidity 0.4.24; + +import "../../apps/AragonApp.sol"; + + +contract IssuesRegistry is AragonApp { + bytes32 constant public SET_ENTRY_SEVERITY_ROLE = keccak256("SET_ENTRY_SEVERITY_ROLE"); + + enum Severity { None, Low, Mid, High, Critical } + + mapping (address => Severity) internal issuesSeverity; + + event SeveritySet(address indexed entry, Severity severity, address sender); + + function initialize() public onlyInit { + initialized(); + } + + 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]; + } + + function setSeverityFor(address entry, Severity severity) authP(SET_ENTRY_SEVERITY_ROLE, arr(entry, msg.sender)) public { + issuesSeverity[entry] = severity; + emit SeveritySet(entry, severity, msg.sender); + } +} diff --git a/contracts/kill_switch/base/KillSwitch.sol b/contracts/kill_switch/base/KillSwitch.sol new file mode 100644 index 000000000..3aea7d9ec --- /dev/null +++ b/contracts/kill_switch/base/KillSwitch.sol @@ -0,0 +1,48 @@ +pragma solidity 0.4.24; + +import "./IssuesRegistry.sol"; + + +contract KillSwitch { + IssuesRegistry public issuesRegistry; + + event IssuesRegistrySet(address issuesRegistry, address sender); + + function isContractIgnored(address _contract) public view returns (bool); + + function isSeverityIgnored(address _contract, IssuesRegistry.Severity _severity) public view returns (bool); + + function shouldDenyCallingContract(address _base, address _instance, address _sender, bytes _data, uint256 _value) public returns (bool) { + // if the call should not be evaluated, then allow given call + if (!_shouldEvaluateCall(_base, _instance, _sender, _data, _value)) return false; + + // if the contract issues are ignored, then allow given call + if (isContractIgnored(_base)) return false; + + // if the issues registry has not been set, then allow given call + if (issuesRegistry == address(0)) return false; + + // if the contract severity found is ignored, then allow given call + IssuesRegistry.Severity _severityFound = issuesRegistry.getSeverityFor(_base); + if (isSeverityIgnored(_base, _severityFound)) return false; + + // if none of the conditions above were met, then deny given call + return true; + } + + /** + * @dev This function allows different kill-switch implementations to provide a custom logic to tell whether a + * certain call should be denied or not. This is important to ensure recoverability. For example, custom + * implementations could override this function to provide a decision based on the msg.sender, timestamp, + * block information, among many other options. + * @return Always true by default. + */ + function _shouldEvaluateCall(address /*_base*/, address /*_instance*/, address /*_sender*/, bytes /*_data*/, uint256 /*_value*/) internal returns (bool) { + return true; + } + + function _setIssuesRegistry(IssuesRegistry _issuesRegistry) internal { + issuesRegistry = _issuesRegistry; + emit IssuesRegistrySet(_issuesRegistry, msg.sender); + } +} From c29a3c67c2654e04c1b41e935e56965a176efbf4 Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Wed, 24 Apr 2019 11:10:55 -0300 Subject: [PATCH 2/8] contracts: binary kill switch implementation --- .../kill_switch/base/BinaryKillSwitch.sol | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 contracts/kill_switch/base/BinaryKillSwitch.sol diff --git a/contracts/kill_switch/base/BinaryKillSwitch.sol b/contracts/kill_switch/base/BinaryKillSwitch.sol new file mode 100644 index 000000000..1f5623c94 --- /dev/null +++ b/contracts/kill_switch/base/BinaryKillSwitch.sol @@ -0,0 +1,28 @@ +pragma solidity 0.4.24; + +import "./KillSwitch.sol"; +import "./IssuesRegistry.sol"; + + +contract BinaryKillSwitch is KillSwitch { + bytes32 constant public SET_IGNORED_CONTRACTS_ROLE = keccak256("SET_IGNORED_CONTRACTS_ROLE"); + + mapping (address => bool) internal ignoredContracts; + + event ContractIgnored(address _contract, bool ignored); + + function setContractIgnore(address _contract, bool _ignored) external; + + function isContractIgnored(address _contract) public view returns (bool) { + return ignoredContracts[_contract]; + } + + function isSeverityIgnored(address /*_contract*/, IssuesRegistry.Severity _severity) public view returns (bool) { + return _severity == IssuesRegistry.Severity.None; + } + + function _setContractIgnore(address _contract, bool _ignored) internal { + ignoredContracts[_contract] = _ignored; + emit ContractIgnored(_contract, _ignored); + } +} From fc567a3d8e6fe8bed8de41448cb191d338d52984 Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Wed, 24 Apr 2019 11:11:19 -0300 Subject: [PATCH 3/8] contracts: severities kill switch implementation --- .../kill_switch/base/SeveritiesKillSwitch.sol | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 contracts/kill_switch/base/SeveritiesKillSwitch.sol diff --git a/contracts/kill_switch/base/SeveritiesKillSwitch.sol b/contracts/kill_switch/base/SeveritiesKillSwitch.sol new file mode 100644 index 000000000..b2912e5d1 --- /dev/null +++ b/contracts/kill_switch/base/SeveritiesKillSwitch.sol @@ -0,0 +1,29 @@ +pragma solidity 0.4.24; + +import "./KillSwitch.sol"; +import "./IssuesRegistry.sol"; + + +contract SeveritiesKillSwitch is KillSwitch { + bytes32 constant public SET_LOWEST_ALLOWED_SEVERITY_ROLE = keccak256("SET_LOWEST_ALLOWED_SEVERITY_ROLE"); + + mapping (address => IssuesRegistry.Severity) internal lowestAllowedSeverityByContract; + + event LowestAllowedSeveritySet(address indexed _contract, IssuesRegistry.Severity severity); + + function setLowestAllowedSeverity(address _contract, IssuesRegistry.Severity _severity) external; + + function isContractIgnored(address _contract) public view returns (bool) { + return lowestAllowedSeverityByContract[_contract] == IssuesRegistry.Severity.None; + } + + function isSeverityIgnored(address _contract, IssuesRegistry.Severity _severity) public view returns (bool) { + IssuesRegistry.Severity lowestAllowedSeverity = lowestAllowedSeverityByContract[_contract]; + return lowestAllowedSeverity > _severity; + } + + function _setLowestAllowedSeverity(address _contract, IssuesRegistry.Severity _severity) internal { + lowestAllowedSeverityByContract[_contract] = _severity; + emit LowestAllowedSeveritySet(_contract, _severity); + } +} From d8b2ef45750da6cf3872a3bcfe31550d85c3fb34 Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Wed, 24 Apr 2019 11:11:36 -0300 Subject: [PATCH 4/8] contracts: application level kill switch --- .../kill_switch/app/AppBinaryKillSwitch.sol | 14 + contracts/kill_switch/app/AppKillSwitch.sol | 15 + .../app/AppSeveritiesKillSwitch.sol | 14 + contracts/kill_switch/app/KillSwitchedApp.sol | 26 ++ .../app/AppKillSwitchedAppMock.sol | 27 ++ .../app/BinaryKillSwitchedAppMock.sol | 21 + .../app/SeveritiesKillSwitchedAppMock.sol | 21 + .../app/AppBinaryKillSwitch.test.js | 283 ++++++++++++ .../app/AppSeveritiesKillSwitch.test.js | 421 ++++++++++++++++++ 9 files changed, 842 insertions(+) create mode 100644 contracts/kill_switch/app/AppBinaryKillSwitch.sol create mode 100644 contracts/kill_switch/app/AppKillSwitch.sol create mode 100644 contracts/kill_switch/app/AppSeveritiesKillSwitch.sol create mode 100644 contracts/kill_switch/app/KillSwitchedApp.sol create mode 100644 contracts/test/mocks/kill_switch/app/AppKillSwitchedAppMock.sol create mode 100644 contracts/test/mocks/kill_switch/app/BinaryKillSwitchedAppMock.sol create mode 100644 contracts/test/mocks/kill_switch/app/SeveritiesKillSwitchedAppMock.sol create mode 100644 test/kill_switch/app/AppBinaryKillSwitch.test.js create mode 100644 test/kill_switch/app/AppSeveritiesKillSwitch.test.js diff --git a/contracts/kill_switch/app/AppBinaryKillSwitch.sol b/contracts/kill_switch/app/AppBinaryKillSwitch.sol new file mode 100644 index 000000000..fce46e780 --- /dev/null +++ b/contracts/kill_switch/app/AppBinaryKillSwitch.sol @@ -0,0 +1,14 @@ +pragma solidity 0.4.24; + +import "./AppKillSwitch.sol"; +import "../base/BinaryKillSwitch.sol"; + + +contract AppBinaryKillSwitch is AppKillSwitch, BinaryKillSwitch { + function setContractIgnore(address _contract, bool _ignored) + external + authP(SET_IGNORED_CONTRACTS_ROLE, arr(_baseApp(), msg.sender)) + { + _setContractIgnore(_contract, _ignored); + } +} diff --git a/contracts/kill_switch/app/AppKillSwitch.sol b/contracts/kill_switch/app/AppKillSwitch.sol new file mode 100644 index 000000000..a3e5569ff --- /dev/null +++ b/contracts/kill_switch/app/AppKillSwitch.sol @@ -0,0 +1,15 @@ +pragma solidity 0.4.24; + +import "../base/KillSwitch.sol"; + + +contract AppKillSwitch is AragonApp, KillSwitch { + function initialize(IssuesRegistry _issuesRegistry) public onlyInit { + initialized(); + _setIssuesRegistry(_issuesRegistry); + } + + function _baseApp() internal view returns (address) { + return kernel().getApp(KERNEL_APP_BASES_NAMESPACE, appId()); + } +} diff --git a/contracts/kill_switch/app/AppSeveritiesKillSwitch.sol b/contracts/kill_switch/app/AppSeveritiesKillSwitch.sol new file mode 100644 index 000000000..bc9d15d60 --- /dev/null +++ b/contracts/kill_switch/app/AppSeveritiesKillSwitch.sol @@ -0,0 +1,14 @@ +pragma solidity 0.4.24; + +import "./AppKillSwitch.sol"; +import "../base/SeveritiesKillSwitch.sol"; + + +contract AppSeveritiesKillSwitch is AppKillSwitch, SeveritiesKillSwitch { + function setLowestAllowedSeverity(address _contract, IssuesRegistry.Severity _severity) + external + authP(SET_LOWEST_ALLOWED_SEVERITY_ROLE, arr(_baseApp(), msg.sender)) + { + _setLowestAllowedSeverity(_contract, _severity); + } +} diff --git a/contracts/kill_switch/app/KillSwitchedApp.sol b/contracts/kill_switch/app/KillSwitchedApp.sol new file mode 100644 index 000000000..d091433a9 --- /dev/null +++ b/contracts/kill_switch/app/KillSwitchedApp.sol @@ -0,0 +1,26 @@ +pragma solidity 0.4.24; + +import "./AppKillSwitch.sol"; +import "../../apps/AragonApp.sol"; + + +contract KillSwitchedApp is AragonApp { + string private constant ERROR_CONTRACT_CALL_NOT_ALLOWED = "APP_CONTRACT_CALL_NOT_ALLOWED"; + + AppKillSwitch internal appKillSwitch; + + modifier killSwitched { + bool _isCallAllowed = !appKillSwitch.shouldDenyCallingContract(_baseApp(), address(this), msg.sender, msg.data, msg.value); + require(_isCallAllowed, ERROR_CONTRACT_CALL_NOT_ALLOWED); + _; + } + + function initialize(AppKillSwitch _appKillSwitch) public onlyInit { + initialized(); + appKillSwitch = _appKillSwitch; + } + + function _baseApp() internal view returns (address) { + return kernel().getApp(KERNEL_APP_BASES_NAMESPACE, appId()); + } +} diff --git a/contracts/test/mocks/kill_switch/app/AppKillSwitchedAppMock.sol b/contracts/test/mocks/kill_switch/app/AppKillSwitchedAppMock.sol new file mode 100644 index 000000000..2bce791c1 --- /dev/null +++ b/contracts/test/mocks/kill_switch/app/AppKillSwitchedAppMock.sol @@ -0,0 +1,27 @@ +pragma solidity 0.4.24; + +import "../../../../kill_switch/app/KillSwitchedApp.sol"; + + +contract AppKillSwitchedAppMock is KillSwitchedApp { + address public owner; + uint256 internal data; + + function initialize(AppKillSwitch _appKillSwitch, address _owner) public onlyInit { + super.initialize(_appKillSwitch); + data = 42; + owner = _owner; + } + + function read() public view returns (uint256) { + return data; + } + + function write(uint256 _data) public killSwitched { + data = _data; + } + + function reset() public killSwitched { + data = 0; + } +} diff --git a/contracts/test/mocks/kill_switch/app/BinaryKillSwitchedAppMock.sol b/contracts/test/mocks/kill_switch/app/BinaryKillSwitchedAppMock.sol new file mode 100644 index 000000000..d746aad91 --- /dev/null +++ b/contracts/test/mocks/kill_switch/app/BinaryKillSwitchedAppMock.sol @@ -0,0 +1,21 @@ +pragma solidity 0.4.24; + +import "./AppKillSwitchedAppMock.sol"; +import "../../../../kill_switch/app/AppBinaryKillSwitch.sol"; + + +contract AppBinaryKillSwitchMock is AppBinaryKillSwitch { + function _shouldEvaluateCall(address /*_base*/, address _instance, address _sender, bytes _data, uint256 /*_value*/) internal returns (bool) { + bytes4 methodID; + assembly { methodID := mload(add(_data, 0x20)) } + + // since this will act for every tx of the app, we provide a whitelist of functions + AppKillSwitchedAppMock app = AppKillSwitchedAppMock(_instance); + + // if called method is #reset, and the sender is the owner, do not evaluate + if (methodID == app.reset.selector && _sender == app.owner()) return false; + + // evaluate otherwise + return true; + } +} diff --git a/contracts/test/mocks/kill_switch/app/SeveritiesKillSwitchedAppMock.sol b/contracts/test/mocks/kill_switch/app/SeveritiesKillSwitchedAppMock.sol new file mode 100644 index 000000000..bbe682a1e --- /dev/null +++ b/contracts/test/mocks/kill_switch/app/SeveritiesKillSwitchedAppMock.sol @@ -0,0 +1,21 @@ +pragma solidity 0.4.24; + +import "./AppKillSwitchedAppMock.sol"; +import "../../../../kill_switch/app/AppSeveritiesKillSwitch.sol"; + + +contract AppSeveritiesKillSwitchMock is AppSeveritiesKillSwitch { + function _shouldEvaluateCall(address /*_base*/, address _instance, address _sender, bytes _data, uint256 /*_value*/) internal returns (bool) { + bytes4 methodID; + assembly { methodID := mload(add(_data, 0x20)) } + + // since this will act for every tx of the app, we provide a whitelist of functions + AppKillSwitchedAppMock app = AppKillSwitchedAppMock(_instance); + + // if called method is #reset, and the sender is the owner, do not evaluate + if (methodID == app.reset.selector && _sender == app.owner()) return false; + + // evaluate otherwise + return true; + } +} diff --git a/test/kill_switch/app/AppBinaryKillSwitch.test.js b/test/kill_switch/app/AppBinaryKillSwitch.test.js new file mode 100644 index 000000000..5043e3379 --- /dev/null +++ b/test/kill_switch/app/AppBinaryKillSwitch.test.js @@ -0,0 +1,283 @@ +const { assertRevert } = require('../../helpers/assertThrow') + +const IssuesRegistry = artifacts.require('IssuesRegistry') +const KillSwitchedApp = artifacts.require('AppKillSwitchedAppMock') +const AppBinaryKillSwitch = artifacts.require('AppBinaryKillSwitchMock') + +const ACL = artifacts.require('ACL') +const Kernel = artifacts.require('Kernel') +const DAOFactory = artifacts.require('DAOFactory') +const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') + +const SEVERITY = { NONE: 0, LOW: 1, MID: 2, HIGH: 3, CRITICAL: 4 } + +const getEventArgument = (receipt, event, arg) => receipt.logs.find(l => l.event === event).args[arg] + +contract('AppBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { + let kernelBase, aclBase, appBase, appKillSwitchBase, issuesRegistryBase + let registryFactory, dao, acl, issuesRegistry, app, appKillSwitch + + before('deploy base implementations', async () => { + kernelBase = await Kernel.new(true) // petrify immediately + aclBase = await ACL.new() + registryFactory = await EVMScriptRegistryFactory.new() + appKillSwitchBase = await AppBinaryKillSwitch.new() + issuesRegistryBase = await IssuesRegistry.new() + appBase = await KillSwitchedApp.new() + }) + + before('deploy DAO', async () => { + const daoFactory = await DAOFactory.new(kernelBase.address, aclBase.address, registryFactory.address) + const kernelReceipt = await daoFactory.newDAO(root) + dao = Kernel.at(getEventArgument(kernelReceipt, 'DeployDAO', 'dao')) + acl = ACL.at(await dao.acl()) + const APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE() + await acl.createPermission(root, dao.address, APP_MANAGER_ROLE, root, { from: root }) + }) + + beforeEach('create issues registry', async () => { + const issuesRegistryReceipt = await dao.newAppInstance('0x1234', issuesRegistryBase.address, '0x', false, { from: root }) + issuesRegistry = IssuesRegistry.at(getEventArgument(issuesRegistryReceipt, 'NewAppProxy', 'proxy')) + await issuesRegistry.initialize() + const SET_ENTRY_SEVERITY_ROLE = await issuesRegistryBase.SET_ENTRY_SEVERITY_ROLE() + await acl.createPermission(securityPartner, issuesRegistry.address, SET_ENTRY_SEVERITY_ROLE, root, { from: root }) + }) + + beforeEach('create app kill switch', async () => { + const appKillSwitchReceipt = await dao.newAppInstance('0x1235', appKillSwitchBase.address, '0x', false, { from: root }) + appKillSwitch = AppBinaryKillSwitch.at(getEventArgument(appKillSwitchReceipt, 'NewAppProxy', 'proxy')) + await appKillSwitch.initialize(issuesRegistry.address) + const SET_IGNORED_CONTRACTS_ROLE = await appKillSwitchBase.SET_IGNORED_CONTRACTS_ROLE() + await acl.createPermission(owner, appKillSwitch.address, SET_IGNORED_CONTRACTS_ROLE, root, { from: root }) + }) + + beforeEach('create kill switched app', async () => { + const appReceipt = await dao.newAppInstance('0x1236', appBase.address, '0x', false, { from: root }) + app = KillSwitchedApp.at(getEventArgument(appReceipt, 'NewAppProxy', 'proxy')) + await app.initialize(appKillSwitch.address, owner) + }) + + context('when the function being called is not tagged', () => { + const itExecutesTheCall = () => { + it('executes the call', async () => { + assert.equal(await app.read(), 42) + }) + } + + context('when there is no bug registered', () => { + context('when the contract being called is not ignored', () => { + itExecutesTheCall() + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + + context('when there is a bug registered', () => { + beforeEach('register a bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.LOW, { from: securityPartner }) + }) + + context('when the contract being called is not ignored', () => { + itExecutesTheCall() + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + }) + + context('when the function being called is tagged', () => { + context('when the function being called is always evaluated', () => { + const itExecutesTheCall = (from = owner) => { + it('executes the call', async () => { + await app.write(10, { from }) + assert.equal(await app.read(), 10) + }) + } + + const itDoesNotExecuteTheCall = (from = owner) => { + it('does not execute the call', async () => { + await assertRevert(app.write(10, { from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') + }) + } + + context('when there is no bug registered', () => { + context('when the contract being called is not ignored', () => { + itExecutesTheCall() + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + + context('when there is a bug registered', () => { + beforeEach('register a bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.LOW, { from: securityPartner }) + }) + + context('when the bug was not fixed yet', () => { + context('when the contract being called is not ignored', () => { + context('when the sender is the owner', () => { + itDoesNotExecuteTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itDoesNotExecuteTheCall(anyone) + }) + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + }) + + context('when the bug was already fixed', () => { + beforeEach('fix bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) + }) + + context('when the contract being called is not ignored', () => { + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + }) + }) + }) + + context('when the function being called is evaluated only when the sender is not the owner', () => { + const itExecutesTheCall = (from = owner) => { + it('executes the call', async () => { + await app.reset({ from }) + assert.equal(await app.read(), 0) + }) + } + + const itDoesNotExecuteTheCall = (from = owner) => { + it('does not execute the call', async () => { + await assertRevert(app.reset({ from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') + }) + } + + context('when there is no bug registered', () => { + context('when the contract being called is not ignored', () => { + itExecutesTheCall() + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + + context('when there is a bug registered', () => { + beforeEach('register a bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.LOW, { from: securityPartner }) + }) + + context('when the bug was not fixed yet', () => { + context('when the contract being called is not ignored', () => { + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itDoesNotExecuteTheCall(anyone) + }) + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + }) + + context('when the bug was already fixed', () => { + beforeEach('fix bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) + }) + + context('when the contract being called is not ignored', () => { + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + }) + }) + }) + }) +}) diff --git a/test/kill_switch/app/AppSeveritiesKillSwitch.test.js b/test/kill_switch/app/AppSeveritiesKillSwitch.test.js new file mode 100644 index 000000000..117b676c1 --- /dev/null +++ b/test/kill_switch/app/AppSeveritiesKillSwitch.test.js @@ -0,0 +1,421 @@ +const { assertRevert } = require('../../helpers/assertThrow') + +const IssuesRegistry = artifacts.require('IssuesRegistry') +const KillSwitchedApp = artifacts.require('AppKillSwitchedAppMock') +const AppSeveritiesKillSwitch = artifacts.require('AppSeveritiesKillSwitchMock') + +const ACL = artifacts.require('ACL') +const Kernel = artifacts.require('Kernel') +const DAOFactory = artifacts.require('DAOFactory') +const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') + +const SEVERITY = { NONE: 0, LOW: 1, MID: 2, HIGH: 3, CRITICAL: 4 } + +const getEventArgument = (receipt, event, arg) => receipt.logs.find(l => l.event === event).args[arg] + +contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) => { + let kernelBase, aclBase, appBase, appKillSwitchBase, issuesRegistryBase + let registryFactory, dao, acl, issuesRegistry, app, appKillSwitch + + before('deploy base implementations', async () => { + kernelBase = await Kernel.new(true) // petrify immediately + aclBase = await ACL.new() + registryFactory = await EVMScriptRegistryFactory.new() + appKillSwitchBase = await AppSeveritiesKillSwitch.new() + issuesRegistryBase = await IssuesRegistry.new() + appBase = await KillSwitchedApp.new() + }) + + before('deploy DAO', async () => { + const daoFactory = await DAOFactory.new(kernelBase.address, aclBase.address, registryFactory.address) + const kernelReceipt = await daoFactory.newDAO(root) + dao = Kernel.at(getEventArgument(kernelReceipt, 'DeployDAO', 'dao')) + acl = ACL.at(await dao.acl()) + const APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE() + await acl.createPermission(root, dao.address, APP_MANAGER_ROLE, root, { from: root }) + }) + + beforeEach('create issues registry', async () => { + const issuesRegistryReceipt = await dao.newAppInstance('0x1234', issuesRegistryBase.address, '0x', false, { from: root }) + issuesRegistry = IssuesRegistry.at(getEventArgument(issuesRegistryReceipt, 'NewAppProxy', 'proxy')) + await issuesRegistry.initialize() + const SET_ENTRY_SEVERITY_ROLE = await issuesRegistryBase.SET_ENTRY_SEVERITY_ROLE() + await acl.createPermission(securityPartner, issuesRegistry.address, SET_ENTRY_SEVERITY_ROLE, root, { from: root }) + }) + + beforeEach('create app kill switch', async () => { + const appKillSwitchReceipt = await dao.newAppInstance('0x1235', appKillSwitchBase.address, '0x', false, { from: root }) + appKillSwitch = AppSeveritiesKillSwitch.at(getEventArgument(appKillSwitchReceipt, 'NewAppProxy', 'proxy')) + await appKillSwitch.initialize(issuesRegistry.address) + const SET_LOWEST_ALLOWED_SEVERITY_ROLE = await appKillSwitchBase.SET_LOWEST_ALLOWED_SEVERITY_ROLE() + await acl.createPermission(owner, appKillSwitch.address, SET_LOWEST_ALLOWED_SEVERITY_ROLE, root, { from: root }) + }) + + beforeEach('create kill switched app', async () => { + const appReceipt = await dao.newAppInstance('0x1236', appBase.address, '0x', false, { from: root }) + app = KillSwitchedApp.at(getEventArgument(appReceipt, 'NewAppProxy', 'proxy')) + await app.initialize(appKillSwitch.address, owner) + }) + + describe('when the function being called is not tagged', () => { + const itExecutesTheCall = () => { + it('executes the call', async () => { + assert.equal(await app.read(), 42) + }) + } + + context('when there is no bug registered', () => { + context('when there is no lowest allowed severity set for the contract being called', () => { + itExecutesTheCall() + }) + + context('when there is a lowest allowed severity set for the contract being called', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + + context('when there is a bug registered', () => { + beforeEach('register a bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.MID, { from: securityPartner }) + }) + + context('when there is no lowest allowed severity set for the contract being called', () => { + itExecutesTheCall() + }) + + context('when there is a lowest allowed severity set for the contract being called', () => { + context('when there lowest allowed severity is under the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) + + itExecutesTheCall() + }) + + context('when there lowest allowed severity is equal to the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + }) + + itExecutesTheCall() + }) + + context('when there lowest allowed severity is greater than the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + }) + }) + + describe('when the function being called is tagged', () => { + describe('when the function being called is always evaluated', () => { + const itExecutesTheCall = (from = owner) => { + it('executes the call', async () => { + await app.write(10, { from }) + assert.equal(await app.read(), 10) + }) + } + + const itDoesNotExecuteTheCall = (from = owner) => { + it('does not execute the call', async () => { + await assertRevert(app.write(10, { from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') + }) + } + + context('when there is no bug registered', () => { + context('when there is no lowest allowed severity set for the contract being called', () => { + itExecutesTheCall() + }) + + context('when there is a lowest allowed severity set for the contract being called', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + + context('when there is a bug registered', () => { + beforeEach('register a bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.MID, { from: securityPartner }) + }) + + context('when the bug was not fixed yet', () => { + context('when there is no lowest allowed severity set for the contract being called', () => { + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + + context('when there is a lowest allowed severity set for the contract being called', () => { + context('when there lowest allowed severity is under the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) + + context('when the sender is the owner', () => { + itDoesNotExecuteTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itDoesNotExecuteTheCall(anyone) + }) + }) + + context('when there lowest allowed severity is equal to the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + }) + + context('when the sender is the owner', () => { + itDoesNotExecuteTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itDoesNotExecuteTheCall(anyone) + }) + }) + + context('when there lowest allowed severity is greater than the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + }) + }) + + context('when the bug was already fixed', () => { + beforeEach('fix bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) + }) + + context('when there is no lowest allowed severity set for the contract being called', () => { + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + + context('when there is a lowest allowed severity set for the contract being called', () => { + context('when there lowest allowed severity is under the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + + context('when there lowest allowed severity is equal to the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + + context('when there lowest allowed severity is greater than the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + }) + }) + }) + }) + + describe('when the function being called is evaluated only when the sender is not the owner', () => { + const itExecutesTheCall = (from = owner) => { + it('executes the call', async () => { + await app.reset({ from }) + assert.equal(await app.read(), 0) + }) + } + + const itDoesNotExecuteTheCall = (from = owner) => { + it('does not execute the call', async () => { + await assertRevert(app.reset({ from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') + }) + } + + context('when there is no bug registered', () => { + context('when there is no lowest allowed severity set for the contract being called', () => { + itExecutesTheCall() + }) + + context('when there is a lowest allowed severity set for the contract being called', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + + context('when there is a bug registered', () => { + beforeEach('register a bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.MID, { from: securityPartner }) + }) + + context('when the bug was not fixed yet', () => { + context('when there is no lowest allowed severity set for the contract being called', () => { + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + + context('when there is a lowest allowed severity set for the contract being called', () => { + context('when there lowest allowed severity is under the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itDoesNotExecuteTheCall(anyone) + }) + }) + + context('when there lowest allowed severity is equal to the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itDoesNotExecuteTheCall(anyone) + }) + }) + + context('when there lowest allowed severity is greater than the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + }) + }) + + context('when the bug was already fixed', () => { + beforeEach('fix bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) + }) + + context('when there is no lowest allowed severity set for the contract being called', () => { + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + + context('when there is a lowest allowed severity set for the contract being called', () => { + context('when there lowest allowed severity is under the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + + context('when there lowest allowed severity is equal to the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + + context('when there lowest allowed severity is greater than the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + }) + }) + }) + }) + }) +}) From c2060960f02a90eda23a1eb6228ee26bb7cb7bdd Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Wed, 24 Apr 2019 11:12:10 -0300 Subject: [PATCH 5/8] contracts: kernel level kill switch --- contracts/factory/DAOFactory.sol | 61 +++++-- .../kernel/KernelBinaryKillSwitch.sol | 16 ++ .../kill_switch/kernel/KernelKillSwitch.sol | 40 +++++ .../kernel/KernelSeveritiesKillSwitch.sol | 16 ++ .../kernel/KernelKillSwitchAppMock.sol | 27 +++ .../kernel/KernelBinaryKillSwitch.test.js | 128 ++++++++++++++ .../kernel/KernelSeveritiesKillSwitch.test.js | 164 ++++++++++++++++++ 7 files changed, 433 insertions(+), 19 deletions(-) create mode 100644 contracts/kill_switch/kernel/KernelBinaryKillSwitch.sol create mode 100644 contracts/kill_switch/kernel/KernelKillSwitch.sol create mode 100644 contracts/kill_switch/kernel/KernelSeveritiesKillSwitch.sol create mode 100644 contracts/test/mocks/kill_switch/kernel/KernelKillSwitchAppMock.sol create mode 100644 test/kill_switch/kernel/KernelBinaryKillSwitch.test.js create mode 100644 test/kill_switch/kernel/KernelSeveritiesKillSwitch.test.js diff --git a/contracts/factory/DAOFactory.sol b/contracts/factory/DAOFactory.sol index a2eec709a..c88ac9771 100644 --- a/contracts/factory/DAOFactory.sol +++ b/contracts/factory/DAOFactory.sol @@ -3,6 +3,7 @@ pragma solidity 0.4.24; import "../kernel/IKernel.sol"; import "../kernel/Kernel.sol"; import "../kernel/KernelProxy.sol"; +import "../kill_switch/kernel/KernelKillSwitch.sol"; import "../acl/IACL.sol"; import "../acl/ACL.sol"; @@ -46,32 +47,54 @@ contract DAOFactory { 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(); + emit DeployDAO(address(dao)); + 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 kernel kill switch + * @return Newly created DAO + */ + function newDAOWithKillSwitch(address _root, IssuesRegistry _issuesRegistry) public returns (KernelKillSwitch) { + KernelKillSwitch dao = KernelKillSwitch(new KernelProxy(baseKernel)); - acl.createPermission(regFactory, dao, appManagerRole, this); + if (address(regFactory) == address(0)) { + dao.initialize(_issuesRegistry, baseACL, _root); + } else { + dao.initialize(_issuesRegistry, baseACL, address(this)); + _setupNewDaoPermissions(_root, Kernel(dao)); + } - EVMScriptRegistry reg = regFactory.newEVMScriptRegistry(dao); - emit DeployEVMScriptRegistry(address(reg)); + emit DeployDAO(address(dao)); + return dao; + } - // Clean up permissions - // First, completely reset the APP_MANAGER_ROLE - acl.revokePermission(regFactory, dao, appManagerRole); - acl.removePermissionManager(dao, appManagerRole); + function _setupNewDaoPermissions(address _root, Kernel _dao) internal { + ACL acl = ACL(_dao.acl()); + bytes32 permRole = acl.CREATE_PERMISSIONS_ROLE(); + bytes32 appManagerRole = _dao.APP_MANAGER_ROLE(); - // 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); - } + acl.grantPermission(regFactory, acl, permRole); - emit DeployDAO(address(dao)); + acl.createPermission(regFactory, _dao, appManagerRole, this); - return dao; + 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); } } diff --git a/contracts/kill_switch/kernel/KernelBinaryKillSwitch.sol b/contracts/kill_switch/kernel/KernelBinaryKillSwitch.sol new file mode 100644 index 000000000..95ce4c4fc --- /dev/null +++ b/contracts/kill_switch/kernel/KernelBinaryKillSwitch.sol @@ -0,0 +1,16 @@ +pragma solidity 0.4.24; + +import "./KernelKillSwitch.sol"; +import "../base/BinaryKillSwitch.sol"; + + +contract KernelBinaryKillSwitch is KernelKillSwitch, BinaryKillSwitch { + constructor(bool _shouldPetrify) Kernel(_shouldPetrify) public {} + + function setContractIgnore(address _contract, bool _ignored) + external + auth(SET_IGNORED_CONTRACTS_ROLE, arr(_contract, msg.sender)) + { + _setContractIgnore(_contract, _ignored); + } +} diff --git a/contracts/kill_switch/kernel/KernelKillSwitch.sol b/contracts/kill_switch/kernel/KernelKillSwitch.sol new file mode 100644 index 000000000..16c96a691 --- /dev/null +++ b/contracts/kill_switch/kernel/KernelKillSwitch.sol @@ -0,0 +1,40 @@ +pragma solidity 0.4.24; + +import "../base/KillSwitch.sol"; +import "../../kernel/Kernel.sol"; + + +contract KernelKillSwitch is Kernel, KillSwitch { + string private constant ERROR_CONTRACT_CALL_NOT_ALLOWED = "KERNEL_CONTRACT_CALL_NOT_ALLOWED"; + + function initialize(IssuesRegistry _issuesRegistry, IACL _baseAcl, address _permissionsCreator) public onlyInit { + _setIssuesRegistry(_issuesRegistry); + Kernel.initialize(_baseAcl, _permissionsCreator); + } + + function getApp(bytes32 _namespace, bytes32 _appId) public view returns (address) { + // TODO: The tx information that the kill switch should eval cannot be accessed from here. + // Note that `msg.sender` is the proxy requesting the base app address, and `msg.data` + // refers to this call (`Kernel#getApp(bytes32,bytes32)`) + + address _app = super.getApp(_namespace, _appId); + bool _isCallAllowed = !shouldDenyCallingContract(_app, msg.sender, address(0), new bytes(0), uint256(0)); + require(_isCallAllowed, ERROR_CONTRACT_CALL_NOT_ALLOWED); + return _app; + } + + function _shouldEvaluateCall(address _base, address _instance, address _sender, bytes _data, uint256 _value) internal returns (bool) { + /****************************************** IMPORTANT *********************************************/ + /* Due to how proxies work, every time we call a proxied app, we will ask the kernel what's the */ + /* address of the base implementation where it should delegate the call to. But since the kernel */ + /* is also a proxy, it will basically delegate that query to the base kernel implementation, and */ + /* that's when this context is evaluated. Thus, we don't have full context of the call that its */ + /* about to be delegated to the base app implementation, the msg.data corresponds to the Kernel */ + /* getApp(bytes32,bytes32) method for example. Therefore, handling specific scenarios here it's */ + /* really cumbersome. We could rely easily on timestamps or block information, but tx data does */ + /* not correspond to the application call in this context. */ + /**************************************************************************************************/ + + return super._shouldEvaluateCall(_base, _instance ,_sender, _data, _value); + } +} diff --git a/contracts/kill_switch/kernel/KernelSeveritiesKillSwitch.sol b/contracts/kill_switch/kernel/KernelSeveritiesKillSwitch.sol new file mode 100644 index 000000000..6fb4519b4 --- /dev/null +++ b/contracts/kill_switch/kernel/KernelSeveritiesKillSwitch.sol @@ -0,0 +1,16 @@ +pragma solidity 0.4.24; + +import "./KernelKillSwitch.sol"; +import "../base/SeveritiesKillSwitch.sol"; + + +contract KernelSeveritiesKillSwitch is KernelKillSwitch, SeveritiesKillSwitch { + constructor(bool _shouldPetrify) Kernel(_shouldPetrify) public {} + + function setLowestAllowedSeverity(address _contract, IssuesRegistry.Severity _severity) + external + auth(SET_LOWEST_ALLOWED_SEVERITY_ROLE, arr(_contract, msg.sender)) + { + _setLowestAllowedSeverity(_contract, _severity); + } +} diff --git a/contracts/test/mocks/kill_switch/kernel/KernelKillSwitchAppMock.sol b/contracts/test/mocks/kill_switch/kernel/KernelKillSwitchAppMock.sol new file mode 100644 index 000000000..9bfec0be7 --- /dev/null +++ b/contracts/test/mocks/kill_switch/kernel/KernelKillSwitchAppMock.sol @@ -0,0 +1,27 @@ +pragma solidity 0.4.24; + +import "../../../../apps/AragonApp.sol"; + + +contract KernelKillSwitchAppMock is AragonApp { + address public owner; + uint256 internal data; + + function initialize(address _owner) public onlyInit { + initialized(); + data = 42; + owner = _owner; + } + + function read() public view returns (uint256) { + return data; + } + + function write(uint256 _data) public { + data = _data; + } + + function reset() public { + data = 0; + } +} diff --git a/test/kill_switch/kernel/KernelBinaryKillSwitch.test.js b/test/kill_switch/kernel/KernelBinaryKillSwitch.test.js new file mode 100644 index 000000000..adc32f301 --- /dev/null +++ b/test/kill_switch/kernel/KernelBinaryKillSwitch.test.js @@ -0,0 +1,128 @@ +const { assertRevert } = require('../../helpers/assertThrow') + +const IssuesRegistry = artifacts.require('IssuesRegistry') +const KernelKillSwitchAppMock = artifacts.require('KernelKillSwitchAppMock') + +const ACL = artifacts.require('ACL') +const RegularKernel = artifacts.require('Kernel') +const KernelKillSwitch = artifacts.require('KernelBinaryKillSwitch') +const DAOFactory = artifacts.require('DAOFactory') +const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') + +const SEVERITY = { NONE: 0, LOW: 1, MID: 2, HIGH: 3, CRITICAL: 4 } + +const getEventArgument = (receipt, event, arg) => receipt.logs.find(l => l.event === event).args[arg] + +contract('KernelBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { + let killSwitchedKernelBase, regularKernelBase, aclBase, appBase, issuesRegistryBase, registryFactory + let regularDao, regularAcl, killSwitchedDao, killSwitchedAcl, issuesRegistry, app + + before('deploy base implementations', async () => { + regularKernelBase = await RegularKernel.new(true) // petrify immediately + killSwitchedKernelBase = await KernelKillSwitch.new(true) // petrify immediately + aclBase = await ACL.new() + appBase = await KernelKillSwitchAppMock.new() + issuesRegistryBase = await IssuesRegistry.new() + registryFactory = await EVMScriptRegistryFactory.new() + }) + + beforeEach('deploy DAO with regular kernel', async () => { + const regularDaoFactory = await DAOFactory.new(regularKernelBase.address, aclBase.address, registryFactory.address) + const regularKernelReceipt = await regularDaoFactory.newDAO(root) + regularDao = RegularKernel.at(getEventArgument(regularKernelReceipt, 'DeployDAO', 'dao')) + regularAcl = ACL.at(await regularDao.acl()) + + const APP_MANAGER_ROLE = await regularKernelBase.APP_MANAGER_ROLE() + await regularAcl.createPermission(root, regularDao.address, APP_MANAGER_ROLE, root, { from: root }) + }) + + beforeEach('create issues registry app from DAO with regular kernel', async () => { + const issuesRegistryReceipt = await regularDao.newAppInstance('0x1234', issuesRegistryBase.address, '0x', false, { from: root }) + issuesRegistry = IssuesRegistry.at(getEventArgument(issuesRegistryReceipt, 'NewAppProxy', 'proxy')) + await issuesRegistry.initialize() + const SET_ENTRY_SEVERITY_ROLE = await issuesRegistryBase.SET_ENTRY_SEVERITY_ROLE() + await regularAcl.createPermission(securityPartner, issuesRegistry.address, SET_ENTRY_SEVERITY_ROLE, root, { from: root }) + }) + + beforeEach('deploy DAO with kernel binary kill switch', async () => { + const killSwitchedDaoFactory = await DAOFactory.new(killSwitchedKernelBase.address, aclBase.address, registryFactory.address) + const killSwitchedKernelReceipt = await killSwitchedDaoFactory.newDAOWithKillSwitch(root, issuesRegistry.address) + killSwitchedDao = KernelKillSwitch.at(getEventArgument(killSwitchedKernelReceipt, 'DeployDAO', 'dao')) + killSwitchedAcl = ACL.at(await killSwitchedDao.acl()) + + const APP_MANAGER_ROLE = await killSwitchedKernelBase.APP_MANAGER_ROLE() + await killSwitchedAcl.createPermission(root, killSwitchedDao.address, APP_MANAGER_ROLE, root, { from: root }) + const SET_IGNORED_CONTRACTS_ROLE = await killSwitchedDao.SET_IGNORED_CONTRACTS_ROLE() + await killSwitchedAcl.createPermission(owner, killSwitchedDao.address, SET_IGNORED_CONTRACTS_ROLE, root, { from: root }) + }) + + beforeEach('create sample app from DAO with kernel binary kill switch', async () => { + const appReceipt = await killSwitchedDao.newAppInstance('0x1235', appBase.address, '0x', false, { from: root }) + app = KernelKillSwitchAppMock.at(getEventArgument(appReceipt, 'NewAppProxy', 'proxy')) + await app.initialize(owner) + }) + + const itExecutesTheCall = () => { + it('executes the call', async () => { + assert.equal(await app.read(), 42) + }) + } + + const itDoesNotExecuteTheCall = () => { + it('does not execute the call', async () => { + await assertRevert(app.read(), 'KERNEL_CONTRACT_CALL_NOT_ALLOWED') + }) + } + + context('when there is no bug registered', () => { + context('when the contract being called is not ignored', () => { + itExecutesTheCall() + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await killSwitchedDao.setContractIgnore(appBase.address, true, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + + context('when there is a bug registered', () => { + beforeEach('register a bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.LOW, { from: securityPartner }) + }) + + context('when the bug was not fixed yet', () => { + context('when the contract being called is not ignored', () => { + itDoesNotExecuteTheCall() + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await killSwitchedDao.setContractIgnore(appBase.address, true, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + + context('when the bug was already fixed', () => { + beforeEach('fix bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) + }) + + context('when the contract being called is not ignored', () => { + itExecutesTheCall() + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await killSwitchedDao.setContractIgnore(appBase.address, true, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + }) +}) diff --git a/test/kill_switch/kernel/KernelSeveritiesKillSwitch.test.js b/test/kill_switch/kernel/KernelSeveritiesKillSwitch.test.js new file mode 100644 index 000000000..6a5d60109 --- /dev/null +++ b/test/kill_switch/kernel/KernelSeveritiesKillSwitch.test.js @@ -0,0 +1,164 @@ +const { assertRevert } = require('../../helpers/assertThrow') + +const IssuesRegistry = artifacts.require('IssuesRegistry') +const KernelKillSwitchAppMock = artifacts.require('KernelKillSwitchAppMock') + +const ACL = artifacts.require('ACL') +const RegularKernel = artifacts.require('Kernel') +const KernelKillSwitch = artifacts.require('KernelSeveritiesKillSwitch') +const DAOFactory = artifacts.require('DAOFactory') +const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') + +const SEVERITY = { NONE: 0, LOW: 1, MID: 2, HIGH: 3, CRITICAL: 4 } + +const getEventArgument = (receipt, event, arg) => receipt.logs.find(l => l.event === event).args[arg] + +contract('KernelSeveritiesKillSwitch', ([_, root, owner, securityPartner]) => { + let killSwitchedKernelBase, regularKernelBase, aclBase, appBase, issuesRegistryBase, registryFactory + let regularDao, regularAcl, killSwitchedDao, killSwitchedAcl, issuesRegistry, app + + before('deploy base implementations', async () => { + regularKernelBase = await RegularKernel.new(true) // petrify immediately + killSwitchedKernelBase = await KernelKillSwitch.new(true) // petrify immediately + aclBase = await ACL.new() + appBase = await KernelKillSwitchAppMock.new() + issuesRegistryBase = await IssuesRegistry.new() + registryFactory = await EVMScriptRegistryFactory.new() + }) + + before('deploy DAO with regular kernel', async () => { + const regularDaoFactory = await DAOFactory.new(regularKernelBase.address, aclBase.address, registryFactory.address) + const regularKernelReceipt = await regularDaoFactory.newDAO(root) + regularDao = RegularKernel.at(getEventArgument(regularKernelReceipt, 'DeployDAO', 'dao')) + regularAcl = ACL.at(await regularDao.acl()) + + const APP_MANAGER_ROLE = await regularKernelBase.APP_MANAGER_ROLE() + await regularAcl.createPermission(root, regularDao.address, APP_MANAGER_ROLE, root, { from: root }) + }) + + beforeEach('create issues registry app from DAO with regular kernel', async () => { + const issuesRegistryReceipt = await regularDao.newAppInstance('0x1234', issuesRegistryBase.address, '0x', false, { from: root }) + issuesRegistry = IssuesRegistry.at(getEventArgument(issuesRegistryReceipt, 'NewAppProxy', 'proxy')) + await issuesRegistry.initialize() + const SET_ENTRY_SEVERITY_ROLE = await issuesRegistryBase.SET_ENTRY_SEVERITY_ROLE() + await regularAcl.createPermission(securityPartner, issuesRegistry.address, SET_ENTRY_SEVERITY_ROLE, root, { from: root }) + }) + + beforeEach('deploy DAO with kernel severities kill switch', async () => { + const killSwitchedDaoFactory = await DAOFactory.new(killSwitchedKernelBase.address, aclBase.address, registryFactory.address) + const killSwitchedKernelReceipt = await killSwitchedDaoFactory.newDAOWithKillSwitch(root, issuesRegistry.address) + killSwitchedDao = KernelKillSwitch.at(getEventArgument(killSwitchedKernelReceipt, 'DeployDAO', 'dao')) + killSwitchedAcl = ACL.at(await killSwitchedDao.acl()) + + const APP_MANAGER_ROLE = await killSwitchedKernelBase.APP_MANAGER_ROLE() + await killSwitchedAcl.createPermission(root, killSwitchedDao.address, APP_MANAGER_ROLE, root, { from: root }) + const SET_LOWEST_ALLOWED_SEVERITY_ROLE = await killSwitchedDao.SET_LOWEST_ALLOWED_SEVERITY_ROLE() + await killSwitchedAcl.createPermission(owner, killSwitchedDao.address, SET_LOWEST_ALLOWED_SEVERITY_ROLE, root, { from: root }) + }) + + beforeEach('create sample app from DAO with kernel severities kill switch', async () => { + const appReceipt = await killSwitchedDao.newAppInstance('0x1235', appBase.address, '0x', false, { from: root }) + app = KernelKillSwitchAppMock.at(getEventArgument(appReceipt, 'NewAppProxy', 'proxy')) + await app.initialize(owner) + }) + + const itExecutesTheCall = () => { + it('executes the call', async () => { + assert.equal(await app.read(), 42) + }) + } + + const itDoesNotExecuteTheCall = () => { + it('does not execute the call', async () => { + await assertRevert(app.read(), 'KERNEL_CONTRACT_CALL_NOT_ALLOWED') + }) + } + + context('when there is no bug registered', () => { + context('when there is no lowest allowed severity set for the contract being called', () => { + itExecutesTheCall() + }) + + context('when there is a lowest allowed severity set for the contract being called', () => { + beforeEach('set lowest allowed severity', async () => { + await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + + context('when there is a bug registered', () => { + beforeEach('register a bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.MID, { from: securityPartner }) + }) + + context('when the bug was not fixed yet', () => { + context('when there is no lowest allowed severity set for the contract being called', () => { + itExecutesTheCall() + }) + + context('when there is a lowest allowed severity set for the contract being called', () => { + context('when there lowest allowed severity is under the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) + + itDoesNotExecuteTheCall() + }) + + context('when there lowest allowed severity is equal to the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + }) + + itDoesNotExecuteTheCall() + }) + + context('when there lowest allowed severity is greater than the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + }) + + context('when the bug was already fixed', () => { + beforeEach('fix bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) + }) + + context('when there is no lowest allowed severity set for the contract being called', () => { + itExecutesTheCall() + }) + + context('when there is a lowest allowed severity set for the contract being called', () => { + context('when there lowest allowed severity is under the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) + + itExecutesTheCall() + }) + + context('when there lowest allowed severity is equal to the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + }) + + itExecutesTheCall() + }) + + context('when there lowest allowed severity is greater than the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + }) + }) +}) From 0cb88ab3bfdf2a5ad5d2f36ac43ba5e2a896c814 Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Sun, 28 Apr 2019 21:45:14 -0300 Subject: [PATCH 6/8] tests: add kill switch unit tests --- .../kill_switch/base/SeveritiesKillSwitch.sol | 2 +- .../app/AppBinaryKillSwitch.test.js | 287 +++++------ .../app/AppSeveritiesKillSwitch.test.js | 469 +++++++++--------- test/kill_switch/base/IssuesRegistry.test.js | 131 +++++ .../base/itBehavesLikeBinaryKillSwitch.js | 50 ++ .../base/itBehavesLikeSeveritiesKillSwitch.js | 73 +++ .../kernel/KernelBinaryKillSwitch.test.js | 83 ++-- .../kernel/KernelSeveritiesKillSwitch.test.js | 141 +++--- 8 files changed, 767 insertions(+), 469 deletions(-) create mode 100644 test/kill_switch/base/IssuesRegistry.test.js create mode 100644 test/kill_switch/base/itBehavesLikeBinaryKillSwitch.js create mode 100644 test/kill_switch/base/itBehavesLikeSeveritiesKillSwitch.js diff --git a/contracts/kill_switch/base/SeveritiesKillSwitch.sol b/contracts/kill_switch/base/SeveritiesKillSwitch.sol index b2912e5d1..1c7106121 100644 --- a/contracts/kill_switch/base/SeveritiesKillSwitch.sol +++ b/contracts/kill_switch/base/SeveritiesKillSwitch.sol @@ -19,7 +19,7 @@ contract SeveritiesKillSwitch is KillSwitch { function isSeverityIgnored(address _contract, IssuesRegistry.Severity _severity) public view returns (bool) { IssuesRegistry.Severity lowestAllowedSeverity = lowestAllowedSeverityByContract[_contract]; - return lowestAllowedSeverity > _severity; + return lowestAllowedSeverity >= _severity; } function _setLowestAllowedSeverity(address _contract, IssuesRegistry.Severity _severity) internal { diff --git a/test/kill_switch/app/AppBinaryKillSwitch.test.js b/test/kill_switch/app/AppBinaryKillSwitch.test.js index 5043e3379..52ae92517 100644 --- a/test/kill_switch/app/AppBinaryKillSwitch.test.js +++ b/test/kill_switch/app/AppBinaryKillSwitch.test.js @@ -1,4 +1,5 @@ const { assertRevert } = require('../../helpers/assertThrow') +const itBehavesLikeBinaryKillSwitch = require('../base/itBehavesLikeBinaryKillSwitch') const IssuesRegistry = artifacts.require('IssuesRegistry') const KillSwitchedApp = artifacts.require('AppKillSwitchedAppMock') @@ -57,58 +58,19 @@ contract('AppBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { await app.initialize(appKillSwitch.address, owner) }) - context('when the function being called is not tagged', () => { - const itExecutesTheCall = () => { - it('executes the call', async () => { - assert.equal(await app.read(), 42) - }) - } - - context('when there is no bug registered', () => { - context('when the contract being called is not ignored', () => { - itExecutesTheCall() - }) - - context('when the contract being called is ignored', () => { - beforeEach('ignore calling contract', async () => { - await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) - }) - - itExecutesTheCall() - }) + describe('binary kill switch', function () { + beforeEach('bind kill switch', function () { + this.killSwitch = appKillSwitch }) - context('when there is a bug registered', () => { - beforeEach('register a bug', async () => { - await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.LOW, { from: securityPartner }) - }) - - context('when the contract being called is not ignored', () => { - itExecutesTheCall() - }) - - context('when the contract being called is ignored', () => { - beforeEach('ignore calling contract', async () => { - await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) - }) - - itExecutesTheCall() - }) - }) + itBehavesLikeBinaryKillSwitch(owner, anyone) }) - context('when the function being called is tagged', () => { - context('when the function being called is always evaluated', () => { - const itExecutesTheCall = (from = owner) => { + describe('integration', () => { + context('when the function being called is not tagged', () => { + const itExecutesTheCall = () => { it('executes the call', async () => { - await app.write(10, { from }) - assert.equal(await app.read(), 10) - }) - } - - const itDoesNotExecuteTheCall = (from = owner) => { - it('does not execute the call', async () => { - await assertRevert(app.write(10, { from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') + assert.equal(await app.read(), 42) }) } @@ -131,15 +93,38 @@ contract('AppBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.LOW, { from: securityPartner }) }) - context('when the bug was not fixed yet', () => { - context('when the contract being called is not ignored', () => { - context('when the sender is the owner', () => { - itDoesNotExecuteTheCall(owner) - }) + context('when the contract being called is not ignored', () => { + itExecutesTheCall() + }) - context('when the sender is not the owner', () => { - itDoesNotExecuteTheCall(anyone) - }) + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + }) + + itExecutesTheCall() + }) + }) + }) + + context('when the function being called is tagged', () => { + context('when the function being called is always evaluated', () => { + const itExecutesTheCall = (from = owner) => { + it('executes the call', async () => { + await app.write(10, { from }) + assert.equal(await app.read(), 10) + }) + } + + const itDoesNotExecuteTheCall = (from = owner) => { + it('does not execute the call', async () => { + await assertRevert(app.write(10, { from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') + }) + } + + context('when there is no bug registered', () => { + context('when the contract being called is not ignored', () => { + itExecutesTheCall() }) context('when the contract being called is ignored', () => { @@ -147,90 +132,90 @@ contract('AppBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) + itExecutesTheCall() }) }) - context('when the bug was already fixed', () => { - beforeEach('fix bug', async () => { - await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) + context('when there is a bug registered', () => { + beforeEach('register a bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.LOW, { from: securityPartner }) }) - context('when the contract being called is not ignored', () => { - context('when the sender is the owner', () => { - itExecutesTheCall(owner) + context('when the bug was not fixed yet', () => { + context('when the contract being called is not ignored', () => { + context('when the sender is the owner', () => { + itDoesNotExecuteTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itDoesNotExecuteTheCall(anyone) + }) }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) }) }) - context('when the contract being called is ignored', () => { - beforeEach('ignore calling contract', async () => { - await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + context('when the bug was already fixed', () => { + beforeEach('fix bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) + context('when the contract being called is not ignored', () => { + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) }) - }) - }) - }) - }) - context('when the function being called is evaluated only when the sender is not the owner', () => { - const itExecutesTheCall = (from = owner) => { - it('executes the call', async () => { - await app.reset({ from }) - assert.equal(await app.read(), 0) - }) - } + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + }) - const itDoesNotExecuteTheCall = (from = owner) => { - it('does not execute the call', async () => { - await assertRevert(app.reset({ from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') - }) - } + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) - context('when there is no bug registered', () => { - context('when the contract being called is not ignored', () => { - itExecutesTheCall() - }) - - context('when the contract being called is ignored', () => { - beforeEach('ignore calling contract', async () => { - await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) }) - - itExecutesTheCall() }) }) - context('when there is a bug registered', () => { - beforeEach('register a bug', async () => { - await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.LOW, { from: securityPartner }) - }) + context('when the function being called is evaluated only when the sender is not the owner', () => { + const itExecutesTheCall = (from = owner) => { + it('executes the call', async () => { + await app.reset({ from }) + assert.equal(await app.read(), 0) + }) + } - context('when the bug was not fixed yet', () => { - context('when the contract being called is not ignored', () => { - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) + const itDoesNotExecuteTheCall = (from = owner) => { + it('does not execute the call', async () => { + await assertRevert(app.reset({ from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') + }) + } - context('when the sender is not the owner', () => { - itDoesNotExecuteTheCall(anyone) - }) + context('when there is no bug registered', () => { + context('when the contract being called is not ignored', () => { + itExecutesTheCall() }) context('when the contract being called is ignored', () => { @@ -238,42 +223,68 @@ contract('AppBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) + itExecutesTheCall() }) }) - context('when the bug was already fixed', () => { - beforeEach('fix bug', async () => { - await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) + context('when there is a bug registered', () => { + beforeEach('register a bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.LOW, { from: securityPartner }) }) - context('when the contract being called is not ignored', () => { - context('when the sender is the owner', () => { - itExecutesTheCall(owner) + context('when the bug was not fixed yet', () => { + context('when the contract being called is not ignored', () => { + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itDoesNotExecuteTheCall(anyone) + }) }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) }) }) - context('when the contract being called is ignored', () => { - beforeEach('ignore calling contract', async () => { - await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + context('when the bug was already fixed', () => { + beforeEach('fix bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) + context('when the contract being called is not ignored', () => { + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) }) }) }) diff --git a/test/kill_switch/app/AppSeveritiesKillSwitch.test.js b/test/kill_switch/app/AppSeveritiesKillSwitch.test.js index 117b676c1..bfe9455a1 100644 --- a/test/kill_switch/app/AppSeveritiesKillSwitch.test.js +++ b/test/kill_switch/app/AppSeveritiesKillSwitch.test.js @@ -1,4 +1,5 @@ const { assertRevert } = require('../../helpers/assertThrow') +const itBehavesLikeSeveritiesKillSwitch = require('../base/itBehavesLikeSeveritiesKillSwitch') const IssuesRegistry = artifacts.require('IssuesRegistry') const KillSwitchedApp = artifacts.require('AppKillSwitchedAppMock') @@ -57,76 +58,19 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await app.initialize(appKillSwitch.address, owner) }) - describe('when the function being called is not tagged', () => { - const itExecutesTheCall = () => { - it('executes the call', async () => { - assert.equal(await app.read(), 42) - }) - } - - context('when there is no bug registered', () => { - context('when there is no lowest allowed severity set for the contract being called', () => { - itExecutesTheCall() - }) - - context('when there is a lowest allowed severity set for the contract being called', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) - }) - - itExecutesTheCall() - }) + describe('binary kill switch', function () { + beforeEach('bind kill switch', function () { + this.killSwitch = appKillSwitch }) - - context('when there is a bug registered', () => { - beforeEach('register a bug', async () => { - await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.MID, { from: securityPartner }) - }) - - context('when there is no lowest allowed severity set for the contract being called', () => { - itExecutesTheCall() - }) - - context('when there is a lowest allowed severity set for the contract being called', () => { - context('when there lowest allowed severity is under the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) - }) - - itExecutesTheCall() - }) - - context('when there lowest allowed severity is equal to the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) - }) - - itExecutesTheCall() - }) - context('when there lowest allowed severity is greater than the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) - }) - - itExecutesTheCall() - }) - }) - }) + itBehavesLikeSeveritiesKillSwitch(owner, anyone) }) - describe('when the function being called is tagged', () => { - describe('when the function being called is always evaluated', () => { - const itExecutesTheCall = (from = owner) => { + describe('integration', () => { + context('when the function being called is not tagged', () => { + const itExecutesTheCall = () => { it('executes the call', async () => { - await app.write(10, { from }) - assert.equal(await app.read(), 10) - }) - } - - const itDoesNotExecuteTheCall = (from = owner) => { - it('does not execute the call', async () => { - await assertRevert(app.write(10, { from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') + assert.equal(await app.read(), 42) }) } @@ -149,83 +93,74 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.MID, { from: securityPartner }) }) - context('when the bug was not fixed yet', () => { - context('when there is no lowest allowed severity set for the contract being called', () => { - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) + context('when there is no lowest allowed severity set for the contract being called', () => { + itExecutesTheCall() + }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when there is a lowest allowed severity set for the contract being called', () => { + context('when the lowest allowed severity is under the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) }) - }) - context('when there is a lowest allowed severity set for the contract being called', () => { - context('when there lowest allowed severity is under the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) - }) - - context('when the sender is the owner', () => { - itDoesNotExecuteTheCall(owner) - }) + itExecutesTheCall() + }) - context('when the sender is not the owner', () => { - itDoesNotExecuteTheCall(anyone) - }) + context('when the lowest allowed severity is equal to the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) }) - context('when there lowest allowed severity is equal to the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) - }) - - context('when the sender is the owner', () => { - itDoesNotExecuteTheCall(owner) - }) + itExecutesTheCall() + }) - context('when the sender is not the owner', () => { - itDoesNotExecuteTheCall(anyone) - }) + context('when the lowest allowed severity is greater than the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) }) - context('when there lowest allowed severity is greater than the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) - }) - - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) - }) + itExecutesTheCall() }) }) + }) + }) + + context('when the function being called is tagged', () => { + describe('when the function being called is always evaluated', () => { + const itExecutesTheCall = (from = owner) => { + it('executes the call', async () => { + await app.write(10, { from }) + assert.equal(await app.read(), 10) + }) + } - context('when the bug was already fixed', () => { - beforeEach('fix bug', async () => { - await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) + const itDoesNotExecuteTheCall = (from = owner) => { + it('does not execute the call', async () => { + await assertRevert(app.write(10, { from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') }) + } + context('when there is no bug registered', () => { context('when there is no lowest allowed severity set for the contract being called', () => { - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) + itExecutesTheCall() + }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when there is a lowest allowed severity set for the contract being called', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) }) + + itExecutesTheCall() }) + }) - context('when there is a lowest allowed severity set for the contract being called', () => { - context('when there lowest allowed severity is under the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) - }) + context('when there is a bug registered', () => { + beforeEach('register a bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.MID, { from: securityPartner }) + }) + context('when the bug was not fixed yet', () => { + context('when there is no lowest allowed severity set for the contract being called', () => { context('when the sender is the owner', () => { itExecutesTheCall(owner) }) @@ -235,25 +170,57 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) }) }) - context('when there lowest allowed severity is equal to the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + context('when there is a lowest allowed severity set for the contract being called', () => { + context('when the lowest allowed severity is under the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) + + context('when the sender is the owner', () => { + itDoesNotExecuteTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itDoesNotExecuteTheCall(anyone) + }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) + context('when the lowest allowed severity is equal to the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when the lowest allowed severity is greater than the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) }) }) + }) - context('when there lowest allowed severity is greater than the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) - }) + context('when the bug was already fixed', () => { + beforeEach('fix bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) + }) + context('when there is no lowest allowed severity set for the contract being called', () => { context('when the sender is the owner', () => { itExecutesTheCall(owner) }) @@ -262,121 +229,149 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) itExecutesTheCall(anyone) }) }) - }) - }) - }) - }) - describe('when the function being called is evaluated only when the sender is not the owner', () => { - const itExecutesTheCall = (from = owner) => { - it('executes the call', async () => { - await app.reset({ from }) - assert.equal(await app.read(), 0) - }) - } + context('when there is a lowest allowed severity set for the contract being called', () => { + context('when the lowest allowed severity is under the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) - const itDoesNotExecuteTheCall = (from = owner) => { - it('does not execute the call', async () => { - await assertRevert(app.reset({ from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') - }) - } + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) - context('when there is no bug registered', () => { - context('when there is no lowest allowed severity set for the contract being called', () => { - itExecutesTheCall() - }) + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) - context('when there is a lowest allowed severity set for the contract being called', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) - }) + context('when the lowest allowed severity is equal to the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + }) - itExecutesTheCall() + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + + context('when the lowest allowed severity is greater than the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) + }) + }) }) }) - context('when there is a bug registered', () => { - beforeEach('register a bug', async () => { - await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.MID, { from: securityPartner }) - }) + describe('when the function being called is evaluated only when the sender is not the owner', () => { + const itExecutesTheCall = (from = owner) => { + it('executes the call', async () => { + await app.reset({ from }) + assert.equal(await app.read(), 0) + }) + } + + const itDoesNotExecuteTheCall = (from = owner) => { + it('does not execute the call', async () => { + await assertRevert(app.reset({ from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') + }) + } - context('when the bug was not fixed yet', () => { + context('when there is no bug registered', () => { context('when there is no lowest allowed severity set for the contract being called', () => { - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) + itExecutesTheCall() + }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when there is a lowest allowed severity set for the contract being called', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) }) + + itExecutesTheCall() }) + }) - context('when there is a lowest allowed severity set for the contract being called', () => { - context('when there lowest allowed severity is under the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) - }) + context('when there is a bug registered', () => { + beforeEach('register a bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.MID, { from: securityPartner }) + }) + context('when the bug was not fixed yet', () => { + context('when there is no lowest allowed severity set for the contract being called', () => { context('when the sender is the owner', () => { itExecutesTheCall(owner) }) context('when the sender is not the owner', () => { - itDoesNotExecuteTheCall(anyone) + itExecutesTheCall(anyone) }) }) - context('when there lowest allowed severity is equal to the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) - }) + context('when there is a lowest allowed severity set for the contract being called', () => { + context('when the lowest allowed severity is under the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) - context('when the sender is not the owner', () => { - itDoesNotExecuteTheCall(anyone) + context('when the sender is not the owner', () => { + itDoesNotExecuteTheCall(anyone) + }) }) - }) - context('when there lowest allowed severity is greater than the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) - }) + context('when the lowest allowed severity is equal to the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) }) - }) - }) - }) - context('when the bug was already fixed', () => { - beforeEach('fix bug', async () => { - await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) - }) + context('when the lowest allowed severity is greater than the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + }) - context('when there is no lowest allowed severity set for the contract being called', () => { - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) + }) }) }) - context('when there is a lowest allowed severity set for the contract being called', () => { - context('when there lowest allowed severity is under the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) - }) + context('when the bug was already fixed', () => { + beforeEach('fix bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) + }) + context('when there is no lowest allowed severity set for the contract being called', () => { context('when the sender is the owner', () => { itExecutesTheCall(owner) }) @@ -386,31 +381,47 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) }) }) - context('when there lowest allowed severity is equal to the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) - }) + context('when there is a lowest allowed severity set for the contract being called', () => { + context('when the lowest allowed severity is under the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) }) - }) - context('when there lowest allowed severity is greater than the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) - }) + context('when the lowest allowed severity is equal to the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when the lowest allowed severity is greater than the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + }) + + context('when the sender is the owner', () => { + itExecutesTheCall(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) }) }) }) diff --git a/test/kill_switch/base/IssuesRegistry.test.js b/test/kill_switch/base/IssuesRegistry.test.js new file mode 100644 index 000000000..5859c88e6 --- /dev/null +++ b/test/kill_switch/base/IssuesRegistry.test.js @@ -0,0 +1,131 @@ +const { assertRevert } = require('../../helpers/assertThrow') + +const IssuesRegistry = artifacts.require('IssuesRegistry') +const ACL = artifacts.require('ACL') +const Kernel = artifacts.require('Kernel') +const DAOFactory = artifacts.require('DAOFactory') +const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') + +const SEVERITY = { NONE: 0, LOW: 1, MID: 2, HIGH: 3, CRITICAL: 4 } + +const getEventArgument = (receipt, event, arg) => receipt.logs.find(l => l.event === event).args[arg] + +contract('IssuesRegistry', ([_, root, implementation, owner, anyone]) => { + let kernelBase, aclBase, issuesRegistryBase, registryFactory, dao, acl, issuesRegistry + + before('deploy base implementations', async () => { + kernelBase = await Kernel.new(true) // petrify immediately + aclBase = await ACL.new() + registryFactory = await EVMScriptRegistryFactory.new() + issuesRegistryBase = await IssuesRegistry.new() + }) + + before('deploy DAO', async () => { + const daoFactory = await DAOFactory.new(kernelBase.address, aclBase.address, registryFactory.address) + const kernelReceipt = await daoFactory.newDAO(root) + dao = Kernel.at(getEventArgument(kernelReceipt, 'DeployDAO', 'dao')) + acl = ACL.at(await dao.acl()) + const APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE() + await acl.createPermission(root, dao.address, APP_MANAGER_ROLE, root, { from: root }) + }) + + beforeEach('create issues registry', async () => { + const issuesRegistryReceipt = await dao.newAppInstance('0x1234', issuesRegistryBase.address, '0x', false, { from: root }) + issuesRegistry = IssuesRegistry.at(getEventArgument(issuesRegistryReceipt, 'NewAppProxy', 'proxy')) + await issuesRegistry.initialize() + const SET_ENTRY_SEVERITY_ROLE = await issuesRegistryBase.SET_ENTRY_SEVERITY_ROLE() + await acl.createPermission(owner, issuesRegistry.address, SET_ENTRY_SEVERITY_ROLE, root, { from: root }) + }) + + describe('isSeverityFor', () => { + context('when there was no severity set before', () => { + it('returns false', async () => { + assert.isFalse(await issuesRegistry.isSeverityFor(implementation), 'did not expect severity for given entry') + }) + }) + + context('when there was a severity already set', () => { + beforeEach('set medium severity', async () => { + await issuesRegistry.setSeverityFor(implementation, SEVERITY.LOW, { from: owner }) + }) + + context('when the issues was not fixed yet', () => { + it('returns true', async () => { + assert.isTrue(await issuesRegistry.isSeverityFor(implementation), 'did not expect severity for given entry') + }) + }) + + context('when the issues was already fixed', () => { + beforeEach('set medium severity', async () => { + await issuesRegistry.setSeverityFor(implementation, SEVERITY.NONE, { from: owner }) + }) + + it('returns false', async () => { + assert.isFalse(await issuesRegistry.isSeverityFor(implementation), 'did not expect severity for given entry') + }) + }) + }) + }) + + describe('getSeverityFor', () => { + context('when there was no severity set before', () => { + it('returns none', async () => { + assert.equal(await issuesRegistry.getSeverityFor(implementation), SEVERITY.NONE, 'severity does not match') + }) + }) + + context('when there was a severity already set', () => { + beforeEach('set medium severity', async () => { + await issuesRegistry.setSeverityFor(implementation, SEVERITY.MID, { from: owner }) + }) + + it('returns the severity already set', async () => { + assert.equal(await issuesRegistry.getSeverityFor(implementation), SEVERITY.MID, 'severity does not match') + }) + }) + }) + + describe('setSeverityFor', () => { + context('when the sender is the owner', () => { + const from = owner + + it('emits an event', async () => { + const { logs } = await issuesRegistry.setSeverityFor(implementation, SEVERITY.LOW, { from }) + + const events = logs.filter(l => l.event === 'SeveritySet') + assert.equal(events.length, 1, 'number of SeveritySet events does not match') + assert.equal(events[0].args.entry, implementation, 'entry address does not match') + assert.equal(events[0].args.severity, SEVERITY.LOW, 'severity does not match') + assert.equal(events[0].args.sender, owner, 'sender does not match') + }) + + context('when there was no severity set before', () => { + it('sets the severity for the given entry', async () => { + await issuesRegistry.setSeverityFor(implementation, SEVERITY.MID, { from }) + + assert.equal(await issuesRegistry.getSeverityFor(implementation), SEVERITY.MID, 'severity does not match') + }) + }) + + context('when there was a severity already set', () => { + beforeEach('set medium severity', async () => { + await issuesRegistry.setSeverityFor(implementation, SEVERITY.MID, { from }) + }) + + it('changes the severity for the given entry', async () => { + await issuesRegistry.setSeverityFor(implementation, SEVERITY.LOW, { from }) + + assert.equal(await issuesRegistry.getSeverityFor(implementation), SEVERITY.LOW, 'severity does not match') + }) + }) + }) + + context('when the sender is not the owner', () => { + const from = anyone + + it('reverts', async () => { + await assertRevert(issuesRegistry.setSeverityFor(implementation, SEVERITY.LOW, { from })) + }) + }) + }) +}) diff --git a/test/kill_switch/base/itBehavesLikeBinaryKillSwitch.js b/test/kill_switch/base/itBehavesLikeBinaryKillSwitch.js new file mode 100644 index 000000000..831ce6003 --- /dev/null +++ b/test/kill_switch/base/itBehavesLikeBinaryKillSwitch.js @@ -0,0 +1,50 @@ +const { assertRevert } = require('../../helpers/assertThrow') + +module.exports = function (owner, address) { + describe('isContractIgnored', function () { + context('when the contract is not ignored', function () { + it('returns false', async function () { + assert.isFalse(await this.killSwitch.isContractIgnored(address)) + }) + }) + + context('when the contract is ignored', function () { + beforeEach('ignore contract', async function () { + await this.killSwitch.setContractIgnore(address, true, { from: owner }) + }) + + it('returns true', async function () { + assert.isTrue(await this.killSwitch.isContractIgnored(address)) + }) + }) + }) + + describe('setContractIgnore', function () { + context('when the sender is the owner', function () { + const from = owner + + context('ignoring a contract', function () { + it('ignores the contract', async function () { + await this.killSwitch.setContractIgnore(address, true, { from }) + + assert.isTrue(await this.killSwitch.isContractIgnored(address)) + }) + }) + + context('reverting a contract ignore', function () { + it('reverts the contract ignore', async function () { + await this.killSwitch.setContractIgnore(address, true, { from }) + await this.killSwitch.setContractIgnore(address, false, { from }) + + assert.isFalse(await this.killSwitch.isContractIgnored(address)) + }) + }) + }) + + context('when the sender is not the owner', function () { + it('reverts', async function () { + await assertRevert(this.killSwitch.setContractIgnore(address, true)) + }) + }) + }) +} diff --git a/test/kill_switch/base/itBehavesLikeSeveritiesKillSwitch.js b/test/kill_switch/base/itBehavesLikeSeveritiesKillSwitch.js new file mode 100644 index 000000000..821b53643 --- /dev/null +++ b/test/kill_switch/base/itBehavesLikeSeveritiesKillSwitch.js @@ -0,0 +1,73 @@ +const { assertRevert } = require('../../helpers/assertThrow') +const SEVERITY = { NONE: 0, LOW: 1, MID: 2, HIGH: 3, CRITICAL: 4 } + +module.exports = function (owner, anAddress) { + describe('isSeverityIgnored', function () { + context('when no lowest allowed severity was set yet', function () { + it('returns false for all the severities', async function () { + for (const key of Object.keys(SEVERITY).slice(1)) { + assert.isFalse(await this.killSwitch.isSeverityIgnored(anAddress, SEVERITY[key])) + } + }) + }) + + context('when a lowest allowed severity was set', function () { + beforeEach('set a lowest allowed severity', async function () { + await this.killSwitch.setLowestAllowedSeverity(anAddress, SEVERITY.MID, { from: owner }) + }) + + context('when the given severity is lower than the one set', function () { + it('returns true', async function () { + assert.isTrue(await this.killSwitch.isSeverityIgnored(anAddress, SEVERITY.LOW)) + }) + }) + + context('when the given severity is equal to the one set', function () { + it('returns true', async function () { + assert.isTrue(await this.killSwitch.isSeverityIgnored(anAddress, SEVERITY.MID)) + }) + }) + + context('when the given severity is greater than the one set', function () { + it('returns false', async function () { + assert.isFalse(await this.killSwitch.isSeverityIgnored(anAddress, SEVERITY.HIGH)) + }) + }) + }) + }) + + describe('setLowestAllowedSeverity', function () { + context('when the contract is the owner', function () { + const from = owner + + context('when there was no severity set', function () { + it('sets the lowest allowed severity', async function () { + await this.killSwitch.setLowestAllowedSeverity(anAddress, SEVERITY.HIGH, { from }) + + assert.isTrue(await this.killSwitch.isSeverityIgnored(anAddress, SEVERITY.HIGH)) + assert.isFalse(await this.killSwitch.isSeverityIgnored(anAddress, SEVERITY.CRITICAL)) + }) + }) + + context('when there was a previous severity set', function () { + beforeEach('set lowest allowed severity', async function () { + await this.killSwitch.setLowestAllowedSeverity(anAddress, SEVERITY.LOW, { from }) + assert.isTrue(await this.killSwitch.isSeverityIgnored(anAddress, SEVERITY.LOW)) + }) + + it('changes the lowest allowed severity', async function () { + await this.killSwitch.setLowestAllowedSeverity(anAddress, SEVERITY.MID, { from }) + + assert.isTrue(await this.killSwitch.isSeverityIgnored(anAddress, SEVERITY.MID)) + assert.isFalse(await this.killSwitch.isSeverityIgnored(anAddress, SEVERITY.HIGH)) + }) + }) + }) + + context('when the sender is not the owner', function () { + it('reverts', async function () { + await assertRevert(this.killSwitch.setLowestAllowedSeverity(anAddress, SEVERITY.MID)) + }) + }) + }) +} diff --git a/test/kill_switch/kernel/KernelBinaryKillSwitch.test.js b/test/kill_switch/kernel/KernelBinaryKillSwitch.test.js index adc32f301..e9dc5e572 100644 --- a/test/kill_switch/kernel/KernelBinaryKillSwitch.test.js +++ b/test/kill_switch/kernel/KernelBinaryKillSwitch.test.js @@ -1,4 +1,5 @@ const { assertRevert } = require('../../helpers/assertThrow') +const itBehavesLikeBinaryKillSwitch = require('../base/itBehavesLikeBinaryKillSwitch') const IssuesRegistry = artifacts.require('IssuesRegistry') const KernelKillSwitchAppMock = artifacts.require('KernelKillSwitchAppMock') @@ -62,40 +63,30 @@ contract('KernelBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) = await app.initialize(owner) }) - const itExecutesTheCall = () => { - it('executes the call', async () => { - assert.equal(await app.read(), 42) + describe('binary kill switch', function () { + beforeEach('bind kill switch', function () { + this.killSwitch = killSwitchedDao }) - } - const itDoesNotExecuteTheCall = () => { - it('does not execute the call', async () => { - await assertRevert(app.read(), 'KERNEL_CONTRACT_CALL_NOT_ALLOWED') - }) - } - - context('when there is no bug registered', () => { - context('when the contract being called is not ignored', () => { - itExecutesTheCall() - }) + itBehavesLikeBinaryKillSwitch(owner, anyone) + }) - context('when the contract being called is ignored', () => { - beforeEach('ignore calling contract', async () => { - await killSwitchedDao.setContractIgnore(appBase.address, true, { from: owner }) + describe('integration', () => { + const itExecutesTheCall = () => { + it('executes the call', async () => { + assert.equal(await app.read(), 42) }) + } - itExecutesTheCall() - }) - }) - - context('when there is a bug registered', () => { - beforeEach('register a bug', async () => { - await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.LOW, { from: securityPartner }) - }) + const itDoesNotExecuteTheCall = () => { + it('does not execute the call', async () => { + await assertRevert(app.read(), 'KERNEL_CONTRACT_CALL_NOT_ALLOWED') + }) + } - context('when the bug was not fixed yet', () => { + context('when there is no bug registered', () => { context('when the contract being called is not ignored', () => { - itDoesNotExecuteTheCall() + itExecutesTheCall() }) context('when the contract being called is ignored', () => { @@ -107,21 +98,41 @@ contract('KernelBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) = }) }) - context('when the bug was already fixed', () => { - beforeEach('fix bug', async () => { - await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) + context('when there is a bug registered', () => { + beforeEach('register a bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.LOW, { from: securityPartner }) }) - context('when the contract being called is not ignored', () => { - itExecutesTheCall() + context('when the bug was not fixed yet', () => { + context('when the contract being called is not ignored', () => { + itDoesNotExecuteTheCall() + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await killSwitchedDao.setContractIgnore(appBase.address, true, { from: owner }) + }) + + itExecutesTheCall() + }) }) - context('when the contract being called is ignored', () => { - beforeEach('ignore calling contract', async () => { - await killSwitchedDao.setContractIgnore(appBase.address, true, { from: owner }) + context('when the bug was already fixed', () => { + beforeEach('fix bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) }) - itExecutesTheCall() + context('when the contract being called is not ignored', () => { + itExecutesTheCall() + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await killSwitchedDao.setContractIgnore(appBase.address, true, { from: owner }) + }) + + itExecutesTheCall() + }) }) }) }) diff --git a/test/kill_switch/kernel/KernelSeveritiesKillSwitch.test.js b/test/kill_switch/kernel/KernelSeveritiesKillSwitch.test.js index 6a5d60109..de4f48dc1 100644 --- a/test/kill_switch/kernel/KernelSeveritiesKillSwitch.test.js +++ b/test/kill_switch/kernel/KernelSeveritiesKillSwitch.test.js @@ -1,4 +1,5 @@ const { assertRevert } = require('../../helpers/assertThrow') +const itBehavesLikeSeveritiesKillSwitch = require('../base/itBehavesLikeSeveritiesKillSwitch') const IssuesRegistry = artifacts.require('IssuesRegistry') const KernelKillSwitchAppMock = artifacts.require('KernelKillSwitchAppMock') @@ -13,7 +14,7 @@ const SEVERITY = { NONE: 0, LOW: 1, MID: 2, HIGH: 3, CRITICAL: 4 } const getEventArgument = (receipt, event, arg) => receipt.logs.find(l => l.event === event).args[arg] -contract('KernelSeveritiesKillSwitch', ([_, root, owner, securityPartner]) => { +contract('KernelSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) => { let killSwitchedKernelBase, regularKernelBase, aclBase, appBase, issuesRegistryBase, registryFactory let regularDao, regularAcl, killSwitchedDao, killSwitchedAcl, issuesRegistry, app @@ -62,101 +63,111 @@ contract('KernelSeveritiesKillSwitch', ([_, root, owner, securityPartner]) => { await app.initialize(owner) }) - const itExecutesTheCall = () => { - it('executes the call', async () => { - assert.equal(await app.read(), 42) + describe('severities kill switch', function () { + beforeEach('bind kill switch', function () { + this.killSwitch = killSwitchedDao }) - } - const itDoesNotExecuteTheCall = () => { - it('does not execute the call', async () => { - await assertRevert(app.read(), 'KERNEL_CONTRACT_CALL_NOT_ALLOWED') - }) - } - - context('when there is no bug registered', () => { - context('when there is no lowest allowed severity set for the contract being called', () => { - itExecutesTheCall() - }) + itBehavesLikeSeveritiesKillSwitch(owner, anyone) + }) - context('when there is a lowest allowed severity set for the contract being called', () => { - beforeEach('set lowest allowed severity', async () => { - await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + describe('integration', () => { + const itExecutesTheCall = () => { + it('executes the call', async () => { + assert.equal(await app.read(), 42) }) + } - itExecutesTheCall() - }) - }) - - context('when there is a bug registered', () => { - beforeEach('register a bug', async () => { - await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.MID, { from: securityPartner }) - }) + const itDoesNotExecuteTheCall = () => { + it('does not execute the call', async () => { + await assertRevert(app.read(), 'KERNEL_CONTRACT_CALL_NOT_ALLOWED') + }) + } - context('when the bug was not fixed yet', () => { + context('when there is no bug registered', () => { context('when there is no lowest allowed severity set for the contract being called', () => { itExecutesTheCall() }) context('when there is a lowest allowed severity set for the contract being called', () => { - context('when there lowest allowed severity is under the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) - }) - - itDoesNotExecuteTheCall() + beforeEach('set lowest allowed severity', async () => { + await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) }) - context('when there lowest allowed severity is equal to the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) - }) + itExecutesTheCall() + }) + }) - itDoesNotExecuteTheCall() + context('when there is a bug registered', () => { + beforeEach('register a bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.MID, { from: securityPartner }) + }) + + context('when the bug was not fixed yet', () => { + context('when there is no lowest allowed severity set for the contract being called', () => { + itExecutesTheCall() }) - context('when there lowest allowed severity is greater than the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + context('when there is a lowest allowed severity set for the contract being called', () => { + context('when the lowest allowed severity is under the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) + + itDoesNotExecuteTheCall() }) - itExecutesTheCall() - }) - }) - }) + context('when the lowest allowed severity is equal to the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + }) - context('when the bug was already fixed', () => { - beforeEach('fix bug', async () => { - await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) - }) + itExecutesTheCall() + }) - context('when there is no lowest allowed severity set for the contract being called', () => { - itExecutesTheCall() - }) + context('when the lowest allowed severity is greater than the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + }) - context('when there is a lowest allowed severity set for the contract being called', () => { - context('when there lowest allowed severity is under the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + itExecutesTheCall() }) + }) + }) + + context('when the bug was already fixed', () => { + beforeEach('fix bug', async () => { + await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) + }) + context('when there is no lowest allowed severity set for the contract being called', () => { itExecutesTheCall() }) - context('when there lowest allowed severity is equal to the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + context('when there is a lowest allowed severity set for the contract being called', () => { + context('when the lowest allowed severity is under the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) + }) + + itExecutesTheCall() }) - itExecutesTheCall() - }) + context('when the lowest allowed severity is equal to the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) + }) - context('when there lowest allowed severity is greater than the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + itExecutesTheCall() }) - itExecutesTheCall() + context('when the lowest allowed severity is greater than the reported bug severity', () => { + beforeEach('set lowest allowed severity', async () => { + await killSwitchedDao.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) + }) + + itExecutesTheCall() + }) }) }) }) From d13da009bcb3cce8cabaf176f48975222a29fea3 Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Mon, 29 Apr 2019 10:21:57 -0300 Subject: [PATCH 7/8] contracts: use ternary ignoration for kill-switch settings --- .../kill_switch/app/AppBinaryKillSwitch.sol | 6 +- .../app/AppSeveritiesKillSwitch.sol | 7 + .../kill_switch/base/BinaryKillSwitch.sol | 17 - contracts/kill_switch/base/KillSwitch.sol | 28 +- .../kill_switch/base/SeveritiesKillSwitch.sol | 4 - .../kernel/KernelBinaryKillSwitch.sol | 6 +- .../kernel/KernelSeveritiesKillSwitch.sol | 7 + .../app/AppBinaryKillSwitch.test.js | 222 ++++++------ .../app/AppSeveritiesKillSwitch.test.js | 321 +++++++++++------- test/kill_switch/base/IssuesRegistry.test.js | 6 +- .../base/itBehavesLikeBinaryKillSwitch.js | 68 +++- .../base/itBehavesLikeSeveritiesKillSwitch.js | 72 +++- test/kill_switch/helpers/enums.js | 7 + test/kill_switch/helpers/events.js | 5 + .../kernel/KernelBinaryKillSwitch.test.js | 52 +-- .../kernel/KernelSeveritiesKillSwitch.test.js | 10 +- 16 files changed, 541 insertions(+), 297 deletions(-) create mode 100644 test/kill_switch/helpers/enums.js create mode 100644 test/kill_switch/helpers/events.js diff --git a/contracts/kill_switch/app/AppBinaryKillSwitch.sol b/contracts/kill_switch/app/AppBinaryKillSwitch.sol index fce46e780..b813755bd 100644 --- a/contracts/kill_switch/app/AppBinaryKillSwitch.sol +++ b/contracts/kill_switch/app/AppBinaryKillSwitch.sol @@ -5,10 +5,10 @@ import "../base/BinaryKillSwitch.sol"; contract AppBinaryKillSwitch is AppKillSwitch, BinaryKillSwitch { - function setContractIgnore(address _contract, bool _ignored) + function setContractAction(address _contract, ContractAction _action) external - authP(SET_IGNORED_CONTRACTS_ROLE, arr(_baseApp(), msg.sender)) + authP(SET_CONTRACT_ACTION_ROLE, arr(_baseApp(), msg.sender)) { - _setContractIgnore(_contract, _ignored); + _setContractAction(_contract, _action); } } diff --git a/contracts/kill_switch/app/AppSeveritiesKillSwitch.sol b/contracts/kill_switch/app/AppSeveritiesKillSwitch.sol index bc9d15d60..005732f9b 100644 --- a/contracts/kill_switch/app/AppSeveritiesKillSwitch.sol +++ b/contracts/kill_switch/app/AppSeveritiesKillSwitch.sol @@ -5,6 +5,13 @@ import "../base/SeveritiesKillSwitch.sol"; contract AppSeveritiesKillSwitch is AppKillSwitch, SeveritiesKillSwitch { + function setContractAction(address _contract, ContractAction _action) + external + authP(SET_CONTRACT_ACTION_ROLE, arr(_baseApp(), msg.sender)) + { + _setContractAction(_contract, _action); + } + function setLowestAllowedSeverity(address _contract, IssuesRegistry.Severity _severity) external authP(SET_LOWEST_ALLOWED_SEVERITY_ROLE, arr(_baseApp(), msg.sender)) diff --git a/contracts/kill_switch/base/BinaryKillSwitch.sol b/contracts/kill_switch/base/BinaryKillSwitch.sol index 1f5623c94..e5f507edd 100644 --- a/contracts/kill_switch/base/BinaryKillSwitch.sol +++ b/contracts/kill_switch/base/BinaryKillSwitch.sol @@ -5,24 +5,7 @@ import "./IssuesRegistry.sol"; contract BinaryKillSwitch is KillSwitch { - bytes32 constant public SET_IGNORED_CONTRACTS_ROLE = keccak256("SET_IGNORED_CONTRACTS_ROLE"); - - mapping (address => bool) internal ignoredContracts; - - event ContractIgnored(address _contract, bool ignored); - - function setContractIgnore(address _contract, bool _ignored) external; - - function isContractIgnored(address _contract) public view returns (bool) { - return ignoredContracts[_contract]; - } - function isSeverityIgnored(address /*_contract*/, IssuesRegistry.Severity _severity) public view returns (bool) { return _severity == IssuesRegistry.Severity.None; } - - function _setContractIgnore(address _contract, bool _ignored) internal { - ignoredContracts[_contract] = _ignored; - emit ContractIgnored(_contract, _ignored); - } } diff --git a/contracts/kill_switch/base/KillSwitch.sol b/contracts/kill_switch/base/KillSwitch.sol index 3aea7d9ec..3305cc938 100644 --- a/contracts/kill_switch/base/KillSwitch.sol +++ b/contracts/kill_switch/base/KillSwitch.sol @@ -4,18 +4,39 @@ import "./IssuesRegistry.sol"; contract KillSwitch { + bytes32 constant public SET_CONTRACT_ACTION_ROLE = keccak256("SET_CONTRACT_ACTION_ROLE"); + + enum ContractAction { Check, Ignore, Deny } + IssuesRegistry public issuesRegistry; + mapping (address => ContractAction) internal contractActions; event IssuesRegistrySet(address issuesRegistry, address sender); + event ContractActionSet(address contractAddress, ContractAction action); + + function getContractAction(address _contract) public view returns (ContractAction) { + return contractActions[_contract]; + } - function isContractIgnored(address _contract) public view returns (bool); + function setContractAction(address _contract, ContractAction _action) external; function isSeverityIgnored(address _contract, IssuesRegistry.Severity _severity) public view returns (bool); + function isContractIgnored(address _contract) public view returns (bool) { + return getContractAction(_contract) == ContractAction.Ignore; + } + + function isContractDenied(address _contract) public view returns (bool) { + return getContractAction(_contract) == ContractAction.Deny; + } + function shouldDenyCallingContract(address _base, address _instance, address _sender, bytes _data, uint256 _value) public returns (bool) { // if the call should not be evaluated, then allow given call if (!_shouldEvaluateCall(_base, _instance, _sender, _data, _value)) return false; + // if the call should be denied, then deny given call + if (isContractDenied(_base)) return true; + // if the contract issues are ignored, then allow given call if (isContractIgnored(_base)) return false; @@ -45,4 +66,9 @@ contract KillSwitch { issuesRegistry = _issuesRegistry; emit IssuesRegistrySet(_issuesRegistry, msg.sender); } + + function _setContractAction(address _contract, ContractAction _action) internal { + contractActions[_contract] = _action; + emit ContractActionSet(_contract, _action); + } } diff --git a/contracts/kill_switch/base/SeveritiesKillSwitch.sol b/contracts/kill_switch/base/SeveritiesKillSwitch.sol index 1c7106121..e38867944 100644 --- a/contracts/kill_switch/base/SeveritiesKillSwitch.sol +++ b/contracts/kill_switch/base/SeveritiesKillSwitch.sol @@ -13,10 +13,6 @@ contract SeveritiesKillSwitch is KillSwitch { function setLowestAllowedSeverity(address _contract, IssuesRegistry.Severity _severity) external; - function isContractIgnored(address _contract) public view returns (bool) { - return lowestAllowedSeverityByContract[_contract] == IssuesRegistry.Severity.None; - } - function isSeverityIgnored(address _contract, IssuesRegistry.Severity _severity) public view returns (bool) { IssuesRegistry.Severity lowestAllowedSeverity = lowestAllowedSeverityByContract[_contract]; return lowestAllowedSeverity >= _severity; diff --git a/contracts/kill_switch/kernel/KernelBinaryKillSwitch.sol b/contracts/kill_switch/kernel/KernelBinaryKillSwitch.sol index 95ce4c4fc..8c8d3eeb7 100644 --- a/contracts/kill_switch/kernel/KernelBinaryKillSwitch.sol +++ b/contracts/kill_switch/kernel/KernelBinaryKillSwitch.sol @@ -7,10 +7,10 @@ import "../base/BinaryKillSwitch.sol"; contract KernelBinaryKillSwitch is KernelKillSwitch, BinaryKillSwitch { constructor(bool _shouldPetrify) Kernel(_shouldPetrify) public {} - function setContractIgnore(address _contract, bool _ignored) + function setContractAction(address _contract, ContractAction _action) external - auth(SET_IGNORED_CONTRACTS_ROLE, arr(_contract, msg.sender)) + auth(SET_CONTRACT_ACTION_ROLE, arr(_contract, msg.sender)) { - _setContractIgnore(_contract, _ignored); + _setContractAction(_contract, _action); } } diff --git a/contracts/kill_switch/kernel/KernelSeveritiesKillSwitch.sol b/contracts/kill_switch/kernel/KernelSeveritiesKillSwitch.sol index 6fb4519b4..00503324b 100644 --- a/contracts/kill_switch/kernel/KernelSeveritiesKillSwitch.sol +++ b/contracts/kill_switch/kernel/KernelSeveritiesKillSwitch.sol @@ -7,6 +7,13 @@ import "../base/SeveritiesKillSwitch.sol"; contract KernelSeveritiesKillSwitch is KernelKillSwitch, SeveritiesKillSwitch { constructor(bool _shouldPetrify) Kernel(_shouldPetrify) public {} + function setContractAction(address _contract, ContractAction _action) + external + auth(SET_CONTRACT_ACTION_ROLE, arr(_contract, msg.sender)) + { + _setContractAction(_contract, _action); + } + function setLowestAllowedSeverity(address _contract, IssuesRegistry.Severity _severity) external auth(SET_LOWEST_ALLOWED_SEVERITY_ROLE, arr(_contract, msg.sender)) diff --git a/test/kill_switch/app/AppBinaryKillSwitch.test.js b/test/kill_switch/app/AppBinaryKillSwitch.test.js index 52ae92517..7ae743735 100644 --- a/test/kill_switch/app/AppBinaryKillSwitch.test.js +++ b/test/kill_switch/app/AppBinaryKillSwitch.test.js @@ -1,4 +1,6 @@ +const { ACTION, SEVERITY } = require('../helpers/enums') const { assertRevert } = require('../../helpers/assertThrow') +const { getEventArgument } = require('../helpers/events') const itBehavesLikeBinaryKillSwitch = require('../base/itBehavesLikeBinaryKillSwitch') const IssuesRegistry = artifacts.require('IssuesRegistry') @@ -10,10 +12,6 @@ const Kernel = artifacts.require('Kernel') const DAOFactory = artifacts.require('DAOFactory') const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') -const SEVERITY = { NONE: 0, LOW: 1, MID: 2, HIGH: 3, CRITICAL: 4 } - -const getEventArgument = (receipt, event, arg) => receipt.logs.find(l => l.event === event).args[arg] - contract('AppBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { let kernelBase, aclBase, appBase, appKillSwitchBase, issuesRegistryBase let registryFactory, dao, acl, issuesRegistry, app, appKillSwitch @@ -48,8 +46,8 @@ contract('AppBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { const appKillSwitchReceipt = await dao.newAppInstance('0x1235', appKillSwitchBase.address, '0x', false, { from: root }) appKillSwitch = AppBinaryKillSwitch.at(getEventArgument(appKillSwitchReceipt, 'NewAppProxy', 'proxy')) await appKillSwitch.initialize(issuesRegistry.address) - const SET_IGNORED_CONTRACTS_ROLE = await appKillSwitchBase.SET_IGNORED_CONTRACTS_ROLE() - await acl.createPermission(owner, appKillSwitch.address, SET_IGNORED_CONTRACTS_ROLE, root, { from: root }) + const SET_CONTRACT_ACTION_ROLE = await appKillSwitchBase.SET_CONTRACT_ACTION_ROLE() + await acl.createPermission(owner, appKillSwitch.address, SET_CONTRACT_ACTION_ROLE, root, { from: root }) }) beforeEach('create kill switched app', async () => { @@ -75,13 +73,21 @@ contract('AppBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { } context('when there is no bug registered', () => { - context('when the contract being called is not ignored', () => { + context('when the contract being called is checked', () => { itExecutesTheCall() }) context('when the contract being called is ignored', () => { beforeEach('ignore calling contract', async () => { - await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + await appKillSwitch.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) + }) + + itExecutesTheCall() + }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) }) itExecutesTheCall() @@ -93,13 +99,21 @@ contract('AppBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.LOW, { from: securityPartner }) }) - context('when the contract being called is not ignored', () => { + context('when the contract being called is checked', () => { itExecutesTheCall() }) context('when the contract being called is ignored', () => { beforeEach('ignore calling contract', async () => { - await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + await appKillSwitch.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) + }) + + itExecutesTheCall() + }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) }) itExecutesTheCall() @@ -109,31 +123,43 @@ contract('AppBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { context('when the function being called is tagged', () => { context('when the function being called is always evaluated', () => { - const itExecutesTheCall = (from = owner) => { + const itExecutesTheCall = () => { it('executes the call', async () => { - await app.write(10, { from }) + await app.write(10, { from: owner }) assert.equal(await app.read(), 10) }) } - const itDoesNotExecuteTheCall = (from = owner) => { + const itDoesNotExecuteTheCall = () => { it('does not execute the call', async () => { - await assertRevert(app.write(10, { from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') + await assertRevert(app.write(10, { from: owner }), 'APP_CONTRACT_CALL_NOT_ALLOWED') }) } - context('when there is no bug registered', () => { - context('when the contract being called is not ignored', () => { + const itExecutesTheCallWhenNotDenied = () => { + context('when the contract being called is checked', () => { itExecutesTheCall() }) context('when the contract being called is ignored', () => { beforeEach('ignore calling contract', async () => { - await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + await appKillSwitch.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) }) itExecutesTheCall() }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) + }) + + itDoesNotExecuteTheCall() + }) + } + + context('when there is no bug registered', () => { + itExecutesTheCallWhenNotDenied() }) context('when there is a bug registered', () => { @@ -142,28 +168,24 @@ contract('AppBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { }) context('when the bug was not fixed yet', () => { - context('when the contract being called is not ignored', () => { - context('when the sender is the owner', () => { - itDoesNotExecuteTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itDoesNotExecuteTheCall(anyone) - }) + context('when the contract being called is checked', () => { + itDoesNotExecuteTheCall(owner) }) context('when the contract being called is ignored', () => { beforeEach('ignore calling contract', async () => { - await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + await appKillSwitch.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) + itExecutesTheCall() + }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) }) + + itDoesNotExecuteTheCall() }) }) @@ -172,59 +194,81 @@ contract('AppBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) }) - context('when the contract being called is not ignored', () => { - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) - }) - - context('when the contract being called is ignored', () => { - beforeEach('ignore calling contract', async () => { - await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) - }) - - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) - }) + itExecutesTheCallWhenNotDenied() }) }) }) context('when the function being called is evaluated only when the sender is not the owner', () => { - const itExecutesTheCall = (from = owner) => { + const itExecutesTheCall = (from) => { it('executes the call', async () => { await app.reset({ from }) assert.equal(await app.read(), 0) }) } - const itDoesNotExecuteTheCall = (from = owner) => { + const itDoesNotExecuteTheCall = (from) => { it('does not execute the call', async () => { await assertRevert(app.reset({ from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') }) } - context('when there is no bug registered', () => { - context('when the contract being called is not ignored', () => { - itExecutesTheCall() + const itExecutesTheCallEvenIfDenied = (from) => { + context('when the contract being called is checked', () => { + itExecutesTheCall(from) }) context('when the contract being called is ignored', () => { beforeEach('ignore calling contract', async () => { - await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) + await appKillSwitch.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) }) - itExecutesTheCall() + itExecutesTheCall(from) }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) + }) + + itExecutesTheCall(from) + }) + } + + const itExecutesTheCallWhenNotDenied = (from) => { + context('when the contract being called is checked', () => { + itExecutesTheCall(from) + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) + }) + + itExecutesTheCall(from) + }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) + }) + + itDoesNotExecuteTheCall(from) + }) + } + + const itExecutesTheCallUnlessItsDeniedAndSenderIsNotOwner = () => { + context('when the sender is the owner', () => { + itExecutesTheCallEvenIfDenied(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCallWhenNotDenied(anyone) + }) + } + + context('when there is no bug registered', () => { + itExecutesTheCallUnlessItsDeniedAndSenderIsNotOwner() }) context('when there is a bug registered', () => { @@ -233,27 +277,31 @@ contract('AppBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { }) context('when the bug was not fixed yet', () => { - context('when the contract being called is not ignored', () => { - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) + context('when the sender is the owner', () => { + itExecutesTheCallEvenIfDenied(owner) + }) - context('when the sender is not the owner', () => { + context('when the sender is not the owner', () => { + context('when the contract being called is checked', () => { itDoesNotExecuteTheCall(anyone) }) - }) - context('when the contract being called is ignored', () => { - beforeEach('ignore calling contract', async () => { - await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) - }) + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) + }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) + context('when the sender is not the owner', () => { + itExecutesTheCall(anyone) + }) }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) + }) + + itDoesNotExecuteTheCall(anyone) }) }) }) @@ -263,29 +311,7 @@ contract('AppBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) }) - context('when the contract being called is not ignored', () => { - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) - }) - - context('when the contract being called is ignored', () => { - beforeEach('ignore calling contract', async () => { - await appKillSwitch.setContractIgnore(appBase.address, true, { from: owner }) - }) - - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) - }) + itExecutesTheCallUnlessItsDeniedAndSenderIsNotOwner() }) }) }) diff --git a/test/kill_switch/app/AppSeveritiesKillSwitch.test.js b/test/kill_switch/app/AppSeveritiesKillSwitch.test.js index bfe9455a1..160080212 100644 --- a/test/kill_switch/app/AppSeveritiesKillSwitch.test.js +++ b/test/kill_switch/app/AppSeveritiesKillSwitch.test.js @@ -1,4 +1,6 @@ +const { ACTION, SEVERITY } = require('../helpers/enums') const { assertRevert } = require('../../helpers/assertThrow') +const { getEventArgument } = require('../helpers/events') const itBehavesLikeSeveritiesKillSwitch = require('../base/itBehavesLikeSeveritiesKillSwitch') const IssuesRegistry = artifacts.require('IssuesRegistry') @@ -10,10 +12,6 @@ const Kernel = artifacts.require('Kernel') const DAOFactory = artifacts.require('DAOFactory') const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') -const SEVERITY = { NONE: 0, LOW: 1, MID: 2, HIGH: 3, CRITICAL: 4 } - -const getEventArgument = (receipt, event, arg) => receipt.logs.find(l => l.event === event).args[arg] - contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) => { let kernelBase, aclBase, appBase, appKillSwitchBase, issuesRegistryBase let registryFactory, dao, acl, issuesRegistry, app, appKillSwitch @@ -48,6 +46,8 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) const appKillSwitchReceipt = await dao.newAppInstance('0x1235', appKillSwitchBase.address, '0x', false, { from: root }) appKillSwitch = AppSeveritiesKillSwitch.at(getEventArgument(appKillSwitchReceipt, 'NewAppProxy', 'proxy')) await appKillSwitch.initialize(issuesRegistry.address) + const SET_CONTRACT_ACTION_ROLE = await appKillSwitchBase.SET_CONTRACT_ACTION_ROLE() + await acl.createPermission(owner, appKillSwitch.address, SET_CONTRACT_ACTION_ROLE, root, { from: root }) const SET_LOWEST_ALLOWED_SEVERITY_ROLE = await appKillSwitchBase.SET_LOWEST_ALLOWED_SEVERITY_ROLE() await acl.createPermission(owner, appKillSwitch.address, SET_LOWEST_ALLOWED_SEVERITY_ROLE, root, { from: root }) }) @@ -68,15 +68,30 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) describe('integration', () => { context('when the function being called is not tagged', () => { - const itExecutesTheCall = () => { - it('executes the call', async () => { - assert.equal(await app.read(), 42) + + const itExecutesTheCallEvenIfDenied = () => { + const itExecutesTheCall = () => { + it('executes the call', async () => { + assert.equal(await app.read(), 42) + }) + } + + context('when the contract being called is not denied', () => { + itExecutesTheCall() + }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) + }) + + itExecutesTheCall() }) } context('when there is no bug registered', () => { context('when there is no lowest allowed severity set for the contract being called', () => { - itExecutesTheCall() + itExecutesTheCallEvenIfDenied() }) context('when there is a lowest allowed severity set for the contract being called', () => { @@ -84,7 +99,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) }) - itExecutesTheCall() + itExecutesTheCallEvenIfDenied() }) }) @@ -94,16 +109,12 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) }) context('when there is no lowest allowed severity set for the contract being called', () => { - itExecutesTheCall() + itExecutesTheCallEvenIfDenied() }) context('when there is a lowest allowed severity set for the contract being called', () => { context('when the lowest allowed severity is under the reported bug severity', () => { - beforeEach('set lowest allowed severity', async () => { - await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) - }) - - itExecutesTheCall() + itExecutesTheCallEvenIfDenied() }) context('when the lowest allowed severity is equal to the reported bug severity', () => { @@ -111,7 +122,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) }) - itExecutesTheCall() + itExecutesTheCallEvenIfDenied() }) context('when the lowest allowed severity is greater than the reported bug severity', () => { @@ -119,7 +130,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) }) - itExecutesTheCall() + itExecutesTheCallEvenIfDenied() }) }) }) @@ -127,22 +138,44 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) context('when the function being called is tagged', () => { describe('when the function being called is always evaluated', () => { - const itExecutesTheCall = (from = owner) => { + const itExecutesTheCall = () => { it('executes the call', async () => { - await app.write(10, { from }) + await app.write(10, { from: owner }) assert.equal(await app.read(), 10) }) } - const itDoesNotExecuteTheCall = (from = owner) => { + const itDoesNotExecuteTheCall = () => { it('does not execute the call', async () => { - await assertRevert(app.write(10, { from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') + await assertRevert(app.write(10, { from: owner }), 'APP_CONTRACT_CALL_NOT_ALLOWED') + }) + } + + const itExecutesTheCallWhenNotDenied = () => { + context('when the contract being called is checked', () => { + itExecutesTheCall() + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) + }) + + itExecutesTheCall() + }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) + }) + + itDoesNotExecuteTheCall() }) } context('when there is no bug registered', () => { context('when there is no lowest allowed severity set for the contract being called', () => { - itExecutesTheCall() + itExecutesTheCallWhenNotDenied() }) context('when there is a lowest allowed severity set for the contract being called', () => { @@ -150,7 +183,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) }) - itExecutesTheCall() + itExecutesTheCallWhenNotDenied() }) }) @@ -161,12 +194,24 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) context('when the bug was not fixed yet', () => { context('when there is no lowest allowed severity set for the contract being called', () => { - context('when the sender is the owner', () => { - itExecutesTheCall(owner) + context('when the contract being called is checked', () => { + itDoesNotExecuteTheCall() }) - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) + }) + + itExecutesTheCall() + }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) + }) + + itDoesNotExecuteTheCall() }) }) @@ -176,12 +221,24 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) }) - context('when the sender is the owner', () => { - itDoesNotExecuteTheCall(owner) + context('when the contract being called is checked', () => { + itDoesNotExecuteTheCall() }) - context('when the sender is not the owner', () => { - itDoesNotExecuteTheCall(anyone) + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) + }) + + itExecutesTheCall() + }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) + }) + + itDoesNotExecuteTheCall() }) }) @@ -190,13 +247,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) + itExecutesTheCallWhenNotDenied() }) context('when the lowest allowed severity is greater than the reported bug severity', () => { @@ -204,13 +255,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) + itExecutesTheCallWhenNotDenied() }) }) }) @@ -221,13 +266,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) }) context('when there is no lowest allowed severity set for the contract being called', () => { - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) + itExecutesTheCallWhenNotDenied() }) context('when there is a lowest allowed severity set for the contract being called', () => { @@ -236,13 +275,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) + itExecutesTheCallWhenNotDenied() }) context('when the lowest allowed severity is equal to the reported bug severity', () => { @@ -250,13 +283,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) + itExecutesTheCallWhenNotDenied() }) context('when the lowest allowed severity is greater than the reported bug severity', () => { @@ -264,13 +291,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) + itExecutesTheCallWhenNotDenied() }) }) }) @@ -278,22 +299,76 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) }) describe('when the function being called is evaluated only when the sender is not the owner', () => { - const itExecutesTheCall = (from = owner) => { + const itExecutesTheCall = (from) => { it('executes the call', async () => { await app.reset({ from }) assert.equal(await app.read(), 0) }) } - const itDoesNotExecuteTheCall = (from = owner) => { + const itDoesNotExecuteTheCall = (from) => { it('does not execute the call', async () => { await assertRevert(app.reset({ from }), 'APP_CONTRACT_CALL_NOT_ALLOWED') }) } + const itExecutesTheCallEvenIfDenied = (from) => { + context('when the contract being called is checked', () => { + itExecutesTheCall(from) + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) + }) + + itExecutesTheCall(from) + }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) + }) + + itExecutesTheCall(from) + }) + } + + const itExecutesTheCallWhenNotDenied = (from) => { + context('when the contract being called is checked', () => { + itExecutesTheCall(from) + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) + }) + + itExecutesTheCall(from) + }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) + }) + + itDoesNotExecuteTheCall(from) + }) + } + + const itExecutesTheCallUnlessItsDeniedAndSenderIsNotOwner = () => { + context('when the sender is the owner', () => { + itExecutesTheCallEvenIfDenied(owner) + }) + + context('when the sender is not the owner', () => { + itExecutesTheCallWhenNotDenied(anyone) + }) + } + context('when there is no bug registered', () => { context('when there is no lowest allowed severity set for the contract being called', () => { - itExecutesTheCall() + itExecutesTheCallUnlessItsDeniedAndSenderIsNotOwner() }) context('when there is a lowest allowed severity set for the contract being called', () => { @@ -301,7 +376,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) }) - itExecutesTheCall() + itExecutesTheCallUnlessItsDeniedAndSenderIsNotOwner() }) }) @@ -313,11 +388,29 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) context('when the bug was not fixed yet', () => { context('when there is no lowest allowed severity set for the contract being called', () => { context('when the sender is the owner', () => { - itExecutesTheCall(owner) + itExecutesTheCallEvenIfDenied(owner) }) context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) + context('when the contract being called is checked', () => { + itDoesNotExecuteTheCall(anyone) + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) + }) + + itExecutesTheCall(anyone) + }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) + }) + + itDoesNotExecuteTheCall(anyone) + }) }) }) @@ -328,11 +421,29 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) }) context('when the sender is the owner', () => { - itExecutesTheCall(owner) + itExecutesTheCallEvenIfDenied(owner) }) context('when the sender is not the owner', () => { - itDoesNotExecuteTheCall(anyone) + context('when the contract being called is checked', () => { + itDoesNotExecuteTheCall(anyone) + }) + + context('when the contract being called is ignored', () => { + beforeEach('ignore calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) + }) + + itExecutesTheCall(anyone) + }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await appKillSwitch.setContractAction(appBase.address, ACTION.DENY, { from: owner }) + }) + + itDoesNotExecuteTheCall(anyone) + }) }) }) @@ -341,13 +452,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) + itExecutesTheCallUnlessItsDeniedAndSenderIsNotOwner() }) context('when the lowest allowed severity is greater than the reported bug severity', () => { @@ -355,13 +460,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) + itExecutesTheCallUnlessItsDeniedAndSenderIsNotOwner() }) }) }) @@ -372,13 +471,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) }) context('when there is no lowest allowed severity set for the contract being called', () => { - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) + itExecutesTheCallUnlessItsDeniedAndSenderIsNotOwner() }) context('when there is a lowest allowed severity set for the contract being called', () => { @@ -387,13 +480,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.LOW, { from: owner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) + itExecutesTheCallUnlessItsDeniedAndSenderIsNotOwner() }) context('when the lowest allowed severity is equal to the reported bug severity', () => { @@ -401,13 +488,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.MID, { from: owner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) + itExecutesTheCallUnlessItsDeniedAndSenderIsNotOwner() }) context('when the lowest allowed severity is greater than the reported bug severity', () => { @@ -415,13 +496,7 @@ contract('AppSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) await appKillSwitch.setLowestAllowedSeverity(appBase.address, SEVERITY.CRITICAL, { from: owner }) }) - context('when the sender is the owner', () => { - itExecutesTheCall(owner) - }) - - context('when the sender is not the owner', () => { - itExecutesTheCall(anyone) - }) + itExecutesTheCallUnlessItsDeniedAndSenderIsNotOwner() }) }) }) diff --git a/test/kill_switch/base/IssuesRegistry.test.js b/test/kill_switch/base/IssuesRegistry.test.js index 5859c88e6..bdc78d618 100644 --- a/test/kill_switch/base/IssuesRegistry.test.js +++ b/test/kill_switch/base/IssuesRegistry.test.js @@ -1,4 +1,6 @@ +const { SEVERITY } = require('../helpers/enums') const { assertRevert } = require('../../helpers/assertThrow') +const { getEventArgument } = require('../helpers/events') const IssuesRegistry = artifacts.require('IssuesRegistry') const ACL = artifacts.require('ACL') @@ -6,10 +8,6 @@ const Kernel = artifacts.require('Kernel') const DAOFactory = artifacts.require('DAOFactory') const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') -const SEVERITY = { NONE: 0, LOW: 1, MID: 2, HIGH: 3, CRITICAL: 4 } - -const getEventArgument = (receipt, event, arg) => receipt.logs.find(l => l.event === event).args[arg] - contract('IssuesRegistry', ([_, root, implementation, owner, anyone]) => { let kernelBase, aclBase, issuesRegistryBase, registryFactory, dao, acl, issuesRegistry diff --git a/test/kill_switch/base/itBehavesLikeBinaryKillSwitch.js b/test/kill_switch/base/itBehavesLikeBinaryKillSwitch.js index 831ce6003..043845b39 100644 --- a/test/kill_switch/base/itBehavesLikeBinaryKillSwitch.js +++ b/test/kill_switch/base/itBehavesLikeBinaryKillSwitch.js @@ -1,50 +1,86 @@ +const { ACTION, SEVERITY } = require('../helpers/enums') const { assertRevert } = require('../../helpers/assertThrow') -module.exports = function (owner, address) { +module.exports = function (owner, anAddress) { describe('isContractIgnored', function () { - context('when the contract is not ignored', function () { + context('when the contract is checked', function () { it('returns false', async function () { - assert.isFalse(await this.killSwitch.isContractIgnored(address)) + assert.isFalse(await this.killSwitch.isContractIgnored(anAddress)) }) }) context('when the contract is ignored', function () { beforeEach('ignore contract', async function () { - await this.killSwitch.setContractIgnore(address, true, { from: owner }) + await this.killSwitch.setContractAction(anAddress, ACTION.IGNORE, { from: owner }) }) it('returns true', async function () { - assert.isTrue(await this.killSwitch.isContractIgnored(address)) + assert.isTrue(await this.killSwitch.isContractIgnored(anAddress)) }) }) }) - describe('setContractIgnore', function () { + describe('isContractDenied', function () { + context('when the contract is not denied', function () { + it('returns false', async function () { + assert.isFalse(await this.killSwitch.isContractDenied(anAddress)) + }) + }) + + context('when the contract is ignored', function () { + beforeEach('ignore contract', async function () { + await this.killSwitch.setContractAction(anAddress, ACTION.DENY, { from: owner }) + }) + + it('returns true', async function () { + assert.isTrue(await this.killSwitch.isContractDenied(anAddress)) + }) + }) + }) + + describe('setContractAction', function () { context('when the sender is the owner', function () { const from = owner - context('ignoring a contract', function () { - it('ignores the contract', async function () { - await this.killSwitch.setContractIgnore(address, true, { from }) + context('when there was no action set yet', function () { + it('sets a new action', async function () { + await this.killSwitch.setContractAction(anAddress, ACTION.DENY, { from }) - assert.isTrue(await this.killSwitch.isContractIgnored(address)) + assert.isTrue(await this.killSwitch.isContractDenied(anAddress)) }) }) - context('reverting a contract ignore', function () { - it('reverts the contract ignore', async function () { - await this.killSwitch.setContractIgnore(address, true, { from }) - await this.killSwitch.setContractIgnore(address, false, { from }) + context('when there was an action already set', function () { + beforeEach('deny contract', async function () { + await this.killSwitch.setContractAction(anAddress, ACTION.DENY, { from }) + assert.isTrue(await this.killSwitch.isContractDenied(anAddress)) + }) + + it('changes the contract action', async function () { + await this.killSwitch.setContractAction(anAddress, ACTION.IGNORE, { from }) - assert.isFalse(await this.killSwitch.isContractIgnored(address)) + assert.isTrue(await this.killSwitch.isContractIgnored(anAddress)) + assert.isFalse(await this.killSwitch.isContractDenied(anAddress)) }) }) }) context('when the sender is not the owner', function () { it('reverts', async function () { - await assertRevert(this.killSwitch.setContractIgnore(address, true)) + await assertRevert(this.killSwitch.setContractAction(anAddress, ACTION.DENY)) }) }) }) + + describe('isSeverityIgnored', function () { + it('returns true for none severities', async function () { + assert.isTrue(await this.killSwitch.isSeverityIgnored(anAddress, SEVERITY.NONE)) + }) + + it('returns false for all the severities', async function () { + for (const key of Object.keys(SEVERITY).slice(1)) { + assert.isFalse(await this.killSwitch.isSeverityIgnored(anAddress, SEVERITY[key])) + } + }) + }) } diff --git a/test/kill_switch/base/itBehavesLikeSeveritiesKillSwitch.js b/test/kill_switch/base/itBehavesLikeSeveritiesKillSwitch.js index 821b53643..d6e8f0730 100644 --- a/test/kill_switch/base/itBehavesLikeSeveritiesKillSwitch.js +++ b/test/kill_switch/base/itBehavesLikeSeveritiesKillSwitch.js @@ -1,7 +1,77 @@ +const { ACTION, SEVERITY } = require('../helpers/enums') const { assertRevert } = require('../../helpers/assertThrow') -const SEVERITY = { NONE: 0, LOW: 1, MID: 2, HIGH: 3, CRITICAL: 4 } module.exports = function (owner, anAddress) { + describe('isContractIgnored', function () { + context('when the contract is checked', function () { + it('returns false', async function () { + assert.isFalse(await this.killSwitch.isContractIgnored(anAddress)) + }) + }) + + context('when the contract is ignored', function () { + beforeEach('ignore contract', async function () { + await this.killSwitch.setContractAction(anAddress, ACTION.IGNORE, { from: owner }) + }) + + it('returns true', async function () { + assert.isTrue(await this.killSwitch.isContractIgnored(anAddress)) + }) + }) + }) + + describe('isContractDenied', function () { + context('when the contract is not denied', function () { + it('returns false', async function () { + assert.isFalse(await this.killSwitch.isContractDenied(anAddress)) + }) + }) + + context('when the contract is ignored', function () { + beforeEach('ignore contract', async function () { + await this.killSwitch.setContractAction(anAddress, ACTION.DENY, { from: owner }) + }) + + it('returns true', async function () { + assert.isTrue(await this.killSwitch.isContractDenied(anAddress)) + }) + }) + }) + + describe('setContractAction', function () { + context('when the sender is the owner', function () { + const from = owner + + context('when there was no action set yet', function () { + it('sets a new action', async function () { + await this.killSwitch.setContractAction(anAddress, ACTION.DENY, { from }) + + assert.isTrue(await this.killSwitch.isContractDenied(anAddress)) + }) + }) + + context('when there was an action already set', function () { + beforeEach('deny contract', async function () { + await this.killSwitch.setContractAction(anAddress, ACTION.DENY, { from }) + assert.isTrue(await this.killSwitch.isContractDenied(anAddress)) + }) + + it('changes the contract action', async function () { + await this.killSwitch.setContractAction(anAddress, ACTION.IGNORE, { from }) + + assert.isTrue(await this.killSwitch.isContractIgnored(anAddress)) + assert.isFalse(await this.killSwitch.isContractDenied(anAddress)) + }) + }) + }) + + context('when the sender is not the owner', function () { + it('reverts', async function () { + await assertRevert(this.killSwitch.setContractAction(anAddress, ACTION.DENY)) + }) + }) + }) + describe('isSeverityIgnored', function () { context('when no lowest allowed severity was set yet', function () { it('returns false for all the severities', async function () { diff --git a/test/kill_switch/helpers/enums.js b/test/kill_switch/helpers/enums.js new file mode 100644 index 000000000..01d118b73 --- /dev/null +++ b/test/kill_switch/helpers/enums.js @@ -0,0 +1,7 @@ +const ACTION = { CHECK: 0, IGNORE: 1, DENY: 2 } +const SEVERITY = { NONE: 0, LOW: 1, MID: 2, HIGH: 3, CRITICAL: 4 } + +module.exports = { + ACTION, + SEVERITY +} diff --git a/test/kill_switch/helpers/events.js b/test/kill_switch/helpers/events.js new file mode 100644 index 000000000..de6e1b63a --- /dev/null +++ b/test/kill_switch/helpers/events.js @@ -0,0 +1,5 @@ +const getEventArgument = (receipt, event, arg) => receipt.logs.find(l => l.event === event).args[arg] + +module.exports = { + getEventArgument +} diff --git a/test/kill_switch/kernel/KernelBinaryKillSwitch.test.js b/test/kill_switch/kernel/KernelBinaryKillSwitch.test.js index e9dc5e572..91be1ff6b 100644 --- a/test/kill_switch/kernel/KernelBinaryKillSwitch.test.js +++ b/test/kill_switch/kernel/KernelBinaryKillSwitch.test.js @@ -1,4 +1,6 @@ const { assertRevert } = require('../../helpers/assertThrow') +const { ACTION, SEVERITY } = require('../helpers/enums') +const { getEventArgument } = require('../helpers/events') const itBehavesLikeBinaryKillSwitch = require('../base/itBehavesLikeBinaryKillSwitch') const IssuesRegistry = artifacts.require('IssuesRegistry') @@ -10,10 +12,6 @@ const KernelKillSwitch = artifacts.require('KernelBinaryKillSwitch') const DAOFactory = artifacts.require('DAOFactory') const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') -const SEVERITY = { NONE: 0, LOW: 1, MID: 2, HIGH: 3, CRITICAL: 4 } - -const getEventArgument = (receipt, event, arg) => receipt.logs.find(l => l.event === event).args[arg] - contract('KernelBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) => { let killSwitchedKernelBase, regularKernelBase, aclBase, appBase, issuesRegistryBase, registryFactory let regularDao, regularAcl, killSwitchedDao, killSwitchedAcl, issuesRegistry, app @@ -53,8 +51,8 @@ contract('KernelBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) = const APP_MANAGER_ROLE = await killSwitchedKernelBase.APP_MANAGER_ROLE() await killSwitchedAcl.createPermission(root, killSwitchedDao.address, APP_MANAGER_ROLE, root, { from: root }) - const SET_IGNORED_CONTRACTS_ROLE = await killSwitchedDao.SET_IGNORED_CONTRACTS_ROLE() - await killSwitchedAcl.createPermission(owner, killSwitchedDao.address, SET_IGNORED_CONTRACTS_ROLE, root, { from: root }) + const SET_CONTRACT_ACTION_ROLE = await killSwitchedDao.SET_CONTRACT_ACTION_ROLE() + await killSwitchedAcl.createPermission(owner, killSwitchedDao.address, SET_CONTRACT_ACTION_ROLE, root, { from: root }) }) beforeEach('create sample app from DAO with kernel binary kill switch', async () => { @@ -84,18 +82,30 @@ contract('KernelBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) = }) } - context('when there is no bug registered', () => { - context('when the contract being called is not ignored', () => { + const itExecutesTheCallWhenNotDenied = () => { + context('when the contract being called is checked', () => { itExecutesTheCall() }) context('when the contract being called is ignored', () => { beforeEach('ignore calling contract', async () => { - await killSwitchedDao.setContractIgnore(appBase.address, true, { from: owner }) + await killSwitchedDao.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) }) itExecutesTheCall() }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await killSwitchedDao.setContractAction(appBase.address, ACTION.DENY, { from: owner }) + }) + + itDoesNotExecuteTheCall() + }) + } + + context('when there is no bug registered', () => { + itExecutesTheCallWhenNotDenied() }) context('when there is a bug registered', () => { @@ -104,17 +114,25 @@ contract('KernelBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) = }) context('when the bug was not fixed yet', () => { - context('when the contract being called is not ignored', () => { + context('when the contract being called is checked', () => { itDoesNotExecuteTheCall() }) context('when the contract being called is ignored', () => { beforeEach('ignore calling contract', async () => { - await killSwitchedDao.setContractIgnore(appBase.address, true, { from: owner }) + await killSwitchedDao.setContractAction(appBase.address, ACTION.IGNORE, { from: owner }) }) itExecutesTheCall() }) + + context('when the contract being called is denied', () => { + beforeEach('deny calling contract', async () => { + await killSwitchedDao.setContractAction(appBase.address, ACTION.DENY, { from: owner }) + }) + + itDoesNotExecuteTheCall() + }) }) context('when the bug was already fixed', () => { @@ -122,17 +140,7 @@ contract('KernelBinaryKillSwitch', ([_, root, owner, securityPartner, anyone]) = await issuesRegistry.setSeverityFor(appBase.address, SEVERITY.NONE, { from: securityPartner }) }) - context('when the contract being called is not ignored', () => { - itExecutesTheCall() - }) - - context('when the contract being called is ignored', () => { - beforeEach('ignore calling contract', async () => { - await killSwitchedDao.setContractIgnore(appBase.address, true, { from: owner }) - }) - - itExecutesTheCall() - }) + itExecutesTheCallWhenNotDenied() }) }) }) diff --git a/test/kill_switch/kernel/KernelSeveritiesKillSwitch.test.js b/test/kill_switch/kernel/KernelSeveritiesKillSwitch.test.js index de4f48dc1..6165b5dff 100644 --- a/test/kill_switch/kernel/KernelSeveritiesKillSwitch.test.js +++ b/test/kill_switch/kernel/KernelSeveritiesKillSwitch.test.js @@ -1,4 +1,6 @@ +const { ACTION, SEVERITY } = require('../helpers/enums') const { assertRevert } = require('../../helpers/assertThrow') +const { getEventArgument } = require('../helpers/events') const itBehavesLikeSeveritiesKillSwitch = require('../base/itBehavesLikeSeveritiesKillSwitch') const IssuesRegistry = artifacts.require('IssuesRegistry') @@ -10,10 +12,6 @@ const KernelKillSwitch = artifacts.require('KernelSeveritiesKillSwitch') const DAOFactory = artifacts.require('DAOFactory') const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') -const SEVERITY = { NONE: 0, LOW: 1, MID: 2, HIGH: 3, CRITICAL: 4 } - -const getEventArgument = (receipt, event, arg) => receipt.logs.find(l => l.event === event).args[arg] - contract('KernelSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone]) => { let killSwitchedKernelBase, regularKernelBase, aclBase, appBase, issuesRegistryBase, registryFactory let regularDao, regularAcl, killSwitchedDao, killSwitchedAcl, issuesRegistry, app @@ -53,6 +51,8 @@ contract('KernelSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone const APP_MANAGER_ROLE = await killSwitchedKernelBase.APP_MANAGER_ROLE() await killSwitchedAcl.createPermission(root, killSwitchedDao.address, APP_MANAGER_ROLE, root, { from: root }) + const SET_CONTRACT_ACTION_ROLE = await killSwitchedDao.SET_CONTRACT_ACTION_ROLE() + await killSwitchedAcl.createPermission(owner, killSwitchedDao.address, SET_CONTRACT_ACTION_ROLE, root, { from: root }) const SET_LOWEST_ALLOWED_SEVERITY_ROLE = await killSwitchedDao.SET_LOWEST_ALLOWED_SEVERITY_ROLE() await killSwitchedAcl.createPermission(owner, killSwitchedDao.address, SET_LOWEST_ALLOWED_SEVERITY_ROLE, root, { from: root }) }) @@ -105,7 +105,7 @@ contract('KernelSeveritiesKillSwitch', ([_, root, owner, securityPartner, anyone context('when the bug was not fixed yet', () => { context('when there is no lowest allowed severity set for the contract being called', () => { - itExecutesTheCall() + itDoesNotExecuteTheCall() }) context('when there is a lowest allowed severity set for the contract being called', () => { From 341c6c8b1849ecb30e64331b6347c281927ccef3 Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Mon, 29 Apr 2019 11:28:48 -0300 Subject: [PATCH 8/8] contracts: fix linting issues --- contracts/kill_switch/base/IssuesRegistry.sol | 2 +- contracts/kill_switch/base/KillSwitch.sol | 26 +++++++++++++------ .../kernel/KernelBinaryKillSwitch.sol | 4 ++- .../kill_switch/kernel/KernelKillSwitch.sol | 2 +- .../kernel/KernelSeveritiesKillSwitch.sol | 4 ++- 5 files changed, 26 insertions(+), 12 deletions(-) diff --git a/contracts/kill_switch/base/IssuesRegistry.sol b/contracts/kill_switch/base/IssuesRegistry.sol index 1cad627dd..8a84359fb 100644 --- a/contracts/kill_switch/base/IssuesRegistry.sol +++ b/contracts/kill_switch/base/IssuesRegistry.sol @@ -24,7 +24,7 @@ contract IssuesRegistry is AragonApp { return issuesSeverity[entry]; } - function setSeverityFor(address entry, Severity severity) authP(SET_ENTRY_SEVERITY_ROLE, arr(entry, msg.sender)) public { + function setSeverityFor(address entry, Severity severity) public authP(SET_ENTRY_SEVERITY_ROLE, arr(entry, msg.sender)) { issuesSeverity[entry] = severity; emit SeveritySet(entry, severity, msg.sender); } diff --git a/contracts/kill_switch/base/KillSwitch.sol b/contracts/kill_switch/base/KillSwitch.sol index 3305cc938..0228e3f86 100644 --- a/contracts/kill_switch/base/KillSwitch.sol +++ b/contracts/kill_switch/base/KillSwitch.sol @@ -14,12 +14,12 @@ contract KillSwitch { event IssuesRegistrySet(address issuesRegistry, address sender); event ContractActionSet(address contractAddress, ContractAction action); + function setContractAction(address _contract, ContractAction _action) external; + function getContractAction(address _contract) public view returns (ContractAction) { return contractActions[_contract]; } - function setContractAction(address _contract, ContractAction _action) external; - function isSeverityIgnored(address _contract, IssuesRegistry.Severity _severity) public view returns (bool); function isContractIgnored(address _contract) public view returns (bool) { @@ -32,20 +32,30 @@ contract KillSwitch { function shouldDenyCallingContract(address _base, address _instance, address _sender, bytes _data, uint256 _value) public returns (bool) { // if the call should not be evaluated, then allow given call - if (!_shouldEvaluateCall(_base, _instance, _sender, _data, _value)) return false; + if (!_shouldEvaluateCall(_base, _instance, _sender, _data, _value)) { + return false; + } // if the call should be denied, then deny given call - if (isContractDenied(_base)) return true; + if (isContractDenied(_base)) { + return true; + } // if the contract issues are ignored, then allow given call - if (isContractIgnored(_base)) return false; + if (isContractIgnored(_base)) { + return false; + } // if the issues registry has not been set, then allow given call - if (issuesRegistry == address(0)) return false; + if (issuesRegistry == address(0)) { + return false; + } // if the contract severity found is ignored, then allow given call IssuesRegistry.Severity _severityFound = issuesRegistry.getSeverityFor(_base); - if (isSeverityIgnored(_base, _severityFound)) return false; + if (isSeverityIgnored(_base, _severityFound)) { + return false; + } // if none of the conditions above were met, then deny given call return true; @@ -58,7 +68,7 @@ contract KillSwitch { * block information, among many other options. * @return Always true by default. */ - function _shouldEvaluateCall(address /*_base*/, address /*_instance*/, address /*_sender*/, bytes /*_data*/, uint256 /*_value*/) internal returns (bool) { + function _shouldEvaluateCall(address, address, address, bytes, uint256) internal returns (bool) { return true; } diff --git a/contracts/kill_switch/kernel/KernelBinaryKillSwitch.sol b/contracts/kill_switch/kernel/KernelBinaryKillSwitch.sol index 8c8d3eeb7..f3f3749b1 100644 --- a/contracts/kill_switch/kernel/KernelBinaryKillSwitch.sol +++ b/contracts/kill_switch/kernel/KernelBinaryKillSwitch.sol @@ -5,7 +5,9 @@ import "../base/BinaryKillSwitch.sol"; contract KernelBinaryKillSwitch is KernelKillSwitch, BinaryKillSwitch { - constructor(bool _shouldPetrify) Kernel(_shouldPetrify) public {} + constructor(bool _shouldPetrify) Kernel(_shouldPetrify) public { + // solium-disable-previous-line no-empty-blocks + } function setContractAction(address _contract, ContractAction _action) external diff --git a/contracts/kill_switch/kernel/KernelKillSwitch.sol b/contracts/kill_switch/kernel/KernelKillSwitch.sol index 16c96a691..8f2261509 100644 --- a/contracts/kill_switch/kernel/KernelKillSwitch.sol +++ b/contracts/kill_switch/kernel/KernelKillSwitch.sol @@ -35,6 +35,6 @@ contract KernelKillSwitch is Kernel, KillSwitch { /* not correspond to the application call in this context. */ /**************************************************************************************************/ - return super._shouldEvaluateCall(_base, _instance ,_sender, _data, _value); + return super._shouldEvaluateCall(_base, _instance, _sender, _data, _value); } } diff --git a/contracts/kill_switch/kernel/KernelSeveritiesKillSwitch.sol b/contracts/kill_switch/kernel/KernelSeveritiesKillSwitch.sol index 00503324b..5dc83147c 100644 --- a/contracts/kill_switch/kernel/KernelSeveritiesKillSwitch.sol +++ b/contracts/kill_switch/kernel/KernelSeveritiesKillSwitch.sol @@ -5,7 +5,9 @@ import "../base/SeveritiesKillSwitch.sol"; contract KernelSeveritiesKillSwitch is KernelKillSwitch, SeveritiesKillSwitch { - constructor(bool _shouldPetrify) Kernel(_shouldPetrify) public {} + constructor(bool _shouldPetrify) Kernel(_shouldPetrify) public { + // solium-disable-previous-line no-empty-blocks + } function setContractAction(address _contract, ContractAction _action) external