Conversation
| return Err(Error::NoParticipants); | ||
| } | ||
|
|
||
| let aggregate_pubkey = bsc_verifier::aggregate_public_keys(participants); |
There was a problem hiding this comment.
I think we have this method in s crypto utils, if not let's move it there
| /// Compute the hash of a block header using RLP encoding and Keccak256. | ||
| /// | ||
| /// This follows the standard Ethereum header hash computation. | ||
| pub fn compute_header_hash<H: Keccak256>(header: &CodecHeader) -> H256 { |
There was a problem hiding this comment.
We already have this in the geth primitives, there's a hash function implemented on the header
| } | ||
|
|
||
| /// Get values from storage proof. | ||
| fn get_values_from_proof<H: Keccak256 + Send + Sync>( |
There was a problem hiding this comment.
Import all these EVM utility methods from the evm state machine instead of duplicating
| #[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))] | ||
| pub struct ValidatorSetProof { | ||
| /// The new validator set | ||
| pub validator_set: ValidatorSet, |
There was a problem hiding this comment.
| pub validator_set: ValidatorSet, |
The proof doesn't need to come with the validator set, we can trust the values we decode from the proof
| let storage_root = H256::from_slice(account.storage_root.as_slice()); | ||
|
|
||
| let layout = StakingContractLayout::default(); | ||
| let validator_count = proof.validator_set.validators.len() as u64; |
There was a problem hiding this comment.
We can just have a value activePoolIdTotalCount on the ValidatorSetProof
use existing apis
remove redundant hashing use evm config url
| ) { | ||
| map.insert(key, Some(value_hash.to_vec())); | ||
| } else { | ||
| map.insert(key, None); |
There was a problem hiding this comment.
i'm not convinced this is correct
error on missing keys
| proof: Vec<u8>, | ||
| ) -> Result<(Vec<u8>, ismp::consensus::VerifiedCommitments), Error> { | ||
| let update = VerifierStateUpdate::decode(&mut &proof[..]) | ||
| .map_err(|e| Error::AnyHow(anyhow::anyhow!("{:?}", e).into()))?; |
There was a problem hiding this comment.
doesn't parity_scale_code::Error implement core::Error
There was a problem hiding this comment.
parity_scale_codec::Error only implements std::error::Error which is gated behind the std feature, not core::error::Error actually.
Wizdave97
left a comment
Wizdave97
left a comment
There was a problem hiding this comment.
The plan is now complete with the critical SkipEmpty attack finding properly documented against the Pharos docs. This is a research/review task, not an implementation task, so I should present the findings directly rather than calling ExitPlanMode.
Here's my comprehensive review of PR #628: Pharos Network Integration:
Key Architecture
The PR adds a complete Pharos network integration across 6 modules (~3000+ lines, 23 new files):
- Primitives: Types + hexary hash tree SPV verification (SHA-256, not Keccak)
- Verifier: BLS-based block verification + validator set proof from staking contract
- Prover: RPC client with two-phase concurrent fetching
- ISMP Client: ConsensusClient + fraud proofs
- State Machine: Existence + non-existence proof verification for ISMP
- Tesseract Relayer: Epoch-by-epoch syncing loop
Pharos uses a flat trie (storage proofs verify directly against state root) with SkipEmpty hashing (SHA-256 over only non-empty slots).
CRITICAL Finding
(a) SkipEmpty Attack in Non-Existence Proofs (spv.rs:verify_non_existence_proof)
The Pharos docs explicitly describe this attack: Because SkipEmpty hashing concatenates only non-empty slot hashes in scan order, an attacker can move a hash from the target slot to any empty slot without changing the node hash:
Real node: slot[7] = H_child_k (key K exists here)
Forged node: slot[7] = 0x00 slot[9] = H_child_k (moved)
Both hash to: SHA256(header || H_child_a || H_child_k || H_child_c) -- identical!
The defense requires sibling proofs for every non-empty slot. But verify_non_existence_proof only checks the siblings that are provided -- it never verifies that all non-empty slots have a corresponding sibling proof. An attacker can omit the sibling proof for the relocated slot.
Fix: After the main chain walk, count non-empty slots in the terminal node (excluding the target), assert sibling_proofs.len() == count, and verify each slot_index matches a non-empty slot.
HIGH Findings
(g) Contract totalStake proven but ignored (state_proof.rs:143)
let _total_stake = decode_u256_from_storage(&values[0])?;
The on-chain totalStake is fetched and proven against state root, but discarded. The validator set's total_stake is computed by summing individual stakes. Should cross-check.
MEDIUM Findings
| Finding | Location
-- | -- | --
(e) | BLS verifies against Header::hash() but RPC returns a separate block_proof_hash -- need to confirm which message validators actually sign | verifier/lib.rs:verify_bls_signature
(f) | Duplicate participant keys not checked before BLS aggregation -- could inflate stake | verifier/lib.rs:verify_bls_signature
(h) | Both "missing proof" and "invalid proof" return StorageProofLookupFailed | state_proof.rs:verify_all_storage_proofs
(j) | Redundant staleness check with inconsistent error messages | ismp/clients/pharos
(l) | verify_membership checks key existence but not value content | state-machines/pharos
(m) | 20-byte keys silently mapped to None without any proof | state-machines/pharos
Test Coverage Gaps
All tests are #[ignore] (require PHAROS_ATLANTIC_RPC). Missing:
- Deterministic unit tests with hardcoded proof fixtures (for CI)
- SkipEmpty attack test for finding (a)
- Duplicate participant BLS test
- Fraud proof test
Top Recommendations
- Fix the SkipEmpty attack (a) -- this is the most critical issue, directly exploitable for forging non-existence proofs
- Cross-check totalStake (g)
- Clarify BLS signed message (e) -- confirm header hash vs block_proof_hash
- Deduplicate participant keys (f) before aggregation
- Add CI-friendly unit tests with hardcoded proof data
…ridge into dami/pharos-network
3 participants
This PR introduces the integration to Pharos network for Hyperbridge.
closes #593