Deployment/testnet/2024-05-06/gip-0088#1335
Closed
RembrandtK wants to merge 142 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>
…nal paths
Pure path rename of the five lows parked in the previous commit to
their final TRST-L-{new}.md paths. Each file's content is byte-
identical across the rename, so git's default detection sees all five
at 100% similarity (no -B required, no cascade).
Adds the v03 report PDF and aligns md responses with the auditor's text. - README: add 2nd fix-review commit, point at v03 PDF, refresh status column, drop M-5/L-6 rows (withdrawn) and add R-14 row, plus a footer noting the dropped findings and where their concerns were addressed. - TRST-M-1, M-4, L-6..L-10: status flipped to Fixed (M-1 main finding Acknowledged); v02 local response promoted into the auditor's "Team Response" section as the auditor incorporated it verbatim; new "Mitigation Review" section added with the v03 verdict. - TRST-R-14: new recommendation about avoiding magic numbers (`getAgreementDetails(0)` should use VERSION_CURRENT). No code changes; the contract fixes for these findings already landed in earlier commits.
…GNED (TRST-L-7) The v03 mitigation review on TRST-L-7 asks for it to be clarified that when a payer is represented by a separate signer (Authorizable), the signer — not the payer — must call cancel() with SCOPE_SIGNED for the revocation to take effect against subsequent accept/update. Updated the IAgreementCollector NatSpec on cancel() to spell out the per-scope caller requirement, including that combining SCOPE_SIGNED with SCOPE_PENDING / SCOPE_ACTIVE only makes sense when msg.sender is both payer and signer. Mirrored the clarification in the RecurringCollector implementation comment so the dev-doc and inline comment agree.
… safety margin (TRST-L-8) The v03 mitigation review on TRST-L-8 asks for tests that ensure the defined CALLBACK_GAS_OVERHEAD constant stays sufficient as the code evolves. Existing tests cover the lower bound (precheck reverts under tight gas) but do not assert the upper bound — that when the precheck passes, the EIP-150 cap still forwards at least MAX_PAYER_CALLBACK_GAS to the callee. Added recordedBeforeCollectionGasleft / recordedAfterCollectionGasleft to MockAgreementOwner, sampled at the first opcode of each callback, and a regression test asserting both stay within 500 gas of MAX. The current overhead constant lands the recorded value ~300 below MAX (function dispatch overhead), leaving ~200 gas of headroom against the 500 tolerance — so an edit that adds pre-CALL Solidity overhead trips the alarm before CALLBACK_GAS_OVERHEAD becomes outright insufficient.
…GIBILITY_CHECK (TRST-L-9) The v03 mitigation review on TRST-L-9 asks for it to be documented that turning CONDITION_ELIGIBILITY_CHECK on against an EOA payer is considered fully trusting that payer, since EIP-7702 lets the EOA attach a contract that returns false from isEligible() to block collections at any time. The existing comment treated the flag as a plain opt-in feature without naming the EOA-via-7702 caveat. Expanded the NatSpec on the constant declaration so the trust boundary is visible at the same site readers consult when deciding whether to opt into the flag for a given payer.
…Details (TRST-R-14) The v03 report's TRST-R-14 flags _getAgreementProvider passing 0 as the index argument to IAgreementCollector.getAgreementDetails: that 0 is no longer a placeholder, it now selects VERSION_CURRENT at the collector, and the named constant communicates intent and survives any future remapping of version indices. Imports VERSION_CURRENT from the interface and substitutes it at the single call site.
- subgraph-service: ScopedCancelActive integration test leaked the payer prank into post-cancel view-reads via resetPrank (startPrank). When fuzz picked rca.payer == SubgraphService's auto-deployed proxy admin (0x8816d8...), the verification reads on subgraphService reverted with ProxyDeniedAdminAccess. Switched to single-shot vm.prank for the cancel call and added a deterministic regression test using the failing seed. - horizon: TRST-L-8's MAX_PAYER_CALLBACK_GAS margin test asserts the production gas overhead, but `forge coverage` disables the optimizer, legitimately growing the dispatch δ past tolerance. Skip under FOUNDRY_COVERAGE=1 (set by the package's coverage scripts); direct `forge coverage` invocations still fail loudly. - foundry.toml (all 4 packages): exclude block-timestamp lint — pure noise across this codebase.
After the audit branch was rebased onto current main as indexing-payments-management-audit-fixed-rebased, the linear chain was re-signed and the SHAs cited throughout the report no longer resolve. Anchor the pre-rebase tip at tag indexing-payments-management-audit-pre-rebase and provide the mapping plus a one-line equivalence check.
Sign-off commit bbec75e. v04 addressed recommendations only; no new findings or status changes versus v03.
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-06/gip-0088
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1335 +/- ##
==========================================
+ Coverage 88.62% 91.16% +2.54%
==========================================
Files 75 81 +6
Lines 4615 5342 +727
Branches 823 1134 +311
==========================================
+ Hits 4090 4870 +780
+ Misses 504 449 -55
- Partials 21 23 +2
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: #1336 |
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. Will soon be replaced with version rebased on top of extra contract fixes.
Basis of deployment for GIP-0088.
Once main contract changes are merged I intend to extract components into separate PRs.