From 932d68f3c2b2dfca650acf525281914caf623cd2 Mon Sep 17 00:00:00 2001 From: Julian R <56316686+julianmrodri@users.noreply.github.com> Date: Tue, 24 Oct 2023 13:49:37 -0300 Subject: [PATCH 1/3] validations on votingDelay --- contracts/plugins/governance/Governance.sol | 21 ++- test/Governance.test.ts | 172 +++++++++++++++++++- 2 files changed, 187 insertions(+), 6 deletions(-) diff --git a/contracts/plugins/governance/Governance.sol b/contracts/plugins/governance/Governance.sol index c20978fa02..b36a78b692 100644 --- a/contracts/plugins/governance/Governance.sol +++ b/contracts/plugins/governance/Governance.sol @@ -8,6 +8,7 @@ import "@openzeppelin/contracts/governance/extensions/GovernorTimelockControl.so import "@openzeppelin/contracts/governance/extensions/GovernorVotes.sol"; import "@openzeppelin/contracts/governance/extensions/GovernorVotesQuorumFraction.sol"; import "../../interfaces/IStRSRVotes.sol"; +import "../../libraries/NetworkConfigLib.sol"; /* * @title Governance @@ -30,7 +31,9 @@ contract Governance is // 100% uint256 public constant ONE_HUNDRED_PERCENT = 1e8; // {micro %} - // solhint-disable no-empty-blocks + // solhint-disable-next-line var-name-mixedcase + uint256 public immutable MIN_VOTING_DELAY; // in blocks, based on network + constructor( IStRSRVotes token_, TimelockController timelock_, @@ -44,7 +47,12 @@ contract Governance is GovernorVotes(IVotes(address(token_))) GovernorVotesQuorumFraction(quorumPercent) GovernorTimelockControl(timelock_) - {} + { + MIN_VOTING_DELAY = + (86400 + NetworkConfigLib.blocktime() - 1) / + NetworkConfigLib.blocktime(); // 1 day, in blocks + requireValidVotingDelay(votingDelay_); + } // solhint-enable no-empty-blocks @@ -56,6 +64,11 @@ contract Governance is return super.votingPeriod(); } + function setVotingDelay(uint256 newVotingDelay) public override { + requireValidVotingDelay(newVotingDelay); + super.setVotingDelay(newVotingDelay); // has onlyGovernance modifier + } + /// @return {qStRSR} The number of votes required in order for a voter to become a proposer function proposalThreshold() public @@ -175,4 +188,8 @@ contract Governance is uint256 currentEra = IStRSRVotes(address(token)).currentEra(); return currentEra == pastEra; } + + function requireValidVotingDelay(uint256 newVotingDelay) private view { + require(newVotingDelay >= MIN_VOTING_DELAY, "invalid votingDelay"); + } } diff --git a/test/Governance.test.ts b/test/Governance.test.ts index 83dffb413a..53b7f7d2f1 100644 --- a/test/Governance.test.ts +++ b/test/Governance.test.ts @@ -59,8 +59,8 @@ describeP1(`Governance - P${IMPLEMENTATION}`, () => { let initialBal: BigNumber const MIN_DELAY = 7 * 60 * 60 * 24 // 7 days - const VOTING_DELAY = 5 // 5 blocks - const VOTING_PERIOD = 100 // 100 blocks + const VOTING_DELAY = 7200 // 1 day (in blocks) + const VOTING_PERIOD = 21600 // 3 days (in blocks) const PROPOSAL_THRESHOLD = 1e6 // 1% const QUORUM_PERCENTAGE = 4 // 4% @@ -306,13 +306,39 @@ describeP1(`Governance - P${IMPLEMENTATION}`, () => { expect(await governor.supportsInterface(interfaceID._hex)).to.equal(true) }) + + it('Should perform validations on votingDelay at deployment', async () => { + // Attempt to deploy with 0 voting delay + await expect( + GovernorFactory.deploy( + stRSRVotes.address, + timelock.address, + bn(0), + VOTING_PERIOD, + PROPOSAL_THRESHOLD, + QUORUM_PERCENTAGE + ) + ).to.be.revertedWith('invalid votingDelay') + + // Attempt to deploy with voting delay below minium (1 day) + await expect( + GovernorFactory.deploy( + stRSRVotes.address, + timelock.address, + bn(2000), // less than 1 day + VOTING_PERIOD, + PROPOSAL_THRESHOLD, + QUORUM_PERCENTAGE + ) + ).to.be.revertedWith('invalid votingDelay') + }) }) describe('Proposals', () => { // Proposal details const newValue: BigNumber = bn('360') - const proposalDescription = 'Proposal #1 - Update Trading Delay to 360' - const proposalDescHash = ethers.utils.keccak256(ethers.utils.toUtf8Bytes(proposalDescription)) + let proposalDescription = 'Proposal #1 - Update Trading Delay to 360' + let proposalDescHash = ethers.utils.keccak256(ethers.utils.toUtf8Bytes(proposalDescription)) let encodedFunctionCall: string let stkAmt1: BigNumber let stkAmt2: BigNumber @@ -873,5 +899,143 @@ describeP1(`Governance - P${IMPLEMENTATION}`, () => { // Check role was granted expect(await main.hasRole(SHORT_FREEZER, other.address)).to.equal(true) }) + + it('Should allow to update GovernorSettings via governance', async () => { + // Attempt to update if not governance + await expect(governor.setVotingDelay(bn(14400))).to.be.revertedWith( + 'Governor: onlyGovernance' + ) + + // Attempt to update without governance process in place + await whileImpersonating(timelock.address, async (signer) => { + await expect(governor.connect(signer).setVotingDelay(bn(14400))).to.be.reverted + }) + + // Update votingDelay via proposal + encodedFunctionCall = governor.interface.encodeFunctionData('setVotingDelay', [ + VOTING_DELAY * 2, + ]) + proposalDescription = 'Proposal #2 - Update Voting Delay to double' + proposalDescHash = ethers.utils.keccak256(ethers.utils.toUtf8Bytes(proposalDescription)) + + // Check current value + expect(await governor.votingDelay()).to.equal(VOTING_DELAY) + + // Propose + const proposeTx = await governor + .connect(addr1) + .propose([governor.address], [0], [encodedFunctionCall], proposalDescription) + + const proposeReceipt = await proposeTx.wait(1) + const proposalId = proposeReceipt.events![0].args!.proposalId + + // Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Pending) + + // Advance time to start voting + await advanceBlocks(VOTING_DELAY + 1) + + // Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Active) + + const voteWay = 1 // for + + // vote + await governor.connect(addr1).castVote(proposalId, voteWay) + await advanceBlocks(1) + + // Advance time till voting is complete + await advanceBlocks(VOTING_PERIOD + 1) + + // Finished voting - Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Succeeded) + + // Queue propoal + await governor + .connect(addr1) + .queue([governor.address], [0], [encodedFunctionCall], proposalDescHash) + + // Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Queued) + + // Advance time required by timelock + await advanceTime(MIN_DELAY + 1) + await advanceBlocks(1) + + // Execute + await governor + .connect(addr1) + .execute([governor.address], [0], [encodedFunctionCall], proposalDescHash) + + // Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Executed) + + // Check value was updated + expect(await governor.votingDelay()).to.equal(VOTING_DELAY * 2) + }) + + it('Should perform validations on votingDelay when updating', async () => { + // Update via proposal - Invalid value + encodedFunctionCall = governor.interface.encodeFunctionData('setVotingDelay', [bn(7100)]) + proposalDescription = 'Proposal #2 - Update Voting Delay to invalid' + proposalDescHash = ethers.utils.keccak256(ethers.utils.toUtf8Bytes(proposalDescription)) + + // Check current value + expect(await governor.votingDelay()).to.equal(VOTING_DELAY) + + // Propose + const proposeTx = await governor + .connect(addr1) + .propose([governor.address], [0], [encodedFunctionCall], proposalDescription) + + const proposeReceipt = await proposeTx.wait(1) + const proposalId = proposeReceipt.events![0].args!.proposalId + + // Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Pending) + + // Advance time to start voting + await advanceBlocks(VOTING_DELAY + 1) + + // Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Active) + + const voteWay = 1 // for + + // vote + await governor.connect(addr1).castVote(proposalId, voteWay) + await advanceBlocks(1) + + // Advance time till voting is complete + await advanceBlocks(VOTING_PERIOD + 1) + + // Finished voting - Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Succeeded) + + // Queue propoal + await governor + .connect(addr1) + .queue([governor.address], [0], [encodedFunctionCall], proposalDescHash) + + // Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Queued) + + // Advance time required by timelock + await advanceTime(MIN_DELAY + 1) + await advanceBlocks(1) + + // Execute + await expect( + governor + .connect(addr1) + .execute([governor.address], [0], [encodedFunctionCall], proposalDescHash) + ).to.be.revertedWith('TimelockController: underlying transaction reverted') + + // Check proposal state, still queued + expect(await governor.state(proposalId)).to.equal(ProposalState.Queued) + + // Check value was not updated + expect(await governor.votingDelay()).to.equal(VOTING_DELAY) + }) }) }) From 8d73ed13dba6c1f612bdedb9ef6cb8859c25c30c Mon Sep 17 00:00:00 2001 From: Julian R <56316686+julianmrodri@users.noreply.github.com> Date: Tue, 24 Oct 2023 15:21:02 -0300 Subject: [PATCH 2/3] fix facadewrite tests --- test/FacadeWrite.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/FacadeWrite.test.ts b/test/FacadeWrite.test.ts index 97210ce749..9176c71ac0 100644 --- a/test/FacadeWrite.test.ts +++ b/test/FacadeWrite.test.ts @@ -195,8 +195,8 @@ describe('FacadeWrite contract', () => { // Set governance params govParams = { - votingDelay: bn(5), // 5 blocks - votingPeriod: bn(100), // 100 blocks + votingDelay: bn(7200), // 1 day + votingPeriod: bn(21600), // 3 days proposalThresholdAsMicroPercent: bn(1e6), // 1% quorumPercent: bn(4), // 4% timelockDelay: bn(60 * 60 * 24), // 1 day From 3fc949094e75e3f93ef39d230e901e7d5081c3cc Mon Sep 17 00:00:00 2001 From: Julian R <56316686+julianmrodri@users.noreply.github.com> Date: Wed, 25 Oct 2023 14:33:29 -0300 Subject: [PATCH 3/3] use constant --- contracts/plugins/governance/Governance.sol | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/contracts/plugins/governance/Governance.sol b/contracts/plugins/governance/Governance.sol index b36a78b692..96de231912 100644 --- a/contracts/plugins/governance/Governance.sol +++ b/contracts/plugins/governance/Governance.sol @@ -10,6 +10,8 @@ import "@openzeppelin/contracts/governance/extensions/GovernorVotesQuorumFractio import "../../interfaces/IStRSRVotes.sol"; import "../../libraries/NetworkConfigLib.sol"; +uint256 constant ONE_DAY = 86400; // {s} + /* * @title Governance * @dev Decentralized Governance for the Reserve Protocol. @@ -32,7 +34,7 @@ contract Governance is uint256 public constant ONE_HUNDRED_PERCENT = 1e8; // {micro %} // solhint-disable-next-line var-name-mixedcase - uint256 public immutable MIN_VOTING_DELAY; // in blocks, based on network + uint256 public immutable MIN_VOTING_DELAY; // {block} equal to ONE_DAY constructor( IStRSRVotes token_, @@ -49,8 +51,8 @@ contract Governance is GovernorTimelockControl(timelock_) { MIN_VOTING_DELAY = - (86400 + NetworkConfigLib.blocktime() - 1) / - NetworkConfigLib.blocktime(); // 1 day, in blocks + (ONE_DAY + NetworkConfigLib.blocktime() - 1) / + NetworkConfigLib.blocktime(); // ONE_DAY, in blocks requireValidVotingDelay(votingDelay_); }