From ad46b86d1c051b1f07256b056d3ee5517a822c31 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Tue, 19 Mar 2019 17:57:29 +0100 Subject: [PATCH 01/15] fix: Use double quotes in contract imports (#494) * fix: Use double quotes in contract imports * fix: add source location of ENS contracts --- contracts/lib/ens/AbstractENS.sol | 2 ++ contracts/lib/ens/ENS.sol | 4 +++- contracts/lib/ens/PublicResolver.sol | 4 +++- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/contracts/lib/ens/AbstractENS.sol b/contracts/lib/ens/AbstractENS.sol index 497343b26..ff9d98654 100644 --- a/contracts/lib/ens/AbstractENS.sol +++ b/contracts/lib/ens/AbstractENS.sol @@ -1,3 +1,5 @@ +// See https://github.com/ensdomains/ens/blob/7e377df83f/contracts/AbstractENS.sol + pragma solidity ^0.4.15; diff --git a/contracts/lib/ens/ENS.sol b/contracts/lib/ens/ENS.sol index 41305c680..5ff394398 100644 --- a/contracts/lib/ens/ENS.sol +++ b/contracts/lib/ens/ENS.sol @@ -1,7 +1,9 @@ +// See https://github.com/ensdomains/ens/blob/7e377df83f/contracts/ENS.sol + pragma solidity ^0.4.0; -import './AbstractENS.sol'; +import "./AbstractENS.sol"; /** * The ENS registry contract. diff --git a/contracts/lib/ens/PublicResolver.sol b/contracts/lib/ens/PublicResolver.sol index fa934b94c..f0e51ff81 100644 --- a/contracts/lib/ens/PublicResolver.sol +++ b/contracts/lib/ens/PublicResolver.sol @@ -1,6 +1,8 @@ +// See https://github.com/ensdomains/ens/blob/7e377df83f/contracts/PublicResolver.sol + pragma solidity ^0.4.0; -import './AbstractENS.sol'; +import "./AbstractENS.sol"; /** * A simple resolver anyone can use; only allows the owner of a node to set its From a59fec6d4d5c75ac7192c54d5d31d88e74f813b8 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Wed, 20 Mar 2019 16:01:15 +0100 Subject: [PATCH 02/15] test: update bytes and address constants (#492) Updates the tests to use `EMPTY_BYTES` and `ZERO_ADDR` constants (should move these out to a shared lib sometime). Also fixes a few instances where we sent an invalid number of hex bytes (usually 1-length bytes like `0x1`; see https://github.com/trufflesuite/ganache-core/issues/283#issuecomment-470997764). --- test/apm_registry.js | 31 ++++++++++++++++------------- test/apm_repo.js | 8 +++++--- test/ens_subdomains.js | 11 +++++------ test/evm_script.js | 41 +++++++++++++++++++++------------------ test/kernel_acl.js | 18 +++++++++++++---- test/kernel_apps.js | 14 ++++++------- test/kernel_lifecycle.js | 17 ++++++++-------- test/recovery_to_vault.js | 11 ++++++----- 8 files changed, 85 insertions(+), 66 deletions(-) diff --git a/test/apm_registry.js b/test/apm_registry.js index fa83e14a3..286f1da1d 100644 --- a/test/apm_registry.js +++ b/test/apm_registry.js @@ -19,6 +19,9 @@ const APMRegistryFactoryMock = artifacts.require('APMRegistryFactoryMock') const getRepoFromLog = receipt => receipt.logs.filter(x => x.event == 'NewRepo')[0].args.repo +const EMPTY_BYTES = '0x' +const ZERO_ADDR = '0x0000000000000000000000000000000000000000' + contract('APMRegistry', accounts => { let baseDeployed, baseAddrs, ensFactory, apmFactory, daoFactory, ens, registry, acl const ensOwner = accounts[0] @@ -38,11 +41,11 @@ contract('APMRegistry', accounts => { const kernelBase = await Kernel.new(true) // petrify immediately const aclBase = await ACL.new() - daoFactory = await DAOFactory.new(kernelBase.address, aclBase.address, '0x00') + daoFactory = await DAOFactory.new(kernelBase.address, aclBase.address, ZERO_ADDR) }) beforeEach(async () => { - apmFactory = await APMRegistryFactory.new(daoFactory.address, ...baseAddrs, '0x0', ensFactory.address) + apmFactory = await APMRegistryFactory.new(daoFactory.address, ...baseAddrs, ZERO_ADDR, ensFactory.address) ens = ENS.at(await apmFactory.ens()) const receipt = await apmFactory.newAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner) @@ -60,7 +63,7 @@ contract('APMRegistry', accounts => { it('inits with existing ENS deployment', async () => { const receipt = await ensFactory.newENS(accounts[0]) const ens2 = ENS.at(receipt.logs.filter(l => l.event == 'DeployENS')[0].args.ens) - const newFactory = await APMRegistryFactory.new(daoFactory.address, ...baseAddrs, ens2.address, '0x00') + const newFactory = await APMRegistryFactory.new(daoFactory.address, ...baseAddrs, ens2.address, ZERO_ADDR) await ens2.setSubnodeOwner(namehash('eth'), '0x'+keccak256('aragonpm'), newFactory.address) const receipt2 = await newFactory.newAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner) @@ -81,12 +84,12 @@ contract('APMRegistry', accounts => { }) it('can create repo with version and dev can create new versions', async () => { - const receipt = await registry.newRepoWithVersion('test', repoDev, [1, 0, 0], '0x00', '0x00', { from: apmOwner }) + const receipt = await registry.newRepoWithVersion('test', repoDev, [1, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: apmOwner }) const repo = Repo.at(getRepoFromLog(receipt)) assert.equal(await repo.getVersionsCount(), 1, 'should have created version') - await repo.newVersion([2, 0, 0], '0x00', '0x00', { from: repoDev }) + await repo.newVersion([2, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: repoDev }) assert.equal(await repo.getVersionsCount(), 2, 'should have created version') }) @@ -94,7 +97,7 @@ contract('APMRegistry', accounts => { it('fails to init with existing ENS deployment if not owner of tld', async () => { const ensReceipt = await ensFactory.newENS(accounts[0]) const ens2 = ENS.at(ensReceipt.logs.filter(l => l.event == 'DeployENS')[0].args.ens) - const newFactory = await APMRegistryFactory.new(daoFactory.address, ...baseAddrs, ens2.address, '0x00') + const newFactory = await APMRegistryFactory.new(daoFactory.address, ...baseAddrs, ens2.address, ZERO_ADDR) // Factory doesn't have ownership over 'eth' tld await assertRevert(async () => { @@ -141,36 +144,36 @@ contract('APMRegistry', accounts => { }) it('repo dev can create versions', async () => { - await repo.newVersion([1, 0, 0], '0x00', '0x00', { from: repoDev }) - await repo.newVersion([2, 0, 0], '0x00', '0x00', { from: repoDev }) + await repo.newVersion([1, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: repoDev }) + await repo.newVersion([2, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: repoDev }) assert.equal(await repo.getVersionsCount(), 2, 'should have created versions') }) it('repo dev can authorize someone to interact with repo', async () => { - await repo.newVersion([1, 0, 0], '0x00', '0x00', { from: repoDev }) + await repo.newVersion([1, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: repoDev }) const newOwner = accounts[8] await acl.grantPermission(newOwner, repo.address, await repo.CREATE_VERSION_ROLE(), { from: repoDev }) - await repo.newVersion([2, 0, 0], '0x00', '0x00', { from: newOwner }) - await repo.newVersion([2, 1, 0], '0x00', '0x00', { from: repoDev }) // repoDev can still create them + await repo.newVersion([2, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: newOwner }) + await repo.newVersion([2, 1, 0], ZERO_ADDR, EMPTY_BYTES, { from: repoDev }) // repoDev can still create them assert.equal(await repo.getVersionsCount(), 3, 'should have created versions') }) it('repo dev can no longer create versions if permission is removed', async () => { - await repo.newVersion([1, 0, 0], '0x00', '0x00', { from: repoDev }) + await repo.newVersion([1, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: repoDev }) await acl.revokePermission(repoDev, repo.address, await repo.CREATE_VERSION_ROLE(), { from: repoDev }) return assertRevert(async () => { - await repo.newVersion([2, 0, 0], '0x00', '0x00', { from: repoDev }) + await repo.newVersion([2, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: repoDev }) }) }) it('cannot create versions if not in ACL', async () => { return assertRevert(async () => { - await repo.newVersion([1, 0, 0], '0x00', '0x00', { from: notOwner }) + await repo.newVersion([1, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: notOwner }) }) }) }) diff --git a/test/apm_repo.js b/test/apm_repo.js index 2a69ece3b..df46b892e 100644 --- a/test/apm_repo.js +++ b/test/apm_repo.js @@ -1,8 +1,10 @@ const { assertRevert } = require('./helpers/assertThrow') - const Repo = artifacts.require('UnsafeRepo') +const EMPTY_BYTES = '0x' +const ZERO_ADDR = '0x0000000000000000000000000000000000000000' + contract('Repo', accounts => { let repo @@ -31,7 +33,7 @@ contract('Repo', accounts => { // valid version as being a correct bump from 0.0.0 it('cannot create invalid first version', async () => { return assertRevert(async () => { - await repo.newVersion([1, 1, 0], '0x00', '0x00') + await repo.newVersion([1, 1, 0], ZERO_ADDR, EMPTY_BYTES) }) }) @@ -71,7 +73,7 @@ contract('Repo', accounts => { }) it('setting contract address to 0 reuses last version address', async () => { - await repo.newVersion([1, 1, 0], '0x00', initialContent) + await repo.newVersion([1, 1, 0], ZERO_ADDR, initialContent) assertVersion(await repo.getByVersionId(2), [1, 1, 0], initialCode, initialContent) }) diff --git a/test/ens_subdomains.js b/test/ens_subdomains.js index 62156bab2..b3f251469 100644 --- a/test/ens_subdomains.js +++ b/test/ens_subdomains.js @@ -17,6 +17,7 @@ const APMRegistryFactory = artifacts.require('APMRegistryFactory') const DAOFactory = artifacts.require('DAOFactory') const EMPTY_BYTES = '0x' +const ZERO_ADDR = '0x0000000000000000000000000000000000000000' // Using APMFactory in order to reuse it contract('ENSSubdomainRegistrar', accounts => { @@ -32,8 +33,6 @@ contract('ENSSubdomainRegistrar', accounts => { const holanode = namehash('hola.aragonpm.eth') const holalabel = '0x'+keccak256('hola') - const zeroAddr = '0x0000000000000000000000000000000000000000' - before(async () => { const bases = [APMRegistry, Repo, ENSSubdomainRegistrar] baseDeployed = await Promise.all(bases.map(base => base.new())) @@ -42,14 +41,14 @@ contract('ENSSubdomainRegistrar', accounts => { const kernelBase = await Kernel.new(true) // petrify immediately const aclBase = await ACL.new() - daoFactory = await DAOFactory.new(kernelBase.address, aclBase.address, '0x00') + daoFactory = await DAOFactory.new(kernelBase.address, aclBase.address, ZERO_ADDR) APP_BASES_NAMESPACE = await kernelBase.APP_BASES_NAMESPACE() }) beforeEach(async () => { const baseAddrs = baseDeployed.map(c => c.address) - apmFactory = await APMRegistryFactory.new(daoFactory.address, ...baseAddrs, '0x0', ensFactory.address) + apmFactory = await APMRegistryFactory.new(daoFactory.address, ...baseAddrs, ZERO_ADDR, ensFactory.address) ens = ENS.at(await apmFactory.ens()) const receipt = await apmFactory.newAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner) @@ -95,14 +94,14 @@ contract('ENSSubdomainRegistrar', accounts => { await registrar.createName(holalabel, apmOwner, { from: apmOwner }) await registrar.deleteName(holalabel, { from: apmOwner }) - assert.equal(await ens.owner(holanode), zeroAddr, 'should have reset name') + assert.equal(await ens.owner(holanode), ZERO_ADDR, 'should have reset name') }) it('can delete names registered to itself', async () => { await registrar.createName(holalabel, registrar.address, { from: apmOwner }) await registrar.deleteName(holalabel, { from: apmOwner }) - assert.equal(await ens.owner(holanode), zeroAddr, 'should have reset name') + assert.equal(await ens.owner(holanode), ZERO_ADDR, 'should have reset name') }) it('fails if initializing without rootnode ownership', async () => { diff --git a/test/evm_script.js b/test/evm_script.js index 9615e02c2..48ad317f1 100644 --- a/test/evm_script.js +++ b/test/evm_script.js @@ -18,6 +18,7 @@ const ExecutionTarget = artifacts.require('ExecutionTarget') const EVMScriptExecutorMock = artifacts.require('EVMScriptExecutorMock') const MockScriptExecutorApp = artifacts.require('MockScriptExecutorApp') +const EMPTY_BYTES = '0x' const ZERO_ADDR = '0x0000000000000000000000000000000000000000' contract('EVM Script', accounts => { @@ -28,6 +29,8 @@ contract('EVM Script', accounts => { const executorAppId = '0x1234' + const createExecutorId = id => `0x${String(id).padStart(8, '0')}` + before(async () => { const regFact = await EVMScriptRegistryFactory.new() callsScriptBase = CallsScript.at(await regFact.baseCallScript()) @@ -51,9 +54,9 @@ contract('EVM Script', accounts => { }) it('factory registered just 1 script executor', async () => { - assert.equal(await reg.getScriptExecutor('0x00000000'), ZERO_ADDR) - assert.notEqual(await reg.getScriptExecutor('0x00000001'), ZERO_ADDR) - assert.equal(await reg.getScriptExecutor('0x00000002'), ZERO_ADDR) + assert.equal(await reg.getScriptExecutor(createExecutorId(0)), ZERO_ADDR) + assert.notEqual(await reg.getScriptExecutor(createExecutorId(1)), ZERO_ADDR) + assert.equal(await reg.getScriptExecutor(createExecutorId(2)), ZERO_ADDR) }) it('fails if directly calling base executor', async () => { @@ -61,7 +64,7 @@ contract('EVM Script', accounts => { const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } const script = encodeCallScript([action]) - await assertRevert(() => callsScriptBase.execScript(script, '0x', [])) + await assertRevert(() => callsScriptBase.execScript(script, EMPTY_BYTES, [])) }) context('> Registry', () => { @@ -109,7 +112,7 @@ contract('EVM Script', accounts => { assertEvent(receipt, 'DisableExecutor') assert.isFalse(executorEntry[1], "executor should now be disabled") - assert.equal(await reg.getScriptExecutor(`0x0000000${newExecutorId}`), ZERO_ADDR, 'getting disabled executor should return zero addr') + assert.equal(await reg.getScriptExecutor(createExecutorId(newExecutorId)), ZERO_ADDR, 'getting disabled executor should return zero addr') }) it('can re-enable an executor', async () => { @@ -118,14 +121,14 @@ contract('EVM Script', accounts => { await reg.disableScriptExecutor(newExecutorId, { from: boss }) let executorEntry = await reg.executors(newExecutorId) assert.isFalse(executorEntry[1], "executor should now be disabled") - assert.equal(await reg.getScriptExecutor(`0x0000000${newExecutorId}`), ZERO_ADDR, 'getting disabled executor should return zero addr') + assert.equal(await reg.getScriptExecutor(createExecutorId(newExecutorId)), ZERO_ADDR, 'getting disabled executor should return zero addr') const receipt = await reg.enableScriptExecutor(newExecutorId, { from: boss }) executorEntry = await reg.executors(newExecutorId) assertEvent(receipt, 'EnableExecutor') assert.isTrue(executorEntry[1], "executor should now be re-enabled") - assert.notEqual(await reg.getScriptExecutor(`0x0000000${newExecutorId}`), ZERO_ADDR, 'getting disabled executor should return non-zero addr') + assert.notEqual(await reg.getScriptExecutor(createExecutorId(newExecutorId)), ZERO_ADDR, 'getting disabled executor should return non-zero addr') }) it('fails to add a new executor without the correct permissions', async () => { @@ -176,7 +179,7 @@ contract('EVM Script', accounts => { context('> Uninitialized executor', () => { beforeEach(async () => { - const receipt = await dao.newAppInstance(executorAppId, executorAppBase.address, '0x', false, { from: boss }) + const receipt = await dao.newAppInstance(executorAppId, executorAppBase.address, EMPTY_BYTES, false, { from: boss }) executorApp = MockScriptExecutorApp.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) // Explicitly don't initialize the executorApp executionTarget = await ExecutionTarget.new() @@ -192,7 +195,7 @@ contract('EVM Script', accounts => { context('> Executor', () => { beforeEach(async () => { - const receipt = await dao.newAppInstance(executorAppId, executorAppBase.address, '0x', false, { from: boss }) + const receipt = await dao.newAppInstance(executorAppId, executorAppBase.address, EMPTY_BYTES, false, { from: boss }) executorApp = MockScriptExecutorApp.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) await executorApp.initialize() executionTarget = await ExecutionTarget.new() @@ -205,29 +208,29 @@ contract('EVM Script', accounts => { it('fails to execute if spec ID is 0', async () => { return assertRevert(async () => { - await executorApp.execute('0x00000000') + await executorApp.execute(createExecutorId(0)) }) }) it('fails to execute if spec ID is unknown', async () => { return assertRevert(async () => { - await executorApp.execute('0x00000004') + await executorApp.execute(createExecutorId(4)) }) }) context('> Spec ID 1', () => { it('is the correct executor type', async () => { const CALLS_SCRIPT_TYPE = soliditySha3('CALLS_SCRIPT') - const executor = IEVMScriptExecutor.at(await reg.getScriptExecutor('0x00000001')) + const executor = IEVMScriptExecutor.at(await reg.getScriptExecutor(createExecutorId(1))) assert.equal(await executor.executorType(), CALLS_SCRIPT_TYPE) }) it('gets the correct executor from the app', async () => { const CALLS_SCRIPT_TYPE = soliditySha3('CALLS_SCRIPT') - const script = '0x00000001' - const executor = await reg.getScriptExecutor(script) + const scriptId = createExecutorId(1) + const executor = await reg.getScriptExecutor(scriptId) - const appExecutor = await executorApp.getEVMScriptExecutor(script) + const appExecutor = await executorApp.getEVMScriptExecutor(scriptId) assert.equal(executor, appExecutor, 'app should return the same evm script executor') }) @@ -242,10 +245,10 @@ contract('EVM Script', accounts => { // Check logs // The Executor always uses 0x for the input and callscripts always have 0x returns const scriptResult = receipt.logs.filter(l => l.event == 'ScriptResult')[0] - assert.equal(scriptResult.args.executor, await reg.getScriptExecutor('0x00000001'), 'should log the same executor') + assert.equal(scriptResult.args.executor, await reg.getScriptExecutor(createExecutorId(1)), 'should log the same executor') assert.equal(scriptResult.args.script, script, 'should log the same script') - assert.equal(scriptResult.args.input, '0x', 'should log the same input') - assert.equal(scriptResult.args.returnData, '0x', 'should log the return data') + assert.equal(scriptResult.args.input, EMPTY_BYTES, 'should log the same input') + assert.equal(scriptResult.args.returnData, EMPTY_BYTES, 'should log the return data') }) it('fails to execute if has blacklist addresses being called', async () => { @@ -323,7 +326,7 @@ contract('EVM Script', accounts => { // Remove 12 first 0s of padding for addr and 28 0s for uint32 return script + addr.slice(24) + length.slice(56) + calldata.slice(2) - }, '0x00000001') // spec 1 + }, createExecutorId(1)) // spec 1 } it('fails if data length is too big', async () => { diff --git a/test/kernel_acl.js b/test/kernel_acl.js index 0ec8e16a8..9b5274f61 100644 --- a/test/kernel_acl.js +++ b/test/kernel_acl.js @@ -13,6 +13,9 @@ const KernelProxy = artifacts.require('KernelProxy') const AppStub = artifacts.require('AppStub') const APP_ID = hash('stub.aragonpm.test') +const EMPTY_BYTES = '0x' +const ZERO_ADDR = '0x0000000000000000000000000000000000000000' + contract('Kernel ACL', accounts => { let aclBase, appBase let APP_MANAGER_ROLE, APP_BASES_NAMESPACE, DEFAULT_ACL_APP_ID, ANY_ENTITY @@ -70,7 +73,7 @@ contract('Kernel ACL', accounts => { it('cannot initialize proxied ACL outside of Kernel', async () => { // Set up ACL proxy await acl.createPermission(permissionsRoot, kernelAddr, APP_MANAGER_ROLE, permissionsRoot) - const receipt = await kernel.newAppInstance(DEFAULT_ACL_APP_ID, aclBase.address, '0x', false) + const receipt = await kernel.newAppInstance(DEFAULT_ACL_APP_ID, aclBase.address, EMPTY_BYTES, false) const newAcl = ACL.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) return assertRevert(async () => { @@ -153,14 +156,21 @@ contract('Kernel ACL', accounts => { // Allows setting code for namespace other than 0 for (grantee of [child, secondChild]) { - const receipt = await kernel.setApp('0x121212', '0x0', appBase.address, { from: grantee }) + const receipt = await kernel.setApp('0x121212', APP_ID, appBase.address, { from: grantee }) assertEvent(receipt, 'SetApp') } // Fail if setting code for namespace 0 for (grantee of [child, secondChild]) { await assertRevert(async () => { - await kernel.setApp('0x0', APP_ID, appBase.address, { from: grantee }) + await kernel.setApp('0x00', APP_ID, appBase.address, { from: grantee }) + }) + } + + // Fail if setting code for empty namespace (which becomes 0) + for (grantee of [child, secondChild]) { + await assertRevert(async () => { + await kernel.setApp(EMPTY_BYTES, APP_ID, appBase.address, { from: grantee }) }) } }) @@ -249,7 +259,7 @@ contract('Kernel ACL', accounts => { it('removes manager', async () => { const noManager = await acl.getPermissionManager(kernelAddr, APP_MANAGER_ROLE) - assert.equal('0x0000000000000000000000000000000000000000', noManager, 'manager should have been removed') + assert.equal(ZERO_ADDR, noManager, 'manager should have been removed') }) it('old manager lost power', async () => { diff --git a/test/kernel_apps.js b/test/kernel_apps.js index d5ac01a54..995924a45 100644 --- a/test/kernel_apps.js +++ b/test/kernel_apps.js @@ -16,8 +16,8 @@ const ERCProxyMock = artifacts.require('ERCProxyMock') const KernelOverloadMock = artifacts.require('KernelOverloadMock') const APP_ID = hash('stub.aragonpm.test') -const ZERO_ADDR = '0x0000000000000000000000000000000000000000' const EMPTY_BYTES = '0x' +const ZERO_ADDR = '0x0000000000000000000000000000000000000000' contract('Kernel apps', accounts => { let aclBase, appBase1, appBase2 @@ -101,7 +101,7 @@ contract('Kernel apps', accounts => { context(`> new ${appProxyType} instances`, () => { onlyAppProxy(() => it('creates a new upgradeable app proxy instance', async () => { - const receipt = await kernel.newAppInstance(APP_ID, appBase1.address, '0x', false) + const receipt = await kernel.newAppInstance(APP_ID, appBase1.address, EMPTY_BYTES, false) const appProxy = AppProxyUpgradeable.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) assert.equal(await appProxy.kernel(), kernel.address, "new appProxy instance's kernel should be set to the originating kernel") @@ -137,18 +137,18 @@ contract('Kernel apps', accounts => { it('sets the app base when not previously registered', async() => { assert.equal(ZERO_ADDR, await kernel.getApp(APP_BASES_NAMESPACE, APP_ID)) - await kernelOverload[newInstanceFn](APP_ID, appBase1.address, '0x', false) + await kernelOverload[newInstanceFn](APP_ID, appBase1.address, EMPTY_BYTES, false) assert.equal(appBase1.address, await kernel.getApp(APP_BASES_NAMESPACE, APP_ID)) }) it("doesn't set the app base when already set", async() => { await kernel.setApp(APP_BASES_NAMESPACE, APP_ID, appBase1.address) - const receipt = await kernelOverload[newInstanceFn](APP_ID, appBase1.address, '0x', false) + const receipt = await kernelOverload[newInstanceFn](APP_ID, appBase1.address, EMPTY_BYTES, false) assert.isFalse(receipt.logs.includes(l => l.event == 'SetApp')) }) it("also sets the default app", async () => { - const receipt = await kernelOverload[newInstanceFn](APP_ID, appBase1.address, '0x', true) + const receipt = await kernelOverload[newInstanceFn](APP_ID, appBase1.address, EMPTY_BYTES, true) const appProxyAddr = receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy // Check that both the app base and default app are set @@ -175,7 +175,7 @@ contract('Kernel apps', accounts => { it("fails if the app base is not given", async() => { return assertRevert(async () => { - await kernelOverload[newInstanceFn](APP_ID, ZERO_ADDR, '0x', false) + await kernelOverload[newInstanceFn](APP_ID, ZERO_ADDR, EMPTY_BYTES, false) }) }) @@ -186,7 +186,7 @@ contract('Kernel apps', accounts => { await kernel.setApp(APP_BASES_NAMESPACE, APP_ID, existingBase) return assertRevert(async () => { - await kernelOverload[newInstanceFn](APP_ID, differentBase, '0x', false) + await kernelOverload[newInstanceFn](APP_ID, differentBase, EMPTY_BYTES, false) }) }) }) diff --git a/test/kernel_lifecycle.js b/test/kernel_lifecycle.js index 8d1c55d26..ae4bb3667 100644 --- a/test/kernel_lifecycle.js +++ b/test/kernel_lifecycle.js @@ -11,6 +11,7 @@ const KernelProxy = artifacts.require('KernelProxy') const AppStub = artifacts.require('AppStub') const APP_ID = hash('stub.aragonpm.test') const VAULT_ID = hash('vault.aragonpm.test') +const EMPTY_BYTES = '0x' contract('Kernel lifecycle', accounts => { let aclBase, appBase @@ -18,10 +19,10 @@ contract('Kernel lifecycle', accounts => { const testUnaccessibleFunctionalityWhenUninitialized = async (kernel) => { // hasPermission should always return false when uninitialized - assert.isFalse(await kernel.hasPermission(accounts[0], kernel.address, APP_MANAGER_ROLE, '0x')) - assert.isFalse(await kernel.hasPermission(accounts[1], kernel.address, APP_MANAGER_ROLE, '0x')) + assert.isFalse(await kernel.hasPermission(accounts[0], kernel.address, APP_MANAGER_ROLE, EMPTY_BYTES)) + assert.isFalse(await kernel.hasPermission(accounts[1], kernel.address, APP_MANAGER_ROLE, EMPTY_BYTES)) - await assertRevert(() => kernel.newAppInstance(APP_ID, appBase.address, '0x', false)) + await assertRevert(() => kernel.newAppInstance(APP_ID, appBase.address, EMPTY_BYTES, false)) await assertRevert(() => kernel.newPinnedAppInstance(APP_ID, appBase.address)) await assertRevert(() => kernel.setApp(APP_BASES_NAMESPACE, APP_ID, appBase.address)) await assertRevert(() => kernel.setRecoveryVaultAppId(VAULT_ID)) @@ -29,17 +30,17 @@ contract('Kernel lifecycle', accounts => { const testUsability = async (kernel) => { // Make sure we haven't already setup the required permission - assert.isFalse(await kernel.hasPermission(accounts[0], kernel.address, APP_MANAGER_ROLE, '0x')) - assert.isFalse(await kernel.hasPermission(accounts[1], kernel.address, APP_MANAGER_ROLE, '0x')) + assert.isFalse(await kernel.hasPermission(accounts[0], kernel.address, APP_MANAGER_ROLE, EMPTY_BYTES)) + assert.isFalse(await kernel.hasPermission(accounts[1], kernel.address, APP_MANAGER_ROLE, EMPTY_BYTES)) // Then set the required permission const acl = ACL.at(await kernel.acl()) await acl.createPermission(accounts[0], kernel.address, APP_MANAGER_ROLE, accounts[0]) - assert.isTrue(await kernel.hasPermission(accounts[0], kernel.address, APP_MANAGER_ROLE, '0x')) - assert.isFalse(await kernel.hasPermission(accounts[1], kernel.address, APP_MANAGER_ROLE, '0x')) + assert.isTrue(await kernel.hasPermission(accounts[0], kernel.address, APP_MANAGER_ROLE, EMPTY_BYTES)) + assert.isFalse(await kernel.hasPermission(accounts[1], kernel.address, APP_MANAGER_ROLE, EMPTY_BYTES)) // And finally test functionality - await kernel.newAppInstance(APP_ID, appBase.address, '0x', false) + await kernel.newAppInstance(APP_ID, appBase.address, EMPTY_BYTES, false) } // Initial setup diff --git a/test/recovery_to_vault.js b/test/recovery_to_vault.js index a92714540..a0c936172 100644 --- a/test/recovery_to_vault.js +++ b/test/recovery_to_vault.js @@ -23,6 +23,7 @@ const KernelDepositableMock = artifacts.require('KernelDepositableMock') const getEvent = (receipt, event, arg) => { return receipt.logs.filter(l => l.event == event)[0].args[arg] } const APP_ID = hash('stub.aragonpm.test') +const EMPTY_BYTES = '0x' const SEND_ETH_GAS = 31000 // 21k base tx cost + 10k limit on depositable proxies contract('Recovery to vault', accounts => { @@ -190,7 +191,7 @@ contract('Recovery to vault', accounts => { vault = vaultBase } else if (vaultType === 'VaultProxy') { // This doesn't automatically setup the recovery address - const receipt = await kernel.newAppInstance(vaultId, vaultBase.address, '0x', false) + const receipt = await kernel.newAppInstance(vaultId, vaultBase.address, EMPTY_BYTES, false) const vaultProxyAddress = getEvent(receipt, 'NewAppProxy', 'proxy') vault = VaultMock.at(vaultProxyAddress) } @@ -249,7 +250,7 @@ contract('Recovery to vault', accounts => { context('> Proxied app with kernel', () => { beforeEach(async () => { // Setup app - const receipt = await kernel.newAppInstance(APP_ID, appBase.address, '0x', false) + const receipt = await kernel.newAppInstance(APP_ID, appBase.address, EMPTY_BYTES, false) const appProxy = getEvent(receipt, 'NewAppProxy', 'proxy') const app = AppStubDepositable.at(appProxy) await app.enableDeposits() @@ -265,7 +266,7 @@ contract('Recovery to vault', accounts => { it('cannot send ETH with data to proxy', async () => { await assertRevert(async () => { - await target.sendTransaction({ value: 1, data: '0x1', gas: SEND_ETH_GAS }) + await target.sendTransaction({ value: 1, data: '0x01', gas: SEND_ETH_GAS }) }) }) @@ -301,7 +302,7 @@ contract('Recovery to vault', accounts => { context('> Conditional fund recovery', () => { beforeEach(async () => { // Setup app with conditional recovery code - const receipt = await kernel.newAppInstance(APP_ID, appConditionalRecoveryBase.address, '0x', false) + const receipt = await kernel.newAppInstance(APP_ID, appConditionalRecoveryBase.address, EMPTY_BYTES, false) const appProxy = getEvent(receipt, 'NewAppProxy', 'proxy') const app = AppStubConditionalRecovery.at(appProxy) await app.initialize() @@ -358,7 +359,7 @@ contract('Recovery to vault', accounts => { // Create a new vault and set that vault as the default vault in the kernel const vaultId = hash('vault.aragonpm.test') const vaultBase = await VaultMock.new() - const vaultReceipt = await kernel.newAppInstance(vaultId, vaultBase.address, '0x', true) + const vaultReceipt = await kernel.newAppInstance(vaultId, vaultBase.address, EMPTY_BYTES, true) const vaultAddress = getEvent(vaultReceipt, 'NewAppProxy', 'proxy') vault = VaultMock.at(vaultAddress) await vault.initialize() From d92e53f7e7632608afdb72f580db5bf06a7375f5 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Mon, 25 Mar 2019 10:33:41 +0100 Subject: [PATCH 03/15] ACLSyntaxSugar: fix returning unallocated memory (#495) --- contracts/acl/ACLSyntaxSugar.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 5e5fe9028..f5683c0bf 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -7,7 +7,7 @@ pragma solidity ^0.4.24; contract ACLSyntaxSugar { function arr() internal pure returns (uint256[]) { - // solium-disable-previous-line no-empty-blocks + return new uint256[](0); } function arr(bytes32 _a) internal pure returns (uint256[] r) { From 9cce83cc02d04ebb8a9dbbb73193f7c35b355432 Mon Sep 17 00:00:00 2001 From: Greg Zaitsev Date: Mon, 25 Mar 2019 13:20:11 +0300 Subject: [PATCH 04/15] docs: Add radspec strings to factories (#498) --- contracts/factory/APMRegistryFactory.sol | 18 ++++++++++++- contracts/factory/AppProxyFactory.sol | 25 +++++++++++++++++++ contracts/factory/DAOFactory.sol | 8 ++++++ contracts/factory/ENSFactory.sol | 11 +++++--- .../factory/EVMScriptRegistryFactory.sol | 8 ++++++ 5 files changed, 66 insertions(+), 4 deletions(-) diff --git a/contracts/factory/APMRegistryFactory.sol b/contracts/factory/APMRegistryFactory.sol index 1ddd2081f..7f0aa3fd3 100644 --- a/contracts/factory/APMRegistryFactory.sol +++ b/contracts/factory/APMRegistryFactory.sol @@ -19,7 +19,16 @@ contract APMRegistryFactory is APMInternalAppNames { event DeployAPM(bytes32 indexed node, address apm); - // Needs either one ENS or ENSFactory + /** + * @notice Create a new factory for deploying Aragon Package Managers (aragonPM) + * @dev Requires either a given ENS registrar or ENSFactory (used for generating a new ENS in test environments). + * @param _daoFactory Base factory for deploying DAOs + * @param _registryBase APMRegistry base contract location + * @param _repoBase Repo base contract location + * @param _ensSubBase ENSSubdomainRegistrar base contract location + * @param _ens ENS instance + * @param _ensFactory ENSFactory (used to generated a new ENS if no ENS is given) + */ constructor( DAOFactory _daoFactory, APMRegistry _registryBase, @@ -40,6 +49,13 @@ contract APMRegistryFactory is APMInternalAppNames { ens = _ens != address(0) ? _ens : _ensFactory.newENS(this); } + /** + * @notice Create a new Aragon Package Manager (aragonPM) DAO, holding the `_label` subdomain from parent `_tld` and controlled by `_root` + * @param _tld The parent node of the controlled subdomain + * @param _label The subdomain label + * @param _root Manager for the new aragonPM DAO + * @return The new aragonPM's APMRegistry app + */ function newAPM(bytes32 _tld, bytes32 _label, address _root) public returns (APMRegistry) { bytes32 node = keccak256(abi.encodePacked(_tld, _label)); diff --git a/contracts/factory/AppProxyFactory.sol b/contracts/factory/AppProxyFactory.sol index f68c9d11f..d851c839c 100644 --- a/contracts/factory/AppProxyFactory.sol +++ b/contracts/factory/AppProxyFactory.sol @@ -7,20 +7,45 @@ import "../apps/AppProxyPinned.sol"; contract AppProxyFactory { event NewAppProxy(address proxy, bool isUpgradeable, bytes32 appId); + /** + * @notice Create a new upgradeable app instance on `_kernel` with identifier `_appId` + * @param _kernel App's Kernel reference + * @param _appId Identifier for app + * @return AppProxyUpgradeable + */ function newAppProxy(IKernel _kernel, bytes32 _appId) public returns (AppProxyUpgradeable) { return newAppProxy(_kernel, _appId, new bytes(0)); } + /** + * @notice Create a new upgradeable app instance on `_kernel` with identifier `_appId` and initialization payload `_initializePayload` + * @param _kernel App's Kernel reference + * @param _appId Identifier for app + * @return AppProxyUpgradeable + */ function newAppProxy(IKernel _kernel, bytes32 _appId, bytes _initializePayload) public returns (AppProxyUpgradeable) { AppProxyUpgradeable proxy = new AppProxyUpgradeable(_kernel, _appId, _initializePayload); emit NewAppProxy(address(proxy), true, _appId); return proxy; } + /** + * @notice Create a new pinned app instance on `_kernel` with identifier `_appId` + * @param _kernel App's Kernel reference + * @param _appId Identifier for app + * @return AppProxyPinned + */ function newAppProxyPinned(IKernel _kernel, bytes32 _appId) public returns (AppProxyPinned) { return newAppProxyPinned(_kernel, _appId, new bytes(0)); } + /** + * @notice Create a new pinned app instance on `_kernel` with identifier `_appId` and initialization payload `_initializePayload` + * @param _kernel App's Kernel reference + * @param _appId Identifier for app + * @param _initializePayload Proxy initialization payload + * @return AppProxyPinned + */ function newAppProxyPinned(IKernel _kernel, bytes32 _appId, bytes _initializePayload) public returns (AppProxyPinned) { AppProxyPinned proxy = new AppProxyPinned(_kernel, _appId, _initializePayload); emit NewAppProxy(address(proxy), false, _appId); diff --git a/contracts/factory/DAOFactory.sol b/contracts/factory/DAOFactory.sol index a5f357d5a..a2eec709a 100644 --- a/contracts/factory/DAOFactory.sol +++ b/contracts/factory/DAOFactory.sol @@ -18,6 +18,12 @@ contract DAOFactory { event DeployDAO(address dao); event DeployEVMScriptRegistry(address reg); + /** + * @notice Create a new DAOFactory, creating DAOs with Kernels proxied to `_baseKernel`, ACLs proxied to `_baseACL`, and new EVMScriptRegistries created from `_regFactory`. + * @param _baseKernel Base Kernel + * @param _baseACL Base ACL + * @param _regFactory EVMScriptRegistry factory + */ constructor(IKernel _baseKernel, IACL _baseACL, EVMScriptRegistryFactory _regFactory) public { // No need to init as it cannot be killed by devops199 if (address(_regFactory) != address(0)) { @@ -29,7 +35,9 @@ contract DAOFactory { } /** + * @notice Create a new DAO with `_root` set as the initial admin * @param _root Address that will be granted control to setup DAO permissions + * @return Newly created DAO */ function newDAO(address _root) public returns (Kernel) { Kernel dao = Kernel(new KernelProxy(baseKernel)); diff --git a/contracts/factory/ENSFactory.sol b/contracts/factory/ENSFactory.sol index 9768f20ea..51c41c671 100644 --- a/contracts/factory/ENSFactory.sol +++ b/contracts/factory/ENSFactory.sol @@ -5,12 +5,17 @@ import "../lib/ens/PublicResolver.sol"; import "../ens/ENSConstants.sol"; -// Note that this contract is NOT meant to be used in production. -// Its only purpose is to easily create ENS instances for testing APM. +// WARNING: This is an incredibly trustful ENS deployment, do NOT use in production! +// This contract is NOT meant to be deployed to a live network. +// Its only purpose is to easily create ENS instances for testing aragonPM. contract ENSFactory is ENSConstants { event DeployENS(address ens); - // This is an incredibly trustfull ENS deployment, only use for testing + /** + * @notice Create a new ENS and set `_owner` as the owner of the top level domain. + * @param _owner Owner of .eth + * @return ENS + */ function newENS(address _owner) public returns (ENS) { ENS ens = new ENS(); diff --git a/contracts/factory/EVMScriptRegistryFactory.sol b/contracts/factory/EVMScriptRegistryFactory.sol index fdd901628..3ff9f3f62 100644 --- a/contracts/factory/EVMScriptRegistryFactory.sol +++ b/contracts/factory/EVMScriptRegistryFactory.sol @@ -13,11 +13,19 @@ contract EVMScriptRegistryFactory is EVMScriptRegistryConstants { EVMScriptRegistry public baseReg; IEVMScriptExecutor public baseCallScript; + /** + * @notice Create a new EVMScriptRegistryFactory. + */ constructor() public { baseReg = new EVMScriptRegistry(); baseCallScript = IEVMScriptExecutor(new CallsScript()); } + /** + * @notice Install a new pinned instance of EVMScriptRegistry on `_dao`. + * @param _dao Kernel + * @return Installed EVMScriptRegistry + */ function newEVMScriptRegistry(Kernel _dao) public returns (EVMScriptRegistry reg) { bytes memory initPayload = abi.encodeWithSelector(reg.initialize.selector); reg = EVMScriptRegistry(_dao.newPinnedAppInstance(EVMSCRIPT_REGISTRY_APP_ID, baseReg, initPayload, true)); From 96c1b59a49f784ba5c795d3b279a0c5cccf770bf Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Fri, 5 Apr 2019 11:26:30 +0200 Subject: [PATCH 05/15] feat: add ReentrancyGuard (#503) --- contracts/apps/AragonApp.sol | 7 +- contracts/common/ReentrancyGuard.sol | 33 ++++++ contracts/test/mocks/KeccakConstants.sol | 1 + contracts/test/mocks/ReentrancyGuardMock.sol | 53 ++++++++++ test/keccak_constants.js | 10 ++ test/reentrancy_guard.js | 100 +++++++++++++++++++ test/unstructured_storage.js | 30 +++++- 7 files changed, 226 insertions(+), 8 deletions(-) create mode 100644 contracts/common/ReentrancyGuard.sol create mode 100644 contracts/test/mocks/ReentrancyGuardMock.sol create mode 100644 test/reentrancy_guard.js diff --git a/contracts/apps/AragonApp.sol b/contracts/apps/AragonApp.sol index 71e7172cc..bc91fa48c 100644 --- a/contracts/apps/AragonApp.sol +++ b/contracts/apps/AragonApp.sol @@ -6,6 +6,7 @@ pragma solidity ^0.4.24; import "./AppStorage.sol"; import "../common/Autopetrified.sol"; +import "../common/ReentrancyGuard.sol"; import "../common/VaultRecoverable.sol"; import "../evmscript/EVMScriptRunner.sol"; import "../acl/ACLSyntaxSugar.sol"; @@ -14,9 +15,9 @@ import "../acl/ACLSyntaxSugar.sol"; // Contracts inheriting from AragonApp are, by default, immediately petrified upon deployment so // that they can never be initialized. // Unless overriden, this behaviour enforces those contracts to be usable only behind an AppProxy. -// ACLSyntaxSugar and EVMScriptRunner are not directly used by this contract, but are included so -// that they are automatically usable by subclassing contracts -contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, EVMScriptRunner, ACLSyntaxSugar { +// ReentrancyGuard, EVMScriptRunner, and ACLSyntaxSugar are not directly used by this contract, but +// are included so that they are automatically usable by subclassing contracts +contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, ReentrancyGuard, EVMScriptRunner, ACLSyntaxSugar { string private constant ERROR_AUTH_FAILED = "APP_AUTH_FAILED"; modifier auth(bytes32 _role) { diff --git a/contracts/common/ReentrancyGuard.sol b/contracts/common/ReentrancyGuard.sol new file mode 100644 index 000000000..0d761f2ad --- /dev/null +++ b/contracts/common/ReentrancyGuard.sol @@ -0,0 +1,33 @@ +/* + * SPDX-License-Identitifer: MIT + */ + +pragma solidity ^0.4.24; + +import "../common/UnstructuredStorage.sol"; + + +contract ReentrancyGuard { + using UnstructuredStorage for bytes32; + + /* Hardcoded constants to save gas + bytes32 internal constant REENTRANCY_MUTEX_POSITION = keccak256("aragonOS.reentrancyGuard.mutex"); + */ + bytes32 private constant REENTRANCY_MUTEX_POSITION = 0xe855346402235fdd185c890e68d2c4ecad599b88587635ee285bce2fda58dacb; + + string private constant ERROR_REENTRANT = "REENTRANCY_REENTRANT_CALL"; + + modifier nonReentrant() { + // Ensure mutex is unlocked + require(!REENTRANCY_MUTEX_POSITION.getStorageBool(), ERROR_REENTRANT); + + // Lock mutex before function call + REENTRANCY_MUTEX_POSITION.setStorageBool(true); + + // Perform function call + _; + + // Unlock mutex after function call + REENTRANCY_MUTEX_POSITION.setStorageBool(false); + } +} diff --git a/contracts/test/mocks/KeccakConstants.sol b/contracts/test/mocks/KeccakConstants.sol index 5bfd6736f..6cc9a5bb5 100644 --- a/contracts/test/mocks/KeccakConstants.sol +++ b/contracts/test/mocks/KeccakConstants.sol @@ -49,6 +49,7 @@ contract KeccakConstants { // Unstructured storage bytes32 public constant initializationBlockPosition = keccak256(abi.encodePacked("aragonOS.initializable.initializationBlock")); bytes32 public constant depositablePosition = keccak256(abi.encodePacked("aragonOS.depositableStorage.depositable")); + bytes32 public constant reentrancyGuardPosition = keccak256(abi.encodePacked("aragonOS.reentrancyGuard.mutex")); bytes32 public constant kernelPosition = keccak256(abi.encodePacked("aragonOS.appStorage.kernel")); bytes32 public constant appIdPosition = keccak256(abi.encodePacked("aragonOS.appStorage.appId")); bytes32 public constant pinnedCodePosition = keccak256(abi.encodePacked("aragonOS.appStorage.pinnedCode")); diff --git a/contracts/test/mocks/ReentrancyGuardMock.sol b/contracts/test/mocks/ReentrancyGuardMock.sol new file mode 100644 index 000000000..45a805ac0 --- /dev/null +++ b/contracts/test/mocks/ReentrancyGuardMock.sol @@ -0,0 +1,53 @@ +pragma solidity 0.4.24; + +import "../../common/ReentrancyGuard.sol"; +import "../../common/UnstructuredStorage.sol"; + + +contract ReentrantActor { + bool reenterNonReentrant; + + constructor(bool _reenterNonReentrant) public { + reenterNonReentrant = _reenterNonReentrant; + } + + function reenter(ReentrancyGuardMock _mock) public { + // Set the reentrancy target to 0 so we don't infinite loop + ReentrantActor reentrancyTarget = ReentrantActor(0); + + if (reenterNonReentrant) { + _mock.nonReentrantCall(reentrancyTarget); + } else { + _mock.reentrantCall(reentrancyTarget); + } + } +} + + +contract ReentrancyGuardMock is ReentrancyGuard { + using UnstructuredStorage for bytes32; + + uint256 public callCounter; + + function nonReentrantCall(ReentrantActor _target) public nonReentrant { + callCounter++; + if (_target != address(0)) { + _target.reenter(this); + } + } + + function reentrantCall(ReentrantActor _target) public { + callCounter++; + if (_target != address(0)) { + _target.reenter(this); + } + } + + function setReentrancyMutex(bool _mutex) public { + getReentrancyMutexPosition().setStorageBool(_mutex); + } + + function getReentrancyMutexPosition() public pure returns (bytes32) { + return keccak256("aragonOS.reentrancyGuard.mutex"); + } +} diff --git a/test/keccak_constants.js b/test/keccak_constants.js index cdd20d78f..c6567bf6d 100644 --- a/test/keccak_constants.js +++ b/test/keccak_constants.js @@ -115,4 +115,14 @@ contract('Constants', accounts => { const initializableMock = await getContract('InitializableStorageMock').new() assert.equal(await initializableMock.getInitializationBlockPosition(), await keccakConstants.initializationBlockPosition(), "initializationBlockPosition doesn't match") }) + + it('checks ReentrancyGuard unstructured storage constants', async () => { + const reentrancyGuardMock = await getContract('ReentrancyGuardMock').new() + // Note that this is a bit of a roundabout test for this unstructured storage slot. Since the + // position is declared as private in the base ReentrancyGuard contract, we redefine in the + // mock. + // This test therefore also relies on the ReentrancyGuard's own tests to make sure we've + // redefined the storage position correctly in the mock. + assert.equal(await reentrancyGuardMock.getReentrancyMutexPosition(), await keccakConstants.reentrancyGuardPosition(), "reentrancyGuardPosition doesn't match") + }) }) diff --git a/test/reentrancy_guard.js b/test/reentrancy_guard.js new file mode 100644 index 000000000..1d0aecbe2 --- /dev/null +++ b/test/reentrancy_guard.js @@ -0,0 +1,100 @@ +const { assertRevert } = require('./helpers/assertThrow') + +const ZERO_ADDR = '0x0000000000000000000000000000000000000000' + +contract('ReentrancyGuard', accounts => { + let reentrancyMock + + async function assertReentrancyMutex(mutex, msg) { + assert.equal( + await web3.eth.getStorageAt(reentrancyMock.address, (await reentrancyMock.getReentrancyMutexPosition())), + mutex, + msg + ) + } + + beforeEach(async () => { + reentrancyMock = await artifacts.require('ReentrancyGuardMock').new() + }) + + it('starts with false mutex', async () => { + await assertReentrancyMutex(false, 're-entrancy mutex should start false') + }) + + it('starts with no calls', async () => { + assert.equal((await reentrancyMock.callCounter()).toString(), 0, 'should start with no calls') + }) + + it('can call re-entrant function normally', async () => { + await reentrancyMock.reentrantCall(ZERO_ADDR) + assert.equal((await reentrancyMock.callCounter()).toString(), 1, 'should have registered one call') + await assertReentrancyMutex(false, 're-entrancy mutex should end false') + }) + + it('can call non-re-entrant function normally', async () => { + await reentrancyMock.nonReentrantCall(ZERO_ADDR) + assert.equal((await reentrancyMock.callCounter()).toString(), 1, 'should have registered one call') + await assertReentrancyMutex(false, 're-entrancy mutex should end false') + }) + + context('> Enabled re-entrancy mutex', () => { + beforeEach(async () => { + // Manually set re-entrancy mutex + await reentrancyMock.setReentrancyMutex(true) + }) + + it('can call re-entrant function if re-entrancy mutex is enabled', async () => { + await reentrancyMock.reentrantCall(ZERO_ADDR) + assert.equal((await reentrancyMock.callCounter()).toString(), 1, 'should have called') + }) + + it('can not call non-re-entrant function if re-entrancy mutex is enabled', async () => { + await assertRevert(async () => { + await reentrancyMock.nonReentrantCall(ZERO_ADDR) + }) + assert.equal((await reentrancyMock.callCounter()).toString(), 0, 'should not have called') + }) + }) + + context('> Re-entering through contract', async () => { + let reentrantActor + + afterEach(async () => { + await assertReentrancyMutex(false, 're-entrancy mutex should end false') + }) + + context('> Re-enters re-entrant call', async () => { + before(async () => { + reentrantActor = await artifacts.require('ReentrantActor').new(false) + }) + + it('allows re-entering re-entrant call', async () => { + await reentrancyMock.reentrantCall(reentrantActor.address) + assert.equal((await reentrancyMock.callCounter()).toString(), 2, 'should have called twice') + }) + + it('allows entering non-re-entrant call from re-entrant call', async () => { + await reentrancyMock.nonReentrantCall(reentrantActor.address) + assert.equal((await reentrancyMock.callCounter()).toString(), 2, 'should have called twice') + }) + }) + + context('> Re-enters non-reentrant call', async () => { + before(async () => { + reentrantActor = await artifacts.require('ReentrantActor').new(true) + }) + + it('disallows re-entering non-re-entrant call', async () => { + await assertRevert(async () => { + await reentrancyMock.nonReentrantCall(reentrantActor.address) + }) + assert.equal((await reentrancyMock.callCounter()).toString(), 0, 'should not have completed any calls') + }) + + it('allows entering non-entrant call from re-entrant call', async () => { + await reentrancyMock.reentrantCall(reentrantActor.address) + assert.equal((await reentrancyMock.callCounter()).toString(), 2, 'should have called twice') + }) + }) + }) +}) diff --git a/test/unstructured_storage.js b/test/unstructured_storage.js index 01a4362ac..e102780ca 100644 --- a/test/unstructured_storage.js +++ b/test/unstructured_storage.js @@ -7,6 +7,7 @@ const AppProxyPinnedStorageMock = artifacts.require('AppProxyPinnedStorageMock') const DepositableStorageMock = artifacts.require('DepositableStorageMock') const InitializableStorageMock = artifacts.require('InitializableStorageMock') const KernelPinnedStorageMock = artifacts.require('KernelPinnedStorageMock') +const ReentrancyGuardMock = artifacts.require('ReentrancyGuardMock') contract('Unstructured storage', accounts => { context('> AppStorage', () => { @@ -19,7 +20,7 @@ contract('Unstructured storage', accounts => { it('tests Kernel storage', async () => { const kernel = await Kernel.new(true) await appStorage.setKernelExt(kernel.address) - //checks + // checks assert.equal( await web3.eth.getStorageAt(appStorage.address, (await appStorage.getKernelPosition())), (await appStorage.kernel()).toString(), @@ -35,7 +36,7 @@ contract('Unstructured storage', accounts => { it('tests appID storage', async () => { const appId = '0x1234000000000000000000000000000000000000000000000000000000000000' await appStorage.setAppIdExt(appId) - //checks + // checks assert.equal( await web3.eth.getStorageAt(appStorage.address, (await appStorage.getAppIdPosition())), (await appStorage.appId()).toString(), @@ -61,7 +62,7 @@ contract('Unstructured storage', accounts => { it('tests pinnedCode storage', async () => { const pinnedCode = '0x1200000000000000000000000000000000005678' await appPinned.setPinnedCodeExt(pinnedCode) - //checks + // checks assert.equal( await web3.eth.getStorageAt(appPinned.address, (await appPinned.getPinnedCodePosition())), (await appPinned.pinnedCodeExt()).toString(), @@ -85,7 +86,7 @@ contract('Unstructured storage', accounts => { it('tests depositable', async () => { // set values await depositableMock.setDepositableExt(true) - //checks + // checks assert.equal( await web3.eth.getStorageAt(depositableMock.address, (await depositableMock.getDepositablePosition())), true, @@ -105,7 +106,7 @@ contract('Unstructured storage', accounts => { // set values await initializableMock.initialize() const blockNumber = web3.eth.blockNumber - //checks + // checks assert.equal( parseInt( await web3.eth.getStorageAt( @@ -130,4 +131,23 @@ contract('Unstructured storage', accounts => { ) }) }) + + context('> ReentrancyGuard', () => { + let reentrancyGuardMock + + beforeEach(async () => { + reentrancyGuardMock = await ReentrancyGuardMock.new() + }) + + it('tests reentrancy mutex', async () => { + // set values + await reentrancyGuardMock.setReentrancyMutex(true) + // checks + assert.equal( + await web3.eth.getStorageAt(reentrancyGuardMock.address, (await reentrancyGuardMock.getReentrancyMutexPosition())), + true, + 'Reentrancy mutex should match' + ) + }) + }) }) From 6c7da962bd33fb8cab17bf9818f8b73450eaf350 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Fri, 5 Apr 2019 11:29:44 +0200 Subject: [PATCH 06/15] feat: add ConversionHelpers lib for converting memory types (#501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidates all the bytes<>uint256[] conversions into a library. It's not _too_ costly to add, and hopefully makes us all feel a bit better about this bit 😄. Also fixes the solidity test runner, which must've broke at some point along the way (due to the assert logs not being decoded properly) 😅. --- contracts/acl/ACL.sol | 13 +- contracts/apps/AragonApp.sol | 19 ++- contracts/common/ConversionHelpers.sol | 26 ++++ contracts/kernel/Kernel.sol | 20 ++- contracts/test/TestConversionHelpers.sol | 151 +++++++++++++++++++++++ test/conversion_helpers.js | 3 + test/helpers/decodeEvent.js | 2 +- test/helpers/runSolidityTest.js | 31 ++++- 8 files changed, 225 insertions(+), 40 deletions(-) create mode 100644 contracts/common/ConversionHelpers.sol create mode 100644 contracts/test/TestConversionHelpers.sol create mode 100644 test/conversion_helpers.js diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index c9c62d770..032f556fb 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -1,6 +1,7 @@ pragma solidity 0.4.24; import "../apps/AragonApp.sol"; +import "../common/ConversionHelpers.sol"; import "../common/TimeHelpers.sol"; import "./ACLSyntaxSugar.sol"; import "./IACL.sol"; @@ -242,17 +243,7 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { * @return boolean indicating whether the ACL allows the role or not */ function hasPermission(address _who, address _where, bytes32 _what, bytes memory _how) public view returns (bool) { - // Force cast the bytes array into a uint256[], by overwriting its length - // Note that the uint256[] doesn't need to be initialized as we immediately overwrite it - // with _how and a new length, and _how becomes invalid from this point forward - uint256[] memory how; - uint256 intsLength = _how.length / 32; - assembly { - how := _how - mstore(how, intsLength) - } - - return hasPermission(_who, _where, _what, how); + return hasPermission(_who, _where, _what, ConversionHelpers.dangerouslyCastBytesToUintArray(_how)); } function hasPermission(address _who, address _where, bytes32 _what, uint256[] memory _how) public view returns (bool) { diff --git a/contracts/apps/AragonApp.sol b/contracts/apps/AragonApp.sol index bc91fa48c..f53c40721 100644 --- a/contracts/apps/AragonApp.sol +++ b/contracts/apps/AragonApp.sol @@ -5,11 +5,12 @@ pragma solidity ^0.4.24; import "./AppStorage.sol"; +import "../acl/ACLSyntaxSugar.sol"; import "../common/Autopetrified.sol"; +import "../common/ConversionHelpers.sol"; import "../common/ReentrancyGuard.sol"; import "../common/VaultRecoverable.sol"; import "../evmscript/EVMScriptRunner.sol"; -import "../acl/ACLSyntaxSugar.sol"; // Contracts inheriting from AragonApp are, by default, immediately petrified upon deployment so @@ -48,16 +49,12 @@ contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, ReentrancyGua return false; } - // Force cast the uint256[] into a bytes array, by overwriting its length - // Note that the bytes array doesn't need to be initialized as we immediately overwrite it - // with _params and a new length, and _params becomes invalid from this point forward - bytes memory how; - uint256 byteLength = _params.length * 32; - assembly { - how := _params - mstore(how, byteLength) - } - return linkedKernel.hasPermission(_sender, address(this), _role, how); + return linkedKernel.hasPermission( + _sender, + address(this), + _role, + ConversionHelpers.dangerouslyCastUintArrayToBytes(_params) + ); } /** diff --git a/contracts/common/ConversionHelpers.sol b/contracts/common/ConversionHelpers.sol new file mode 100644 index 000000000..e574a634b --- /dev/null +++ b/contracts/common/ConversionHelpers.sol @@ -0,0 +1,26 @@ +pragma solidity ^0.4.24; + + +library ConversionHelpers { + function dangerouslyCastUintArrayToBytes(uint256[] memory _input) internal pure returns (bytes memory output) { + // Force cast the uint256[] into a bytes array, by overwriting its length + // Note that the bytes array doesn't need to be initialized as we immediately overwrite it + // with the input and a new length. The input becomes invalid from this point forward. + uint256 byteLength = _input.length * 32; + assembly { + output := _input + mstore(output, byteLength) + } + } + + function dangerouslyCastBytesToUintArray(bytes memory _input) internal pure returns (uint256[] memory output) { + // Force cast the bytes array into a uint256[], by overwriting its length + // Note that the uint256[] doesn't need to be initialized as we immediately overwrite it + // with the input and a new length. The input becomes invalid from this point forward. + uint256 intsLength = _input.length / 32; + assembly { + output := _input + mstore(output, intsLength) + } + } +} diff --git a/contracts/kernel/Kernel.sol b/contracts/kernel/Kernel.sol index 71c312d21..1fc919055 100644 --- a/contracts/kernel/Kernel.sol +++ b/contracts/kernel/Kernel.sol @@ -5,11 +5,12 @@ import "./KernelConstants.sol"; import "./KernelStorage.sol"; import "../acl/IACL.sol"; import "../acl/ACLSyntaxSugar.sol"; -import "../lib/misc/ERCProxy.sol"; +import "../common/ConversionHelpers.sol"; import "../common/IsContract.sol"; import "../common/Petrifiable.sol"; import "../common/VaultRecoverable.sol"; import "../factory/AppProxyFactory.sol"; +import "../lib/misc/ERCProxy.sol"; // solium-disable-next-line max-len @@ -227,18 +228,11 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant } } - modifier auth(bytes32 _role, uint256[] memory params) { - // Force cast the uint256[] into a bytes array, by overwriting its length - // Note that the bytes array doesn't need to be initialized as we immediately overwrite it - // with params and a new length, and params becomes invalid from this point forward - bytes memory how; - uint256 byteLength = params.length * 32; - assembly { - how := params - mstore(how, byteLength) - } - - require(hasPermission(msg.sender, address(this), _role, how), ERROR_AUTH_FAILED); + modifier auth(bytes32 _role, uint256[] memory _params) { + require( + hasPermission(msg.sender, address(this), _role, ConversionHelpers.dangerouslyCastUintArrayToBytes(_params)), + ERROR_AUTH_FAILED + ); _; } } diff --git a/contracts/test/TestConversionHelpers.sol b/contracts/test/TestConversionHelpers.sol new file mode 100644 index 000000000..802a6ee79 --- /dev/null +++ b/contracts/test/TestConversionHelpers.sol @@ -0,0 +1,151 @@ +pragma solidity 0.4.24; + +import "./helpers/Assert.sol"; +import "../common/ConversionHelpers.sol"; + + +contract TestConversionHelpers { + uint256 constant internal FIRST = uint256(keccak256("0")); + uint256 constant internal SECOND = uint256(keccak256("1")); + uint256 constant internal THIRD = uint256(keccak256("2")); + + function testUintArrayConvertedToBytes() public { + uint256[] memory arr = new uint256[](3); + arr[0] = FIRST; + arr[1] = SECOND; + arr[2] = THIRD; + uint256 arrLength = arr.length; + + // Do conversion + bytes memory arrBytes = ConversionHelpers.dangerouslyCastUintArrayToBytes(arr); + + // Check length + Assert.equal(arrBytes.length, arrLength * 32, "should have correct length as bytes array"); + + // Check values + assertValues(arrBytes); + + // Check memory position (conversion should be in place) + uint256 arrMemLoc; + uint256 arrBytesMemLoc; + assembly { + arrMemLoc := arr + arrBytesMemLoc := arrBytes + } + Assert.equal(arrMemLoc, arrBytesMemLoc, "should have same memory location after conversion"); + } + + function testUintArrayIntactIfConvertedBack() public { + uint256[] memory arr = new uint256[](3); + arr[0] = FIRST; + arr[1] = SECOND; + arr[2] = THIRD; + uint256 arrLength = arr.length; + + // Convert to and back + bytes memory arrBytes = ConversionHelpers.dangerouslyCastUintArrayToBytes(arr); + uint256[] memory arrReconverted = ConversionHelpers.dangerouslyCastBytesToUintArray(arrBytes); + + // Check length + Assert.equal(arrLength, arrReconverted.length, "should have correct length after reconverting"); + + // Check values + assertValues(arrReconverted); + + // Check memory position (conversion should be in place) + uint256 arrMemLoc; + uint256 arrReconvertedMemLoc; + assembly { + arrMemLoc := arr + arrReconvertedMemLoc := arrReconverted + } + Assert.equal(arrMemLoc, arrReconvertedMemLoc, "should have same memory location after reconverting"); + } + + function testBytesConvertedToUintArray() public { + bytes memory arr = new bytes(96); + + // Fill in bytes arr + uint256 first = FIRST; + uint256 second = SECOND; + uint256 third = THIRD; + assembly { + mstore(add(arr, 0x20), first) + mstore(add(arr, 0x40), second) + mstore(add(arr, 0x60), third) + } + uint256 arrLength = arr.length; + + // Do conversion + uint256[] memory arrUint = ConversionHelpers.dangerouslyCastBytesToUintArray(arr); + + // Check length + Assert.equal(arrUint.length, arrLength / 32, "should have correct length as uint256 array"); + + // Check values + assertValues(arrUint); + + // Check memory position (conversion should be in place) + uint256 arrMemLoc; + uint256 arrUintMemLoc; + assembly { + arrMemLoc := arr + arrUintMemLoc := arrUint + } + Assert.equal(arrMemLoc, arrUintMemLoc, "should have same memory location after conversion"); + } + + function testBytesIntactIfConvertedBack() public { + bytes memory arr = new bytes(96); + + // Fill in bytes arr + uint256 first = FIRST; + uint256 second = SECOND; + uint256 third = THIRD; + assembly { + mstore(add(arr, 0x20), first) + mstore(add(arr, 0x40), second) + mstore(add(arr, 0x60), third) + } + uint256 arrLength = arr.length; + + // Convert to and back + uint256[] memory arrUint = ConversionHelpers.dangerouslyCastBytesToUintArray(arr); + bytes memory arrReconverted = ConversionHelpers.dangerouslyCastUintArrayToBytes(arrUint); + + // Check length + Assert.equal(arrLength, arrReconverted.length, "should have correct length after reconverting"); + + // Check values + assertValues(arrReconverted); + + // Check memory position (conversion should be in place) + uint256 arrMemLoc; + uint256 arrReconvertedMemLoc; + assembly { + arrMemLoc := arr + arrReconvertedMemLoc := arrReconverted + } + Assert.equal(arrMemLoc, arrReconvertedMemLoc, "should have same memory location after reconverting"); + } + + function assertValues(uint256[] memory _data) { + Assert.equal(_data[0], FIRST, "should have correct index value at 0"); + Assert.equal(_data[1], SECOND, "should have correct index value at 1"); + Assert.equal(_data[2], THIRD, "should have correct index value at 2"); + } + + function assertValues(bytes memory _data) { + uint256 first; + uint256 second; + uint256 third; + assembly { + first := mload(add(_data, 0x20)) + second := mload(add(_data, 0x40)) + third := mload(add(_data, 0x60)) + } + Assert.equal(first, FIRST, "should have correct first value"); + Assert.equal(second, SECOND, "should have correct second value"); + Assert.equal(third, THIRD, "should have correct third value"); + } +} diff --git a/test/conversion_helpers.js b/test/conversion_helpers.js new file mode 100644 index 000000000..b25f3010d --- /dev/null +++ b/test/conversion_helpers.js @@ -0,0 +1,3 @@ +const runSolidityTest = require('./helpers/runSolidityTest') + +runSolidityTest('TestConversionHelpers') diff --git a/test/helpers/decodeEvent.js b/test/helpers/decodeEvent.js index 0a8a2d7e8..8e974a15b 100644 --- a/test/helpers/decodeEvent.js +++ b/test/helpers/decodeEvent.js @@ -6,7 +6,7 @@ module.exports = { const eventSignature = abi.encodeEventSignature(eventAbi) const eventLogs = receipt.logs.filter(l => l.topics[0] === eventSignature) return eventLogs.map(log => { - log.event = abi.name + log.event = eventAbi.name log.args = abi.decodeLog(eventAbi.inputs, log.data, log.topics.slice(1)) return log }) diff --git a/test/helpers/runSolidityTest.js b/test/helpers/runSolidityTest.js index 9e12c31f2..1978769e1 100644 --- a/test/helpers/runSolidityTest.js +++ b/test/helpers/runSolidityTest.js @@ -1,3 +1,25 @@ +const { decodeEventsOfType } = require('./decodeEvent') + +const ASSERT_LIB_EVENTS_ABI = [ + { + "anonymous": false, + "inputs": [ + { + "indexed": true, + "name": "result", + "type": "bool" + }, + { + "indexed": false, + "name": "message", + "type": "string" + } + ], + "name": "TestEvent", + "type": "event" + }, +] + const HOOKS_MAP = { beforeAll: 'before', beforeEach: 'beforeEach', @@ -5,11 +27,12 @@ const HOOKS_MAP = { afterAll: 'afterAll', } -const processResult = receipt => { - if (!receipt) { +const processResult = txReceipt => { + if (!txReceipt || !txReceipt.receipt) { return } - receipt.logs.forEach(log => { + const decodedLogs = decodeEventsOfType(txReceipt.receipt, ASSERT_LIB_EVENTS_ABI, 'TestEvent') + decodedLogs.forEach(log => { if (log.event === 'TestEvent' && log.args.result !== true) { throw new Error(log.args.message) } @@ -27,7 +50,7 @@ const linkLib = async (contract, libName) => { const prefix = underscores(PREFIX_UNDERSCORES) const suffix = underscores(ADDR_LENGTH - PREFIX_UNDERSCORES - libName.length) - + const libPlaceholder = `${prefix}${libName}${suffix}` const lib = await artifacts.require(libName).new() From 2bfd50a450512221126545aabca897af2b233a4c Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Fri, 5 Apr 2019 11:39:38 +0200 Subject: [PATCH 07/15] chore: Improve APM Repo version creation notice (#502) --- contracts/apm/APMRegistry.sol | 2 +- contracts/apm/Repo.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/apm/APMRegistry.sol b/contracts/apm/APMRegistry.sol index b457f4b70..1a0ae42c4 100644 --- a/contracts/apm/APMRegistry.sol +++ b/contracts/apm/APMRegistry.sol @@ -59,7 +59,7 @@ contract APMRegistry is AragonApp, AppProxyFactory, APMInternalAppNames { } /** - * @notice Create new repo in registry with `_name` and first repo version + * @notice Create new repo in registry with `_name` and publish a first version with contract `_contractAddress` and content `@fromHex(_contentURI)` * @param _name Repo name * @param _dev Address that will be given permission to create versions * @param _initialSemanticVersion Semantic version for new repo version diff --git a/contracts/apm/Repo.sol b/contracts/apm/Repo.sol index 30b54d430..0cb73c2a8 100644 --- a/contracts/apm/Repo.sol +++ b/contracts/apm/Repo.sol @@ -38,7 +38,7 @@ contract Repo is AragonApp { } /** - * @notice Create new version for repo + * @notice Create new version with contract `_contractAddress` and content `@fromHex(_contentURI)` * @param _newSemanticVersion Semantic version for new repo version * @param _contractAddress address for smart contract logic for version (if set to 0, it uses last versions' contractAddress) * @param _contentURI External URI for fetching new version's content From 446653d01a7b3f2344a32da244ef432edd38d0dc Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Fri, 5 Apr 2019 13:37:56 +0200 Subject: [PATCH 08/15] EVMScripts: check byte lengths, avoid returning unallocated memory, and forward error data (#496) --- contracts/evmscript/EVMScriptRegistry.sol | 4 + contracts/evmscript/EVMScriptRunner.sol | 54 +- contracts/evmscript/executors/CallsScript.sol | 48 +- contracts/test/mocks/AppStubScriptRunner.sol | 57 ++ .../test/mocks/EVMScriptExecutorMock.sol | 7 +- .../mocks/EVMScriptExecutorRevertMock.sol | 17 + contracts/test/mocks/ExecutionTarget.sol | 9 +- .../test/mocks/MockScriptExecutorApp.sol | 33 - test/evm_script.js | 716 +++++++++++------- test/evm_script_factory.js | 125 +++ test/helpers/assertThrow.js | 48 +- test/helpers/evmScript.js | 15 +- test/helpers/revertStrings.js | 74 ++ 13 files changed, 845 insertions(+), 362 deletions(-) create mode 100644 contracts/test/mocks/AppStubScriptRunner.sol create mode 100644 contracts/test/mocks/EVMScriptExecutorRevertMock.sol delete mode 100644 contracts/test/mocks/MockScriptExecutorApp.sol create mode 100644 test/evm_script_factory.js create mode 100644 test/helpers/revertStrings.js diff --git a/contracts/evmscript/EVMScriptRegistry.sol b/contracts/evmscript/EVMScriptRegistry.sol index 4f7f308fe..7eb8dd3d6 100644 --- a/contracts/evmscript/EVMScriptRegistry.sol +++ b/contracts/evmscript/EVMScriptRegistry.sol @@ -19,9 +19,12 @@ contract EVMScriptRegistry is IEVMScriptRegistry, EVMScriptRegistryConstants, Ar // WARN: Manager can censor all votes and the like happening in an org bytes32 public constant REGISTRY_MANAGER_ROLE = 0xf7a450ef335e1892cb42c8ca72e7242359d7711924b75db5717410da3f614aa3; + uint256 internal constant SCRIPT_START_LOCATION = 4; + string private constant ERROR_INEXISTENT_EXECUTOR = "EVMREG_INEXISTENT_EXECUTOR"; string private constant ERROR_EXECUTOR_ENABLED = "EVMREG_EXECUTOR_ENABLED"; string private constant ERROR_EXECUTOR_DISABLED = "EVMREG_EXECUTOR_DISABLED"; + string private constant ERROR_SCRIPT_LENGTH_TOO_SHORT = "EVMREG_SCRIPT_LENGTH_TOO_SHORT"; struct ExecutorEntry { IEVMScriptExecutor executor; @@ -96,6 +99,7 @@ contract EVMScriptRegistry is IEVMScriptRegistry, EVMScriptRegistryConstants, Ar * @param _script EVMScript being inspected */ function getScriptExecutor(bytes _script) public view returns (IEVMScriptExecutor) { + require(_script.length >= SCRIPT_START_LOCATION, ERROR_SCRIPT_LENGTH_TOO_SHORT); uint256 id = _script.getSpecId(); // Note that we don't need to check for an executor's existence in this case, as only diff --git a/contracts/evmscript/EVMScriptRunner.sol b/contracts/evmscript/EVMScriptRunner.sol index 44c20459f..28970632c 100644 --- a/contracts/evmscript/EVMScriptRunner.sol +++ b/contracts/evmscript/EVMScriptRunner.sol @@ -14,7 +14,6 @@ import "../common/Initializable.sol"; contract EVMScriptRunner is AppStorage, Initializable, EVMScriptRegistryConstants, KernelNamespaceConstants { string private constant ERROR_EXECUTOR_UNAVAILABLE = "EVMRUN_EXECUTOR_UNAVAILABLE"; - string private constant ERROR_EXECUTION_REVERTED = "EVMRUN_EXECUTION_REVERTED"; string private constant ERROR_PROTECTED_STATE_MODIFIED = "EVMRUN_PROTECTED_STATE_MODIFIED"; event ScriptResult(address indexed executor, bytes script, bytes input, bytes returnData); @@ -34,36 +33,51 @@ contract EVMScriptRunner is AppStorage, Initializable, EVMScriptRegistryConstant protectState returns (bytes) { - // TODO: Too much data flying around, maybe extracting spec id here is cheaper IEVMScriptExecutor executor = getEVMScriptExecutor(_script); require(address(executor) != address(0), ERROR_EXECUTOR_UNAVAILABLE); bytes4 sig = executor.execScript.selector; bytes memory data = abi.encodeWithSelector(sig, _script, _input, _blacklist); - require(address(executor).delegatecall(data), ERROR_EXECUTION_REVERTED); - bytes memory output = returnedDataDecoded(); - - emit ScriptResult(address(executor), _script, _input, output); + bytes memory output; + assembly { + let success := delegatecall( + gas, // forward all gas + executor, // address + add(data, 0x20), // calldata start + mload(data), // calldata length + 0, // don't write output (we'll handle this ourselves) + 0 // don't write output + ) - return output; - } + output := mload(0x40) // free mem ptr get - /** - * @dev copies and returns last's call data. Needs to ABI decode first - */ - function returnedDataDecoded() internal pure returns (bytes ret) { - assembly { - let size := returndatasize - switch size - case 0 {} + switch success + case 0 { + // If the call errored, forward its full error data + returndatacopy(output, 0, returndatasize) + revert(output, returndatasize) + } default { - ret := mload(0x40) // free mem ptr get - mstore(0x40, add(ret, add(size, 0x20))) // free mem ptr set - returndatacopy(ret, 0x20, sub(size, 0x20)) // copy return data + // Copy result + // + // Needs to perform an ABI decode for the expected `bytes` return type of + // `executor.execScript()` as solidity will automatically ABI encode the returned bytes as: + // [ position of the first dynamic length return value = 0x20 (32 bytes) ] + // [ output length (32 bytes) ] + // [ output content (N bytes) ] + // + // Perform the ABI decode by ignoring the first 32 bytes of the return data + let copysize := sub(returndatasize, 0x20) + returndatacopy(output, 0x20, copysize) + + mstore(0x40, add(output, copysize)) // free mem ptr set } } - return ret; + + emit ScriptResult(address(executor), _script, _input, output); + + return output; } modifier protectState { diff --git a/contracts/evmscript/executors/CallsScript.sol b/contracts/evmscript/executors/CallsScript.sol index e46ebf5bc..7345ae31a 100644 --- a/contracts/evmscript/executors/CallsScript.sol +++ b/contracts/evmscript/executors/CallsScript.sol @@ -16,7 +16,10 @@ contract CallsScript is BaseEVMScriptExecutor { string private constant ERROR_BLACKLISTED_CALL = "EVMCALLS_BLACKLISTED_CALL"; string private constant ERROR_INVALID_LENGTH = "EVMCALLS_INVALID_LENGTH"; + + /* This is manually crafted in assembly string private constant ERROR_CALL_REVERTED = "EVMCALLS_CALL_REVERTED"; + */ event LogScriptCall(address indexed sender, address indexed src, address indexed dst); @@ -25,14 +28,17 @@ contract CallsScript is BaseEVMScriptExecutor { * @param _script [ specId (uint32) ] many calls with this structure -> * [ to (address: 20 bytes) ] [ calldataLength (uint32: 4 bytes) ] [ calldata (calldataLength bytes) ] * @param _blacklist Addresses the script cannot call to, or will revert. - * @return always returns empty byte array + * @return Always returns empty byte array */ function execScript(bytes _script, bytes, address[] _blacklist) external isInitialized returns (bytes) { uint256 location = SCRIPT_START_LOCATION; // first 32 bits are spec id while (location < _script.length) { + // Check there's at least address + calldataLength available + require(_script.length - location >= 0x18, ERROR_INVALID_LENGTH); + address contractAddress = _script.addressAt(location); // Check address being called is not blacklist - for (uint i = 0; i < _blacklist.length; i++) { + for (uint256 i = 0; i < _blacklist.length; i++) { require(contractAddress != _blacklist[i], ERROR_BLACKLISTED_CALL); } @@ -50,11 +56,43 @@ contract CallsScript is BaseEVMScriptExecutor { bool success; assembly { - success := call(sub(gas, 5000), contractAddress, 0, calldataStart, calldataLength, 0, 0) - } + success := call( + sub(gas, 5000), // forward gas left - 5000 + contractAddress, // address + 0, // no value + calldataStart, // calldata start + calldataLength, // calldata length + 0, // don't write output + 0 // don't write output + ) - require(success, ERROR_CALL_REVERTED); + switch success + case 0 { + let ptr := mload(0x40) + + switch returndatasize + case 0 { + // No error data was returned, revert with "EVMCALLS_CALL_REVERTED" + // See remix: doing a `revert("EVMCALLS_CALL_REVERTED")` always results in + // this memory layout + mstore(ptr, 0x08c379a000000000000000000000000000000000000000000000000000000000) // error identifier + mstore(add(ptr, 0x04), 0x0000000000000000000000000000000000000000000000000000000000000020) // starting offset + mstore(add(ptr, 0x24), 0x0000000000000000000000000000000000000000000000000000000000000016) // reason length + mstore(add(ptr, 0x44), 0x45564d43414c4c535f43414c4c5f524556455254454400000000000000000000) // reason + + revert(ptr, 100) // 100 = 4 + 3 * 32 (error identifier + 3 words for the ABI encoded error) + } + default { + // Forward the full error data + returndatacopy(ptr, 0, returndatasize) + revert(ptr, returndatasize) + } + } + default { } + } } + // No need to allocate empty bytes for the return as this can only be called via an delegatecall + // (due to the isInitialized modifier) } function executorType() external pure returns (bytes32) { diff --git a/contracts/test/mocks/AppStubScriptRunner.sol b/contracts/test/mocks/AppStubScriptRunner.sol new file mode 100644 index 000000000..6fcf54aa6 --- /dev/null +++ b/contracts/test/mocks/AppStubScriptRunner.sol @@ -0,0 +1,57 @@ +pragma solidity 0.4.24; + +import "../../apps/AragonApp.sol"; + + +contract AppStubScriptRunner is AragonApp { + event ReturnedBytes(bytes returnedBytes); + + // Initialization is required to access any of the real executors + function initialize() public { + initialized(); + } + + function runScript(bytes script) public returns (bytes) { + bytes memory returnedBytes = runScript(script, new bytes(0), new address[](0)); + emit ReturnedBytes(returnedBytes); + return returnedBytes; + } + + function runScriptWithBan(bytes script, address[] memory blacklist) public returns (bytes) { + bytes memory returnedBytes = runScript(script, new bytes(0), blacklist); + emit ReturnedBytes(returnedBytes); + return returnedBytes; + } + + function runScriptWithIO(bytes script, bytes input, address[] memory blacklist) public returns (bytes) { + bytes memory returnedBytes = runScript(script, input, blacklist); + emit ReturnedBytes(returnedBytes); + return returnedBytes; + } + + function runScriptWithNewBytesAllocation(bytes script) public returns (bytes) { + bytes memory returnedBytes = runScript(script, new bytes(0), new address[](0)); + bytes memory newBytes = new bytes(64); + + // Fill in new bytes array with some dummy data to let us check it doesn't corrupt the + // script's returned bytes + uint256 first = uint256(keccak256("test")); + uint256 second = uint256(keccak256("mock")); + assembly { + mstore(add(newBytes, 0x20), first) + mstore(add(newBytes, 0x40), second) + } + emit ReturnedBytes(returnedBytes); + return returnedBytes; + } + + /* + function getActionsCount(bytes script) public constant returns (uint256) { + return getScriptActionsCount(script); + } + + function getAction(bytes script, uint256 i) public constant returns (address, bytes) { + return getScriptAction(script, i); + } + */ +} diff --git a/contracts/test/mocks/EVMScriptExecutorMock.sol b/contracts/test/mocks/EVMScriptExecutorMock.sol index e96713e42..3db26fe82 100644 --- a/contracts/test/mocks/EVMScriptExecutorMock.sol +++ b/contracts/test/mocks/EVMScriptExecutorMock.sol @@ -6,7 +6,12 @@ import "../../evmscript/executors/BaseEVMScriptExecutor.sol"; contract EVMScriptExecutorMock is BaseEVMScriptExecutor { bytes32 internal constant EXECUTOR_TYPE = keccak256("MOCK_SCRIPT"); - function execScript(bytes, bytes, address[]) external isInitialized returns (bytes) { + function execScript(bytes _script, bytes, address[]) external isInitialized returns (bytes) { + // Return full input script if it's more than just the spec ID, otherwise return an empty + // bytes array + if (_script.length > SCRIPT_START_LOCATION) { + return _script; + } } function executorType() external pure returns (bytes32) { diff --git a/contracts/test/mocks/EVMScriptExecutorRevertMock.sol b/contracts/test/mocks/EVMScriptExecutorRevertMock.sol new file mode 100644 index 000000000..9ee5dd5b7 --- /dev/null +++ b/contracts/test/mocks/EVMScriptExecutorRevertMock.sol @@ -0,0 +1,17 @@ +pragma solidity 0.4.24; + + +import "../../evmscript/executors/BaseEVMScriptExecutor.sol"; + +contract EVMScriptExecutorRevertMock is BaseEVMScriptExecutor { + string public constant ERROR_MOCK_REVERT = "MOCK_REVERT"; + bytes32 internal constant EXECUTOR_TYPE = keccak256("MOCK_REVERT_SCRIPT"); + + function execScript(bytes, bytes, address[]) external isInitialized returns (bytes) { + revert(ERROR_MOCK_REVERT); + } + + function executorType() external pure returns (bytes32) { + return EXECUTOR_TYPE; + } +} diff --git a/contracts/test/mocks/ExecutionTarget.sol b/contracts/test/mocks/ExecutionTarget.sol index c37930c17..55c41a113 100644 --- a/contracts/test/mocks/ExecutionTarget.sol +++ b/contracts/test/mocks/ExecutionTarget.sol @@ -2,6 +2,7 @@ pragma solidity 0.4.24; contract ExecutionTarget { + string public constant ERROR_EXECUTION_TARGET = "EXECUTION_TARGET_REVERTED"; uint public counter; function execute() public { @@ -9,8 +10,12 @@ contract ExecutionTarget { emit Executed(counter); } - function failExecute() public pure { - revert(); + function failExecute(bool errorWithData) public pure { + if (errorWithData) { + revert(ERROR_EXECUTION_TARGET); + } else { + revert(); + } } function setCounter(uint x) public { diff --git a/contracts/test/mocks/MockScriptExecutorApp.sol b/contracts/test/mocks/MockScriptExecutorApp.sol deleted file mode 100644 index 291272390..000000000 --- a/contracts/test/mocks/MockScriptExecutorApp.sol +++ /dev/null @@ -1,33 +0,0 @@ -pragma solidity 0.4.24; - -import "../../apps/AragonApp.sol"; - - -contract MockScriptExecutorApp is AragonApp { - // Initialization is required to access any of the real executors - function initialize() public { - initialized(); - } - - function execute(bytes script) public { - runScript(script, new bytes(0), new address[](0)); - } - - function executeWithBan(bytes script, address[] memory blacklist) public { - runScript(script, new bytes(0), blacklist); - } - - function executeWithIO(bytes script, bytes input, address[] memory blacklist) public returns (bytes) { - return runScript(script, input, blacklist); - } - - /* - function getActionsCount(bytes script) public constant returns (uint256) { - return getScriptActionsCount(script); - } - - function getAction(bytes script, uint256 i) public constant returns (address, bytes) { - return getScriptAction(script, i); - } - */ -} diff --git a/test/evm_script.js b/test/evm_script.js index 48ad317f1..b88cb0f2f 100644 --- a/test/evm_script.js +++ b/test/evm_script.js @@ -3,362 +3,522 @@ const { soliditySha3 } = require('web3-utils') const assertEvent = require('./helpers/assertEvent') const { assertRevert } = require('./helpers/assertThrow') -const { encodeCallScript } = require('./helpers/evmScript') +const { createExecutorId, encodeCallScript } = require('./helpers/evmScript') +const reverts = require('./helpers/revertStrings') const Kernel = artifacts.require('Kernel') +const KernelProxy = artifacts.require('KernelProxy') const ACL = artifacts.require('ACL') -const DAOFactory = artifacts.require('DAOFactory') -const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') const EVMScriptRegistry = artifacts.require('EVMScriptRegistry') const CallsScript = artifacts.require('CallsScript') const IEVMScriptExecutor = artifacts.require('IEVMScriptExecutor') // Mocks +const AppStubScriptRunner = artifacts.require('AppStubScriptRunner') const ExecutionTarget = artifacts.require('ExecutionTarget') const EVMScriptExecutorMock = artifacts.require('EVMScriptExecutorMock') -const MockScriptExecutorApp = artifacts.require('MockScriptExecutorApp') +const EVMScriptExecutorRevertMock = artifacts.require('EVMScriptExecutorRevertMock') +const EVMScriptRegistryConstantsMock = artifacts.require('EVMScriptRegistryConstantsMock') -const EMPTY_BYTES = '0x' const ZERO_ADDR = '0x0000000000000000000000000000000000000000' +const EMPTY_BYTES = '0x' contract('EVM Script', accounts => { - let callsScriptBase, executorAppBase, executorApp, executionTarget, dao, daoFact, reg, acl - let APP_BASES_NAMESPACE + let kernelBase, aclBase, evmScriptRegBase, dao, acl, evmScriptReg, scriptExecutorMock, scriptExecutorRevertMock + let APP_BASES_NAMESPACE, APP_ADDR_NAMESPACE, APP_MANAGER_ROLE + let EVMSCRIPT_REGISTRY_APP_ID, REGISTRY_ADD_EXECUTOR_ROLE, REGISTRY_MANAGER_ROLE + let ERROR_MOCK_REVERT, ERROR_EXECUTION_TARGET + + const boss = accounts[1] + + const scriptRunnerAppId = '0x1234' + + before(async () => { + kernelBase = await Kernel.new(true) // petrify immediately + aclBase = await ACL.new() + evmScriptRegBase = await EVMScriptRegistry.new() + scriptExecutorMock = await EVMScriptExecutorMock.new() + scriptExecutorRevertMock = await EVMScriptExecutorRevertMock.new() + const evmScriptRegConstants = await EVMScriptRegistryConstantsMock.new() + const executionTarget = await ExecutionTarget.new() + + APP_BASES_NAMESPACE = await kernelBase.APP_BASES_NAMESPACE() + APP_ADDR_NAMESPACE = await kernelBase.APP_ADDR_NAMESPACE() + APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE() + + EVMSCRIPT_REGISTRY_APP_ID = await evmScriptRegConstants.getEVMScriptRegistryAppId() + REGISTRY_ADD_EXECUTOR_ROLE = await evmScriptRegBase.REGISTRY_ADD_EXECUTOR_ROLE() + REGISTRY_MANAGER_ROLE = await evmScriptRegBase.REGISTRY_MANAGER_ROLE() + + ERROR_MOCK_REVERT = await scriptExecutorRevertMock.ERROR_MOCK_REVERT() + ERROR_EXECUTION_TARGET = await executionTarget.ERROR_EXECUTION_TARGET() + }) + + beforeEach(async () => { + dao = Kernel.at((await KernelProxy.new(kernelBase.address)).address) + await dao.initialize(aclBase.address, boss) + acl = ACL.at(await dao.acl()) + + // Set up app management permissions + await acl.createPermission(boss, dao.address, APP_MANAGER_ROLE, boss, { from: boss }) + + // Set up script registry (MUST use correct app ID and set as default app) + const initPayload = evmScriptRegBase.contract.initialize.getData() + const evmScriptRegReceipt = await dao.newAppInstance(EVMSCRIPT_REGISTRY_APP_ID, evmScriptRegBase.address, initPayload, true, { from: boss }) + evmScriptReg = EVMScriptRegistry.at(evmScriptRegReceipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) + }) + + context('> Registry', () => { + beforeEach(async () => { + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE, boss, { from: boss }) + }) + + it('is initialized', async () => { + assert.isTrue(await evmScriptReg.hasInitialized(), "should be initialized") + }) - const boss = accounts[1] + it('fails if reinitializing registry', async () => { + await assertRevert(evmScriptReg.initialize(), reverts.INIT_ALREADY_INITIALIZED) + }) - const executorAppId = '0x1234' + it('fails to get an executor if not enough bytes are given', async () => { + // Not enough bytes are given (need 4, given 3) + await assertRevert(evmScriptReg.getScriptExecutor('0x'.padEnd(6, 0)), reverts.EVMREG_SCRIPT_LENGTH_TOO_SHORT) + }) - const createExecutorId = id => `0x${String(id).padStart(8, '0')}` + it('can add a new executor', async () => { + const receipt = await evmScriptReg.addScriptExecutor(scriptExecutorMock.address, { from: boss }) - before(async () => { - const regFact = await EVMScriptRegistryFactory.new() - callsScriptBase = CallsScript.at(await regFact.baseCallScript()) + const newExecutorId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId + const [executorAddress, executorEnabled] = await evmScriptReg.executors(newExecutorId) + const newExecutor = IEVMScriptExecutor.at(executorAddress) + + assert.equal(await scriptExecutorMock.executorType(), await newExecutor.executorType(), "executor type should be the same") + assert.equal(await scriptExecutorMock.address, await executorAddress, "executor address should be the same") + assert.isTrue(executorEnabled, "new executor should be enabled") + }) + + it('fails to add a new executor without the correct permissions', async () => { + await assertRevert(evmScriptReg.addScriptExecutor(scriptExecutorMock.address), reverts.APP_AUTH_FAILED) + }) + + context('> Existing executor', () => { + let installedExecutorId + + beforeEach(async () => { + const receipt = await evmScriptReg.addScriptExecutor(scriptExecutorMock.address, { from: boss }) + + installedExecutorId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId + }) + + it('can disable an executor', async () => { + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss }) + + let executorEntry = await evmScriptReg.executors(installedExecutorId) + assert.isTrue(executorEntry[1], "executor should be enabled") + + const receipt = await evmScriptReg.disableScriptExecutor(installedExecutorId, { from: boss }) + executorEntry = await evmScriptReg.executors(installedExecutorId) - const kernelBase = await Kernel.new(true) // petrify immediately - const aclBase = await ACL.new() - daoFact = await DAOFactory.new(kernelBase.address, aclBase.address, regFact.address) + assertEvent(receipt, 'DisableExecutor') + assert.isFalse(executorEntry[1], "executor should now be disabled") + assert.equal(await evmScriptReg.getScriptExecutor(createExecutorId(installedExecutorId)), ZERO_ADDR, 'getting disabled executor should return zero addr') + }) - APP_BASES_NAMESPACE = await kernelBase.APP_BASES_NAMESPACE() + it('can re-enable an executor', async () => { + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss }) + + await evmScriptReg.disableScriptExecutor(installedExecutorId, { from: boss }) + let executorEntry = await evmScriptReg.executors(installedExecutorId) + assert.isFalse(executorEntry[1], "executor should now be disabled") + assert.equal(await evmScriptReg.getScriptExecutor(createExecutorId(installedExecutorId)), ZERO_ADDR, 'getting disabled executor should return zero addr') + + const receipt = await evmScriptReg.enableScriptExecutor(installedExecutorId, { from: boss }) + executorEntry = await evmScriptReg.executors(installedExecutorId) + + assertEvent(receipt, 'EnableExecutor') + assert.isTrue(executorEntry[1], "executor should now be re-enabled") + assert.notEqual(await evmScriptReg.getScriptExecutor(createExecutorId(installedExecutorId)), ZERO_ADDR, 'getting disabled executor should return non-zero addr') + }) + + it('fails to disable an executor without the correct permissions', async () => { + await assertRevert(evmScriptReg.disableScriptExecutor(installedExecutorId), reverts.APP_AUTH_FAILED) + }) + + it('fails to enable an executor without the correct permissions', async () => { + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss }) + await evmScriptReg.disableScriptExecutor(installedExecutorId, { from: boss }) + + await assertRevert(evmScriptReg.enableScriptExecutor(installedExecutorId), reverts.APP_AUTH_FAILED) + }) + + it('fails to enable an already enabled executor', async () => { + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss }) + await assertRevert(evmScriptReg.enableScriptExecutor(installedExecutorId, { from: boss }), reverts.EVMREG_EXECUTOR_ENABLED) + }) + + it('fails to enable a non-existent executor', async () => { + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss }) + await assertRevert(evmScriptReg.enableScriptExecutor(installedExecutorId + 1, { from: boss }), reverts.EVMREG_INEXISTENT_EXECUTOR) + }) + + it('fails to disable an already disabled executor', async () => { + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss }) + await evmScriptReg.disableScriptExecutor(installedExecutorId, { from: boss }) + + await assertRevert(evmScriptReg.disableScriptExecutor(installedExecutorId, { from: boss }), reverts.EVMREG_EXECUTOR_DISABLED) + }) + }) + }) + + context('> ScriptRunner', () => { + let scriptRunnerApp + + before(async () => { + scriptRunnerAppBase = await AppStubScriptRunner.new() }) beforeEach(async () => { - const receipt = await daoFact.newDAO(boss) - dao = Kernel.at(receipt.logs.filter(l => l.event == 'DeployDAO')[0].args.dao) - acl = ACL.at(await dao.acl()) - reg = EVMScriptRegistry.at(receipt.logs.filter(l => l.event == 'DeployEVMScriptRegistry')[0].args.reg) - - await acl.createPermission(boss, dao.address, await dao.APP_MANAGER_ROLE(), boss, { from: boss }) - executorAppBase = await MockScriptExecutorApp.new() - await dao.setApp(APP_BASES_NAMESPACE, executorAppId, executorAppBase.address, { from: boss }) + const receipt = await dao.newAppInstance(scriptRunnerAppId, scriptRunnerAppBase.address, EMPTY_BYTES, false, { from: boss }) + scriptRunnerApp = AppStubScriptRunner.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) + await scriptRunnerApp.initialize() }) - it('factory registered just 1 script executor', async () => { - assert.equal(await reg.getScriptExecutor(createExecutorId(0)), ZERO_ADDR) - assert.notEqual(await reg.getScriptExecutor(createExecutorId(1)), ZERO_ADDR) - assert.equal(await reg.getScriptExecutor(createExecutorId(2)), ZERO_ADDR) + it('gets the correct executor registry from the app', async () => { + const registryFromApp = await scriptRunnerApp.getEVMScriptRegistry() + assert.equal(evmScriptReg.address, registryFromApp, 'app should return the same EVMScriptRegistry') }) - it('fails if directly calling base executor', async () => { - const executionTarget = await ExecutionTarget.new() - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScript([action]) + it('fails to execute if spec ID is 0', async () => { + await assertRevert(scriptRunnerApp.runScript(createExecutorId(0)), reverts.EVMRUN_EXECUTOR_UNAVAILABLE) + }) - await assertRevert(() => callsScriptBase.execScript(script, EMPTY_BYTES, [])) + it('fails to execute if spec ID is unknown', async () => { + await assertRevert(scriptRunnerApp.runScript(createExecutorId(4)), reverts.EVMRUN_EXECUTOR_UNAVAILABLE) }) - context('> Registry', () => { - it('is initialized', async () => { - assert.isTrue(await reg.hasInitialized(), "should be initialized") - }) + context('> Uninitialized', () => { + beforeEach(async () => { + // Install mock executor onto registry + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE, boss, { from: boss }) + await evmScriptReg.addScriptExecutor(scriptExecutorMock.address, { from: boss }) + + // Install new script runner app + const receipt = await dao.newAppInstance(scriptRunnerAppId, scriptRunnerAppBase.address, EMPTY_BYTES, false, { from: boss }) + scriptRunnerApp = AppStubScriptRunner.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) + // Explicitly don't initialize the scriptRunnerApp + }) + + it('fails to execute any executor', async () => { + await assertRevert(scriptRunnerApp.runScript(createExecutorId(1)), reverts.INIT_NOT_INITIALIZED) + }) + }) - it('fails if reinitializing registry', async () => { - await assertRevert(async () => { - await reg.initialize() - }) - }) + context('> Registry actions', () => { + let executorSpecId - context('> New executor', () => { - let executorMock, newExecutorId - - before(async () => { - executorMock = await EVMScriptExecutorMock.new() - }) - - beforeEach(async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_ADD_EXECUTOR_ROLE(), boss, { from: boss }) - const receipt = await reg.addScriptExecutor(executorMock.address, { from: boss }) - - newExecutorId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId - }) - - it('can add a new executor', async () => { - const executorEntry = await reg.executors(newExecutorId) - const newExecutor = IEVMScriptExecutor.at(executorEntry[0]) - - assert.equal(await executorMock.executorType(), await newExecutor.executorType(), "executor type should be the same") - // Enabled flag is second in struct - assert.isTrue(executorEntry[1], "new executor should be enabled") - }) - - it('can disable an executor', async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - - let executorEntry = await reg.executors(newExecutorId) - assert.isTrue(executorEntry[1], "executor should be enabled") - - const receipt = await reg.disableScriptExecutor(newExecutorId, { from: boss }) - executorEntry = await reg.executors(newExecutorId) - - assertEvent(receipt, 'DisableExecutor') - assert.isFalse(executorEntry[1], "executor should now be disabled") - assert.equal(await reg.getScriptExecutor(createExecutorId(newExecutorId)), ZERO_ADDR, 'getting disabled executor should return zero addr') - }) - - it('can re-enable an executor', async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - - await reg.disableScriptExecutor(newExecutorId, { from: boss }) - let executorEntry = await reg.executors(newExecutorId) - assert.isFalse(executorEntry[1], "executor should now be disabled") - assert.equal(await reg.getScriptExecutor(createExecutorId(newExecutorId)), ZERO_ADDR, 'getting disabled executor should return zero addr') - - const receipt = await reg.enableScriptExecutor(newExecutorId, { from: boss }) - executorEntry = await reg.executors(newExecutorId) - - assertEvent(receipt, 'EnableExecutor') - assert.isTrue(executorEntry[1], "executor should now be re-enabled") - assert.notEqual(await reg.getScriptExecutor(createExecutorId(newExecutorId)), ZERO_ADDR, 'getting disabled executor should return non-zero addr') - }) - - it('fails to add a new executor without the correct permissions', async () => { - await assertRevert(async () => { - await reg.addScriptExecutor(executorMock.address) - }) - }) - - it('fails to disable an executor without the correct permissions', async () => { - await assertRevert(async () => { - await reg.disableScriptExecutor(newExecutorId) - }) - }) - - it('fails to enable an executor without the correct permissions', async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - await reg.disableScriptExecutor(newExecutorId, { from: boss }) - - await assertRevert(async () => { - await reg.enableScriptExecutor(newExecutorId) - }) - }) - - it('fails to enable an already enabled executor', async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - await assertRevert(async () => { - await reg.enableScriptExecutor(newExecutorId, { from: boss }) - }) - }) - - it('fails to enable a non-existent executor', async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - await assertRevert(async () => { - await reg.enableScriptExecutor(newExecutorId + 1, { from: boss }) - }) - }) - - it('fails to disable an already disabled executor', async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - await reg.disableScriptExecutor(newExecutorId, { from: boss }) - - await assertRevert(async () => { - await reg.disableScriptExecutor(newExecutorId, { from: boss }) - }) - }) - }) + beforeEach(async () => { + // Let boss enable and disable executors + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss }) + + // Install mock executor onto registry + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE, boss, { from: boss }) + const receipt = await evmScriptReg.addScriptExecutor(scriptExecutorMock.address, { from: boss }) + + executorSpecId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId + }) + + it("can't execute disabled spec ID", async () => { + await evmScriptReg.disableScriptExecutor(executorSpecId, { from: boss }) + + await assertRevert(scriptRunnerApp.runScript(encodeCallScript([])), reverts.EVMRUN_EXECUTOR_UNAVAILABLE) + }) + + it('can execute once spec ID is re-enabled', async () => { + // Disable then re-enable the executor + await evmScriptReg.disableScriptExecutor(executorSpecId, { from: boss }) + await evmScriptReg.enableScriptExecutor(executorSpecId, { from: boss }) + + await scriptRunnerApp.runScript(encodeCallScript([])) + }) }) - context('> Uninitialized executor', () => { - beforeEach(async () => { - const receipt = await dao.newAppInstance(executorAppId, executorAppBase.address, EMPTY_BYTES, false, { from: boss }) - executorApp = MockScriptExecutorApp.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) - // Explicitly don't initialize the executorApp - executionTarget = await ExecutionTarget.new() + context('> Returned bytes', () => { + beforeEach(async () => { + // Install mock executor onto registry + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE, boss, { from: boss }) + const receipt = await evmScriptReg.addScriptExecutor(scriptExecutorMock.address, { from: boss }) + + // Sanity check it's at spec ID 1 + const executorSpecId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId + assert.equal(executorSpecId, 1, 'CallsScript should be installed as spec ID 1') + }) + + it('properly returns if no bytes are returned from executor', async () => { + // Only executes executor with bytes for spec ID + const inputScript = createExecutorId(1) + const receipt = await scriptRunnerApp.runScript(inputScript) + + // Check logs + // The executor app always uses 0x for the input and the mock script executor should return + // an empty bytes array if only the spec ID is given + const scriptResult = receipt.logs.filter(l => l.event == 'ScriptResult')[0] + assert.equal(scriptResult.args.script, inputScript, 'should log the same script') + assert.equal(scriptResult.args.input, EMPTY_BYTES, 'should log the same input') + assert.equal(scriptResult.args.returnData, EMPTY_BYTES, 'should log the correct return data') + }) + + for (inputBytes of [ + soliditySha3('test').slice(2, 10), // bytes4 + soliditySha3('test').slice(2), // bytes32 + `${soliditySha3('test').slice(2)}${soliditySha3('test2').slice(2, 10)}`, // bytes36 + `${soliditySha3('test').slice(2)}${soliditySha3('test2').slice(2)}`, // bytes64 + ]) { + it(`properly returns bytes (length: ${inputBytes.length / 2}) from executor`, async () => { + const inputScript = `${createExecutorId(1)}${inputBytes}` + const receipt = await scriptRunnerApp.runScript(inputScript) + + // Check logs + // The executor app always uses 0x for the input and the mock script executor should return + // the full input script since it's more than just the spec ID + const scriptResult = receipt.logs.filter(l => l.event == 'ScriptResult')[0] + assert.equal(scriptResult.args.script, inputScript, 'should log the same script') + assert.equal(scriptResult.args.input, EMPTY_BYTES, 'should log the same input') + assert.equal(scriptResult.args.returnData, inputScript, 'should log the correct return data') }) + } + + it('properly allocates the free memory pointer after no bytes were returned from executor', async () => { + const inputScript = createExecutorId(1) + const receipt = await scriptRunnerApp.runScriptWithNewBytesAllocation(inputScript) + + // Check logs for returned bytes + const returnedBytes = receipt.logs.filter(l => l.event == 'ReturnedBytes')[0] + assert.equal(returnedBytes.args.returnedBytes, EMPTY_BYTES, 'should log the correct return data') + }) + + it('properly allocates the free memory pointer after returning bytes from executor', async () => { + const inputBytes = `${soliditySha3('test').slice(2)}${soliditySha3('mock').slice(2)}` + // Adjust to completely fill a 32-byte words (64 in this case: 4 + 32 + 32 - 4) + const inputScript = `${createExecutorId(1)}${inputBytes}`.slice(0, -8) + const receipt = await scriptRunnerApp.runScriptWithNewBytesAllocation(inputScript) + + // Check logs for returned bytes + const returnedBytes = receipt.logs.filter(l => l.event == 'ReturnedBytes')[0] + assert.equal(returnedBytes.args.returnedBytes, inputScript, 'should log the correct return data') + }) + }) - it('fails to execute any executor', async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScript([action]) + context('> Reverted script', () => { + beforeEach(async () => { + // Install mock reverting executor onto registry + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE, boss, { from: boss }) + const receipt = await evmScriptReg.addScriptExecutor(scriptExecutorRevertMock.address, { from: boss }) - await assertRevert(() => executorApp.execute(script)) - }) + // Sanity check it's at spec ID 1 + const executorSpecId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId + assert.equal(executorSpecId, 1, 'CallsScript should be installed as spec ID 1') + }) + + it('forwards the revert data correctly', async () => { + // Only executes executor with bytes for spec ID + const inputScript = createExecutorId(1) + + // Should revert and forward the script executor's revert message + assertRevert(scriptRunnerApp.runScript(inputScript), ERROR_MOCK_REVERT) + }) }) - context('> Executor', () => { - beforeEach(async () => { - const receipt = await dao.newAppInstance(executorAppId, executorAppBase.address, EMPTY_BYTES, false, { from: boss }) - executorApp = MockScriptExecutorApp.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) - await executorApp.initialize() - executionTarget = await ExecutionTarget.new() - }) + context('> CallsScript', () => { + let callsScriptBase, executionTarget - it('gets the correct executor registry from the app', async () => { - const appRegistry = await executorApp.getEVMScriptRegistry() - assert.equal(reg.address, appRegistry, 'app should return the same evm script registry') - }) + beforeEach(async () => { + callsScriptBase = await CallsScript.new() - it('fails to execute if spec ID is 0', async () => { - return assertRevert(async () => { - await executorApp.execute(createExecutorId(0)) - }) - }) + // Install CallsScript onto registry + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE, boss, { from: boss }) + const receipt = await evmScriptReg.addScriptExecutor(callsScriptBase.address, { from: boss }) - it('fails to execute if spec ID is unknown', async () => { - return assertRevert(async () => { - await executorApp.execute(createExecutorId(4)) - }) - }) + // Sanity check it's at spec ID 1 + const callsScriptExecutorId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId + assert.equal(callsScriptExecutorId, 1, 'CallsScript should be installed as spec ID 1') - context('> Spec ID 1', () => { - it('is the correct executor type', async () => { - const CALLS_SCRIPT_TYPE = soliditySha3('CALLS_SCRIPT') - const executor = IEVMScriptExecutor.at(await reg.getScriptExecutor(createExecutorId(1))) - assert.equal(await executor.executorType(), CALLS_SCRIPT_TYPE) - }) + executionTarget = await ExecutionTarget.new() + }) - it('gets the correct executor from the app', async () => { - const CALLS_SCRIPT_TYPE = soliditySha3('CALLS_SCRIPT') - const scriptId = createExecutorId(1) - const executor = await reg.getScriptExecutor(scriptId) + it('fails if directly calling calls script', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScript([action]) - const appExecutor = await executorApp.getEVMScriptExecutor(scriptId) - assert.equal(executor, appExecutor, 'app should return the same evm script executor') - }) + await assertRevert(callsScriptBase.execScript(script, EMPTY_BYTES, []), reverts.INIT_NOT_INITIALIZED) + }) - it('executes single action script', async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScript([action]) + it('is the correct executor type', async () => { + const CALLS_SCRIPT_TYPE = soliditySha3('CALLS_SCRIPT') + const executor = IEVMScriptExecutor.at(await evmScriptReg.getScriptExecutor(createExecutorId(1))) + assert.equal(await executor.executorType(), CALLS_SCRIPT_TYPE) + }) - const receipt = await executorApp.execute(script) + it('gets the correct executor from the app', async () => { + const script = createExecutorId(1) + const executor = await evmScriptReg.getScriptExecutor(script) - assert.equal(await executionTarget.counter(), 1, 'should have executed action') + const scriptExecutor = await scriptRunnerApp.getEVMScriptExecutor(script) + assert.equal(executor, scriptExecutor, 'app should return the same evm script executor') + }) - // Check logs - // The Executor always uses 0x for the input and callscripts always have 0x returns - const scriptResult = receipt.logs.filter(l => l.event == 'ScriptResult')[0] - assert.equal(scriptResult.args.executor, await reg.getScriptExecutor(createExecutorId(1)), 'should log the same executor') - assert.equal(scriptResult.args.script, script, 'should log the same script') - assert.equal(scriptResult.args.input, EMPTY_BYTES, 'should log the same input') - assert.equal(scriptResult.args.returnData, EMPTY_BYTES, 'should log the return data') - }) + it('executes single action script', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScript([action]) + + const receipt = await scriptRunnerApp.runScript(script) + + assert.equal(await executionTarget.counter(), 1, 'should have executed action') + + // Check logs + // The executor app always uses 0x for the input and the calls script always returns 0x output + const scriptResult = receipt.logs.filter(l => l.event == 'ScriptResult')[0] + assert.equal(scriptResult.args.executor, await evmScriptReg.getScriptExecutor(createExecutorId(1)), 'should log the same executor') + assert.equal(scriptResult.args.script, script, 'should log the same script') + assert.equal(scriptResult.args.input, EMPTY_BYTES, 'should log the same input') + assert.equal(scriptResult.args.returnData, EMPTY_BYTES, 'should log the empty return data') + }) + + it("can execute if call doesn't contain blacklist addresses", async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScript([action]) + + await scriptRunnerApp.runScriptWithBan(script, ['0x1234']) + + assert.equal(await executionTarget.counter(), 1, 'should have executed action') + }) - it('fails to execute if has blacklist addresses being called', async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScript([action]) + it('executes multi action script', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScript([action, action, action, action]) - return assertRevert(async () => { - await executorApp.executeWithBan(script, [executionTarget.address]) - }) - }) + await scriptRunnerApp.runScript(script) - it("can execute if call doesn't contain blacklist addresses", async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScript([action]) + assert.equal(await executionTarget.counter(), 4, 'should have executed multiple actions') + }) - await executorApp.executeWithBan(script, ['0x1234']) + it('executes multi action script to multiple addresses', async () => { + const executionTarget2 = await ExecutionTarget.new() - assert.equal(await executionTarget.counter(), 1, 'should have executed action') - }) + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const action2 = { to: executionTarget2.address, calldata: executionTarget2.contract.execute.getData() } - it('executes multi action script', async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScript([action, action, action, action]) + const script = encodeCallScript([action2, action, action2, action, action2]) - await executorApp.execute(script) + await scriptRunnerApp.runScript(script) - assert.equal(await executionTarget.counter(), 4, 'should have executed action') - }) + assert.equal(await executionTarget.counter(), 2, 'should have executed action') + assert.equal(await executionTarget2.counter(), 3, 'should have executed action') + }) - it('executes multi action script to multiple addresses', async () => { - const executionTarget2 = await ExecutionTarget.new() + it('executes with parameters', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.setCounter.getData(101) } + const script = encodeCallScript([action]) - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const action2 = { to: executionTarget2.address, calldata: executionTarget2.contract.execute.getData() } + await scriptRunnerApp.runScript(script) - const script = encodeCallScript([action2, action, action2, action, action2]) + assert.equal(await executionTarget.counter(), 101, 'should have set counter') + }) - await executorApp.execute(script) + it('execution fails if one call fails', async () => { + const action1 = { to: executionTarget.address, calldata: executionTarget.contract.setCounter.getData(101) } + const action2 = { to: executionTarget.address, calldata: executionTarget.contract.failExecute.getData(true) } - assert.equal(await executionTarget.counter(), 2, 'should have executed action') - assert.equal(await executionTarget2.counter(), 3, 'should have executed action') - }) + const script = encodeCallScript([action1, action2]) - it('executes with parameters', async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.setCounter.getData(101) } - const script = encodeCallScript([action]) + await assertRevert(scriptRunnerApp.runScript(script), ERROR_EXECUTION_TARGET) + }) - await executorApp.execute(script) + it('execution fails with correctly forwarded error data', async () => { + const action1 = { to: executionTarget.address, calldata: executionTarget.contract.failExecute.getData(true) } - assert.equal(await executionTarget.counter(), 101, 'should have set counter') - }) + const script = encodeCallScript([action1]) - it('execution fails if one call fails', async () => { - const action1 = { to: executionTarget.address, calldata: executionTarget.contract.setCounter.getData(101) } - const action2 = { to: executionTarget.address, calldata: executionTarget.contract.failExecute.getData() } + await assertRevert(scriptRunnerApp.runScript(script), ERROR_EXECUTION_TARGET) + }) - const script = encodeCallScript([action1, action2]) + it('execution fails with default error data if no error data is returned', async () => { + const action1 = { to: executionTarget.address, calldata: executionTarget.contract.failExecute.getData(false) } - return assertRevert(async () => { - await executorApp.execute(script) - }) - }) + const script = encodeCallScript([action1]) - it('can execute empty script', async () => { - await executorApp.execute(encodeCallScript([])) - }) + await assertRevert(scriptRunnerApp.runScript(script), reverts.EVMCALLS_CALL_REVERTED) + }) - context('> Script overflow', () => { - const encodeCallScriptBad = actions => { - return actions.reduce((script, { to, calldata }) => { - const addr = rawEncode(['address'], [to]).toString('hex') - // length should be (calldata.length - 2) / 2 instead of calldata.length - // as this one is bigger, it would overflow and therefore must revert - const length = rawEncode(['uint256'], [calldata.length]).toString('hex') + it('can execute empty script', async () => { + await scriptRunnerApp.runScript(encodeCallScript([])) + }) - // Remove 12 first 0s of padding for addr and 28 0s for uint32 - return script + addr.slice(24) + length.slice(56) + calldata.slice(2) - }, createExecutorId(1)) // spec 1 - } + it('fails to execute if has blacklist addresses being called', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScript([action]) - it('fails if data length is too big', async () => { - const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } - const script = encodeCallScriptBad([action]) + await assertRevert(scriptRunnerApp.runScriptWithBan(script, [executionTarget.address]), reverts.EVMCALLS_BLACKLISTED_CALL) + }) - return assertRevert(async () => { - await executorApp.execute(script) - }) - }) - }) + context('> Script underflow', () => { + const encodeCallScriptAddressUnderflow = actions => { + return actions.reduce((script, { to }) => { + const addr = rawEncode(['address'], [to]).toString('hex') - context('> Registry actions', () => { - it("can't be executed once disabled", async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - await reg.disableScriptExecutor(1, { from: boss }) + // Remove too much of the address (should just remove first 12 0s of padding as addr) + return script + addr.slice(24 + 4) + }, createExecutorId(1)) // spec 1 + } - return assertRevert(async () => { - await executorApp.execute(encodeCallScript([])) - }) - }) + const encodeCallScriptCalldataUnderflow = actions => { + return actions.reduce((script, { to, calldata }) => { + const addr = rawEncode(['address'], [to]).toString('hex') + const length = rawEncode(['uint256'], [calldata.length]).toString('hex') - it('can be re-enabled', async () => { - await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) + // Remove too much of the calldataLength (should just remove first 28 0s of padding as uint32) + return script + addr.slice(24) + length.slice(56 + 4) + }, createExecutorId(1)) // spec 1 + } - // Disable then re-enable the executor - await reg.disableScriptExecutor(1, { from: boss }) - await reg.enableScriptExecutor(1, { from: boss }) + it('fails if data length is too small to contain address', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScriptAddressUnderflow([action]) - await executorApp.execute(encodeCallScript([])) - }) - }) + // EVMScriptRunner doesn't pass through the revert yet + await assertRevert(scriptRunnerApp.runScript(script), reverts.EVMCALLS_INVALID_LENGTH) + }) + + it('fails if data length is too small to contain calldata', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScriptCalldataUnderflow([action]) + + // EVMScriptRunner doesn't pass through the revert yet + await assertRevert(scriptRunnerApp.runScript(script), reverts.EVMCALLS_INVALID_LENGTH) + }) + }) + + context('> Script overflow', () => { + const encodeCallScriptCalldataOverflow = actions => { + return actions.reduce((script, { to, calldata }) => { + const addr = rawEncode(['address'], [to]).toString('hex') + // length should be (calldata.length - 2) / 2 instead of calldata.length + // as this one is bigger, it would overflow and therefore must revert + const length = rawEncode(['uint256'], [calldata.length]).toString('hex') + + // Remove 12 first 0s of padding for addr and 28 0s for uint32 + return script + addr.slice(24) + length.slice(56) + calldata.slice(2) + }, createExecutorId(1)) // spec 1 + } + + it('fails if data length is too big', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScriptCalldataOverflow([action]) + + // EVMScriptRunner doesn't pass through the revert yet + await assertRevert(scriptRunnerApp.runScript(script), reverts.EVMCALLS_INVALID_LENGTH) }) + }) }) + }) }) diff --git a/test/evm_script_factory.js b/test/evm_script_factory.js new file mode 100644 index 000000000..8a9934d11 --- /dev/null +++ b/test/evm_script_factory.js @@ -0,0 +1,125 @@ +const { soliditySha3 } = require('web3-utils') +const { createExecutorId, encodeCallScript } = require('./helpers/evmScript') + +const Kernel = artifacts.require('Kernel') +const ACL = artifacts.require('ACL') +const EVMScriptRegistry = artifacts.require('EVMScriptRegistry') +const DAOFactory = artifacts.require('DAOFactory') +const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') + +// Mocks +const AppStubScriptRunner = artifacts.require('AppStubScriptRunner') +const ExecutionTarget = artifacts.require('ExecutionTarget') +const EVMScriptRegistryConstantsMock = artifacts.require('EVMScriptRegistryConstantsMock') + +const ZERO_ADDR = '0x0000000000000000000000000000000000000000' +const EMPTY_BYTES = '0x' + +contract('EVM Script Factory', accounts => { + let evmScriptRegBase, callsScriptBase + let daoFact, regFact, dao, acl, evmScriptReg + let APP_BASES_NAMESPACE, APP_ADDR_NAMESPACE, APP_MANAGER_ROLE, CREATE_PERMISSIONS_ROLE + let EVMSCRIPT_REGISTRY_APP_ID, REGISTRY_ADD_EXECUTOR_ROLE, REGISTRY_MANAGER_ROLE + + const permissionsRoot = accounts[0] + + const scriptRunnerAppId = '0x1234' + + before(async () => { + const kernelBase = await Kernel.new(true) // petrify immediately + const aclBase = await ACL.new() + + regFact = await EVMScriptRegistryFactory.new() + daoFact = await DAOFactory.new(kernelBase.address, aclBase.address, regFact.address) + callsScriptBase = await regFact.baseCallScript() + evmScriptRegBase = EVMScriptRegistry.at(await regFact.baseReg()) + const evmScriptRegConstants = await EVMScriptRegistryConstantsMock.new() + + APP_BASES_NAMESPACE = await kernelBase.APP_BASES_NAMESPACE() + APP_ADDR_NAMESPACE = await kernelBase.APP_ADDR_NAMESPACE() + APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE() + CREATE_PERMISSIONS_ROLE = await aclBase.CREATE_PERMISSIONS_ROLE() + + EVMSCRIPT_REGISTRY_APP_ID = await evmScriptRegConstants.getEVMScriptRegistryAppId() + REGISTRY_ADD_EXECUTOR_ROLE = await evmScriptRegBase.REGISTRY_ADD_EXECUTOR_ROLE() + REGISTRY_MANAGER_ROLE = await evmScriptRegBase.REGISTRY_MANAGER_ROLE() + }) + + beforeEach(async () => { + const receipt = await daoFact.newDAO(permissionsRoot) + dao = Kernel.at(receipt.logs.filter(l => l.event == 'DeployDAO')[0].args.dao) + evmScriptReg = EVMScriptRegistry.at(receipt.logs.filter(l => l.event == 'DeployEVMScriptRegistry')[0].args.reg) + + acl = ACL.at(await dao.acl()) + }) + + it('factory installed EVMScriptRegistry correctly', async () => { + assert.equal(await dao.getApp(APP_BASES_NAMESPACE, EVMSCRIPT_REGISTRY_APP_ID), evmScriptRegBase.address, 'should have set correct base EVMScriptRegistry app in kernel') + assert.equal(await dao.getApp(APP_ADDR_NAMESPACE, EVMSCRIPT_REGISTRY_APP_ID), evmScriptReg.address, 'should have set correct default EVMScriptRegistry app in kernel') + assert.isTrue(await evmScriptReg.hasInitialized(), "should be initialized") + }) + + it('factory registered just 1 script executor', async () => { + assert.equal(await evmScriptReg.getScriptExecutor(createExecutorId(0)), ZERO_ADDR, 'spec ID 0 should always be empty') + assert.notEqual(await evmScriptReg.getScriptExecutor(createExecutorId(1)), callsScriptBase.address, 'spec ID 1 should be calls script') + assert.equal(await evmScriptReg.getScriptExecutor(createExecutorId(2)), ZERO_ADDR, 'spec ID 2 and higher should be empty') + }) + + it('factory cleaned up permissions', async () => { + assert.isFalse(await acl.hasPermission(regFact.address, dao.address, APP_MANAGER_ROLE), 'EVMScriptRegistryFactory should not have APP_MANAGER_ROLE for created kernel') + assert.isFalse(await acl.hasPermission(regFact.address, acl.address, CREATE_PERMISSIONS_ROLE), 'EVMScriptRegistryFactory should not have CREATE_PERMISSIONS_ROLE for created ACL') + assert.isFalse(await acl.hasPermission(regFact.address, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE), 'EVMScriptRegistryFactory should not have REGISTRY_ADD_EXECUTOR_ROLE for created EVMScriptRegistry') + assert.isFalse(await acl.hasPermission(regFact.address, evmScriptReg.address, REGISTRY_MANAGER_ROLE), 'EVMScriptRegistryFactory should not have REGISTRY_MANAGER_ROLE for created EVMScriptRegistry') + + assert.equal(await acl.getPermissionManager(evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE), ZERO_ADDR, 'created EVMScriptRegistry should not have manager for REGISTRY_ADD_EXECUTOR_ROLE') + assert.equal(await acl.getPermissionManager(evmScriptReg.address, REGISTRY_MANAGER_ROLE), ZERO_ADDR, 'created EVMScriptRegistry should not have manager for REGISTRY_MANAGER_ROLE') + }) + + context('> Executor app', () => { + let scriptRunnerAppBase, scriptRunnerApp, executionTarget + + before(async () => { + scriptRunnerAppBase = await AppStubScriptRunner.new() + }) + + beforeEach(async () => { + // Set up app management permissions + await acl.createPermission(permissionsRoot, dao.address, APP_MANAGER_ROLE, permissionsRoot) + + const receipt = await dao.newAppInstance(scriptRunnerAppId, scriptRunnerAppBase.address, EMPTY_BYTES, false) + scriptRunnerApp = AppStubScriptRunner.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) + await scriptRunnerApp.initialize() + executionTarget = await ExecutionTarget.new() + }) + + it('gets the correct executor registry from the app', async () => { + const registryFromApp = await scriptRunnerApp.getEVMScriptRegistry() + assert.equal(evmScriptReg.address, registryFromApp, 'app should return the same EVMScriptRegistry') + }) + + it('gets the correct executor from the app', async () => { + const script = createExecutorId(1) + const executor = await evmScriptReg.getScriptExecutor(script) + + const scriptExecutor = await scriptRunnerApp.getEVMScriptExecutor(script) + assert.equal(executor, scriptExecutor, 'app should return the same evm script executor') + }) + + it('can execute calls script (spec ID 1)', async () => { + const action = { to: executionTarget.address, calldata: executionTarget.contract.execute.getData() } + const script = encodeCallScript([action], 1) + + const receipt = await scriptRunnerApp.runScript(script) + + assert.equal(await executionTarget.counter(), 1, 'should have executed action') + + // Check logs + // The executor always uses 0x for the input and callscripts always have 0x returns + const scriptResult = receipt.logs.filter(l => l.event == 'ScriptResult')[0] + assert.equal(scriptResult.args.executor, await evmScriptReg.getScriptExecutor(createExecutorId(1)), 'should log the same executor') + assert.equal(scriptResult.args.script, script, 'should log the same script') + assert.equal(scriptResult.args.input, EMPTY_BYTES, 'should log the same input') + assert.equal(scriptResult.args.returnData, EMPTY_BYTES, 'should log the return data') + }) + }) +}) diff --git a/test/helpers/assertThrow.js b/test/helpers/assertThrow.js index 94633e96a..f16d8a7fa 100644 --- a/test/helpers/assertThrow.js +++ b/test/helpers/assertThrow.js @@ -1,26 +1,38 @@ -function assertError(error, s, message) { - assert.isAbove(error.message.search(s), -1, `${message}\n\t(error: ${error})`); +const THROW_ERROR_PREFIX = 'VM Exception while processing transaction:' + +function assertError(error, expectedErrorCode) { + assert(error.message.search(expectedErrorCode) > -1, `Expected error code "${expectedErrorCode}" but failed with "${error}" instead.`) } -async function assertThrows(codeBlock, message, errorCode) { - try { - await codeBlock() - } catch (e) { - return assertError(e, errorCode, message) - } - assert.fail('should have thrown before') +async function assertThrows(blockOrPromise, expectedErrorCode, expectedReason) { + try { + (typeof blockOrPromise === 'function') ? await blockOrPromise() : await blockOrPromise + } catch (error) { + assertError(error, expectedErrorCode) + return error + } + // assert.fail() for some reason does not have its error string printed 🤷 + assert(0, `Expected "${expectedErrorCode}"${expectedReason ? ` (with reason: "${expectedReason}")` : ''} but it did not fail`) } module.exports = { - async assertJump(codeBlock, message = 'should have failed with invalid JUMP') { - return assertThrows(codeBlock, message, 'invalid JUMP') - }, + async assertJump(blockOrPromise) { + return assertThrows(blockOrPromise, 'invalid JUMP') + }, - async assertInvalidOpcode(codeBlock, message = 'should have failed with invalid opcode') { - return assertThrows(codeBlock, message, 'invalid opcode') - }, + async assertInvalidOpcode(blockOrPromise) { + return assertThrows(blockOrPromise, 'invalid opcode') + }, - async assertRevert(codeBlock, message = 'should have failed by reverting') { - return assertThrows(codeBlock, message, 'revert') - }, + async assertRevert(blockOrPromise, reason) { + const error = await assertThrows(blockOrPromise, 'revert', reason) + const errorPrefix = `${THROW_ERROR_PREFIX} revert` + if (error.message.includes(errorPrefix)) { + error.reason = error.message.replace(errorPrefix, '').trim() + } + + if (process.env.SOLIDITY_COVERAGE !== 'true' && reason) { + assert.equal(reason, error.reason, `Expected revert reason "${reason}" but failed with "${error.reason || 'no reason'}" instead.`) + } + }, } diff --git a/test/helpers/evmScript.js b/test/helpers/evmScript.js index 999718519..8b44f110b 100644 --- a/test/helpers/evmScript.js +++ b/test/helpers/evmScript.js @@ -1,16 +1,21 @@ const abi = require('ethereumjs-abi') +const createExecutorId = id => `0x${String(id).padStart(8, '0')}` + module.exports = { - // Encodes an array of actions ({ to: address, calldata: bytes}) into the EVM call script format - // Sets spec id 1 = 0x00000001 and - // Concatenates per call [ 20 bytes (address) ] + [ 4 bytes (uint32: calldata length) ] + [ calldataLength bytes (payload) ] - encodeCallScript: actions => { + createExecutorId, + + // Encodes an array of actions ({ to: address, calldata: bytes }) into the EVM call script format + // Sets spec id and concatenates per call + // [ 20 bytes (address) ] + [ 4 bytes (uint32: calldata length) ] + [ calldataLength bytes (payload) ] + // Defaults spec id to 1 + encodeCallScript: (actions, specId = 1) => { return actions.reduce((script, { to, calldata }) => { const addr = abi.rawEncode(['address'], [to]).toString('hex') const length = abi.rawEncode(['uint256'], [(calldata.length - 2) / 2]).toString('hex') // Remove 12 first 0s of padding for addr and 28 0s for uint32 return script + addr.slice(24) + length.slice(56) + calldata.slice(2) - }, '0x00000001') // spec 1 + }, createExecutorId(specId)) }, } diff --git a/test/helpers/revertStrings.js b/test/helpers/revertStrings.js new file mode 100644 index 000000000..0c8d440cb --- /dev/null +++ b/test/helpers/revertStrings.js @@ -0,0 +1,74 @@ +function makeErrorMappingProxy (target) { + return new Proxy(target, { + get (target, property) { + if (property in target) { + return target[property] + } + + throw new Error(`Could not find error ${property} in error mapping`) + }, + set () { + throw new Error('Unexpected set to error mapping') + } + }) +} + +module.exports = makeErrorMappingProxy({ + // acl/ACL.sol + ACL_AUTH_INIT_KERNEL: 'ACL_AUTH_INIT_KERNEL', + ACL_AUTH_NO_MANAGER: 'ACL_AUTH_NO_MANAGER', + ACL_EXISTENT_MANAGER: 'ACL_EXISTENT_MANAGER', + + // apm/APMRegistry.sol + APMREG_INIT_PERMISSIONS: 'APMREG_INIT_PERMISSIONS', + APMREG_EMPTY_NAME: 'APMREG_EMPTY_NAME', + + // apm/Repo.sol + REPO_INVALID_BUMP: 'REPO_INVALID_BUMP', + REPO_INVALID_VERSION: 'REPO_INVALID_VERSION', + REPO_INEXISTENT_VERSION: 'REPO_INEXISTENT_VERSION', + + // apps/AragonApp.sol + APP_AUTH_FAILED: 'APP_AUTH_FAILED', + + // common/Initializable.sol + INIT_ALREADY_INITIALIZED: 'INIT_ALREADY_INITIALIZED', + INIT_NOT_INITIALIZED: 'INIT_NOT_INITIALIZED', + + // common/SafeERC20.sol + SAFE_ERC_20_BALANCE_REVERTED: 'SAFE_ERC_20_BALANCE_REVERTED', + SAFE_ERC_20_ALLOWANCE_REVERTED: 'SAFE_ERC_20_ALLOWANCE_REVERTED', + + // common/Uint256Helpers.sol + UINT64_NUMBER_TOO_BIG: 'UINT64_NUMBER_TOO_BIG', + + // common/VaultRecoverable.sol + RECOVER_DISALLOWED: 'RECOVER_DISALLOWED', + RECOVER_VAULT_NOT_CONTRACT: 'RECOVER_VAULT_NOT_CONTRACT', + RECOVER_TOKEN_TRANSFER_FAILED: 'RECOVER_TOKEN_TRANSFER_FAILED', + + // ens/ENSSubdomainRegistrar.sol + ENSSUB_NO_NODE_OWNERSHIP: 'ENSSUB_NO_NODE_OWNERSHIP', + ENSSUB_NAME_EXISTS: 'ENSSUB_NAME_EXISTS', + ENSSUB_DOESNT_EXIST: 'ENSSUB_DOESNT_EXIST', + + // evmscript/EVMScriptRegistry.sol + EVMREG_INEXISTENT_EXECUTOR: 'EVMREG_INEXISTENT_EXECUTOR', + EVMREG_EXECUTOR_ENABLED: 'EVMREG_EXECUTOR_ENABLED', + EVMREG_EXECUTOR_DISABLED: 'EVMREG_EXECUTOR_DISABLED', + EVMREG_SCRIPT_LENGTH_TOO_SHORT: 'EVMREG_SCRIPT_LENGTH_TOO_SHORT', + + // evmscript/EVMScriptRunner.sol + EVMRUN_EXECUTOR_UNAVAILABLE: 'EVMRUN_EXECUTOR_UNAVAILABLE', + EVMRUN_PROTECTED_STATE_MODIFIED: 'EVMRUN_PROTECTED_STATE_MODIFIED', + + // evmscript/executors/CallsScript.sol + EVMCALLS_BLACKLISTED_CALL: 'EVMCALLS_BLACKLISTED_CALL', + EVMCALLS_INVALID_LENGTH: 'EVMCALLS_INVALID_LENGTH', + EVMCALLS_CALL_REVERTED: 'EVMCALLS_CALL_REVERTED', + + // kernel/Kernel.sol + KERNEL_APP_NOT_CONTRACT: 'KERNEL_APP_NOT_CONTRACT', + KERNEL_INVALID_APP_CHANGE: 'KERNEL_INVALID_APP_CHANGE', + KERNEL_AUTH_FAILED: 'KERNEL_AUTH_FAILED', +}) From 76addefdf2b55fd4d7d57ae81e4be697f4ab9866 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Fri, 5 Apr 2019 13:38:13 +0200 Subject: [PATCH 09/15] chore: upgrade ganache-cli to 6.4.2 (#504) --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 1ee5b1680..9460d6466 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,7 @@ "eth-ens-namehash": "^2.0.8", "eth-gas-reporter": "^0.1.1", "ethereumjs-abi": "^0.6.5", - "ganache-cli": "6.2.3", + "ganache-cli": "^6.4.2", "mocha-lcov-reporter": "^1.3.0", "solidity-coverage": "0.5.8", "solium": "^1.2.3", From bef014183d09f7e9f3fb2fc45ba4064eed74cfc6 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Tue, 9 Apr 2019 10:39:12 +0200 Subject: [PATCH 10/15] test: fix test parameterization variable binding (#506) --- contracts/test/TestConversionHelpers.sol | 4 ++-- test/app_funds.js | 4 ++-- test/evm_script.js | 2 +- test/kernel_acl.js | 8 ++++---- test/recovery_to_vault.js | 12 +++--------- test/safe_erc20.js | 2 +- 6 files changed, 13 insertions(+), 19 deletions(-) diff --git a/contracts/test/TestConversionHelpers.sol b/contracts/test/TestConversionHelpers.sol index 802a6ee79..15b044471 100644 --- a/contracts/test/TestConversionHelpers.sol +++ b/contracts/test/TestConversionHelpers.sol @@ -129,13 +129,13 @@ contract TestConversionHelpers { Assert.equal(arrMemLoc, arrReconvertedMemLoc, "should have same memory location after reconverting"); } - function assertValues(uint256[] memory _data) { + function assertValues(uint256[] memory _data) public { Assert.equal(_data[0], FIRST, "should have correct index value at 0"); Assert.equal(_data[1], SECOND, "should have correct index value at 1"); Assert.equal(_data[2], THIRD, "should have correct index value at 2"); } - function assertValues(bytes memory _data) { + function assertValues(bytes memory _data) public { uint256 first; uint256 second; uint256 third; diff --git a/test/app_funds.js b/test/app_funds.js index a65579f4b..152c984a5 100644 --- a/test/app_funds.js +++ b/test/app_funds.js @@ -33,7 +33,7 @@ contract('App funds', accounts => { APP_BASES_NAMESPACE = await kernelBase.APP_BASES_NAMESPACE() }) - const appBases = [ + const appBaseGroups = [ { base: AppStub, unsafeBase: UnsafeAppStub, @@ -43,7 +43,7 @@ contract('App funds', accounts => { unsafeBase: UnsafeAppStubDepositable, } ] - for ({ base: appBaseType, unsafeBase: unsafeAppBaseType } of appBases) { + for (const { base: appBaseType, unsafeBase: unsafeAppBaseType } of appBaseGroups) { context(`> ${appBaseType.contractName}`, () => { const onlyAppStubDepositable = onlyIf(() => appBaseType === AppStubDepositable) diff --git a/test/evm_script.js b/test/evm_script.js index b88cb0f2f..b24ab5929 100644 --- a/test/evm_script.js +++ b/test/evm_script.js @@ -268,7 +268,7 @@ contract('EVM Script', accounts => { assert.equal(scriptResult.args.returnData, EMPTY_BYTES, 'should log the correct return data') }) - for (inputBytes of [ + for (const inputBytes of [ soliditySha3('test').slice(2, 10), // bytes4 soliditySha3('test').slice(2), // bytes32 `${soliditySha3('test').slice(2)}${soliditySha3('test2').slice(2, 10)}`, // bytes36 diff --git a/test/kernel_acl.js b/test/kernel_acl.js index 9b5274f61..73e1bcb37 100644 --- a/test/kernel_acl.js +++ b/test/kernel_acl.js @@ -155,20 +155,20 @@ contract('Kernel ACL', accounts => { ) // Allows setting code for namespace other than 0 - for (grantee of [child, secondChild]) { + for (const grantee of [child, secondChild]) { const receipt = await kernel.setApp('0x121212', APP_ID, appBase.address, { from: grantee }) assertEvent(receipt, 'SetApp') } // Fail if setting code for namespace 0 - for (grantee of [child, secondChild]) { + for (const grantee of [child, secondChild]) { await assertRevert(async () => { await kernel.setApp('0x00', APP_ID, appBase.address, { from: grantee }) }) } // Fail if setting code for empty namespace (which becomes 0) - for (grantee of [child, secondChild]) { + for (const grantee of [child, secondChild]) { await assertRevert(async () => { await kernel.setApp(EMPTY_BYTES, APP_ID, appBase.address, { from: grantee }) }) @@ -181,7 +181,7 @@ contract('Kernel ACL', accounts => { assertEvent(receipt, 'SetPermissionParams', 0) // should not have emitted this // Any entity can succesfully perform action - for (granteeIndex of [4, 5, 6]) { + for (const granteeIndex of [4, 5, 6]) { const grantee = accounts[granteeIndex] assert.isTrue(await acl.hasPermission(grantee, kernelAddr, APP_MANAGER_ROLE), `account[${granteeIndex}] should have perm`) const setReceipt = await kernel.setApp('0x121212', APP_ID, appBase.address, { from: grantee }) diff --git a/test/recovery_to_vault.js b/test/recovery_to_vault.js index a0c936172..5fbb823a9 100644 --- a/test/recovery_to_vault.js +++ b/test/recovery_to_vault.js @@ -207,7 +207,7 @@ contract('Recovery to vault', accounts => { ) ) - for ({ title, tokenContract} of tokenTestGroups) { + for (const { title, tokenContract } of tokenTestGroups) { it(`kernel recovers ${title}`, async () => { await recoverTokens({ tokenContract, @@ -215,9 +215,7 @@ contract('Recovery to vault', accounts => { target: kernel }) }) - } - for ({ title, tokenContract} of tokenTestGroups) { it(`kernel reverts on failing recovery for ${title}`, async () => { await failingRecoverTokens({ tokenContract, @@ -274,7 +272,7 @@ contract('Recovery to vault', accounts => { await recoverEth({ target, vault }) )) - for ({ title, tokenContract} of tokenTestGroups) { + for (const { title, tokenContract } of tokenTestGroups) { it(`recovers ${title}`, async () => { await recoverTokens({ tokenContract, @@ -282,9 +280,7 @@ contract('Recovery to vault', accounts => { vault, }) }) - } - for ({ title, tokenContract} of tokenTestGroups) { it(`reverts on failing recovery for ${title}`, async () => { await failingRecoverTokens({ tokenContract, @@ -315,7 +311,7 @@ contract('Recovery to vault', accounts => { await recoverEth({ target, vault, shouldFail: true }) )) - for ({ title, tokenContract} of tokenTestGroups) { + for (const { title, tokenContract } of tokenTestGroups) { it(`allows recovers ${title}`, async () => { await recoverTokens({ tokenContract, @@ -323,9 +319,7 @@ contract('Recovery to vault', accounts => { vault, }) }) - } - for ({ title, tokenContract} of tokenTestGroups) { it(`reverts on failing recovery for ${title}`, async () => { await failingRecoverTokens({ tokenContract, diff --git a/test/safe_erc20.js b/test/safe_erc20.js index 7996f091e..60b173c20 100644 --- a/test/safe_erc20.js +++ b/test/safe_erc20.js @@ -36,7 +36,7 @@ contract('SafeERC20', accounts => { }, ] - for ({ title, tokenContract} of testGroups) { + for (const { title, tokenContract } of testGroups) { context(`> ${title}`, () => { let tokenMock From 28ad69d4b92414018f087cadd48a8f3e280c5b7a Mon Sep 17 00:00:00 2001 From: Facu Spagnuolo Date: Tue, 9 Apr 2019 23:20:42 -0300 Subject: [PATCH 11/15] Fix TimeHelpers relative path for Initializable (#507) --- contracts/common/Initializable.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/common/Initializable.sol b/contracts/common/Initializable.sol index ca015a6f6..d267ab2f1 100644 --- a/contracts/common/Initializable.sol +++ b/contracts/common/Initializable.sol @@ -4,8 +4,8 @@ pragma solidity ^0.4.24; +import "./TimeHelpers.sol"; import "./UnstructuredStorage.sol"; -import "../common/TimeHelpers.sol"; contract Initializable is TimeHelpers { From b6823b5f0d536c262f2045b87381ada092557b8e Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Sun, 14 Apr 2019 21:28:31 +0200 Subject: [PATCH 12/15] ConversionHelpers: check length on bytes to uint256[] (#509) There are publicly exposed interfaces that expect `bytes` and immediately turn them into `uint256[]` (e.g. `hasPermission()` in the ACL and Kernel. There might be some cases where the truncation could lead to Bad ThingsTM, like the ACL being tricked into thinking a contract had permission to do something when it actually didn't. We never use the `bytes` form of `hasPermission()` directly ourselves, so this isn't exploitable, but could be if an external contract decided to. --- contracts/common/ConversionHelpers.sol | 4 ++++ contracts/test/TestConversionHelpers.sol | 29 ++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/contracts/common/ConversionHelpers.sol b/contracts/common/ConversionHelpers.sol index e574a634b..3b7cd9d76 100644 --- a/contracts/common/ConversionHelpers.sol +++ b/contracts/common/ConversionHelpers.sol @@ -2,6 +2,8 @@ pragma solidity ^0.4.24; library ConversionHelpers { + string private constant ERROR_IMPROPER_LENGTH = "CONVERSION_IMPROPER_LENGTH"; + function dangerouslyCastUintArrayToBytes(uint256[] memory _input) internal pure returns (bytes memory output) { // Force cast the uint256[] into a bytes array, by overwriting its length // Note that the bytes array doesn't need to be initialized as we immediately overwrite it @@ -18,6 +20,8 @@ library ConversionHelpers { // Note that the uint256[] doesn't need to be initialized as we immediately overwrite it // with the input and a new length. The input becomes invalid from this point forward. uint256 intsLength = _input.length / 32; + require(_input.length == intsLength * 32, ERROR_IMPROPER_LENGTH); + assembly { output := _input mstore(output, intsLength) diff --git a/contracts/test/TestConversionHelpers.sol b/contracts/test/TestConversionHelpers.sol index 15b044471..8593c5684 100644 --- a/contracts/test/TestConversionHelpers.sol +++ b/contracts/test/TestConversionHelpers.sol @@ -1,9 +1,21 @@ pragma solidity 0.4.24; import "./helpers/Assert.sol"; +import "./helpers/ThrowProxy.sol"; + import "../common/ConversionHelpers.sol"; +contract InvalidBytesLengthConversionThrows { + function tryConvertLength(uint256 _badLength) public { + bytes memory arr = new bytes(_badLength); + + // Do failing conversion + uint256[] memory arrUint = ConversionHelpers.dangerouslyCastBytesToUintArray(arr); + } +} + + contract TestConversionHelpers { uint256 constant internal FIRST = uint256(keccak256("0")); uint256 constant internal SECOND = uint256(keccak256("1")); @@ -129,6 +141,23 @@ contract TestConversionHelpers { Assert.equal(arrMemLoc, arrReconvertedMemLoc, "should have same memory location after reconverting"); } + function testBytesConversionThrowsOnInvalidLength() public { + InvalidBytesLengthConversionThrows thrower = new InvalidBytesLengthConversionThrows(); + ThrowProxy throwProxy = new ThrowProxy(address(thrower)); + + InvalidBytesLengthConversionThrows(throwProxy).tryConvertLength(15); + throwProxy.assertThrows("should have reverted due to invalid length"); + + InvalidBytesLengthConversionThrows(throwProxy).tryConvertLength(36); + throwProxy.assertThrows("should have reverted due to invalid length"); + + InvalidBytesLengthConversionThrows(throwProxy).tryConvertLength(61); + throwProxy.assertThrows("should have reverted due to invalid length"); + + InvalidBytesLengthConversionThrows(throwProxy).tryConvertLength(128); + throwProxy.assertItDoesntThrow("should not have reverted as length was valid"); + } + function assertValues(uint256[] memory _data) public { Assert.equal(_data[0], FIRST, "should have correct index value at 0"); Assert.equal(_data[1], SECOND, "should have correct index value at 1"); From 009e0f50775fb3620608d490ddf2fdee4006d309 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Sun, 14 Apr 2019 21:28:58 +0200 Subject: [PATCH 13/15] EVMScriptRunner: handle no return data from executors (#508) `EVMScriptRunner` was previously assuming its executor would always return correctly ABI-encoded data of type `bytes`, which are at least 64 bytes in length (32 bytes for position and 32 bytes for actual data). Although an underflow here would've simply caused an out-of-gas error on trying to copy too much memory, reverting with an error message is much more friendly. --- contracts/evmscript/EVMScriptRunner.sol | 45 +++++++++++++------ .../mocks/EVMScriptExecutorNoReturnMock.sol | 18 ++++++++ test/evm_script.js | 28 ++++++++++-- test/helpers/revertStrings.js | 1 + 4 files changed, 76 insertions(+), 16 deletions(-) create mode 100644 contracts/test/mocks/EVMScriptExecutorNoReturnMock.sol diff --git a/contracts/evmscript/EVMScriptRunner.sol b/contracts/evmscript/EVMScriptRunner.sol index 28970632c..576dfa1f1 100644 --- a/contracts/evmscript/EVMScriptRunner.sol +++ b/contracts/evmscript/EVMScriptRunner.sol @@ -16,6 +16,10 @@ contract EVMScriptRunner is AppStorage, Initializable, EVMScriptRegistryConstant string private constant ERROR_EXECUTOR_UNAVAILABLE = "EVMRUN_EXECUTOR_UNAVAILABLE"; string private constant ERROR_PROTECTED_STATE_MODIFIED = "EVMRUN_PROTECTED_STATE_MODIFIED"; + /* This is manually crafted in assembly + string private constant ERROR_EXECUTOR_INVALID_RETURN = "EVMRUN_EXECUTOR_INVALID_RETURN"; + */ + event ScriptResult(address indexed executor, bytes script, bytes input, bytes returnData); function getEVMScriptExecutor(bytes _script) public view returns (IEVMScriptExecutor) { @@ -59,19 +63,34 @@ contract EVMScriptRunner is AppStorage, Initializable, EVMScriptRegistryConstant revert(output, returndatasize) } default { - // Copy result - // - // Needs to perform an ABI decode for the expected `bytes` return type of - // `executor.execScript()` as solidity will automatically ABI encode the returned bytes as: - // [ position of the first dynamic length return value = 0x20 (32 bytes) ] - // [ output length (32 bytes) ] - // [ output content (N bytes) ] - // - // Perform the ABI decode by ignoring the first 32 bytes of the return data - let copysize := sub(returndatasize, 0x20) - returndatacopy(output, 0x20, copysize) - - mstore(0x40, add(output, copysize)) // free mem ptr set + switch gt(returndatasize, 0x3f) + case 0 { + // Need at least 0x40 bytes returned for properly ABI-encoded bytes values, + // revert with "EVMRUN_EXECUTOR_INVALID_RETURN" + // See remix: doing a `revert("EVMRUN_EXECUTOR_INVALID_RETURN")` always results in + // this memory layout + mstore(output, 0x08c379a000000000000000000000000000000000000000000000000000000000) // error identifier + mstore(add(output, 0x04), 0x0000000000000000000000000000000000000000000000000000000000000020) // starting offset + mstore(add(output, 0x24), 0x000000000000000000000000000000000000000000000000000000000000001e) // reason length + mstore(add(output, 0x44), 0x45564d52554e5f4558454355544f525f494e56414c49445f52455455524e0000) // reason + + revert(output, 100) // 100 = 4 + 3 * 32 (error identifier + 3 words for the ABI encoded error) + } + default { + // Copy result + // + // Needs to perform an ABI decode for the expected `bytes` return type of + // `executor.execScript()` as solidity will automatically ABI encode the returned bytes as: + // [ position of the first dynamic length return value = 0x20 (32 bytes) ] + // [ output length (32 bytes) ] + // [ output content (N bytes) ] + // + // Perform the ABI decode by ignoring the first 32 bytes of the return data + let copysize := sub(returndatasize, 0x20) + returndatacopy(output, 0x20, copysize) + + mstore(0x40, add(output, copysize)) // free mem ptr set + } } } diff --git a/contracts/test/mocks/EVMScriptExecutorNoReturnMock.sol b/contracts/test/mocks/EVMScriptExecutorNoReturnMock.sol new file mode 100644 index 000000000..ecf84cd6b --- /dev/null +++ b/contracts/test/mocks/EVMScriptExecutorNoReturnMock.sol @@ -0,0 +1,18 @@ +pragma solidity 0.4.24; + + +import "../../evmscript/executors/BaseEVMScriptExecutor.sol"; + +contract EVMScriptExecutorNoReturnMock is BaseEVMScriptExecutor { + bytes32 internal constant EXECUTOR_TYPE = keccak256("NO_RETURN_SCRIPT"); + + function execScript(bytes, bytes, address[]) external isInitialized returns (bytes) { + assembly { + stop + } + } + + function executorType() external pure returns (bytes32) { + return EXECUTOR_TYPE; + } +} diff --git a/test/evm_script.js b/test/evm_script.js index b24ab5929..19fb34fc3 100644 --- a/test/evm_script.js +++ b/test/evm_script.js @@ -17,6 +17,7 @@ const IEVMScriptExecutor = artifacts.require('IEVMScriptExecutor') const AppStubScriptRunner = artifacts.require('AppStubScriptRunner') const ExecutionTarget = artifacts.require('ExecutionTarget') const EVMScriptExecutorMock = artifacts.require('EVMScriptExecutorMock') +const EVMScriptExecutorNoReturnMock = artifacts.require('EVMScriptExecutorNoReturnMock') const EVMScriptExecutorRevertMock = artifacts.require('EVMScriptExecutorRevertMock') const EVMScriptRegistryConstantsMock = artifacts.require('EVMScriptRegistryConstantsMock') @@ -24,7 +25,8 @@ const ZERO_ADDR = '0x0000000000000000000000000000000000000000' const EMPTY_BYTES = '0x' contract('EVM Script', accounts => { - let kernelBase, aclBase, evmScriptRegBase, dao, acl, evmScriptReg, scriptExecutorMock, scriptExecutorRevertMock + let kernelBase, aclBase, evmScriptRegBase, dao, acl, evmScriptReg + let scriptExecutorMock, scriptExecutorNoReturnMock, scriptExecutorRevertMock let APP_BASES_NAMESPACE, APP_ADDR_NAMESPACE, APP_MANAGER_ROLE let EVMSCRIPT_REGISTRY_APP_ID, REGISTRY_ADD_EXECUTOR_ROLE, REGISTRY_MANAGER_ROLE let ERROR_MOCK_REVERT, ERROR_EXECUTION_TARGET @@ -38,6 +40,7 @@ contract('EVM Script', accounts => { aclBase = await ACL.new() evmScriptRegBase = await EVMScriptRegistry.new() scriptExecutorMock = await EVMScriptExecutorMock.new() + scriptExecutorNoReturnMock = await EVMScriptExecutorNoReturnMock.new() scriptExecutorRevertMock = await EVMScriptExecutorRevertMock.new() const evmScriptRegConstants = await EVMScriptRegistryConstantsMock.new() const executionTarget = await ExecutionTarget.new() @@ -251,7 +254,7 @@ contract('EVM Script', accounts => { // Sanity check it's at spec ID 1 const executorSpecId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId - assert.equal(executorSpecId, 1, 'CallsScript should be installed as spec ID 1') + assert.equal(executorSpecId, 1, 'EVMScriptExecutorMock should be installed as spec ID 1') }) it('properly returns if no bytes are returned from executor', async () => { @@ -309,6 +312,25 @@ contract('EVM Script', accounts => { }) }) + context('> No return from script', () => { + beforeEach(async () => { + // Install mock executor onto registry + await acl.createPermission(boss, evmScriptReg.address, REGISTRY_ADD_EXECUTOR_ROLE, boss, { from: boss }) + const receipt = await evmScriptReg.addScriptExecutor(scriptExecutorNoReturnMock.address, { from: boss }) + + // Sanity check it's at spec ID 1 + const executorSpecId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId + assert.equal(executorSpecId, 1, 'EVMScriptExecutorNoReturnMock should be installed as spec ID 1') + }) + + it('fails if not return data is returned for ABI-encoded bytes', async () => { + const inputScript = createExecutorId(1) + + // Should revert with invalid return + assertRevert(scriptRunnerApp.runScript(inputScript), reverts.EVMRUN_EXECUTOR_INVALID_RETURN) + }) + }) + context('> Reverted script', () => { beforeEach(async () => { // Install mock reverting executor onto registry @@ -317,7 +339,7 @@ contract('EVM Script', accounts => { // Sanity check it's at spec ID 1 const executorSpecId = receipt.logs.filter(l => l.event == 'EnableExecutor')[0].args.executorId - assert.equal(executorSpecId, 1, 'CallsScript should be installed as spec ID 1') + assert.equal(executorSpecId, 1, 'EVMScriptExecutorRevertMock should be installed as spec ID 1') }) it('forwards the revert data correctly', async () => { diff --git a/test/helpers/revertStrings.js b/test/helpers/revertStrings.js index 0c8d440cb..d8c805b66 100644 --- a/test/helpers/revertStrings.js +++ b/test/helpers/revertStrings.js @@ -61,6 +61,7 @@ module.exports = makeErrorMappingProxy({ // evmscript/EVMScriptRunner.sol EVMRUN_EXECUTOR_UNAVAILABLE: 'EVMRUN_EXECUTOR_UNAVAILABLE', EVMRUN_PROTECTED_STATE_MODIFIED: 'EVMRUN_PROTECTED_STATE_MODIFIED', + EVMRUN_EXECUTOR_INVALID_RETURN: 'EVMRUN_EXECUTOR_INVALID_RETURN', // evmscript/executors/CallsScript.sol EVMCALLS_BLACKLISTED_CALL: 'EVMCALLS_BLACKLISTED_CALL', From d23e28fc3562c4f55022fe259a636665b3a1003f Mon Sep 17 00:00:00 2001 From: Jorge Izquierdo Date: Mon, 15 Apr 2019 12:59:49 +0200 Subject: [PATCH 14/15] chore: allow to skip deploying Kernel and ACL bases in deploy-daofactory (#511) Needed for the Aragon 0.7 re-deployment of kits, which will have an upgraded `DAOFactory` but keeping the bases for `ACL` and `Kernel` of the current deployment. --- scripts/deploy-daofactory.js | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/scripts/deploy-daofactory.js b/scripts/deploy-daofactory.js index 99f8aadc9..f3bfa9829 100644 --- a/scripts/deploy-daofactory.js +++ b/scripts/deploy-daofactory.js @@ -4,24 +4,45 @@ const globalArtifacts = this.artifacts // Not injected unless called directly vi const ZERO_ADDR = '0x0000000000000000000000000000000000000000' +const defaultKernelBase = process.env.KERNEL_BASE +const defaultAclBaseAddress = process.env.ACL_BASE + module.exports = async ( truffleExecCallback, { artifacts = globalArtifacts, + kernelBaseAddress = defaultKernelBase, + aclBaseAddress = defaultAclBaseAddress, withEvmScriptRegistryFactory = true, verbose = true } = {} ) => { + const log = (...args) => { + if (verbose) { console.log(...args) } + } + const ACL = artifacts.require('ACL') const Kernel = artifacts.require('Kernel') const DAOFactory = artifacts.require('DAOFactory') - const kernelBase = await Kernel.new(true) // immediately petrify - await logDeploy(kernelBase, { verbose }) + let kernelBase + if (kernelBaseAddress) { + kernelBase = Kernel.at(kernelBaseAddress) + log(`Skipping deploying new Kernel base, using provided address: ${kernelBaseAddress}`) + } else { + kernelBase = await Kernel.new(true) // immediately petrify + await logDeploy(kernelBase, { verbose }) + } - const aclBase = await ACL.new() - await logDeploy(aclBase, { verbose }) + let aclBase + if (aclBaseAddress) { + aclBase = ACL.at(aclBaseAddress) + log(`Skipping deploying new ACL base, using provided address: ${aclBaseAddress}`) + } else { + aclBase = await ACL.new() + await logDeploy(aclBase, { verbose }) + } let evmScriptRegistryFactory if (withEvmScriptRegistryFactory) { From 3fe48d6c460ac366a6284501c4d1a3b336ac3b55 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Mon, 15 Apr 2019 13:08:43 +0200 Subject: [PATCH 15/15] 4.2.0 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 9460d6466..f6a3218ed 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@aragon/os", - "version": "4.1.0", + "version": "4.2.0", "description": "Core contracts for Aragon", "scripts": { "compile": "truffle compile",