From 4ac1628330791067a181a711373f20a14fc1f9a7 Mon Sep 17 00:00:00 2001 From: Joey <31974730+Joeysantoro@users.noreply.github.com> Date: Sun, 3 Apr 2022 18:13:28 -0700 Subject: [PATCH 1/9] Update TimelockController.sol --- contracts/governance/TimelockController.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 376023fe35b..dc3ae1d6513 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -352,7 +352,7 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver address target, uint256 value, bytes calldata data - ) private { + ) internal virtual { (bool success, ) = target.call{value: value}(data); require(success, "TimelockController: underlying transaction reverted"); From b9d39c51c2978954a8f677bc2e17e108056efeb6 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 4 Apr 2022 17:08:07 +0200 Subject: [PATCH 2/9] further refactor --- contracts/governance/TimelockController.sol | 46 +++++++++++---------- test/governance/TimelockController.test.js | 40 +++++++++++++++--- 2 files changed, 59 insertions(+), 27 deletions(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index dc3ae1d6513..a0e32597468 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -6,6 +6,7 @@ pragma solidity ^0.8.0; import "../access/AccessControl.sol"; import "../token/ERC721/IERC721Receiver.sol"; import "../token/ERC1155/IERC1155Receiver.sol"; +import "../utils/Address.sol"; /** * @dev Contract module which acts as a timelocked controller. When set as the @@ -288,13 +289,15 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver function execute( address target, uint256 value, - bytes calldata data, + bytes calldata payload, bytes32 predecessor, bytes32 salt ) public payable virtual onlyRoleOrOpenRole(EXECUTOR_ROLE) { - bytes32 id = hashOperation(target, value, data, predecessor, salt); + bytes32 id = hashOperation(target, value, payload, predecessor, salt); + _beforeCall(id, predecessor); - _call(id, 0, target, value, data); + _execute(target, value, payload); + emit CallExecuted(id, 0, target, value, payload); _afterCall(id); } @@ -318,13 +321,30 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver require(targets.length == payloads.length, "TimelockController: length mismatch"); bytes32 id = hashOperationBatch(targets, values, payloads, predecessor, salt); + _beforeCall(id, predecessor); for (uint256 i = 0; i < targets.length; ++i) { - _call(id, i, targets[i], values[i], payloads[i]); + address target = targets[i]; + uint256 value = values[i]; + bytes calldata payload = payloads[i]; + _execute(target, value, payload); + emit CallExecuted(id, i, target, value, payload); } _afterCall(id); } + /** + * @dev Execute an operation's call. + */ + function _execute( + address target, + uint256 value, + bytes calldata data + ) internal virtual { + (bool success, bytes memory returndata) = target.call{value: value}(data); + Address.verifyCallResult(success, returndata, "TimelockController: underlying transaction reverted"); + } + /** * @dev Checks before execution of an operation's calls. */ @@ -341,24 +361,6 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver _timestamps[id] = _DONE_TIMESTAMP; } - /** - * @dev Execute an operation's call. - * - * Emits a {CallExecuted} event. - */ - function _call( - bytes32 id, - uint256 index, - address target, - uint256 value, - bytes calldata data - ) internal virtual { - (bool success, ) = target.call{value: value}(data); - require(success, "TimelockController: underlying transaction reverted"); - - emit CallExecuted(id, index, target, value, data); - } - /** * @dev Changes the minimum timelock duration for future operations. * diff --git a/test/governance/TimelockController.test.js b/test/governance/TimelockController.test.js index 4584b2252e1..94dd6f3ffee 100644 --- a/test/governance/TimelockController.test.js +++ b/test/governance/TimelockController.test.js @@ -634,7 +634,7 @@ contract('TimelockController', function (accounts) { ], [ this.callreceivermock.contract.methods.mockFunction().encodeABI(), - this.callreceivermock.contract.methods.mockFunctionThrows().encodeABI(), + this.callreceivermock.contract.methods.mockFunctionRevertsNoReason().encodeABI(), this.callreceivermock.contract.methods.mockFunction().encodeABI(), ], ZERO_BYTES32, @@ -886,6 +886,38 @@ contract('TimelockController', function (accounts) { ); }); + it('call reverting with message', async function () { + const operation = genOperation( + this.callreceivermock.address, + 0, + this.callreceivermock.contract.methods.mockFunctionRevertsReason().encodeABI(), + ZERO_BYTES32, + '0xb1b1b276fdf1a28d1e00537ea73b04d56639128b08063c1a2f70a52e38cba693', + ); + + await this.mock.schedule( + operation.target, + operation.value, + operation.data, + operation.predecessor, + operation.salt, + MINDELAY, + { from: proposer }, + ); + await time.increase(MINDELAY); + await expectRevert( + this.mock.execute( + operation.target, + operation.value, + operation.data, + operation.predecessor, + operation.salt, + { from: executor }, + ), + 'CallReceiverMock: reverting', + ); + }); + it('call throw', async function () { const operation = genOperation( this.callreceivermock.address, @@ -905,7 +937,7 @@ contract('TimelockController', function (accounts) { { from: proposer }, ); await time.increase(MINDELAY); - await expectRevert( + await expectRevert.unspecified( this.mock.execute( operation.target, operation.value, @@ -914,7 +946,6 @@ contract('TimelockController', function (accounts) { operation.salt, { from: executor }, ), - 'TimelockController: underlying transaction reverted', ); }); @@ -937,7 +968,7 @@ contract('TimelockController', function (accounts) { { from: proposer }, ); await time.increase(MINDELAY); - await expectRevert( + await expectRevert.outOfGas( this.mock.execute( operation.target, operation.value, @@ -946,7 +977,6 @@ contract('TimelockController', function (accounts) { operation.salt, { from: executor, gas: '70000' }, ), - 'TimelockController: underlying transaction reverted', ); }); From 2a564fe7120d4fa4de6ecc6485b6c6af1dec0053 Mon Sep 17 00:00:00 2001 From: Joey <31974730+Joeysantoro@users.noreply.github.com> Date: Mon, 4 Apr 2022 10:14:40 -0700 Subject: [PATCH 3/9] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3394576a22..11edda560c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ * `Governor`: Implement `IERC721Receiver` and `IERC1155Receiver` to improve token custody by governors. ([#3230](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3230)) * `TimelockController`: Implement `IERC721Receiver` and `IERC1155Receiver` to improve token custody by timelocks. ([#3230](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3230)) * `TimelockController`: Add a separate canceller role for the ability to cancel. ([#3165](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3165)) + * `TimelockController`: Migrate `_call` to `_execute` and allow inheritance similar to `Governor`. ([#3317](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3317)) * `Initializable`: add a reinitializer modifier that enables the initialization of new modules, added to already initialized contracts through upgradeability. ([#3232](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3232)) * `Initializable`: add an Initialized event that tracks initialized version numbers. ([#3294](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3294)) From e861f2867e15f7287f31b5498ed6ba554ba7fdbb Mon Sep 17 00:00:00 2001 From: Joey <31974730+Joeysantoro@users.noreply.github.com> Date: Mon, 4 Apr 2022 10:16:28 -0700 Subject: [PATCH 4/9] Update contracts/governance/TimelockController.sol --- contracts/governance/TimelockController.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index a0e32597468..4119e6c722f 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -341,7 +341,7 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver uint256 value, bytes calldata data ) internal virtual { - (bool success, bytes memory returndata) = target.call{value: value}(data); + (bool success, bytes memory returndata) = target.call{value: value}(data); Address.verifyCallResult(success, returndata, "TimelockController: underlying transaction reverted"); } From 14a0ac3b58c1fd82eeba4c98885f89ae34a85125 Mon Sep 17 00:00:00 2001 From: Joey <31974730+Joeysantoro@users.noreply.github.com> Date: Thu, 7 Apr 2022 17:29:40 -0700 Subject: [PATCH 5/9] Update contracts/governance/TimelockController.sol --- contracts/governance/TimelockController.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 4119e6c722f..cbb1f96e1e6 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -342,7 +342,7 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver bytes calldata data ) internal virtual { (bool success, bytes memory returndata) = target.call{value: value}(data); - Address.verifyCallResult(success, returndata, "TimelockController: underlying transaction reverted"); + require(success, "TimelockController: underlying transaction reverted"); } /** From 486d41b4679cb0c6f52f54765dc51886091c725d Mon Sep 17 00:00:00 2001 From: Joey <31974730+Joeysantoro@users.noreply.github.com> Date: Thu, 7 Apr 2022 17:33:15 -0700 Subject: [PATCH 6/9] Undo test changes --- test/governance/TimelockController.test.js | 40 +++------------------- 1 file changed, 5 insertions(+), 35 deletions(-) diff --git a/test/governance/TimelockController.test.js b/test/governance/TimelockController.test.js index 94dd6f3ffee..4584b2252e1 100644 --- a/test/governance/TimelockController.test.js +++ b/test/governance/TimelockController.test.js @@ -634,7 +634,7 @@ contract('TimelockController', function (accounts) { ], [ this.callreceivermock.contract.methods.mockFunction().encodeABI(), - this.callreceivermock.contract.methods.mockFunctionRevertsNoReason().encodeABI(), + this.callreceivermock.contract.methods.mockFunctionThrows().encodeABI(), this.callreceivermock.contract.methods.mockFunction().encodeABI(), ], ZERO_BYTES32, @@ -886,38 +886,6 @@ contract('TimelockController', function (accounts) { ); }); - it('call reverting with message', async function () { - const operation = genOperation( - this.callreceivermock.address, - 0, - this.callreceivermock.contract.methods.mockFunctionRevertsReason().encodeABI(), - ZERO_BYTES32, - '0xb1b1b276fdf1a28d1e00537ea73b04d56639128b08063c1a2f70a52e38cba693', - ); - - await this.mock.schedule( - operation.target, - operation.value, - operation.data, - operation.predecessor, - operation.salt, - MINDELAY, - { from: proposer }, - ); - await time.increase(MINDELAY); - await expectRevert( - this.mock.execute( - operation.target, - operation.value, - operation.data, - operation.predecessor, - operation.salt, - { from: executor }, - ), - 'CallReceiverMock: reverting', - ); - }); - it('call throw', async function () { const operation = genOperation( this.callreceivermock.address, @@ -937,7 +905,7 @@ contract('TimelockController', function (accounts) { { from: proposer }, ); await time.increase(MINDELAY); - await expectRevert.unspecified( + await expectRevert( this.mock.execute( operation.target, operation.value, @@ -946,6 +914,7 @@ contract('TimelockController', function (accounts) { operation.salt, { from: executor }, ), + 'TimelockController: underlying transaction reverted', ); }); @@ -968,7 +937,7 @@ contract('TimelockController', function (accounts) { { from: proposer }, ); await time.increase(MINDELAY); - await expectRevert.outOfGas( + await expectRevert( this.mock.execute( operation.target, operation.value, @@ -977,6 +946,7 @@ contract('TimelockController', function (accounts) { operation.salt, { from: executor, gas: '70000' }, ), + 'TimelockController: underlying transaction reverted', ); }); From bd50defa47be1eb7e4f176d3cfb91fc1c83df455 Mon Sep 17 00:00:00 2001 From: Joey <31974730+Joeysantoro@users.noreply.github.com> Date: Thu, 7 Apr 2022 17:36:01 -0700 Subject: [PATCH 7/9] remove unused param --- contracts/governance/TimelockController.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index cbb1f96e1e6..cef21d6cd18 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -341,7 +341,7 @@ contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver uint256 value, bytes calldata data ) internal virtual { - (bool success, bytes memory returndata) = target.call{value: value}(data); + (bool success, ) = target.call{value: value}(data); require(success, "TimelockController: underlying transaction reverted"); } From a1548f606d5b4ad72121423dd70343349ae8a62c Mon Sep 17 00:00:00 2001 From: Joey <31974730+Joeysantoro@users.noreply.github.com> Date: Fri, 8 Apr 2022 11:17:42 -0700 Subject: [PATCH 8/9] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11edda560c9..279615e5123 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,7 @@ * `Governor`: Implement `IERC721Receiver` and `IERC1155Receiver` to improve token custody by governors. ([#3230](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3230)) * `TimelockController`: Implement `IERC721Receiver` and `IERC1155Receiver` to improve token custody by timelocks. ([#3230](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3230)) * `TimelockController`: Add a separate canceller role for the ability to cancel. ([#3165](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3165)) - * `TimelockController`: Migrate `_call` to `_execute` and allow inheritance similar to `Governor`. ([#3317](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3317)) + * `TimelockController`: Migrate `_call` to `_execute` and allow inheritance and overriding similar to `Governor`. ([#3317](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3317)) * `Initializable`: add a reinitializer modifier that enables the initialization of new modules, added to already initialized contracts through upgradeability. ([#3232](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3232)) * `Initializable`: add an Initialized event that tracks initialized version numbers. ([#3294](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3294)) From 66bf559085a2bd016ed208e4c1da5cfac8724430 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 8 Apr 2022 17:05:21 -0300 Subject: [PATCH 9/9] move changelog entry to newer section --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 279615e5123..ae7826d50e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Unreleased * `ERC2981`: make `royaltiInfo` public to allow super call in overrides. ([#3305](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3305)) + * `TimelockController`: Migrate `_call` to `_execute` and allow inheritance and overriding similar to `Governor`. ([#3317](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3317)) ## Unreleased @@ -22,7 +23,6 @@ * `Governor`: Implement `IERC721Receiver` and `IERC1155Receiver` to improve token custody by governors. ([#3230](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3230)) * `TimelockController`: Implement `IERC721Receiver` and `IERC1155Receiver` to improve token custody by timelocks. ([#3230](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3230)) * `TimelockController`: Add a separate canceller role for the ability to cancel. ([#3165](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3165)) - * `TimelockController`: Migrate `_call` to `_execute` and allow inheritance and overriding similar to `Governor`. ([#3317](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3317)) * `Initializable`: add a reinitializer modifier that enables the initialization of new modules, added to already initialized contracts through upgradeability. ([#3232](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3232)) * `Initializable`: add an Initialized event that tracks initialized version numbers. ([#3294](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3294))