diff --git a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol index 5208757dfc49c..42572a27be269 100644 --- a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol +++ b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol @@ -33,6 +33,7 @@ interface IProposalValidator is ISemver { error ProposalValidator_InvalidVotingModule(); error ProposalValidator_InvalidTotalBudget(); error ProposalValidator_AttestationCreatedAfterLastVotingCycle(); + error ProposalValidator_PreviousVotingCycleNotStarted(); event ProposalSubmitted( uint256 indexed proposalId, diff --git a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json index f2305002325f9..1ae822e53901f 100644 --- a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json @@ -849,6 +849,11 @@ "name": "ProposalValidator_InvalidVotingModule", "type": "error" }, + { + "inputs": [], + "name": "ProposalValidator_PreviousVotingCycleNotStarted", + "type": "error" + }, { "inputs": [], "name": "ProposalValidator_ProposalAlreadyApproved", diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index 17d6828ea7785..d4e56adb4f1d2 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -172,12 +172,8 @@ "sourceCodeHash": "0x9baa0f9e744cc0ecc61d0fade8bffc18321b228833ea0904dc645f3975be9ed1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0xfd63a8de6795e33cba8b47ce2cab24f09a003065292d74bdbe236161335b02ba", - "sourceCodeHash": "0xa774418504002f9f2aaff0333a534c65a46e8bb9d9d1e9408fbe1b3204b10462" - }, - "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0xfd63a8de6795e33cba8b47ce2cab24f09a003065292d74bdbe236161335b02ba", - "sourceCodeHash": "0xa774418504002f9f2aaff0333a534c65a46e8bb9d9d1e9408fbe1b3204b10462" + "initCodeHash": "0x07538dde1aeaef48d7a8dbe5f5abdc34d44167f3cfb62340f42f30aa1a955322", + "sourceCodeHash": "0x874017b054fa1593ddb9136fc92b1f827c4201fda11af930ee409e2c513028d0" }, "src/legacy/DeployerWhitelist.sol:DeployerWhitelist": { "initCodeHash": "0x53099379ed48b87f027d55712dbdd1da7d7099925426eb0531da9c0012e02c29", diff --git a/packages/contracts-bedrock/src/governance/ProposalValidator.sol b/packages/contracts-bedrock/src/governance/ProposalValidator.sol index a1d9a76d09cc6..5ca7d1673eafe 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -98,6 +98,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Thrown when the attestation was created after the last voting cycle. error ProposalValidator_AttestationCreatedAfterLastVotingCycle(); + /// @notice Thrown when trying to approve and the previous voting cycle has not started. + error ProposalValidator_PreviousVotingCycleNotStarted(); + /*////////////////////////////////////////////////////////////// EVENTS //////////////////////////////////////////////////////////////*/ @@ -640,6 +643,12 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { previousVotingCycle = proposal.votingCycle; } + // revert if the previous voting cycle has not started, we should only allow delegates + // to approve relative close to the proposals voting cycle + if (votingCycles[previousVotingCycle].startingTimestamp > block.timestamp) { + revert ProposalValidator_PreviousVotingCycleNotStarted(); + } + // validate the attestation _validateTopDelegateAttestation(_attestationUid, previousVotingCycle); @@ -1020,8 +1029,7 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { // since the attestations are updated daily we should only allow attestations // created before the last voting cycle of the proposal - // check if attestation was created after the previous voting cycle - if (attestation.time > previousVotingCycleData.startingTimestamp + previousVotingCycleData.duration) { + if (attestation.time > previousVotingCycleData.startingTimestamp) { revert ProposalValidator_AttestationCreatedAfterLastVotingCycle(); } diff --git a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol index be1305762c1ff..280d1fd2a421d 100644 --- a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol +++ b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol @@ -1883,10 +1883,12 @@ contract ProposalValidator_ApproveProposal_Test is ProposalValidator_Init { function setUp() public override { super.setUp(); - // create a new voting cycle + // create the previous voting cycle // cycle number decreased by 1 and start time CYCLE_DURATION before the current cycle vm.prank(owner); validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); + // warp to the start of the previous cycle + vm.warp(START_TIMESTAMP - DURATION); } function test_approveProposal_succeeds(uint256 _proposalId, uint8 proposalTypeValue) public { @@ -1904,6 +1906,11 @@ contract ProposalValidator_ApproveProposal_Test is ProposalValidator_Init { vm.expectEmit(address(validator)); emit ProposalApproved(_proposalId, topDelegate_A); + // warp to the start of current cycle if the proposal is ProtocolOrGovernorUpgrade + if (proposalType == ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade) { + vm.warp(START_TIMESTAMP); + } + // Approve the proposal, use the attestation of the top delegate that was created in setUp vm.prank(topDelegate_A); validator.approveProposal(_proposalId, topDelegateAttestation_A); @@ -1921,6 +1928,13 @@ contract ProposalValidator_ApproveProposal_Test is ProposalValidator_Init { contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { function setUp() public override { super.setUp(); + + // create the previous voting cycle + // cycle number decreased by 1 and start time CYCLE_DURATION before the current cycle + vm.prank(owner); + validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); + // warp to the start of the previous cycle + vm.warp(START_TIMESTAMP - DURATION); } function test_approveProposal_proposalDoesNotExist_reverts(uint256 _proposalId) public { @@ -1966,6 +1980,26 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { validator.approveProposal(_proposalId, topDelegateAttestation_A); } + function test_approveProposal_previousVotingCycleNotStarted_reverts( + uint256 _proposalId, + uint8 proposalTypeValue + ) + public + { + // Bound the proposal type to valid enum values (0-4) + proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); + ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); + // set proposal data so that the proposal exists + validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + + // warp before the start of the previous cycle so that it reverts + vm.warp(START_TIMESTAMP - DURATION - 1); + + vm.expectRevert(IProposalValidator.ProposalValidator_PreviousVotingCycleNotStarted.selector); + vm.prank(topDelegate_A); + validator.approveProposal(_proposalId, topDelegateAttestation_A); + } + function test_approveProposal_invalidVotingCycle_reverts( uint256 _proposalId, uint8 proposalTypeValue, @@ -1983,6 +2017,11 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // set proposal data so that the proposal exists validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, votingCycle); + // warp to start of current voting cycle if the proposal is ProtocolOrGovernorUpgrade + if (proposalType == ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade) { + vm.warp(START_TIMESTAMP); + } + vm.expectRevert(IProposalValidator.ProposalValidator_InvalidVotingCycle.selector); vm.prank(topDelegate_A); validator.approveProposal(_proposalId, topDelegateAttestation_A); @@ -2017,9 +2056,10 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // set proposal data so that the proposal exists validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); - // set the voting cycle data of the previous cycle - vm.prank(owner); - validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); + // warp to start of current voting cycle if the proposal is ProtocolOrGovernorUpgrade + if (proposalType == ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade) { + vm.warp(START_TIMESTAMP); + } vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestationSchema.selector); vm.prank(topDelegate_A); @@ -2032,9 +2072,10 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); // set proposal data so that the proposal exists validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); - // set the voting cycle data of the previous cycle - vm.prank(owner); - validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); + // warp to start of current voting cycle if the proposal is ProtocolOrGovernorUpgrade + if (proposalType == ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade) { + vm.warp(START_TIMESTAMP); + } // revoke the attestation vm.prank(owner); @@ -2050,6 +2091,42 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { validator.approveProposal(_proposalId, topDelegateAttestation_A); } + function test_approveProposal_attestationCreatedAfterPreviousVotingCycle_reverts( + uint256 _proposalId, + uint8 proposalTypeValue + ) + public + { + // Bound the proposal type to valid enum values (0-4) + proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); + ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); + + // create a new delegate and attestation + address _delegate = makeAddr("delegate"); + bytes32 _attestationUid; + + // create the attestation based on the proposal type + if (proposalType == ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade) { + // warp to after the start of the current voting cycle if the proposal is ProtocolOrGovernorUpgrade + // because this proposal can be submitted and approved outside of a voting cycle + vm.warp(START_TIMESTAMP + 1); + _attestationUid = _createTopDelegateAttestation(_delegate); + } else { + // warp to after the start of the previous cycle for an other proposal type + vm.warp(START_TIMESTAMP - DURATION + 1); + _attestationUid = _createTopDelegateAttestation(_delegate); + } + + // set proposal data so that the proposal exists + validator.setProposalData(_proposalId, _delegate, proposalType, false, 0, CYCLE_NUMBER); + + // warp to after the start of the current voting cycle + vm.warp(START_TIMESTAMP + 2); + vm.expectRevert(IProposalValidator.ProposalValidator_AttestationCreatedAfterLastVotingCycle.selector); + vm.prank(_delegate); + validator.approveProposal(_proposalId, _attestationUid); + } + function test_approveProposal_invalidAttestationCaller_reverts( uint256 _proposalId, uint8 proposalTypeValue, @@ -2066,9 +2143,10 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // Set mock proposal data of a random proposal in the validator contract validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); - // set the voting cycle data of the previous cycle - vm.prank(owner); - validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); + // warp to start of current voting cycle if the proposal is ProtocolOrGovernorUpgrade + if (proposalType == ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade) { + vm.warp(START_TIMESTAMP); + } // Expect the invalid attestation error to be reverted vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestation.selector); @@ -2088,9 +2166,10 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // Set mock proposal data of a random proposal in the validator contract validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); - // set the voting cycle data of the previous cycle - vm.prank(owner); - validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); + // warp to start of current voting cycle if the proposal is ProtocolOrGovernorUpgrade + if (proposalType == ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade) { + vm.warp(START_TIMESTAMP); + } // create an attestation with partial delegation vm.prank(owner); @@ -2130,9 +2209,10 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // Set mock proposal data of a random proposal in the validator contract validator.setProposalData(_proposalId, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); - // set the voting cycle data of the previous cycle - vm.prank(owner); - validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); + // warp to start of current voting cycle if the proposal is ProtocolOrGovernorUpgrade + if (proposalType == ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade) { + vm.warp(START_TIMESTAMP); + } // Expect the invalid attestation error to be reverted when attestation doesn't exist vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestation.selector);