From 9cbcc6706c4efced336cdf7efe71b0fcc611f3d6 Mon Sep 17 00:00:00 2001 From: Leila Wang Date: Wed, 4 Oct 2023 10:41:22 +0200 Subject: [PATCH] feat: constrain return notes from oracle call. (#2639) Constrain the return notes based on the values in `selects`, `sorts`, and `limit` in `NoteGetterOptions`. Custom filter and offset are still unconstrained. # Checklist: Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge. - [ ] If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag. - [ ] I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code. - [ ] Every change is related to the PR description. - [ ] I have [linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue) this pull request to relevant issues (if any exist). --- .../docs/dev_docs/contracts/syntax/storage.md | 8 ++-- .../aztec-nr/aztec/src/note/note_getter.nr | 39 ++++++++++++++++++- .../aztec/src/note/note_getter_options.nr | 6 --- yarn-project/aztec-nr/aztec/src/types/vec.nr | 4 ++ .../docs_example_contract/src/main.nr | 17 +------- .../src/contracts/escrow_contract/src/main.nr | 8 +--- 6 files changed, 48 insertions(+), 34 deletions(-) diff --git a/docs/docs/dev_docs/contracts/syntax/storage.md b/docs/docs/dev_docs/contracts/syntax/storage.md index fc5c06c831a3..2503bbcae89f 100644 --- a/docs/docs/dev_docs/contracts/syntax/storage.md +++ b/docs/docs/dev_docs/contracts/syntax/storage.md @@ -433,6 +433,8 @@ This setting enables us to skip the first `offset` notes. It's particularly usef Developers have the option to provide a custom filter. This allows specific logic to be applied to notes that meet the criteria outlined above. The filter takes the notes returned from the oracle and `filter_args` as its parameters. +It's important to note that the process of applying the custom filter to get the final notes is not constrained. It's crucial to verify the returned notes even if certain assumptions are made in the custom filter. + #### `filter_args: FILTER_ARGS` `filter_args` provides a means to furnish additional data or context to the custom filter. @@ -495,10 +497,6 @@ We can use it as a filter to further reduce the number of the final notes: One thing to remember is, `filter` will be applied on the notes after they are picked from the database. Therefore, it's possible that the actual notes we end up getting are fewer than the limit. -The limit is `MAX_READ_REQUESTS_PER_CALL` by default. But we can set it to any value "smaller" than that: +The limit is `MAX_READ_REQUESTS_PER_CALL` by default. But we can set it to any value **smaller** than that: #include_code state_vars-NoteGetterOptionsPickOne /yarn-project/noir-contracts/src/contracts/docs_example_contract/src/options.nr rust - -The process of applying the options to get the final notes is not constrained. It's necessary to always check the returned notes even when some conditions have been specified in the options. - -#include_code state_vars-check_return_notes /yarn-project/noir-contracts/src/contracts/docs_example_contract/src/main.nr rust diff --git a/yarn-project/aztec-nr/aztec/src/note/note_getter.nr b/yarn-project/aztec-nr/aztec/src/note/note_getter.nr index c4d86b56093b..cfac7ed9986a 100644 --- a/yarn-project/aztec-nr/aztec/src/note/note_getter.nr +++ b/yarn-project/aztec-nr/aztec/src/note/note_getter.nr @@ -8,7 +8,7 @@ use crate::constants_gen::{ }; use crate::context::PrivateContext; use crate::note::{ - note_getter_options::{NoteGetterOptions, Select, Sort}, + note_getter_options::{NoteGetterOptions, Select, Sort, SortOrder}, note_interface::NoteInterface, note_viewer_options::NoteViewerOptions, utils::compute_note_hash_for_read_or_nullify, @@ -29,6 +29,29 @@ fn check_note_header( assert(header.storage_slot == storage_slot); } +fn check_note_fields( + fields: [Field; N], + selects: BoundedVec, N>, +) { + for i in 0..selects.len { + let select = selects.get_unchecked(i).unwrap_unchecked(); + assert(fields[select.field_index] == select.value, "Mismatch return note field."); + } +} + +fn check_notes_order(fields_0: [Field; N], fields_1: [Field; N], sorts: BoundedVec, N>) { + for i in 0..sorts.len { + let sort = sorts.get_unchecked(i).unwrap_unchecked(); + let eq = fields_0[sort.field_index] == fields_1[sort.field_index]; + let lt = fields_0[sort.field_index] as u120 < fields_1[sort.field_index] as u120; + if sort.order == SortOrder.ASC { + assert(eq | lt, "Return notes not sorted in ascending order."); + } else if !eq { + assert(!lt, "Return notes not sorted in descending order."); + } + } +} + fn get_note( context: &mut PrivateContext, storage_slot: Field, @@ -51,18 +74,32 @@ fn get_notes( options: NoteGetterOptions, ) -> [Option; MAX_READ_REQUESTS_PER_CALL] { let opt_notes = get_notes_internal(storage_slot, note_interface, options); + let mut num_notes = 0; + let mut prev_fields = [0; N]; for i in 0..opt_notes.len() { let opt_note = opt_notes[i]; if opt_note.is_some() { let note = opt_note.unwrap_unchecked(); + let serialize = note_interface.serialize; + let fields = serialize(note); check_note_header(*context, storage_slot, note_interface, note); + check_note_fields(fields, options.selects); + if i != 0 { + check_notes_order(prev_fields, fields, options.sorts); + } + prev_fields = fields; let note_hash_for_read_request = compute_note_hash_for_read_or_nullify(note_interface, note); // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1410): test to ensure // failure if malicious oracle injects 0 nonce here for a "pre-existing" note. context.push_read_request(note_hash_for_read_request); + + num_notes += 1; }; }; + if options.limit != 0 { + assert(num_notes <= options.limit, "Invalid number of return notes."); + } opt_notes } diff --git a/yarn-project/aztec-nr/aztec/src/note/note_getter_options.nr b/yarn-project/aztec-nr/aztec/src/note/note_getter_options.nr index f927cebd85a2..d8905b2452e3 100644 --- a/yarn-project/aztec-nr/aztec/src/note/note_getter_options.nr +++ b/yarn-project/aztec-nr/aztec/src/note/note_getter_options.nr @@ -16,13 +16,11 @@ impl Select { struct SortOrderEnum { DESC: u2, ASC: u2, - NADA: u2, } global SortOrder = SortOrderEnum { DESC: 1, ASC: 2, - NADA: 0, }; struct Sort { @@ -34,10 +32,6 @@ impl Sort { fn new(field_index: u8, order: u2) -> Self { Sort { field_index, order } } - - fn nada() -> Self { - Sort { field_index: 0, order: SortOrder.NADA } - } } fn return_all_notes(notes: [Option; MAX_READ_REQUESTS_PER_CALL], _p: Field) -> [Option; MAX_READ_REQUESTS_PER_CALL] { diff --git a/yarn-project/aztec-nr/aztec/src/types/vec.nr b/yarn-project/aztec-nr/aztec/src/types/vec.nr index 38fafa05d84d..589159eb805b 100644 --- a/yarn-project/aztec-nr/aztec/src/types/vec.nr +++ b/yarn-project/aztec-nr/aztec/src/types/vec.nr @@ -14,6 +14,10 @@ impl BoundedVec { self.storage[index] } + fn get_unchecked(mut self: Self, index: Field) -> T { + self.storage[index] + } + fn push(&mut self, elem: T) { assert(self.len as u64 < MaxLen as u64); diff --git a/yarn-project/noir-contracts/src/contracts/docs_example_contract/src/main.nr b/yarn-project/noir-contracts/src/contracts/docs_example_contract/src/main.nr index e005960119ba..0bb236529242 100644 --- a/yarn-project/noir-contracts/src/contracts/docs_example_contract/src/main.nr +++ b/yarn-project/noir-contracts/src/contracts/docs_example_contract/src/main.nr @@ -19,7 +19,7 @@ contract DocsExample { // docs:end:state_vars-PublicStateBoolImport use crate::account_contract_interface::AccountContractInterface; use crate::actions; - use crate::options::{create_account_card_getter_options, create_largest_account_card_getter_options}; + use crate::options::create_account_card_getter_options; use crate::types::{ card_note::{CardNote, CardNoteMethods, CARD_NOTE_LEN}, profile_note::{ProfileNote, ProfileNoteMethods, PROFILE_NOTE_LEN}, @@ -222,21 +222,6 @@ contract DocsExample { context.return_values.push(total_points as Field); } - // docs:start:state_vars-check_return_notes - #[aztec(private)] - fn discard_largest_card() { - - - let account = context.msg_sender(); - let options = create_largest_account_card_getter_options(account); - let card = actions::get_cards(storage.cards, options)[0].unwrap_unchecked(); - // highlight-next-line:state_vars-check_return_notes - assert(card.owner == account); - - actions::remove_card(storage.cards, card); - } - // docs:end:state_vars-check_return_notes - // docs:start:functions-UncontrainedFunction unconstrained fn get_total_points(account: Field) -> u8 { diff --git a/yarn-project/noir-contracts/src/contracts/escrow_contract/src/main.nr b/yarn-project/noir-contracts/src/contracts/escrow_contract/src/main.nr index 9eb7c7a21b90..ca6a648f51b6 100644 --- a/yarn-project/noir-contracts/src/contracts/escrow_contract/src/main.nr +++ b/yarn-project/noir-contracts/src/contracts/escrow_contract/src/main.nr @@ -67,15 +67,11 @@ contract Escrow { ) { let this = context.this_address(); let sender = context.msg_sender(); - - + // We don't remove note from the owners set. If a note exists, the owner and recipient are legit. let options = NoteGetterOptions::new().select(0, sender).select(1, this).set_limit(1); let notes = storage.owners.get_notes(options); - let note = notes[0].unwrap_unchecked(); - // Filter is not constrained. We still need to check if the note is what we expected. - assert(note.address == sender); - assert(note.owner == this); + assert(notes[0].is_some(), "Sender is not an owner."); let selector = compute_selector("transfer((Field),(Field),Field,Field)"); let _callStackItem = context.call_private_function(