diff --git a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol index f270847582df6..2682c9eb5a7dd 100644 --- a/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol +++ b/packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol @@ -31,6 +31,7 @@ interface IProposalValidator is ISemver { error ProposalValidator_InvalidProposer(); error ProposalValidator_InvalidProposal(); error ProposalValidator_InvalidVotingModule(); + error ProposalValidator_AttestationCreatedAfterLastVotingCycle(); event ProposalSubmitted( bytes32 indexed proposalHash, @@ -130,7 +131,7 @@ interface IProposalValidator is ISemver { function approveProposal(bytes32 _proposalHash, bytes32 _attestationUid) external; - function canApproveProposal(bytes32 _attestationUid, address _delegate) external view returns (bool canApprove_); + function canApproveProposal(bytes32 _attestationUid, address _delegate, bytes32 _proposalHash) external view returns (bool canApprove_); function moveToVoteProtocolOrGovernorUpgradeProposal( uint248 _againstThreshold, diff --git a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json index 044e2188e955f..9c41904e7fdd9 100644 --- a/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json +++ b/packages/contracts-bedrock/snapshots/abi/ProposalValidator.json @@ -101,6 +101,11 @@ "internalType": "address", "name": "_delegate", "type": "address" + }, + { + "internalType": "bytes32", + "name": "_proposalHash", + "type": "bytes32" } ], "name": "canApproveProposal", @@ -788,6 +793,11 @@ "name": "VotingCycleDataSet", "type": "event" }, + { + "inputs": [], + "name": "ProposalValidator_AttestationCreatedAfterLastVotingCycle", + "type": "error" + }, { "inputs": [], "name": "ProposalValidator_AttestationExpired", diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index fe9a198219386..cef9eab6c8552 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -176,8 +176,8 @@ "sourceCodeHash": "0x18f43b227decd0f2a895b8b55e23fa6a47706697c272bbf2482b3f912be446e1" }, "src/governance/ProposalValidator.sol:ProposalValidator": { - "initCodeHash": "0x33e740162106c353e5d9f550c67d47286846b80a91f042f5b600703e4ed0b42c", - "sourceCodeHash": "0xb51010203b3a894896a1e70c999f858d6d8e937c08feb301642e93f9787081e5" + "initCodeHash": "0xd359b54afbf8f0bfcde0e24b648d20f1eecdc306b511d3c0cd90afa5b61382ac", + "sourceCodeHash": "0x6da6042e1bc89da33dad2ce39aaef685f2a67c04e4693f69c2693b349899d131" }, "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 704bfd2653f9f..537d808e31434 100644 --- a/packages/contracts-bedrock/src/governance/ProposalValidator.sol +++ b/packages/contracts-bedrock/src/governance/ProposalValidator.sol @@ -92,6 +92,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Thrown when the voting module address is invalid (zero address). error ProposalValidator_InvalidVotingModule(); + /// @notice Thrown when the attestation was created after the last voting cycle. + error ProposalValidator_AttestationCreatedAfterLastVotingCycle(); + /*////////////////////////////////////////////////////////////// EVENTS //////////////////////////////////////////////////////////////*/ @@ -591,7 +594,9 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { address _delegate = _msgSender(); ProposalData storage proposal = _proposals[_proposalHash]; // check if the proposal exists - if (proposal.proposer == address(0)) { + // proposal.votingCycle should never be 0, voting cycles already exist before the ProposalValidator is deployed + // and should be set by the OP Foundation + if (proposal.proposer == address(0) || proposal.votingCycle == 0) { revert ProposalValidator_ProposalDoesNotExist(); } @@ -605,8 +610,17 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_ProposalAlreadyMovedToVote(); } + // The previous voting cycle of a proposal should be the one before the + // proposal's targetted voting cycle. + uint256 previousVotingCycle = proposal.votingCycle - 1; + // Proposal or Governor Upgrade proposals are submitted with the latest voting cycle number, + // because they can be submitted outside of a voting cycle. + if (proposal.proposalType == ProposalType.ProtocolOrGovernorUpgrade) { + previousVotingCycle = proposal.votingCycle; + } + // validate the attestation - _validateTopDelegateAttestation(_attestationUid, _msgSender()); + _validateTopDelegateAttestation(_attestationUid, _msgSender(), previousVotingCycle); // store the approval proposal.delegateApprovals[_delegate] = true; @@ -618,9 +632,25 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @notice Checks if a delegate can approve a proposal. /// @dev Helper function for UI integration. /// @param _attestationUid The UID of the attestation to check. + /// @param _delegate The delegate to check the attestation for. + /// @param _proposalHash The hash of the proposal to check the attestation for. /// @return canApprove_ True if the delegate can approve the proposal, false otherwise. - function canApproveProposal(bytes32 _attestationUid, address _delegate) external view returns (bool canApprove_) { - canApprove_ = _validateTopDelegateAttestation(_attestationUid, _delegate); + function canApproveProposal( + bytes32 _attestationUid, + address _delegate, + bytes32 _proposalHash + ) + external + view + returns (bool canApprove_) + { + // TODO: this function should be fixed in OPT-957 + ProposalData storage proposal = _proposals[_proposalHash]; + if (proposal.votingCycle == 0) { + return false; + } + + canApprove_ = _validateTopDelegateAttestation(_attestationUid, _delegate, proposal.votingCycle - 1); } /// @notice Moves a Protocol or Governor Upgrade proposal to vote by proposing it on the Governor. @@ -955,13 +985,18 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { /// @return canApprove_ True if the attestation is valid, false otherwise. function _validateTopDelegateAttestation( bytes32 _attestationUid, - address _delegate + address _delegate, + uint256 _lastVotingCycle ) internal view returns (bool canApprove_) { Attestation memory attestation = IEAS(Predeploys.EAS).getAttestation(_attestationUid); + VotingCycleData memory previousVotingCycleData = votingCycles[_lastVotingCycle]; + if (previousVotingCycleData.startingTimestamp == 0) { + revert ProposalValidator_InvalidVotingCycle(); + } // Check if attestation exists, equivalent to calling EAS.isAttestationValid(_attestationUid) if (attestation.uid == bytes32(0)) { @@ -978,6 +1013,13 @@ contract ProposalValidator is OwnableUpgradeable, ReinitializableBase, ISemver { revert ProposalValidator_AttestationRevoked(); } + // 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) { + revert ProposalValidator_AttestationCreatedAfterLastVotingCycle(); + } + (, bool _includePartialDelegation,) = abi.decode(attestation.data, (string, bool, string)); // check if the attestation includes partial delegation or the recipient is not the caller diff --git a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol index b70fb6e43a730..7e88af159b627 100644 --- a/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol +++ b/packages/contracts-bedrock/test/governance/ProposalValidator.t.sol @@ -1884,6 +1884,15 @@ contract ProposalValidator_SubmitFundingProposal_TestFail is ProposalValidator_I /// @title ProposalValidator_ApproveProposal_Test /// @notice Happy path tests for approveProposal function contract ProposalValidator_ApproveProposal_Test is ProposalValidator_Init { + function setUp() public override { + super.setUp(); + + // create a new 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); + } + function test_approveProposal_succeeds(bytes32 _proposalHash, uint8 proposalTypeValue) public { // Ensure the proposal hash is not 0 vm.assume(_proposalHash != bytes32(0)); @@ -1961,6 +1970,25 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { validator.approveProposal(_proposalHash, topDelegateAttestation_A); } + function test_approveProposal_invalidVotingCycle_reverts( + bytes32 _proposalHash, + uint8 proposalTypeValue, + uint256 votingCycle + ) + public + { + vm.assume(votingCycle != CYCLE_NUMBER); + // 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(_proposalHash, topDelegate_A, proposalType, false, 0, votingCycle); + + vm.expectRevert(IProposalValidator.ProposalValidator_InvalidVotingCycle.selector); + vm.prank(topDelegate_A); + validator.approveProposal(_proposalHash, topDelegateAttestation_A); + } + function test_approveProposal_invalidSchema_reverts(bytes32 _proposalHash, uint8 proposalTypeValue) public { // Bound the proposal type to valid enum values (0-4) proposalTypeValue = uint8(bound(proposalTypeValue, 0, 4)); @@ -1990,6 +2018,9 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // set proposal data so that the proposal exists validator.setProposalData(_proposalHash, 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); vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestationSchema.selector); vm.prank(topDelegate_A); @@ -2002,6 +2033,9 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { ProposalValidator.ProposalType proposalType = ProposalValidator.ProposalType(proposalTypeValue); // set proposal data so that the proposal exists validator.setProposalData(_proposalHash, 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); // revoke the attestation vm.prank(owner); @@ -2033,6 +2067,9 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // Set mock proposal data of a random proposal in the validator contract validator.setProposalData(_proposalHash, 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); // Expect the invalid attestation error to be reverted vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestation.selector); @@ -2052,6 +2089,9 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // Set mock proposal data of a random proposal in the validator contract validator.setProposalData(_proposalHash, 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); // create an attestation with partial delegation vm.prank(owner); @@ -2091,6 +2131,9 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { // Set mock proposal data of a random proposal in the validator contract validator.setProposalData(_proposalHash, 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); // Expect the invalid attestation error to be reverted when attestation doesn't exist vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestation.selector); @@ -2102,20 +2145,34 @@ contract ProposalValidator_ApproveProposal_TestFail is ProposalValidator_Init { /// @title ProposalValidator_CanApproveProposal_Test /// @notice Tests for the canApproveProposal function contract ProposalValidator_CanApproveProposal_Test is ProposalValidator_Init { - function test_canApproveProposal_returnTrue_succeeds() public view { + function test_canApproveProposal_returnTrue_succeeds(bytes32 _proposalHash, 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 the voting cycle data of the previous cycle + validator.setProposalData(_proposalHash, topDelegate_A, proposalType, false, 0, CYCLE_NUMBER); + vm.prank(owner); + validator.setVotingCycleData(CYCLE_NUMBER - 1, START_TIMESTAMP - DURATION, DURATION, DISTRIBUTION_THRESHOLD); + // Attestation already created in setUp - bool canApprove = validator.canApproveProposal(topDelegateAttestation_A, topDelegate_A); + bool canApprove = validator.canApproveProposal(topDelegateAttestation_A, topDelegate_A, _proposalHash); assertTrue(canApprove); } - function test_canApproveProposal_returnFalse_succeeds(bytes32 attestationUid, address delegate) public { + function test_canApproveProposal_returnFalseRevert_succeeds( + bytes32 _attestationUid, + address _delegate, + bytes32 _proposalHash + ) + public + { // Ensure the attestation uid is not one of the top delegates - vm.assume(attestationUid != topDelegateAttestation_A); + vm.assume(_attestationUid != topDelegateAttestation_A); bool canApprove; // Expect the invalid attestation error to be reverted vm.expectRevert(IProposalValidator.ProposalValidator_InvalidAttestation.selector); - try validator.canApproveProposal(attestationUid, delegate) returns (bool result_) { + try validator.canApproveProposal(_attestationUid, _delegate, _proposalHash) returns (bool result_) { canApprove = result_; } catch { canApprove = false; @@ -2123,6 +2180,15 @@ contract ProposalValidator_CanApproveProposal_Test is ProposalValidator_Init { assertEq(canApprove, false); } + + function test_canApproveProposal_returnFalseProposalNotFound_reverts(bytes32 _proposalHash) public { + validator.setProposalData( + _proposalHash, topDelegate_A, ProposalValidator.ProposalType.ProtocolOrGovernorUpgrade, false, 0, 0 + ); + + bool canApprove = validator.canApproveProposal(topDelegateAttestation_A, topDelegate_A, _proposalHash); + assertEq(canApprove, false); + } } /// @title ProposalValidator_MoveToVoteProtocolOrGovernorUpgradeProposal_Test