Skip to content

fix: add approval timing check #472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 8, 2025
Merged

fix: add approval timing check #472

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
45b6628
fix: add approval timing check
0xOneTony Aug 6, 2025
030b599
fix: add missing test
0xOneTony Aug 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
5 changes: 5 additions & 0 deletions packages/contracts-bedrock/snapshots/abi/ProposalValidator.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,6 +849,11 @@
"name": "ProposalValidator_InvalidVotingModule",
"type": "error"
},
{
"inputs": [],
"name": "ProposalValidator_PreviousVotingCycleNotStarted",
"type": "error"
},
{
"inputs": [],
"name": "ProposalValidator_ProposalAlreadyApproved",
Expand Down
8 changes: 2 additions & 6 deletions packages/contracts-bedrock/snapshots/semver-lock.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
11 changes: 10 additions & 1 deletion packages/contracts-bedrock/src/governance/ProposalValidator.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
//////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -1021,7 +1030,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
Copy link
Member

@agusduha agusduha Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we fix this comment too?

Copy link
Member Author

@0xOneTony 0xOneTony Aug 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the last comment, I think this is good, wdyt?

        // since the attestations are updated daily we should only allow attestations
        // created before the last voting cycle of the proposal

if (attestation.time > previousVotingCycleData.startingTimestamp + previousVotingCycleData.duration) {
if (attestation.time > previousVotingCycleData.startingTimestamp) {
revert ProposalValidator_AttestationCreatedAfterLastVotingCycle();
}

Expand Down
76 changes: 60 additions & 16 deletions packages/contracts-bedrock/test/governance/ProposalValidator.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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);
Expand All @@ -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 {
Expand Down Expand Up @@ -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,
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -2066,9 +2107,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);
Expand All @@ -2088,9 +2130,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);
Expand Down Expand Up @@ -2130,9 +2173,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);
Expand Down