IA: Audit fix/response for Issuance Allocator #1266
Closed
+1,306
−177
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
…eporting Modified getTotalAllocation() to exclude default allocation from reported totals when the default target is address(0), since address(0) cannot receive minting. This provides clearer reporting of actual allocated issuance while maintaining the internal 100% allocation invariant. - Updated getTotalAllocation() to check if default is address(0) - When default is address(0), subtract default allocation from reported totalAllocationPPM and allocatorMintingPPM - Internal 100% allocation invariant remains unchanged - Updated 12 tests across 3 test files to expect new reporting behavior - All 165 tests passing
Added two tests to achieve 100% branch coverage: - Test for TargetAddressCannotBeZero error when trying to set allocation for address(0) while default is a real address - Test for getTotalAllocation() when default is a real address (not address(0)) Branch coverage: 97.96% → 100% All files now at 100% coverage (statements, branches, functions, lines) Total tests: 165 → 167
Add two new addresses to RewardsManager V6 storage that allow denied rewards to be minted to designated reclaim addresses instead of being lost: - indexerEligibilityReclaimAddress: receives tokens denied due to indexer eligibility - subgraphDeniedReclaimAddress: receives tokens denied due to subgraph denylist Both addresses default to zero (disabled) and can be set by governor. When set to non-zero, denied rewards are minted to these addresses instead of being burned. Original denial events are always emitted, with additional reclaim events when minting occurs. Changes: - Add setIndexerEligibilityReclaimAddress() and setSubgraphDeniedReclaimAddress() setter functions - Add corresponding events for address changes and reclaim operations - Refactor takeRewards() denial logic into helper functions to maintain code quality - Update IRewardsManager interface (interface ID: 0x731e44f0) - Add comprehensive test coverage (12 tests)
RembrandtK
force-pushed
the
issuance-allocator-5
branch
from
d20735e to
3cc5fbc
Compare
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 implements audit fixes/responses for the Issuance Allocator, introducing two main features: a default allocation mechanism and reclaim addresses for denied rewards.
Key Changes:
- Default allocation mechanism: Maintains a 100% allocation invariant where unallocated portions automatically go to a default target (initially
address(0)) - Rewards reclaim addresses: Adds functionality to reclaim denied rewards by minting them to configured addresses instead of burning them
- Updated tests and interfaces: Comprehensive test coverage for new features and updated interface IDs
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
packages/issuance/contracts/allocate/IssuanceAllocator.sol |
Implements default allocation mechanism, auto-adjusts to maintain 100% invariant, skips minting to address(0) |
packages/interfaces/contracts/issuance/allocate/IIssuanceAllocationAdministration.sol |
Adds setDefaultAllocationAddress function to interface |
packages/contracts/contracts/rewards/RewardsManager.sol |
Implements reclaim address logic for denied rewards (eligibility and subgraph denylist) |
packages/contracts/contracts/rewards/RewardsManagerStorage.sol |
Adds storage for eligibility and subgraph reclaim addresses |
packages/interfaces/contracts/contracts/rewards/IRewardsManager.sol |
Adds setter functions for reclaim addresses |
packages/issuance/test/tests/allocate/DefaultAllocation.test.ts |
New comprehensive test suite for default allocation feature |
packages/contracts/test/tests/unit/rewards/rewards-reclaim.test.ts |
New test suite covering reclaim address functionality |
packages/issuance/test/tests/allocate/IssuanceAllocator.test.ts |
Updates all tests to account for default allocation at index 0 |
packages/issuance/test/tests/allocate/IssuanceSystem.test.ts |
Updates tests with correct allocation calculations |
packages/issuance/test/tests/allocate/optimizedFixtures.ts |
Updates reset helper to handle default allocation |
packages/issuance/test/tests/allocate/InterfaceIdStability.test.ts |
Updates interface ID for breaking change |
packages/contracts/test/tests/unit/rewards/rewards-interface.test.ts |
Updates interface ID for breaking change |
packages/subgraph-service/test/unit/mocks/MockRewardsManager.sol |
Adds mock functions for reclaim address setters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/issuance/test/tests/allocate/DefaultAllocation.test.ts
Outdated
Show resolved
Hide resolved
When changing the default allocation address, both the old and new addresses need to be notified via beforeIssuanceAllocationChange if they implement IIssuanceTarget. This ensures proper lifecycle management for affected targets.
Move address(0) checks before loading storage in both distribution loops to avoid unnecessary gas costs. This prevents loading AllocationTarget storage when the target is the zero address.
- Add note about O(n) performance in setDefaultAllocationAddress loop, explaining this is theoretical and other operations would hit gas limits first, with recovery mechanisms available - Fix grammar: "reentrancy looping changing" to "reentrancy from looping and changing" - Consolidate duplicate reentrancy comments into one detailed explanation in _setTargetAllocation and a shorter reference in _validateAndUpdateTotalAllocations - Clarify RewardsManagerStorage documentation: "mint tokens that would be denied" to "receive tokens denied due to indexer eligibility checks"
Rename test from "should report total allocation as 100%" to "should report total allocation as 0% when default is address(0)" to accurately reflect that the test expects 0% when the default allocation address is address(0).
Move address(0) check before loading storage in _notifyTarget to avoid unnecessary gas costs when the target is the zero address.
Contributor
Author
|
Replaced by: #1267 |
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.
Replaced by: #1267