fix(agglayer): backport AuthNetworkAccount bug fixes for audit verification#2951
Open
partylikeits1983 wants to merge 7 commits into
Open
fix(agglayer): backport AuthNetworkAccount bug fixes for audit verification#2951partylikeits1983 wants to merge 7 commits into
partylikeits1983 wants to merge 7 commits into
Conversation
* feat(kernel): expose tx script root via public kernel procedure Adds tx_get_script_root to the transaction kernel API so that MASM auth code can read the executed tx script root (zero word if no script ran). Required by the upcoming NetworkAccount auth component (#2814). - lib/memory.masm: new get_tx_script_root word getter - lib/tx.masm: re-export as tx::get_script_root - api.masm: new tx_get_script_root public kernel procedure - kernel_proc_offsets.masm: new TX_GET_SCRIPT_ROOT_OFFSET=51 - protocol/tx.masm: new user-facing tx::get_script_root wrapper - tests: returns expected root when script is set, zero word otherwise Part of #2813, related to #2797. * docs(changelog): add entry for tx::get_script_root kernel procedure * refactor: rename tx script root kernel procedure to retain `tx_script` in identifiers Keep `tx_script` in the names to match the well-defined concept, per review on #2816. Renames: - kernel lib `tx::get_script_root` -> `tx::get_tx_script_root` - kernel api proc `tx_get_script_root` -> `tx_get_tx_script_root` - offset const `TX_GET_SCRIPT_ROOT_OFFSET` -> `TX_GET_TX_SCRIPT_ROOT_OFFSET` - user-facing wrapper `tx::get_script_root` -> `tx::get_tx_script_root` Updates call sites and the CHANGELOG entry accordingly. * style: cargo fmt * refactor: use "empty word" in tx script root docs --------- Co-authored-by: Bobbin Threadbare <43513081+bobbinth@users.noreply.github.com>
* feat(protocol): add NoteScriptRoot newtype * docs: add changelog entry for NoteScriptRoot * refactor(protocol): drop NoteScriptRoot::as_inner, unify on Word::from / into Addresses PR #2851 review: prefer the existing Into<Word> conversion over a bespoke as_inner accessor. Callsites that need a Word now use Word::from(...) or .into() depending on whether the surrounding expression provides a target type.
* feat(kernel): expose tx script root via public kernel procedure Adds tx_get_script_root to the transaction kernel API so that MASM auth code can read the executed tx script root (zero word if no script ran). Required by the upcoming NetworkAccount auth component (#2814). - lib/memory.masm: new get_tx_script_root word getter - lib/tx.masm: re-export as tx::get_script_root - api.masm: new tx_get_script_root public kernel procedure - kernel_proc_offsets.masm: new TX_GET_SCRIPT_ROOT_OFFSET=51 - protocol/tx.masm: new user-facing tx::get_script_root wrapper - tests: returns expected root when script is set, zero word otherwise Part of #2813, related to #2797. * docs(changelog): add entry for tx::get_script_root kernel procedure * feat(standards): add NetworkAccount auth component Adds a new auth component intended for network-owned accounts such as the AggLayer bridge and the AggLayer faucet. Replaces the NoAuth pattern which lets any transaction emit output notes authored by the account. The auth procedure enforces two invariants: 1. Rejects the transaction if a tx script was executed, using `tx::get_script_root` from the kernel. 2. Rejects the transaction if any consumed input note's script root is not present in the whitelist stored at a well-known storage slot. If both checks pass, the nonce is incremented when the account state changed or the account is new, matching NoAuth/SingleSig behavior. The whitelist is a StorageMap keyed by script root (with a sentinel value marking presence) and is fixed at account creation. A mutable whitelist is a possible future improvement. - new network_account.masm auth component - new NetworkAccount Rust struct with From<NetworkAccount> for AccountComponent - register library in components::mod and extend StandardAccountComponent - extend AccountComponentInterface and AccountInterfaceExt with AuthNetworkAccount variant (maps to AuthMethod::NoAuth for now) - unit tests for the Rust builder - integration tests in tests/auth/network_account.rs covering tx-script-reject, non-whitelisted-note-reject, and happy-path Part of #2814. Depends on #2813 (merged via #2816). Related to the #2797 fix. * docs(changelog): add entry for NetworkAccount auth component * style: apply rustfmt to NetworkAccount files * style(standards): address NetworkAccount review nits Drop the stale AggLayer-faucet reference in the doc comment (the component is used by network faucets more generally) and rewrite the WHITELIST_SENTINEL comment to explain why a non-empty marker value is needed (storage maps use the empty word as the "key absent" marker). Also shorten the fully qualified miden_protocol::Felt path to use a direct Felt import, matching the pattern used elsewhere in the crate. * docs(standards): add stack comment after nonce-increment block in network_account.masm Document the post-stack state after the if.true/end block that conditionally increments the nonce, matching the commenting style used by the rest of the procedure. * refactor: rename tx script root kernel procedure to retain `tx_script` in identifiers Keep `tx_script` in the names to match the well-defined concept, per review on #2816. Renames: - kernel lib `tx::get_script_root` -> `tx::get_tx_script_root` - kernel api proc `tx_get_script_root` -> `tx_get_tx_script_root` - offset const `TX_GET_SCRIPT_ROOT_OFFSET` -> `TX_GET_TX_SCRIPT_ROOT_OFFSET` - user-facing wrapper `tx::get_script_root` -> `tx::get_tx_script_root` Updates call sites and the CHANGELOG entry accordingly. * refactor: follow kernel rename to `tx::get_tx_script_root` in NetworkAccount Update the auth call site and drop the stale CHANGELOG bullet left behind by the merge of `ajl-kernel-tx-script-root`. * style: cargo fmt * style: cargo fmt * docs(standards): clarify NetworkAccount sentinel comment and rephrase example accounts Reword the doc-comment example from "AggLayer bridge and a network faucet" to "network faucets and the AggLayer bridge" to foreground the general case over the specific deployment. Expand the WHITELIST_SENTINEL comment to define the term "sentinel value" and explain why we call the constant one: we only check that the stored value differs from the empty word, so the contents carry no information on their own. * Update crates/miden-standards/src/account/auth/network_account.rs Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com> * Update crates/miden-standards/asm/account_components/auth/network_account.masm Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com> * Update crates/miden-standards/asm/account_components/auth/network_account.masm Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com> * refactor(standards): rename to NoteScriptAllowlistAuth and split allowlist primitives * fix(standards): update note script allowlist test body * refactor(standards): rename NoteScriptAllowlistAuth to AuthNetworkAccount Re-frame the auth component around the network account use case per review feedback. Keeps the shared `note_script_allowlist` primitives module (assert_no_tx_script / assert_all_input_notes_allowed) untouched. * style: collapse two match arms to satisfy rustfmt The renamed `AuthNetworkAccount` identifier is shorter than the original, so rustfmt collapses these arms onto a single (or fewer) line(s). * refactor(standards): address final NetworkAccount review nits - Use `AuthNetworkAccount` consistently in network_account.masm doc comments (replaces the two stray `NetworkAccountAuth` references). - Drop `components::` from `AuthNetworkAccount::NAME` so the component name is a prefix of its storage-slot name (`miden::standards::auth::network_account::allowed_note_scripts`). The same `components::` cleanup is needed for the other auth components (no_auth, singlesig, singlesig_acl, multisig, guarded_multisig) and is tracked separately, per Bobbin's note that it should not land in this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(testing): convert NoteScriptRoot to Word at allowlist callsite Adapts the network-account auth test to the NoteScriptRoot newtype introduced in #2851; matches the repo convention of converting at the boundary rather than widening the AuthNetworkAccount API. * fix: change wording in comment Co-authored-by: Marti <marcin.gorny.94@protonmail.com> * refactor(standards): address final review nits on AuthNetworkAccount - Drop the 'replaces NoAuth' line from the AuthNetworkAccount docstring - Rename ALLOWED_SENTINEL to ALLOWED_FLAG - Replace single-disallowed-note test with a mixed allowed/disallowed case --------- Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Bobbin Threadbare <43513081+bobbinth@users.noreply.github.com> Co-authored-by: Marti <marcin.gorny.94@protonmail.com>
…k account detection (#2883) * feat: Introduce dedicated `NetworkAccountNoteAllowlist` type * chore: add changelog * chore: implement reconstruction from storage * chore: Use `NoteScriptRoot` rather than `Word` * chore: change from `Vec` to `BTreeSet` * fix: no-std compilation * fix: no-std compilation * fix: rustfmt * chore: reject empty allowlist
…nd faucet Adapted backport of #2818 (fixes #2797) onto the `agglayer` branch. Original PR #2818 by Alexander John Lee (partylikeits1983). #2818 landed on `next`, where the AggLayer faucet had already been refactored onto a `TokenPolicyManager` policy subsystem absent from `agglayer`, so the commit could not be cherry-picked verbatim. This applies #2818's genuine security delta onto agglayer's existing faucet/bridge structure: - Install `AuthNetworkAccount` on the bridge and faucet builders, replacing `NoAuth`. Both accounts now reject any transaction that runs a tx script or consumes a note whose script root is outside a fixed allowlist, closing the forged-MINT path where a transaction against the bridge could emit a bridge-authored MINT note. - Add `AggLayerBridge::allowed_notes()` (CLAIM, B2AGG, CONFIG_AGG_BRIDGE, UPDATE_GER) and `AggLayerFaucet::allowed_notes()` (MINT, BURN). - Add `claim_script_root()` accessor. - build.rs: compute code commitments with `AuthNetworkAccount` and a placeholder allowlist (the allowlist lives in storage, not code). - Add the two `network_account_regression` tests. The unrelated `ClaimNote` API refactor and faucet-policy churn bundled into #2818 are intentionally not backported. Also drops CHANGELOG entries (#2794, #2856) that leaked in as cherry-pick context.
…2917) * fix(bridge): pass bridge account by reference in is_ger_registered * fix(bridge): pass bridge account by reference at call site in update_ger test --------- Co-authored-by: Marti <marti@miden.team>
The `@note_script` / `pub proc` entrypoint syntax used by the upstream #2817 and #2818 tests is `next`-only; agglayer's note-script compiler requires the `begin ... end` form. Rewrites the custom attack/disallowed note scripts accordingly so the AuthNetworkAccount regression tests compile and pass on the `agglayer` branch.
Contributor
Author
mmagician
added
agglayer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
The
agglayerbranch was audited externally. The bug-report issues opened during that audit have since been fixed, but the fixes mostly landed onnext(now 97+ commits ahead ofagglayer). This PR backports the relevant fixes onto a branch based onagglayerso the auditors can verify them against the branch they audited.Audit-status of the four
agglayerbug issuesNoAuthon the bridge account lets anyone emit bridge-authored MINT notes #2797 -NoAuthon the bridge lets anyone emit bridge-authored MINT notes - fixed in this PR (see "Adapted fix" below).agglayerby fix(AggLayer): key faucet registry by (origin_network, origin_token_address) #2860; no action needed.u32downscaling in the claim service leave valid deposits unclaimable #2804 - Exact-divisibility /u32downscaling leaves deposits unclaimable - fixed in the external repo gateway-fm/miden-agglayer#32; not present in this repo.Commits
Faithful cherry-picks (dependencies of the #2797 fix):
feat(kernel): expose tx script root via public kernel procedureNoteScriptRootnewtype #2851 -feat(protocol): add NoteScriptRoot newtypeNetworkAccountauth component #2817 -feat(standards): add NetworkAccount auth componentNetworkAccountNoteAllowlistslot for network account detection #2883 -chore: add standardized NetworkAccountNoteAllowlist slotfix(bridge): pass bridge account by reference in is_ger_registeredAdapted fix:
fix(agglayer): replace NoAuth with AuthNetworkAccount on bridge and faucet- adapted backport of fix(agglayer): replaceNoAuthwithAuthNetworkAccounton bridge and faucet #2818 (fixesNoAuthon the bridge account lets anyone emit bridge-authored MINT notes #2797). fix(agglayer): replaceNoAuthwithAuthNetworkAccounton bridge and faucet #2818 could not be cherry-picked verbatim: it was authored on anexttree where the AggLayer faucet had been refactored onto aTokenPolicyManagerpolicy subsystem absent fromagglayer. Only fix(agglayer): replaceNoAuthwithAuthNetworkAccounton bridge and faucet #2818's genuine security delta is applied here, onto agglayer's existing faucet/bridge structure. The unrelatedClaimNoteAPI refactor and faucet-policy churn bundled into fix(agglayer): replaceNoAuthwithAuthNetworkAccounton bridge and faucet #2818 are intentionally not backported.Test adaptation:
test: adapt AuthNetworkAccount test note scripts to agglayer note syntax- the@note_script/pub procentrypoint syntax used by the upstream feat(standards): addNetworkAccountauth component #2817/fix(agglayer): replaceNoAuthwithAuthNetworkAccounton bridge and faucet #2818 tests isnext-only; rewritten to thebegin ... endform agglayer's note-script compiler requires.Notes on faithfulness
NoteScriptRootnewtype #2851'sWordWrapperderive was re-pointed from thenext-onlymiden_crypto_derivecrate to the identical in-repomiden_protocol_macros::WordWrapperalready present onagglayer- same macro, different crate name. This avoids dragging in an unrelatedmiden-crypto0.23 -> 0.25 bump.AccountProcedureRootusage overWord#2914 (Prefer AccountProcedureRoot over Word) was evaluated and found unnecessary once fix(agglayer): replaceNoAuthwithAuthNetworkAccounton bridge and faucet #2818 is adapted, so it is not included.Verification
cargo check --workspace --all-targetspasses.cargo test -p miden-standards- 102 tests pass.cargo test -p miden-testingagglayer suite - 55 tests pass, including the two newnetwork_account_regressiontests (bridge_rejects_tx_script,bridge_rejects_non_allowlisted_input_note).get_tx_script_roottests pass.🤖 Generated with Claude Code