VerifyingRpcClient never checks that range-scoped responses fall inside the requested block window
- Lenguaje dominante
- Rust
- Estrellas
- 78
- Forks
- 129
- Merge medio
- 4 d 14 h
- PR fusionados (30 d)
- 52
Descripción
`VerifyingRpcClient` exists to reject responses that don't answer the request that was made, and it
does a thorough job on one half of that. Returned note IDs, note tags, nullifier prefixes, account
IDs and script roots are all checked against what was asked for.
The other half is missing. Five of its methods take a `block_from`/`block_to` window, and none of
them ever look at whether the response actually falls inside it. Both parameters are forwarded to
the inner client and then never mentioned again:
* `sync_notes` (verifying_client/mod.rs:238) — checks tags, not heights
* `sync_nullifiers` (:252) — checks prefixes, not heights
* `sync_storage_maps` (:293) — delegates, no checks
* `sync_account_vault` (:302) — delegates, no checks
* `sync_transactions` (:311) — checks account IDs, not heights
So a node can answer a question about blocks 100-200 with data stamped block 5000, and the wrapper
whose whole purpose is catching that will pass it straight through.
## This has already bitten the notes path
#2400 is this gap, one layer down. A note whose inclusion proof claims a height past the client's
partial MMR reaches `adjust_merkle_path_for_forest`, hits
`leaf_to_corresponding_tree(..).expect("forest includes block number")`, and panics the client. It
was reported against `sync_expected_notes`, but the reason the bad height ever arrives is that
nothing upstream ever rejected an out-of-range block.
I'm not proposing to duplicate that fix. #2401 handles the notes path and touches `import.rs`,
`rpc/domain/note.rs` and `sync/block_header.rs` — it doesn't touch the verifying client, so the
other four methods keep the same blind spot after it lands.
## The transactions path has the same shape
`sync_transactions` is called for `current_block_num + 1 .. chain_tip`
(`sync/state_sync.rs:529`). `verify_account_ids` confirms every record belongs to a requested
account, and nothing confirms `record.block_num` is inside the window that was asked for. That value
is used directly:
```rust
transaction.commit_transaction(record.block_num, timestamp);
```
(`sync/state_sync_update.rs:315`, and again at `:326` in the unauthenticated-notes fallback.)
A record stamped outside the requested range therefore commits a locally tracked transaction at
whatever height the response carried, including one past the chain tip the client just read.
## What I'd change
Add a range check next to the existing `verify_*` helpers and call it from all five methods, so the
window is enforced in the same place the rest of the request/response agreement already is. The
per-method wiring differs slightly — notes and nullifiers carry a height per item, the storage-map
and vault responses carry a single `block_number` — so it's one helper plus five small call sites,
matching the style of `verify_note_tags` and `verify_account_ids`.
Worth deciding as part of this: whether an out-of-range item should fail the whole call with
`RpcError::InvalidResponse`, consistent with every other check in this file, or be dropped with a
warning the way `sync_notes_with_content` drops notes with unusable content. I'd lean towards the
former, since unlike attachment availability a block height isn't attacker-influenced in a way an
honest node can't control.
Checked against `next` @ `1b16ea8d`. I haven't stood up a hostile node to observe the mis-stamped
commit end to end; the absent checks and the `commit_transaction` call path are both from the
current source.
Could this be assigned to me? Happy to do the work.
Guía de contribución
Línea de trabajo
The issue is in verifying_client/mod.rs, methods sync_notes, sync_nullifiers, sync_storage_maps, sync_account_vault, and sync_transactions. Start by reading the existing verify_* helpers (like verify_note_tags, verify_account_ids) to understand the pattern. Then write a new helper to check block ranges, and integrate it into each method. Ensure the helper aligns with the existing error handling (RpcError::InvalidResponse). Run the existing tests to confirm the changes don't break anything.
Escrito por el modelo de indexación a partir del texto del issue.
Evaluación
- Stack tecnológico
- rust
- Área
- backend, security
- Tipo de issue
- Error
- Dificultad
- 3/5
- Tiempo estimado
- 1-2 días
- Estado de actividad
- Activo
- Claridad
- Bien especificado
- Aptitud para principiantes
- 65/100