Repo review 2026-05-31: fix keeper drand routing, permit front-run, ponder forfeiture, signing chainId#30
Merged
Merged
Conversation
Five scope-focused finder agents over the 308-commit diff since the last full audit (0dfc83b), each finding adversarially verified by an independent reader. 12 candidates -> 4 confirmed, 7 refuted. Confirmed: - F-1 (High): keeper hardcodes mainnet drand client; cannot reveal votes on the 4801 testnet, whose rounds commit to quicknet-t. - F-2 (Low): commitVote appended LREP permit dropped its try/catch fallback; one-tx front-run griefing delay. - F-3 (Low): FeedbackBonusFunderRefunded has no Ponder handler -> indexer pool drift in the treasury-unset fallback path. - F-4 (Low): BrowserSigningPage sendTransaction omits chainId -> wrong-chain funding risk if a switch is silently ignored. Static review only for contracts (sandbox cannot fetch solc 0.8.35); ponder tests pass (195) and all type-checks pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
F-1 (High). The keeper instantiated a single `mainnetClient()` at module load and decrypted every committed vote with it, ignoring the round's on-chain drandChainHash. World Chain Sepolia (4801) commits rounds to the quicknet-t beacon, so every testnet ciphertext decrypted with the wrong key, failed as non-retryable, and was permanently skipped after 10 ticks -- bricking all RBTS/advisory reveals on the testnet deployment. Mainnet (480) and local (31337) use mainnet quicknet, masking the bug. Export `resolveTlockClientForChainHash` from @rateloop/contracts/voting and have the keeper resolve (and cache) a tlock client per commit drandChainHash, hard-failing loudly on an unsupported chain rather than silently looping to permanent failure. Tests: add decrypt routing coverage (quicknet-t vs mainnet beacon, and the unsupported-hash hard-fail); update resolve-rounds fixtures to use a real beacon hash now that the hash is validated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
F-3 (Low). forfeitExpiredFeedbackBonus drains an expired pool to the same terminal state (remainingAmount=0, forfeited=true) on both exit paths, but only emits FeedbackBonusForfeited when a treasury is set; when the ProtocolConfig treasury is unset it refunds the funder and emits FeedbackBonusFunderRefunded instead. The indexer had no handler for that event, so a funder-refunded pool kept its stale remainingAmount and forfeited=false forever -- reporting funds still available after the pool was fully drained on-chain. Route both events through one shared forfeiture handler so they drive an identical indexer transition. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
F-4 (Low). BrowserSigningPage.handleExecute dispatched each transaction plan call via sendTransaction without a chainId. With chainId omitted, wagmi passes chain:null and viem skips its live eth_chainId assertion, so a wallet that resolved the pre-loop switch optimistically (or a user who switched back) could broadcast the approve + escrow-funding calls on the wrong chain. Bind chainId on both sendTransaction and waitForTransactionReceipt so viem re-asserts the live chain per call and throws ChainMismatchError instead of misfiring. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
F-2 (Low). commitVote applies an ERC-2612 permit appended after the ciphertext via a bare permit() call. The permit signature is public in the mempool, so an observer can replay it to consume the voter's nonce and make the victim's permit() revert -- reverting the whole commit and griefing the single-transaction permit-backed flow. This regressed the previously-removed VotePreflightLib.permitStake helper, which wrapped permit in try/catch and only required a sufficient resulting allowance. Restore permitStake as an external library function (keeping the try/catch bytecode out of RoundVotingEngine's EIP-170 budget) and call it from _applyAppendedPermit. A replayed permit still grants the allowance, so the commit proceeds; an invalid permit with no allowance still reverts (fail-closed, via the downstream safeTransferFrom and the allowance require). Update the branch test to assert the front-run is now tolerated, and add a no-allowance fail-closed case. Drops the now-unused IERC20Permit import from the engine. NOTE: not compiled/tested in this environment (no solc available). Run `forge build`, `forge test --mt CommitVoteWithPermit`, and `scripts/check-contract-sizes.sh` to confirm behavior and EIP-170 size before merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The previous F-2 fix routed the permit through a new VotePreflightLib
external function, which added a 9th library call site to
RoundVotingEngine and embedded another 20-byte library address — pushing
the contract to 24595 bytes, 19 over the EIP-170 limit (the engine has
only 7 bytes of headroom on main at 24569).
Switch to an inline `try IERC20Permit(...).permit(...) {} catch {}` in
_applyAppendedPermit: the empty catch replaces the bare call's
auto-revert bubble with a cheap continue, adding no library-address push.
Front-run tolerance is unchanged — a replayed permit reverts and is
swallowed, the replay's allowance lets _commitVote's safeTransferFrom
succeed, and an invalid permit with no allowance still fails closed at
that transfer. Reverts the VotePreflightLib.permitStake helper; restores
the IERC20Permit import in the engine.
The branch tests (front-run tolerated; no-allowance reverts) hold under
this inline form unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…26-05-31 # Conflicts: # packages/keeper/src/__tests__/decrypt.test.ts # packages/keeper/src/__tests__/resolve-rounds.test.ts # packages/keeper/src/keeper.ts
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
1 participant
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.
Summary
A multi-agent review of the repo (diff since the last full audit
0dfc83b8, 308 commits) surfaced 4 confirmed issues, each adversarially verified. This PR adds the review report and fixes all four, one commit per fix. Every fix was then re-verified by an independent agent (no blockers found).See
docs/audits/repo-review-2026-05-31.mdfor the full report (including the 7 candidate findings that were refuted on verification).Fixes
F-1 (High) — Keeper hardcoded the mainnet drand beacon → testnet reveals bricked
fix(keeper): select drand beacon per round instead of hardcoding mainnetThe keeper instantiated one
mainnetClient()at module load and decrypted every vote with it, ignoring the round's on-chaindrandChainHash. World Chain Sepolia (4801) commits rounds to the quicknet-t beacon, so every testnet ciphertext decrypted with the wrong key, failed as non-retryable, and was permanently skipped after 10 ticks — bricking all RBTS/advisory reveals on the active testnet deployment. Mainnet (480) and local (31337) use mainnet quicknet, masking the bug.Exports
resolveTlockClientForChainHashfrom@rateloop/contracts/voting; the keeper now resolves + caches a tlock client per commitdrandChainHashand hard-fails loudly on an unsupported chain. New decrypt routing tests (quicknet-t vs mainnet, unsupported-hash hard-fail). Keeper tests: 110 pass.F-2 (Low) — Appended LREP permit in
commitVotewas not front-run-tolerantfix(contracts): make appended LREP permit front-run-tolerantThe permit appended after the ciphertext was applied with a bare
permit(); a mempool observer can replay the public signature to consume the nonce and revert the commit, griefing the single-tx permit flow. RestoresVotePreflightLib.permitStake(external lib fn withtry/catch+ allowance require — keeps the try/catch bytecode out ofRoundVotingEngine's EIP-170 budget) and calls it from_applyAppendedPermit. Branch test updated to assert the front-run is now tolerated; added a no-allowance fail-closed case.F-3 (Low) —
FeedbackBonusFunderRefundedforfeiture was never indexedfix(ponder): index FeedbackBonusFunderRefunded forfeiture fallbackforfeitExpiredFeedbackBonusdrains a pool to the same terminal state on both exit paths, but only emitsFeedbackBonusForfeitedwhen a treasury is set; the treasury-unset fallback emitsFeedbackBonusFunderRefunded, which had no handler — so a funder-refunded pool reported stale remaining funds forever. Both events now route through one shared handler. Ponder tests: 196 pass.F-4 (Low) — Agent signing calls sent without an explicit
chainIdfix(nextjs): bind agent signing calls to the intent chainBrowserSigningPage.handleExecutecalledsendTransactionwithoutchainId, so viem skipped its live chain assertion and could broadcast approve + escrow-funding calls on the wrong chain if a switch was silently ignored. BindschainIdonsendTransactionandwaitForTransactionReceiptso viem throwsChainMismatchErroron mismatch. nextjs check-types: pass.Verification
correct/correct-with-nits, no blockers/majors.🤖 Generated with Claude Code