feat(agglayer): add faucet deregistration#2838
Conversation
a6017ce to
6c7c155
Compare
|
Putting into draft mode until |
287204f to
50a54bf
Compare
50a54bf to
3328c3d
Compare
Adds a bridge-admin-gated `deregister_faucet` MASM procedure that revokes a faucet, clearing every entry registration wrote for it: the `faucet_registry_map`, `token_registry_map`, and all four `faucet_metadata_map` sub-keys. Wired through a new `DEREGISTER_AGG_BRIDGE` note script and a `DeregisterAggBridgeNote` Rust builder. After deregistration, in-flight B2AGG and CLAIM notes referencing the cleared faucet fail their existing registration checks. The token registry is pair-keyed on (origin_token_address, origin_network). Rather than trusting note-supplied values, `deregister_faucet` reads the faucet's registered address and network back from its own `faucet_metadata_map` (via `get_faucet_conversion_info`) and recomputes the key, so the cleared token-registry entry is provably the one `register_faucet` wrote and the two registries cannot desynchronize. The note therefore carries only the faucet id (2 felts). - bridge_config.masm: new `deregister_faucet` proc reusing `assert_sender_is_bridge_admin`, `assert_faucet_registered`, `get_faucet_conversion_info`, and `hash_token_address`. - components/bridge.masm: re-export `deregister_faucet`. - note_scripts/deregister_agg_bridge.masm: new note script. - src/deregister_note.rs: new `DeregisterAggBridgeNote` builder. - bridge.rs: add the DEREGISTER script root to the network-account note allowlist. - tests/agglayer/config_bridge.rs: round-trip test verifying all three maps are cleared, an end-to-end test proving the faucet is revoked, and negative tests for `ERR_FAUCET_NOT_REGISTERED` and `ERR_SENDER_NOT_BRIDGE_ADMIN`. - SPEC.md, CHANGELOG.md: documentation updates.
3328c3d to
b26fec7
Compare
mmagician
left a comment
There was a problem hiding this comment.
LGTM modulo:
- verbosity
- naming of the note & related variables
- stack comments need fixing
- (optional) small refactors around code de-dup
| #! `assert_faucet_registered` and `lookup_faucet_by_token_address` calls panic, so any in-flight | ||
| #! notes (B2AGG / CLAIM) targeting the deregistered faucet fail their existing registration checks. | ||
| #! | ||
| #! The origin token address and origin network are NOT taken from the note: they are read back |
There was a problem hiding this comment.
I don;t think we necessarily care where token address is NOT taken from.
| #! 1. Writes `[0, 0, faucet_id_suffix, faucet_id_prefix] -> [0, 0, 0, 0]` into `faucet_registry`. | ||
| #! 2. Writes `hash(tokenAddress[5] || origin_network) -> [0, 0, 0, 0]` into `token_registry`, where | ||
| #! the address/network come from `faucet_metadata` and origin_network is byte-swapped before | ||
| #! hashing (matching `register_faucet` Step 4 and `lookup_faucet_by_token_address`). | ||
| #! 3. Clears all four `faucet_metadata` sub-keys (addr-lo, addr-hi/network/scale, hash-lo, hash-hi). |
There was a problem hiding this comment.
This is explaining again what the above section already explain, just using concrete values / slots
| # Read the faucet's registered (origin_token_addr, origin_network) from faucet_metadata so the | ||
| # cleared key is provably the one register_faucet wrote. Drop the scale we don't need. |
There was a problem hiding this comment.
| # Read the faucet's registered (origin_token_addr, origin_network) from faucet_metadata so the | |
| # cleared key is provably the one register_faucet wrote. Drop the scale we don't need. | |
| # Read the faucet's registered (origin_token_addr, origin_network) from faucet_metadata. Drop the scale we don't need. |
| # cleared key is provably the one register_faucet wrote. Drop the scale we don't need. | ||
| loc_load.DEREG_FAUCET_ID_PREFIX_LOC loc_load.DEREG_FAUCET_ID_SUFFIX_LOC | ||
| exec.get_faucet_conversion_info | ||
| # => [origin_token_addr(5), origin_network, scale, pad(8)] |
There was a problem hiding this comment.
The stack layout comments are off by 1, they add up to 15 not 16
There was a problem hiding this comment.
and this spills over to the following stack layout comments too
| # Each sub-key KEY = [sub_key, 0, suffix, prefix] is cleared to VALUE = [0, 0, 0, 0]. | ||
| push.0.0.0.0 | ||
| loc_load.DEREG_FAUCET_ID_PREFIX_LOC loc_load.DEREG_FAUCET_ID_SUFFIX_LOC push.0.FAUCET_METADATA_SUBKEY_ADDR_LO | ||
| push.FAUCET_METADATA_MAP_SLOT[0..2] | ||
| exec.native_account::set_map_item | ||
| dropw | ||
| # => [pad(16)] | ||
|
|
||
| push.0.0.0.0 | ||
| loc_load.DEREG_FAUCET_ID_PREFIX_LOC loc_load.DEREG_FAUCET_ID_SUFFIX_LOC push.0.FAUCET_METADATA_SUBKEY_ADDR_HI | ||
| push.FAUCET_METADATA_MAP_SLOT[0..2] | ||
| exec.native_account::set_map_item | ||
| dropw | ||
| # => [pad(16)] | ||
|
|
||
| push.0.0.0.0 | ||
| loc_load.DEREG_FAUCET_ID_PREFIX_LOC loc_load.DEREG_FAUCET_ID_SUFFIX_LOC push.0.FAUCET_METADATA_SUBKEY_HASH_LO | ||
| push.FAUCET_METADATA_MAP_SLOT[0..2] | ||
| exec.native_account::set_map_item | ||
| dropw | ||
| # => [pad(16)] |
There was a problem hiding this comment.
would be cool if we could write a loop for clearing the four subkeys, it would simplify the code quite a bit
| call.bridge_config::deregister_faucet | ||
| # => [pad(18)] | ||
|
|
||
| # Drop the 2 overflow pad felts to bring sdepth back to the 16-minimum. |
There was a problem hiding this comment.
| # Drop the 2 overflow pad felts to bring sdepth back to the 16-minimum. |
| /// and faucet metadata. It carries only the faucet account ID; the bridge reads the registered | ||
| /// origin token address and origin network back from its own faucet metadata, so the token | ||
| /// registry key it clears matches the faucet's current registration rather than trusting | ||
| /// note-supplied values. The note is always public. |
There was a problem hiding this comment.
The note is always public.
I don't think this is asserted in MASM.
Is there an attack vector if the note is private?
| /// Tests that deregistration is an unconditional revocation even when a stale `token_registry` key | ||
| /// survives. | ||
| /// | ||
| /// `register_faucet` does not clear a faucet's previous token key when the faucet is re-registered | ||
| /// under a different `(origin_token_address, origin_network)`, so after deregistration the prior | ||
| /// token key can still resolve to the (now-unregistered) faucet via | ||
| /// `lookup_faucet_by_token_address`. Because `claim` re-checks `assert_faucet_registered` after the | ||
| /// token lookup, such a CLAIM is rejected with `ERR_FAUCET_NOT_REGISTERED` instead of minting | ||
| /// through the revoked faucet. |
There was a problem hiding this comment.
Would it be complicated to also clear the stale key? This seems to cause a bit of overhead, in terms of documentation at the very least.
| Ok(()) | ||
| } | ||
|
|
||
| /// Tests that DEREGISTER_AGG_BRIDGE panics with `ERR_SENDER_NOT_BRIDGE_ADMIN` when the note |
There was a problem hiding this comment.
we have a few failure scenarios, could we bundle them with rstest?
| - [BREAKING] Removed `AccountStorageMode::Network`; network accounts are now identified via `NetworkAccountNoteAllowlist` ([#2900](https://github.com/0xMiden/protocol/pull/2900)). | ||
| - Added `PswapAttachment` scheme and `PswapNote::payback_note` / `remainder_note` discovery helpers so creators can reconstruct private paybacks from on-chain commitments ([#2909](https://github.com/0xMiden/protocol/pull/2909)). | ||
| - Added benchmark for ECDSA signed transaction ([#2967](https://github.com/0xMiden/protocol/pull/2967)). | ||
| - [agglayer] Added `bridge_config::deregister_faucet` MASM procedure, `DEREGISTER_AGG_BRIDGE` note script, and `DeregisterAggBridgeNote` Rust builder, enabling the bridge admin to revoke a faucet's authorization by clearing its `faucet_registry_map`, `token_registry_map`, and `faucet_metadata_map` entries. The token-registry key is recomputed from the faucet's stored metadata so the cleared key always matches what registration wrote, rather than trusting note-supplied values. `bridge_in::claim` now also re-checks `assert_faucet_registered` after the token lookup, making deregistration an unconditional revocation even if a stale token-registry key survives a re-registration ([#2838](https://github.com/0xMiden/protocol/pull/2838)). |
Closes #2705. Rebased onto
next(the agglayer crate now lives there); base changed fromagglayertonext.Adds a bridge-admin-gated
deregister_faucetMASM procedure that fully revokes a faucet, clearing every entry registration wrote for it:faucet_registry_map,token_registry_map, and all fourfaucet_metadata_mapsub-keys. Wired through a newDEREGISTER_AGG_BRIDGEnote script and aDeregisterAggBridgeNoteRust builder.The token registry is pair-keyed on
(origin_token_address, origin_network). Rather than trusting note-supplied values,deregister_faucetreads the faucet's registered address and network back from its ownfaucet_metadata_map(viaget_faucet_conversion_info) and recomputes the key, so the cleared entry is provably the oneregister_faucetwrote and the registries cannot desynchronize. The note therefore carries only the faucet id (2 felts).bridge_in::claimnow also re-checksassert_faucet_registeredafter the token lookup. Together withbridge_out's existing check, this makes deregistration an unconditional revocation: a deregistered faucet can never mint or unlock, even via a staletoken_registrykey that a re-registration under a different token identity might leave behind (register_faucetdoes not clear the prior key).Changes
bridge_config.masm: newderegister_faucetproc reusingassert_sender_is_bridge_admin,assert_faucet_registered,get_faucet_conversion_info, andhash_token_address.components/bridge.masm: re-exportderegister_faucet.note_scripts/deregister_agg_bridge.masm: new note script (carries only the faucet id).src/deregister_note.rs: newDeregisterAggBridgeNotebuilder.bridge.rs: add the DEREGISTER script root to the network-account note allowlist.bridge_in.masm: re-checkassert_faucet_registeredinclaim.ERR_FAUCET_NOT_REGISTERED/ERR_SENDER_NOT_BRIDGE_ADMIN.SPEC.md,CHANGELOG.md: documentation.Follow-up (out of scope)
register_faucetdoes not clear a faucet's priortoken_registrykey when re-registered under a different(origin_token_address, origin_network). The claim-sideassert_faucet_registeredcheck above neutralizes any security impact, but clearing the stale key inregister_faucetwould be a tidy follow-up.