feat: validating partial note sender

noir-projects/aztec-nr/uint-note/src/uint_note.nr

@@ -7,7 +7,10 @@ use dep::aztec::{
     oracle::random::random,
     protocol_types::{
         address::AztecAddress,
-        constants::{GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER},
+        constants::{
+            GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER,
+            GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT,
+        },
         hash::poseidon2_hash_with_separator,
         traits::{Deserialize, Hash, Packable, Serialize, ToField},
         utils::arrays::array_concat,
@@ -201,12 +204,16 @@ pub struct PartialUintNote {
 }
 
 impl PartialUintNote {
-    pub fn commitment(self) -> Field {
-        self.commitment
+    /// Computes a partial note validity commitment. This commitment is stored in public storage when creating the
+    /// partial note in private and then is used to check that completer and note are valid upon partial note
+    /// completion in public (completer is the entity that can complete the partial note).
+    pub fn compute_validity_commitment(self, completer: AztecAddress) -> Field {
+        poseidon2_hash_with_separator(
+            [self.commitment, completer.to_field()],
+            GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT,
+        )
     }
-}
 
-impl PartialUintNote {
     /// Completes the partial note, creating a new note that can be used like any other UintNote.
     pub fn complete(self, value: u128, context: &mut PublicContext) {
         // A note with a value of zero is valid, but we cannot currently complete a partial note with such a value

noir-projects/noir-contracts/contracts/app/nft_contract/src/main.nr

@@ -197,16 +197,30 @@ pub contract NFT {
         context: &mut PrivateContext,
         storage: Storage<&mut PrivateContext>,
     ) -> PartialNFTNote {
+        let sender_and_completer = context.msg_sender();
+
         // We create a partial note with unpopulated/zero token id for 'to'
         let partial_note = NFTNote::partial(
             to,
             storage.private_nfts.at(to).storage_slot,
             context,
             to,
-            context.msg_sender(),
+            sender_and_completer,
         );
 
-        NFT::at(context.this_address())._store_nft_set_partial_note(partial_note).enqueue(context);
+        // We can't simply return the partial note because we won't be able to later on verify that it was created
+        // correctly (e.g. that the storage slot corresponds to the owner, and that we're using the `private_nfts`
+        // state variable and not a different one) once this information is hidden in the partial note commitment.
+        // We also want to verify that only the caller of this function can complete the partial note.
+        //
+        // We achieve the above by storing a flag in public storage under a slot index equal to its
+        // `validity_commitment`. This commitment contains all the necessary information to verify the completer and
+        // to validate the partial note.
+        let validity_commitment = partial_note.compute_validity_commitment(sender_and_completer);
+
+        NFT::at(context.this_address())._set_nft_partial_note_validity(validity_commitment).enqueue(
+            context,
+        );
 
         partial_note
     }
@@ -215,56 +229,78 @@ pub contract NFT {
     // docs:start:store_payload_in_transient_storage_unsafe
     #[public]
     #[internal]
-    fn _store_nft_set_partial_note(partial_note: PartialNFTNote) {
-        // We store the partial note in a slot equal to its commitment. This is safe because the commitment is computed
-        // using a generator different from the one used to compute storage slots, so there can be no collisions.
-        // We could consider storing all pending partial notes in e.g. some array, but ultimately this is pointless: all
-        // we need to verify is that the note is valid.
-        context.storage_write(partial_note.commitment(), true);
+    fn _set_nft_partial_note_validity(validity_commitment: Field) {
+        // We store the partial note validity flag in a slot equal to its validity commitment. This is safe because
+        // the commitment is computed using a generator different from the one used to compute storage slots, so there
+        // can be no collisions. We could consider storing all pending partial notes in e.g. some array, but ultimately
+        // this is pointless: all we need to verify is that the note is valid.
+        context.storage_write(validity_commitment, true);
     }
     // docs:end:store_payload_in_transient_storage_unsafe
-    /// Finalizes a transfer of NFT with `token_id` from public balance of `from` to a private balance of `to`.
-    /// The transfer must be prepared by calling `prepare_private_balance_increase` first and the resulting
-    /// `partial_note` must be passed as an argument to this function.
+    /// Finalizes a transfer of NFT with `token_id` from public balance of `msg_sender` to a private balance of `to`.
+    /// The transfer must be prepared by calling `prepare_private_balance_increase` from `msg_sender` account and the
+    /// resulting `partial_note` must be passed as an argument to this function.
     // docs:start:finalize_transfer_to_private
     #[public]
     fn finalize_transfer_to_private(token_id: Field, partial_note: PartialNFTNote) {
-        let from = context.msg_sender();
-        _finalize_transfer_to_private(from, token_id, partial_note, &mut context, storage);
+        // Completer is the entity that can complete the partial note. In this case, it's the same as the account
+        // `from` from whose account the token is being transferred.
+        let from_and_completer = context.msg_sender();
+        _finalize_transfer_to_private(
+            from_and_completer,
+            token_id,
+            partial_note,
+            &mut context,
+            storage,
+        );
     }
     // docs:end:finalize_transfer_to_private
 
     // docs:start:finalize_transfer_to_private_unsafe
+    /// This is a wrapper around `_finalize_transfer_to_private` placed here so that a call
+    /// to `_finalize_transfer_to_private` can be enqueued. Called unsafe as it does not check `from_and_completer`
+    /// (this has to be done in the calling function).
     #[public]
     #[internal]
     fn _finalize_transfer_to_private_unsafe(
-        from: AztecAddress,
+        from_and_completer: AztecAddress,
         token_id: Field,
         partial_note: PartialNFTNote,
     ) {
-        _finalize_transfer_to_private(from, token_id, partial_note, &mut context, storage);
+        _finalize_transfer_to_private(
+            from_and_completer,
+            token_id,
+            partial_note,
+            &mut context,
+            storage,
+        );
     }
     // docs:end:finalize_transfer_to_private_unsafe
 
+    // In all the flows in this contract, `from` (the account from which we're transferring the NFT) and `completer`
+    // (the entity that can complete the partial note) are the same so we represent them with a single argument.
     #[contract_library_method]
     fn _finalize_transfer_to_private(
-        from: AztecAddress,
+        from_and_completer: AztecAddress,
         token_id: Field,
         partial_note: PartialNFTNote,
         context: &mut PublicContext,
         storage: Storage<&mut PublicContext>,
     ) {
         let public_owners_storage = storage.public_owners.at(token_id);
-        assert(public_owners_storage.read().eq(from), "invalid NFT owner");
+        assert(public_owners_storage.read().eq(from_and_completer), "invalid NFT owner");
 
         // Set the public NFT owner to zero
         public_owners_storage.write(AztecAddress::zero());
 
-        // We verify that the partial note we're completing is valid (i.e. it uses the correct state variable's storage
-        // slot, and it is internally consistent). We *could* clear the storage since each partial note should only be
-        // used once, but since the AVM offers no gas refunds for doing so this would just make the transaction be more
-        // expensive.
-        assert(context.storage_read(partial_note.commitment()), "Invalid partial note");
+        // We verify that the partial note we're completing is valid (i.e. completer is correct, it uses the correct
+        // state variable's storage slot, and it is internally consistent). We *could* clear the storage since each
+        // partial note should only be used once, but since the AVM offers no gas refunds for doing so this would just
+        // make the transaction be more expensive.
+        assert(
+            context.storage_read(partial_note.compute_validity_commitment(from_and_completer)),
+            "Invalid partial note or completer",
+        );
         partial_note.complete(token_id, context);
     }
 

noir-projects/noir-contracts/contracts/app/nft_contract/src/test/transfer_to_private.nr

@@ -47,7 +47,7 @@ unconstrained fn transfer_to_private_external_orchestration() {
     utils::assert_owns_public_nft(env, nft_contract_address, AztecAddress::zero(), token_id);
 }
 
-#[test(should_fail_with = "Invalid partial note")]
+#[test(should_fail_with = "Invalid partial note or completer")]
 unconstrained fn transfer_to_private_transfer_not_prepared() {
     // Setup without account contracts. We are not using authwits here, so dummy accounts are enough
     let (env, nft_contract_address, _, _, token_id) =
@@ -81,3 +81,27 @@ unconstrained fn transfer_to_private_failure_not_an_owner() {
         &mut env.public(),
     );
 }
+
+#[test(should_fail_with = "Invalid partial note or completer")]
+unconstrained fn incorrect_completer_cannot_complete_partial_note() {
+    // Setup without account contracts. We are not using authwits here, so dummy accounts are enough.
+    // We mint to the incorrect completer to avoid the test failing due to the NFT owner check (we want to fail on
+    // the partial note validity check).
+    let (env, nft_contract_address, incorrect_completer, recipient, token_id) =
+        utils::setup_and_mint(/* with_account_contracts */ false);
+
+    // We set up the completer
+    let completer = env.create_account(3);
+
+    // We prepare the partial note as a completer
+    env.impersonate(completer);
+    let partial_note = NFT::at(nft_contract_address)
+        .prepare_private_balance_increase(recipient)
+        .call(&mut env.private());
+
+    // Now we try to complete the partial note as the incorrect completer
+    env.impersonate(incorrect_completer);
+    NFT::at(nft_contract_address).finalize_transfer_to_private(token_id, partial_note).call(
+        &mut env.public(),
+    );
+}

noir-projects/noir-contracts/contracts/app/nft_contract/src/types/nft_note.nr

@@ -7,9 +7,12 @@ use dep::aztec::{
     oracle::random::random,
     protocol_types::{
         address::AztecAddress,
-        constants::{GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER},
+        constants::{
+            GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER,
+            GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT,
+        },
         hash::poseidon2_hash_with_separator,
-        traits::{Deserialize, Hash, Packable, Serialize},
+        traits::{Deserialize, Hash, Packable, Serialize, ToField},
         utils::arrays::array_concat,
     },
 };
@@ -202,12 +205,16 @@ pub struct PartialNFTNote {
 }
 
 impl PartialNFTNote {
-    pub fn commitment(self) -> Field {
-        self.commitment
+    /// Computes a partial note validity commitment. This commitment is stored in public storage when creating the
+    /// partial note in private and then is used to check that completer and note are valid upon partial note completion
+    /// in public (completer is the entity that can complete the partial note).
+    pub fn compute_validity_commitment(self, completer: AztecAddress) -> Field {
+        poseidon2_hash_with_separator(
+            [self.commitment, completer.to_field()],
+            GENERATOR_INDEX__PARTIAL_NOTE_VALIDITY_COMMITMENT,
+        )
     }
-}
 
-impl PartialNFTNote {
     /// Completes the partial note, creating a new note that can be used like any other NFTNote.
     pub fn complete(self, token_id: Field, context: &mut PublicContext) {
         // A note with a value of zero is valid, but we cannot currently complete a partial note with such a value

noir-projects/noir-contracts/contracts/app/simple_token_contract/src/main.nr

@@ -29,7 +29,6 @@ pub contract SimpleToken {
     };
 
     use dep::uint_note::uint_note::{PartialUintNote, UintNote};
-    use aztec::protocol_types::traits::ToField;
 
     use dep::authwit::auth::{
         assert_current_call_valid_authwit, assert_current_call_valid_authwit_public,
@@ -232,64 +231,100 @@ pub contract SimpleToken {
             from,
         );
 
+        let validity_commitment = partial_note.compute_validity_commitment(context.msg_sender());
+
         SimpleToken::at(context.this_address())
-            ._store_balances_set_partial_note(partial_note)
+            ._set_uint_partial_note_validity(validity_commitment)
             .enqueue(context);
 
         partial_note
     }
 
     #[public]
     fn finalize_transfer_to_private(amount: u128, partial_note: PartialUintNote) {
-        let from = context.msg_sender();
-        _finalize_transfer_to_private(from, amount, partial_note, &mut context, storage);
+        let from_and_completer = context.msg_sender();
+        _finalize_transfer_to_private(
+            from_and_completer,
+            amount,
+            partial_note,
+            &mut context,
+            storage,
+        );
     }
 
     #[public]
     #[internal]
     fn _finalize_transfer_to_private_unsafe(
-        from: AztecAddress,
+        from_and_completer: AztecAddress,
         amount: u128,
         partial_note: PartialUintNote,
     ) {
-        _finalize_transfer_to_private(from, amount, partial_note, &mut context, storage);
+        _finalize_transfer_to_private(
+            from_and_completer,
+            amount,
+            partial_note,
+            &mut context,
+            storage,
+        );
     }
 
     #[contract_library_method]
     fn _finalize_transfer_to_private(
-        from: AztecAddress,
+        from_and_completer: AztecAddress,
         amount: u128,
         partial_note: PartialUintNote,
         context: &mut PublicContext,
         storage: Storage<&mut PublicContext>,
     ) {
-        let from_balance = storage.public_balances.at(from).read().sub(amount);
-        storage.public_balances.at(from).write(from_balance);
+        let from_balance = storage.public_balances.at(from_and_completer).read().sub(amount);
+        storage.public_balances.at(from_and_completer).write(from_balance);
 
-        assert(context.storage_read(partial_note.commitment()), "Invalid partial note");
+        assert(
+            context.storage_read(partial_note.compute_validity_commitment(from_and_completer)),
+            "Invalid partial note or completer",
+        );
         partial_note.complete(amount, context);
     }
 
     #[private]
     fn mint_privately(from: AztecAddress, to: AztecAddress, amount: u128) {
         let token = SimpleToken::at(context.this_address());
         let partial_note = _prepare_private_balance_increase(from, to, &mut context, storage);
-        token._finalize_mint_to_private_unsafe(amount, partial_note).enqueue(&mut context);
+        token._finalize_mint_to_private_unsafe(context.msg_sender(), amount, partial_note).enqueue(
+            &mut context,
+        );
     }
 
     #[public]
     fn finalize_mint_to_private(amount: u128, partial_note: PartialUintNote) {
-        _finalize_mint_to_private(amount, partial_note, &mut context, storage);
+        _finalize_mint_to_private(
+            context.msg_sender(),
+            amount,
+            partial_note,
+            &mut context,
+            storage,
+        );
     }
 
     #[public]
     #[internal]
-    fn _finalize_mint_to_private_unsafe(amount: u128, partial_note: PartialUintNote) {
-        _finalize_mint_to_private(amount, partial_note, &mut context, storage);
+    fn _finalize_mint_to_private_unsafe(
+        minter_and_completer: AztecAddress,
+        amount: u128,
+        partial_note: PartialUintNote,
+    ) {
+        _finalize_mint_to_private(
+            minter_and_completer,
+            amount,
+            partial_note,
+            &mut context,
+            storage,
+        );
     }
 
     #[contract_library_method]
     fn _finalize_mint_to_private(
+        completer: AztecAddress,
         amount: u128,
         partial_note: PartialUintNote,
         context: &mut PublicContext,
@@ -298,14 +333,17 @@ pub contract SimpleToken {
         let supply = storage.total_supply.read().add(amount);
         storage.total_supply.write(supply);
 
-        assert(context.storage_read(partial_note.commitment()), "Invalid partial note");
+        assert(
+            context.storage_read(partial_note.compute_validity_commitment(completer)),
+            "Invalid partial note or completer",
+        );
         partial_note.complete(amount, context);
     }
 
     #[public]
     #[internal]
-    fn _store_balances_set_partial_note(partial_note: PartialUintNote) {
-        context.storage_write(partial_note.commitment(), true);
+    fn _set_uint_partial_note_validity(validity_commitment: Field) {
+        context.storage_write(validity_commitment, true);
     }
 
     #[public]