Skip to content

fix: budget cap dos #453

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 5 commits into from
Jul 22, 2025
Merged

fix: budget cap dos #453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d006403
fix: budget cap dos
0xOneTony Jul 21, 2025
18fb7df
fix: invalid proposal case
0xOneTony Jul 21, 2025
563c8c9
fix: test
0xOneTony Jul 21, 2025
8546f56
fix: tests
0xOneTony Jul 21, 2025
3348e3f
fix: conflicts and upgrade proposal edge case
0xOneTony Jul 22, 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
3 changes: 2 additions & 1 deletion packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
10 changes: 10 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 @@ -101,6 +101,11 @@
"internalType": "address",
"name": "_delegate",
"type": "address"
},
{
"internalType": "bytes32",
"name": "_proposalHash",
"type": "bytes32"
}
],
"name": "canApproveProposal",
Expand Down Expand Up @@ -788,6 +793,11 @@
"name": "VotingCycleDataSet",
"type": "event"
},
{
"inputs": [],
"name": "ProposalValidator_AttestationCreatedAfterLastVotingCycle",
"type": "error"
},
{
"inputs": [],
"name": "ProposalValidator_AttestationExpired",
Expand Down
4 changes: 2 additions & 2 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 @@ -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",
Expand Down
52 changes: 47 additions & 5 deletions packages/contracts-bedrock/src/governance/ProposalValidator.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
//////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -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();
}

Expand All @@ -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;
Expand All @@ -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.
Expand Down Expand Up @@ -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)) {
Expand All @@ -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
Expand Down
76 changes: 71 additions & 5 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 @@ -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));
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -2102,27 +2145,50 @@ 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;
}

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
Expand Down