Skip to content

Conversation

@pdyraga
Copy link
Member

@pdyraga pdyraga commented Mar 5, 2025

The allowlist contract replaces the Threshold TokenStaking contract and is as an outcome of TIP-092 and TIP-100 governance decisions. Staking tokens is no longer required to operate nodes. Beta stakers are selected by the DAO and operate the network based on the allowlist maintained by the DAO. The contract will be integrated with the WalletRegistry and replace calls to TokenStaking.

I have been experimenting with various approaches, and the most extreme one was to remove most of the EcdsaAuthorization logic as well as all TokenStaking.seize calls. This would have cascading effects on tBTC Bridge contracts as they rely on WalletRegistry.seize. That would also require implementing weight decrease delays in the Allowlist, so essentially doing work that is already done in WalletRegistry. Considering the pros and cons, I decided on the least invasive option. The WalletRegistry still thinks in terms of stake authorization, but everything is based on the staking provider's weight as set in the Allowlist, and weight decrease delays are enforced by the existing mechanism in EcdsaAuthorization. The seize function does nothing except of emitting an event about detecting beta staker misbehavior.

To be done

Deployment script

We need to capture all existing beta stakers along with their current authorizations and initialize the Allowlist contract. We can do it by either replicating the existing weights or giving them all the same weight.

Integrate with WalletRegistry and tests

There are two approaches to achieve it. The first one is to get rid of all references to TokenStaking from tests and update them to work with Allowlist. Another approach is to let them work with TokenStaking but introduce another integration test for those two contracts. In this option, we could use in WalletRegistry something like:

    modifier onlyStakingContract() {
        address _allowlist = address(allowlist);
        require(
            // If the allowlist is set, accept calls only from the allowlist.
            // This is post-TIP-98 scenario. If the allowlist is not set, accept
            // calls only from the staking contract. This is pre-TIP-98 scenario.
            (_allowlist != address(0) && msg.sender == _allowlist) ||
                (_allowlist == address(0) && msg.sender == address(staking)),
            "Caller is not the staking contract"
        );
        _;
    }

    /// @notice Initializes V2 version of the WalletRegistry operating with the
    ///         Allowlist contract, as a result of TIP-098 and TIP-100 governance
    ///         decisions.
    function initializeV2(address _allowlist) external reinitializer(2) {
        allowlist = Allowlist(_allowlist);
    }

    /// @dev Provides the expected IStaking reference. If the allowlist is set,
    ///      it acts as the staking contract. If it is not set, the TokenStaking
    ///      acts as the staking contract.
    function _staking() internal returns (IStaking) {
        if (address(allowlist) != address(0)) {
            return IStaking(allowlist);
        }

        return staking;
    }

Note that the WalletRegistry is close to the maximum allowed contract size and - surprise! - adding the logic above makes it exceed the allowed size. This could potentially be alleviated by removing some of the functionality. For example, in the challengeDkgResult function we have a try catch as well as a call to dkg.requireChallengeExtraGas(). This could potentially be eliminated as a no-op seize in Allowlist is guaranteed to always succeed. Also, post EIP-7702, the require(msg.sender == tx.origin, "Not EOA") check is no longer guaranteed to work as expected.

pdyraga added 3 commits March 5, 2025 08:48
The current project configuration supports at most node 18.x so setting
lts/hydrogen as the version to be used.
The project is on OpenZeppelin 4 and the latest version currently
available is 4.9.6. This change will also allow us to use
Ownable2StepUpgradeable that was not available in OpenZeppelin 4.6. The
upgrade required increasing the version of
@openzeppelin/hardhat-upgrades plugin given the bug in the previously
used 1.20.0 version not resolving the correct version of dependencies
used (OZ contracts vs contracts-upgradeable) and in our case complaining
about the delegatecall used in the OZ's AddressUpgradeable code. The bug
was fixed in 1.20.4 version of the plugin. This also required updates in
the deployment tests as some functions are no longer available in the
upgraded version.

https://forum.openzeppelin.com/t/interacting-with-uups-upgradeable-contracts-in-test-throwing-contract-is-not-upgrade-safe-use-of-delegatecall-is-not-allowed/32743/5
The allowlist contract replaces the Threshold `TokenStaking` contract and
is as an outcome of TIP-092 and TIP-100 governance decisions. Staking
tokens is no longer required to operate nodes. Beta stakers are selected
by the DAO and operate the network based on the allowlist maintained by
the DAO. The contract will be integrated with the `WalletRegistry` and
replace calls to `TokenStaking`.

I have been experimenting with various approaches, and the most extreme
one was to remove most of the `EcdsaAuthorization` logic as well as all
`TokenStaking.seize` calls. This would have cascading effects on tBTC
Bridge contracts as they rely on `WalletRegistry.seize`. That would also
require implementing weight decrease delays in the `Allowlist,` so
essentially doing work that is already done in `WalletRegistry`.
Considering the pros and cons, I decided on the least invasive option.
The `WalletRegistry` still thinks in terms of stake authorization, but
everything is based on the staking provider's weight as set in the
`Allowlist`, and weight decrease delays are enforced by the existing
mechanism in `EcdsaAuthorization`. The `seize` function does nothing
except of emitting an event about detecting beta staker misbehavior.

This code is still a draft. Has to be integrated and covered with proper
tests!
piotr-roslaniec added a commit that referenced this pull request Aug 4, 2025
Fixes PR #3826 by implementing missing components for Allowlist contract:

- Add comprehensive test suite (432 lines, full coverage)
- Create deployment script with upgradeable proxy pattern
- Add migration script to initialize existing beta staker weights
- Complete documentation with deployment and consolidation guides

- Add one-click consolidation script based on authoritative research
- Consolidates 18 operators → 3 operators (83% reduction)
- Keeps 1 operator per entity (Boar, P2P, Staked)
- Natural fund drainage through weight management (no forced migrations)

- DAO-controlled operator management without token staking
- Gradual, reversible consolidation process
- Complete audit trails and safety checks
- Production-ready deployment infrastructure

This enables the transition from TokenStaking to Allowlist as specified in
TIP-092 and TIP-100, with a direct path to beta staker consolidation.
piotr-roslaniec and others added 7 commits August 18, 2025 11:16
- Add Allowlist contract to replace TokenStaking per TIP-092/TIP-100
- Implement weight-based operator management without token staking
- Add deployment and initialization scripts
- Include consolidation script for operator reduction (20→4 operators)
  - Includes NUCO operators (1 kept, 1 consolidated)
- Add comprehensive test coverage
- Maintain compatibility with existing WalletRegistry interface
- Add two-step process enforcement for weight decrease (Issue #1)
  - Introduce decreasePending flag to track valid decrease requests
  - Prevent bypassing the intended authorization flow

- Add zero address validation (Issue #3)
  - Validate walletRegistry in initialize()
  - Validate stakingProvider in addStakingProvider()

- Add zero weight validation (Issue #5)
  - Prevent adding staking providers with zero weight
  - Avoid potential duplicate additions

- Add comprehensive test coverage for all security fixes

Note: Issue #8 (seize function access) intentionally not restricted
as public reporting of malicious behavior is desired functionality
…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).
…#3833)

Fixes PR #3826 by implementing missing components for Allowlist
contract:

- Add a test suite
- Create deployment script with upgradeable proxy pattern
- Add migration script to initialize existing beta staker weights
@piotr-roslaniec piotr-roslaniec marked this pull request as ready for review October 9, 2025 13:17
lrsaturnino
lrsaturnino previously approved these changes Oct 9, 2025
Copy link
Member

@lrsaturnino lrsaturnino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.
…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.
…actions

- Add solidity/ to paths-ignore and path-filter exclusions
  Prevents client workflow from running on Solidity-only changes
- Upgrade actions/upload-artifact from v3 to v4
- Upgrade actions/download-artifact from v3 to v4
  Fixes auto-failure due to deprecated action versions
## 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)
- **What**: Removed `DkgMaliciousResultSlashingFailed` event
- **Impact**: Slashing failures no longer emit events (detection via
event correlation)
- **Security**: Challenge completion still guaranteed; slashing success
still emitted

### 2. EIP-7702 Compatibility (Commit e2a35bc)
- **What**: Removed EOA-only restriction from `challengeDkgResult()`
- **Why**: Future-proof for account abstraction; gas protection via
inline `gasleft()` check
- **Impact**: Smart contract wallets can now challenge DKG results
- **Security**: Reentrancy and gas manipulation vectors expanded;
EIP-150 protection retained

### 3. Bytecode Optimizations (Commit e655538)
- **What**: Inlined `requireChallengeExtraGas()`, consolidated DKG state
checks, shortened error messages
- **Why**: Function call overhead and redundant state checks
- **Impact**: Pure optimization, no behavioral changes

### 4. Dual-Mode Authorization (Commit d2ddcb3)
- **What**: Added `Allowlist` state variable and modified
`onlyStakingContract` modifier
- **Why**: Enable migration from TokenStaking to weight-based Allowlist
- **Impact**:
  - Starts in legacy mode (`allowlist = 0x0` → TokenStaking authorized)
- Calling `initializeV2(allowlist_address)` switches to allowlist mode
(irreversible)
- **Migration**: Controlled by governance multi-sig; testnet validated
before mainnet

### 5-6. Custom Error Migration (Commits 04ebe63, 357328b)
- **What**: Migrated 15 `require()` statements to custom errors
- **Impact**:
  - ABI breaking change
  - Go bindings require regeneration
- Test assertions need updating (`.revertedWith()` →
`.revertedWithCustomError()`)

## Trade-offs

**Prioritized**:
- Protocol security (DKG validation intact)
- Deployment capability (bytecode under 24KB)
- Future compatibility (EIP-7702, dual-mode authorization)

**Traded**:
- Direct observability (silent slashing failures)
- Simplicity (dual-mode authorization complexity)
- Client compatibility (ABI changes)

**Preserved**:
- Economic deterrents
- Validation logic
- Access controls

## Review Focus

### Critical
1. **Silent Slashing Economic Model**: Does economic security hold with
occasional undetectable slashing failures?
2. **Dual-Mode Authorization**: Edge cases in mode switching;
irreversibility implications
3. **EIP-7702 Attack Vectors**: Reentrancy with contract callers; gas
manipulation via proxies

### Important
4. **Custom Error Logical Equivalence**: All 15 conversions maintain
correctness
5. **Storage Layout Safety**: Proxy upgrade compatibility; no storage
collisions
6. **Allowlist Single Point of Failure**: Post-upgrade dependency on
Allowlist contract

### Operational
7. **Deployment Order**: Allowlist → WalletRegistry V2 → Proxy upgrade
with `initializeV2()`
8. **Rollback Strategy**: Irreversible mode switch; contingency if
Allowlist has critical bug
9. **Testnet Validation**: Storage preservation, dual-mode
authorization, edge cases

## Known Issues

- **Observability Gap**: Slashing failures not directly observable
(mitigation: event correlation)
- **Irreversible Mode Switch**: Cannot revert to legacy after
`initializeV2()` (mitigation: Allowlist upgradeability, testnet
validation)
- **ABI Breaking Changes**: Client updates required (mitigation: Go
bindings regeneration)

## Test Status

- **Current**: 758/772 passing (98.2%)
- **Pending**: 14 test assertions need custom error updates

## Files Changed

- `solidity/ecdsa/contracts/WalletRegistry.sol`
- `solidity/ecdsa/contracts/libraries/EcdsaDkg.sol`
- `solidity/ecdsa/test/WalletRegistry.CustomErrors.test.ts` (NEW)
- Multiple test files updated for custom errors

## External Dependencies

- **No Changes**: SortitionPool, RandomBeacon, TokenStaking
- **New Dependency**: Allowlist (separately audited)
lrsaturnino
lrsaturnino previously approved these changes Dec 16, 2025
Add complete deployment workflow for Allowlist integration and operator
weight migration with support for Ownable2StepUpgradeable pattern.

Changes:
- Add deployment script for WalletRegistry V2 upgrade with atomic
  initializeV2 execution via upgradeToAndCall pattern
- Fix Allowlist deployment to defer ownership transfer until after
  weight initialization (prevents Ownable2Step pendingOwner issue)
- Update weight initialization script to use actual contract owner
  and complete two-step ownership transfer to governance at end
- Configure Sepolia and Mainnet named accounts for proper deployment
  authority (0x68ad60CC for Sepolia, 0x716089 for Mainnet deployer)
- Add allowlist operator weights data with accumulated stakes from
  consolidated beta staker groups (1.3B total T vs 743M individual)
- Add .env configuration infrastructure with template and gitignore
  rules to protect private keys

Technical notes:
- Script 17 uses upgradeToAndCall to atomically upgrade proxy and
  call initializeV2, preventing front-running window
- Script 16 defers to actual contract owner() instead of assuming
  governance, fixing Ownable2StepUpgradeable compatibility
- Beta staker weights accumulate all T stake from provider groups
  (e.g., P2P: 200M T from 6 operators vs 20M T from staying node)
- Ownership transfer initiates at script end but requires governance
  to call acceptOwnership() to complete (two-step pattern)

Files:
- solidity/ecdsa/deploy/17_upgrade_wallet_registry_v2.ts (new)
- solidity/ecdsa/deploy/15_deploy_allowlist.ts (ownership fix)
- solidity/ecdsa/deploy/16_initialize_allowlist_weights.ts (owner fix)
- solidity/ecdsa/deploy/data/allowlist-weights.json (new)
- solidity/ecdsa/.env.example (new)
- solidity/ecdsa/.gitignore (env protection)
- solidity/ecdsa/hardhat.config.ts (named accounts)
This commit updates the hardhat verification tooling and adds support for
Sepolia testnet deployments:

Changes:
- Replace deprecated @nomiclabs/hardhat-etherscan with @nomicfoundation/hardhat-verify
- Add Sepolia external artifacts directory for dependency resolution
- Add allowlist weights configuration for Sepolia network
- Include RandomBeacon, T token, and TokenStaking contract artifacts for Sepolia
- Add deployment function IDs to all deploy scripts for better tracking
- Remove unused mainnet OpenZeppelin deployment metadata
- Update package dependencies and lockfiles

These changes enable deployment and testing on Sepolia while modernizing
the verification infrastructure to use the latest Hardhat tooling.
This commit introduces a new JSON file containing allowlist weights for mainnet, detailing operator stakes, accumulated weights, and consolidation information. The data includes metadata about the generation time, source, and weight calculation formulas, as well as a summary of operators added and not added to the allowlist. This enhancement supports the ongoing integration of beta staker consolidation and improves the overall deployment infrastructure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants