Deployment/testnet/2024-05-08/gip-0088#1336
Closed
RembrandtK wants to merge 151 commits intomainfrom
Closed
Conversation
Fixes TRST-M-1 audit finding: Wrong TYPEHASH string is used for agreement updates, limiting functionality. * Fixed EIP712_RCAU_TYPEHASH to use correct uint64 types for deadline and endsAt fields (was incorrectly using uint256) * This prevents signature verification failures for RecurringCollectionAgreementUpdate
Fixes TRST-M-2 audit finding: Collection for an elapsed or canceled agreement could be wrong due to temporal calculation inconsistencies between IndexingAgreement and RecurringCollector layers. * Replace isCollectable() with getCollectionInfo() that returns both collectability and duration * Make RecurringCollector the single source of truth for temporal logic * Update IndexingAgreement to call getCollectionInfo() once and pass duration to _tokensToCollect()
Fixes signature replay attack vulnerability where old signed RecurringCollectionAgreementUpdate messages could be replayed to revert agreements to previous terms. ## Changes - Add `nonce` field to RecurringCollectionAgreementUpdate struct (uint32) - Add `updateNonce` field to AgreementData struct to track current nonce - Add nonce validation in RecurringCollector.update() to ensure sequential updates - Update EIP712_RCAU_TYPEHASH to include nonce field - Add comprehensive tests for nonce validation and replay attack prevention - Add RecurringCollectorInvalidUpdateNonce error for invalid nonce attempts ## Implementation Details - Nonces start at 0 when agreement is accepted - Each update must use current nonce + 1 - Nonce is incremented after successful update - Uses uint32 for gas optimization (supports 4B+ updates per agreement) - Single source of truth: nonce stored in AgreementData struct
Implements slippage protection mechanism to prevent silent token loss during rate-limited collections in RecurringCollector agreements. The implementation uses type(uint256).max convention to disable slippage checks, providing users full control over acceptable token loss during rate limiting. Resolves audit finding TRST-L-5: "RecurringCollector silently reduces collected tokens without user consent"
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
…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.
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.
…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.
Hardhat (production deploy): - Toolshed base now sets viaIR=true, runs=100, evm_version=cancun, solc 0.8.34. Bumps interfaces, horizon, data-edge from older solc pins (0.8.27 / 0.8.12). 0.8.35 is released but Hardhat 2.26.3 mis-resolves it to the pre.1 binary; defer until upstream fix. - horizon and subgraph-service hardhat configs simplified to inherit cleanly. - RecurringCollector deployed bytecode: 24,598 B (over EIP-170 at the prior runs=20/paris) → 22,029 B (2.5 KiB headroom). ~10-15% reduction across deployable contracts. Foundry: fast default, opt-in production: - [profile.default]: optimizer/viaIR off, evm_version=cancun, solc=0.8.34. Forge build 2-9s instead of ~4 min. - [profile.prod]: optimizer + runs=100 + viaIR for prod-matching CI runs (FOUNDRY_PROFILE=prod forge test). - 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). - Solc warning cleanups across test files (unused locals / params, missing view/pure, dead isDelegationSlashingEnabled in _slash). 1509 forge tests pass across all four packages.
Squash of the GIP-0088 deployment package introduction and its three foundational follow-ups: - feat: GIP-0088 deployment infrastructure (deploy/ scripts, configs, ABI generation) - fix: register governor as rocketh named account for local/test TX signing - fix(ignition): update HorizonStaking and SubgraphService modules for localNetwork - chore(tag-deployment): drop commit sha from tag annotation
…tions in RCA decoder)
Squash of 4 commits: - feat: read revertOnIneligible from network config - chore: remove vestigial RecurringCollector block from network configs - refactor: introduce ResolvedSettings to centralise config resolution - feat: bundle RM.setRevertOnIneligible into upgrade governance batch Wires the RM revertOnIneligible gate from the deployment network config through ResolvedSettings into the upgrade governance batch, and removes the now-vestigial RecurringCollector block from network configs.
…ation
When syncing a non-proxy address-book entry that has no rocketh deployment
record, sync was unconditionally seeding rocketh's record from the local
artifact's bytecode. If the artifact had drifted from on-chain (source
recompiled since the impl was deployed), this masked the change: rocketh's
native bytecode comparison then matched its (just-seeded) record against the
artifact and returned newlyDeployed=false. The address book never advanced,
proxy scripts saw no impl-address change, and shared-impl proxies
(DefaultAllocation, ReclaimedRewards) ended up with code-changed status but
no pendingImplementation set — so the upgrade orchestrator skipped them.
Mirror the gate the proxy path already uses for ${name}_Implementation:
seed only when the local artifact's bytecode hash matches the address
book's stored hash. When it doesn't, leave rocketh's record absent so the
next deployFn sees no prior bytecode and produces newlyDeployed=true,
propagating the new impl address through to proxy pending-upgrade detection
naturally.
Scope of the gate:
- Only registered registry names — proxy sync recurses with synthetic names
(e.g. RewardsManager_Implementation) that aren't real address-book
entries; the proxy path has its own hashMatches gate before recursing.
- Only non-prerequisites — externally-deployed contracts (L2GraphToken)
are never run through deployFn, so the dedup-masking concern doesn't
apply and they still need an env record for downstream reads.
Also replace the silent fall-through in deployProxyContract's shared-impl
branch with a throw when env.getOrNull(sharedImplementation.name) returns
null. With the sync fix in place, missing implDep is no longer a routine
state masking drift; it's a real signal that the impl deploy script didn't
run or didn't produce a record.
test(deployment): cover shouldSeedRocketh gate truth table
Extract the inline gate from syncContract's non-proxy seed path into a
pure shouldSeedRocketh helper and pin its truth table.
The helper already had three known failure modes during this fix's
development: it had to let unregistered synthetic names through (proxy
sync recurses with `${name}_Implementation` names), let prerequisites
through (L2GraphToken-style external deployments), and skip the seed
only on a *verified* mismatch — not whenever any signal is missing.
Each of those is a regression test: getting the gate wrong on any of
them broke a real deploy run.
Squash across deployment / horizon / subgraph-service of localNetwork config corrections so addresses match the deploy modules' hardcoded ACCOUNT1 convention. Previously caused OwnableUnauthorizedAccount reverts mid-batch when run-scripts signed with ACCOUNT1_SECRET. Squash of: - fix(deployment): align localNetwork migrate governor with ACCOUNT1 - chore(subgraph-service): align localNetwork migrate governor with horizon sibling - fix(subgraph-service): align localNetwork protocol governor with ACCOUNT1 - fix(horizon): correct localNetwork pauseGuardian and subgraphAvailabilityOracle addresses
Static unit test that catches drift between per-network Ignition
config files in packages/horizon/ignition/configs/ and
packages/subgraph-service/ignition/configs/.
Four checks:
1. Cross-package sibling agreement. For each (prefix, network) pair
where both packages have a file (e.g. both migrate.arbitrumOne.json5),
every overlapping non-empty $global field must match.
2. localNetwork all-files $global agreement. For localNetwork
specifically (one stack, one governor) every $global field
meaningfully declared in more than one of the four
{horizon,subgraph-service}/{migrate,protocol}.localNetwork.json5
files must match across all of them. Stricter than #1 — catches
same-package cross-prefix drift.
3. localNetwork same-package cross-prefix sub-object agreement. Each
package's per-contract config blocks (e.g. "DisputeManager": { ... },
"RecurringCollector": { ... }) must agree leaf-by-leaf between
migrate and protocol. Catches drift in fields like eip712Name /
eip712Version (signature verification breakers) and disputePeriod /
disputeDeposit parameters. Restricted to localNetwork because for
other networks (notably default) migrate and protocol are
intentionally different templates.
4. localNetwork mnemonic-index correctness. Lines like
"governor": "0x70997970…", // index 1
must have an address that derives from the hardhat default mnemonic
at the stated BIP44 index. Catches copy-paste mistakes where the
value updates but the comment doesn't, or vice versa.
Verified to fail on the SS protocol governor drift the test was
designed against: reverting governor in
subgraph-service/ignition/configs/protocol.localNetwork.json5 to
ACCOUNT0 produces a clean three-way mismatch report from check #2.
Adds json5 as a direct devDep (was transitively available via
toolshed but not declarable that way under pnpm strict mode).
Squash of: - fix: sync RewardsManager in mock REO integrate (otherwise stale on subsequent runs) - fix: namespace mock REO governance-tx label and document the integrate path
…-fix-3 dips publish
…aphService impl Switch SubgraphService implementation deploy from m.contract() to the deployImplementation() helper from @graphprotocol/horizon/ignition, matching the pattern every other core module already uses (HorizonStaking, GraphPayments, PaymentsEscrow, RecurringCollector, RewardsManager, Curation). SubgraphService was the odd one out. Functionally identical — the helper dispatches to m.contract(name, constructorArgs, options) when no artifact is supplied, so future IDs and bytecode resolution are unchanged. The win is a single seam for any future cross-cutting concern (verification metadata, build-info pinning, journaling) and visual consistency with the rest of the implementation deploys.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Deployment/testnet/2024-05-08/gip-0088
🚨 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 #1336 +/- ##
==========================================
+ Coverage 88.62% 91.16% +2.53%
==========================================
Files 75 81 +6
Lines 4615 5318 +703
Branches 823 969 +146
==========================================
+ Hits 4090 4848 +758
+ Misses 504 448 -56
- Partials 21 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:
|
Contributor
Author
|
Replaced by: #1337 |
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
3 participants
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.
Tracking as PR for visibility. This is likely not the final version prior to deployment, and subject to being replaced or force-pushed.
Basis of deployment for GIP-0088.
Once main contract changes are merged I intend to extract components into separate PRs.
Replaces: #1335