From 62953f54d40c4d37e34976b6bbb314b4e898f9bc Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Mon, 16 Jul 2018 04:20:18 +0200 Subject: [PATCH 1/2] ACL: log whether a permission was set with parameters --- contracts/acl/ACL.sol | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index e13bb36ac..0cb2ed6e1 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -43,6 +43,7 @@ contract ACL is IACL, AragonApp, ACLHelpers { // Hardcoded constant to save gas //bytes32 constant public EMPTY_PARAM_HASH = keccak256(uint256(0)); bytes32 constant public EMPTY_PARAM_HASH = 0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563; + bytes32 constant public NO_PERMISSION = bytes32(0); address constant public ANY_ENTITY = address(-1); modifier onlyPermissionManager(address _app, bytes32 _role) { @@ -50,7 +51,7 @@ contract ACL is IACL, AragonApp, ACLHelpers { _; } - event SetPermission(address indexed entity, address indexed app, bytes32 indexed role, bool allowed); + event SetPermission(address indexed entity, address indexed app, bytes32 indexed role, bool allowed, bool hasParams); event ChangePermissionManager(address indexed app, bytes32 indexed role, address indexed manager); /** @@ -120,7 +121,7 @@ contract ACL is IACL, AragonApp, ACLHelpers { onlyPermissionManager(_app, _role) external { - _setPermission(_entity, _app, _role, bytes32(0)); + _setPermission(_entity, _app, _role, NO_PERMISSION); } /** @@ -205,12 +206,12 @@ contract ACL is IACL, AragonApp, ACLHelpers { function hasPermission(address _who, address _where, bytes32 _what, uint256[] memory _how) public view returns (bool) { bytes32 whoParams = permissions[permissionHash(_who, _where, _what)]; - if (whoParams != bytes32(0) && evalParams(whoParams, _who, _where, _what, _how)) { + if (whoParams != NO_PERMISSION && evalParams(whoParams, _who, _where, _what, _how)) { return true; } bytes32 anyParams = permissions[permissionHash(ANY_ENTITY, _where, _what)]; - if (anyParams != bytes32(0) && evalParams(anyParams, ANY_ENTITY, _where, _what, _how)) { + if (anyParams != NO_PERMISSION && evalParams(anyParams, ANY_ENTITY, _where, _what, _how)) { return true; } @@ -253,8 +254,10 @@ contract ACL is IACL, AragonApp, ACLHelpers { */ function _setPermission(address _entity, address _app, bytes32 _role, bytes32 _paramsHash) internal { permissions[permissionHash(_entity, _app, _role)] = _paramsHash; + bool hasPermission = _paramsHash != NO_PERMISSION; + bool hasParams = hasPermission && _paramsHash != EMPTY_PARAM_HASH; - SetPermission(_entity, _app, _role, _paramsHash != bytes32(0)); + SetPermission(_entity, _app, _role, hasPermission, hasParams); } function _saveParams(uint256[] _encodedParams) internal returns (bytes32) { From c5ae5dfbad0921e5564771bf5bf4e4cd7435a083 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Fri, 27 Jul 2018 12:16:46 +0200 Subject: [PATCH 2/2] ACL: use a separate event to log permission params --- contracts/acl/ACL.sol | 8 ++++++-- package.json | 3 ++- test/kernel_acl.js | 13 ++++++++++++- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index 0cb2ed6e1..f9fe5843d 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -51,7 +51,8 @@ contract ACL is IACL, AragonApp, ACLHelpers { _; } - event SetPermission(address indexed entity, address indexed app, bytes32 indexed role, bool allowed, bool hasParams); + event SetPermission(address indexed entity, address indexed app, bytes32 indexed role, bool allowed); + event SetPermissionParams(address indexed entity, address indexed app, bytes32 indexed role, bytes32 paramsHash); event ChangePermissionManager(address indexed app, bytes32 indexed role, address indexed manager); /** @@ -257,7 +258,10 @@ contract ACL is IACL, AragonApp, ACLHelpers { bool hasPermission = _paramsHash != NO_PERMISSION; bool hasParams = hasPermission && _paramsHash != EMPTY_PARAM_HASH; - SetPermission(_entity, _app, _role, hasPermission, hasParams); + SetPermission(_entity, _app, _role, hasPermission); + if (hasParams) { + SetPermissionParams(_entity, _app, _role, _paramsHash); + } } function _saveParams(uint256[] _encodedParams) internal returns (bytes32) { diff --git a/package.json b/package.json index 963a116af..68022d7fc 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,8 @@ "solidity-coverage": "^0.3.5", "solium": "^1.1.2", "truffle": "4.0.5", - "truffle-config": "^1.0.4" + "truffle-config": "^1.0.4", + "web3-utils": "1.0.0-beta.33" }, "dependencies": { "homedir": "^0.6.0", diff --git a/test/kernel_acl.js b/test/kernel_acl.js index 7251d7d65..aee1e5a16 100644 --- a/test/kernel_acl.js +++ b/test/kernel_acl.js @@ -2,6 +2,7 @@ const { assertRevert } = require('./helpers/assertThrow') const Kernel = artifacts.require('Kernel') const KernelProxy = artifacts.require('KernelProxy') const { getBlockNumber } = require('./helpers/web3') +const { soliditySha3 } = require('web3-utils') const assertEvent = require('./helpers/assertEvent') const DAOFactory = artifacts.require('DAOFactory') @@ -91,6 +92,7 @@ contract('Kernel ACL', accounts => { beforeEach(async () => { const receipt = await acl.createPermission(granted, app, role, granted, { from: permissionsRoot }) assertEvent(receipt, 'SetPermission') + assertEvent(receipt, 'SetPermissionParams', 0) // should not have emitted this assertEvent(receipt, 'ChangePermissionManager') }) @@ -112,6 +114,12 @@ contract('Kernel ACL', accounts => { assert.equal(returnedParam[1].valueOf(), parseInt(op, 10), 'param op should match') assert.equal(returnedParam[2].valueOf(), parseInt(value, 10), 'param value should match') + // Assert that the right events have been emitted with the right args + assertEvent(r1, 'SetPermission') + assertEvent(r1, 'SetPermissionParams') + const setParamsHash = r1.logs.filter(l => l.event == 'SetPermissionParams')[0].args.paramsHash + assert.equal(setParamsHash, soliditySha3(param)) + // grants again without re-saving params const r2 = await acl.grantPermissionP(accounts[4], app, role, [param], { from: granted }) @@ -130,7 +138,10 @@ contract('Kernel ACL', accounts => { it('can grant a public permission', async () => { const anyEntity = "0xffffffffffffffffffffffffffffffffffffffff" - await acl.grantPermission(anyEntity, app, role, { from: granted }) + const receipt = await acl.grantPermission(anyEntity, app, role, { from: granted }) + assertEvent(receipt, 'SetPermission') + assertEvent(receipt, 'SetPermissionParams', 0) // should not have emitted this + // many entities can succesfully perform action // acl is used here just to provide an address which is a contract await kernel.setApp('0x121212', '0x00', acl.address, { from: accounts[4] })