From 0f16ebe2cb7e3327b9edfb3a9f2f1f1eb9173ded Mon Sep 17 00:00:00 2001 From: benesjan Date: Thu, 8 Aug 2024 09:39:58 +0000 Subject: [PATCH] vuln fix --- docs/docs/migration_notes.md | 6 +++++- .../contracts/benchmarking_contract/src/main.nr | 2 +- .../contracts/card_game_contract/src/cards.nr | 1 + .../contracts/inclusion_proofs_contract/src/main.nr | 3 ++- .../contracts/token_blacklist_contract/src/main.nr | 3 ++- .../noir-contracts/contracts/token_contract/src/main.nr | 3 ++- .../contracts/token_with_refunds_contract/src/main.nr | 3 ++- .../src/e2e_blacklist_token_contract/minting.test.ts | 2 +- .../end-to-end/src/e2e_token_contract/minting.test.ts | 2 +- 9 files changed, 17 insertions(+), 8 deletions(-) diff --git a/docs/docs/migration_notes.md b/docs/docs/migration_notes.md index c418e4ff3615..919fa6ca3651 100644 --- a/docs/docs/migration_notes.md +++ b/docs/docs/migration_notes.md @@ -39,16 +39,20 @@ Token contract diff: - subtracted = subtracted + note.get_amount(); - } -} +-assert(minuend >= subtrahend, "Balance too low"); +let options = NoteGetterOptions::with_filter(filter_notes_min_sum, target_amount).set_limit(max_notes); +let notes = self.map.at(owner).pop_notes(options); +let mut subtracted = U128::from_integer(0); +for i in 0..options.limit { + if i < notes.len() { + let note = notes.get_unchecked(i); -+ self.map.at(owner).remove(note); + subtracted = subtracted + note.get_amount(); + } +} ++assert(minuend >= subtrahend, "Balance too low"); + +Note that the notes are not ensured to be obtained and removed so you have the place checks on the returned notes (e.g. in the example above by checking a sum of balances or by checking that returned note length assert(notes.len() == expected_num_notes)). + ## 0.47.0 diff --git a/noir-projects/noir-contracts/contracts/benchmarking_contract/src/main.nr b/noir-projects/noir-contracts/contracts/benchmarking_contract/src/main.nr index c01cf52efce6..0475eea921cc 100644 --- a/noir-projects/noir-contracts/contracts/benchmarking_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/benchmarking_contract/src/main.nr @@ -32,7 +32,7 @@ contract Benchmarking { let owner_notes = storage.notes.at(owner); let mut getter_options = NoteGetterOptions::new(); let notes = owner_notes.pop_notes(getter_options.set_limit(1).set_offset(index)); - let note = notes.get_unchecked(0); + let note = notes.get(0); increment(owner_notes, note.value, owner, outgoing_viewer); } diff --git a/noir-projects/noir-contracts/contracts/card_game_contract/src/cards.nr b/noir-projects/noir-contracts/contracts/card_game_contract/src/cards.nr index 871332ae6011..bb9dca96a6bf 100644 --- a/noir-projects/noir-contracts/contracts/card_game_contract/src/cards.nr +++ b/noir-projects/noir-contracts/contracts/card_game_contract/src/cards.nr @@ -121,6 +121,7 @@ impl Deck<&mut PrivateContext> { } pub fn remove_cards(&mut self, cards: [Card; N]) { + // TODO(benesjan): try to simplify this func let options = NoteGetterOptions::with_filter(filter_cards, cards); let notes = self.set.pop_notes(options); diff --git a/noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/main.nr b/noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/main.nr index 5b690c3eb100..d91d5455807d 100644 --- a/noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/main.nr @@ -149,7 +149,8 @@ contract InclusionProofs { let private_values = storage.private_values.at(owner); let mut options = NoteGetterOptions::new(); options = options.set_limit(1); - private_values.pop_notes(options); + let notes = private_values.pop_notes(options); + assert(notes.len() == 1, "note not popped"); } // docs:end:nullify_note diff --git a/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/main.nr b/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/main.nr index 0bd3a149e09d..c445e179405b 100644 --- a/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/main.nr @@ -173,7 +173,8 @@ contract TokenBlacklist { Option::none() ).set_limit(1); - storage.pending_shields.pop_notes(options); + let notes = storage.pending_shields.pop_notes(options); + assert(notes.len() == 1, "note not popped"); // Add the token note to user's balances set let caller = context.msg_sender(); diff --git a/noir-projects/noir-contracts/contracts/token_contract/src/main.nr b/noir-projects/noir-contracts/contracts/token_contract/src/main.nr index c6adfa18fdb3..489b9fce80e0 100644 --- a/noir-projects/noir-contracts/contracts/token_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/token_contract/src/main.nr @@ -304,7 +304,8 @@ contract Token { Option::none() ).set_limit(1); - storage.pending_shields.pop_notes(options); + let notes = storage.pending_shields.pop_notes(options); + assert(notes.len() == 1, "note not popped"); // Add the token note to user's balances set // Note: Using context.msg_sender() as a sender below makes this incompatible with escrows because we send diff --git a/noir-projects/noir-contracts/contracts/token_with_refunds_contract/src/main.nr b/noir-projects/noir-contracts/contracts/token_with_refunds_contract/src/main.nr index 0656fcafc037..f60a2cffea7e 100644 --- a/noir-projects/noir-contracts/contracts/token_with_refunds_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/token_with_refunds_contract/src/main.nr @@ -291,7 +291,8 @@ contract TokenWithRefunds { Option::none() ).set_limit(1); - storage.pending_shields.pop_notes(options); + let notes = storage.pending_shields.pop_notes(options); + assert(notes.len() == 1, "note not popped"); // Add the token note to user's balances set // Note: Using context.msg_sender() as a sender below makes this incompatible with escrows because we send diff --git a/yarn-project/end-to-end/src/e2e_blacklist_token_contract/minting.test.ts b/yarn-project/end-to-end/src/e2e_blacklist_token_contract/minting.test.ts index 37016cdbd20f..19dd05a9aade 100644 --- a/yarn-project/end-to-end/src/e2e_blacklist_token_contract/minting.test.ts +++ b/yarn-project/end-to-end/src/e2e_blacklist_token_contract/minting.test.ts @@ -111,7 +111,7 @@ describe('e2e_blacklist_token_contract mint', () => { 'The note has been destroyed.', ); await expect(asset.methods.redeem_shield(wallets[0].getAddress(), amount, secret).prove()).rejects.toThrow( - `Assertion failed: Attempted to read past end of BoundedVec`, + `Assertion failed: note not popped 'notes.len() == 1'`, ); }); diff --git a/yarn-project/end-to-end/src/e2e_token_contract/minting.test.ts b/yarn-project/end-to-end/src/e2e_token_contract/minting.test.ts index 83d3ea02ce61..208cfe2902c7 100644 --- a/yarn-project/end-to-end/src/e2e_token_contract/minting.test.ts +++ b/yarn-project/end-to-end/src/e2e_token_contract/minting.test.ts @@ -101,7 +101,7 @@ describe('e2e_token_contract minting', () => { 'The note has been destroyed.', ); await expect(asset.methods.redeem_shield(accounts[0].address, amount, secret).simulate()).rejects.toThrow( - `Assertion failed: Attempted to read past end of BoundedVec`, + `Assertion failed: note not popped 'notes.len() == 1'`, ); });