fix: Phase 0 pre-mainnet security fixes (6 findings)#31
fix: Phase 0 pre-mainnet security fixes (6 findings)#31jeremylongshore merged 6 commits intomainfrom
Conversation
… analysis Comprehensive pre-mortem analysis covering: - Top 5 project killers (bond ratio, no timelock, arbitrator incentive, no cross-chain, governance ossification) - 83 findings across 8 categories (SC, EC, GV, OF, PK, AG, MC, LF) - Economic attack scenarios with code-verified parameters - Multi-chain feasibility (hub-and-spoke recommended) - Infinite lifecycle requirements - 4-phase remediation roadmap (pre-mainnet through governance) - Risk matrix with likelihood x impact scoring Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add explicit nullity checks after both tryRecover calls in ReceiptV2Extension to prevent address(0) from passing signature validation when tryRecover silently returns zero address on malformed signatures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change terms encoding to include expected nonce value. The enforcer now validates that the current storage nonce matches the expected value before incrementing, preventing replay attacks from blind nonce increment. Breaking change to terms format (pre-mainnet only). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…drawals (PM-EC-002, PM-SC-023) Two related fixes in OptimisticDisputeModule: 1. Arbitrator incentive (PM-EC-002): Replace percentage-based arbitrator slash (10%) with flat fee (0.005 ETH) from protocol balance. New distribution: 75% user / 25% treasury / 0% arbitrator from slash. 2. Pull-pattern withdrawals (PM-SC-023): Replace push-based _transferETH with pendingWithdrawals mapping + withdrawPending() to prevent DoS via non-payable recipient contracts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add declaredVolume parameter to postReceipt/batchPostReceipts so bond-to-volume ratio is enforced on-chain. SolverRegistry gets bondRatioBps (default 5%, governable) and requiredBondForVolume() view. Prevents rational fraud where a small bond covers arbitrarily large transaction volumes. Breaking change: postReceipt/batchPostReceipts signatures updated. All callers (tests, scripts, fuzz, invariants) updated to pass declaredVolume parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Deploy OZ TimelockController with 48h minimum delay, Safe as sole proposer/executor, and transfer ownership of all core contracts. Includes integration tests verifying admin calls revert when called directly and succeed through the timelock after delay. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces timelock-based governance controls, implements a declared volume-to-bond ratio validation system for solvers, migrates dispute resolution ETH transfers to a pull-based withdrawal pattern, hardens nonce enforcement to require exact matches, and includes comprehensive pre-mainnet security analysis documentation. Changes
Sequence DiagramssequenceDiagram
participant Safe as Safe (Proposer/Executor)
participant Timelock as TimelockController
participant Registry as SolverRegistry
participant Hub as IntentReceiptHub
Safe->>Timelock: schedule(operation, salt, predecessor, delay)
Note over Timelock: Operation queued with 48-hour delay
Note over Timelock: Wait: MIN_DELAY elapsed
Safe->>Timelock: execute(operation, salt, predecessor)
Timelock->>Registry: onlyOwner function (e.g., setBondRatioBps)
Registry->>Registry: Update state
Timelock->>Hub: onlyOwner function (e.g., setChallengeWindow)
Hub->>Hub: Update state
sequenceDiagram
participant Solver as Solver/User
participant Hub as IntentReceiptHub
participant Registry as SolverRegistry
participant Dispute as OptimisticDisputeModule
Solver->>Registry: requiredBondForVolume(volume)
Registry-->>Solver: required bond amount
Solver->>Hub: postReceipt(receipt, declaredVolume)
Hub->>Registry: requiredBondForVolume(declaredVolume)
Registry-->>Hub: required bond
Hub->>Hub: Validate solver bond >= required
alt Insufficient Bond
Hub-->>Solver: InsufficientBondForVolume
else Bond Sufficient
Hub->>Hub: Store declaredVolume
Hub-->>Solver: Receipt posted
end
Note over Hub,Dispute: On dispute resolution...
Dispute->>Dispute: Determine winner, calculate slash
Dispute->>Dispute: pendingWithdrawals[recipient] += amount
Dispute-->>Dispute: emit WithdrawalPending
Dispute->>Dispute: pendingWithdrawals[arbitrator] += flatFee
Solver->>Dispute: withdrawPending()
Dispute->>Dispute: pendingWithdrawals[solver] = 0
Dispute-->>Solver: Transfer ETH
Dispute-->>Dispute: emit WithdrawalCompleted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @jeremylongshore, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements six critical security and economic fixes identified in a pre-mortem analysis, crucial for the protocol's mainnet deployment. The changes enhance the system's resilience against rational fraud by introducing volume-proportional bond requirements, improve governance security through a timelock for administrative actions, and refine dispute resolution mechanics by decoupling arbitrator incentives and implementing a safer withdrawal pattern. These updates collectively strengthen the protocol's integrity and prepare it for a robust launch. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements several critical pre-mainnet security fixes identified in a security audit. The changes are well-structured and address each finding effectively. Key improvements include introducing a TimelockController for governance actions, implementing volume-proportional bonds to mitigate rational fraud, redesigning the arbitrator incentive with a flat fee, and using a pull pattern for ETH withdrawals to prevent DoS attacks. The NonceEnforcer has also been strengthened to require an exact expected nonce. The code quality is high, and the changes are thoroughly tested with new unit and integration tests. I have one suggestion for the new deployment script to improve its verification logging.
| // Step 2-7: Transfer ownership of all contracts to TimelockController | ||
| console.log("[2/7] Transferring SolverRegistry ownership..."); | ||
| Ownable(solverRegistry).transferOwnership(address(timelock)); | ||
| console.log(" Owner:", Ownable(solverRegistry).owner()); | ||
|
|
||
| console.log("[3/7] Transferring IntentReceiptHub ownership..."); | ||
| Ownable(intentReceiptHub).transferOwnership(address(timelock)); | ||
| console.log(" Owner:", Ownable(intentReceiptHub).owner()); | ||
|
|
||
| console.log("[4/7] Transferring DisputeModule ownership..."); | ||
| Ownable(disputeModule).transferOwnership(address(timelock)); | ||
| console.log(" Owner:", Ownable(disputeModule).owner()); | ||
|
|
||
| console.log("[5/7] Transferring OptimisticDisputeModule ownership..."); | ||
| Ownable(optimisticDisputeModule).transferOwnership(address(timelock)); | ||
| console.log(" Owner:", Ownable(optimisticDisputeModule).owner()); | ||
|
|
||
| console.log("[6/7] Transferring EscrowVault ownership..."); | ||
| Ownable(escrowVault).transferOwnership(address(timelock)); | ||
| console.log(" Owner:", Ownable(escrowVault).owner()); | ||
|
|
||
| console.log("[7/7] Transferring ReceiptV2Extension ownership..."); | ||
| Ownable(receiptV2Extension).transferOwnership(address(timelock)); | ||
| console.log(" Owner:", Ownable(receiptV2Extension).owner()); | ||
|
|
There was a problem hiding this comment.
The console.log statements that verify the new owner are placed within the vm.startBroadcast() block. In Forge scripts, state changes from broadcasted transactions are not applied until after vm.stopBroadcast(). As a result, these logs will incorrectly print the old owner address, not the new timelock address, which can be misleading during deployment.
To correctly verify the ownership changes, all the Ownable(...).owner() calls for verification should be moved to after the vm.stopBroadcast() call.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/OptimisticDisputeModule.sol (1)
254-313:⚠️ Potential issue | 🟠 MajorArbitration fee can over-allocate contract balance.
arbitrationFlatFeeis credited based only onaddress(this).balance, which already includes counter-bonds that are also credited topendingWithdrawals. If the module isn’t pre-funded, total pending can exceed free balance and later withdrawals will revert for either the arbitrator or the bond recipient.🛠️ Suggested fix (track free balance before crediting fees)
+ uint256 public totalPendingWithdrawals; @@ - pendingWithdrawals[dispute.challenger] += dispute.counterBond; + pendingWithdrawals[dispute.challenger] += dispute.counterBond; + totalPendingWithdrawals += dispute.counterBond; @@ - pendingWithdrawals[solverOperator] += dispute.counterBond; + pendingWithdrawals[solverOperator] += dispute.counterBond; + totalPendingWithdrawals += dispute.counterBond; @@ - if (arbitrationFlatFee > 0 && address(this).balance >= arbitrationFlatFee) { + uint256 freeBalance = address(this).balance - totalPendingWithdrawals; + if (arbitrationFlatFee > 0 && freeBalance >= arbitrationFlatFee) { pendingWithdrawals[arbitrator] += arbitrationFlatFee; + totalPendingWithdrawals += arbitrationFlatFee; emit WithdrawalPending(arbitrator, arbitrationFlatFee); } @@ - pendingWithdrawals[msg.sender] = 0; + pendingWithdrawals[msg.sender] = 0; + totalPendingWithdrawals -= amount;- pendingWithdrawals[dispute.challenger] += dispute.counterBond; + pendingWithdrawals[dispute.challenger] += dispute.counterBond; + totalPendingWithdrawals += dispute.counterBond;
🤖 Fix all issues with AI agents
In `@src/SolverRegistry.sol`:
- Around line 468-474: Replace the inline require string checks in
setBondRatioBps with custom errors: declare two errors (e.g.,
InvalidBondRatioZero() and BondRatioExceedsMax()) near the contract's error
declarations, then in setBondRatioBps(uint256 _bps) use conditional reverts
(e.g., if (_bps == 0) revert InvalidBondRatioZero(); if (_bps > BPS) revert
BondRatioExceedsMax();) and keep the assignment to bondRatioBps and the
onlyOwner modifier unchanged so callers see the new custom error types instead
of require strings.
🧹 Nitpick comments (3)
test/ReceiptV2Extension.t.sol (1)
555-575: Test duplicates existing coverage.This new test
test_PostReceiptV2_RevertZeroAddressRecoveryuses the same corrupted signature bytes (bytes32(0), bytes32(0), uint8(27)) as the existingtest_PostReceiptV2_RevertInvalidSolverSignatureat lines 223-234. Both tests verify the same behavior—thatInvalidSolverSignature()is reverted.The added value is the PM-SC-022 documentation in comments, but consider consolidating or renaming one test to make the distinction clearer (e.g., one could test a signature that recovers to a wrong address vs. one that recovers to
address(0)). Currently, both use the same malformed signature input.script/DeployTimelock.s.sol (1)
54-98: Consider transaction failure handling for partial deployment scenarios.The ownership transfers execute sequentially without error handling. If a transfer fails mid-script (e.g., contract not owned by deployer), the timelock will be deployed but only some contracts will have transferred ownership.
For production deployment, consider:
- Verifying deployer owns all contracts before starting transfers
- Documenting the expected pre-conditions clearly
🛡️ Optional: Add ownership verification before transfers
vm.startBroadcast(deployerPrivateKey); +// Verify deployer owns all contracts before proceeding +require(Ownable(solverRegistry).owner() == deployer, "Deployer must own SolverRegistry"); +require(Ownable(intentReceiptHub).owner() == deployer, "Deployer must own IntentReceiptHub"); +require(Ownable(disputeModule).owner() == deployer, "Deployer must own DisputeModule"); +require(Ownable(optimisticDisputeModule).owner() == deployer, "Deployer must own OptimisticDisputeModule"); +require(Ownable(escrowVault).owner() == deployer, "Deployer must own EscrowVault"); +require(Ownable(receiptV2Extension).owner() == deployer, "Deployer must own ReceiptV2Extension"); + // Step 1: Deploy TimelockController000-docs/039-AA-AUDT-pre-mortem-analysis.md (1)
839-852: Add language specifier to fenced code block.The ASCII architecture diagram should have a language specifier for the fenced code block, or use
textif no syntax highlighting is needed.📝 Suggested fix
-``` +```text ┌──────────────────────┐
| /// @notice Set bond-to-volume ratio (PM-EC-001, governable via timelock) | ||
| /// @param _bps New ratio in basis points (e.g. 500 = 5%) | ||
| function setBondRatioBps(uint256 _bps) external onlyOwner { | ||
| require(_bps > 0, "Ratio must be > 0"); | ||
| require(_bps <= BPS, "Ratio exceeds 100%"); | ||
| bondRatioBps = _bps; | ||
| } |
There was a problem hiding this comment.
Use custom errors instead of require strings in the new setter.
🔧 Suggested change
+ error BondRatioZero();
+ error BondRatioTooHigh();
@@
function setBondRatioBps(uint256 _bps) external onlyOwner {
- require(_bps > 0, "Ratio must be > 0");
- require(_bps <= BPS, "Ratio exceeds 100%");
+ if (_bps == 0) revert BondRatioZero();
+ if (_bps > BPS) revert BondRatioTooHigh();
bondRatioBps = _bps;
}As per coding guidelines src/**/*.sol: Implement custom errors in Solidity (e.g., SolverNotActive()) instead of require strings.
🤖 Prompt for AI Agents
In `@src/SolverRegistry.sol` around lines 468 - 474, Replace the inline require
string checks in setBondRatioBps with custom errors: declare two errors (e.g.,
InvalidBondRatioZero() and BondRatioExceedsMax()) near the contract's error
declarations, then in setBondRatioBps(uint256 _bps) use conditional reverts
(e.g., if (_bps == 0) revert InvalidBondRatioZero(); if (_bps > BPS) revert
BondRatioExceedsMax();) and keep the assignment to bondRatioBps and the
onlyOwner modifier unchanged so callers see the new custom error types instead
of require strings.
1 participant
Summary
Implements all 6 Phase 0 pre-mainnet security fixes from the pre-mortem analysis (039-AA-AUDT). These are the blockers identified before mainnet deployment.
Commits (in execution order)
0ffddfbaddress(0)nullity check afterECDSA.tryRecoverin ReceiptV2Extensiond3fbd8dc62693450f2cd3bondRatioBps=500,declaredVolumeparam)16e3b34Breaking Changes
postReceipt(receipt)→postReceipt(receipt, declaredVolume)— all callers updatedbatchPostReceipts(receipts)→batchPostReceipts(receipts, declaredVolumes)— all callers updatedpendingWithdrawals+withdrawPending()(pull pattern)Key Design Decisions
ReceiptMismatchdispute reason.setBondRatioBps()through timelock.New Slash Distribution
Test Results
Test plan
forge build— compiles with via_ir, optimizer 200forge test— 474/474 passing🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation