ACL: remove ACLOracle canPerform() gas limit (#565)

contracts/acl/ACL.sol

@@ -41,8 +41,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";
@@ -430,11 +428,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) {

contracts/test/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);
 

contracts/test/TestDelegateProxy.sol

@@ -69,6 +69,9 @@ contract TestDelegateProxy is DelegateProxy {
 
     // keep as last test as it will kill this contract
     function testDieIfMinReturn0() public {
+        Assert.isTrue(true, ''); // Make at least one assertion to satisfy the runner
+
         delegatedFwd(target, target.die.selector.toBytes());
+        Assert.fail('should be dead');
     }
 }

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;
+    }
+}

test/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 }))
+            })
+          })
+        })
+      })
+    }
+  })
+})

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,
 }

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
+}