IA: Audit fix/response for Issuance Allocator #1267
Draft
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
The default allocation receives issuance that is not explicitly allocated to another target. - Added setDefaultAllocationAddress() to set. - getTotalAllocation() excludes default when it is the zero address. - Tests and documentation updated.
Added two configurable reclaim addresses: - indexerEligibilityReclaimAddress - subgraphDeniedReclaimAddress When rewards are denied and the reclaim address is set (non-zero), tokens are minted to that address instead of not being minted at all. Defaults to address(0) (original behavior). Also updated associated documentation and tests.
IA: Audit fix/response for Issuance Allocator
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds two enhancements to the Graph Protocol's issuance and rewards system:
Purpose: Enable better management of unallocated issuance and denied rewards through configurable reclaim mechanisms.
Key Changes:
- Issuance Allocator: Introduces a default allocation mechanism that automatically receives unallocated issuance portions, with address configurable via
setDefaultAllocationAddress() - Rewards Manager: Adds two reclaim addresses (
indexerEligibilityReclaimAddressandsubgraphDeniedReclaimAddress) that can receive rewards that would otherwise be denied, preventing token loss
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
IssuanceAllocator.sol |
Implements default allocation mechanism with 100% invariant, adds setDefaultAllocationAddress(), modifies allocation tracking to exclude default from reported totals when address(0) |
IIssuanceAllocationAdministration.sol |
Adds setDefaultAllocationAddress() interface method |
RewardsManager.sol |
Implements reclaim functionality with two helper functions to handle denied rewards, refactors takeRewards() to use them |
RewardsManagerStorage.sol |
Adds storage variables for reclaim addresses with documentation |
IRewardsManager.sol |
Adds interface methods for setting reclaim addresses |
MockRewardsManager.sol |
Adds mock implementations of new reclaim setter methods |
IssuanceAllocator.test.ts |
Updates all tests to account for default allocation always existing at index 0 |
IssuanceSystem.test.ts |
Updates tests with corrected total allocation expectations |
DefaultAllocation.test.ts |
New comprehensive test suite covering default allocation behavior, edge cases, and distribution scenarios |
rewards-reclaim.test.ts |
New comprehensive test suite covering reclaim address functionality for both denylist and eligibility scenarios |
optimizedFixtures.ts |
Updates reset helper to skip default allocation and reset it properly |
InterfaceIdStability.test.ts (both) |
Updates expected interface IDs for modified interfaces |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add clarity to availablePPM calculation explaining it comprises default allocation's allocator-minting PPM, target's allocator-minting PPM, and target's self-minting PPM to maintain 100% allocation invariant - Refine reentrancy comment to explicitly reference that calculations occur after notifications to prevent reentrancy issues Addresses PR feedback from code review
…wing (#1268) Replace vm.assume with bounded inputs to fix "vm.assume rejected too many inputs" error. The previous implementation used overly restrictive constraints that caused the fuzzer to reject most random inputs. Now limits amountThawing and amountCollected to half of MAX_STAKING_TOKENS, guaranteeing valid deposit ranges while maintaining test coverage.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## issuance-allocator-3 #1267 +/- ##
========================================================
+ Coverage 85.83% 86.14% +0.30%
========================================================
Files 45 45
Lines 2294 2345 +51
Branches 680 698 +18
========================================================
+ Hits 1969 2020 +51
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:
|
…address Change setDefaultAllocationAddress to distribute any pending issuance to the old default address before changing to the new address. Add evenIfDistributionPending parameter to handle the case when distribution cannot proceed (e.g., when paused). This allows callers to either: - Return false when distribution is pending (evenIfDistributionPending=false) - Force the change anyway (evenIfDistributionPending=true) Implementation: - Calls _handleDistributionBeforeAllocation before changing default address - Distributes pending issuance to old default address up to current block - Then changes allocation to new default address - When paused and evenIfDistributionPending=false, returns false instead of changing - Original setDefaultAllocationAddress(address) signature wraps new implementation with evenIfDistributionPending=false
Refactor _removeTargetFromList to _removeTarget to logically group target removal operations (array removal and mapping deletion) in a single function. Loop starts at index 1 to protect default allocation at index 0. Also fixes MILLION/PPM terminology, adds missing natspec, and adds test for address(0) default allocation.
When changing default allocation address, allocation data is copied from oldAddress to newAddress, which was overwriting newAddress's lastChangeNotifiedBlock. - If newAddress was just notified, the notification record was lost - If newAddress had a future block set (via forceTargetNoChangeNotificationBlock), that restriction was lost - When changing from address(0) (which is never notified), newAddress's lastChangeNotifiedBlock was incorrectly set to 0 Fix: preserve newAddress's lastChangeNotifiedBlock when copying allocation data. Also clarifies that notification blocks are target-specific, not about the default in general.
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.
This PR adds two enhancements:
Issuance Allocator: Default Allocation
The default allocation automatically receives whatever percentage is not explicitly allocated to other targets.
Added
setDefaultAllocationAddress()to configure where unallocated issuance is sent.Modified
getTotalAllocation()to exclude the default allocation if it is the zero address (as it will in this scenario not be minted).RewardsManager: Reclaim Addresses
Added two configurable reclaim addresses:
indexerEligibilityReclaimAddress- receives rewards denied due to eligibility oraclesubgraphDeniedReclaimAddress- receives rewards denied due to subgraph denylistWhen rewards are denied and a reclaim address is set (non-zero), tokens are minted to that address instead of not being minted at all. Defaults to
address(0)(original behavior).