ACL: remove ACLOracle canPerform() gas limit

contracts/acl/ACL.sol

@@ -42,11 +42,12 @@ 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;
+    uint256 internal constant ERROR_GAS_ALLOWANCE = 50000;
 
     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";
+    string private constant ERROR_ORACLE_OOG = "ACL_ORACLE_OOG";
 
     // Whether someone has a permission
     mapping (bytes32 => bytes32) internal permissions; // permissions hash => params hash
@@ -417,18 +418,21 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers {
     }
 
     function checkOracle(IACLOracle _oracleAddr, address _who, address _where, bytes32 _what, uint256[] _how) internal view returns (bool) {
-        bytes4 sig = _oracleAddr.canPerform.selector;
-
         // 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;
+        bytes memory checkCalldata = abi.encodeWithSelector(_oracleAddr.canPerform.selector, _who, _where, _what, _how);
 
         bool ok;
-        assembly {
-            ok := staticcall(oracleCheckGas, _oracleAddr, add(checkCalldata, 0x20), mload(checkCalldata), 0, 0)
+        uint256 gasLeftBeforeCall = gasleft();
+
+        if (gasLeftBeforeCall > ERROR_GAS_ALLOWANCE) {
+            uint256 oracleCanPerformGas = gasLeftBeforeCall - ERROR_GAS_ALLOWANCE;
+            assembly {
+                ok := staticcall(oracleCanPerformGas, _oracleAddr, add(checkCalldata, 0x20), mload(checkCalldata), 0, 0)
+            }
         }
 
         if (!ok) {
+            require(gasleft() > max(gasLeftBeforeCall / 64, ERROR_GAS_ALLOWANCE), ERROR_ORACLE_OOG);
             return false;
         }
 
@@ -449,6 +453,10 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers {
         return result;
     }
 
+    function max(uint256 _a, uint256 _b) internal pure returns (uint256) {
+        return _a > _b ? _a : _b;
+    }
+
     /**
     * @dev Internal function that sets management
     */

contracts/test/helpers/ACLHelper.sol

@@ -57,3 +57,14 @@ contract ConditionalOracle is IACLOracle {
         return how[0] > 0;
     }
 }
+
+contract OverGasLimitOracle is IACLOracle {
+    uint256 storageVar;
+
+    function canPerform(address, address, bytes32, uint256[]) external view returns (bool) {
+        while (true) {
+            uint256 memoryVar = storageVar;
+        }
+        return true;
+    }
+}

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);
 

test/contracts/acl/acl_params.js

@@ -0,0 +1,63 @@
+const { assertRevert } = require('../../helpers/assertThrow')
+const { paramForOracle } = require('../../helpers/permissionParams')
+
+const ACL = artifacts.require('ACL')
+const Kernel = artifacts.require('Kernel')
+const KernelProxy = artifacts.require('KernelProxy')
+const AcceptOracle = artifacts.require('AcceptOracle')
+const OverGasLimitOracle = artifacts.require('OverGasLimitOracle')
+const StateModifyingOracle = artifacts.require('StateModifyingOracle')
+
+const ANY_ADDR = '0xffffffffffffffffffffffffffffffffffffffff'
+
+contract('ACL', ([permissionsRoot, 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)
+  })
+
+  it('ACLOracle succeeds when oracle canPerform returns true', async () => {
+    const acceptOracle = await AcceptOracle.new()
+    const param = paramForOracle(acceptOracle.address)
+    await acl.grantPermissionP(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, [param])
+    assert.isTrue(await acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE))
+  })
+
+  it('ACLOracle fails when oracle canPerform modifies state', async () => {
+    const stateModifyingOracle = await StateModifyingOracle.new()
+    const param = paramForOracle(stateModifyingOracle.address)
+    await acl.grantPermissionP(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, [param])
+    await assertRevert(acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE), "ACL_ORACLE_OOG")
+  })
+
+  context('> ACLOracle OverGasLimitOracle', () => {
+    let overGasLimitOracle, param
+
+    beforeEach(async () => {
+      overGasLimitOracle = await OverGasLimitOracle.new()
+      param = paramForOracle(overGasLimitOracle.address)
+    })
+
+    // Note `evalParams()` is called twice when calling `hasPermission` for `ANY_ADDR`
+    it('fails when oracle canPerform goes OOG', async () => {
+      await acl.grantPermissionP(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, [param])
+      await assertRevert(acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE), "ACL_ORACLE_OOG")
+    })
+
+    // Note `evalParams()` is only called once when calling `hasPermission` for a specific address
+    it('fails when oracle canPerform goes OOG with specified permission owner', async () => {
+      await acl.grantPermissionP(permissionsRoot, mockAppAddress, MOCK_APP_ROLE, [param])
+      await assertRevert(acl.hasPermission(permissionsRoot, mockAppAddress, MOCK_APP_ROLE), "ACL_ORACLE_OOG")
+    })
+  })
+})

test/helpers/permissionParams.js

@@ -0,0 +1,12 @@
+
+const paramForOracle = (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 = `00000000000000000000${oracleAddress.slice(2)}`
+  return new web3.BigNumber(`${argId}${op}${value}`)
+}
+
+module.exports = {
+  paramForOracle
+}