feat: val config v2 - separate vals into active and inactive#2890
feat: val config v2 - separate vals into active and inactive#2890
Conversation
Amp-Thread-ID: https://ampcode.com/threads/T-019c9cdd-469e-763b-a64c-631b986b2d93 Co-authored-by: Amp <amp@ampcode.com>
📊 Tempo Precompiles Coverage |
howydev
requested review from
0xKitsune,
0xrusowsky,
dankrad,
danrobinson,
fgimenez,
klkvr,
legion2002,
mattsse and
rakita
as code owners
howydev
changed the title
🐺 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
This PR refactors ValidatorConfigV2 from a single validators array to separate active_validators / inactive_validators lists with swap_remove for O(1) deactivation. State-changing APIs now take a mutable idx instead of validatorAddress. The refactor is well-tested, but three findings survived deduplication and verification across audit workers.
Reviewer Callouts:
- ⚡ Gas-Bounded Pagination:
getInactiveValidatorssilently truncates output based ongas_remaining()— callers may unknowingly process incomplete data. - ⚡ Read-Only Behavior Drift:
ReadOnlyStorageProvider::gas_remaining()returnsu64::MAX, silently disabling the pagination guard ineth_callcontexts. - ⚡ V2 Integration Boundary: When
peer_manager/actor.rsswitches from V1 to V2 reads, re-check active-only index space and pubkey/address reuse semantics.
| Err(ValidatorConfigV2Error::validator_not_found())? | ||
| } | ||
|
|
||
| let mut to_deactivate = self.active_validators[call.idx as usize].read()?; |
There was a problem hiding this comment.
🚨 [SECURITY] Index-Only Mutations Are Reorderable And Can Deactivate The Wrong Validator
State-changing APIs (deactivate_validator, rotate_validator, set_ip_addresses, transfer_validator_ownership) identify targets only by mutable active-array idx. Because deactivate_validator uses swap-and-pop (L455–L466), pending owner transactions can be silently retargeted to a different validator if another deactivation changes active-set ordering first.
Attack path: Owner submits deactivateValidator(idx=0) to remove attacker. Attacker front-runs with self-authorized deactivateValidator(idx=0). Swap-and-pop moves an innocent validator into idx=0. Owner tx executes and deactivates the wrong validator.
Recommended Fix: Require an immutable identity binding in mutating calls — e.g. (idx, expectedValidatorAddress) — and revert if the entry at idx doesn't match. This preserves O(1) access while preventing silent retargeting.
| if start_index >= count { | ||
| return Ok(Vec::new()); | ||
| } | ||
| let mut out = Vec::with_capacity((count - start_index) as usize); |
There was a problem hiding this comment.
🚨 [SECURITY] Unmetered Host Preallocation From Unbounded Historical Length
Vec::with_capacity((count - start_index) as usize) allocates host memory proportional to the full inactive set size before the gas-bounded loop at L219. count is attacker-growable via repeated rotate_validator/deactivate_validator. This creates unmetered host-memory pressure independent of EVM gas budget.
Additionally, in read-only contexts (eth_call), gas_remaining() returns u64::MAX (see crates/revm/src/common.rs:401), entirely disabling the pagination guard and attempting a full historical copy.
Recommended Fix: Replace with_capacity(count - start_index) with bounded growth — compute affordable_items from available gas and per-item cost, then cap both allocation and loop iteration by that bound.
| } | ||
| _checkOnlyOwnerOrValidator(oldValidator.validatorAddress); | ||
|
|
||
| _validateRotateParams(publicKey, ingress, egress); |
There was a problem hiding this comment.
🚨 [SECURITY] Solidity rotateValidator Incorrectly Reverts When Keeping Same Ingress IP
_validateRotateParams unconditionally calls _requireUniqueIngressIp(ingress), which reverts with IngressAlreadyExists when a validator rotates their key while keeping the same ingress IP — because the validator is still active when the check runs. The Rust precompile (update_ingress_ip_tracking at mod.rs:275-289) correctly short-circuits when old == new. This breaks legitimate key rotation simulation/testing against the Solidity mock.
Recommended Fix: Remove _requireUniqueIngressIp(ingress) from _validateRotateParams. The subsequent _updateIngressIp call already enforces uniqueness correctly by skipping when old and new hashes match.
| } | ||
|
|
||
| fn gas_remaining(&self) -> u64 { | ||
| u64::MAX |
There was a problem hiding this comment.
🛡️ [DEFENSE-IN-DEPTH] gas_remaining() returns u64::MAX in read-only context
This effectively disables the gas-based pagination guard in get_inactive_validators during eth_call queries, causing full historical copy attempts. Consider plumbing actual call gas limits or making pagination explicitly cap-based (e.g., max_items argument) independent of gas.
SuperFluffy
left a comment
SuperFluffy
left a comment
There was a problem hiding this comment.
I'll do a second pass, but the unstable indices raise concern.
If your data structure gives out indices as part of its public API, we better make sure they don't become invalid.
Instead of splitting the the vector into two, why don't we maintain a single vector of all validators (as before), and have a second array of active validators that only contains indices into the global array?
That way your indices are stable and cannot be invalidated.
| /// Rotate a validator to new identity (owner or validator) | ||
| function rotateValidator( | ||
| address validatorAddress, | ||
| uint64 idx, |
There was a problem hiding this comment.
I don't understand the reasoning behind this. Why does this need an index now?
Is the index stable? IIRC, we had a mapping address -> index.
| internal | ||
| { | ||
| uint64 idx = uint64(validatorsArray.length); | ||
| uint64 idx = deactivatedAtHeight == 0 ? uint64(activeValidatorsArray.length) : 0; |
There was a problem hiding this comment.
These indices are unstable. That's a very dangerous design.
|
I propose this alternative: #2900 |
SuperFluffy
left a comment
SuperFluffy
left a comment
There was a problem hiding this comment.
This design breaks CL, see specifically this PR #2872 how validatorByPublicKey will be used.
| revert ValidatorNotFound(); | ||
| } | ||
| return validatorsArray[idx - 1]; | ||
| return activeValidatorsArray[idx - 1]; |
There was a problem hiding this comment.
On DKG failure CL reverts to the previous set (to use DKG nomenclature - the committee of epoch E+1 will be the dealers of E because the resharing process failed and the players of E do not have valid shares). This means that for a given epoch validators can be in the committee but no longer active.
This design breaks CL and will cause an unrecoverable chainhalt.
3 participants
Changes
index_byIndexview functions now return 0 if the validator associated with that value is deactivatedNote: On correctness of migration, there's a future PR that will overhaul migration. So any security issues around migration should be checked in that PR
Callouts: