SmartTokenLabs / attestation

Paper and implementation of blockchain attestations
MIT License
48 stars 10 forks source link

improvement proposal: merge CSR and NFTAttestation #203

Closed oleggrib closed 2 years ago

oleggrib commented 2 years ago

I suggest to upgrade NFT attestation flow to improve user experience and ask to sign single message. We can use that update for AutographNFT and other flows, where user create attestaion, based on simple message + web2.0 auth (looks like Ta! flow have same senseless double signature)

Lets see example of AutographNFT Currently:

  1. User signs simple text message and send signature+web2 auth data to create IDattestation (it can be reused because it doesnt have variable part, message to sign always the same)
  2. User signs "IDattestation + additional data(NFTs in our example)" to create signedNFTAttestation. Its required for the current flow, becayse signedNFTAttestation based on the signature of IDattestation, what is variable and cant be predefined.

My suggestion:

  1. we create payloadObject "additional data(NFTs in our example) + timestamps(to avoid signature reuse)" and hash it.
  2. User signs Message "Please sign message 0xff897da6799.. to create your TwitterID attestation."
  3. Dapp sends to server payloadObject + web2 auth data to create signedNFTAttestation.(without additional step)

benefits: User signs single user-friendly text message, not 2 messages. then user take attestaion +nfts and sign it

does it make sense? @colourful-land , @jot2re

cc: @zhangzhongnan928 @foxgem @AW-STJ

jot2re commented 2 years ago

Thinking a bit more about the details of this I believe it is also needed for the user to sign they twitter ID along with a hash of the NFTs and the time stamp. Otherwise a leakage of the signature could be used in a mix-and-match attack (combining it with another twitter handle) in the short time the signature will be valid. Another minor detail is that the there should also be a message linking the signature to this specific use-case (just to avoid misuse or multiple use if we end up with other settings that require a signature of the NFTs and time stamp) Finally, instead of using a time-stamp we might want to consider using an unpredictable number like we did with our initial Devcon ticket proof-of-concept.

oleggrib commented 2 years ago

@jot2re, It makes sense. unpredictable number contains timestamp and webdomain, so we limit usage to the specific website and add expiration. so object have to contain

I think would be good to add domain name to the message to sign to avoid sign some data from hidden iframe (Metamask dispalys domail, but I am not sure about all other wallets)

so user hashes that object , creates string prefix+hash + "(" + domain + ")" +postfix and send to the server:

and based on that info attestation.id creates signedNFTAttestation/CoAttestation/etc... and send it to the dapp for use with SmartContract/other

oleggrib commented 2 years ago

I have to add another comment: currently signedNFTAttestation signed by user, but in the new flow it will be signed by attestation.id, so it will need small update to the VaidationSmartContract.

oleggrib commented 2 years ago

@jot2re , @colourful-land , can we make final decision on then flow and implement it?

it will improve UX, because users try to use wallets carefully and every signature can look challenging for wallet with some funds.

cc @AW-STJ

jot2re commented 2 years ago

@jot2re, It makes sense. unpredictable number contains timestamp and webdomain, so we limit usage to the specific website and add expiration. so object have to contain

  • user identifier
  • unpredictable number
  • NFTs

I think would be good to add domain name to the message to sign to avoid sign some data from hidden iframe (Metamask dispalys domail, but I am not sure about all other wallets)

so user hashes that object , creates string prefix+hash + "(" + domain + ")" +postfix and send to the server:

  • object
  • prefix
  • postfix
  • signature
  • identifier_auth_data (for attestation.id to check identifier validity)

and based on that info attestation.id creates signedNFTAttestation/CoAttestation/etc... and send it to the dapp for use with SmartContract/other

I believe that in a situation as this it makes sense that all the data that is humanly readable is presented in a readable format to the user. This ensures that they have a better understanding of what they sign and don't have to trust the hex data. This is basically what we worked on in issue #201. In relation to the changes suggested by @oleggrib I think it makes sense that what is signed in readable format is:

  1. Hidden identifier
  2. unpredictable number (including expiration time, domain, randomness)
  3. hash of NFTs
  4. Context information

Besides that, the user sends a proof that the hidden identifier contains their email, and shares this email in plain with attestation.id. This allows attestation.id to verify the mail via OPT (assuming everything happens in one session over TLS). Then attestation.id co-signs the bundle signed by the user. Thus the user's mail still remain hidden and we ensure that a malicious attestation.id cannot impersonate a user without the user's private key.

However we now end up having to have the user sign lots of different pieces of information that is readable in plain, so maybe it makes sense to do this flow as EIP712?

jot2re commented 2 years ago

then user take attestaion +nfts and sign it

I am a bit unsure if this is actually what you mean @oleggrib since then both the attestation request and the usage of it is still signed, right? Or maybe I am misunderstanding. Is my understanding as I wrote it above correct?

oleggrib commented 2 years ago

@jot2re , I am trying to find best way to allow user sign single message. Current problem - recursive signature. userSignature1->identifierAttestation->nftAttestation->secondUserSignature

if we will change object to avoid recursive signatures then we can use payloadSignature to verify users address.

I suggest split signedIdentifierAttestation to the 2 objects:

  1. PayloadObject(timestamp, domain, nfts, identifier) signed by user
  2. IdentifierAttestation based on twitter Auth info + PayloadObject signature

@colourful-land , any suggestions?

jot2re commented 2 years ago

I don't think it is necessary to have two signatures. Consider the following flow (in line with what @oleggrib suggest):

  1. User requests UN from attestation.id and an OTP to their email.
  2. User constructs and signs PayloadObject(UN (including expiration and domain in plain), hash( hidden identifier, NFTs, OTP), context information.
  3. The user then sends PayloadObject along with email, ZKP of hidden identifier to email, NFTs, OTP
  4. Attestation.id validates the PayloadObject, ZKP and OTP and if it is okay, then it co-signs PayloadObject and returns it to the user.
  5. The user can then post this co-signed PayloadObject to the smart contract
  6. The smart contract validates attesation.id's signature and that expiration has not passed. It can then do its thing with the user's address which it can extract from the user's signature.

This way the user only signs once and the usage of a UN protects against future signings. The email is never revealed to the smart contract, yet what is signed is actually linked to the email verification via the OTP

Note: Even better we can have the OTP can simply be used as part of the randomness/context in the ZKP.

@colourful-land what do you think about this flow?

oleggrib commented 2 years ago

@jot2re , in case if we want to use IdentifierAttestation multiple times (per 1 hour or per 1 day ...) then we have to skip OTP from PayloadObject, because on next usage we will not have saved OTP and it can be not very good user experience if user have to enter OTP on each request.

And in case of twitter attestation we dont have OTP, but auth token. So better send OTP/twitterAuth/etc... in raw format to the attestation.id ( time to time attestation.id can add new auth type or update existing, so better separate it from PayloadFlow and let attestation.id deside format of the auth data )

SmartLayer commented 2 years ago

@oleggrib :

I have to add another comment: currently signedNFTAttestation signed by user, but in the new flow it will be signed by attestation.id, so it will need small update to the VaidationSmartContract.

Why? I feel much was lost in the communication, attestation.id is a shared service to issue identifier attestations, hence the name, that is, it's not attetation.com or attestation.xyz, it is attestation.id so that it is for identifier attestations. why should it sign nft attestations? Why should a user trust a 3rd party service to dictate what happens to their NFT? I will be pretty upset as an NFT holder to know that there exists a key that is not in my wallet that can do things to my nft.

SmartLayer commented 2 years ago

@jot2re @oleggrib I'm afraid we forgot why NFT attestation and the signature to obtain identifier attestation was created in the first place.

First, there is a size limit on how much data we can send to a smart contract and we should not waste it, since additional data easily gets denied by eth nodes or cost gas. we learned this the hard way in 2017 when a user buys too many FIFA tickets and the transaction can't go through. I remember even 9 tickets is too many. With the bag of goods @oleggrib is proposing I think we will hit the same problem again. Byte length was also the main reason why we used ASN.1 instead of plain text json file for attestations. (the secondary reason was to make the format extensible).

Second, that this statement is incorrect. If it is implemented that way, it must change:

User signs simple text message and send signature+web2 auth data to create IDattestation (it can be reused because it doesnt have variable part, message to sign always the same)

The reason is the message must not be reusable, since the user is expected to recreate identifier attestation once it expires, and there is an attack model to capture this message and reuse it.

Third, attestation.id is a 3rd party identifier attestation issue, for god sake, the name was chosen to reflect that (.id), if you make it an authority in token ownership or derivation right no one is going to trust it as a 3rd party service. Are you okay if your lawyer, who can attest to your identity, also passes judgement on who should go to jail??

Finally, the signature (2 times) is only for the first time user, for the sequential transactions they only needed to sign nftAttestation. It's not impossible to combine these two (the signature for attestation.id to grant identifier attestation and the signature to create nftattestation), but it's going to be nearly impossible, without breaking the design, as the data needed for the 2 signature has no cross section! NFT data is not needed when user try to get identifier attestation, and it is actually harmful for it to be there, since attestation.id should only learn about user's identifier and not what the user is going to do with it. Imagine a user wants to sell a $10m USD worth of NFT and now attestation.id knows what the user is going to do with it. Again, identifier attestation is needed as part of NFTAttestation, so how do you get that before it is granted?

Signing messages 2 times is not good, but leaking privacy to attestation.id and/or making attestation.id an authority on anything but identity (therefore killing its reusability for other NFT projects) is even worse. remember we are building attestation.id with the view for them to be directly used as an identifier attestation issuer, not as a client-deployed micro component for each client to issue their own identifier. I just don't see how every token project issuing their own identifier attestation will work in the long run, since the result is not interoperatable, token projects already know identifier data therefore can just issue tokens based on them without any need to attest in the first place, and the network is centralised around projects.

jot2re commented 2 years ago

Second, that this statement is incorrect. If it is implemented that way, it must change:

User signs simple text message and send signature+web2 auth data to create IDattestation (it can be reused because it doesnt have variable part, message to sign always the same)

The reason is the message must not be reusable, since the user is expected to recreate identifier attestation once it expires, and there is an attack model to capture this message and reuse it.

@colourful-land Currently (as least when reading the java code) the AttestationRequest is based on a nonce, consisting of the address of the user, attestation.id and a timestamp. Hence reuse of the original AttestationRequest is not possible since the timestamp is only valid for 20 min (current configuration).

jot2re commented 2 years ago

@colourful-land in relation to the other point you make, I totally understand the desire to segregate roles. However, consider the flow mentioned above:

I don't think it is necessary to have two signatures. Consider the following flow (in line with what @oleggrib suggest):

  1. User requests UN from attestation.id and an OTP to their email.
  2. User constructs and signs PayloadObject(UN (including expiration and domain in plain), hash( hidden identifier, NFTs, OTP), context information.
  3. The user then sends PayloadObject along with email, ZKP of hidden identifier to email, NFTs, OTP
  4. Attestation.id validates the PayloadObject, ZKP and OTP and if it is okay, then it co-signs PayloadObject and returns it to the user.
  5. The user can then post this co-signed PayloadObject to the smart contract
  6. The smart contract validates attesation.id's signature and that expiration has not passed. It can then do its thing with the user's address which it can extract from the user's signature.

This way the user only signs once and the usage of a UN protects against future signings. The email is never revealed to the smart contract, yet what is signed is actually linked to the email verification via the OTP

Note: Even better we can have the OTP can simply be used as part of the randomness/context in the ZKP.

Here attestation.id does not need to learn about the NFTs. It only learns a hash digest (which can easily be hiding by adding some salt). All attestation.id learns is that the user wish to create a single-use attestation to be used for NFTs. However, I also understand if that might be too much, because in that case attestation.id could block all requests of this nature if it wants to not allow attestations to be used for NFTs.