-
Notifications
You must be signed in to change notification settings - Fork 598
feat: Private refunds #7226
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
feat: Private refunds #7226
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| [package] | ||
| name = "private_fpc_contract" | ||
| authors = [""] | ||
| compiler_version = ">=0.25.0" | ||
| type = "contract" | ||
|
|
||
| [dependencies] | ||
| aztec = { path = "../../../aztec-nr/aztec" } | ||
| authwit = { path = "../../../aztec-nr/authwit" } | ||
| private_token = { path = "../private_token_contract" } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| use dep::aztec::protocol_types::abis::log_hash::LogHash; | ||
| use dep::aztec::oracle::logs::emit_unencrypted_log_private_internal; | ||
| use dep::aztec::hash::compute_unencrypted_log_hash; | ||
| use dep::aztec::context::PrivateContext; | ||
|
|
||
| fn emit_nonce_as_unencrypted_log(context: &mut PrivateContext, nonce: Field) { | ||
| let counter = context.next_counter(); | ||
| let event_type_id = 0; | ||
| let log_slice = nonce.to_be_bytes_arr(); | ||
| let log_hash = compute_unencrypted_log_hash(context.this_address(), event_type_id, nonce); | ||
| // 44 = addr (32) + selector (4) + raw log len (4) + processed log len (4) | ||
| let len = 44 + log_slice.len().to_field(); | ||
| let side_effect = LogHash { value: log_hash, counter, length: len }; | ||
| context.unencrypted_logs_hashes.push(side_effect); | ||
| let _void = emit_unencrypted_log_private_internal(context.this_address(), event_type_id, nonce, counter); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| mod lib; | ||
|
|
||
| contract PrivateFPC { | ||
| use dep::aztec::protocol_types::{abis::log_hash::LogHash, address::AztecAddress}; | ||
| use dep::aztec::state_vars::SharedImmutable; | ||
| use dep::private_token::PrivateToken; | ||
| use crate::lib::emit_nonce_as_unencrypted_log; | ||
|
|
||
| #[aztec(storage)] | ||
| struct Storage { | ||
| other_asset: SharedImmutable<AztecAddress>, | ||
| admin_npk_m_hash: SharedImmutable<Field> | ||
| } | ||
|
|
||
| #[aztec(public)] | ||
| #[aztec(initializer)] | ||
| fn constructor(other_asset: AztecAddress, admin_npk_m_hash: Field) { | ||
| storage.other_asset.initialize(other_asset); | ||
| storage.admin_npk_m_hash.initialize(admin_npk_m_hash); | ||
| } | ||
|
|
||
| #[aztec(private)] | ||
| fn fund_transaction_privately(amount: Field, asset: AztecAddress, nonce: Field) { | ||
| assert(asset == storage.other_asset.read_private()); | ||
| // convince the FPC we are not cheating | ||
| context.push_new_nullifier(nonce, 0); | ||
|
Comment on lines
+25
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I understood: why does this convince the FPC we are not cheating?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also: should we mix the nonce with the msg.sender before emitting it as a nullifier? I'm worried a user could abuse it to invalidate another user's txs.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suppose user chooses nonce of 42, and the amount due to the FPC is 100. Then if user chooses a nonce of 42 again, then it will create a duplicate note if the amount due to the FPC is the same. For that reason I think we ought not mix the nonce with the msg.sender, since we would want the other user's tx to be invalidated for the same reason.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. Still, it feels risky to have a nullifier purely controlled by the caller. What if we mixed
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could do that, but from the protocol perspective, the nullifier will be siloed to this contract instance, so no risk there, and from the FPC's perspective, it wants to do absolutely as little as possible. That said, I also have a somewhat icky feeling about this. Can you think of any attack or exploit that could come from it? It would be helpful to understand if this type of pattern is "safe" generally, even if it feels risky.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In this PR I just fixed the privacy leak which Lasse and Santiago spotted by introducing the second randomness value (computed by hashing the first one) and emitting as unencrypted log only the second value which should reliably prevent the front-running attack (basically the same solution as what Mitch described in the edit).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do I understand this correctly that this is only a problem because we do not yet inject nonces to public note hashes (issue here)? If we had the nonces there I think there should be no issue with duplicate notes. If this is the case I would prioritize finishing tackling that issue.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @benesjan I think that the original issue raised by Palla has already been fixed by emitting the hash of the user's randomness as the unencrypted log. But I think that if we injected nonces into public notes, we might be able to get away with not pushing the nullifier of the user's randomness (since that was originally done to convince the FPC that its note would have a unique preimage/image). Thought I'm not sure in that case how the FPC/user recover the nonce that gets injected?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool cool. I am just working on storage slot as point PR and with that we'll be able to fix the remaining vulnerabilities (#7323 and #7324). Once that is done the note hash will be siloed to user address by the contract. At that point the severity of the attack of re-using randomness (assuming we remove pushing the nullifier) drops from an attacker potentially preventing someone else from spending their notes (by emitting the same note and then spending it) to a user potentially losing money if they are stupid and use the same randomness twice. We should protect users against stupidity so it makes sense to prioritize tackling public note hash nonce. Once we have that implemented we can safely drop emitting the nullifier. This is super nice because we want to hyper-optimize these functions since it's a cost which I assume will be part of all the txs. Will take a note to do that once all the pieces are in place.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just nuked the nullifier in this PR as it's no longer needed. |
||
|
|
||
| // allow the FPC to reconstruct their fee note | ||
| emit_nonce_as_unencrypted_log(&mut context, nonce); | ||
|
|
||
| PrivateToken::at(asset).setup_refund( | ||
| storage.admin_npk_m_hash.read_private(), | ||
| context.msg_sender(), | ||
| amount, | ||
| nonce | ||
| ).call(&mut context); | ||
| context.set_as_fee_payer(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| [package] | ||
| name = "private_token_contract" | ||
| authors = [""] | ||
| compiler_version = ">=0.25.0" | ||
| type = "contract" | ||
|
|
||
| [dependencies] | ||
| aztec = { path = "../../../aztec-nr/aztec" } | ||
| compressed_string = { path = "../../../aztec-nr/compressed-string" } | ||
| authwit = { path = "../../../aztec-nr/authwit" } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,239 @@ | ||
| mod types; | ||
| mod test; | ||
|
|
||
| // Minimal token implementation that supports `AuthWit` accounts and private refunds | ||
|
|
||
| contract PrivateToken { | ||
| use dep::compressed_string::FieldCompressedString; | ||
| use dep::aztec::{ | ||
| hash::compute_secret_hash, | ||
| prelude::{NoteGetterOptions, Map, PublicMutable, SharedImmutable, PrivateSet, AztecAddress}, | ||
| protocol_types::{ | ||
| abis::function_selector::FunctionSelector, hash::pedersen_hash, | ||
| constants::GENERATOR_INDEX__INNER_NOTE_HASH | ||
| }, | ||
| oracle::unsafe_rand::unsafe_rand, | ||
| encrypted_logs::encrypted_note_emission::{encode_and_encrypt_note, encode_and_encrypt_note_with_keys} | ||
| }; | ||
| use dep::authwit::{auth::{assert_current_call_valid_authwit, assert_current_call_valid_authwit_public}}; | ||
| use crate::types::{token_note::{TokenNote, TOKEN_NOTE_LEN}, balances_map::BalancesMap}; | ||
| use dep::std::embedded_curve_ops::EmbeddedCurvePoint; | ||
| use dep::std::ec::tecurve::affine::Point; | ||
|
|
||
| #[aztec(storage)] | ||
| struct Storage { | ||
| admin: PublicMutable<AztecAddress>, | ||
| minters: Map<AztecAddress, PublicMutable<bool>>, | ||
| balances: BalancesMap<TokenNote>, | ||
| total_supply: PublicMutable<U128>, | ||
| symbol: SharedImmutable<FieldCompressedString>, | ||
| name: SharedImmutable<FieldCompressedString>, | ||
| decimals: SharedImmutable<u8>, | ||
| } | ||
|
|
||
| #[aztec(public)] | ||
| #[aztec(initializer)] | ||
| fn constructor(admin: AztecAddress, name: str<31>, symbol: str<31>, decimals: u8) { | ||
| assert(!admin.is_zero(), "invalid admin"); | ||
| storage.admin.write(admin); | ||
| storage.minters.at(admin).write(true); | ||
| storage.name.initialize(FieldCompressedString::from_string(name)); | ||
| storage.symbol.initialize(FieldCompressedString::from_string(symbol)); | ||
| storage.decimals.initialize(decimals); | ||
| } | ||
|
|
||
| #[aztec(public)] | ||
| fn set_admin(new_admin: AztecAddress) { | ||
| assert(storage.admin.read().eq(context.msg_sender()), "caller is not admin"); | ||
| storage.admin.write(new_admin); | ||
| } | ||
|
|
||
| #[aztec(public)] | ||
| fn public_get_name() -> pub FieldCompressedString { | ||
| storage.name.read_public() | ||
| } | ||
|
|
||
| #[aztec(private)] | ||
| fn private_get_name() -> pub FieldCompressedString { | ||
| storage.name.read_private() | ||
| } | ||
|
|
||
| unconstrained fn un_get_name() -> pub [u8; 31] { | ||
| storage.name.read_public().to_bytes() | ||
| } | ||
|
|
||
| #[aztec(public)] | ||
| fn public_get_symbol() -> pub FieldCompressedString { | ||
| storage.symbol.read_public() | ||
| } | ||
|
|
||
| #[aztec(private)] | ||
| fn private_get_symbol() -> pub FieldCompressedString { | ||
| storage.symbol.read_private() | ||
| } | ||
|
|
||
| unconstrained fn un_get_symbol() -> pub [u8; 31] { | ||
| storage.symbol.read_public().to_bytes() | ||
| } | ||
|
|
||
| #[aztec(public)] | ||
| fn public_get_decimals() -> pub u8 { | ||
| storage.decimals.read_public() | ||
| } | ||
|
|
||
| #[aztec(private)] | ||
| fn private_get_decimals() -> pub u8 { | ||
| storage.decimals.read_private() | ||
| } | ||
|
|
||
| unconstrained fn un_get_decimals() -> pub u8 { | ||
| storage.decimals.read_public() | ||
| } | ||
|
|
||
| #[aztec(public)] | ||
| fn set_minter(minter: AztecAddress, approve: bool) { | ||
| assert(storage.admin.read().eq(context.msg_sender()), "caller is not admin"); | ||
| storage.minters.at(minter).write(approve); | ||
| } | ||
|
|
||
| #[aztec(private)] | ||
| fn privately_mint_private_note(amount: Field) { | ||
| let caller = context.msg_sender(); | ||
| let header = context.get_header(); | ||
| let caller_npk_m_hash = header.get_npk_m_hash(&mut context, caller); | ||
| storage.balances.add(caller_npk_m_hash, U128::from_integer(amount)).emit(encode_and_encrypt_note(&mut context, caller, caller)); | ||
| PrivateToken::at(context.this_address()).assert_minter_and_mint(context.msg_sender(), amount).enqueue(&mut context); | ||
| } | ||
|
|
||
| #[aztec(public)] | ||
| 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); | ||
| } | ||
|
|
||
| #[aztec(private)] | ||
| fn transfer_from(from: AztecAddress, to: AztecAddress, amount: Field, nonce: Field) { | ||
| if (!from.eq(context.msg_sender())) { | ||
| assert_current_call_valid_authwit(&mut context, from); | ||
| } else { | ||
| assert(nonce == 0, "invalid nonce"); | ||
| } | ||
|
|
||
| let header = context.get_header(); | ||
| let from_ovpk = header.get_ovpk_m(&mut context, from); | ||
| let from_ivpk = header.get_ivpk_m(&mut context, from); | ||
| let from_npk_m_hash = header.get_npk_m_hash(&mut context, from); | ||
| let to_ivpk = header.get_ivpk_m(&mut context, to); | ||
| let to_npk_m_hash = header.get_npk_m_hash(&mut context, to); | ||
|
|
||
| let amount = U128::from_integer(amount); | ||
| storage.balances.sub(from_npk_m_hash, amount).emit(encode_and_encrypt_note_with_keys(&mut context, from_ovpk, from_ivpk)); | ||
| storage.balances.add(to_npk_m_hash, amount).emit(encode_and_encrypt_note_with_keys(&mut context, from_ovpk, to_ivpk)); | ||
| } | ||
|
|
||
| #[aztec(private)] | ||
| fn transfer(to: AztecAddress, amount: Field) { | ||
| let from = context.msg_sender(); | ||
| let header = context.get_header(); | ||
| let from_ovpk = header.get_ovpk_m(&mut context, from); | ||
| let from_ivpk = header.get_ivpk_m(&mut context, from); | ||
| let from_npk_m_hash = header.get_npk_m_hash(&mut context, from); | ||
| let to_ivpk = header.get_ivpk_m(&mut context, to); | ||
| let to_npk_m_hash = header.get_npk_m_hash(&mut context, to); | ||
|
|
||
| let amount = U128::from_integer(amount); | ||
| storage.balances.sub(from_npk_m_hash, amount).emit(encode_and_encrypt_note_with_keys(&mut context, from_ovpk, from_ivpk)); | ||
| storage.balances.add(to_npk_m_hash, amount).emit(encode_and_encrypt_note_with_keys(&mut context, from_ovpk, to_ivpk)); | ||
| } | ||
|
|
||
| #[aztec(private)] | ||
| fn balance_of_private(owner: AztecAddress) -> pub Field { | ||
| let header = context.get_header(); | ||
| let owner_npk_m_hash = header.get_npk_m_hash(&mut context, owner); | ||
| storage.balances.to_unconstrained().balance_of(owner_npk_m_hash).to_integer() | ||
| } | ||
|
|
||
| unconstrained fn balance_of_unconstrained(owner_npk_m_hash: Field) -> pub Field { | ||
| storage.balances.balance_of(owner_npk_m_hash).to_integer() | ||
| } | ||
|
|
||
| #[aztec(private)] | ||
| fn setup_refund( | ||
| fee_payer_npk_m_hash: Field, | ||
| sponsored_user: AztecAddress, | ||
| funded_amount: Field, | ||
| refund_nonce: Field | ||
| ) { | ||
| assert_current_call_valid_authwit(&mut context, sponsored_user); | ||
| let header = context.get_header(); | ||
| let sponsored_user_npk_m_hash = header.get_npk_m_hash(&mut context, sponsored_user); | ||
| let sponsored_user_ovpk = header.get_ovpk_m(&mut context, sponsored_user); | ||
| let sponsored_user_ivpk = header.get_ivpk_m(&mut context, sponsored_user); | ||
| storage.balances.sub(sponsored_user_npk_m_hash, U128::from_integer(funded_amount)).emit(encode_and_encrypt_note_with_keys(&mut context, sponsored_user_ovpk, sponsored_user_ivpk)); | ||
| let points = TokenNote::generate_refund_points( | ||
| fee_payer_npk_m_hash, | ||
| sponsored_user_npk_m_hash, | ||
| funded_amount, | ||
| refund_nonce | ||
| ); | ||
| context.set_public_teardown_function( | ||
| context.this_address(), | ||
| FunctionSelector::from_signature("complete_refund(Field,Field,Field,Field)"), | ||
| [points[0].x, points[0].y, points[1].x, points[1].y] | ||
| ); | ||
| } | ||
|
|
||
| #[aztec(public)] | ||
| #[aztec(internal)] | ||
| fn complete_refund( | ||
| fpc_point_x: Field, | ||
| fpc_point_y: Field, | ||
| user_point_x: Field, | ||
| user_point_y: Field | ||
| ) { | ||
| let fpc_point = EmbeddedCurvePoint { x: fpc_point_x, y: fpc_point_y, is_infinite: false }; | ||
| let user_point = EmbeddedCurvePoint { x: user_point_x, y: user_point_y, is_infinite: false }; | ||
| let tx_fee = context.transaction_fee(); | ||
| let note_hashes = TokenNote::complete_refund(fpc_point, user_point, tx_fee); | ||
|
|
||
| // `compute_inner_note_hash` manually, without constructing the note | ||
| // `3` is the storage slot of the balances | ||
| context.push_new_note_hash( | ||
| pedersen_hash( | ||
| [PrivateToken::storage().balances.slot, note_hashes[0]], | ||
| GENERATOR_INDEX__INNER_NOTE_HASH | ||
| ) | ||
| ); | ||
| context.push_new_note_hash( | ||
| pedersen_hash( | ||
| [PrivateToken::storage().balances.slot, note_hashes[1]], | ||
| GENERATOR_INDEX__INNER_NOTE_HASH | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| /// Internal /// | ||
|
|
||
| #[aztec(public)] | ||
| #[aztec(internal)] | ||
| fn _reduce_total_supply(amount: Field) { | ||
| // Only to be called from burn. | ||
| let new_supply = storage.total_supply.read().sub(U128::from_integer(amount)); | ||
| storage.total_supply.write(new_supply); | ||
| } | ||
|
|
||
| /// Unconstrained /// | ||
|
|
||
| unconstrained fn admin() -> pub Field { | ||
| storage.admin.read().to_field() | ||
| } | ||
|
|
||
| unconstrained fn is_minter(minter: AztecAddress) -> pub bool { | ||
| storage.minters.at(minter).read() | ||
| } | ||
|
|
||
| unconstrained fn total_supply() -> pub Field { | ||
| storage.total_supply.read().to_integer() | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| mod basic; | ||
| mod utils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why pass the
assetas an argument at all?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thinking here is the same as the existing FPC- this FPC could be used with a whitelist of supported underlying tokens.