WalletRegistry Bytecode Optimization for EIP-170 Compliance #3837
Conversation
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
…r handling Reduce contract size by removing DkgMaliciousResultSlashingFailed event and simplifying try-catch block in challengeDkgResult function. This optimization follows the pattern from commit 412a8e6 to meet bytecode size constraints. Changes: - Remove DkgMaliciousResultSlashingFailed event definition from WalletRegistry and all test upgrade contracts (V2, V2MissingSlot, V2MisplacedNewSlot) - Simplify challengeDkgResult try-catch: empty catch block allows silent slashing failure while ensuring challenge always completes - Add clear documentation explaining bytecode optimization trade-offs - Fix deployment script issues: add func.id to 15_deploy_allowlist.ts and Array.from() iteration in 16_initialize_allowlist_weights.ts - Add comprehensive test coverage with 11 new tests validating both success and failure paths for DKG challenge with simplified error handling Results: - WalletRegistry contract size: 24.115 KB (under 24.576 KB Spurious Dragon limit) - All 54 tests passing (20s execution time) - No regressions in TD-2 security audit fixes - Challenge completion preserved regardless of slashing outcome This optimization is the first step toward meeting the <22KB bytecode target for the WalletRegistry contract while maintaining all critical functionality and security guarantees.
Remove the EOA-only restriction from WalletRegistry.challengeDkgResult to enable compatibility with EIP-7702 (delegated code execution). This allows accounts with delegated code to participate in DKG result challenges while maintaining gas manipulation protection through the existing requireChallengeExtraGas mechanism. Changes: - Remove msg.sender == tx.origin check from challengeDkgResult - Add EIP-7702 compatibility documentation - Update test to validate contract caller challenge scenarios - Reduce bytecode size by ~150 bytes Gas manipulation protection remains enforced via the existing requireChallengeExtraGas function, which is independent of caller type and provides robust protection against gas limit attacks per EIP-150.
Reduce WalletRegistry contract size by 289 bytes through strategic bytecode optimizations to meet EIP-170 24KB deployment limit. Changes: - Inline requireChallengeExtraGas() function at single call site - Consolidate DKG setter state validation in updateDkgParameters - Optimize error message strings for brevity - Add technical comments explaining optimizations Files modified: - contracts/WalletRegistry.sol: Consolidated state check, inlined gas validation - contracts/libraries/EcdsaDkg.sol: Removed function, simplified setters, optimized errors - contracts/test/upgrades/WalletRegistryV2*.sol: Updated test upgrade contracts Results: - Bytecode reduced from 24.058 KB to 23.769 KB (289 bytes saved) - Contract now 237 bytes under 24KB limit - Zero compiler warnings - All validation logic preserved
Implement dual-mode modifier supporting both Allowlist and legacy TokenStaking authorization paths to enable gradual migration while maintaining backward compatibility. Changes: - Add allowlist state variable with comprehensive documentation - Update onlyStakingContract modifier with precedence logic: - Allowlist takes precedence when set (non-zero address) - Falls back to legacy staking when allowlist is zero - Add initializeV2 function for proxy upgrade with reinitializer(2) - Include gas optimization via address caching in modifier - Add comprehensive test suite (19 tests, 100% passing) The implementation preserves all existing security fixes and maintains backward compatibility with current deployments. Gas overhead is minimal (<0.5% increase).
Replace all require() statements with custom error reverts to improve gas efficiency and error clarity in the WalletRegistry contract. Changes: - Added 13 custom error definitions for authorization, validation, state, and configuration errors - Converted all require() statements to conditional revert with custom errors - Updated modifiers (onlyStakingContract, onlyWalletOwner, onlyReimbursableAdmin) to use custom errors - Updated test suites to expect custom error messages instead of string messages Benefits: - Reduced gas costs for error handling - More precise error identification and debugging - Improved code maintainability and readability
Update test assertions to match custom error format following the require-to-custom-error refactoring. Add comprehensive custom error validation test suite. Changes: - Replace string error messages with custom error names in Slashing tests - Replace string error messages with custom error names in Wallets tests - Add new WalletRegistry.CustomErrors.test.ts for comprehensive validation - Authorization errors (CallerNotStakingContract, CallerNotWalletOwner, etc.) - Validation errors (InvalidWalletMembersIdentifiers, NotSortitionPoolOperator, etc.) - State errors (CurrentStateNotIdle, NotEnoughExtraGasLeft) This completes the custom error migration by ensuring all tests validate the new error format consistently.
Introduce a comprehensive audit briefing for the WalletRegistry contract, detailing optimizations made for EIP-170 compliance. The document outlines the project's scope, executive summary, changes made, and trade-offs accepted during the optimization process. Key highlights include the reduction of contract bytecode to 23.824 KB, the implementation of dual-mode authorization, and the migration to custom error handling for improved gas efficiency. The document serves as a guide for auditors, emphasizing areas of focus and known issues related to the recent changes.
lrsaturnino
requested review from
evandrosaturnino,
pdyraga and
piotr-roslaniec
piotr-roslaniec
approved these changes
Oct 10, 2025
…nt authorization routing Security Critical Changes: - Add onlyGovernance modifier to WalletRegistry.initializeV2() to prevent front-running attacks during allowlist initialization - Implement _currentAuthorizationSource() helper to enable conditional authorization routing during TIP-092 migration period Authorization Routing Implementation: - Route authorization queries to Allowlist when set, TokenStaking otherwise - Update 6 callsites: joinSortitionPool, updateOperatorStatus, approveAuthorizationDecrease, involuntaryAuthorizationDecrease, eligibleStake, isOperatorUpToDate - Preserve TokenStaking routing for withdrawRewards (beneficiary lookup) and challengeDkgResult (slashing) as documented "NOT MIGRATED" functions Test Infrastructure: - Restructure authorization tests for TIP-092 dual-mode architecture - Add Migration Scenario Tests with comprehensive coverage of both modes (Pre-Upgrade: TokenStaking, Post-Upgrade: Allowlist) - Extract helper functions for test setup to reduce duplication - Add detailed documentation explaining migration strategy and rationale - Comment out legacy tests that depend on removed TokenStaking methods (stake, increaseAuthorization, processSlashing, approveApplication) Documentation: - Add inline comments explaining NOT MIGRATED decisions with ACP consensus references and technical rationale (bytecode cost vs benefit analysis) - Document TIP-092/100 historical context and reward halting timeline - Preserve legacy test patterns for migration reference This change enables safe migration from TokenStaking to Allowlist-based authorization while maintaining backward compatibility and preventing initialization vulnerabilities.
Add hardhat-chai-matchers dependency to enable custom error assertion testing. Update all WalletRegistry test suites to use the new assertion patterns following the custom error migration. Update test upgrade contracts to align with V2 implementation changes. Add type suppressions for legacy TokenStaking API calls retained in tests for reference.
…bility Remove onlyGovernance modifier from initializeV2 to save 42 bytes of bytecode. Security is maintained through atomic upgradeToAndCall pattern enforced at governance process level. Add comprehensive security assumption documentation explaining the atomic upgrade requirement and front-running protection model. Add DkgMaliciousResultSlashingFailed event to improve observability when external slashing calls fail. Emit this event in the catch block to ensure operators and monitors can detect and investigate slashing failures. Remove dated consensus references from inline comments. Apply formatting improvements for readability.
Contributor
|
Cool work on the bytecode optimization! The custom error migration should save alot of contract size. Quick question: whats the final bytecode size after these changes? Would be good to document the before/after numbers to show we're safely under the 24KB EIP-170 limit with some headroom for future changes. Also, have you run gas consumption comparisons? Custom errors should be cheaper, but im curious about the actual savings on common operations like DKG result submission. |
| /// compatibility with existing deployments. | ||
| /// @dev Set via initializeV2() during proxy upgrade. When allowlist is zero | ||
| /// address (default), the legacy TokenStaking authorization path is used. | ||
| Allowlist public allowlist; |
Contributor
There was a problem hiding this comment.
Nice addition for the dual-mode authorization. One thing, since this changes the authorization flow when allowlist is set, have you verified the gas costs dont increase significantly compared to the old TokenStaking path? The _currentAuthorizationSource() indirection adds an extra call on every auth check.
| /// - Upgrade implementation + initialize MUST be atomic | ||
| /// - No front-running window between upgrade and initialization | ||
| /// - Violation of this assumption creates front-running vulnerability | ||
| function initializeV2(address _allowlist) external reinitializer(2) { |
Contributor
There was a problem hiding this comment.
Apprecciate the docs on the atomic upgrade pattern. Quick question: is initializeV2 missing access control? I see its just external reinitializer(2) without any governance check.
I know the reinitializer modifier prevents double-init, but could someone front-run the proxy upgrade and call this before the admin does? Or does upgradeToAndCall handle that automatically? Just curious if theres any risk there.
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.
Summary
Optimized WalletRegistry to fit within Ethereum's 24KB contract size limit while adding dual-mode authorization for beta staker consolidation.
Final Bytecode: 23.824 KB
Context
After adding the allowlist feature for beta staker consolidation (18 → 3 operators, 83% cost reduction per TIP-92/TIP-100), WalletRegistry exceeded the 24KB contract size limit and could not be deployed. These changes reduce bytecode while maintaining security properties and adding required dual-mode authorization.
Changes
1. Silent Slashing (Commit 73dbec6)
DkgMaliciousResultSlashingFailedevent2. EIP-7702 Compatibility (Commit e2a35bc)
challengeDkgResult()gasleft()check3. Bytecode Optimizations (Commit e655538)
requireChallengeExtraGas(), consolidated DKG state checks, shortened error messages4. Dual-Mode Authorization (Commit d2ddcb3)
Allowliststate variable and modifiedonlyStakingContractmodifierallowlist = 0x0→ TokenStaking authorized)initializeV2(allowlist_address)switches to allowlist mode (irreversible)5-6. Custom Error Migration (Commits 04ebe63, 357328b)
require()statements to custom errors.revertedWith()→.revertedWithCustomError())Trade-offs
Prioritized:
Traded:
Preserved:
Review Focus
Critical
Important
Operational
initializeV2()Known Issues
initializeV2()(mitigation: Allowlist upgradeability, testnet validation)Test Status
Files Changed
solidity/ecdsa/contracts/WalletRegistry.solsolidity/ecdsa/contracts/libraries/EcdsaDkg.solsolidity/ecdsa/test/WalletRegistry.CustomErrors.test.ts(NEW)External Dependencies