Closed hdevalence closed 1 year ago
```the fixed-length encoding we use to construct the swap ciphertext````
This part isn't entirely clear to me - since it's fixed length, how should I correctly add this new field without breaking any expectations unnecessarily?
@aubrika The fixed-length encoding is the one defined here:
https://github.com/penumbra-zone/penumbra/blob/main/crypto/src/dex/swap/plaintext.rs#L143-L155
We have to use a specially defined, fixed-length encoding when encrypting the swap plaintext, rather than the normal proto encoding, because proto encodings have lengths that vary based on contents (so the size of the ciphertext would leak information). We don't use it anywhere else, though, it's an internal implementation detail of the encryption mechanism.
To change it, increase SWAP_LEN_BYTES
by 32 (the size of the blinding factor) and copy in self.blinding_factor.to_bytes()
to the array, then do the same in the decoding here:
https://github.com/penumbra-zone/penumbra/blob/main/crypto/src/dex/swap/plaintext.rs#L163-L206
Add the swap blinding factor to the SwapPlan, and extend the Planner API to choose a random blinding factor when constructing the swap plan, similar to other blinding factors (e.g., the note blinding in an output plan, etc)
Also asked elsewhere, but what should be the relationship between the blinding factor chosen when constructing the swap plan for the SwapPlan blinding factor field, and the blinding factor field present on the SwapPlaintext?
Add the swap blinding factor to the SwapPlan, and extend the Planner API to choose a random blinding factor when constructing the swap plan, similar to other blinding factors (e.g., the note blinding in an output plan, etc)
Also asked elsewhere, but what should be the relationship between the blinding factor chosen when constructing the swap plan for the SwapPlan blinding factor field, and the blinding factor field present on the SwapPlaintext?
Answered elsewhere, but for the record: the original scoping was wrong, and duplicated the blinding factor between the SwapPlaintext
and the SwapPlan
(which already has the blinding factor, transitively, via the SwapPlaintext
).
Is your feature request related to a problem? Please describe.
1544 flags an issue with the current implementation of the swap/swapclaim mechanism: because the output note plaintexts are generated by the claimer, and not verified as part of the proof, a malicious claimer (in possession of only a full viewing key) can burn output funds by correctly constructing output notes and proving correctness of their note commitments, but not correctly encrypting the notes themselves to the recipient.
Fixing this has two parts. In the first part, this issue, we change the swap/swapclaim mechanism so that the output notes are deterministically derived only from data in the original swap, so that the entity that signed the swap can always derive the output notes. In the second part, later, we would like to eliminate the deterministically-derived notes from the swapclaim, to save space on chain and work for other clients.
Describe the solution you'd like
Fq
element) to the swap plaintextPlanner
API to choose a random blinding factor when constructing the swap plan, similar to other blinding factors (e.g., the note blinding in an output plan, etc)SwapPlaintext::asset_id
) to compute the asset id ashash_7(DOM_SEP, (swap_blinding, amount, ...))
instead ofhash_6(DOM_SEP, (amount, ...))
.SwapProof
. Currently, there's no asset id integrity check in theSwapProof
to adjust -- this is actually a security-critical soundness bug. To fix it:swap_nft_asset_id
field from theSwapProof
. Instead, recompute what it should be inside of the proof verification, by using the data in theSwapProof
to construct aSwapPlaintext
, and compute the expected asset ID by callingSwapPlaintext::asset_id
, rather than using the attacker-controlled asset ID.SwapClaimProof
, and adjust its checks too. Currently, the swap claim proof checks the asset ID using aswap_nft_asset_id_integrity
gadget, which constructs aSwapPlaintext
out of the data in theSwapClaimProof
, recomputes the expected asset ID, and checks that the provided one matches. Instead, we should also remove theswap_nft_asset_id
from theSwapClaimProof
, and rather than checking it against the expected value we recompute, we should just use the recomputed value directly.[x] Change the
SwapClaim
to deterministically generate note blinding factors from the original randomness in the swap blinding:pub SwapPlaintext::output_blinding_factors(&self) -> (Fq, Fq)
method that generates the blinding factors for the two output notes, with something like the following (see also theSwapPlaintext::asset_id()
implementation):impl SwapPlaintext { pub fn output_blinding_factors(&self) -> (Fq, Fq) { ( hash_1(OUTPUT_1_BLINDING_DOMAIN_SEPARATOR, self.swap_blinding), hash_1(OUTPUT_2_BLINDING_DOMAIN_SEPARATOR, self.swap_blinding), ) } }
This shouldn't require any changes to the view service, or downstream users of the planner API, because it only changes how the blinding factors are derived (deterministically, rather than internal randomness), and because it only changes the format of the
SwapPlaintext
, which is stored in the view service as a blob.