fix(contract): avoid validator config v2 resource exhaustion#2900
fix(contract): avoid validator config v2 resource exhaustion#2900SuperFluffy wants to merge 3 commits intomainfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
🐺 Ralph Security Review
Findings
📋 Consolidation complete · Thread 📜 28 events🔍 |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
🐺 Ralph Review
Summary
The Solidity active-index fix is algorithmically correct, but the PR has several gaps: the Rust node precompile still has the O(N) scan (highest priority), a previous audit finding (ZELLIC-24) is reintroduced, migration can permanently halt on IP collisions, and off-diff invariant tests are broken.
🚨 [SECURITY] getActiveValidators Gas Exhaustion Fix Missing from Rust Node Precompile
File: crates/precompiles/src/validator_config_v2/mod.rs:209-217
The O(N) linear scan fix was only applied to the Solidity reference implementation. The Rust precompile's get_active_validators() still iterates over all historical validators via self.read_validator_at(i), issuing ~2100 gas per SLOAD. Once the array exceeds ~14K entries, this exceeds the block gas limit and becomes permanently uncallable. Additionally, getAllValidators() is still exposed in the Rust ABI and dispatcher, creating spec drift.
Recommended Fix: Port the activeValidatorsByIndex pattern to the Rust precompile. Remove getAllValidators() from the Rust ABI and dispatch layer.
🚨 [SECURITY] Migration Permanently Halts on IP Collision
File: crates/precompiles/src/validator_config_v2/mod.rs:591-606, tips/ref-impls/src/ValidatorConfigV2.sol:337
migrateValidator unconditionally enforces _requireUniqueIngressIp on all validators including inactive ones. If a deactivated V1 validator shares an IP with an already-migrated active validator, migration reverts and cannot be resumed due to strict sequential indexing.
Recommended Fix: Only enforce IP uniqueness for active validators during migration.
⚠️ [ISSUE] Solidity rotateValidator Incorrectly Reverts When Keeping Same IP
File: tips/ref-impls/src/ValidatorConfigV2.sol:165-174
rotateValidator calls _validateRotateParams → _requireUniqueIngressIp(ingress) before freeing the old IP. If a validator keeps their existing IP, the call incorrectly reverts. The Rust precompile handles this correctly.
Recommended Fix: Free the old IP before validation, or exclude the validator's own IP from the uniqueness check.
| external | ||
| onlyOwnerOrValidator(validatorAddress) | ||
| { | ||
| // idx is 1-indexed. |
There was a problem hiding this comment.
🚨 [SECURITY] ZELLIC-24 Regression: Validators Can Block Deactivation via Front-Running
V1 fixed ZELLIC-24 by using index-based lookups for deactivation. V2 regresses this: deactivateValidator(address) and rotateValidator(address, ...) use addressToIndex[validatorAddress]. A malicious validator can front-run the owner's deactivation with transferValidatorOwnership(currentAddress, newAddress), deleting addressToIndex[currentAddress] and causing the owner's tx to revert with ValidatorNotFound(). This is repeatable indefinitely.
Recommended Fix: Use publicKey (immutable, unique) or array index as the lookup key, matching the V1 ZELLIC-24 fix. Update TIP-1017 spec accordingly.
| function getAllValidators() external view returns (Validator[] memory validators); | ||
|
|
||
| /// @notice Get only active validators (where deactivatedAtHeight == 0) | ||
| /// @notice Get only active validators (where deactivatedAtHeight == 0). |
There was a problem hiding this comment.
getAllValidators() Removal
Removing getAllValidators() breaks invariant tests in test/invariants/ValidatorConfigV2.t.sol (lines 983, 1009, 1018, 1036+) that are outside this diff. forge test now fails at compile time, silently disabling regression coverage for append-only and history invariants (TEMPO-VALV2-14 through 19).
Recommended Fix: Migrate invariant tests to use validatorCount() + validatorByIndex() before merging. Do not ship with broken invariant coverage.
2 participants
This patch fixes a potential resource exhaustion in
ValidatorConfigV2.getAllValidators.Active V2 validators are now tracked in a separate array that holds indices into the global append only
validatorsArray. Consistency is ensured by deleting entries ondeactiveValidatorandrotateValidator.