Conversation
🦋 Changeset detectedLatest commit: 91c1048 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 35 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughA bugfix correction to market logic's maximum supply validation in the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🚩 Treasury minting can push totalSupply above maxSupply
depositToTreasury(treasuryFee) at contracts/Market.sol:716 calls _mint(treasury, previewDeposit(fee)) (see contracts/MarketBase.sol:97), which increases totalSupply AFTER the max supply check at line 710. If a treasury fee is accrued during the deposit, the final totalSupply could exceed maxSupply. This is a pre-existing issue (the old code also checked before depositToTreasury), and treasury fees are typically small, but it means maxSupply is not a strict hard cap. Worth noting for protocol operators setting tight supply limits.
(Refers to line 716)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in the Market contract where the max supply check was double-counting shares in the afterDeposit hook. The changes ensure that the check correctly accounts for the updated total supply, and a new test case has been added to verify this behavior. Feedback points out an opportunity to improve gas efficiency by moving the supply check to the beforeDeposit hook, which would avoid unnecessary asset transfers when the transaction is destined to revert.
🐛 market: fix double-count in max supply check
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
4cedf4c to
91c1048
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## ito #801 +/- ##
========================================
Coverage 94.82% 94.82%
========================================
Files 31 31
Lines 2724 2724
Branches 354 457 +103
========================================
Hits 2583 2583
Misses 140 140
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests