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

Expand Low Level Shutter Spec #28

Merged
merged 6 commits into from
Dec 20, 2023

Conversation

jannikluhn
Copy link
Contributor

This PR expands /shutter/low-level.md. In particular, it

  • specifies the encrypting RPC server.
  • fixes a malleability vulnerability.
  • defines the encode_gt function, the last missing crypto function.
  • removes the section on the decryption key relay (which is not needed anymore as Nethermind has a libp2p implementation now).


#### eth_sendRawTransaction

The server exposes a method `eth_sendRawTransaction` that behaves differently to the standard. It takes a hex encoded, `0x` prefixed string `h` as its sole parameter. If not exactly one parameter is provided, the server responds with an error with code `-32602`. If `h` is not the hex encoding of a byte string `b` or `b` is not the RLP encoding of a valid transaction `tx`, it returns an error with code `-32602`.
Copy link
Member

Choose a reason for hiding this comment

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

can you use regular names instead of single letters h, b, s? Should make it a little more readable.


If the sender of `tx` has insufficient funds to pay the transaction fee or if the sender's nonce does not match the account nonce, the server responds with an error with code `-32000`.

Upon receiving the request, the server seeks the eon key `eon_key = getEonKey(eon)` with `eon = keyperSetManager.getKeyperSetIndexBySlot(s + keyper_set_change_lookahead)` where `s` is the current slot number. It also generates two random 32 byte strings `sigma` and `identity_prefix` using a cryptographically secure random number generator. Based on these values, it computes `encrypted_transaction = encrypt(b, identity, eon_key, sigma)` with `identity = compute_identity(identity_prefix, address)`.
Copy link
Member

Choose a reason for hiding this comment

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

Should the caller of eth_sendRawTransaction specify the slot at which to encrypt the transaction at? Or the point is to have the same API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the point is to have the exact same API so that the encrypting RPC server can be a drop-in replacement. Also, there is no target slot number anymore, transactions get decrypted and included whenever there's space (see the get_next_transactions function).

Copy link
Member

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Thanks! Quite straightforward and complete, just some minor comments.

Can you detail more the malleability vulnerability addressed by the PR?

@jannikluhn
Copy link
Contributor Author

Can you detail more the malleability vulnerability addressed by the PR?

Sure! The way the encryption works in the end is by XORing the message with a symmetric key (see compute_c3). This is malleable: An attacker could take an encrypted message, flip the nth bit, and after decryption you'd get the original message, but with the nth bit flipped.

To prevent this, there's a kind of checksum in the encrypted message, which includes the hash of the encrypted message (compute_r, compute_c1). So if an attacker modifies the message, you'd notice that the checksum doesn't match and you can abort. The bug was that we didn't include the message hash when computing the checksum, and we also didn't check the checksum properly during decryption. Note that the bug is only in the implementation, the formal specification is fine.

Now, for Shutterized Gnosis Chain all this doesn't technically matter since all we encrypt is signed messages, so all malleability attempts would simply result in invalid signatures. But obviously it's still better to fix it anyway.

Copy link
Member

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments! Looks good to me

@dapplion dapplion merged commit 6617d21 into gnosischain:master Dec 20, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants