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: checking funded amount is enough #7648

Merged
merged 11 commits into from
Aug 1, 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
Expand Up @@ -457,7 +457,6 @@ contract TokenWithRefunds {
let (fee_payer_point, user_point) = TokenNote::generate_refund_points(
fee_payer_npk_m_hash,
user_npk_m_hash,
funded_amount,
user_randomness,
fee_payer_randomness
);
Expand All @@ -473,20 +472,25 @@ contract TokenWithRefunds {
// function has access to the final transaction fee, which is needed to compute the actual refund amount.
context.set_public_teardown_function(
context.this_address(),
FunctionSelector::from_signature("complete_refund((Field,Field,bool),(Field,Field,bool))"),
FunctionSelector::from_signature("complete_refund((Field,Field,bool),(Field,Field,bool),Field)"),
[
slotted_fee_payer_point.x, slotted_fee_payer_point.y, slotted_fee_payer_point.is_infinite as Field, slotted_user_point.x, slotted_user_point.y, slotted_user_point.is_infinite as Field
slotted_fee_payer_point.x, slotted_fee_payer_point.y, slotted_fee_payer_point.is_infinite as Field, slotted_user_point.x, slotted_user_point.y, slotted_user_point.is_infinite as Field, funded_amount
]
);
}

#[aztec(public)]
#[aztec(internal)]
fn complete_refund(fee_payer_point: Point, user_point: Point) {
// 1. We get the final note hiding points by calling a `complete_refund` function on the note.
// We use 1:1 exchange rate between fee juice and token. So using `tx_fee` is enough
let tx_fee = context.transaction_fee();
let (fee_payer_note_hash, user_note_hash) = TokenNote::complete_refund(fee_payer_point, user_point, tx_fee);
fn complete_refund(fee_payer_point: Point, user_point: Point, funded_amount: Field) {
// 1. We get the final note hashes by calling a `complete_refund` function on the note.
// We use 1:1 exchange rate between fee juice and token so just passing transaction fee and funded amount
// to `complete_refund(...)` function is enough.
let (fee_payer_note_hash, user_note_hash) = TokenNote::complete_refund(
fee_payer_point,
user_point,
funded_amount,
context.transaction_fee()
);

// 2. At last we emit the note hashes.
context.push_note_hash(fee_payer_note_hash);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ unconstrained fn setup_refund_success() {

// When the refund was set up, we would've spent the note worth mint_amount, and inserted a note worth
//`mint_amount - funded_amount`. When completing the refund, we would've constructed a hash corresponding to a note
// worth `funded_amount - transaction_fee`. We "know" the transaction fee was 1 (it is hardcoded in TXE oracle)
// but we need to notify TXE of the note (preimage).
// worth `funded_amount - transaction_fee`. We "know" the transaction fee was 1 (it is hardcoded in
// `executePublicFunction` TXE oracle) but we need to notify TXE of the note (preimage).
env.store_note_in_cache(
&mut TokenNote {
amount: U128::from_integer(funded_amount - 1),
Expand All @@ -68,3 +68,36 @@ unconstrained fn setup_refund_success() {
utils::check_private_balance(token_contract_address, fee_payer, 1)
}

// TODO(#7694): Ideally we would check the error message here but it's currently not possible because TXE does not
// support checking messages of errors thrown in a public teardown function. Once this is supported, check the message
// here and delete the e2e test checking it.
// #[test(should_fail_with = "tx fee is higher than funded amount")]
#[test(should_fail)]
unconstrained fn setup_refund_insufficient_funded_amount() {
let (env, token_contract_address, owner, recipient, mint_amount) = utils::setup_and_mint(true);

// Renaming owner and recipient to match naming in TokenWithRefunds
let user = owner;
let fee_payer = recipient;

// We set funded amount to 0 to make the transaction fee higher than the funded amount
let funded_amount = 0;
let user_randomness = 42;
let fee_payer_randomness = 123;
let mut context = env.private();

let setup_refund_from_call_interface = TokenWithRefunds::at(token_contract_address).setup_refund(
fee_payer,
user,
funded_amount,
user_randomness,
fee_payer_randomness
);

authwit_cheatcodes::add_private_authwit_from_call_interface(user, fee_payer, setup_refund_from_call_interface);

env.impersonate(fee_payer);

// The following should fail with "tx fee is higher than funded amount" because funded amount is 0
env.call_private_void(setup_refund_from_call_interface);
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@ trait PrivatelyRefundable {
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
) -> (Point, Point);

fn complete_refund(
incomplete_fee_payer_point: Point,
incomplete_user_point: Point,
funded_amount: Field,
transaction_fee: Field
) -> (Field, Field);
}
Expand Down Expand Up @@ -157,25 +157,26 @@ impl OwnedNote for TokenNote {
*
* However we can still perform addition/subtraction on points! That is why we generate those two points, which are:
* incomplete_fee_payer_point := G_npk * fee_payer_npk + G_rnd * fee_payer_randomness
* incomplete_user_point := G_npk * user_npk + G_amt * funded_amount + G_rnd * user_randomness
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine that x + G_amt * y is equal to x if y == 0? So removing this term is the same as saying amount = 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow what you are trying to say here but I've noticed that the related comments needed to be updated as well and I did that in c0570e6

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that the comment now doesn't list amount as being included in the computation, I was thinking how this is equivalent to amount actually being there, but being equal to 0.

This is relevant for things like #7585, since we'd want to avoid having to track whether the amount is inside the point or not.

*
* where `funded_amount` is the total amount in tokens that the sponsored user initially supplied, from which
* the transaction fee will be subtracted.
* incomplete_user_point := G_npk * user_npk + G_rnd * user_randomness
*
* So we pass those points into the teardown function (here) and compute a third point corresponding to the transaction
* fee as just:
*
* fee_point := G_amt * transaction_fee
* refund_point := G_amt * (funded_amount - transaction_fee)
*
* Then we arrive at the final points via addition/subtraction of that transaction fee point:
* where `funded_amount` is the total amount in tokens that the sponsored user initially supplied and the transaction
* fee is the final transaction fee whose value is made available in the public teardown function.
*
* Then we arrive at the final points via addition of the fee and refund points:
*
* fee_payer_point := incomplete_fee_payer_point + fee_point =
* = (G_npk * fee_payer_npk + G_rnd * fee_payer_randomness) + G_amt * transaction_fee =
* = G_amt * transaction_fee + G_npk * fee_payer_npk + G_rnd * fee_payer_randomness
*
* user_point := incomplete_user_point - fee_point =
* = (G_amt * funded_amount + G_npk * user_npk + G_rnd + user_randomness) - G_amt * transaction_fee =
* = G_amt * (funded_amount - transaction_fee) + G_npk * user_npk + G_rnd + user_randomness
* user_point := incomplete_user_point + refund_point =
* = (G_npk * user_npk + G_rnd + user_randomness) + G_amt * (funded_amount - transaction_fee) =
* = G_amt * (funded_amount - transaction_fee) + G_npk * user_npk + G_rnd * user_randomness
*
* The point above matches the note_hiding_point of (and therefore *is*) notes like:
* {
Expand All @@ -189,17 +190,16 @@ impl OwnedNote for TokenNote {
* 1) randomness_influence = incomplete_fee_payer_point - G_npk * fee_payer_npk =
* = (G_npk * fee_payer_npk + G_rnd * randomness) - G_npk * fee_payer_npk =
* = G_rnd * randomness
* 2) user_fingerprint = incomplete_user_point - G_amt * funded_amount - randomness_influence =
* = (G_npk * user_npk + G_amt * funded_amount + G_rnd * randomness) - G_amt * funded_amount
* - G_rnd * randomness =
* 2) user_fingerprint = incomplete_user_point - randomness_influence =
* = (G_npk * user_npk + G_rnd * randomness) - G_rnd * randomness =
* = G_npk * user_npk
* 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, user_randomness: Field, fee_payer_randomness: Field) -> (Point, Point) {
fn generate_refund_points(fee_payer_npk_m_hash: Field, user_npk_m_hash: Field, user_randomness: Field, fee_payer_randomness: Field) -> (Point, Point) {
// 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.
// We use the unsafe version because the multi_scalar_mul will constrain the scalars.
Expand All @@ -215,44 +215,44 @@ impl PrivatelyRefundable for TokenNote {

// 3. We do the necessary conversion for values relevant for the sponsored user point.
// We use the unsafe version because the multi_scalar_mul will constrain the scalars.
let funded_amount_scalar = from_field_unsafe(funded_amount);
let user_npk_m_hash_scalar = from_field_unsafe(user_npk_m_hash);
let user_randomness_scalar = from_field_unsafe(user_randomness);

// 4. We compute `G_amt * funded_amount + G_npk * user_npk_m_hash + G_rnd * randomness`.
// 4. We compute `G_npk * user_npk_m_hash + G_rnd * randomness`.
let incomplete_user_point = multi_scalar_mul(
[G_amt, G_npk, G_rnd],
[funded_amount_scalar, user_npk_m_hash_scalar, user_randomness_scalar]
[G_npk, G_rnd],
[user_npk_m_hash_scalar, user_randomness_scalar]
);

// 5. At last we return the points.
(incomplete_fee_payer_point, incomplete_user_point)
}

fn complete_refund(incomplete_fee_payer_point: Point, incomplete_user_point: Point, transaction_fee: Field) -> (Field, Field) {
// 1. We convert the transaction fee to high and low limbs to be able to use BB API.
fn complete_refund(incomplete_fee_payer_point: Point, incomplete_user_point: Point, funded_amount: Field, transaction_fee: Field) -> (Field, Field) {
// 1. We check that user funded the fee payer contract with at least the transaction fee.
assert(!funded_amount.lt(transaction_fee), "tx fee is higher than funded amount"); // funded_amout >= transaction_fee

// 2. We convert the transaction fee and refund amount to high and low limbs to be able to use BB API.
// We use the unsafe version because the multi_scalar_mul will constrain the scalars.
let transaction_fee_scalar = from_field_unsafe(transaction_fee);
let refund_scalar = from_field_unsafe(funded_amount - transaction_fee);
benesjan marked this conversation as resolved.
Show resolved Hide resolved

// 2. We compute the fee point as `G_amt * transaction_fee`
let fee_point = multi_scalar_mul(
[G_amt],
[transaction_fee_scalar]
);
// 3. We compute the fee point as `G_amt * transaction_fee`
let fee_point = multi_scalar_mul([G_amt], [transaction_fee_scalar]);

// 3. Now we leverage homomorphism to privately add the fee to fee payer point and subtract it from
// the sponsored user point in public.
let fee_payer_point = incomplete_fee_payer_point + fee_point;
let user_point = incomplete_user_point - fee_point;
// 4. We compute the refund point as `G_amt * refund`
let refund_point = multi_scalar_mul([G_amt], [refund_scalar]);

assert_eq(user_point.is_infinite, false);
nventuro marked this conversation as resolved.
Show resolved Hide resolved
// 5. Now we leverage homomorphism to privately add the fee to fee payer point and we add refund to the user point.
let fee_payer_point = incomplete_fee_payer_point + fee_point;
let user_point = incomplete_user_point + refund_point;
benesjan marked this conversation as resolved.
Show resolved Hide resolved

// 4. We no longer need to do any elliptic curve operations with the points so we collapse them to the final
// 6. We no longer need to do any elliptic curve operations with the points so we collapse them to the final
// note hashes.
let fee_payer_note_hash = fee_payer_point.x;
let user_note_hash = user_point.x;

// 5. Finally we return the hashes.
// 7. Finally we return the hashes.
(fee_payer_note_hash, user_note_hash)
}
}
36 changes: 34 additions & 2 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 @@ -129,6 +129,31 @@ describe('e2e_fees/private_refunds', () => {
[initialAliceBalance - tx.transactionFee!, initialBobBalance + tx.transactionFee!],
);
});

// TODO(#7694): Remove this test once the lacking feature in TXE is implemented.
it('insufficient funded amount is correctly handled', async () => {
// 1. We generate randomness for Alice and derive randomness for Bob.
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.
await expect(
tokenWithRefunds.methods.private_get_name().prove({
fee: {
gasSettings: t.gasSettings,
paymentMethod: new PrivateRefundPaymentMethod(
tokenWithRefunds.address,
privateFPC.address,
aliceWallet,
aliceRandomness,
bobRandomness,
t.bobWallet.getAddress(), // Bob is the recipient of the fee notes.
true, // We set max fee/funded amount to zero to trigger the error.
),
},
}),
).rejects.toThrow('tx fee is higher than funded amount');
});
});

class PrivateRefundPaymentMethod implements FeePaymentMethod {
Expand Down Expand Up @@ -163,6 +188,12 @@ class PrivateRefundPaymentMethod implements FeePaymentMethod {
* Address that the FPC sends notes it receives to.
*/
private feeRecipient: AztecAddress,

/**
* If true, the max fee will be set to 0.
* TODO(#7694): Remove this param once the lacking feature in TXE is implemented.
*/
private setMaxFeeToZero = false,
) {}

/**
Expand All @@ -183,8 +214,9 @@ class PrivateRefundPaymentMethod implements FeePaymentMethod {
* @returns The function call to pay the fee.
*/
async getFunctionCalls(gasSettings: GasSettings): Promise<FunctionCall[]> {
// we assume 1:1 exchange rate between fee juice and token. But in reality you would need to convert feeLimit (maxFee) to be in token denomination
const maxFee = gasSettings.getFeeLimit();
// We assume 1:1 exchange rate between fee juice and token. But in reality you would need to convert feeLimit
// (maxFee) to be in token denomination.
const maxFee = this.setMaxFeeToZero ? Fr.ZERO : gasSettings.getFeeLimit();

await this.wallet.createAuthWit({
caller: this.paymentContract,
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/txe/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"formatting": "run -T prettier --check ./src && run -T eslint ./src",
"formatting:fix": "run -T eslint --fix ./src && run -T prettier -w ./src",
"test": "NODE_NO_WARNINGS=1 node --experimental-vm-modules ../node_modules/.bin/jest --passWithNoTests",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bug in TXE which resulted in a test passing even when an error was thrown. Grego fixed it in this PR and what follows are changes made by him.

"dev": "DEBUG='aztec:*' LOG_LEVEL=debug && node ./dest/bin/index.js",
"dev": "DEBUG='aztec:*' LOG_LEVEL=debug node ./dest/bin/index.js",
"start": "node ./dest/bin/index.js"
},
"inherits": [
Expand Down
10 changes: 9 additions & 1 deletion yarn-project/txe/src/oracle/txe_oracle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -772,8 +772,12 @@ export class TXE implements TypedOracle {

const executionResult = await this.executePublicFunction(targetContractAddress, args, callContext);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bug in TXE which resulted in a test passing even when an error was thrown. Grego fixed it in this PR and what follows are changes made by him.

if (executionResult.reverted) {
throw new Error(`Execution reverted with reason: ${executionResult.revertReason}`);
}

// Apply side effects
this.sideEffectsCounter = executionResult.endSideEffectCounter.toNumber();
this.sideEffectsCounter += executionResult.endSideEffectCounter.toNumber();
this.setContractAddress(currentContractAddress);
this.setMsgSender(currentMessageSender);
this.setFunctionSelector(currentFunctionSelector);
Expand Down Expand Up @@ -808,6 +812,10 @@ export class TXE implements TypedOracle {

const executionResult = await this.executePublicFunction(targetContractAddress, args, callContext);

if (executionResult.reverted) {
throw new Error(`Execution reverted with reason: ${executionResult.revertReason}`);
}

// Apply side effects
this.sideEffectsCounter += executionResult.endSideEffectCounter.toNumber();
this.setContractAddress(currentContractAddress);
Expand Down
Loading