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/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/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/helpers/coverage.js b/test/helpers/coverage.js index f5f313242..346f5b667 100644 --- a/test/helpers/coverage.js +++ b/test/helpers/coverage.js @@ -9,6 +9,12 @@ const skipCoverage = 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 + 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 +}