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

Using 0x0000...00 as a secret works in the client but not in the smart contracts #3091

Closed
pirapira opened this Issue Nov 28, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@pirapira
Contributor

pirapira commented Nov 28, 2018

Currently, the SecretRegistry contract refuses 0x00...00 as the secret. If I create a hash lock with keccack(0x0000..00), send the secret, but never send a balance proof, what happens?

If the recipient believes it has received an offchain payment, but cannot really claim the payment online, the experience is unpleasant.

  • See if Python client accepts keccack(0x0000..00) as a secrethash when it receives LockedTransfer message
  • See if Python client accepts 0x0000..00 as a secret when it receives RevealSecret message
  • See if Python client accepts a 31-byte or 33-byte secret when it receives RevealSecret message (this is @konradkonrad 's idea).

(How I found it: I started reading Solidy code with #3070 in mind.)

Ping: @loredanacirstea

@pirapira pirapira self-assigned this Nov 29, 2018

@pirapira

This comment has been minimized.

Contributor

pirapira commented Nov 29, 2018

if Python client accepts keccack(0x0000..00) as a secrethash when it receives LockedTransfer message

Seems like yes. message_handler.py: def handle_message_lockedtransfer() has a similar check against already-registered secrets, but not against 0x0000...00.

@pirapira pirapira added the security label Nov 29, 2018

@pirapira

This comment has been minimized.

Contributor

pirapira commented Nov 29, 2018

if Python client accepts 0x0000..00 as a secret when it receives RevealSecret message

Seems like yes. I followed till mediator.py: def handle_offchain_secretreveal(), but I haven't seen a check against 0x0000...00.

@pirapira

This comment has been minimized.

Contributor

pirapira commented Nov 29, 2018

if Python client accepts a 31-byte or 33-byte secret

Seems like no. On the messages, they are always 32 bytes.

secret = make_field('secret', 32, '32s')

secret = make_field('secret', 32, '32s')

For now I suggest adding an explicit check about the length somewhere.

@pirapira

This comment has been minimized.

Contributor

pirapira commented Nov 29, 2018

@LefterisJP does this look like a critical bug?

@loredanacirstea

This comment has been minimized.

Collaborator

loredanacirstea commented Nov 29, 2018

My reasoning for not allowing registration for a secret of the form bytes32(0) in the SecretRegistry contract was that it could imply an error in the way the secret is computed.
E.g.: the secret is somehow not set and it is considered 0x00..0 -> if this happens, then multiple transfers could potentially "reuse" the bytes32(0) secret without knowing.

Indeed, I should have checked the client implementation more closely.

@pirapira pirapira assigned LefterisJP and unassigned pirapira Nov 29, 2018

@LefterisJP

This comment has been minimized.

Collaborator

LefterisJP commented Nov 29, 2018

Yes indeed after an offline discussion with @pirapira this seems like a potential critical issue in misalignment between client and contracts which can lead to loss of funds when a user needs to go on-chain.

The suggested solution would be to do the following three things.

  • Make sure that we never generate a secret of 0x00000.000
  • Reject any RevealSecret message with secret being 0x00000.000
  • Calculate the keccak hash of 0x00000.000and if we get a message with that hashlock reject it.

How does that sound @pirapira?

@pirapira

This comment has been minimized.

Contributor

pirapira commented Nov 29, 2018

Perhaps do all of them. Just 1. is not enough. Do 2. and 3.

@LefterisJP

This comment has been minimized.

Collaborator

LefterisJP commented Nov 29, 2018

@pirapira yeah that's what I meant. Edited comment for clarity.

@LefterisJP LefterisJP changed the title from An attack idea with 0x0000...00 as a secret (not sure if it works) to Using 0x0000...00 as a secret works in the client but not in the smart contracts Nov 29, 2018

LefterisJP added a commit to LefterisJP/raiden that referenced this issue Nov 29, 2018

Do not generate a 0x0 secret when initiating a transfer
It would not be accepted in the contracts.

Part of raiden-network#3091

LefterisJP added a commit to LefterisJP/raiden that referenced this issue Nov 30, 2018

LefterisJP added a commit to LefterisJP/raiden that referenced this issue Nov 30, 2018

LefterisJP added a commit to LefterisJP/raiden that referenced this issue Nov 30, 2018

LefterisJP added a commit to LefterisJP/raiden that referenced this issue Nov 30, 2018

LefterisJP added a commit that referenced this issue Nov 30, 2018

Do not generate a 0x0 secret when initiating a transfer
It would not be accepted in the contracts.

Part of #3091
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment