From d251703213c42c427ed3e0f8ff1098edf3b6a2e3 Mon Sep 17 00:00:00 2001 From: Lasse Herskind <16536249+LHerskind@users.noreply.github.com> Date: Wed, 29 Nov 2023 09:14:24 +0000 Subject: [PATCH] fix: check version, chainid and sender for cross-chain l1 to l2 msgs (#3457) Fixes #3455 by also checking the portal address, version and chainid. Needed updates in the tests to pass the additional tests. Existing tests generally seem to be very heavily focused on happy paths, we need to do something there. Also ran `nargo fmt` --- .../src/client/private_execution.test.ts | 8 +++++- .../acir-simulator/src/public/index.test.ts | 11 ++++++-- yarn-project/aztec-nr/aztec/src/context.nr | 4 +-- yarn-project/aztec-nr/aztec/src/messaging.nr | 20 ++++++++++++- .../contracts/card_game_contract/src/main.nr | 2 +- .../contracts/lending_contract/src/main.nr | 28 +++++++++++++++---- .../contracts/test_contract/src/interface.nr | 9 ++---- .../token_bridge_contract/src/main.nr | 2 +- .../src/contracts/token_contract/src/main.nr | 1 - 9 files changed, 63 insertions(+), 22 deletions(-) diff --git a/yarn-project/acir-simulator/src/client/private_execution.test.ts b/yarn-project/acir-simulator/src/client/private_execution.test.ts index fd0a52fac57..b96d69dc338 100644 --- a/yarn-project/acir-simulator/src/client/private_execution.test.ts +++ b/yarn-project/acir-simulator/src/client/private_execution.test.ts @@ -467,7 +467,13 @@ describe('Private Execution test suite', () => { messageKey, secretForL1ToL2MessageConsumption, ]; - const result = await runSimulator({ contractAddress, artifact, args }); + const result = await runSimulator({ + contractAddress, + artifact, + args, + portalContractAddress: preimage.sender.sender, + txContext: { version: new Fr(1n), chainId: new Fr(1n) }, + }); // Check a nullifier has been inserted const newNullifiers = result.callStackItem.publicInputs.newNullifiers.filter(field => !field.equals(Fr.ZERO)); diff --git a/yarn-project/acir-simulator/src/public/index.test.ts b/yarn-project/acir-simulator/src/public/index.test.ts index 74abeab2e0b..4d985f476af 100644 --- a/yarn-project/acir-simulator/src/public/index.test.ts +++ b/yarn-project/acir-simulator/src/public/index.test.ts @@ -386,7 +386,7 @@ describe('ACIR public execution simulator', () => { const callContext = CallContext.from({ msgSender: AztecAddress.random(), storageContractAddress: contractAddress, - portalContractAddress: EthAddress.random(), + portalContractAddress: preimage.sender.sender, functionSelector: FunctionSelector.empty(), isContractDeployment: false, isDelegateCall: false, @@ -406,7 +406,14 @@ describe('ACIR public execution simulator', () => { }); const execution: PublicExecution = { contractAddress, functionData, args, callContext }; - const result = await executor.simulate(execution, GlobalVariables.empty()); + + const gv = new GlobalVariables( + new Fr(preimage.sender.chainId), + new Fr(preimage.recipient.version), + Fr.ZERO, + Fr.ZERO, + ); + const result = await executor.simulate(execution, gv); expect(result.newNullifiers.length).toEqual(1); }); diff --git a/yarn-project/aztec-nr/aztec/src/context.nr b/yarn-project/aztec-nr/aztec/src/context.nr index a2f34867c6c..e10b6233ec6 100644 --- a/yarn-project/aztec-nr/aztec/src/context.nr +++ b/yarn-project/aztec-nr/aztec/src/context.nr @@ -202,7 +202,7 @@ impl PrivateContext { ) // docs:end:context_consume_l1_to_l2_message { - let nullifier = process_l1_to_l2_message(self.block_data.l1_to_l2_messages_tree_root, self.this_address(), msg_key, content, secret); + let nullifier = process_l1_to_l2_message(self.block_data.l1_to_l2_messages_tree_root, self.this_address(), self.this_portal_address(), self.chain_id(), self.version(), msg_key, content, secret); // Push nullifier (and the "commitment" corresponding to this can be "empty") self.push_new_nullifier(nullifier, EMPTY_NULLIFIED_COMMITMENT) @@ -543,7 +543,7 @@ impl PublicContext { // Note this returns self to get around an issue where mutable structs do not maintain mutations unless reassigned pub fn consume_l1_to_l2_message(&mut self, msg_key: Field, content: Field, secret: Field) { let this = (*self).this_address(); - let nullifier = process_l1_to_l2_message(self.block_data.l1_to_l2_messages_tree_root, this, msg_key, content, secret); + let nullifier = process_l1_to_l2_message(self.block_data.l1_to_l2_messages_tree_root, this, self.this_portal_address(), self.chain_id(), self.version(), msg_key, content, secret); // Push nullifier (and the "commitment" corresponding to this can be "empty") self.push_new_nullifier(nullifier, EMPTY_NULLIFIED_COMMITMENT) diff --git a/yarn-project/aztec-nr/aztec/src/messaging.nr b/yarn-project/aztec-nr/aztec/src/messaging.nr index 185733de9a8..e5bca11b582 100644 --- a/yarn-project/aztec-nr/aztec/src/messaging.nr +++ b/yarn-project/aztec-nr/aztec/src/messaging.nr @@ -7,7 +7,16 @@ use crate::abi::PublicContextInputs; use crate::oracle::get_l1_to_l2_message::get_l1_to_l2_message_call; // Returns the nullifier for the message -pub fn process_l1_to_l2_message(l1_to_l2_root: Field, storage_contract_address: Field, msg_key: Field, content: Field, secret: Field) -> Field { +pub fn process_l1_to_l2_message( + l1_to_l2_root: Field, + storage_contract_address: Field, + portal_contract_address: Field, + chain_id: Field, + version: Field, + msg_key: Field, + content: Field, + secret: Field +) -> Field { let returned_message = get_l1_to_l2_message_call(msg_key); let l1_to_l2_message_data = make_l1_to_l2_message_getter_data(returned_message, 0, secret); @@ -17,6 +26,15 @@ pub fn process_l1_to_l2_message(l1_to_l2_root: Field, storage_contract_address: // Validate this is the target contract assert(l1_to_l2_message_data.message.recipient == storage_contract_address); + // Validate the sender is the portal contract + assert(l1_to_l2_message_data.message.sender == portal_contract_address); + + // Validate the chain id is correct + assert(l1_to_l2_message_data.message.chainId == chain_id); + + // Validate the version is correct + assert(l1_to_l2_message_data.message.version == version); + // Validate the message hash is correct assert(l1_to_l2_message_data.message.content == content); diff --git a/yarn-project/noir-contracts/src/contracts/card_game_contract/src/main.nr b/yarn-project/noir-contracts/src/contracts/card_game_contract/src/main.nr index 9136189202b..2792859e9b4 100644 --- a/yarn-project/noir-contracts/src/contracts/card_game_contract/src/main.nr +++ b/yarn-project/noir-contracts/src/contracts/card_game_contract/src/main.nr @@ -163,7 +163,7 @@ contract CardGame { context.call_public_function(context.this_address(), selector, [game as Field, player, card.to_field()]); - // docs:end:call_public_function + // docs:end:call_public_function } #[aztec(public)] diff --git a/yarn-project/noir-contracts/src/contracts/lending_contract/src/main.nr b/yarn-project/noir-contracts/src/contracts/lending_contract/src/main.nr index 8a24bd2e2b9..f34b9119cb2 100644 --- a/yarn-project/noir-contracts/src/contracts/lending_contract/src/main.nr +++ b/yarn-project/noir-contracts/src/contracts/lending_contract/src/main.nr @@ -158,9 +158,15 @@ contract Lending { #[aztec(public)] fn deposit_public(amount: Field, nonce: Field, on_behalf_of: Field, collateral_asset: Field) { - Token::at(collateral_asset).transfer_public(context, context.msg_sender(), context.this_address(), amount, nonce); + Token::at(collateral_asset).transfer_public(context, + context.msg_sender(), + context.this_address(), + amount, + nonce); let selector = compute_selector("_deposit(Field,Field,Field)"); - context.call_public_function(context.this_address(), selector, [on_behalf_of, amount, collateral_asset]); + context.call_public_function(context.this_address(), + selector, + [on_behalf_of, amount, collateral_asset]); } #[aztec(public)] @@ -185,7 +191,9 @@ contract Lending { #[aztec(public)] fn withdraw_public(to: Field, amount: Field) { let selector = compute_selector("_withdraw(Field,Field,Field)"); - context.call_public_function(context.this_address(), selector, [context.msg_sender(), to, amount]); + context.call_public_function(context.this_address(), + selector, + [context.msg_sender(), to, amount]); } #[aztec(public)] @@ -201,7 +209,11 @@ contract Lending { // debt_covered will revert if decrease would leave insufficient collateral to cover debt. // or trying to remove more collateral than available - let debt_covered = covered_by_collateral(price, asset.loan_to_value, collateral as u120, 0, amount as u120); + let debt_covered = covered_by_collateral(price, + asset.loan_to_value, + collateral as u120, + 0, + amount as u120); let debt_returns = debt_updates(asset.interest_accumulator, static_debt as u120, 0, 0); assert(debt_returns.debt_value < debt_covered); @@ -223,7 +235,9 @@ contract Lending { #[aztec(public)] fn borrow_public(to: Field, amount: Field) { let selector = compute_selector("_borrow(Field,Field,Field)"); - context.call_public_function(context.this_address(), selector, [context.msg_sender(), to, amount]); + context.call_public_function(context.this_address(), + selector, + [context.msg_sender(), to, amount]); } #[aztec(public)] @@ -252,7 +266,9 @@ contract Lending { let on_behalf_of = compute_identifier(secret, on_behalf_of, context.msg_sender()); let _res = Token::at(stable_coin).burn(&mut context, from, amount, nonce); let selector = compute_selector("_repay(Field,Field,Field)"); - context.call_public_function(context.this_address(), selector, [on_behalf_of, amount, stable_coin]); + context.call_public_function(context.this_address(), + selector, + [on_behalf_of, amount, stable_coin]); } #[aztec(public)] diff --git a/yarn-project/noir-contracts/src/contracts/test_contract/src/interface.nr b/yarn-project/noir-contracts/src/contracts/test_contract/src/interface.nr index 1bb62e9f3f6..65cf52e96b5 100644 --- a/yarn-project/noir-contracts/src/contracts/test_contract/src/interface.nr +++ b/yarn-project/noir-contracts/src/contracts/test_contract/src/interface.nr @@ -1,5 +1,5 @@ /* Autogenerated file, do not edit! */ - + use dep::std; use dep::aztec::context::{ PrivateContext, PublicContext }; use dep::aztec::constants_gen::RETURN_VALUES_LENGTH; @@ -26,7 +26,6 @@ struct ManyNotesADeepStructTestCodeGenStruct { secret_hash: Field, } - // Interface for calling Test functions from a private context struct TestPrivateContextInterface { address: Field, @@ -242,9 +241,6 @@ impl TestPrivateContextInterface { } } - - - // Interface for calling Test functions from a public context struct TestPublicContextInterface { @@ -330,5 +326,4 @@ impl TestPublicContextInterface { } } - - + diff --git a/yarn-project/noir-contracts/src/contracts/token_bridge_contract/src/main.nr b/yarn-project/noir-contracts/src/contracts/token_bridge_contract/src/main.nr index fe2e708ff78..42332685bc6 100644 --- a/yarn-project/noir-contracts/src/contracts/token_bridge_contract/src/main.nr +++ b/yarn-project/noir-contracts/src/contracts/token_bridge_contract/src/main.nr @@ -175,5 +175,5 @@ contract TokenBridge { unconstrained fn compute_note_hash_and_nullifier(contract_address: Field, nonce: Field, storage_slot: Field, serialized_note: [Field; 0]) -> [Field; 4] { [0, 0, 0, 0] } -// docs:end:compute_note_hash_and_nullifier_placeholder + // docs:end:compute_note_hash_and_nullifier_placeholder } diff --git a/yarn-project/noir-contracts/src/contracts/token_contract/src/main.nr b/yarn-project/noir-contracts/src/contracts/token_contract/src/main.nr index 2a52ab0607b..c530f0bf66d 100644 --- a/yarn-project/noir-contracts/src/contracts/token_contract/src/main.nr +++ b/yarn-project/noir-contracts/src/contracts/token_contract/src/main.nr @@ -216,7 +216,6 @@ contract Token { let to_balance = storage.public_balances.at(to.address).read().add(amount); storage.public_balances.at(to.address).write(to_balance); - } // docs:end:transfer_public