-
Notifications
You must be signed in to change notification settings - Fork 12.3k
fix(governance): add underflow protection in vote override subtraction #6122
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.
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
base: master
Are you sure you want to change the base?
fix(governance): add underflow protection in vote override subtraction #6122
Conversation
|
WalkthroughThe GovernorCountingOverridable contract was modified to enhance vote handling during overrides. A new error, Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/governance/extensions/GovernorCountingOverridable.sol (1)
158-162: Excellent defensive programming for a critical governance contract.The underflow protection is well-implemented and follows Solidity best practices. While the existing logic should theoretically prevent
currentVotesfrom being less thanoverriddenWeight, this explicit check:
- Protects against potential bugs in vote accounting logic
- Provides a clear, actionable error message if something unexpected occurs
- Has minimal gas overhead (one SLOAD and comparison)
- Is appropriate for a security-critical governance contract
The change enhances the contract's robustness without altering its intended behavior.
Optional future consideration: A similar underflow check could be added at line 124 in
_countVotewhereoverriddenWeightis subtracted fromtotalWeight. While the existing logic should prevent underflow there as well, adding a check would provide consistent defensive programming throughout the contract.// Example of potential future enhancement at line 124 if (totalWeight < proposalVote.voteReceipt[account].overriddenWeight) { revert GovernorInsufficientVotingPower(totalWeight, proposalVote.voteReceipt[account].overriddenWeight); } totalWeight -= proposalVote.voteReceipt[account].overriddenWeight;This is not required for the current PR but could be considered in a follow-up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/governance/extensions/GovernorCountingOverridable.sol(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Redirect rules - solidity-contracts
- GitHub Check: Header rules - solidity-contracts
- GitHub Check: slither
- GitHub Check: coverage
- GitHub Check: tests-foundry
- GitHub Check: tests-upgradeable
- GitHub Check: tests
- GitHub Check: halmos
- GitHub Check: Pages changed - solidity-contracts
🔇 Additional comments (1)
contracts/governance/extensions/GovernorCountingOverridable.sol (1)
48-49: LGTM! Well-designed error declaration.The error declaration follows best practices with clear naming and informative parameters. The
availableandrequiredparameters will help diagnose any issues if this error is triggered.
Add explicit check to prevent underflow when subtracting overriddenWeight
from delegate votes. The check ensures votes[delegateSupport] >= overriddenWeight
before subtraction and reverts with GovernorInsufficientDelegateVotes error
if the invariant is violated.
This protects against edge cases where multiple override votes could theoretically
cause the delegate vote count to become insufficient, even though the current
logic should prevent this scenario.