Skip to content

Conversation

@0xOneTony
Copy link
Member

Closes OPT-949

@linear
Copy link

linear bot commented Jul 21, 2025

@0xOneTony 0xOneTony requested a review from 0xChin July 21, 2025 15:33
cursor[bot]

This comment was marked as outdated.

@0xOneTony 0xOneTony mentioned this pull request Jul 21, 2025
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

// validate the attestation
_validateTopDelegateAttestation(_attestationUid, _msgSender());
// proposal.votingCycle should never be 0, voting cycles already exist before the ProposalValidator is deployed
// and should be set by the OP Foundation
Copy link
Member

Choose a reason for hiding this comment

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

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Bug: Proposal Validation Mismatch in Voting Cycle

The canApproveProposal function uses an inconsistent voting cycle for attestation validation compared to approveProposal. For ProtocolOrGovernorUpgrade proposals, approveProposal correctly uses the proposal's current votingCycle for validation, while canApproveProposal incorrectly always uses proposal.votingCycle - 1. This discrepancy can lead to canApproveProposal returning misleading results for these proposal types.

packages/contracts-bedrock/src/governance/ProposalValidator.sol#L637-L654

/// @return canApprove_ True if the delegate can approve the proposal, false otherwise.
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);
}

Fix in CursorFix in Web


Was this report helpful? Give feedback by reacting with 👍 or 👎

@0xOneTony 0xOneTony merged commit 6638584 into fix/ir-findings Jul 22, 2025
3 checks passed
@0xOneTony 0xOneTony deleted the fix/budget-cap-dos branch July 22, 2025 20:14
0xOneTony added a commit that referenced this pull request Jul 29, 2025
* fix: budget cap dos

* fix: invalid proposal case

* fix: test

* fix: tests
0xOneTony added a commit that referenced this pull request Jul 29, 2025
* fix: budget cap dos

* fix: invalid proposal case

* fix: test

* fix: tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants