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
@@ -421,14 +422,18 @@ 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)
+
+        if (gasleft() > ERROR_GAS_ALLOWANCE) {
+            uint256 oracleCheckGas = gasleft() - ERROR_GAS_ALLOWANCE;
+            assembly {
+                ok := staticcall(oracleCheckGas, _oracleAddr, add(checkCalldata, 0x20), mload(checkCalldata), 0, 0)
+            }
         }
 
         if (!ok) {
+            require(gasleft() > ERROR_GAS_ALLOWANCE, ERROR_ORACLE_OOG);
             return false;
         }
 

contracts/test/helpers/ACLHelper.sol

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

contracts/test/tests/TestACLInterpreter.sol

@@ -80,10 +80,6 @@ contract TestACLInterpreter is ACL, ACLHelper {
         assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(new RejectOracle()), false);
         assertEval(arr(), ORACLE_PARAM_ID, Op.NEQ, uint256(new RejectOracle()), true);
 
-        // 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);
 
@@ -94,6 +90,16 @@ contract TestACLInterpreter is ACL, ACLHelper {
         assertEval(arr(uint256(0), uint256(1)), ORACLE_PARAM_ID, Op.EQ, uint256(conditionalOracle), false);
     }
 
+    function testRevertOracle() public {
+        // doesn't revert even if oracle reverts
+        assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(new RevertOracle()), false);
+    }
+
+    function testStateModifyingOracle() public {
+        // 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);
+    }
+
     function testReturn() public {
         assertEval(arr(), PARAM_VALUE_PARAM_ID, Op.RET, uint256(1), true);
         assertEval(arr(), PARAM_VALUE_PARAM_ID, Op.RET, uint256(0), false);

test/contracts/acl/acl_params.js

@@ -0,0 +1,42 @@
+const { assertRevert } = require('../../helpers/assertThrow')
+
+const ACL = artifacts.require('ACL')
+const Kernel = artifacts.require('Kernel')
+const KernelProxy = artifacts.require('KernelProxy')
+const OverGasLimitOracle = artifacts.require('OverGasLimitOracle')
+
+const ANY_ADDR = '0xffffffffffffffffffffffffffffffffffffffff'
+
+contract('ACL', ([permissionsRoot, mockAppAddress]) => {
+  let aclBase, kernelBase, acl, kernel, overGasLimitOracle
+  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())
+    overGasLimitOracle = await OverGasLimitOracle.new()
+  })
+
+  context('> ACLOracle Permission', () => {
+
+    it('fails when oracle canPerform goes OOG', async () => {
+      // 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${overGasLimitOracle.address.slice(2)}`  // oracle address
+      const param = new web3.BigNumber(`${argId}${op}${value}`)
+
+      await acl.createPermission(permissionsRoot, mockAppAddress, MOCK_APP_ROLE, permissionsRoot)
+      await acl.grantPermissionP(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, [param])
+
+      await assertRevert(acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE), "ACL_ORACLE_OOG")
+    })
+
+  })
+})