diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index eb79eb8ec..0483f5140 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -582,13 +582,9 @@ contract AccessManager is Context, Multicall, IAccessManager { } // If caller is authorised, schedule operation - operationId = _hashOperation(caller, target, data); + operationId = hashOperation(caller, target, data); - // Cannot reschedule unless the operation has expired - uint48 prevTimepoint = _schedules[operationId].timepoint; - if (prevTimepoint != 0 && !_isExpired(prevTimepoint)) { - revert AccessManagerAlreadyScheduled(operationId); - } + _checkNotScheduled(operationId); unchecked { // It's not feasible to overflow the nonce in less than 1000 years @@ -601,6 +597,17 @@ contract AccessManager is Context, Multicall, IAccessManager { // Using named return values because otherwise we get stack too deep } + /** + * @dev Reverts if the operation is currently scheduled and has not expired. + * (Note: This function was introduced due to stack too deep errors in schedule.) + */ + function _checkNotScheduled(bytes32 operationId) private view { + uint48 prevTimepoint = _schedules[operationId].timepoint; + if (prevTimepoint != 0 && !_isExpired(prevTimepoint)) { + revert AccessManagerAlreadyScheduled(operationId); + } + } + /** * @dev Execute a function that is delay restricted, provided it was properly scheduled beforehand, or the * execution delay is 0. @@ -625,7 +632,7 @@ contract AccessManager is Context, Multicall, IAccessManager { } // If caller is authorised, check operation was scheduled early enough - bytes32 operationId = _hashOperation(caller, target, data); + bytes32 operationId = hashOperation(caller, target, data); uint32 nonce; if (setback != 0) { @@ -659,7 +666,7 @@ contract AccessManager is Context, Multicall, IAccessManager { if (IAccessManaged(target).isConsumingScheduledOp() != IAccessManaged.isConsumingScheduledOp.selector) { revert AccessManagerUnauthorizedConsume(target); } - _consumeScheduledOp(_hashOperation(caller, target, data)); + _consumeScheduledOp(hashOperation(caller, target, data)); } /** @@ -699,7 +706,7 @@ contract AccessManager is Context, Multicall, IAccessManager { address msgsender = _msgSender(); bytes4 selector = bytes4(data[0:4]); - bytes32 operationId = _hashOperation(caller, target, data); + bytes32 operationId = hashOperation(caller, target, data); if (_schedules[operationId].timepoint == 0) { revert AccessManagerNotScheduled(operationId); } else if (caller != msgsender) { @@ -721,7 +728,7 @@ contract AccessManager is Context, Multicall, IAccessManager { /** * @dev Hashing function for delayed operations */ - function _hashOperation(address caller, address target, bytes calldata data) private pure returns (bytes32) { + function hashOperation(address caller, address target, bytes calldata data) public view virtual returns (bytes32) { return keccak256(abi.encode(caller, target, data)); } @@ -756,7 +763,7 @@ contract AccessManager is Context, Multicall, IAccessManager { (, uint64 requiredRole, ) = _getAdminRestrictions(_msgData()); revert AccessManagerUnauthorizedAccount(caller, requiredRole); } else { - _consumeScheduledOp(_hashOperation(caller, address(this), _msgData())); + _consumeScheduledOp(hashOperation(caller, address(this), _msgData())); } } } diff --git a/contracts/access/manager/IAccessManager.sol b/contracts/access/manager/IAccessManager.sol index b0c9a51e4..0fec166f9 100644 --- a/contracts/access/manager/IAccessManager.sol +++ b/contracts/access/manager/IAccessManager.sol @@ -57,6 +57,8 @@ interface IAccessManager { bytes4 selector ) external view returns (bool allowed, uint32 delay); + function hashOperation(address caller, address target, bytes calldata data) external view returns (bytes32); + function expiration() external view returns (uint32); function isTargetClosed(address target) external view returns (bool); diff --git a/contracts/governance/extensions/GovernorTimelockAccess.sol b/contracts/governance/extensions/GovernorTimelockAccess.sol index c1c944e43..36f82a20f 100644 --- a/contracts/governance/extensions/GovernorTimelockAccess.sol +++ b/contracts/governance/extensions/GovernorTimelockAccess.sol @@ -21,9 +21,19 @@ import {Time} from "../../utils/types/Time.sol"; * permissions. Operations that are delay-restricted by the manager, however, will be executed through the * {AccessManager-execute} function. * - * Note that some operations may be cancelable in the {AccessManager} by the admin or a set of guardians, depending on - * the restricted operation being invoked. Since proposals are atomic, the cancellation by a guardian of a single - * operation in a proposal will cause all of it to become unable to execute. + * ==== Security Considerations + * + * Some operations may be cancelable in the `AccessManager` by the admin or a set of guardians, depending on the + * restricted function being invoked. Since proposals are atomic, the cancellation by a guardian of a single operation + * in a proposal will cause all of the proposal to become unable to execute. Consider proposing cancellable operations + * separately. + * + * By default, function calls will be routed through the associated `AccessManager` whenever it claims the target + * function to be restricted by it. However, admins may configure the manager to make that claim for functions that a + * governor would want to call directly (e.g., token transfers) in an attempt to deny it access to those functions. To + * mitigate this attack vector, the governor is able to ignore the restrictions claimed by the `AccessManager` using + * {setAccessManagerIgnored}. While permanent denial of service is mitigated, temporary DoS may still be technically + * possible. All of the governor's own functions (e.g., {setBaseDelaySeconds}) ignore the `AccessManager` by default. */ abstract contract GovernorTimelockAccess is Governor { // An execution plan is produced at the moment a proposal is created, in order to fix at that point the exact @@ -39,6 +49,11 @@ abstract contract GovernorTimelockAccess is Governor { mapping(uint256 operationBucket => uint32[8]) managerData; } + // The meaning of the "toggle" set to true depends on the target contract. + // If target == address(this), the manager is ignored by default, and a true toggle means it won't be ignored. + // For all other target contracts, the manager is used by default, and a true toggle means it will be ignored. + mapping(address target => mapping(bytes4 selector => bool)) private _ignoreToggle; + mapping(uint256 proposalId => ExecutionPlan) private _executionPlan; uint32 private _baseDelay; @@ -47,8 +62,10 @@ abstract contract GovernorTimelockAccess is Governor { error GovernorUnmetDelay(uint256 proposalId, uint256 neededTimestamp); error GovernorMismatchedNonce(uint256 proposalId, uint256 expectedNonce, uint256 actualNonce); + error GovernorLockedIgnore(); event BaseDelaySet(uint32 oldBaseDelaySeconds, uint32 newBaseDelaySeconds); + event AccessManagerIgnoredSet(address target, bytes4 selector, bool ignored); /** * @dev Initialize the governor with an {AccessManager} and initial base delay. @@ -93,21 +110,61 @@ abstract contract GovernorTimelockAccess is Governor { } /** - * @dev Public accessor to check the execution plan, including the number of seconds that the proposal will be - * delayed since queuing, and an array indicating which of the proposal actions will be executed indirectly through - * the associated {AccessManager}. + * @dev Check if restrictions from the associated {AccessManager} are ignored for a target function. Returns true + * when the target function will be invoked directly regardless of `AccessManager` settings for the function. + * See {setAccessManagerIgnored} and Security Considerations above. */ - function proposalExecutionPlan(uint256 proposalId) public view returns (uint32, bool[] memory) { + function isAccessManagerIgnored(address target, bytes4 selector) public view virtual returns (bool) { + bool isGovernor = target == address(this); + return _ignoreToggle[target][selector] != isGovernor; // equivalent to: isGovernor ? !toggle : toggle + } + + /** + * @dev Configure whether restrictions from the associated {AccessManager} are ignored for a target function. + * See Security Considerations above. + */ + function setAccessManagerIgnored( + address target, + bytes4[] calldata selectors, + bool ignored + ) public virtual onlyGovernance { + for (uint256 i = 0; i < selectors.length; ++i) { + _setAccessManagerIgnored(target, selectors[i], ignored); + } + } + + /** + * @dev Internal version of {setAccessManagerIgnored} without access restriction. + */ + function _setAccessManagerIgnored(address target, bytes4 selector, bool ignored) internal virtual { + bool isGovernor = target == address(this); + if (isGovernor && selector == this.setAccessManagerIgnored.selector) { + revert GovernorLockedIgnore(); + } + _ignoreToggle[target][selector] = ignored != isGovernor; // equivalent to: isGovernor ? !ignored : ignored + emit AccessManagerIgnoredSet(target, selector, ignored); + } + + /** + * @dev Public accessor to check the execution plan, including the number of seconds that the proposal will be + * delayed since queuing, an array indicating which of the proposal actions will be executed indirectly through + * the associated {AccessManager}, and another indicating which will be scheduled in {queue}. Note that + * those that must be scheduled are cancellable by `AccessManager` guardians. + */ + function proposalExecutionPlan( + uint256 proposalId + ) public view returns (uint32 delay, bool[] memory indirect, bool[] memory withDelay) { ExecutionPlan storage plan = _executionPlan[proposalId]; - uint32 delay = plan.delay; uint32 length = plan.length; - bool[] memory indirect = new bool[](length); + delay = plan.delay; + indirect = new bool[](length); + withDelay = new bool[](length); for (uint256 i = 0; i < length; ++i) { - (indirect[i], ) = _getManagerData(plan, i); + (indirect[i], withDelay[i], ) = _getManagerData(plan, i); } - return (delay, indirect); + return (delay, indirect, withDelay); } /** @@ -134,12 +191,19 @@ abstract contract GovernorTimelockAccess is Governor { plan.length = SafeCast.toUint16(targets.length); for (uint256 i = 0; i < targets.length; ++i) { - uint32 delay = _detectExecutionRequirements(targets[i], bytes4(calldatas[i])); - if (delay > 0) { - _setManagerData(plan, i, 0); + address target = targets[i]; + bytes4 selector = bytes4(calldatas[i]); + (bool immediate, uint32 delay) = AuthorityUtils.canCallWithDelay( + address(_manager), + address(this), + target, + selector + ); + if ((immediate || delay > 0) && !isAccessManagerIgnored(target, selector)) { + _setManagerData(plan, i, !immediate, 0); + // downcast is safe because both arguments are uint32 + neededDelay = uint32(Math.max(delay, neededDelay)); } - // downcast is safe because both arguments are uint32 - neededDelay = uint32(Math.max(delay, neededDelay)); } plan.delay = neededDelay; @@ -164,10 +228,10 @@ abstract contract GovernorTimelockAccess is Governor { uint48 eta = Time.timestamp() + plan.delay; for (uint256 i = 0; i < targets.length; ++i) { - (bool delayed, ) = _getManagerData(plan, i); - if (delayed) { + (, bool withDelay, ) = _getManagerData(plan, i); + if (withDelay) { (, uint32 nonce) = _manager.schedule(targets[i], calldatas[i], eta); - _setManagerData(plan, i, nonce); + _setManagerData(plan, i, true, nonce); } } @@ -192,10 +256,10 @@ abstract contract GovernorTimelockAccess is Governor { ExecutionPlan storage plan = _executionPlan[proposalId]; for (uint256 i = 0; i < targets.length; ++i) { - (bool delayed, uint32 nonce) = _getManagerData(plan, i); - if (delayed) { + (bool controlled, bool withDelay, uint32 nonce) = _getManagerData(plan, i); + if (controlled) { uint32 executedNonce = _manager.execute{value: values[i]}(targets[i], calldatas[i]); - if (executedNonce != nonce) { + if (withDelay && executedNonce != nonce) { revert GovernorMismatchedNonce(proposalId, nonce, executedNonce); } } else { @@ -220,15 +284,23 @@ abstract contract GovernorTimelockAccess is Governor { ExecutionPlan storage plan = _executionPlan[proposalId]; - // If the proposal has been scheduled it will have an ETA and we have to externally cancel + // If the proposal has been scheduled it will have an ETA and we may have to externally cancel if (eta != 0) { for (uint256 i = 0; i < targets.length; ++i) { - (bool delayed, uint32 nonce) = _getManagerData(plan, i); - if (delayed) { - // Attempt to cancel considering the operation could have been cancelled and rescheduled already - uint32 canceledNonce = _manager.cancel(address(this), targets[i], calldatas[i]); - if (canceledNonce != nonce) { - revert GovernorMismatchedNonce(proposalId, nonce, canceledNonce); + (, bool withDelay, uint32 nonce) = _getManagerData(plan, i); + // Only attempt to cancel if the execution plan included a delay + if (withDelay) { + bytes32 operationId = _manager.hashOperation(address(this), targets[i], calldatas[i]); + // Check first if the current operation nonce is the one that we observed previously. It could + // already have been cancelled and rescheduled. We don't want to cancel unless it is exactly the + // instance that we previously scheduled. + if (nonce == _manager.getNonce(operationId)) { + // It is important that all calls have an opportunity to be cancelled. We chose to ignore + // potential failures of some of the cancel operations to give the other operations a chance to + // be properly cancelled. In particular cancel might fail if the operation was already cancelled + // by guardians previously. We don't match on the revert reason to avoid encoding assumptions + // about specific errors. + try _manager.cancel(address(this), targets[i], calldatas[i]) {} catch {} } } } @@ -237,39 +309,27 @@ abstract contract GovernorTimelockAccess is Governor { return proposalId; } - /** - * @dev Check if the execution of a call needs to be performed through an AccessManager and what delay should be - * applied to this call. - * - * Returns { manager: address(0), delay: 0 } if: - * - target does not have code - * - target does not implement IAccessManaged - * - calling canCall on the target's manager returns a 0 delay - * - calling canCall on the target's manager reverts - * Otherwise (calling canCall on the target's manager returns a non 0 delay), return the address of the - * AccessManager to use, and the delay for this call. - */ - function _detectExecutionRequirements(address target, bytes4 selector) private view returns (uint32 delay) { - (, delay) = AuthorityUtils.canCallWithDelay(address(_manager), address(this), target, selector); - } - /** * @dev Returns whether the operation at an index is delayed by the manager, and its scheduling nonce once queued. */ - function _getManagerData(ExecutionPlan storage plan, uint256 index) private view returns (bool, uint32) { + function _getManagerData( + ExecutionPlan storage plan, + uint256 index + ) private view returns (bool controlled, bool withDelay, uint32 nonce) { (uint256 bucket, uint256 subindex) = _getManagerDataIndices(index); - uint32 nonce = plan.managerData[bucket][subindex]; + uint32 value = plan.managerData[bucket][subindex]; unchecked { - return nonce > 0 ? (true, nonce - 1) : (false, 0); + return (value > 0, value > 1, value > 1 ? value - 2 : 0); } } /** - * @dev Marks an operation at an index as delayed by the manager, and sets its scheduling nonce. + * @dev Marks an operation at an index as permissioned by the manager, potentially delayed, and + * when delayed sets its scheduling nonce. */ - function _setManagerData(ExecutionPlan storage plan, uint256 index, uint32 nonce) private { + function _setManagerData(ExecutionPlan storage plan, uint256 index, bool withDelay, uint32 nonce) private { (uint256 bucket, uint256 subindex) = _getManagerDataIndices(index); - plan.managerData[bucket][subindex] = nonce + 1; + plan.managerData[bucket][subindex] = withDelay ? nonce + 2 : 1; } /** diff --git a/test/governance/extensions/GovernorTimelockAccess.test.js b/test/governance/extensions/GovernorTimelockAccess.test.js index c6e230a31..59ddf6dac 100644 --- a/test/governance/extensions/GovernorTimelockAccess.test.js +++ b/test/governance/extensions/GovernorTimelockAccess.test.js @@ -5,6 +5,7 @@ const Enums = require('../../helpers/enums'); const { GovernorHelper, proposalStatesToBitMap } = require('../../helpers/governance'); const { expectRevertCustomError } = require('../../helpers/customError'); const { clockFromReceipt } = require('../../helpers/time'); +const { selector } = require('../../helpers/methods'); const AccessManager = artifacts.require('$AccessManager'); const Governor = artifacts.require('$GovernorTimelockAccessMock'); @@ -210,36 +211,196 @@ contract('GovernorTimelockAccess', function (accounts) { await expectEvent.inTransaction(txExecute.tx, this.receiver, 'CalledUnrestricted'); }); - it('cancellation after queue (internal)', async function () { + describe('cancel', function () { const delay = 1000; const roleId = '1'; - await this.manager.setTargetFunctionRole(this.receiver.address, [this.restricted.selector], roleId, { - from: admin, - }); - await this.manager.grantRole(roleId, this.mock.address, delay, { from: admin }); - - this.proposal = await this.helper.setProposal([this.restricted.operation], 'descr'); - - await this.helper.propose(); - await this.helper.waitForSnapshot(); - await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); - await this.helper.waitForDeadline(); - await this.helper.queue(); - - const txCancel = await this.helper.cancel('internal'); - expectEvent(txCancel, 'ProposalCanceled', { proposalId: this.proposal.id }); - await expectEvent.inTransaction(txCancel.tx, this.manager, 'OperationCanceled', { - operationId: this.restricted.operationId, - nonce: '1', + beforeEach(async function () { + await this.manager.setTargetFunctionRole(this.receiver.address, [this.restricted.selector], roleId, { + from: admin, + }); + await this.manager.grantRole(roleId, this.mock.address, delay, { from: admin }); }); - await this.helper.waitForEta(); - await expectRevertCustomError(this.helper.execute(), 'GovernorUnexpectedProposalState', [ - this.proposal.id, - Enums.ProposalState.Canceled, - proposalStatesToBitMap([Enums.ProposalState.Succeeded, Enums.ProposalState.Queued]), - ]); + it('cancellation after queue (internal)', async function () { + this.proposal = await this.helper.setProposal([this.restricted.operation], 'descr'); + + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await this.helper.waitForDeadline(); + await this.helper.queue(); + + const txCancel = await this.helper.cancel('internal'); + expectEvent(txCancel, 'ProposalCanceled', { proposalId: this.proposal.id }); + await expectEvent.inTransaction(txCancel.tx, this.manager, 'OperationCanceled', { + operationId: this.restricted.operationId, + nonce: '1', + }); + + await this.helper.waitForEta(); + await expectRevertCustomError(this.helper.execute(), 'GovernorUnexpectedProposalState', [ + this.proposal.id, + Enums.ProposalState.Canceled, + proposalStatesToBitMap([Enums.ProposalState.Succeeded, Enums.ProposalState.Queued]), + ]); + }); + + it('cancel calls already canceled by guardian', async function () { + const operationA = { target: this.receiver.address, data: this.restricted.selector + '00' }; + const operationB = { target: this.receiver.address, data: this.restricted.selector + '01' }; + const operationC = { target: this.receiver.address, data: this.restricted.selector + '02' }; + const operationAId = hashOperation(this.mock.address, operationA.target, operationA.data); + const operationBId = hashOperation(this.mock.address, operationB.target, operationB.data); + + const proposal1 = new GovernorHelper(this.mock, mode); + const proposal2 = new GovernorHelper(this.mock, mode); + proposal1.setProposal([operationA, operationB], 'proposal A+B'); + proposal2.setProposal([operationA, operationC], 'proposal A+C'); + + for (const p of [proposal1, proposal2]) { + await p.propose(); + await p.waitForSnapshot(); + await p.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await p.waitForDeadline(); + } + + // Can queue the first proposal + await proposal1.queue(); + + // Cannot queue the second proposal: operation A already scheduled with delay + await expectRevertCustomError(proposal2.queue(), 'AccessManagerAlreadyScheduled', [operationAId]); + + // Admin cancels operation B on the manager + await this.manager.cancel(this.mock.address, operationB.target, operationB.data, { from: admin }); + + // Still cannot queue the second proposal: operation A already scheduled with delay + await expectRevertCustomError(proposal2.queue(), 'AccessManagerAlreadyScheduled', [operationAId]); + + await proposal1.waitForEta(); + + // Cannot execute first proposal: operation B has been canceled + await expectRevertCustomError(proposal1.execute(), 'AccessManagerNotScheduled', [operationBId]); + + // Cancel the first proposal to release operation A + await proposal1.cancel('internal'); + + // can finally queue the second proposal + await proposal2.queue(); + + await proposal2.waitForEta(); + + // Can execute second proposal + await proposal2.execute(); + }); + }); + + describe('ignore AccessManager', function () { + it('defaults', async function () { + expect(await this.mock.isAccessManagerIgnored(this.receiver.address, this.restricted.selector)).to.equal( + false, + ); + expect(await this.mock.isAccessManagerIgnored(this.mock.address, '0x12341234')).to.equal(true); + }); + + it('internal setter', async function () { + const p1 = { target: this.receiver.address, selector: this.restricted.selector, ignored: true }; + const tx1 = await this.mock.$_setAccessManagerIgnored(p1.target, p1.selector, p1.ignored); + expect(await this.mock.isAccessManagerIgnored(p1.target, p1.selector)).to.equal(p1.ignored); + expectEvent(tx1, 'AccessManagerIgnoredSet', p1); + + const p2 = { target: this.mock.address, selector: '0x12341234', ignored: false }; + const tx2 = await this.mock.$_setAccessManagerIgnored(p2.target, p2.selector, p2.ignored); + expect(await this.mock.isAccessManagerIgnored(p2.target, p2.selector)).to.equal(p2.ignored); + expectEvent(tx2, 'AccessManagerIgnoredSet', p2); + }); + + it('external setter', async function () { + const setAccessManagerIgnored = (...args) => + this.mock.contract.methods.setAccessManagerIgnored(...args).encodeABI(); + + await this.helper.setProposal( + [ + { + target: this.mock.address, + data: setAccessManagerIgnored( + this.receiver.address, + [this.restricted.selector, this.unrestricted.selector], + true, + ), + value: '0', + }, + { + target: this.mock.address, + data: setAccessManagerIgnored(this.mock.address, ['0x12341234', '0x67896789'], false), + value: '0', + }, + ], + 'descr', + ); + + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await this.helper.waitForDeadline(); + const tx = await this.helper.execute(); + + expectEvent(tx, 'AccessManagerIgnoredSet'); + + expect(await this.mock.isAccessManagerIgnored(this.receiver.address, this.restricted.selector)).to.equal( + true, + ); + expect(await this.mock.isAccessManagerIgnored(this.receiver.address, this.unrestricted.selector)).to.equal( + true, + ); + + expect(await this.mock.isAccessManagerIgnored(this.mock.address, '0x12341234')).to.equal(false); + expect(await this.mock.isAccessManagerIgnored(this.mock.address, '0x67896789')).to.equal(false); + }); + + it('locked function', async function () { + const setAccessManagerIgnored = selector('setAccessManagerIgnored(address,bytes4[],bool)'); + await expectRevertCustomError( + this.mock.$_setAccessManagerIgnored(this.mock.address, setAccessManagerIgnored, true), + 'GovernorLockedIgnore', + [], + ); + await this.mock.$_setAccessManagerIgnored(this.receiver.address, setAccessManagerIgnored, true); + }); + + it('ignores access manager', async function () { + const amount = 100; + + const target = this.token.address; + const data = this.token.contract.methods.transfer(voter4, amount).encodeABI(); + const selector = data.slice(0, 10); + await this.token.$_mint(this.mock.address, amount); + + const roleId = '1'; + await this.manager.setTargetFunctionRole(target, [selector], roleId, { from: admin }); + await this.manager.grantRole(roleId, this.mock.address, 0, { from: admin }); + + await this.helper.setProposal([{ target, data, value: '0' }], '1'); + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await this.helper.waitForDeadline(); + await expectRevertCustomError(this.helper.execute(), 'ERC20InsufficientBalance', [ + this.manager.address, + 0, + amount, + ]); + + await this.mock.$_setAccessManagerIgnored(target, selector, true); + + await this.helper.setProposal([{ target, data, value: '0' }], '2'); + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await this.helper.waitForDeadline(); + const tx = await this.helper.execute(); + expectEvent.inTransaction(tx, this.token, 'Transfer', { from: this.mock.address }); + }); }); }); }