Introduce note sealing via output_note::seal

[mmagician]

To address the issue of someone changing the attachment of a note in an unintended way, we can introduce a concept of note sealing. That is, the user could call something like output_note::seal to seal a given output note, and after this point the note would become immutable.

Originally posted by @bobbinth in #2109

This issue is to discuss whether we want to introduce note sealing or not.

I don't want to get bogged down with the implementation details, especially if we decide sealing is not needed, but I think it would roughly work as follows:

1. Developers can call output_note::seal at any time after the note has been created.
2. This will set some flag on this note that it's been sealed. Probably there will be a memory section allocated to this flag, somewhere following the OUTPUT_NOTE_SECTION_OFFSET. Let's name it OUTPUT_NOTE_IS_SEALED.
3. When any procedure call tries to modify an output note, the value at OUTPUT_NOTE_IS_SEALED is checked (get_sealed_flag assertz)

My initial thinking is that while the implementation wouldn't be all that complicated, it's an unnecessary addition:

1. Sealing is optional and most developers will never need it.
2. Since the note creation script is fully local to the caller, there is no risk of an outside party interfering with the sealing process.

So unless we can come up with a concrete malicious scenario posing a security risk, which can only be mitigated by sealing, I'd strongly prefer not to introduce it.
Let's discuss

[PhilippGackstatter]

In general, I would like to avoid introducing note sealing as much as possible to keep APIs simple, but I'm no longer sure we can do this.

Consider the following scenario:

• Alice creates a SWAP(USDC=500, ETH=2) note with a private payback note.
  • The SWAP note takes the exact parameters for the P2ID payback note as inputs, and so Alice can control exactly what the payback note will look like. She also knows exactly that the payback note will contain the asset ETH = 2. This means once the SWAP note is consumed, Alice can simply check that the P2ID payback note she derives from those details exists on chain and fetch a witness for it.
• Eve consumes the SWAP note, in the process creating the private P2ID payback note with ETH = 2. This is fully encoded in the SWAP script and mandated by it. This note will be created exactly as is.
  • But, Eve can deterministically guess the note index of the payback note and then, in the tx script that she controls, either:
    • Overwrite the P2ID note's attachment.
    • Add some useless asset to the note.

After Eve's transaction, the payback note for Alice is unspendable because she doesn't know the note details of the note anymore, because either the overwritten attachment or the additional asset changed the note commitment.

The only real way to solve this is through:

• Require that all note parameters are provided to output_note::create, i.e. including assets and attachment, and remove the set_attachment and add_asset APIs.
• Introduce output_note::seal.

Both of these make it possible for Alice to write a SWAP script that guarantees to create a payback note whose note details she can fully derive deterministically and that cannot be overwritten by the consumer. In the sealing case, the SWAP note would be written to seal the payback note after assets and attachment have been set.

Separately to this, I think if we introduce sealing, we should make it mandatory. That is, if an output note is not sealed by the end of the transaction, the kernel panics.
Assuming note sealing isn't mandatory, attacks like the above are way too subtle for most users to come up with and they'll simply either think they don't need note sealing or simply forget the seal API even exists, because it would be rarely used and not on anyone's mind. The only fix for this I can think of is to make sealing mandatory which alerts users to an unsealed note in the development phase. This would be fairly easy to explain too: Create a note, add as many assets as you want or set an attachment, and finish by sealing. It's like a builder API.

Rust test showing the above scenario by overwriting the attachment (run in swap.rs)

/// Tests the vulnerability where a consumer can mutate a precomputed payback note by setting
/// an attachment via a transaction script, making the note unspendable by the original sender.
///
/// This test demonstrates the attack scenario described in sealing.md:
/// - Alice creates a SWAP note with a private payback note that she can precompute
/// - Eve consumes the SWAP note and the payback note is created as expected
/// - Eve's transaction script then mutates the payback note by setting an attachment
/// - The final payback note commitment differs from what Alice precomputed, making it unspendable
#[tokio::test]
async fn consume_swap_note_with_attachment_mutation() -> anyhow::Result<()> {
    let payback_note_type = NoteType::Private;
    let SwapTestSetup {
        mock_chain,
        sender_account,
        mut target_account,
        offered_asset,
        requested_asset,
        swap_note,
        payback_note,
    } = setup_swap_test(payback_note_type)?;

    // PRECOMPUTE THE EXPECTED PAYBACK NOTE
    // --------------------------------------------------------------------------------------------
    // Alice precomputes the exact payback note she expects to receive
    let precomputed_payback_note = create_p2id_note_exact(
        target_account.id(),
        sender_account.id(),
        vec![requested_asset],
        payback_note_type,
        payback_note.serial_num(),
    )?;

    // CONSUME SWAP NOTE WITH TX SCRIPT THAT MUTATES THE PAYBACK NOTE
    // --------------------------------------------------------------------------------------------
    // Eve consumes the SWAP note and uses a transaction script to mutate the payback note
    // by setting an attachment on it
    let malicious_attachment = Word::from([Felt::new(1), Felt::new(2), Felt::new(3), Felt::new(4)]);
    let attachment_kind = 1u32; // ATTACHMENT_KIND_WORD
    let attachment_scheme = 42u32; // Some arbitrary user-defined scheme

    let tx_script_src = &format!(
        "
        use miden::protocol::output_note
        begin
            # The SWAP note script creates the payback note at index 0
            # Eve's malicious script mutates it by setting an attachment
            # Stack needs to be: [note_idx, attachment_scheme, attachment_kind, ATTACHMENT]
            push.{felt0}  # ATTACHMENT element 0 (top of stack after all pushes)
            push.{felt1}  # ATTACHMENT element 1
            push.{felt2}  # ATTACHMENT element 2
            push.{felt3}  # ATTACHMENT element 3 (bottom of ATTACHMENT word)
            push.{attachment_kind}
            push.{attachment_scheme}
            push.0  # note_idx
            exec.output_note::set_attachment
        end
        ",
        attachment_scheme = attachment_scheme,
        attachment_kind = attachment_kind,
        felt0 = malicious_attachment[0],
        felt1 = malicious_attachment[1],
        felt2 = malicious_attachment[2],
        felt3 = malicious_attachment[3],
    );

    let tx_script = CodeBuilder::default().compile_tx_script(tx_script_src)?;

    let consume_swap_note_tx = mock_chain
        .build_tx_context(target_account.id(), &[swap_note.id()], &[])
        .context("failed to build tx context")?
        .tx_script(tx_script)
        .build()?
        .execute()
        .await?;

    target_account
        .apply_delta(consume_swap_note_tx.account_delta())
        .context("failed to apply delta to target account")?;

    let output_payback_note = consume_swap_note_tx.output_notes().iter().next().unwrap().clone();

    // VERIFY THAT THE PAYBACK NOTE WAS MUTATED
    // --------------------------------------------------------------------------------------------
    // The note ID matches what Alice expected (same serial number, recipient, etc.)
    assert_eq!(output_payback_note.id(), payback_note.id());
    assert_eq!(output_payback_note.id(), precomputed_payback_note.id());

    // However, the note commitment differs because Eve mutated the attachment
    // This makes the note unspendable by Alice since she doesn't know the attachment details
    assert_ne!(
        output_payback_note.commitment(),
        precomputed_payback_note.commitment(),
        "Payback note commitment should differ due to attachment mutation"
    );

    // The asset is still correct
    assert_eq!(output_payback_note.assets().unwrap().iter().next().unwrap(), &requested_asset);

    // Eve successfully received the offered asset
    assert!(target_account.vault().assets().count() == 1);
    assert!(target_account.vault().assets().any(|asset| asset == offered_asset));

    Ok(())
}

Context Tracking

A solution that doesn't require sealing would be restricting note mutation to the context that created the note. So, if a note script like a SWAP note creates a note, only it can add assets or set the attachment. Mutating it from the tx script would fail.

First conceptual idea:

• On output_note::create, store the caller hash of the context that created the note.
• On output_note::{add_asset, set_attachment}, check that caller matches the creator's caller hash.

This doesn't work quite that easily unfortunately, because the SWAP note does:

• output_note::create -> store caller_hash = SWAP_HASH
• call.wallet::move_asset_to_note -> different caller hash and caller_hash != SWAP_HASH. So this prevents a legitimate way of adding an asset to a note, i.e. it is too restrictive.

The alternative is changing the "context" on every note::prepare_note call, when the tx script executes and when the auth procedure executes. And then as long as the SWAP note is executing, mutating the note is fine since the "context" from which the asset is added is the SWAP context. This requires a bit more thinking about whether that eliminates any use cases, but if not, would be a better alternative than sealing, since it doesn't require any user interaction.