diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index 032f556fb..6ac8a57ec 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -42,8 +42,6 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { address public constant ANY_ENTITY = address(-1); address public constant BURN_ENTITY = address(1); // address(0) is already used as "no permission manager" - uint256 internal constant ORACLE_CHECK_GAS = 30000; - string private constant ERROR_AUTH_INIT_KERNEL = "ACL_AUTH_INIT_KERNEL"; string private constant ERROR_AUTH_NO_MANAGER = "ACL_AUTH_NO_MANAGER"; string private constant ERROR_EXISTENT_MANAGER = "ACL_EXISTENT_MANAGER"; @@ -421,11 +419,13 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { // a raw call is required so we can return false if the call reverts, rather than reverting bytes memory checkCalldata = abi.encodeWithSelector(sig, _who, _where, _what, _how); - uint256 oracleCheckGas = ORACLE_CHECK_GAS; bool ok; assembly { - ok := staticcall(oracleCheckGas, _oracleAddr, add(checkCalldata, 0x20), mload(checkCalldata), 0, 0) + // send all available gas; if the oracle eats up all the gas, we will eventually revert + // note that we are currently guaranteed to still have some gas after the call from + // EIP-150's 63/64 gas forward rule + ok := staticcall(gas, _oracleAddr, add(checkCalldata, 0x20), mload(checkCalldata), 0, 0) } if (!ok) { diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index f5683c0bf..d27f4aa0f 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/acl/IACL.sol b/contracts/acl/IACL.sol index cf2825113..3a304bfe3 100644 --- a/contracts/acl/IACL.sol +++ b/contracts/acl/IACL.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/acl/IACLOracle.sol b/contracts/acl/IACLOracle.sol index 802537f08..e837d3a36 100644 --- a/contracts/acl/IACLOracle.sol +++ b/contracts/acl/IACLOracle.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/apm/APMNamehash.sol b/contracts/apm/APMNamehash.sol index fda755f10..69f0d0e82 100644 --- a/contracts/apm/APMNamehash.sol +++ b/contracts/apm/APMNamehash.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/apps/AppStorage.sol b/contracts/apps/AppStorage.sol index b37dd363d..07f601d64 100644 --- a/contracts/apps/AppStorage.sol +++ b/contracts/apps/AppStorage.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/apps/AragonApp.sol b/contracts/apps/AragonApp.sol index f53c40721..b31cce7e6 100644 --- a/contracts/apps/AragonApp.sol +++ b/contracts/apps/AragonApp.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/apps/UnsafeAragonApp.sol b/contracts/apps/UnsafeAragonApp.sol index 7e46b7a65..032571ebc 100644 --- a/contracts/apps/UnsafeAragonApp.sol +++ b/contracts/apps/UnsafeAragonApp.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/common/Autopetrified.sol b/contracts/common/Autopetrified.sol index 1224319d5..0004f354a 100644 --- a/contracts/common/Autopetrified.sol +++ b/contracts/common/Autopetrified.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/common/EtherTokenConstant.sol b/contracts/common/EtherTokenConstant.sol index 63a512bfd..81c4e0544 100644 --- a/contracts/common/EtherTokenConstant.sol +++ b/contracts/common/EtherTokenConstant.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/common/IForwarder.sol b/contracts/common/IForwarder.sol index 5876f465d..a54b8cfdc 100644 --- a/contracts/common/IForwarder.sol +++ b/contracts/common/IForwarder.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/common/IForwarderFee.sol b/contracts/common/IForwarderFee.sol index 80913fe64..0fecb3418 100644 --- a/contracts/common/IForwarderFee.sol +++ b/contracts/common/IForwarderFee.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/common/IVaultRecoverable.sol b/contracts/common/IVaultRecoverable.sol index 3c8c3676c..8d0e284d7 100644 --- a/contracts/common/IVaultRecoverable.sol +++ b/contracts/common/IVaultRecoverable.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/common/Initializable.sol b/contracts/common/Initializable.sol index d267ab2f1..ba20d88f5 100644 --- a/contracts/common/Initializable.sol +++ b/contracts/common/Initializable.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/common/IsContract.sol b/contracts/common/IsContract.sol index 9c5d412b6..531ce47af 100644 --- a/contracts/common/IsContract.sol +++ b/contracts/common/IsContract.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/common/Petrifiable.sol b/contracts/common/Petrifiable.sol index bd7803e96..bb534d21a 100644 --- a/contracts/common/Petrifiable.sol +++ b/contracts/common/Petrifiable.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/common/ReentrancyGuard.sol b/contracts/common/ReentrancyGuard.sol index 0d761f2ad..b2de8485c 100644 --- a/contracts/common/ReentrancyGuard.sol +++ b/contracts/common/ReentrancyGuard.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/common/TimeHelpers.sol b/contracts/common/TimeHelpers.sol index cb9993331..92ec1f8c3 100644 --- a/contracts/common/TimeHelpers.sol +++ b/contracts/common/TimeHelpers.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/common/UnstructuredStorage.sol b/contracts/common/UnstructuredStorage.sol index db0a56d98..a2312331d 100644 --- a/contracts/common/UnstructuredStorage.sol +++ b/contracts/common/UnstructuredStorage.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/common/VaultRecoverable.sol b/contracts/common/VaultRecoverable.sol index 74aacd41d..e9bf2b17e 100644 --- a/contracts/common/VaultRecoverable.sol +++ b/contracts/common/VaultRecoverable.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/ens/ENSConstants.sol b/contracts/ens/ENSConstants.sol index 5069408bd..bcbe0d982 100644 --- a/contracts/ens/ENSConstants.sol +++ b/contracts/ens/ENSConstants.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/evmscript/EVMScriptRunner.sol b/contracts/evmscript/EVMScriptRunner.sol index 576dfa1f1..c2f5a5d86 100644 --- a/contracts/evmscript/EVMScriptRunner.sol +++ b/contracts/evmscript/EVMScriptRunner.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/evmscript/IEVMScriptExecutor.sol b/contracts/evmscript/IEVMScriptExecutor.sol index ffa036824..d3269889c 100644 --- a/contracts/evmscript/IEVMScriptExecutor.sol +++ b/contracts/evmscript/IEVMScriptExecutor.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/evmscript/IEVMScriptRegistry.sol b/contracts/evmscript/IEVMScriptRegistry.sol index d6cfddbcb..6e4ed0f16 100644 --- a/contracts/evmscript/IEVMScriptRegistry.sol +++ b/contracts/evmscript/IEVMScriptRegistry.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/evmscript/ScriptHelpers.sol b/contracts/evmscript/ScriptHelpers.sol index bdf26d32d..aee2379f0 100644 --- a/contracts/evmscript/ScriptHelpers.sol +++ b/contracts/evmscript/ScriptHelpers.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/evmscript/executors/BaseEVMScriptExecutor.sol b/contracts/evmscript/executors/BaseEVMScriptExecutor.sol index 6802c1098..ff78d8cbe 100644 --- a/contracts/evmscript/executors/BaseEVMScriptExecutor.sol +++ b/contracts/evmscript/executors/BaseEVMScriptExecutor.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/kernel/IKernel.sol b/contracts/kernel/IKernel.sol index e1a2b40e5..1686e7711 100644 --- a/contracts/kernel/IKernel.sol +++ b/contracts/kernel/IKernel.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/kernel/KernelConstants.sol b/contracts/kernel/KernelConstants.sol index 77816a74c..3406dab45 100644 --- a/contracts/kernel/KernelConstants.sol +++ b/contracts/kernel/KernelConstants.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/lib/misc/ERCProxy.sol b/contracts/lib/misc/ERCProxy.sol index 43115876e..e7ddc1fae 100644 --- a/contracts/lib/misc/ERCProxy.sol +++ b/contracts/lib/misc/ERCProxy.sol @@ -1,5 +1,5 @@ /* - * SPDX-License-Identitifer: MIT + * SPDX-License-Identifier: MIT */ pragma solidity ^0.4.24; diff --git a/contracts/test/helpers/ACLHelper.sol b/contracts/test/helpers/ACLHelper.sol index 404978572..265d6a84d 100644 --- a/contracts/test/helpers/ACLHelper.sol +++ b/contracts/test/helpers/ACLHelper.sol @@ -34,6 +34,12 @@ contract RevertOracle is IACLOracle { } } +contract AssertOracle is IACLOracle { + function canPerform(address, address, bytes32, uint256[]) external view returns (bool) { + assert(false); + } +} + // Can't implement from IACLOracle as its canPerform() is marked as view-only contract StateModifyingOracle /* is IACLOracle */ { bool modifyState; @@ -57,3 +63,14 @@ contract ConditionalOracle is IACLOracle { return how[0] > 0; } } + +contract OverGasLimitOracle is IACLOracle { + function canPerform(address, address, bytes32, uint256[]) external view returns (bool) { + while (true) { + // Do an SLOAD to increase the per-loop gas costs + uint256 loadFromStorage; + assembly { loadFromStorage := sload(0) } + } + return true; + } +} diff --git a/contracts/test/tests/TestACLInterpreter.sol b/contracts/test/tests/TestACLInterpreter.sol index b4441d2a4..05bb9275c 100644 --- a/contracts/test/tests/TestACLInterpreter.sol +++ b/contracts/test/tests/TestACLInterpreter.sol @@ -82,8 +82,6 @@ contract TestACLInterpreter is ACL, ACLHelper { // doesn't revert even if oracle reverts assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(new RevertOracle()), false); - // the staticcall will error as the oracle tries to modify state, so a no is returned - assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(new StateModifyingOracle()), false); // if returned data size is not correct, returns false assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(new EmptyDataReturnOracle()), false); diff --git a/package.json b/package.json index 2c02b0ef1..a98d57de5 100644 --- a/package.json +++ b/package.json @@ -35,25 +35,24 @@ "license": "(GPL-3.0-or-later OR MIT)", "devDependencies": { "@codechecks/client": "^0.1.5", - "coveralls": "^2.13.3", + "coveralls": "^3.0.9", "eth-ens-namehash": "^2.0.8", - "eth-gas-reporter": "^0.2.9", + "eth-gas-reporter": "^0.2.14", "ethereumjs-abi": "^0.6.5", - "ganache-cli": "^6.4.2", + "ganache-cli": "^6.9.0", "mocha-lcov-reporter": "^1.3.0", "solidity-coverage": "0.6.2", "solium": "^1.2.3", "truffle": "4.1.14", "truffle-bytecode-manager": "^1.1.1", "truffle-extract": "^1.2.1", - "web3-eth-abi": "1.0.0-beta.33", - "web3-utils": "1.0.0-beta.33" + "web3-eth-abi": "1.2.5", + "web3-utils": "1.2.5" }, "dependencies": { + "@aragon/truffle-config-v4": "^1.0.1", "mkdirp": "^0.5.1", "truffle-flattener": "^1.2.9", - "homedir": "^0.6.0", - "truffle-hdwallet-provider": "0.0.3", - "truffle-hdwallet-provider-privkey": "0.3.0" + "homedir": "^0.6.0" } } diff --git a/scripts/deploy-daofactory.js b/scripts/deploy-daofactory.js index f3bfa9829..c613bb8a7 100644 --- a/scripts/deploy-daofactory.js +++ b/scripts/deploy-daofactory.js @@ -6,6 +6,7 @@ const ZERO_ADDR = '0x0000000000000000000000000000000000000000' const defaultKernelBase = process.env.KERNEL_BASE const defaultAclBaseAddress = process.env.ACL_BASE +const defaultEvmScriptRegistryFactoryAddress = process.env.EVM_REG_FACTORY module.exports = async ( truffleExecCallback, @@ -13,6 +14,7 @@ module.exports = async ( artifacts = globalArtifacts, kernelBaseAddress = defaultKernelBase, aclBaseAddress = defaultAclBaseAddress, + evmScriptRegistryFactoryAddress = defaultEvmScriptRegistryFactoryAddress, withEvmScriptRegistryFactory = true, verbose = true } = {} @@ -47,8 +49,14 @@ module.exports = async ( let evmScriptRegistryFactory if (withEvmScriptRegistryFactory) { const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') - evmScriptRegistryFactory = await EVMScriptRegistryFactory.new() - await logDeploy(evmScriptRegistryFactory, { verbose }) + + if (evmScriptRegistryFactoryAddress) { + evmScriptRegistryFactory = EVMScriptRegistryFactory.at(evmScriptRegistryFactoryAddress) + log(`Skipping deploying new EVMScriptRegistryFactory, using provided address: ${evmScriptRegistryFactoryAddress}`) + } else { + evmScriptRegistryFactory = await EVMScriptRegistryFactory.new() + await logDeploy(evmScriptRegistryFactory, { verbose }) + } } const daoFactory = await DAOFactory.new( kernelBase.address, diff --git a/test/contracts/acl/acl_params.js b/test/contracts/acl/acl_params.js new file mode 100644 index 000000000..f9d1cc96b --- /dev/null +++ b/test/contracts/acl/acl_params.js @@ -0,0 +1,206 @@ +const { assertRevert } = require('../../helpers/assertThrow') +const { skipSuiteCoverage } = require('../../helpers/coverage') +const { permissionParamEqOracle } = require('../../helpers/permissionParams') + +const ACL = artifacts.require('ACL') +const Kernel = artifacts.require('Kernel') +const KernelProxy = artifacts.require('KernelProxy') + +const AcceptOracle = artifacts.require('AcceptOracle') +const RejectOracle = artifacts.require('RejectOracle') +const RevertOracle = artifacts.require('RevertOracle') +const AssertOracle = artifacts.require('AssertOracle') +const OverGasLimitOracle = artifacts.require('OverGasLimitOracle') +const StateModifyingOracle = artifacts.require('StateModifyingOracle') + +const ANY_ADDR = '0xffffffffffffffffffffffffffffffffffffffff' +const MAX_GAS_AVAILABLE = 6900000 + +const getExpectedGas = gas => gas * 63 / 64 + +contract('ACL params', ([permissionsRoot, specificEntity, noPermission, mockAppAddress]) => { + let aclBase, kernelBase, acl, kernel + const MOCK_APP_ROLE = "0xAB" + + before(async () => { + kernelBase = await Kernel.new(true) // petrify immediately + aclBase = await ACL.new() + }) + + beforeEach(async () => { + kernel = Kernel.at((await KernelProxy.new(kernelBase.address)).address) + await kernel.initialize(aclBase.address, permissionsRoot) + acl = ACL.at(await kernel.acl()) + await acl.createPermission(permissionsRoot, mockAppAddress, MOCK_APP_ROLE, permissionsRoot) + }) + + // More complex cases are checked via the solidity test in TestAclInterpreter.sol + context('> ACL Oracle', () => { + let aclParams + + const testOraclePermissions = ({ shouldHavePermission }) => { + const allowText = shouldHavePermission ? 'allows' : 'disallows' + + describe('when permission is set for ANY_ADDR', () => { + beforeEach(async () => { + await acl.grantPermissionP(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, [aclParams]) + }) + + it(`ACL ${allowText} actions for ANY_ADDR`, async () => { + assertion = shouldHavePermission ? assert.isTrue : assert.isFalse + assertion(await acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE)) + }) + + it(`ACL ${allowText} actions for specific address`, async () => { + assertion = shouldHavePermission ? assert.isTrue : assert.isFalse + assertion(await acl.hasPermission(specificEntity, mockAppAddress, MOCK_APP_ROLE)) + }) + }) + + describe('when permission is set for specific address', async () => { + beforeEach(async () => { + await acl.grantPermissionP(specificEntity, mockAppAddress, MOCK_APP_ROLE, [aclParams]) + }) + + it(`ACL ${allowText} actions for specific address`, async () => { + assertion = shouldHavePermission ? assert.isTrue : assert.isFalse + assertion(await acl.hasPermission(specificEntity, mockAppAddress, MOCK_APP_ROLE)) + }) + + it('ACL disallows actions for other addresses', async () => { + assert.isFalse(await acl.hasPermission(noPermission, mockAppAddress, MOCK_APP_ROLE)) + assert.isFalse(await acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE)) + }) + }) + } + + describe('when the oracle accepts', () => { + let acceptOracle + + before(async () => { + acceptOracle = await AcceptOracle.new() + aclParams = permissionParamEqOracle(acceptOracle.address) + }) + + testOraclePermissions({ shouldHavePermission: true }) + }) + + for (let [description, FailingOracle] of [ + ['rejects', RejectOracle], + ['reverts', RevertOracle], + ]) { + describe(`when the oracle ${description}`, () => { + let failingOracle + + before(async () => { + failingOracle = await FailingOracle.new() + aclParams = permissionParamEqOracle(failingOracle.address) + }) + + testOraclePermissions({ shouldHavePermission: false }) + }) + } + + describe('when the oracle modifies state', () => { + let stateModifyingOracle + + before(async () => { + stateModifyingOracle = await StateModifyingOracle.new() + aclParams = permissionParamEqOracle(stateModifyingOracle.address) + }) + + testOraclePermissions({ shouldHavePermission: false }) + }) + + // Both the assert and oog gas cases should be similar, since assert should eat all the + // available gas + for (let [description, FailingOutOfGasOracle] of [ + ['asserts', AssertOracle], + ['uses all available gas', OverGasLimitOracle], + ]) { + skipSuiteCoverage(describe)(`when the oracle ${description}`, () => { + let overGasLimitOracle + + before(async () => { + overGasLimitOracle = await FailingOutOfGasOracle.new() + aclParams = permissionParamEqOracle(overGasLimitOracle.address) + }) + + testOraclePermissions({ shouldHavePermission: false }) + + describe('gas', () => { + describe('when permission is set for ANY_ADDR', () => { + // Note `evalParams()` is called twice when calling `hasPermission` for `ANY_ADDR`, so + // gas costs are much, much higher to compensate for the 63/64th rule on the second call + const MEDIUM_GAS = 3000000 + const LOW_GAS = 2900000 + + beforeEach(async () => { + await acl.grantPermissionP(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, [aclParams]) + }) + + it('ACL disallows and uses all gas when given large amount of gas', async () => { + assert.isFalse(await acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, { gas: MAX_GAS_AVAILABLE })) + + const hasPermissionTxHash = await acl.hasPermission.sendTransaction(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, { gas: MAX_GAS_AVAILABLE }) + const hasPermissionGasConsumed = web3.eth.getTransactionReceipt(hasPermissionTxHash).gasUsed + // Surprisingly, the actual gas used is quite a lot lower than expected, but it is + // unclear if this is a ganache issue or if there are gas refunds we're not taking + // into account + assert.closeTo(hasPermissionGasConsumed, getExpectedGas(MAX_GAS_AVAILABLE), 105000) + }) + + it('ACL disallows and uses all gas when given medium amount of gas', async () => { + assert.isFalse(await acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, { gas: MEDIUM_GAS })) + + const hasPermissionTxHash = await acl.hasPermission.sendTransaction(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, { gas: MEDIUM_GAS }) + const hasPermissionGasConsumed = web3.eth.getTransactionReceipt(hasPermissionTxHash).gasUsed + assert.closeTo(hasPermissionGasConsumed, getExpectedGas(MEDIUM_GAS), 10000) + }) + + it('ACL reverts when given small amount of gas', async () => { + await assertRevert(acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, { gas: LOW_GAS })) + }) + }) + + describe('when permission is set for specific address', async () => { + const MEDIUM_GAS = 190000 + // Note that these gas values are still quite high for causing reverts in "low gas" + // situations, as we incur some overhead with delegating into proxies and other checks. + // Assuming we incur 40-60k gas overhead for this, we only have ~140,000 gas left. + // After the oracle call, we only have 140,000 / 64 ~= 2000 gas left, which begins to + // quick run out with SLOADs. + const LOW_GAS = 180000 + + beforeEach(async () => { + await acl.grantPermissionP(specificEntity, mockAppAddress, MOCK_APP_ROLE, [aclParams]) + }) + + it('ACL disallows and uses all gas when given large amount of gas', async () => { + assert.isFalse(await acl.hasPermission(specificEntity, mockAppAddress, MOCK_APP_ROLE, { gas: MAX_GAS_AVAILABLE })) + + const hasPermissionTxHash = await acl.hasPermission.sendTransaction(specificEntity, mockAppAddress, MOCK_APP_ROLE, { gas: MAX_GAS_AVAILABLE }) + const hasPermissionGasConsumed = web3.eth.getTransactionReceipt(hasPermissionTxHash).gasUsed + // Surprisingly, the actual gas used is quite a lot lower than expected, but it is + // unclear if this is a ganache issue or if there are gas refunds we're not taking + // into account + assert.closeTo(hasPermissionGasConsumed, getExpectedGas(MAX_GAS_AVAILABLE), 105000) + }) + + it('ACL disallows and uses all gas when given medium amount of gas', async () => { + assert.isFalse(await acl.hasPermission(specificEntity, mockAppAddress, MOCK_APP_ROLE, { gas: MEDIUM_GAS })) + + const hasPermissionTxHash = await acl.hasPermission.sendTransaction(specificEntity, mockAppAddress, MOCK_APP_ROLE, { gas: MEDIUM_GAS }) + const hasPermissionGasConsumed = web3.eth.getTransactionReceipt(hasPermissionTxHash).gasUsed + assert.closeTo(hasPermissionGasConsumed, getExpectedGas(MEDIUM_GAS), 10000) + }) + + it('ACL reverts when given small amount of gas', async () => { + await assertRevert(acl.hasPermission(specificEntity, mockAppAddress, MOCK_APP_ROLE, { gas: LOW_GAS })) + }) + }) + }) + }) + } + }) +}) diff --git a/test/contracts/common/depositable_delegate_proxy.js b/test/contracts/common/depositable_delegate_proxy.js index c05231b3a..07ce0537e 100644 --- a/test/contracts/common/depositable_delegate_proxy.js +++ b/test/contracts/common/depositable_delegate_proxy.js @@ -1,5 +1,6 @@ const { toChecksumAddress } = require('web3-utils') const { assertAmountOfEvents, assertEvent } = require('../../helpers/assertEvent')(web3) +const { skipCoverage } = require('../../helpers/coverage') const { decodeEventsOfType } = require('../../helpers/decodeEvent') const { assertRevert, assertOutOfGas } = require('../../helpers/assertThrow') const { getBalance } = require('../../helpers/web3') @@ -15,7 +16,6 @@ const SEND_ETH_GAS = TX_BASE_GAS + 9999 // <10k gas is the threshold for deposit const PROXY_FORWARD_GAS = TX_BASE_GAS + 2e6 // high gas amount to ensure that the proxy forwards the call const FALLBACK_SETUP_GAS = process.env.SOLIDITY_COVERAGE ? 5000 : 100 // rough estimation of how much gas it spends before executing the fallback code const SOLIDITY_TRANSFER_GAS = 2300 -const ISTANBUL_SLOAD_GAS_INCREASE = 600 contract('DepositableDelegateProxy', ([ sender ]) => { let ethSender, proxy, target, proxyTargetWithoutFallbackBase, proxyTargetWithFallbackBase @@ -117,29 +117,21 @@ contract('DepositableDelegateProxy', ([ sender ]) => { } } - it('can receive ETH (Constantinople)', async () => { + it('can receive ETH', async () => { const { gasUsed } = await assertSendEthToProxy({ value, gas: SEND_ETH_GAS }) - console.log('Used gas:', gasUsed - TX_BASE_GAS) }) - // TODO: Remove when the targetted EVM has been upgraded to Istanbul (EIP-1884) - it('can receive ETH (Istanbul, EIP-1884)', async () => { - const gas = TX_BASE_GAS + SOLIDITY_TRANSFER_GAS - ISTANBUL_SLOAD_GAS_INCREASE - const { gasUsed } = await assertSendEthToProxy({ value, gas }) - const gasUsedIstanbul = gasUsed - TX_BASE_GAS + ISTANBUL_SLOAD_GAS_INCREASE - console.log('Used gas (Istanbul):', gasUsedIstanbul) - - assert.isBelow(gasUsedIstanbul, 2300, 'Gas cost under Istanbul cannot be above 2300 gas') - }) - - // TODO: Remove when the targetted EVM has been upgraded to Istanbul (EIP-1884) - it('cannot receive ETH if sent with a small amount of gas', async () => { - // solidity-coverage seems to be increasing the gas amount to prevent failures - const oogDecrease = process.env.SOLIDITY_COVERAGE ? 600 : 250 - // deposit cannot be done with this amount of gas - const gas = TX_BASE_GAS + SOLIDITY_TRANSFER_GAS - ISTANBUL_SLOAD_GAS_INCREASE - oogDecrease - await assertSendEthToProxy({ shouldOOG: true, value, gas }) - }) + it( + 'cannot receive ETH if sent with a small amount of gas', + // Our version of solidity-coverage is not on an istanbul context yet + // TODO: update solidity-coverage + skipCoverage(async () => { + const oogDecrease = 250 + // deposit cannot be done with this amount of gas + const gas = TX_BASE_GAS + SOLIDITY_TRANSFER_GAS - oogDecrease + await assertSendEthToProxy({ shouldOOG: true, value, gas }) + }) + ) it('can receive ETH from contract', async () => { const { tx } = await ethSender.sendEth(proxy.address, { value }) @@ -170,4 +162,4 @@ contract('DepositableDelegateProxy', ([ sender ]) => { itForwardsToImplementationIfGasIsOverThreshold() }) }) -}) \ No newline at end of file +}) diff --git a/test/helpers/coverage.js b/test/helpers/coverage.js new file mode 100644 index 000000000..346f5b667 --- /dev/null +++ b/test/helpers/coverage.js @@ -0,0 +1,20 @@ +const skipCoverage = test => { + // Required dynamic this binding to attach onto the running test + return function skipCoverage() { + if (process.env.SOLIDITY_COVERAGE === 'true') { + this.skip() + } else { + return test() + } + } +} + +// For some reason, adding skipCoverage() to `before()`s were not working +const skipSuiteCoverage = suite => { + return process.env.SOLIDITY_COVERAGE === 'true' ? suite.skip : suite +} + +module.exports = { + skipCoverage, + skipSuiteCoverage, +} diff --git a/test/helpers/permissionParams.js b/test/helpers/permissionParams.js new file mode 100644 index 000000000..92ca90193 --- /dev/null +++ b/test/helpers/permissionParams.js @@ -0,0 +1,11 @@ +const permissionParamEqOracle = (oracleAddress) => { + // Set role such that the Oracle canPerform() function is used to determine the permission + const argId = '0xCB' // arg 203 - Oracle ID + const op = '01' // equal + const value = oracleAddress.slice(2).padStart(60, 0) // 60 as params are uint240 + return new web3.BigNumber(`${argId}${op}${value}`) +} + +module.exports = { + permissionParamEqOracle +} diff --git a/truffle-config.js b/truffle-config.js index 6831f9a6c..31f7ad982 100644 --- a/truffle-config.js +++ b/truffle-config.js @@ -1,117 +1 @@ -const homedir = require('homedir') -const path = require('path') - -const HDWalletProvider = require('truffle-hdwallet-provider') -const HDWalletProviderPrivkey = require('truffle-hdwallet-provider-privkey') - -const DEFAULT_MNEMONIC = 'stumble story behind hurt patient ball whisper art swift tongue ice alien' - -const defaultRPC = (network) => - `https://${network}.infura.io` - -const configFilePath = (filename) => - path.join(homedir(), `.aragon/${filename}`) - -const mnemonic = () => { - try { - return require(configFilePath('mnemonic.json')).mnemonic - } catch (e) { - return DEFAULT_MNEMONIC - } -} - -const settingsForNetwork = (network) => { - try { - return require(configFilePath(`${network}_key.json`)) - } catch (e) { - return { } - } -} - -// Lazily loaded provider -const providerForNetwork = (network) => ( - () => { - let { rpc, keys } = settingsForNetwork(network) - - rpc = rpc || defaultRPC(network) - - if (!keys || keys.length == 0) { - return new HDWalletProvider(mnemonic(), rpc) - } - - return new HDWalletProviderPrivkey(keys, rpc) - } -) - -const mochaGasSettings = { - reporter: 'eth-gas-reporter', - reporterOptions : { - currency: 'USD', - gasPrice: 3 - } -} - -const mocha = process.env.GAS_REPORTER ? mochaGasSettings : {} - -module.exports = { - networks: { - rpc: { - network_id: 15, - host: 'localhost', - port: 8545, - gas: 6.9e6, - gasPrice: 15000000001 - }, - devnet: { - network_id: 16, - host: 'localhost', - port: 8535, - gas: 6.9e6, - gasPrice: 15000000001 - }, - mainnet: { - network_id: 1, - provider: providerForNetwork('mainnet'), - gas: 7.9e6, - gasPrice: 3000000001 - }, - ropsten: { - network_id: 3, - provider: providerForNetwork('ropsten'), - gas: 4.712e6 - }, - rinkeby: { - network_id: 4, - provider: providerForNetwork('rinkeby'), - gas: 6.9e6, - gasPrice: 15000000001 - }, - kovan: { - network_id: 42, - provider: providerForNetwork('kovan'), - gas: 6.9e6 - }, - coverage: { - host: "localhost", - network_id: "*", - port: 8555, - gas: 0xffffffffff, - gasPrice: 0x01 - }, - development: { - host: 'localhost', - network_id: '*', - port: 8545, - gas: 6.9e6, - gasPrice: 15000000001 - }, - }, - build: {}, - mocha, - solc: { - optimizer: { - enabled: true, - runs: 10000 - } - }, -} +module.exports = require('@aragon/truffle-config-v4')