Skip to content

Commit

Permalink
Add explicit getters for TimelockAuthorizer (#1298)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@TomAFrench
TomAFrench authored May 20, 2022
1 parent d391165 commit 4b3dc4e
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 41 deletions.
93 changes: 65 additions & 28 deletions pkg/vault/contracts/authorizer/TimelockAuthorizer.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// along with this program. If not, see <http://www.gnu.org/licenses/>.

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";
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -127,7 +128,7 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication {
IAuthentication vault,
uint256 rootTransferDelay
) {
root = admin;
_root = admin;
_vault = vault;
_executor = new TimelockExecutor();
_rootTransferDelay = rootTransferDelay;
Expand All @@ -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;
}

/**
Expand All @@ -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`.
*/
Expand All @@ -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`.
*/
Expand All @@ -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`.
*/
Expand All @@ -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)];
}

/**
Expand Down Expand Up @@ -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);
}

/**
Expand All @@ -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 &&
Expand All @@ -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);
}

Expand All @@ -299,18 +336,18 @@ 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);
}

/**
* @dev Sets a new delay `delay` for action `actionId`.
*/
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);
}

Expand All @@ -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);
Expand All @@ -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");

Expand All @@ -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");
Expand Down Expand Up @@ -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);
}
}
Expand All @@ -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);
}
}
Expand All @@ -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);
}
Expand All @@ -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++) {
Expand Down Expand Up @@ -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);
}

Expand Down
20 changes: 10 additions & 10 deletions pkg/vault/test/authorizer/TimelockAuthorizer.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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(
Expand Down Expand Up @@ -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);
Expand All @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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;
});

Expand Down Expand Up @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions pvt/helpers/src/models/authorizer/TimelockAuthorizer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@ export default class TimelockAuthorizer {
}

async delay(action: string): Promise<BigNumberish> {
return this.instance.delaysPerActionId(action);
return this.instance.getActionIdDelay(action);
}

async scheduledExecutions(
async getScheduledExecution(
id: BigNumberish
): Promise<{
executed: boolean;
Expand All @@ -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<boolean> {
Expand Down

0 comments on commit 4b3dc4e

Please sign in to comment.