Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: privacy leak in private refund #7358

Merged
merged 2 commits into from
Jul 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
mod lib;

contract PrivateFPC {
use dep::aztec::protocol_types::{abis::log_hash::LogHash, address::AztecAddress};
use dep::aztec::protocol_types::{abis::log_hash::LogHash, address::AztecAddress, hash::poseidon2_hash};
use dep::aztec::state_vars::SharedImmutable;
use dep::private_token::PrivateToken;
use crate::lib::emit_randomness_as_unencrypted_log;
Expand All @@ -20,19 +20,23 @@ contract PrivateFPC {
}

#[aztec(private)]
fn fund_transaction_privately(amount: Field, asset: AztecAddress, randomness: Field) {
fn fund_transaction_privately(amount: Field, asset: AztecAddress, user_randomness: Field) {
assert(asset == storage.other_asset.read_private());
// convince the FPC we are not cheating
context.push_nullifier(randomness, 0);
context.push_nullifier(user_randomness, 0);

// allow the FPC to reconstruct their fee note
emit_randomness_as_unencrypted_log(&mut context, randomness);
// We use different randomness for fee payer to prevent a potential privay leak (see impl
// of PrivatelyRefundable for TokenNote for details).
let fee_payer_randomness = poseidon2_hash([user_randomness]);
// We emit fee payer randomness to ensure FPC admin can reconstruct their fee note
emit_randomness_as_unencrypted_log(&mut context, fee_payer_randomness);

PrivateToken::at(asset).setup_refund(
storage.admin_npk_m_hash.read_private(),
context.msg_sender(),
amount,
randomness
user_randomness,
fee_payer_randomness
).call(&mut context);
context.set_as_fee_payer();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,8 @@ contract PrivateToken {
fee_payer_npk_m_hash: Field, // NpkMHash of the entity which will receive the fee note.
user: AztecAddress, // A user for which we are setting up the fee refund.
funded_amount: Field, // The amount the user funded the fee payer with (represents fee limit).
randomness: Field // A randomness to mix in with the generated notes.
user_randomness: Field, // A randomness to mix in with the generated refund note for the sponsored user.
fee_payer_randomness: Field // A randomness to mix in with the generated fee note for the fee payer.
) {
// 1. This function is called by fee paying contract (fee_payer) when setting up a refund so we need to support
// the authwit flow here and check that the user really permitted fee_payer to set up a refund on their behalf.
Expand All @@ -183,7 +184,8 @@ contract PrivateToken {
fee_payer_npk_m_hash,
user_npk_m_hash,
funded_amount,
randomness
user_randomness,
fee_payer_randomness
);

// 5. Set the public teardown function to `complete_refund(...)`. Public teardown is the only time when a public
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,17 @@ unconstrained fn setup_refund_success() {
let (env, token_contract_address, owner, recipient, mint_amount) = utils::setup_and_mint(true);

let funded_amount = 1_000;
let refund_nonce = 42;
let user_randomness = 42;
let fee_payer_randomness = 123;
let mut context = env.private();
let recipient_npk_m_hash = context.get_header().get_npk_m_hash(&mut context, recipient);

let setup_refund_from_call_interface = PrivateToken::at(token_contract_address).setup_refund(
recipient_npk_m_hash, // fee payer
owner, // sponsored user
funded_amount,
refund_nonce
user_randomness,
fee_payer_randomness
);

authwit_cheatcodes::add_private_authwit_from_call_interface(owner, recipient, setup_refund_from_call_interface);
Expand All @@ -70,7 +72,7 @@ unconstrained fn setup_refund_success() {
&mut TokenNote {
amount: U128::from_integer(funded_amount - 1),
npk_m_hash: owner_npk_m_hash,
randomness: refund_nonce,
randomness: user_randomness,
header: NoteHeader::empty()
},
PrivateToken::storage().balances.slot,
Expand All @@ -80,7 +82,7 @@ unconstrained fn setup_refund_success() {
&mut TokenNote {
amount: U128::from_integer(1),
npk_m_hash: recipient_npk_m_hash,
randomness: refund_nonce,
randomness: fee_payer_randomness,
header: NoteHeader::empty()
},
PrivateToken::storage().balances.slot,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ trait PrivatelyRefundable {
fee_payer_npk_m_hash: Field,
user_npk_m_hash: Field,
funded_amount: Field,
randomness: Field
user_randomness: Field,
fee_payer_randomness: Field
) -> (EmbeddedCurvePoint, EmbeddedCurvePoint);

fn complete_refund(
Expand Down Expand Up @@ -155,8 +156,8 @@ impl OwnedNote for TokenNote {
* deduce what n is. This is the discrete log problem.
*
* However we can still perform addition/subtraction on points! That is why we generate those two points, which are:
* incomplete_fee_payer_point := (fee_payer_npk + randomness) * G
* incomplete_user_point := (user_npk + funded_amount + randomness) * G
* incomplete_fee_payer_point := (fee_payer_npk + fee_payer_randomness) * G
* incomplete_user_point := (user_npk + funded_amount + user_randomness) * G
*
* where `funded_amount` is the total amount in tokens that the sponsored user initially supplied, from which the transaction fee will be subtracted.
*
Expand All @@ -167,25 +168,37 @@ impl OwnedNote for TokenNote {
* Then we arrive at the final points via addition/subtraction of that transaction fee point:
*
* fee_payer_point := incomplete_fee_payer_point + fee_point
* = (fee_payer_npk + randomness) * G + transaction_fee * G
* = (fee_payer_npk + randomness + transaction_fee) * G
* = (fee_payer_npk + fee_payer_randomness) * G + transaction_fee * G
* = (fee_payer_npk + fee_payer_randomness + transaction_fee) * G
*
* user_point := incomplete_user_point - fee_point
* = (user_npk + funded_amount + randomness) * G - transaction_fee * G
* = (user_npk + randomness + (funded_amount - transaction_fee)) * G
* = (user_npk + funded_amount + user_randomness) * G - transaction_fee * G
* = (user_npk + user_randomness + (funded_amount - transaction_fee)) * G
*
* When we return the x-coordinate of those points, it identically matches the note_content_hash of (and therefore *is*) notes like:
* {
* amount: (funded_amount - transaction_fee),
* npk_m_hash: user_npk,
* randomness: randomness
* randomness: user_randomness
* }
*
* Why do we need different randomness for the user and the fee payer notes?
* --> This is because if the randomness values were the same we could fingerprint the user by doing the following:
* 1) randomness_influence = incomplete_fee_payer_point - G * fee_payer_npk =
* = (fee_payer_npk + randomness) * G - G * fee_payer_npk = randomness * G
* 2) user_fingerprint = incomplete_user_point - G * funded_amount - randomness_influence =
* = (user_npk + funded_amount + randomness) * G - funded_amount * G - randomness * G =
* = user_npk * G
* 3) Then the second time the user would use this fee paying contract we would recover the same fingerprint and
* link that the 2 transactions were made by the same user. Given that it's expected that only a limited set
* of fee paying contracts will be used and they will be known searching for fingerprints by trying different
* fee payer npk values of these known contracts is a feasible attack.
*/
impl PrivatelyRefundable for TokenNote {
fn generate_refund_points(fee_payer_npk_m_hash: Field, user_npk_m_hash: Field, funded_amount: Field, randomness: Field) -> (EmbeddedCurvePoint, EmbeddedCurvePoint) {
fn generate_refund_points(fee_payer_npk_m_hash: Field, user_npk_m_hash: Field, funded_amount: Field, user_randomness: Field, fee_payer_randomness: Field) -> (EmbeddedCurvePoint, EmbeddedCurvePoint) {
// 1. To be able to multiply generators with randomness and npk_m_hash using barretneberg's (BB) blackbox function we
// first need to convert the fields to high and low limbs.
let (randomness_lo, randomness_hi) = decompose(randomness);
let (fee_payer_randomness_lo, fee_payer_randomness_hi) = decompose(fee_payer_randomness);
let (fee_payer_npk_m_hash_lo, fee_payer_npk_m_hash_hi) = decompose(fee_payer_npk_m_hash);

// 2. Now that we have correct representationsn of fee payer and randomness we can compute `G ^ (fee_payer_npk + randomness)`
Expand All @@ -196,12 +209,13 @@ impl PrivatelyRefundable for TokenNote {
hi: fee_payer_npk_m_hash_hi
},
EmbeddedCurveScalar {
lo: randomness_lo,
hi: randomness_hi
lo: fee_payer_randomness_lo,
hi: fee_payer_randomness_hi
}]
);

// 3. We do the necessary conversion for values relevant for the sponsored user point.
let (user_randomness_lo, user_randomness_hi) = decompose(user_randomness);
// TODO(#7324): representing user with their npk_m_hash here does not work with key rotation
let (user_lo, user_hi) = decompose(user_npk_m_hash);
let (funded_amount_lo, funded_amount_hi) = decompose(funded_amount);
Expand All @@ -218,8 +232,8 @@ impl PrivatelyRefundable for TokenNote {
hi: funded_amount_hi
},
EmbeddedCurveScalar {
lo: randomness_lo,
hi: randomness_hi
lo: user_randomness_lo,
hi: user_randomness_hi
}]
);

Expand Down
33 changes: 24 additions & 9 deletions yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from '@aztec/aztec.js';
import { Fr, type GasSettings } from '@aztec/circuits.js';
import { FunctionSelector, FunctionType } from '@aztec/foundation/abi';
import { poseidon2Hash } from '@aztec/foundation/crypto';
import { type PrivateFPCContract, PrivateTokenContract } from '@aztec/noir-contracts.js';

import { expectMapping } from '../fixtures/utils.js';
Expand Down Expand Up @@ -53,7 +54,8 @@ describe('e2e_fees/private_refunds', () => {
// TODO(#7324): The values in complete address are currently not updated after the keys are rotated so this does
// not work with key rotation as the key might be the old one and then we would fetch a new one in the contract.
const bobNpkMHash = t.bobWallet.getCompleteAddress().publicKeys.masterNullifierPublicKey.hash();
const randomness = Fr.random();
const aliceRandomness = Fr.random(); // Called user_randomness in contracts
const bobRandomness = poseidon2Hash([aliceRandomness]); // Called fee_payer_randomness in contracts

// 2. We call arbitrary `private_get_name(...)` function to check that the fee refund flow works.
const tx = await privateToken.methods
Expand All @@ -65,7 +67,8 @@ describe('e2e_fees/private_refunds', () => {
privateToken.address,
privateFPC.address,
aliceWallet,
randomness,
aliceRandomness,
bobRandomness,
bobNpkMHash, // We use Bob's npk_m_hash in the notes that contain the transaction fee.
),
},
Expand All @@ -81,7 +84,7 @@ describe('e2e_fees/private_refunds', () => {
// TODO(#7324): The values in complete address are currently not updated after the keys are rotated so this does
// not work with key rotation as the key might be the old one and then we would fetch a new one in the contract.
const aliceNpkMHash = t.aliceWallet.getCompleteAddress().publicKeys.masterNullifierPublicKey.hash();
const aliceRefundNote = new Note([refundNoteValue, aliceNpkMHash, randomness]);
const aliceRefundNote = new Note([refundNoteValue, aliceNpkMHash, aliceRandomness]);

// 4. If the refund flow worked it should have added emitted a note hash of the note we constructed above and we
// should be able to add the note to our PXE. Just calling `pxe.addNote(...)` is enough of a check that the note
Expand All @@ -102,7 +105,7 @@ describe('e2e_fees/private_refunds', () => {
// npk_m_hash (set in the paymentMethod above) and the randomness.
// Note that FPC emits randomness as unencrypted log and the tx fee is publicly know so Bob is able to reconstruct
// his note just from on-chain data.
const bobFeeNote = new Note([new Fr(tx.transactionFee!), bobNpkMHash, randomness]);
const bobFeeNote = new Note([new Fr(tx.transactionFee!), bobNpkMHash, bobRandomness]);

// 6. Once again we add the note to PXE which computes the note hash and checks that it is in the note hash tree.
await t.bobWallet.addNote(
Expand Down Expand Up @@ -144,10 +147,16 @@ class PrivateRefundPaymentMethod implements FeePaymentMethod {
private wallet: Wallet,

/**
* A randomness to mix in with the generated notes.
* A randomness to mix in with the generated refund note for the sponsored user.
* Use this to reconstruct note preimages for the PXE.
*/
private randomness: Fr,
private userRandomness: Fr,

/**
* A randomness to mix in with the generated fee note for the fee payer.
* Use this to reconstruct note preimages for the PXE.
*/
private feePayerRandomness: Fr,

/**
* The hash of the master nullifier public key that the FPC sends notes it receives to.
Expand Down Expand Up @@ -179,8 +188,14 @@ class PrivateRefundPaymentMethod implements FeePaymentMethod {
caller: this.paymentContract,
action: {
name: 'setup_refund',
args: [this.feeRecipientNpkMHash, this.wallet.getCompleteAddress().address, maxFee, this.randomness],
selector: FunctionSelector.fromSignature('setup_refund(Field,(Field),Field,Field)'),
args: [
this.feeRecipientNpkMHash,
this.wallet.getCompleteAddress().address,
maxFee,
this.userRandomness,
this.feePayerRandomness,
],
selector: FunctionSelector.fromSignature('setup_refund(Field,(Field),Field,Field,Field)'),
type: FunctionType.PRIVATE,
isStatic: false,
to: this.asset,
Expand All @@ -195,7 +210,7 @@ class PrivateRefundPaymentMethod implements FeePaymentMethod {
selector: FunctionSelector.fromSignature('fund_transaction_privately(Field,(Field),Field)'),
type: FunctionType.PRIVATE,
isStatic: false,
args: [maxFee, this.asset, this.randomness],
args: [maxFee, this.asset, this.userRandomness],
returnTypes: [],
},
];
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/noir-contracts.js/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,4 @@
"engines": {
"node": ">=18"
}
}
}
Loading