fix: fix msg_sender direct call exploit (#7404)

Fixes exploit as first written here #7190. Essentially, a user calling a
contract directly (bypassing the account contract) could impersonate any
chosen address. This PR adds a check in the private init kernel to
prevent setting the `msg_sender`. Commits are:

-
[7c08eab](7c08eab)
->
[35ed54b](35ed54b)
- shows exploit working in `token_contract` test - apologies for the
format fails (taken exactly from Lasse's PR #7190, slight change to
allow for direct call)
-
[4072ae3](4072ae3)
->
[b7f5987](b7f5987)
- shows fix to exploit and edits above test to expect a fail (=> a pass
on `token_contract` means exploit is fixed)
-
[85da5a9](85da5a9)
- removes test code from `token_contract`

l1-contracts/src/core/libraries/ConstantsGen.sol

@@ -12,8 +12,9 @@ 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;

l1-contracts/test/Inbox.t.sol

@@ -105,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)
     );
@@ -114,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)
     );

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)
     ]
 }
 

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;
@@ -34,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() == MAX_FIELD_VALUE);
         assert_current_call_valid_authwit(&mut context, user_address);
 
         let mut note = storage.subscriptions.at(user_address).get_note().note;

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
 };
@@ -121,6 +122,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(MAX_FIELD_VALUE), "Users cannot set msg_sender in first call"
+        );
     }
 
     // Confirm that the TxRequest (user's intent) matches the private call being executed.

noir-projects/noir-protocol-circuits/crates/private-kernel-lib/src/private_kernel_init.nr

@@ -72,7 +72,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 }
         }

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;

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();
 }

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;
 
 /**

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,
@@ -203,6 +203,11 @@ impl FixtureBuilder {
         *self
     }
 
+    pub fn is_first_call(&mut self) -> Self {
+        self.msg_sender = AztecAddress::from_field(MAX_FIELD_VALUE);
+        *self
+    }
+
     pub fn to_constant_data(self) -> CombinedConstantData {
         CombinedConstantData {
             historical_header: self.historical_header,

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);
+}

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<Tx> {
     return this.pxe.proveTx(txRequest, simulatePublic);
   }
-  simulateTx(txRequest: TxExecutionRequest, simulatePublic: boolean, msgSender: AztecAddress): Promise<SimulatedTx> {
+  simulateTx(txRequest: TxExecutionRequest, simulatePublic: boolean, msgSender?: AztecAddress): Promise<SimulatedTx> {
     return this.pxe.simulateTx(txRequest, simulatePublic, msgSender);
   }
   sendTx(tx: Tx): Promise<TxHash> {

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;

yarn-project/circuits.js/src/scripts/constants.in.ts

@@ -263,7 +263,6 @@ library Constants {
   // Prime field modulus
   uint256 internal constant P =
     21888242871839275222246405745257275088548364400416034343698204186575808495617;
-  uint256 internal constant MAX_FIELD_VALUE = P - 1;
 
 ${processConstantsSolidity(constants)}
 }\n`;

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());
+    });
+  });
 });

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;

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;
 

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);

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([
+      Fr.MAX_FIELD_VALUE,
+      functionSelector.toField(),
+      entrypointPackedArgs.hash,
+    ]);
     const outerHash = computeAuthWitMessageHash(
       { consumer: this.dappEntrypointAddress, innerHash },
       { chainId: new Fr(this.chainId), version: new Fr(this.version) },