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

dex: swap claim should check diversified address integrity #3978

Closed
Tracked by #1724
redshiftzero opened this issue Mar 8, 2024 · 0 comments · Fixed by #4013
Closed
Tracked by #1724

dex: swap claim should check diversified address integrity #3978

redshiftzero opened this issue Mar 8, 2024 · 0 comments · Fixed by #4013
Assignees
Labels
A-dex Area: Relates to the dex consensus-breaking breaking change to execution of on-chain data security Issues or work related to security.
Milestone

Comments

@redshiftzero
Copy link
Contributor

redshiftzero commented Mar 8, 2024

Is your feature request related to a problem? Please describe.

You should only be able to claim swap outputs once. Instead, an attacker can construct a valid SwapClaim multiple times for the same swap.

Iin the swap claim, we are demonstrating the integrity of the revealed nullifier via nullifier = hash3(nk, position, swap commitment). Elsewhere in the circuit we have the transmission key pk_d, and the diversified base B_d in order to demonstrate swap commitment integrity. However, pk_d and nk are not checked to be associated with one another, so an attacker can witness multiple values for nk in order to generate multiple nullifiers that will each be accepted as valid.

PoC: f21d8b5

     Running `target/debug/pcli tx swap 1test_usd --into upenumbra`
Scanning blocks from last sync height 3025 to latest height 3025
[0s] ██████████████████████████████████████████████████       0/0       0/s ETA: 0s
building transaction...
finished proving in 13.828 seconds [3 actions, 3 proofs, 2373 bytes]
broadcasting transaction and awaiting confirmation...
transaction broadcast successfully: 3fbe3561656f9b8bd4394c077003b58a360aae9966edd15362e26196eadaee13
transaction confirmed and detected: 3fbe3561656f9b8bd4394c077003b58a360aae9966edd15362e26196eadaee13 @ height 3029
Swap submitted and batch confirmed!
You will receive outputs of 0test_usd and 49.999mpenumbra. Claiming now...
building transaction...
finished proving in 8.715 seconds [1 actions, 1 proofs, 570 bytes]
broadcasting transaction and awaiting confirmation...
transaction broadcast successfully: d8bc3575a646a5825a3e9ebcd17194076d07a9ebcb2c15b8ab280703be7570c2
transaction confirmed and detected: d8bc3575a646a5825a3e9ebcd17194076d07a9ebcb2c15b8ab280703be7570c2
PoC: claiming swap outputs again
building transaction...
finished proving in 7.258 seconds [1 actions, 1 proofs, 570 bytes]
broadcasting transaction and awaiting confirmation...
transaction broadcast successfully: 159b7294bf3dc7629e42659d5ca29277d27f7865d9d54af615f72966c8994c70
transaction confirmed and detected: 159b7294bf3dc7629e42659d5ca29277d27f7865d9d54af615f72966c8994c70

Describe the solution you'd like

  1. Witness ak
  2. Using 1 and the existing variables in circuit for pk_d and nk, check pk_d = [ivk] B_d deriving the ivk from ak and nk
  3. Check diversified base B_d is not identity
@redshiftzero redshiftzero added the security Issues or work related to security. label Mar 8, 2024
@github-project-automation github-project-automation bot moved this to 🗄️ Backlog in Penumbra Mar 8, 2024
@erwanor erwanor changed the title swap claim should check diversified address integrity dex: swap claim should check diversified address integrity Mar 12, 2024
@erwanor erwanor added the A-dex Area: Relates to the dex label Mar 12, 2024
@cronokirby cronokirby self-assigned this Mar 12, 2024
@erwanor erwanor added the consensus-breaking breaking change to execution of on-chain data label Mar 15, 2024
@github-project-automation github-project-automation bot moved this from 🗄️ Backlog to Done in Penumbra Mar 15, 2024
redshiftzero added a commit that referenced this issue Mar 19, 2024
add note for future maintainers to only use the
`NullifierDerivationCircuit` on the client-side, since the nk is not
actually demonstrated to be associated with the address on the note in
circuit (related issue #3978)
@erwanor erwanor added this to the Sprint 2 milestone Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dex Area: Relates to the dex consensus-breaking breaking change to execution of on-chain data security Issues or work related to security.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants