Notes / Corrections

Correction: adversarial review of Ventuals vHYPE staking (2026-09-18)

Corrections2026-09-18Chain: Hyperliquid

Source: data/correction_2026-09-18-ventuals-vhype-staking-adversarial-review.md in the repository, rendered here without changes. Source file

Same-day follow-up to the Ventuals vHYPE staking addition (a third HyperEVM-native Hyperliquid target, found by sweeping every HIP-3 dex deployer's own EIP-1967 implementation slot -- a still-dormant dex, vntl, turned out to carry a real, named liquid-staking product: new score_ventuals_vhype_staking in chains/hyperliquid/scripts/ methodology_test.py, wired into chains/hyperliquid/scorers.py, plus chains/hyperliquid/data/methodology_test_2026-09-18-ventuals-vhype- staking.md). Before committing, this addition went through an adversarial review workflow (3 independent dimensions -- facts-and- attribution, the manual-verification judgment call itself, and scorer-code robustness -- each finding piped to an independent verifier), matching this project's established "no shortcuts" review discipline, already applied twice the day before to Kinetiq's and para's own promotions.

Result: 12 findings returned, 11 confirmed via independent re-verification (fresh live RPC calls, live hyperevmscan.io source fetches, and even independent Solady/OpenZeppelin cross-checks -- not just re-reading the original claim), 1 refuted.

The 1 refutation#

The refuted finding ("Q3: no gap found -- an unexpected second MANAGER_ROLE holder would correctly fall through to the conservative path") was itself correctly refuted, but for a reason unrelated to this target: the reviewer's own verification pulled live on-chain data for the WRONG contract (para's RoleRegistry, not Ventuals'), citing a "2-of-3 Safe" that is actually para's own 1-of-1 Safe, and claiming to have "read RoleRegistry.sol source" for a contract that is in fact unverified. The underlying practical conclusion the finding was checking -- that only the RoleRegistry's own owner can grant a new role, confirmed by the verifying agent's own live eth_call simulation of setRole() from different msg.sender values -- held up regardless. No corrective action needed; noted here for anyone reading the raw workflow output later, and because it is the SAME "reviewer looked at a sibling target instead of this one" failure mode this project's Kinetiq/para correction docs also recorded, now a third time.

Fixed (6 real code gaps, all confirmed by independent re-verification)#

  1. MEDIUM -- the manual-verification shortcut had no runtime check that the verified implementation was still in place. The scorer trusts exactly one address (the MANAGER_ROLE holder) as "manually verified" based on having read its VERIFIED source once, at derivation time. But a UUPS proxy's own address never changes across an upgrade -- only its backing implementation moves -- and the SAME Safe this function already trusts (RoleRegistry.owner()) is the sole entity that can push such an upgrade (Base.sol's _authorizeUpgrade -> onlyOwner). Without a runtime check, a routine, legitimate Safe-authorized upgrade could silently swap this contract's real logic while the code kept applying a manual verification that no longer described its behavior -- confirmed by the verifying agent as "entirely plausible as ordinary protocol maintenance, not a hypothetical attack." Fixed: the MANAGER_ROLE holder's own EIP-1967 implementation slot is now read and compared against the verified implementation address (0x0000000c21e635b59edff54e70fe21315fa9b245) on EVERY run; a mismatch falls through to the normal conservative _classify_authority_holder path instead of continuing to trust a stale reading. New constant EIP1967_IMPLEMENTATION_SLOT added (previously only used as an inline literal in two other functions).
  2. MEDIUM -- the docstring's claim that "every privileged, fund- affecting admin action is gated onlyOwner" was overstated. An adversarial reviewer, having pulled the FULL verified source (not just the pasted excerpt) via Sourcify's v2 API, found finalizeBatch() (lines 376-432 of the real deployed contract) is itself whenNotPaused-only -- no onlyOwner, no role check -- and calls the underlying vault's stake()/unstake()/transferHypeToCore() directly: the exact three functions MANAGER_ROLE gates on that vault. What actually preserves the score, independently confirmed: every amount (amountToStake/amountToUnstake/depositsInBatch) and the destination (the onlyOwner-set validator) are computed purely from internal accounting, with zero caller-supplied discretion -- finalizeBatch takes no arguments at all. Reworded the docstring (and this target's own methodology doc) to state the real distinction: discretionary, parameter-changing, and emergency actions are onlyOwner-gated; the permissionless entry point is non-discretionary -- not a blanket "onlyOwner everywhere."
  3. MEDIUM -- a new role-hash length check crashed the whole target instead of degrading one role. Added in the same pass as this target's own scorer, the check for name, val in (...): if len(val) < 66: raise HyperEvmReadError(...) ran BEFORE the per-role try/except blocks that catch this exact exception type a few lines later for the sibling read_role_registry_role_holders failure -- breaking, for this one input, the graceful-degradation convention the function otherwise follows (confirmed by direct execution: the raise propagates straight out, the later except blocks never run because the function has already exited). Fixed by moving the length check inside each role's own try/except, so a malformed OPERATOR_ROLE()/MANAGER_ROLE() response (e.g. a future RoleRegistry upgrade that renames or removes a role) degrades that ONE role to the conservative (20, 0, 0) sentinel instead of crashing the entire score.
  4. MEDIUM -- a role legitimately having zero holders was indistinguishable from "not checked." Confirmed by reading the actual committed source at HEAD: both Kinetiq's (score_kinetiq_staking_manager) and para's own (score_para_staking_vault) scorers append a "<ROLE>: 0 holders" note whenever a role's member list comes back empty; this new function had no equivalent guard for either OPERATOR_ROLE or MANAGER_ROLE. Fixed: added the same note, matching the existing convention exactly.
  5. LOW -- the "...:UNRESOLVED" sentinel key leaked into the cross-exposure address set. all_holder_addrs_lower = {key.split( ":", 1)[1].lower() for key in role_paths} | nested_owner_addrs splits every role_paths key on its first : -- for a key like "MANAGER_ROLE:UNRESOLVED", this yields the literal string "unresolved", which can never coincide with a real dex signer address (confirmed: it doesn't match the 0x[0-9a-f]{40} shape) but has no place in a set meant to contain only addresses. Fixed by excluding any key ending in ":UNRESOLVED" from the split -- in THIS target's function AND in para StakingVault's own sibling function, which had the identical gap (found by checking the same pattern across every function using it, not just the one under review).
  6. LOW -- the same physical address holding two roles was classified twice. Live state has 0x72298a4c...eafe08 holding BOTH RoleRegistry.owner() and OPERATOR_ROLE directly (not nested inside a different Safe) -- the function ran two fully independent _classify_authority_holder calls (two separate eth_getCode/ getOwners/getThreshold/getModulesPaginated/guard-slot RPC sequences) for the identical address instead of reusing the already-computed result, wasting RPC calls and carrying a narrow, low-probability inconsistency risk if a Safe reconfiguration transaction landed in the gap between the two independent "latest"-block reads. Fixed by short-circuiting to the already-computed RoleRegistry.owner() classification whenever OPERATOR_ROLE's holder address matches.

What was independently re-confirmed as already correct (no action needed)#

Verification#

Reproduction#

python3 -m unittest discover -s scripts/lib/tests
python3 scripts/validate_all_scorers.py --ecosystem hyperliquid
cd chains/hyperliquid/scripts && python3 -c "
import json
from methodology_test import score_ventuals_vhype_staking
print(json.dumps(score_ventuals_vhype_staking(), indent=2))
"

Not an audit, not investment advice. All notes ยท Back to the dashboard.