Issuance allocator audit responses (for Graph_IssuanceAllocator_v02) #1272
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Addresses TRST-M-3 audit finding by documenting the mathematical relationship between self-minting and allocator-minting. Changes: - Add comprehensive documentation of 5 key invariants in IssuanceAllocator.sol proving total issuance never exceeds Σ(issuancePerBlock_b) for any period - Remove duplicate sections (Events, Error Conditions, Storage) from IssuanceAllocator.md that are better documented in NatSpec - Add WIP Initial Setup deployment sequence to IssuanceAllocator.md
Addresses TRST-M-3 audit finding. Events for offset adjustments of accumulation and during pending distribution, emitting whenever offset changes and showing both before and after states for complete accounting transparency.
Addresses TRST-L-5 by adding configurable event emission to prevent potential gas limit deadlock in _advanceSelfMintingBlock(). Adds three modes controllable by governance: - None (0): Skip event emission entirely (lowest gas) - Aggregate (1): Single aggregated event (medium gas) - PerTarget (2): Per-target events (highest gas, default) Defaults to PerTarget to provide event visibility by default. Governance can switch to None or Aggregate to reduce gas costs as target count grows. Implementation: - Add SelfMintingEventMode enum to IIssuanceAllocatorTypes - Add selfMintingEventMode field to storage struct - Add IssuanceSelfMintAllowanceAggregate event - Add SelfMintingEventModeUpdated event - Add setSelfMintingEventMode(mode) governance function - Add getSelfMintingEventMode() view function - Modify _advanceSelfMintingBlock() to respect emission mode - Initialize to PerTarget in initialize()
…adeable version Replace ReentrancyGuardTransientUpgradeable (deprecated in OpenZeppelin 5.5+) with ReentrancyGuardTransient. The non-upgradeable version is appropriate since the guard only uses transient storage and has no upgradeable state. Resolves TRST-R-10
Add comments clarifying that accRewardsPending tracking is incomplete due to _presentPOI clearing pending even when rewards are not consumed. Addresses audit finding TRST-L-4 as acknowledged limitation.
Issuance allocator audit responses (for Graph_IssuanceAllocator_v02)
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## issuance-allocator-3 #1272 +/- ##
========================================================
+ Coverage 86.30% 86.41% +0.11%
========================================================
Files 45 45
Lines 2373 2393 +20
Branches 708 714 +6
========================================================
+ Hits 2048 2068 +20
Misses 325 325
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
RembrandtK
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request implements audit responses for the IssuanceAllocator contract (Graph_IssuanceAllocator_v02), adding configurable event emission modes for gas optimization, improved offset tracking with visibility events, and comprehensive documentation enhancements.
Key changes:
- Introduced
SelfMintingEventModeenum (None, Aggregate, PerTarget) to allow governance to control event emission behavior based on gas cost requirements - Added
SelfMintingOffsetReconciledandSelfMintingOffsetAccumulatedevents for complete visibility into offset accounting during pause/distribution cycles - Enhanced documentation with detailed accounting invariants, reclaim target hierarchy, and retroactive address change behavior
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/issuance/contracts/allocate/IssuanceAllocator.sol | Core implementation: added SelfMintingEventMode functionality, offset reconciliation events, transient reentrancy guard, accounting invariants documentation, and various typo/clarity fixes |
| packages/interfaces/contracts/issuance/allocate/IIssuanceAllocatorTypes.sol | Added SelfMintingEventMode enum with gas cost documentation |
| packages/interfaces/contracts/issuance/allocate/IIssuanceAllocationAdministration.sol | Added setSelfMintingEventMode() and getSelfMintingEventMode() interface methods |
| packages/issuance/test/tests/allocate/SelfMintingEventMode.test.ts | Comprehensive test coverage for all three event modes, mode switching, edge cases, and gas optimization verification |
| packages/issuance/test/tests/allocate/IssuanceAllocator.test.ts | Added test verifying SelfMintingOffsetReconciled event is not emitted when offset unchanged |
| packages/issuance/test/tests/allocate/InterfaceIdStability.test.ts | Updated interface ID for IIssuanceAllocationAdministration to reflect new methods |
| packages/issuance/contracts/test/allocate/IssuanceAllocatorTestHarness.sol | Added @Custom:oz-upgrades-unsafe-allow constructor annotation |
| packages/issuance/contracts/allocate/IssuanceAllocator.md | Updated deployment documentation and removed broken target references |
| packages/subgraph-service/contracts/utilities/AllocationManager.sol | Documented reclaim target hierarchy and retroactive reclaim address change behavior |
| packages/interfaces/contracts/contracts/rewards/IRewardsManager.sol | Documented retroactive behavior of setReclaimAddress() |
| packages/contracts/contracts/rewards/RewardsManager.sol | Documented retroactive behavior of setReclaimAddress() implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/interfaces/contracts/issuance/allocate/IIssuanceAllocatorTypes.sol
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Address open issues and recommendations in Graph_IssuanceAllocator_v02.
TRST-M-3 More tokens could be issued than what is tracked by the accumulation of all distribution periods (8f5c074, f1337d9)
selfMintingOffset.selfMintingOffsetrelated event logging more complete (f1337d9) for better tracking.TRST-L-4 Historic pending rewards could display wrong values in several cases (468f176)
TRST-L-5 Key issuance operations could become permanently unreachable if gas limit is reached (aedce7c, f283815)
TRST-R-8 Improve safety of reclaim address modification
TRST-R-9 Improve documentation (aedce7c)
TRST-R-10 Avoid legacy library integrations (32c6809)
TRST-R-11 Avoid dead code
TRST-R-12 Mixing of different self-minting pause restrictions is incompatible with IssuanceAllocator