-
Notifications
You must be signed in to change notification settings - Fork 106
add b2agg ntx tag check
#2189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
add b2agg ntx tag check
#2189
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a security check to the B2AGG note script to prevent unauthorized consumption of in-flight B2AGG notes. The check ensures that if a B2AGG note is not being reclaimed by its creator, it can only be consumed by the intended bridge account by comparing the note's tag with the consuming account's network tag.
Key changes:
- Added
assert_account_id_is_targetprocedure to verify the consuming account matches the note's intended recipient via tag comparison - Updated test bridge accounts to use
AccountStorageMode::Networkinstead ofPublic - Modified note tag generation to use
NoteTag::from_account_id()for proper network tag derivation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/miden-lib/asm/agglayer/note_scripts/B2AGG.masm |
Added security check procedure and integrated it into the non-reclaim branch to verify consuming account matches note tag |
crates/miden-testing/tests/agglayer/bridge_out.rs |
Updated bridge accounts to use Network storage mode and changed tag generation to use proper account-based tags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Verifies that the consuming account is the bridge contract by comparing note tags. | ||
| # Panics if the B2AGG note's tag doesn't match the consuming account's network tag. | ||
| #! | ||
| #! Inputs: [account_id_prefix, account_id_suffix] | ||
| #! Outputs: [] | ||
| proc assert_account_id_is_target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know very little about the agglayer design, so this is just a question. At first glance, this looks a lot like a target account ID check in a P2ID note, except we're using the note tag instead of note inputs. Why can we not use note inputs instead?
The way I understand it is that we want a B2AGG note to be only consumed by a specific, canonical network faucet instead of any faucet that has a matching account interface. If that's true, then its account ID should be known and we should be able to use note inputs instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we could add it to the note inputs, but then we'd have to increase the size of the note inputs of the B2AGG note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with #2109 in mind, it will be easier to rely on note inputs instead of the format we'll end up using for network account targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but then we'd have to increase the size of the note inputs of the B2AGG note
I think that would be preferable indeed to include it in note inputs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds like a plan, will refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought a bit about your comment on this topic in the call. I think it could be fine to check the target account ID via the note attachment, in the future.
We'll only have NoteAttachment::Raw and NoteAttachment::Commitment after #2109 and I personally would like to avoid adding a NetworkAccount variant. Primarily because network notes and txs have very little support at the protocol level and they already work fine. Instead of enshrining more network transaction things at the protocol level, it'd be amazing to make network notes a miden standard instead of a miden protocol feature, to keep the protocol lean. The few things we do have in the protocol can be done in other ways now that we have named storage slots (detecting network accounts) and once we have user-extensible note attachments (detecting network account targets). Specifically, the last part, relevant here, can be specified as a miden standard.
So, checking via the note tag/attachment is possible but would make the agglayer more dependent on the network transaction "standard", i.e. the layout of how target account IDs are encoded in a note attachment. That's probably fine, because the agglayer already strongly assumes network transactions, afaict, but it's an additional thing to keep in mind.
On the other hand, having the target account ID in the note inputs sidesteps the network transaction specifics, as it becomes simply part of the "agglayer standard". This comes at the cost of being less efficient and encoding the account ID twice.
So, to summarize, I think checking via the tag/attachment is fine and probably a good idea to avoid that inefficiency. Also, the network transactions as a standard instead of a protocol feature is only an idea at this point that needs to be discussed more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the question is whether we'd want to make note attachments accessible programmatically. My initial thinking was that no - but now I'm wondering if maybe we should do this. So, then this boils down to:
- If note attachment is not accessible programmatically, the target account ID would need to go into note inputs.
- Otherwise, we could check note attachments and that's maybe a better option (and this pattern would probably be used in most network transactions).
If we decide to go with option 1, we can refactor this PR now. If we go with option 2, we should wait until #2109 is implemented.
I am leaning slightly toward option 2.
Resolves: #2173
This PR adds a check to the
B2AGGnote that ensures that if it is not being reclaimed, it is being consumed by the agglayer bridge.The
B2AGGnote checks the following:B2AGG.note_tag()==current_consuming_account.id().network_tag()This prevents a possible attack vector where someone could consume inflight
B2AGGnotes before that agglayer bridge.