From 5c02893efa9594cc4ba13a3c6646c45156e96ae2 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 19 Oct 2021 13:56:02 +0200 Subject: [PATCH 1/6] Add a relay mechanism in the governor --- contracts/governance/Governor.sol | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index f11287ca8b7..e064ddb5dd8 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -347,6 +347,14 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { return weight; } + /** + * @dev Relaying mechanism allowing the acting governor (possibly a timelock) to extract value and token sent to + * the governor by mistake. + */ + function relay(address target, uint256 value, bytes calldata data) external onlyGovernance { + Address.functionCallWithValue(target, data, value); + } + /** * @dev Address through which the governor executes action. Will be overloaded by module that execute actions * through another contract such as a timelock. From 91320f135590cc0b7ed3d9723731eea5c2d03e3a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 28 Oct 2021 16:43:52 +0200 Subject: [PATCH 2/6] add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a73672793a..6abb0f338a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ * `VestingWallet`: new contract that handles the vesting of Ether and ERC20 tokens following a customizable vesting schedule. ([#2748](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2748)) * `Governor`: enable receiving Ether when a Timelock contract is not used. ([#2748](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2748)) * `GovernorTimelockCompound`: fix ability to use Ether stored in the Timelock contract. ([#2748](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2748)) + * `Governor`: add a relay function to help recover assets sent to a timelock-using governor instance. ([#2926](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2926)) ## 4.3.2 (2021-09-14) From bd34a6714ca0f299dc195b4a4783dfbca78a8c9e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 28 Oct 2021 17:16:37 +0200 Subject: [PATCH 3/6] fix lint --- contracts/governance/Governor.sol | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index e064ddb5dd8..a95342acee2 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -351,7 +351,11 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { * @dev Relaying mechanism allowing the acting governor (possibly a timelock) to extract value and token sent to * the governor by mistake. */ - function relay(address target, uint256 value, bytes calldata data) external onlyGovernance { + function relay( + address target, + uint256 value, + bytes calldata data + ) external onlyGovernance { Address.functionCallWithValue(target, data, value); } From e49ac491a29f28b7cdf1467c8cfc1c49f152e7af Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Oct 2021 11:48:20 +0200 Subject: [PATCH 4/6] add testing --- CHANGELOG.md | 5 +- .../GovernorTimelockCompound.test.js | 53 ++++++++++++++++++- .../GovernorTimelockControl.test.js | 53 ++++++++++++++++++- 3 files changed, 108 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6abb0f338a3..45f961fec00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + + * `Governor`: add a relay function to help recover assets sent to a timelock-using governor instance. ([#2926](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2926)) + ## Unreleased * `Ownable`: add an internal `_transferOwnership(address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2568)) @@ -15,7 +19,6 @@ * `VestingWallet`: new contract that handles the vesting of Ether and ERC20 tokens following a customizable vesting schedule. ([#2748](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2748)) * `Governor`: enable receiving Ether when a Timelock contract is not used. ([#2748](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2748)) * `GovernorTimelockCompound`: fix ability to use Ether stored in the Timelock contract. ([#2748](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2748)) - * `Governor`: add a relay function to help recover assets sent to a timelock-using governor instance. ([#2926](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2926)) ## 4.3.2 (2021-09-14) diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js index 303cc97e973..bc4f15c99b5 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -21,7 +21,7 @@ function makeContractAddress (creator, nonce) { } contract('GovernorTimelockCompound', function (accounts) { - const [ admin, voter ] = accounts; + const [ admin, voter, other ] = accounts; const name = 'OZ-Governor'; // const version = '1'; @@ -328,6 +328,57 @@ contract('GovernorTimelockCompound', function (accounts) { runGovernorWorkflow(); }); + describe('relay', function () { + beforeEach(async function () { + await this.token.mint(this.mock.address, 1); + this.call = [ + this.token.address, + 0, + this.token.contract.methods.transfer(other, 1).encodeABI(), + ]; + }); + + it('protected', async function () { + await expectRevert( + this.mock.relay(...this.call), + 'Governor: onlyGovernance', + ); + }); + + describe('using workflow', function () { + beforeEach(async function () { + this.settings = { + proposal: [ + [ + this.mock.address, + ], + [ + web3.utils.toWei('0'), + ], + [ + this.mock.contract.methods.relay(...this.call).encodeABI(), + ], + '', + ], + voters: [ + { voter: voter, support: Enums.VoteType.For }, + ], + steps: { + queue: { delay: 7 * 86400 }, + }, + }; + + expect(await this.token.balanceOf(this.mock.address), 1); + expect(await this.token.balanceOf(other), 0); + }); + afterEach(async function () { + expect(await this.token.balanceOf(this.mock.address), 0); + expect(await this.token.balanceOf(other), 1); + }); + runGovernorWorkflow(); + }); + }); + describe('updateTimelock', function () { beforeEach(async function () { this.newTimelock = await Timelock.new(this.mock.address, 7 * 86400); diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index fca7bd53558..b6bbaccf285 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -16,7 +16,7 @@ const Governor = artifacts.require('GovernorTimelockControlMock'); const CallReceiver = artifacts.require('CallReceiverMock'); contract('GovernorTimelockControl', function (accounts) { - const [ voter ] = accounts; + const [ voter, other ] = accounts; const name = 'OZ-Governor'; // const version = '1'; @@ -320,6 +320,57 @@ contract('GovernorTimelockControl', function (accounts) { runGovernorWorkflow(); }); + describe('relay', function () { + beforeEach(async function () { + await this.token.mint(this.mock.address, 1); + this.call = [ + this.token.address, + 0, + this.token.contract.methods.transfer(other, 1).encodeABI(), + ]; + }); + + it('protected', async function () { + await expectRevert( + this.mock.relay(...this.call), + 'Governor: onlyGovernance', + ); + }); + + describe('using workflow', function () { + beforeEach(async function () { + this.settings = { + proposal: [ + [ + this.mock.address, + ], + [ + web3.utils.toWei('0'), + ], + [ + this.mock.contract.methods.relay(...this.call).encodeABI(), + ], + '', + ], + voters: [ + { voter: voter, support: Enums.VoteType.For }, + ], + steps: { + queue: { delay: 7 * 86400 }, + }, + }; + + expect(await this.token.balanceOf(this.mock.address), 1); + expect(await this.token.balanceOf(other), 0); + }); + afterEach(async function () { + expect(await this.token.balanceOf(this.mock.address), 0); + expect(await this.token.balanceOf(other), 1); + }); + runGovernorWorkflow(); + }); + }); + describe('updateTimelock', function () { beforeEach(async function () { this.newTimelock = await Timelock.new(3600, [], []); From e6f4a0a7f4379878c7110bc19f79a02af0543352 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 18 Nov 2021 16:55:13 +0100 Subject: [PATCH 5/6] Update contracts/governance/Governor.sol Co-authored-by: Francisco Giordano --- contracts/governance/Governor.sol | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index a95342acee2..f5c3e922b3f 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -348,8 +348,10 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { } /** - * @dev Relaying mechanism allowing the acting governor (possibly a timelock) to extract value and token sent to - * the governor by mistake. + * @dev Relays a transaction or function call to an arbitrary target. In cases where the governance executor + * is some contract other than the governor itself, like when using a timelock, this function can be invoked + * in a governance proposal to recover tokens or Ether that was sent to the governor contract by mistake. + * Note that if the executor is simply the governor itself, use of `relay` is redundant. */ function relay( address target, From 598152077fe8245f2398605c883f382d72a9fc00 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 30 Nov 2021 16:46:51 -0300 Subject: [PATCH 6/6] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45f961fec00..82ae16194be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased - * `Governor`: add a relay function to help recover assets sent to a timelock-using governor instance. ([#2926](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2926)) + * `Governor`: add a relay function to help recover assets sent to a governor that is not its own executor (e.g. when using a timelock). ([#2926](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2926)) ## Unreleased