From 9a07b523198d181435837d789d3eae6a03f7c41a Mon Sep 17 00:00:00 2001 From: Richard Ramos Date: Fri, 2 Mar 2018 15:00:46 -0400 Subject: [PATCH] Updated approval process - Managers can only execute actions on identity - Actors can execute actions outside identity - Validated minimum approvals to not exceed available keyTypes - Updated identity test unit to reflect changes --- contracts/identity/Identity.sol | 87 ++++++++++---------- contracts/identity/IdentityKernel.sol | 4 +- contracts/tests/UpdatedIdentityKernel.sol | 4 +- test/identity.js | 97 +++++++++++++++-------- 4 files changed, 112 insertions(+), 80 deletions(-) diff --git a/contracts/identity/Identity.sol b/contracts/identity/Identity.sol index acb85e8..64322a9 100644 --- a/contracts/identity/Identity.sol +++ b/contracts/identity/Identity.sol @@ -12,11 +12,11 @@ contract Identity is ERC725, ERC735 { mapping (uint256 => bytes32[]) claimsByType; mapping (bytes32 => uint256) indexes; mapping (uint => Transaction) txx; - mapping (uint256 => uint8) minimumApprovalsByKeyType; + mapping (uint256 => uint256) minimumApprovalsByKeyPurpose; bytes32[] pendingTransactions; uint nonce = 0; address recoveryContract; - address managerReset; + address recoveryManager; struct Transaction { address to; @@ -54,10 +54,11 @@ contract Identity is ERC725, ERC735 { } modifier managerOrActor(bytes32 _key) { + + require( isKeyType(bytes32(msg.sender), MANAGEMENT_KEY) || - isKeyType(bytes32(msg.sender), ACTION_KEY) || - (recoverySet && msg.sender == address(recoveryContract)) + isKeyType(bytes32(msg.sender), ACTION_KEY) ); _; } @@ -65,39 +66,42 @@ contract Identity is ERC725, ERC735 { function Identity() public { _addKey(bytes32(msg.sender), MANAGEMENT_KEY, 0); - minimumApprovalsByKeyType[MANAGEMENT_KEY] = 1; - minimumApprovalsByKeyType[ACTION_KEY] = 1; + minimumApprovalsByKeyPurpose[MANAGEMENT_KEY] = 1; + minimumApprovalsByKeyPurpose[ACTION_KEY] = 1; } function managerReset(address _newKey) external recoveryOnly { - managerReset = _newKey; - _addKey(managerReset, purpose, _newType); - minimumApprovalsByKeyType[MANAGEMENT_KEY] = keysByPurpose[MANAGEMENT_KEY].length; + recoveryManager = _newKey; + _addKey(bytes32(recoveryManager), MANAGEMENT_KEY, 0); + minimumApprovalsByKeyPurpose[MANAGEMENT_KEY] = keysByPurpose[MANAGEMENT_KEY].length; } - function processManagerReset(uint limit) - external + function processManagerReset(uint256 limit) + public { - require(managerReset != address(0)); - bytes32 newKey = bytes32(managerReset); + require(recoveryManager != address(0)); + bytes32 newKey = bytes32(recoveryManager); bytes32[] memory managers = keysByPurpose[MANAGEMENT_KEY]; - uint totalManagers = managers.lenght; + uint256 totalManagers = managers.length; + if (limit == 0) { limit = totalManagers; } - minimumApprovalsByKeyType[MANAGEMENT_KEY] = totalManagers - limit + 1; - for (uint i = 0; i < limit; i++) { - address manager = managers[i]; + + minimumApprovalsByKeyPurpose[MANAGEMENT_KEY] = totalManagers - limit + 1; + for (uint256 i = 0; i < limit; i++) { + bytes32 manager = managers[i]; if (manager != newKey) { _removeKey(manager, MANAGEMENT_KEY); totalManagers--; } } + if (totalManagers == 1) { - managerReset = address(0); + recoveryManager = address(0); } } @@ -123,7 +127,7 @@ contract Identity is ERC725, ERC735 { selfOnly returns (bool success) { - uint256 purpose = key[_oldKey].purpose; + uint256 purpose = keys[_oldKey].purpose; _addKey(_newKey, purpose, _newType); _removeKey(_oldKey, purpose); return true; @@ -170,52 +174,54 @@ contract Identity is ERC725, ERC735 { internal returns(bool success) { + Transaction storage trx = txx[_id]; - bytes32 managerKeyHash = keccak256(_key, MANAGEMENT_KEY); - bytes32 actorKeyHash = keccak256(_key, ACTION_KEY); - - uint8 approvalCount; - uint256 requiredKeyType; + uint256 approvalCount; + uint256 requiredKeyPurpose; Approved(_id, _approve); +Debug1(_key, isKeyType(_key, ACTION_KEY)); + if (trx.to == address(this)) { - requiredKeyType = MANAGEMENT_KEY; - if (keys[managerKeyHash].purpose == MANAGEMENT_KEY) { - approvalCount = _calculateApprovals(managerKeyHash, _approve, trx); - } + require(isKeyType(_key, MANAGEMENT_KEY)); + bytes32 managerKeyHash = keccak256(_key, MANAGEMENT_KEY); + requiredKeyPurpose = MANAGEMENT_KEY; + approvalCount = _calculateApprovals(managerKeyHash, _approve, trx); } else { - requiredKeyType = ACTION_KEY; - if (keys[managerKeyHash].purpose == ACTION_KEY) { - approvalCount = _calculateApprovals(actorKeyHash, _approve, trx); - } + require(isKeyType(_key, ACTION_KEY)); + bytes32 actorKeyHash = keccak256(_key, ACTION_KEY); + requiredKeyPurpose = ACTION_KEY; + approvalCount = _calculateApprovals(actorKeyHash, _approve, trx); } - if (approvalCount >= minimumApprovalsByKeyType[requiredKeyType]) { + if (approvalCount >= minimumApprovalsByKeyPurpose[requiredKeyPurpose]) { Executed(_id, trx.to, trx.value, trx.data); success = trx.to.call.value(trx.value)(trx.data); } } - +event Debug1(bytes32 d, bool b); function setMinimumApprovalsByKeyType( - uint256 _type, - uint8 _minimumApprovals + uint256 _purpose, + uint256 _minimumApprovals ) public selfOnly { + Debug(_minimumApprovals, keysByPurpose[_purpose].length, 0); require(_minimumApprovals > 0); - minimumApprovalsByKeyType[_type] = _minimumApprovals; + require(_minimumApprovals <= keysByPurpose[_purpose].length); + minimumApprovalsByKeyPurpose[_purpose] = _minimumApprovals; } - +event Debug(uint256 a, uint256 b, uint256 c); function _calculateApprovals( bytes32 _keyHash, bool _approve, Transaction storage trx ) private - returns (uint8 approvalCount) + returns (uint256 approvalCount) { require(trx.approvals[_keyHash] != _approve); @@ -403,9 +409,6 @@ contract Identity is ERC725, ERC735 { } delete keys[keyHash]; - - // MUST only be done by keys of purpose 1, or the identity itself. - // TODO If its the identity itself, the approval process will determine its approval. } function getKey( diff --git a/contracts/identity/IdentityKernel.sol b/contracts/identity/IdentityKernel.sol index 7966b8b..0f74cc4 100644 --- a/contracts/identity/IdentityKernel.sol +++ b/contracts/identity/IdentityKernel.sol @@ -6,8 +6,8 @@ import "./Identity.sol"; contract IdentityKernel is InstanceStorage, Identity { function initIdentity(address _caller) external { - require(minimumApprovalsByKeyType[MANAGEMENT_KEY] == 0); + require(minimumApprovalsByKeyPurpose[MANAGEMENT_KEY] == 0); _addKey(bytes32(_caller), MANAGEMENT_KEY, 0); - minimumApprovalsByKeyType[MANAGEMENT_KEY] = 1; + minimumApprovalsByKeyPurpose[MANAGEMENT_KEY] = 1; } } diff --git a/contracts/tests/UpdatedIdentityKernel.sol b/contracts/tests/UpdatedIdentityKernel.sol index ba1d288..23ad12b 100644 --- a/contracts/tests/UpdatedIdentityKernel.sol +++ b/contracts/tests/UpdatedIdentityKernel.sol @@ -5,9 +5,9 @@ import "../identity/IdentityKernel.sol"; contract UpdatedIdentityKernel is IdentityKernel { - event TestFunctionExecuted(uint8 minApprovalsByManagementKeys); + event TestFunctionExecuted(uint256 minApprovalsByManagementKeys); function test() public { - TestFunctionExecuted(minimumApprovalsByKeyType[MANAGEMENT_KEY]); + TestFunctionExecuted(minimumApprovalsByKeyPurpose[MANAGEMENT_KEY]); } } \ No newline at end of file diff --git a/test/identity.js b/test/identity.js index dd8bc45..f240209 100644 --- a/test/identity.js +++ b/test/identity.js @@ -65,12 +65,17 @@ contract('Identity', function(accounts) { {from: accounts[0]} ); - await identity.execute( - identity.address, - 0, - idUtils.encode.addKey(accounts[1], idUtils.purposes.MANAGEMENT, idUtils.types.ADDRESS), - {from: accounts[2]} - ); + try { + await identity.execute( + identity.address, + 0, + idUtils.encode.addKey(accounts[1], idUtils.purposes.MANAGEMENT, idUtils.types.ADDRESS), + {from: accounts[2]} + ); + assert.fail('should have reverted before'); + } catch(error) { + TestUtils.assertJump(error); + } assert.equal( await identity.getKeyPurpose(TestUtils.addressToBytes32(accounts[1])), @@ -115,7 +120,7 @@ contract('Identity', function(accounts) { identity.address+".getKeyPurpose("+accounts[1]+") is not 0") }); - it("other key should not removes a key", async () => { + it("other key should not remove a key", async () => { await identity.execute( identity.address, 0, @@ -156,12 +161,17 @@ contract('Identity', function(accounts) { {from: accounts[0]} ); - await identity.execute( - identity.address, - 0, - idUtils.encode.removeKey(accounts[1], idUtils.purposes.ACTION), - {from: accounts[2]} - ); + try { + await identity.execute( + identity.address, + 0, + idUtils.encode.removeKey(accounts[1], idUtils.purposes.ACTION), + {from: accounts[2]} + ); + assert.fail('should have reverted before'); + } catch(error) { + TestUtils.assertJump(error); + } assert.equal( await identity.getKeyPurpose(TestUtils.addressToBytes32(accounts[1])), @@ -313,18 +323,17 @@ contract('Identity', function(accounts) { "Test function was not executed"); }); - it("MANAGEMENT_KEY execute arbitrary transaction", async () => { - await identity.execute( - testContractInstance.address, - 0, - functionPayload, - {from: accounts[0]} - ); - - assert.notEqual( - await TestUtils.listenForEvent(testContractInstance.TestFunctionExecuted()), - undefined, - "Test function was not executed"); + it("MANAGEMENT_KEY cannot execute arbitrary transaction", async () => { + try { + await identity.execute( + testContractInstance.address, + 0, + functionPayload, + {from: accounts[0]} + ); + } catch(error) { + TestUtils.assertJump(error); + } }); it("Other keys NOT execute arbitrary transaction", async () => { @@ -342,7 +351,13 @@ contract('Identity', function(accounts) { }); - it("MANAGEMENT_KEY should send ether from contract", async () => { + it("ACTION_KEY should send ether from contract", async () => { + await identity.execute( + identity.address, + 0, + idUtils.encode.addKey(accounts[1], idUtils.purposes.ACTION, idUtils.types.ADDRESS), + {from: accounts[0]} + ); // Adding funds to contract await web3.eth.sendTransaction({from:accounts[0], to:identity.address, value: web3.toWei(0.05, "ether")}) @@ -350,28 +365,35 @@ contract('Identity', function(accounts) { const amountToSend = web3.toWei(0.01, "ether"); let idBalance0 = web3.eth.getBalance(identity.address); - let a1Balance0 = web3.eth.getBalance(accounts[1]); + let a2Balance0 = web3.eth.getBalance(accounts[2]); await identity.execute( - accounts[1], + accounts[2], amountToSend, '', - {from: accounts[0]} + {from: accounts[1]} ); let idBalance1 = web3.eth.getBalance(identity.address); - let a1Balance1 = web3.eth.getBalance(accounts[1]); + let a2Balance1 = web3.eth.getBalance(accounts[2]); assert(idBalance1.toNumber, idBalance0.toNumber - amountToSend, "Contract did not send ether"); - assert(a1Balance1.toNumber, a1Balance0.toNumber + amountToSend, accounts[1] + " did not receive ether"); + assert(a2Balance1.toNumber, a2Balance0.toNumber + amountToSend, accounts[2] + " did not receive ether"); }); it("fire ExecutionRequested(uint256 indexed executionId, address indexed to, uint256 indexed value, bytes data)", async () => { + await identity.execute( + identity.address, + 0, + idUtils.encode.addKey(accounts[1], idUtils.purposes.ACTION, idUtils.types.ADDRESS), + {from: accounts[0]} + ); + await identity.execute( testContractInstance.address, 0, functionPayload, - {from: accounts[0]} + {from: accounts[1]} ); const executionRequested = await TestUtils.listenForEvent(identity.ExecutionRequested()); @@ -381,11 +403,18 @@ contract('Identity', function(accounts) { }); it("fire Executed(uint256 indexed executionId, address indexed to, uint256 indexed value, bytes data)", async () => { + await identity.execute( + identity.address, + 0, + idUtils.encode.addKey(accounts[1], idUtils.purposes.ACTION, idUtils.types.ADDRESS), + {from: accounts[0]} + ); + await identity.execute( testContractInstance.address, 0, functionPayload, - {from: accounts[0]} + {from: accounts[1]} ); const executed = await TestUtils.listenForEvent(identity.Executed()); @@ -399,7 +428,7 @@ contract('Identity', function(accounts) { /* - describe("setMinimumApprovalsByKeyType(uint256 _type, uint8 _minimumApprovals)", () => { + describe("setMinimumApprovalsByKeyPurpose(uint256 _type, uint8 _minimumApprovals)", () => { it("MANAGEMENT_KEY should set minimum approvals for MANAGEMENT_KEYs", async () => { });