From 4b3dc4ebfeb41fe65cd5a043486cc15f1801bd69 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Fri, 20 May 2022 16:32:55 +0100 Subject: [PATCH] Add explicit getters for TimelockAuthorizer (#1298) * refactor: make root storage variable private * refactor: make `delayerPerActionId` storage variable private * refactor: make scheduledExecutions storage variable private * refactor: make isPermissionGranted storage variable private * refactor: use getRootTransferDelay getter rather than using variable directly * refactor: rename isPermissionGranted to isPermissionGrantedOnTarget --- .../authorizer/TimelockAuthorizer.sol | 93 +++++++++++++------ .../authorizer/TimelockAuthorizer.test.ts | 20 ++-- .../models/authorizer/TimelockAuthorizer.ts | 6 +- 3 files changed, 78 insertions(+), 41 deletions(-) diff --git a/pkg/vault/contracts/authorizer/TimelockAuthorizer.sol b/pkg/vault/contracts/authorizer/TimelockAuthorizer.sol index a26fec6971..995e82f82b 100644 --- a/pkg/vault/contracts/authorizer/TimelockAuthorizer.sol +++ b/pkg/vault/contracts/authorizer/TimelockAuthorizer.sol @@ -13,6 +13,7 @@ // along with this program. If not, see . pragma solidity ^0.7.0; +pragma experimental ABIEncoderV2; import "@balancer-labs/v2-interfaces/contracts/solidity-utils/helpers/BalancerErrors.sol"; import "@balancer-labs/v2-interfaces/contracts/solidity-utils/helpers/IAuthentication.sol"; @@ -77,10 +78,10 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { IAuthentication private immutable _vault; uint256 private immutable _rootTransferDelay; - address public root; - ScheduledExecution[] public scheduledExecutions; - mapping(bytes32 => bool) public isPermissionGranted; - mapping(bytes32 => uint256) public delaysPerActionId; + address private _root; + ScheduledExecution[] private _scheduledExecutions; + mapping(bytes32 => bool) private _isPermissionGranted; + mapping(bytes32 => uint256) private _delaysPerActionId; /** * @dev Emitted when a new execution `scheduledExecutionId` is scheduled. @@ -127,7 +128,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { IAuthentication vault, uint256 rootTransferDelay ) { - root = admin; + _root = admin; _vault = vault; _executor = new TimelockExecutor(); _rootTransferDelay = rootTransferDelay; @@ -148,7 +149,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { * @dev Returns true if `account` is the root. */ function isRoot(address account) public view returns (bool) { - return account == root; + return account == _root; } /** @@ -172,6 +173,13 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { return address(_executor); } + /** + * @dev Returns the root address. + */ + function getRoot() external view returns (address) { + return _root; + } + /** * @dev Returns the action ID for function selector `selector`. */ @@ -186,6 +194,13 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { return keccak256(abi.encodePacked(actionId, how)); } + /** + * @dev Returns the execution delay for action `actionId`. + */ + function getActionIdDelay(bytes32 actionId) external view returns (uint256) { + return _delaysPerActionId[actionId]; + } + /** * @dev Returns the permission ID for action `actionId`, account `account` and target `where`. */ @@ -197,6 +212,21 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { return keccak256(abi.encodePacked(actionId, account, where)); } + /** + * @dev Returns true if `account` has the permission defined by action `actionId` and target `where`. + * This function is specific for the strict permission defined by the tuple `(actionId, where)`, `account` may also + * hold the global permission for the action `actionId` which would allow them to perform this action on `where`. + * For this reason, it's recommended to use `hasPermission` if checking whether `account` is allowed to perform + * a given action. + */ + function isPermissionGrantedOnTarget( + bytes32 actionId, + address account, + address where + ) external view returns (bool) { + return _isPermissionGranted[permissionId(actionId, account, where)]; + } + /** * @dev Returns true if `account` is allowed to perform action `actionId` in target `where`. */ @@ -206,8 +236,8 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { address where ) public view returns (bool) { return - isPermissionGranted[permissionId(actionId, account, where)] || - isPermissionGranted[permissionId(actionId, account, EVERYWHERE)]; + _isPermissionGranted[permissionId(actionId, account, where)] || + _isPermissionGranted[permissionId(actionId, account, EVERYWHERE)]; } /** @@ -241,7 +271,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { address where ) public view override returns (bool) { return - (delaysPerActionId[actionId] > 0) ? account == address(_executor) : hasPermission(actionId, account, where); + _delaysPerActionId[actionId] > 0 ? account == address(_executor) : hasPermission(actionId, account, where); } /** @@ -266,13 +296,20 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { return _canPerformOrWhatever(REVOKE_ACTION_ID, account, where, actionId); } + /** + * @dev Returns the scheduled execution `scheduledExecutionId`. + */ + function getScheduledExecution(uint256 scheduledExecutionId) external view returns (ScheduledExecution memory) { + return _scheduledExecutions[scheduledExecutionId]; + } + /** * @dev Returns true if execution `scheduledExecutionId` can be executed. * Only true if it is not already executed or cancelled, and if the execution delay has passed. */ function canExecute(uint256 scheduledExecutionId) external view returns (bool) { - require(scheduledExecutionId < scheduledExecutions.length, "ACTION_DOES_NOT_EXIST"); - ScheduledExecution storage scheduledExecution = scheduledExecutions[scheduledExecutionId]; + require(scheduledExecutionId < _scheduledExecutions.length, "ACTION_DOES_NOT_EXIST"); + ScheduledExecution storage scheduledExecution = _scheduledExecutions[scheduledExecutionId]; return !scheduledExecution.executed && !scheduledExecution.cancelled && @@ -284,7 +321,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { * @dev Sets the root address to `newRoot`. */ function setRoot(address newRoot) external onlyExecutor { - root = newRoot; + _root = newRoot; emit RootSet(newRoot); } @@ -299,7 +336,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { bytes32 actionId = getActionId(this.setRoot.selector); bytes32 scheduleRootChangeActionId = getActionId(SCHEDULE_DELAY_ACTION_ID, actionId); bytes memory data = abi.encodeWithSelector(this.setRoot.selector, newRoot); - return _scheduleWithDelay(scheduleRootChangeActionId, address(this), data, _rootTransferDelay, executors); + return _scheduleWithDelay(scheduleRootChangeActionId, address(this), data, getRootTransferDelay(), executors); } /** @@ -307,10 +344,10 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { */ function setDelay(bytes32 actionId, uint256 delay) external onlyExecutor { bytes32 setAuthorizerActionId = _vault.getActionId(IVault.setAuthorizer.selector); - bool isAllowed = actionId == setAuthorizerActionId || delay <= delaysPerActionId[setAuthorizerActionId]; + bool isAllowed = actionId == setAuthorizerActionId || delay <= _delaysPerActionId[setAuthorizerActionId]; require(isAllowed, "DELAY_EXCEEDS_SET_AUTHORIZER"); - delaysPerActionId[actionId] = delay; + _delaysPerActionId[actionId] = delay; emit ActionDelaySet(actionId, delay); } @@ -330,7 +367,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { // critical, as otherwise it'd be possible to execute an action with a delay shorter than its current one // by first changing it to a smaller (or zero) value. - uint256 actionDelay = delaysPerActionId[actionId]; + uint256 actionDelay = _delaysPerActionId[actionId]; bytes32 scheduleDelayActionId = getActionId(SCHEDULE_DELAY_ACTION_ID, actionId); bytes memory data = abi.encodeWithSelector(this.setDelay.selector, actionId, newDelay); return _scheduleWithDelay(scheduleDelayActionId, address(this), data, actionDelay, executors); @@ -354,8 +391,8 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { * @dev Executes a scheduled action `scheduledExecutionId`. */ function execute(uint256 scheduledExecutionId) external returns (bytes memory result) { - require(scheduledExecutionId < scheduledExecutions.length, "ACTION_DOES_NOT_EXIST"); - ScheduledExecution storage scheduledExecution = scheduledExecutions[scheduledExecutionId]; + require(scheduledExecutionId < _scheduledExecutions.length, "ACTION_DOES_NOT_EXIST"); + ScheduledExecution storage scheduledExecution = _scheduledExecutions[scheduledExecutionId]; require(!scheduledExecution.executed, "ACTION_ALREADY_EXECUTED"); require(!scheduledExecution.cancelled, "ACTION_ALREADY_CANCELLED"); @@ -376,8 +413,8 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { * @dev Cancels a scheduled action `scheduledExecutionId`. */ function cancel(uint256 scheduledExecutionId) external { - require(scheduledExecutionId < scheduledExecutions.length, "ACTION_DOES_NOT_EXIST"); - ScheduledExecution storage scheduledExecution = scheduledExecutions[scheduledExecutionId]; + require(scheduledExecutionId < _scheduledExecutions.length, "ACTION_DOES_NOT_EXIST"); + ScheduledExecution storage scheduledExecution = _scheduledExecutions[scheduledExecutionId]; require(!scheduledExecution.executed, "ACTION_ALREADY_EXECUTED"); require(!scheduledExecution.cancelled, "ACTION_ALREADY_CANCELLED"); @@ -499,8 +536,8 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { address where ) private { bytes32 permission = permissionId(actionId, account, where); - if (!isPermissionGranted[permission]) { - isPermissionGranted[permission] = true; + if (!_isPermissionGranted[permission]) { + _isPermissionGranted[permission] = true; emit PermissionGranted(actionId, account, where); } } @@ -511,8 +548,8 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { address where ) private { bytes32 permission = permissionId(actionId, account, where); - if (isPermissionGranted[permission]) { - isPermissionGranted[permission] = false; + if (_isPermissionGranted[permission]) { + _isPermissionGranted[permission] = false; emit PermissionRevoked(actionId, account, where); } } @@ -523,7 +560,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { bytes memory data, address[] memory executors ) private returns (uint256 scheduledExecutionId) { - uint256 delay = delaysPerActionId[actionId]; + uint256 delay = _delaysPerActionId[actionId]; require(delay > 0, "CANNOT_SCHEDULE_ACTION"); return _scheduleWithDelay(actionId, where, data, delay, executors); } @@ -535,13 +572,13 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { uint256 delay, address[] memory executors ) private returns (uint256 scheduledExecutionId) { - scheduledExecutionId = scheduledExecutions.length; + scheduledExecutionId = _scheduledExecutions.length; emit ExecutionScheduled(actionId, scheduledExecutionId); // solhint-disable-next-line not-rely-on-time uint256 executableAt = block.timestamp + delay; bool protected = executors.length > 0; - scheduledExecutions.push(ScheduledExecution(where, data, false, false, protected, executableAt)); + _scheduledExecutions.push(ScheduledExecution(where, data, false, false, protected, executableAt)); bytes32 executeActionId = getActionId(EXECUTE_ACTION_ID, bytes32(scheduledExecutionId)); for (uint256 i = 0; i < executors.length; i++) { @@ -569,7 +606,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication { // If there is a delay defined for the granular action ID, then the sender must be the authorizer (scheduled // execution) bytes32 granularActionId = getActionId(actionId, how); - if (delaysPerActionId[granularActionId] > 0) { + if (_delaysPerActionId[granularActionId] > 0) { return account == address(_executor); } diff --git a/pkg/vault/test/authorizer/TimelockAuthorizer.test.ts b/pkg/vault/test/authorizer/TimelockAuthorizer.test.ts index 4fac4d6eb7..5c52fe0e44 100644 --- a/pkg/vault/test/authorizer/TimelockAuthorizer.test.ts +++ b/pkg/vault/test/authorizer/TimelockAuthorizer.test.ts @@ -1926,7 +1926,7 @@ describe('TimelockAuthorizer', () => { it('schedules a delay change', async () => { const id = await authorizer.scheduleDelayChange(action, delay, [], { from: admin }); - const scheduledExecution = await authorizer.scheduledExecutions(id); + const scheduledExecution = await authorizer.getScheduledExecution(id); expect(scheduledExecution.executed).to.be.false; expect(scheduledExecution.data).to.be.equal(expectedData); expect(scheduledExecution.where).to.be.equal(authorizer.address); @@ -1960,7 +1960,7 @@ describe('TimelockAuthorizer', () => { it('schedules a delay change', async () => { const id = await authorizer.scheduleDelayChange(action, delay, [], { from: admin }); - const scheduledExecution = await authorizer.scheduledExecutions(id); + const scheduledExecution = await authorizer.getScheduledExecution(id); expect(scheduledExecution.executed).to.be.false; expect(scheduledExecution.data).to.be.equal(expectedData); expect(scheduledExecution.where).to.be.equal(authorizer.address); @@ -2082,7 +2082,7 @@ describe('TimelockAuthorizer', () => { it('schedules a non-protected execution', async () => { const id = await schedule(); - const scheduledExecution = await authorizer.scheduledExecutions(id); + const scheduledExecution = await authorizer.getScheduledExecution(id); expect(scheduledExecution.executed).to.be.false; expect(scheduledExecution.data).to.be.equal(data); expect(scheduledExecution.where).to.be.equal(where.address); @@ -2102,7 +2102,7 @@ describe('TimelockAuthorizer', () => { const receipt = await authorizer.execute(id); expectEvent.inReceipt(await receipt.wait(), 'ExecutionExecuted', { scheduledExecutionId: id }); - const scheduledExecution = await authorizer.scheduledExecutions(id); + const scheduledExecution = await authorizer.getScheduledExecution(id); expect(scheduledExecution.executed).to.be.true; expectEvent.inIndirectReceipt( @@ -2132,7 +2132,7 @@ describe('TimelockAuthorizer', () => { it('schedules the requested execution', async () => { const id = await schedule(); - const scheduledExecution = await authorizer.scheduledExecutions(id); + const scheduledExecution = await authorizer.getScheduledExecution(id); expect(scheduledExecution.executed).to.be.false; expect(scheduledExecution.data).to.be.equal(data); expect(scheduledExecution.where).to.be.equal(where.address); @@ -2156,7 +2156,7 @@ describe('TimelockAuthorizer', () => { const receipt = await authorizer.execute(id, { from: executors[0] }); expectEvent.inReceipt(await receipt.wait(), 'ExecutionExecuted', { scheduledExecutionId: id }); - const scheduledExecution = await authorizer.scheduledExecutions(id); + const scheduledExecution = await authorizer.getScheduledExecution(id); expect(scheduledExecution.executed).to.be.true; expectEvent.inIndirectReceipt( @@ -2275,7 +2275,7 @@ describe('TimelockAuthorizer', () => { it('executes the action', async () => { const receipt = await authorizer.execute(id, { from }); - const scheduledExecution = await authorizer.scheduledExecutions(id); + const scheduledExecution = await authorizer.getScheduledExecution(id); expect(scheduledExecution.executed).to.be.true; expectEvent.inIndirectReceipt( @@ -2345,7 +2345,7 @@ describe('TimelockAuthorizer', () => { const receipt = await authorizer.execute(id); - const scheduledExecution = await authorizer.scheduledExecutions(id); + const scheduledExecution = await authorizer.getScheduledExecution(id); expect(scheduledExecution.executed).to.be.true; expectEvent.inIndirectReceipt( @@ -2402,7 +2402,7 @@ describe('TimelockAuthorizer', () => { it('cancels the action', async () => { await authorizer.cancel(id, { from }); - const scheduledExecution = await authorizer.scheduledExecutions(id); + const scheduledExecution = await authorizer.getScheduledExecution(id); expect(scheduledExecution.cancelled).to.be.true; }); @@ -2472,7 +2472,7 @@ describe('TimelockAuthorizer', () => { const id = await authorizer.scheduleRootChange(grantee, [], { from: admin }); - const scheduledExecution = await authorizer.scheduledExecutions(id); + const scheduledExecution = await authorizer.getScheduledExecution(id); expect(scheduledExecution.executed).to.be.false; expect(scheduledExecution.data).to.be.equal(expectedData); expect(scheduledExecution.where).to.be.equal(authorizer.address); diff --git a/pvt/helpers/src/models/authorizer/TimelockAuthorizer.ts b/pvt/helpers/src/models/authorizer/TimelockAuthorizer.ts index 07b473b465..78088b4cd4 100644 --- a/pvt/helpers/src/models/authorizer/TimelockAuthorizer.ts +++ b/pvt/helpers/src/models/authorizer/TimelockAuthorizer.ts @@ -61,10 +61,10 @@ export default class TimelockAuthorizer { } async delay(action: string): Promise { - return this.instance.delaysPerActionId(action); + return this.instance.getActionIdDelay(action); } - async scheduledExecutions( + async getScheduledExecution( id: BigNumberish ): Promise<{ executed: boolean; @@ -74,7 +74,7 @@ export default class TimelockAuthorizer { data: string; where: string; }> { - return this.instance.scheduledExecutions(id); + return this.instance.getScheduledExecution(id); } async canPerform(action: string, account: Account, where: Account): Promise {