Skip to content

Commit c50c3ca

Browse files
willjgriffwilljgriff
authored
ACL: remove ACLOracle canPerform() gas limit (#565)
1 parent fb93dee commit c50c3ca

File tree

6 files changed

+245
-7
lines changed

6 files changed

+245
-7
lines changed

‎contracts/acl/ACL.sol‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,6 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers {
4242
address public constant ANY_ENTITY = address(-1);
4343
address public constant BURN_ENTITY = address(1); // address(0) is already used as "no permission manager"
4444

45-
uint256 internal constant ORACLE_CHECK_GAS = 30000;
46-
4745
string private constant ERROR_AUTH_INIT_KERNEL = "ACL_AUTH_INIT_KERNEL";
4846
string private constant ERROR_AUTH_NO_MANAGER = "ACL_AUTH_NO_MANAGER";
4947
string private constant ERROR_EXISTENT_MANAGER = "ACL_EXISTENT_MANAGER";
@@ -421,11 +419,13 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers {
421419

422420
// a raw call is required so we can return false if the call reverts, rather than reverting
423421
bytes memory checkCalldata = abi.encodeWithSelector(sig, _who, _where, _what, _how);
424-
uint256 oracleCheckGas = ORACLE_CHECK_GAS;
425422

426423
bool ok;
427424
assembly {
428-
ok := staticcall(oracleCheckGas, _oracleAddr, add(checkCalldata, 0x20), mload(checkCalldata), 0, 0)
425+
// send all available gas; if the oracle eats up all the gas, we will eventually revert
426+
// note that we are currently guaranteed to still have some gas after the call from
427+
// EIP-150's 63/64 gas forward rule
428+
ok := staticcall(gas, _oracleAddr, add(checkCalldata, 0x20), mload(checkCalldata), 0, 0)
429429
}
430430

431431
if (!ok) {

‎contracts/test/helpers/ACLHelper.sol‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,12 @@ contract RevertOracle is IACLOracle {
3434
}
3535
}
3636

37+
contract AssertOracle is IACLOracle {
38+
function canPerform(address, address, bytes32, uint256[]) external view returns (bool) {
39+
assert(false);
40+
}
41+
}
42+
3743
// Can't implement from IACLOracle as its canPerform() is marked as view-only
3844
contract StateModifyingOracle /* is IACLOracle */ {
3945
bool modifyState;
@@ -57,3 +63,14 @@ contract ConditionalOracle is IACLOracle {
5763
return how[0] > 0;
5864
}
5965
}
66+
67+
contract OverGasLimitOracle is IACLOracle {
68+
function canPerform(address, address, bytes32, uint256[]) external view returns (bool) {
69+
while (true) {
70+
// Do an SLOAD to increase the per-loop gas costs
71+
uint256 loadFromStorage;
72+
assembly { loadFromStorage := sload(0) }
73+
}
74+
return true;
75+
}
76+
}

‎contracts/test/tests/TestACLInterpreter.sol‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,6 @@ contract TestACLInterpreter is ACL, ACLHelper {
8282

8383
// doesn't revert even if oracle reverts
8484
assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(new RevertOracle()), false);
85-
// the staticcall will error as the oracle tries to modify state, so a no is returned
86-
assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(new StateModifyingOracle()), false);
8785
// if returned data size is not correct, returns false
8886
assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(new EmptyDataReturnOracle()), false);
8987

‎test/contracts/acl/acl_params.js‎

Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,206 @@
1+
const { assertRevert } = require('../../helpers/assertThrow')
2+
const { skipSuiteCoverage } = require('../../helpers/coverage')
3+
const { permissionParamEqOracle } = require('../../helpers/permissionParams')
4+
5+
const ACL = artifacts.require('ACL')
6+
const Kernel = artifacts.require('Kernel')
7+
const KernelProxy = artifacts.require('KernelProxy')
8+
9+
const AcceptOracle = artifacts.require('AcceptOracle')
10+
const RejectOracle = artifacts.require('RejectOracle')
11+
const RevertOracle = artifacts.require('RevertOracle')
12+
const AssertOracle = artifacts.require('AssertOracle')
13+
const OverGasLimitOracle = artifacts.require('OverGasLimitOracle')
14+
const StateModifyingOracle = artifacts.require('StateModifyingOracle')
15+
16+
const ANY_ADDR = '0xffffffffffffffffffffffffffffffffffffffff'
17+
const MAX_GAS_AVAILABLE = 6900000
18+
19+
const getExpectedGas = gas => gas * 63 / 64
20+
21+
contract('ACL params', ([permissionsRoot, specificEntity, noPermission, mockAppAddress]) => {
22+
let aclBase, kernelBase, acl, kernel
23+
const MOCK_APP_ROLE = "0xAB"
24+
25+
before(async () => {
26+
kernelBase = await Kernel.new(true) // petrify immediately
27+
aclBase = await ACL.new()
28+
})
29+
30+
beforeEach(async () => {
31+
kernel = Kernel.at((await KernelProxy.new(kernelBase.address)).address)
32+
await kernel.initialize(aclBase.address, permissionsRoot)
33+
acl = ACL.at(await kernel.acl())
34+
await acl.createPermission(permissionsRoot, mockAppAddress, MOCK_APP_ROLE, permissionsRoot)
35+
})
36+
37+
// More complex cases are checked via the solidity test in TestAclInterpreter.sol
38+
context('> ACL Oracle', () => {
39+
let aclParams
40+
41+
const testOraclePermissions = ({ shouldHavePermission }) => {
42+
const allowText = shouldHavePermission ? 'allows' : 'disallows'
43+
44+
describe('when permission is set for ANY_ADDR', () => {
45+
beforeEach(async () => {
46+
await acl.grantPermissionP(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, [aclParams])
47+
})
48+
49+
it(`ACL ${allowText} actions for ANY_ADDR`, async () => {
50+
assertion = shouldHavePermission ? assert.isTrue : assert.isFalse
51+
assertion(await acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE))
52+
})
53+
54+
it(`ACL ${allowText} actions for specific address`, async () => {
55+
assertion = shouldHavePermission ? assert.isTrue : assert.isFalse
56+
assertion(await acl.hasPermission(specificEntity, mockAppAddress, MOCK_APP_ROLE))
57+
})
58+
})
59+
60+
describe('when permission is set for specific address', async () => {
61+
beforeEach(async () => {
62+
await acl.grantPermissionP(specificEntity, mockAppAddress, MOCK_APP_ROLE, [aclParams])
63+
})
64+
65+
it(`ACL ${allowText} actions for specific address`, async () => {
66+
assertion = shouldHavePermission ? assert.isTrue : assert.isFalse
67+
assertion(await acl.hasPermission(specificEntity, mockAppAddress, MOCK_APP_ROLE))
68+
})
69+
70+
it('ACL disallows actions for other addresses', async () => {
71+
assert.isFalse(await acl.hasPermission(noPermission, mockAppAddress, MOCK_APP_ROLE))
72+
assert.isFalse(await acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE))
73+
})
74+
})
75+
}
76+
77+
describe('when the oracle accepts', () => {
78+
let acceptOracle
79+
80+
before(async () => {
81+
acceptOracle = await AcceptOracle.new()
82+
aclParams = permissionParamEqOracle(acceptOracle.address)
83+
})
84+
85+
testOraclePermissions({ shouldHavePermission: true })
86+
})
87+
88+
for (let [description, FailingOracle] of [
89+
['rejects', RejectOracle],
90+
['reverts', RevertOracle],
91+
]) {
92+
describe(`when the oracle ${description}`, () => {
93+
let failingOracle
94+
95+
before(async () => {
96+
failingOracle = await FailingOracle.new()
97+
aclParams = permissionParamEqOracle(failingOracle.address)
98+
})
99+
100+
testOraclePermissions({ shouldHavePermission: false })
101+
})
102+
}
103+
104+
describe('when the oracle modifies state', () => {
105+
let stateModifyingOracle
106+
107+
before(async () => {
108+
stateModifyingOracle = await StateModifyingOracle.new()
109+
aclParams = permissionParamEqOracle(stateModifyingOracle.address)
110+
})
111+
112+
testOraclePermissions({ shouldHavePermission: false })
113+
})
114+
115+
// Both the assert and oog gas cases should be similar, since assert should eat all the
116+
// available gas
117+
for (let [description, FailingOutOfGasOracle] of [
118+
['asserts', AssertOracle],
119+
['uses all available gas', OverGasLimitOracle],
120+
]) {
121+
skipSuiteCoverage(describe)(`when the oracle ${description}`, () => {
122+
let overGasLimitOracle
123+
124+
before(async () => {
125+
overGasLimitOracle = await FailingOutOfGasOracle.new()
126+
aclParams = permissionParamEqOracle(overGasLimitOracle.address)
127+
})
128+
129+
testOraclePermissions({ shouldHavePermission: false })
130+
131+
describe('gas', () => {
132+
describe('when permission is set for ANY_ADDR', () => {
133+
// Note `evalParams()` is called twice when calling `hasPermission` for `ANY_ADDR`, so
134+
// gas costs are much, much higher to compensate for the 63/64th rule on the second call
135+
const MEDIUM_GAS = 3000000
136+
const LOW_GAS = 2900000
137+
138+
beforeEach(async () => {
139+
await acl.grantPermissionP(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, [aclParams])
140+
})
141+
142+
it('ACL disallows and uses all gas when given large amount of gas', async () => {
143+
assert.isFalse(await acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, { gas: MAX_GAS_AVAILABLE }))
144+
145+
const hasPermissionTxHash = await acl.hasPermission.sendTransaction(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, { gas: MAX_GAS_AVAILABLE })
146+
const hasPermissionGasConsumed = web3.eth.getTransactionReceipt(hasPermissionTxHash).gasUsed
147+
// Surprisingly, the actual gas used is quite a lot lower than expected, but it is
148+
// unclear if this is a ganache issue or if there are gas refunds we're not taking
149+
// into account
150+
assert.closeTo(hasPermissionGasConsumed, getExpectedGas(MAX_GAS_AVAILABLE), 105000)
151+
})
152+
153+
it('ACL disallows and uses all gas when given medium amount of gas', async () => {
154+
assert.isFalse(await acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, { gas: MEDIUM_GAS }))
155+
156+
const hasPermissionTxHash = await acl.hasPermission.sendTransaction(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, { gas: MEDIUM_GAS })
157+
const hasPermissionGasConsumed = web3.eth.getTransactionReceipt(hasPermissionTxHash).gasUsed
158+
assert.closeTo(hasPermissionGasConsumed, getExpectedGas(MEDIUM_GAS), 10000)
159+
})
160+
161+
it('ACL reverts when given small amount of gas', async () => {
162+
await assertRevert(acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, { gas: LOW_GAS }))
163+
})
164+
})
165+
166+
describe('when permission is set for specific address', async () => {
167+
const MEDIUM_GAS = 190000
168+
// Note that these gas values are still quite high for causing reverts in "low gas"
169+
// situations, as we incur some overhead with delegating into proxies and other checks.
170+
// Assuming we incur 40-60k gas overhead for this, we only have ~140,000 gas left.
171+
// After the oracle call, we only have 140,000 / 64 ~= 2000 gas left, which begins to
172+
// quick run out with SLOADs.
173+
const LOW_GAS = 180000
174+
175+
beforeEach(async () => {
176+
await acl.grantPermissionP(specificEntity, mockAppAddress, MOCK_APP_ROLE, [aclParams])
177+
})
178+
179+
it('ACL disallows and uses all gas when given large amount of gas', async () => {
180+
assert.isFalse(await acl.hasPermission(specificEntity, mockAppAddress, MOCK_APP_ROLE, { gas: MAX_GAS_AVAILABLE }))
181+
182+
const hasPermissionTxHash = await acl.hasPermission.sendTransaction(specificEntity, mockAppAddress, MOCK_APP_ROLE, { gas: MAX_GAS_AVAILABLE })
183+
const hasPermissionGasConsumed = web3.eth.getTransactionReceipt(hasPermissionTxHash).gasUsed
184+
// Surprisingly, the actual gas used is quite a lot lower than expected, but it is
185+
// unclear if this is a ganache issue or if there are gas refunds we're not taking
186+
// into account
187+
assert.closeTo(hasPermissionGasConsumed, getExpectedGas(MAX_GAS_AVAILABLE), 105000)
188+
})
189+
190+
it('ACL disallows and uses all gas when given medium amount of gas', async () => {
191+
assert.isFalse(await acl.hasPermission(specificEntity, mockAppAddress, MOCK_APP_ROLE, { gas: MEDIUM_GAS }))
192+
193+
const hasPermissionTxHash = await acl.hasPermission.sendTransaction(specificEntity, mockAppAddress, MOCK_APP_ROLE, { gas: MEDIUM_GAS })
194+
const hasPermissionGasConsumed = web3.eth.getTransactionReceipt(hasPermissionTxHash).gasUsed
195+
assert.closeTo(hasPermissionGasConsumed, getExpectedGas(MEDIUM_GAS), 10000)
196+
})
197+
198+
it('ACL reverts when given small amount of gas', async () => {
199+
await assertRevert(acl.hasPermission(specificEntity, mockAppAddress, MOCK_APP_ROLE, { gas: LOW_GAS }))
200+
})
201+
})
202+
})
203+
})
204+
}
205+
})
206+
})

‎test/helpers/coverage.js‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,12 @@ const skipCoverage = test => {
99
}
1010
}
1111

12+
// For some reason, adding skipCoverage() to `before()`s were not working
13+
const skipSuiteCoverage = suite => {
14+
return process.env.SOLIDITY_COVERAGE === 'true' ? suite.skip : suite
15+
}
16+
1217
module.exports = {
13-
skipCoverage
18+
skipCoverage,
19+
skipSuiteCoverage,
1420
}

‎test/helpers/permissionParams.js‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
const permissionParamEqOracle = (oracleAddress) => {
2+
// Set role such that the Oracle canPerform() function is used to determine the permission
3+
const argId = '0xCB' // arg 203 - Oracle ID
4+
const op = '01' // equal
5+
const value = oracleAddress.slice(2).padStart(60, 0) // 60 as params are uint240
6+
return new web3.BigNumber(`${argId}${op}${value}`)
7+
}
8+
9+
module.exports = {
10+
permissionParamEqOracle
11+
}

0 commit comments

Comments
 (0)