From 82055ebc20c72d2c903f3e7ce26fb11ac0babfff Mon Sep 17 00:00:00 2001 From: razvan Date: Thu, 11 Apr 2019 12:24:44 +0200 Subject: [PATCH 1/4] add onlyAction modifier --- contracts/Identity.sol | 14 +++++--------- contracts/KeyManager.sol | 11 ++++++++++- test/Identity.test.js | 4 ++-- test/KeyManager.test.js | 2 +- 4 files changed, 18 insertions(+), 13 deletions(-) diff --git a/contracts/Identity.sol b/contracts/Identity.sol index 374170b..3004413 100644 --- a/contracts/Identity.sol +++ b/contracts/Identity.sol @@ -64,24 +64,20 @@ contract Identity is KeyManager { address to, uint256 value, bytes memory data - ) + ) onlyAction public returns (bool success) { - - bytes32 key_ = addressToKey(msg.sender); - require( - keyHasPurpose(key_, ACTION) && _keys[key_].revokedAt == 0, - "Requester must have an ACTION purpose" - ); - // solium-disable-next-line security/no-inline-assembly assembly { - // check if contract to be call exists + // check if contract to be called exists if iszero(extcodesize(to)) { revert(0, 0) } success := call(gas, to, value, add(data, 0x20), mload(data), 0, 0) } } + + + } diff --git a/contracts/KeyManager.sol b/contracts/KeyManager.sol index c25643c..37e7746 100644 --- a/contracts/KeyManager.sol +++ b/contracts/KeyManager.sol @@ -209,7 +209,16 @@ contract KeyManager { bytes32 key_ = addressToKey(msg.sender); require( keyHasPurpose(key_, MANAGEMENT) && _keys[key_].revokedAt == 0, - "No management right" + "Sender must have MANAGEMENT purpose" + ); + _; + } + + modifier onlyAction() { + bytes32 key_ = addressToKey(msg.sender); + require( + keyHasPurpose(key_, ACTION) && _keys[key_].revokedAt == 0, + "Sender must have ACTION purpose" ); _; } diff --git a/test/Identity.test.js b/test/Identity.test.js index 82c2a67..879893e 100644 --- a/test/Identity.test.js +++ b/test/Identity.test.js @@ -37,7 +37,7 @@ contract("Identity", function (accounts) { const data = this.testProxy.contract.methods.callMe().encodeABI(); await shouldRevert( this.identity.execute(this.testProxy.address, 0, data), - "Requester must have an ACTION purpose" + "Sender must have ACTION purpose" ); }) @@ -56,7 +56,7 @@ contract("Identity", function (accounts) { const data = this.testProxy.contract.methods.callMe().encodeABI(); await shouldRevert( this.identity.execute(this.testProxy.address, 0, data, {from: accounts[2]}), - "Requester must have an ACTION purpose" + "Sender must have ACTION purpose" ); }) diff --git a/test/KeyManager.test.js b/test/KeyManager.test.js index 5dee97b..7880202 100644 --- a/test/KeyManager.test.js +++ b/test/KeyManager.test.js @@ -40,7 +40,7 @@ contract("KeyManager", function (accounts) { await this.identity.revokeKey(addressToBytes32(accounts[1])); await shouldRevert( this.identity.addKey(key, P2P_IDENTITY, 1, {from: accounts[1]}), - "No management right" + "Sender must have MANAGEMENT purpose" ); }) From 02791e34c044e03cbcd2973934d1667816635ee3 Mon Sep 17 00:00:00 2001 From: razvan Date: Thu, 11 Apr 2019 20:41:55 +0200 Subject: [PATCH 2/4] add transferEth method --- contracts/Identity.sol | 24 ++++++++++++++++++++++++ test/Identity.test.js | 13 +++++++++++-- test/Utilities.test.js | 3 --- 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/contracts/Identity.sol b/contracts/Identity.sol index 3004413..af3d02e 100644 --- a/contracts/Identity.sol +++ b/contracts/Identity.sol @@ -77,6 +77,30 @@ contract Identity is KeyManager { success := call(gas, to, value, add(data, 0x20), mload(data), 0, 0) } } + /** + * @dev Transfer eth + * @param to address of account where to transfer eth. + * it can not be a contract. Use execute method in that case + * @param value uint256 wei supply for proxy execution + * @param data bytes ABI encoded call data + */ + function transferEth( + address to, + uint256 value, + bytes memory data + ) onlyAction + public + returns (bool success) + { + // solium-disable-next-line security/no-inline-assembly + assembly { + // Make sure the address is not a contract + if gt(extcodesize(to),0) { + revert(0, 0) + } + success := call(gas, to, value, add(data, 0x20), mload(data), 0, 0) + } + } diff --git a/test/Identity.test.js b/test/Identity.test.js index 879893e..3610671 100644 --- a/test/Identity.test.js +++ b/test/Identity.test.js @@ -45,12 +45,16 @@ contract("Identity", function (accounts) { it('should execute contract method for identity ACTION key', async function () { const data = this.testProxy.contract.methods.callMe().encodeABI(); await this.identity.addKey(addressToBytes32(accounts[1]), ACTION, 1); - shouldSucceed(await this.identity.execute(this.testProxy.address, 0, data, {from: accounts[1]})); + await shouldSucceed(this.identity.execute(this.testProxy.address, 0, data, {from: accounts[1]})); const numOfCalls = await this.testProxy.getCallsFrom(this.identity.address); assert.equal(1, numOfCalls.toNumber()); }) + it('should transferEth using the a identity ACTION key', async function () { + await this.identity.addKey(addressToBytes32(accounts[1]), ACTION, 1); + await shouldSucceed(this.identity.transferEth(accounts[2], 1, "0x1234", {from: accounts[1]})); + }) it('should revert execute for non ACTION keys ', async function () { const data = this.testProxy.contract.methods.callMe().encodeABI(); @@ -64,9 +68,14 @@ contract("Identity", function (accounts) { const data = this.testProxy.contract.methods.callMe().encodeABI(); await this.identity.addKey(addressToBytes32(accounts[1]), ACTION, 1); await shouldRevert( - this.identity.execute(this.testProxy.address, 0, data, {from: accounts[2]}) + this.identity.execute(accounts[1], 0, data, {from: accounts[1]}) ); }); + + it('should faild to transferEth if the address is a contract', async function () { + await this.identity.addKey(addressToBytes32(accounts[1]), ACTION, 1); + await shouldRevert(this.identity.transferEth(this.testProxy.address, 1, "0x444", {from: accounts[1]})); + }) }) }); diff --git a/test/Utilities.test.js b/test/Utilities.test.js index 2e1a017..011c511 100644 --- a/test/Utilities.test.js +++ b/test/Utilities.test.js @@ -49,7 +49,6 @@ contract("Utilities", function (accounts) { it('it should return the same hex', async function () { const payload = web3.utils.randomHex(32); const result = await this.utilities.bytesToUint(payload); - console.log(web3.utils.toHex(result)); assert.equal(web3.utils.toHex(result), payload) }); @@ -84,7 +83,6 @@ contract("Utilities", function (accounts) { it('it should return the same byte32 hex', async function () { const payload = web3.utils.randomHex(32); const result = await this.utilities.uintToHexStr(payload); - console.log(result); assert.equal("0x" + result.toLowerCase(), payload) }); @@ -92,7 +90,6 @@ contract("Utilities", function (accounts) { it('it should return the same 0x65', async function () { const payload = 101; const result = await this.utilities.uintToHexStr(payload); - console.log(result); assert.equal("0x" + result.toLowerCase(), web3.utils.toHex(payload)) }); From 49c1c447545bfc5a7166002a7f893f60c92af738 Mon Sep 17 00:00:00 2001 From: razvan Date: Thu, 11 Apr 2019 20:46:56 +0200 Subject: [PATCH 3/4] add comment to onlyAction --- contracts/KeyManager.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contracts/KeyManager.sol b/contracts/KeyManager.sol index 37e7746..09e3e04 100644 --- a/contracts/KeyManager.sol +++ b/contracts/KeyManager.sol @@ -214,6 +214,9 @@ contract KeyManager { _; } + /** + * @dev Throws if called by any account other than a ACTION key. + */ modifier onlyAction() { bytes32 key_ = addressToKey(msg.sender); require( From 6c8caf547bf350ef73d511e7b5bc32f87e89e3f1 Mon Sep 17 00:00:00 2001 From: razvan Date: Fri, 12 Apr 2019 17:58:09 +0200 Subject: [PATCH 4/4] add payable to execute, upgrade Ganache(stuff costs more) and test balances --- contracts/Identity.sol | 2 + contracts/mock/TestProxyExecution.sol | 18 ++ package-lock.json | 394 ++++++++++++++++++++++++-- package.json | 2 +- test/Gas.test.js | 10 +- test/Identity.test.js | 26 +- 6 files changed, 424 insertions(+), 28 deletions(-) diff --git a/contracts/Identity.sol b/contracts/Identity.sol index af3d02e..be8c0dd 100644 --- a/contracts/Identity.sol +++ b/contracts/Identity.sol @@ -66,6 +66,7 @@ contract Identity is KeyManager { bytes memory data ) onlyAction public + payable returns (bool success) { // solium-disable-next-line security/no-inline-assembly @@ -90,6 +91,7 @@ contract Identity is KeyManager { bytes memory data ) onlyAction public + payable returns (bool success) { // solium-disable-next-line security/no-inline-assembly diff --git a/contracts/mock/TestProxyExecution.sol b/contracts/mock/TestProxyExecution.sol index fa7595d..2b54f25 100644 --- a/contracts/mock/TestProxyExecution.sol +++ b/contracts/mock/TestProxyExecution.sol @@ -5,13 +5,23 @@ contract TextProxyExecution { // Counts calls by msg.sender mapping(address => uint) public numCalls; + mapping(address => uint) public paidCalls; function callMe() external + payable { numCalls[msg.sender] += 1; } + function callMeWithMoney() + external + payable + { + require(msg.value > 0); + paidCalls[msg.sender] += 1; + } + function getCallsFrom(address caller) external view @@ -19,4 +29,12 @@ contract TextProxyExecution { { return numCalls[caller]; } + + function getPayedCallsFrom(address caller) + external + view + returns (uint noOfCalls) + { + return paidCalls[caller]; + } } diff --git a/package-lock.json b/package-lock.json index a494118..6ccb8e8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3863,12 +3863,382 @@ "dev": true }, "ganache-cli": { - "version": "6.1.8", - "resolved": "https://registry.npmjs.org/ganache-cli/-/ganache-cli-6.1.8.tgz", - "integrity": "sha512-yXzteu4SIgUL31mnpm9j+x6dpHUw0p/nsRVkcySKq0w+1vDxH9jMErP1QhZAJuTVE6ni4nfvGSNkaQx5cD3jfg==", + "version": "6.4.2", + "resolved": "https://registry.npmjs.org/ganache-cli/-/ganache-cli-6.4.2.tgz", + "integrity": "sha512-2hZY5xjvfVI2fvTtqQwzW5kgiqv6gtOONdqnNfBO38GGwQIZKdZCeQNwpFHZTUqPZ/hVwT4u5J5WUYyP3fG2YA==", "dev": true, "requires": { - "source-map-support": "^0.5.3" + "bn.js": "4.11.8", + "source-map-support": "0.5.9", + "yargs": "11.1.0" + }, + "dependencies": { + "ansi-regex": { + "version": "3.0.0", + "bundled": true, + "dev": true + }, + "bn.js": { + "version": "4.11.8", + "bundled": true, + "dev": true + }, + "buffer-from": { + "version": "1.1.1", + "bundled": true, + "dev": true + }, + "camelcase": { + "version": "4.1.0", + "bundled": true, + "dev": true + }, + "cliui": { + "version": "4.1.0", + "bundled": true, + "dev": true, + "requires": { + "string-width": "^2.1.1", + "strip-ansi": "^4.0.0", + "wrap-ansi": "^2.0.0" + } + }, + "code-point-at": { + "version": "1.1.0", + "bundled": true, + "dev": true + }, + "cross-spawn": { + "version": "5.1.0", + "bundled": true, + "dev": true, + "requires": { + "lru-cache": "^4.0.1", + "shebang-command": "^1.2.0", + "which": "^1.2.9" + } + }, + "decamelize": { + "version": "1.2.0", + "bundled": true, + "dev": true + }, + "execa": { + "version": "0.7.0", + "bundled": true, + "dev": true, + "requires": { + "cross-spawn": "^5.0.1", + "get-stream": "^3.0.0", + "is-stream": "^1.1.0", + "npm-run-path": "^2.0.0", + "p-finally": "^1.0.0", + "signal-exit": "^3.0.0", + "strip-eof": "^1.0.0" + } + }, + "find-up": { + "version": "2.1.0", + "bundled": true, + "dev": true, + "requires": { + "locate-path": "^2.0.0" + } + }, + "get-caller-file": { + "version": "1.0.3", + "bundled": true, + "dev": true + }, + "get-stream": { + "version": "3.0.0", + "bundled": true, + "dev": true + }, + "invert-kv": { + "version": "1.0.0", + "bundled": true, + "dev": true + }, + "is-fullwidth-code-point": { + "version": "2.0.0", + "bundled": true, + "dev": true + }, + "is-stream": { + "version": "1.1.0", + "bundled": true, + "dev": true + }, + "isexe": { + "version": "2.0.0", + "bundled": true, + "dev": true + }, + "lcid": { + "version": "1.0.0", + "bundled": true, + "dev": true, + "requires": { + "invert-kv": "^1.0.0" + } + }, + "locate-path": { + "version": "2.0.0", + "bundled": true, + "dev": true, + "requires": { + "p-locate": "^2.0.0", + "path-exists": "^3.0.0" + } + }, + "lru-cache": { + "version": "4.1.4", + "bundled": true, + "dev": true, + "requires": { + "pseudomap": "^1.0.2", + "yallist": "^3.0.2" + } + }, + "mem": { + "version": "1.1.0", + "bundled": true, + "dev": true, + "requires": { + "mimic-fn": "^1.0.0" + } + }, + "mimic-fn": { + "version": "1.2.0", + "bundled": true, + "dev": true + }, + "npm-run-path": { + "version": "2.0.2", + "bundled": true, + "dev": true, + "requires": { + "path-key": "^2.0.0" + } + }, + "number-is-nan": { + "version": "1.0.1", + "bundled": true, + "dev": true + }, + "os-locale": { + "version": "2.1.0", + "bundled": true, + "dev": true, + "requires": { + "execa": "^0.7.0", + "lcid": "^1.0.0", + "mem": "^1.1.0" + } + }, + "p-finally": { + "version": "1.0.0", + "bundled": true, + "dev": true + }, + "p-limit": { + "version": "1.3.0", + "bundled": true, + "dev": true, + "requires": { + "p-try": "^1.0.0" + } + }, + "p-locate": { + "version": "2.0.0", + "bundled": true, + "dev": true, + "requires": { + "p-limit": "^1.1.0" + } + }, + "p-try": { + "version": "1.0.0", + "bundled": true, + "dev": true + }, + "path-exists": { + "version": "3.0.0", + "bundled": true, + "dev": true + }, + "path-key": { + "version": "2.0.1", + "bundled": true, + "dev": true + }, + "pseudomap": { + "version": "1.0.2", + "bundled": true, + "dev": true + }, + "require-directory": { + "version": "2.1.1", + "bundled": true, + "dev": true + }, + "require-main-filename": { + "version": "1.0.1", + "bundled": true, + "dev": true + }, + "set-blocking": { + "version": "2.0.0", + "bundled": true, + "dev": true + }, + "shebang-command": { + "version": "1.2.0", + "bundled": true, + "dev": true, + "requires": { + "shebang-regex": "^1.0.0" + } + }, + "shebang-regex": { + "version": "1.0.0", + "bundled": true, + "dev": true + }, + "signal-exit": { + "version": "3.0.2", + "bundled": true, + "dev": true + }, + "source-map": { + "version": "0.6.1", + "bundled": true, + "dev": true + }, + "source-map-support": { + "version": "0.5.9", + "bundled": true, + "dev": true, + "requires": { + "buffer-from": "^1.0.0", + "source-map": "^0.6.0" + } + }, + "string-width": { + "version": "2.1.1", + "bundled": true, + "dev": true, + "requires": { + "is-fullwidth-code-point": "^2.0.0", + "strip-ansi": "^4.0.0" + } + }, + "strip-ansi": { + "version": "4.0.0", + "bundled": true, + "dev": true, + "requires": { + "ansi-regex": "^3.0.0" + } + }, + "strip-eof": { + "version": "1.0.0", + "bundled": true, + "dev": true + }, + "which": { + "version": "1.3.1", + "bundled": true, + "dev": true, + "requires": { + "isexe": "^2.0.0" + } + }, + "which-module": { + "version": "2.0.0", + "bundled": true, + "dev": true + }, + "wrap-ansi": { + "version": "2.1.0", + "bundled": true, + "dev": true, + "requires": { + "string-width": "^1.0.1", + "strip-ansi": "^3.0.1" + }, + "dependencies": { + "ansi-regex": { + "version": "2.1.1", + "bundled": true, + "dev": true + }, + "is-fullwidth-code-point": { + "version": "1.0.0", + "bundled": true, + "dev": true, + "requires": { + "number-is-nan": "^1.0.0" + } + }, + "string-width": { + "version": "1.0.2", + "bundled": true, + "dev": true, + "requires": { + "code-point-at": "^1.0.0", + "is-fullwidth-code-point": "^1.0.0", + "strip-ansi": "^3.0.0" + } + }, + "strip-ansi": { + "version": "3.0.1", + "bundled": true, + "dev": true, + "requires": { + "ansi-regex": "^2.0.0" + } + } + } + }, + "y18n": { + "version": "3.2.1", + "bundled": true, + "dev": true + }, + "yallist": { + "version": "3.0.2", + "bundled": true, + "dev": true + }, + "yargs": { + "version": "11.1.0", + "bundled": true, + "dev": true, + "requires": { + "cliui": "^4.0.0", + "decamelize": "^1.1.1", + "find-up": "^2.1.0", + "get-caller-file": "^1.0.1", + "os-locale": "^2.0.0", + "require-directory": "^2.1.1", + "require-main-filename": "^1.0.1", + "set-blocking": "^2.0.0", + "string-width": "^2.0.0", + "which-module": "^2.0.0", + "y18n": "^3.2.1", + "yargs-parser": "^9.0.2" + } + }, + "yargs-parser": { + "version": "9.0.2", + "bundled": true, + "dev": true, + "requires": { + "camelcase": "^4.1.0" + } + } } }, "get-caller-file": { @@ -7514,12 +7884,6 @@ } } }, - "source-map": { - "version": "0.6.1", - "resolved": "https://registry.npmjs.org/source-map/-/source-map-0.6.1.tgz", - "integrity": "sha512-UjgapumWlbMhkBgzT7Ykc5YXUT46F0iKu8SGXq0bcwP5dz/h0Plj6enJqjz1Zbq2l5WaqYnrVbwWOWMyF3F47g==", - "dev": true - }, "source-map-resolve": { "version": "0.5.2", "resolved": "https://registry.npmjs.org/source-map-resolve/-/source-map-resolve-0.5.2.tgz", @@ -7533,16 +7897,6 @@ "urix": "^0.1.0" } }, - "source-map-support": { - "version": "0.5.10", - "resolved": "https://registry.npmjs.org/source-map-support/-/source-map-support-0.5.10.tgz", - "integrity": "sha512-YfQ3tQFTK/yzlGJuX8pTwa4tifQj4QS2Mj7UegOu8jAz59MqIiMGPXxQhVQiIMNzayuUSF/jEuVnfFF5JqybmQ==", - "dev": true, - "requires": { - "buffer-from": "^1.0.0", - "source-map": "^0.6.0" - } - }, "source-map-url": { "version": "0.4.0", "resolved": "https://registry.npmjs.org/source-map-url/-/source-map-url-0.4.0.tgz", diff --git a/package.json b/package.json index 1bcbced..bcc3c00 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ "ethereumjs-util": "5.2.0", "ethereumjs-wallet": "0.6.2", "fs-extra": "^7.0.1", - "ganache-cli": "6.1.8", + "ganache-cli": "6.4.2", "husky": "^1.3.1", "semver": "^6.0.0", "solium": "^1.2.3", diff --git a/test/Gas.test.js b/test/Gas.test.js index 6ca4589..3366c00 100644 --- a/test/Gas.test.js +++ b/test/Gas.test.js @@ -99,7 +99,7 @@ contract("Gas costs", function (accounts) { describe("Check the gas cost for preCommit and commit with the identity proxy for ACTION key", async function () { const preCommitMaxGas = 95000; - const commitMaxGas = 85000; + const commitMaxGas = 175000; it(`should have preCommit gas cost less then ${preCommitMaxGas} `, async function () { const {anchorId, signingRoot, callOptions} = await getBasicTestNeeds(accounts); @@ -193,7 +193,7 @@ contract("Gas costs", function (accounts) { }); describe("check the gas cost for mint with the identity proxy for ACTION key", async function () { - const mintMaxGas = 833819; + const mintMaxGas = 862480; it(`should have mint gas cost less then ${mintMaxGas} `, async function () { await this.anchorRepository.commit( docIdPreImage, @@ -223,3 +223,9 @@ contract("Gas costs", function (accounts) { }); }); +/* +* +* +ACOUNTS 91582220760000000000 +ACOUNTS 100000000000000000000 +ACOUNTS 100000000000000000000*/ diff --git a/test/Identity.test.js b/test/Identity.test.js index 3610671..830b5ae 100644 --- a/test/Identity.test.js +++ b/test/Identity.test.js @@ -39,8 +39,7 @@ contract("Identity", function (accounts) { this.identity.execute(this.testProxy.address, 0, data), "Sender must have ACTION purpose" ); - }) - + }); it('should execute contract method for identity ACTION key', async function () { const data = this.testProxy.contract.methods.callMe().encodeABI(); @@ -50,10 +49,27 @@ contract("Identity", function (accounts) { assert.equal(1, numOfCalls.toNumber()); }) + it('should execute a payable contract method for identity ACTION key', async function () { + const data = this.testProxy.contract.methods.callMeWithMoney().encodeABI(); + await this.identity.addKey(addressToBytes32(accounts[1]), ACTION, 1); + const value = web3.utils.toWei("1", 'ether'); + await shouldSucceed(this.identity.execute(this.testProxy.address, value, data, {from: accounts[1], value})); + const numOfCalls = await this.testProxy.getPayedCallsFrom(this.identity.address); + assert.equal(1, numOfCalls.toNumber()); + assert.equal(value, await web3.eth.getBalance(this.testProxy.address)); + }) + it('should transferEth using the a identity ACTION key', async function () { - await this.identity.addKey(addressToBytes32(accounts[1]), ACTION, 1); - await shouldSucceed(this.identity.transferEth(accounts[2], 1, "0x1234", {from: accounts[1]})); + await this.identity.addKey(addressToBytes32(accounts[2]), ACTION, 1); + const balanceBefore = await web3.eth.getBalance(accounts[1]); + const value = web3.utils.toWei("5", 'ether'); + await shouldSucceed(this.identity.transferEth(accounts[1], value, "0x1234", {from: accounts[2], value})); + + const newBalance = (new web3.utils.BN(balanceBefore)).add(new web3.utils.BN(value)); + + assert.equal(newBalance, await web3.eth.getBalance(accounts[1])) + }) it('should revert execute for non ACTION keys ', async function () { @@ -72,7 +88,7 @@ contract("Identity", function (accounts) { ); }); - it('should faild to transferEth if the address is a contract', async function () { + it('should fail to transferEth if the address is a contract', async function () { await this.identity.addKey(addressToBytes32(accounts[1]), ACTION, 1); await shouldRevert(this.identity.transferEth(this.testProxy.address, 1, "0x444", {from: accounts[1]})); })