Indexing payments management post audit fixes#1334
Merged
Conversation
…ion-start anchor getMaxNextClaim() under SCOPE_PENDING used block.timestamp as the window start, but update() applies new terms retroactively from _agreementCollectionStartAt and never advances lastCollectionAt. When (now - collectionStart) exceeded (rcau.endsAt - now), the pre-update envelope under-reserved escrow relative to the very next collect(), risking PaymentsEscrow.collect reverts on insufficient balance via RAM._reconcileAgreement. Switch the windowStart to _agreementCollectionStartAt for Accepted agreements (the only state where update() can promote the pending RCAU); other states keep block.timestamp since pending is structurally unreachable for promotion there. Adds three regression tests in getMaxNextClaim.t.sol asserting the envelope invariant (post-update collect amount <= pre-update getMaxNextClaim): deterministic never-collected path, post-prior-collect path, and a bounded fuzz across (elapsed, rate, maxSec, window).
…ents After the upcoming invariant fix, selfMintingOffset cycles 0 -> X -> 0 on every distributeIssuance call. Continuing to emit either event would force a choice between steady-state noise and increasingly arbitrary "is this transition meaningful" gating. Once the path collapses, the offset carries no observer-actionable signal anyway. The accounting consumers care about (self-minting + allocator-minting = expected) is already covered by IssuanceDistributed, IssuanceSelfMintAllowance / IssuanceSelfMintAllowanceAggregate, IssuancePerBlockUpdated, TargetAllocationUpdated, and Pausable's Paused / Unpaused. No tests reference the offset events.
Contributor
There was a problem hiding this comment.
Pull request overview
Updates issuance accounting and recurring-payment max-claim calculations to address post-audit edge cases where budgeting/envelope computations could diverge from actual mint/collect behavior.
Changes:
- Refactors
IssuanceAllocatorto use a single “pending distribution” path for allocator-minting and to maintain self-minting offset accumulation unconditionally. - Adds allocator distribution tests covering “no prior accumulated offset” scenarios and budgeting bounds.
- Fixes
RecurringCollector.getMaxNextClaim(..., SCOPE_PENDING)for pending updates by anchoring the max-claim window to the agreement’s actual collection start; adds regression + fuzz tests to enforce the envelope invariant acrossupdate().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/issuance/test/unit/allocator/distribution.t.sol |
Adds unit/fuzz coverage for pending distribution behavior when starting from a clean offset state. |
packages/issuance/contracts/allocate/IssuanceAllocator.sol |
Unifies distribution logic via _distributePendingIssuance, changes self-minting offset accumulation semantics, and removes offset-specific events. |
packages/horizon/test/unit/payments/recurring-collector/getMaxNextClaim.t.sol |
Adds regression + fuzz tests ensuring pre-update max-claim envelopes cover post-update collection amounts. |
packages/horizon/contracts/payments/collectors/RecurringCollector.sol |
Fixes pending-scope max-claim calculation to use the agreement’s true collection start for Accepted agreements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Indexing payments management post audit fixes
🚨 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 @@
## main #1334 +/- ##
==========================================
- Coverage 91.16% 91.16% -0.01%
==========================================
Files 81 81
Lines 5342 5318 -24
Branches 975 969 -6
==========================================
- Hits 4870 4848 -22
+ Misses 449 448 -1
+ Partials 23 22 -1
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:
|
Four tests covering the case where the governor calls distributePendingIssuance() with selfMintingOffset == 0 and a non-empty undistributed range with totalSelfMintingRate > 0: - DefaultGetsOwnRateOnly: full flush over-mints to default by totalSelfMintingRate * blocks. - MixedTargets: same with mixed allocator/self-minting allocations. - ToBlock_Partial: partial flush exceeds defaultRate * K; cumulative after subsequent flush should restore per-rate * total blocks. - StaysWithinAllocatorBudget fuzz: allocator-minted never exceeds (issuancePerBlock - totalSelfMintingRate) * blocks. All four fail. Fixed in the next commit.
…e distribution path The Self-Minting Accumulation invariant (selfMintingOffset = cumulative self-minting share over the undistributed range) was only conditionally maintained: _advanceSelfMintingBlock accumulated when paused() || 0 < offsetBefore. The pending math depends on it, and distributePendingIssuance reaches that math from states where the invariant silently fails - producing total minted > issuancePerBlock * blocks. Two coupled changes for full correctness: 1. Drop the conditional gate in _advanceSelfMintingBlock so the invariant holds in every reachable state. 2. Collapse _distributeIssuance's offset-vs-normal dispatch to delegate to _distributePendingIssuance, and delete _performNormalDistribution. It was reachable post-partial-flush (where the reconcile formula clamps offset to 0) and would mint per-rate without seeing that self-minting was already authorized for the post-partial range, under-distributing to allocator targets. The four bug-demonstration tests now pass.
Now a single ternary; the helper added no abstraction value. Inlining puts lastDistributionBlock and selfMintingOffset updates next to each other where the period accounting reads naturally end-to-end.
…gCollector Removes three errors that were declared but never thrown: - RecurringCollectorAgreementNotFound - RecurringCollectorInvalidPaymentType (and its IGraphPayments import) - RecurringCollectorZeroCollectionSeconds Not-collectable conditions go through RecurringCollectorAgreementNotCollectable with an AgreementNotCollectableReason; these three were stragglers from an earlier shape of the API.
RecurringCollector accept/update — describe the actual auth model: The empty-signature path on accept and update authorizes via stored offer hash (registered through IAgreementCollector.offer), not via a callback to a now-removed IAgreementOwner.approveAgreement. Update the natspec to describe the actual mechanism, capture the idempotent re-accept / re-apply paths, and move the deadline check out of the signature-conditional since it applies to both paths. Sweep a stale comment in the gas test along the way. Other correctness fixes: - IRecurringCollector.RecurringCollectorAgreementInvalidCollectionWindow: notice claimed 'elapsed endsAt' but the error fires on (max - min) below MIN_SECONDS_COLLECTION_WINDOW. Rewrite to match. - IRecurringCollector.cancel: notice said 'indexing agreement'; replace with 'Recurring Collection Agreement' and add a dev note covering caller, state preconditions, and pending-RCAU cleanup. - IRecurringAgreementManagement.ProviderRemoved: notice said 'balance zero', but tracking is dropped at balance < 2^minResidualEscrowFactor. Reword. - IRecurringAgreements.getTotalEscrowDeficit: described as a per-provider sum; it is per (collector, provider) pair. Match the storage docstring. Trim repetition: - RecurringCollector impls (collect/accept/cancel/update, pauseGuardians): drop @notice/@dev that just restated the interface; keep @inheritdoc plus any genuinely new @dev (only update has one — the pricing-applies-immediately note). pauseGuardians now uses @inheritdoc.
RembrandtK
force-pushed
the
indexing-payments-management-audit-fix-3
branch
from
7aab0cf to
12958ea
Compare
…eClaimsClaimNotFound revert Closes the two real coverage gaps from PR #1331: - RewardsManager.supportsInterface(IProviderEligibilityManagement.interfaceId) was the only OR-clause never asserted true. - StakeClaims.processStakeClaim's claim-not-found require was never exercised; reachable only via library misuse, so test directly through a thin harness with empty mappings.
Base automatically changed from
indexing-payments-management-audit-fixed-rebased
to
main
May 9, 2026 10:24
Hardhat (production deploy): - Toolshed base sets viaIR=true, runs=100, evm_version=cancun, solc 0.8.35, hardhat network hardfork pinned to cancun. Hardhat 2.28.6 (catalog ^2.28.0) for the upstream solc resolver fix; older versions mis-resolve 0.8.35 to its pre.1 build. - interfaces, horizon, data-edge bumped from older solc pins (0.8.27 / 0.8.12). - horizon and subgraph-service hardhat configs simplified to inherit cleanly. - RecurringCollector deployed bytecode 22.029 KiB (2.5 KiB EIP-170 headroom); ~10-15% reduction across deployable contracts vs the prior runs=20/paris baseline. Foundry: fast default, opt-in production: - [profile.default]: optimizer/viaIR off, evm_version=cancun, solc=0.8.35. Forge build 2-9s instead of ~4 min. - [profile.prod]: optimizer + runs=100 + viaIR for prod-matching CI runs. Root `pnpm test:prod` sets FOUNDRY_PROFILE=prod for the recursive run. - lint_on_build = false: forge 1.7's auto-lint duplicates pnpm lint:forge and infinite-loops on testing's pnpm-hoisted node_modules symlink cycle. - ignored_warnings_from = ["node_modules"] silences third-party warnings. Test changes induced by the compiler config: - HorizonStakingShared._slash split into _expectSlashEmits and _assertSlashStateAfter; block-scoping in AgreementLifecycle and FullStackHarness — legacy pipeline stack relief for [profile.prod]. - RecurringCollectorCallbackGas warm-path moved forge → hardhat (forge default is unoptimized, exceeds the assertion's 500-gas tolerance). - getMaxNextClaim PastEndsAt fuzz tests use vm.getBlockTimestamp() instead of `block.timestamp` capture — viaIR treats TIMESTAMP as pure and CSE-eliminates the local across vm.warp, re-reading post-warp. - Solc warning cleanups across test files (unused locals / params, missing view/pure, dead isDelegationSlashingEnabled in _slash). 1509 forge tests pass across all four packages under both profiles.
RembrandtK
force-pushed
the
indexing-payments-management-audit-fix-3
branch
from
7574652 to
8d148f3
Compare
tmigone
reviewed
May 12, 2026
Member
There was a problem hiding this comment.
What's the plan for these audits md files? Are they going to be replaced by the audit pdf report once we have it?
Contributor
Author
There was a problem hiding this comment.
Audit PDF is committed (in 22042cd). Likely will drop the MD files in a subsequent commit.
tmigone
approved these changes
May 12, 2026
Maikol
approved these changes
May 12, 2026
|
|
||
| /** | ||
| * @notice Advances self-minting block and emits allowance events | ||
| * @dev When paused, accumulates self-minting amounts. This accumulation reduces the allocator-minting |
Member
There was a problem hiding this comment.
nit: I believe we should remove the when paused after this fix
Contributor
Author
There was a problem hiding this comment.
Ah, yes. Techincally the wording should ideally change, although the drift is minor. For context I would not describe this as such a behaviour change, although internally it technically looks like it. The accumulation that now additionally happens when not paused is temporary accounting, undone within the same transaction. When paused it genuinely accumulates past the end of the transaction.
| * @dev Pause behavior: | ||
| * - When paused: Self-minting allowances tracked via events/accumulation, but no allocator-minting tokens distributed. | ||
| * Returns lastDistributionBlock (frozen at pause point). lastSelfMintingBlock advances to current block. | ||
| * - When unpaused: Normal distribution if no accumulated self-minting, otherwise retroactive distribution |
Member
There was a problem hiding this comment.
nit: we should update this to reflect the new behavior.
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
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.
Additional fixes since #1331.
IssuanceAllocator:distributePendingIssuanceover-issuanceThe Self-Minting Accumulation invariant (
selfMintingOffset= cumulative self-minting share over the undistributed range) was only conditionally maintained —_advanceSelfMintingBlockskipped accumulation unlesspaused() || 0 < offsetBefore.distributePendingIssuancereaches the dependent pending math from states where the invariant silently fails, producingissuancePerBlock * blocks < total minted.Fix:
_advanceSelfMintingBlockso the invariant holds always._distributeIssuanceinto_distributePendingIssuance; delete_performNormalDistribution. The deleted path was reachable post-partial-flush (reconcile clamps offset to 0) and re-minted per-rate, under-distributing to allocator targets.Demonstration tests (committed failing pre-fix) in distribution.t.sol.
Drive-bys: drop
SelfMintingOffset{Accumulated,Reconciled}events; inline_reconcileSelfMintingOffset.RecurringCollector: pending-RCAU max-claim window misalignmentgetMaxNextClaim()underSCOPE_PENDINGusedblock.timestampas window start, butupdate()applies new terms retroactively from_agreementCollectionStartAtand never advanceslastCollectionAt. When(rcau.endsAt - now) < (now - collectionStart), the pre-update envelope under-reserved escrow vs. the very nextcollect(), riskingPaymentsEscrow.collectreverts viaRAM._reconcileAgreement.Fix: use
_agreementCollectionStartAtaswindowStartforAcceptedagreements (the only state whereupdate()can promote the pending RCAU); other states keepblock.timestamp.Regression tests in getMaxNextClaim.t.sol assert
post-update collect <= pre-update getMaxNextClaim.Minor
IRecurringCollectorerrors (RecurringCollectorAgreementNotFound,RecurringCollectorInvalidPaymentType,RecurringCollectorZeroCollectionSeconds) and the now-redundantIGraphPaymentsimport.IRecurringCollector.accept/cancel/update(idempotency, contract-approval via stored offer hash),IRecurringAgreementManagement.PairUntracked(residual-threshold semantics),IRecurringAgreements.getTotalEscrowDeficit(per-pair, not per-provider), andRecurringCollectorAgreementInvalidCollectionWindow.Compiler config
Hardhat (production): toolshed base now
solc 0.8.35,viaIR=true,runs=100,evm_version=cancun(was 0.8.27, paris, no viaIR; horizon/subgraph-service inherit cleanly with no per-package overrides).RecurringCollectordeployed bytecode 24,598 B (over EIP-170 at the priorruns=20/paris) → 22,029 B (2.5 KiB headroom). ~10–15% reduction across deployable contracts.