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

Liquid Claim covenant #113

Merged
merged 3 commits into from
Jan 31, 2024
Merged

Liquid Claim covenant #113

merged 3 commits into from
Jan 31, 2024

Conversation

michael1011
Copy link
Member

@michael1011 michael1011 commented Jan 28, 2024

@michael1011 michael1011 changed the title Claim cov Liquid Claim covenant Jan 28, 2024
@madis
Copy link

madis commented Jan 29, 2024

@michael1011 you're on fire 🔥 🤩

lib/liquid/swap/Claim.ts Outdated Show resolved Hide resolved
lib/liquid/swap/Claim.ts Show resolved Hide resolved

claimCovenantOutputIndex,
liquidOps.OP_INSPECTOUTPUTASSET,
ops.OP_DROP,
Copy link

Choose a reason for hiding this comment

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

Is it definitely safe to just compare the asset hash returned by INSPECTOUTPUTASSET without checking the asset prefix, too ?

It might be safe to compare value returned by INSPECTOUTPUTVALUE without checking the commitment prefix, because unconfidential value will be size 8 and confidential will be size 32

But the asset size is 32 bytes regardless if confidential or not, and as I remember in the blinding process there's 'balancing' values are added to the blinders of the last output

My understanding is that it is not required for these 'balancing' values to be added to any particular output, and maybe that balancing values can be also somehow distributed between different outputs ?

Might it be that it is possible to craft an arbitrary value for the (blinded) asset in one of the outputs ? Like, the covenant checks for unblinded asset value, ignores the commitment prefix, but the blinded value with the same bytes as for unblinded asset is crafted, while it is actually a blinded value?

I cannot say that I really understand the math behind the blinding process, so I wanted to ask someone knowledgeable in this, tagging them.

@apoelstra @roconnor-blockstream

Choose a reason for hiding this comment

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

In short, yes, it is safe. But the argument for safety is complicated and crypto-heavy so if it's possible to avoid relying on it I would.

The argument is roughly:

  • An explict asset ID is formed as the hash of some transaction data from the original issuance, to ensure that it is unique.
  • The "bare" confidential asset ID is derived from this by a hash-to-curve operation that re-hashes the the original asset ID and coerces that hash to a point. (It actually does this twice in slighly different ways and adds the resulting points, an algorithm we call "SWU" after its inventors Shallue, van Woestijne and Ulas.)
  • Then the actual confidential asset that you put on-chain starts with the "bare" confidential asset and adds r*G for a value of r which is supposed to be uniformly random but is 100% user-controllable.

So the question becomes: can you find a collision between a value x which is the output of SHA2 and a value SWU(y) + rG where y is a potentially-different output of SHA2 and r is attacker-controlled.

You can see that this is a variant of "can you collide SHA2" and I suspect you can even prove that it's impossible if you model both SHA2 and SWU as random oracles. But it's not the kind of argument I'd like to depend on if I could avoid it.

Copy link

Choose a reason for hiding this comment

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

Thanks for the explanation !

Copy link

@dgpv dgpv Jan 30, 2024

Choose a reason for hiding this comment

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

Isn't it "finding privkey for arbitrary pubkey" rather than sha2 collision ? If I understand correctly, you would need to get the binary representation of SWU(y) + rG to be equal to x, SWU(y) can be fixed, and that means you need to find r such as rG = x - SWU(y) (assuming that x can be interpreted as a point on the curve).

Choose a reason for hiding this comment

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

Ah, yep, that would also do it.

Copy link

Choose a reason for hiding this comment

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

Adding a check for the value/asset to be explicit should be enough

OP_1
OP_EQUALVERIFY

Copy link
Member

Choose a reason for hiding this comment

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

Thank you guys for the independent review, glad to hear what we are doing is safe 🙏

Copy link
Member Author

@michael1011 michael1011 Jan 30, 2024

Choose a reason for hiding this comment

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

In short, yes, it is safe. But the argument for safety is complicated and crypto-heavy so if it's possible to avoid relying on it I would.

As @tiero said, checking explicitly for the prefix fixes the collision concern. That's what we'll do. Thank you for the explanation!

@michael1011 michael1011 merged commit 90174e9 into master Jan 31, 2024
4 checks passed
@michael1011 michael1011 deleted the claim-cov branch January 31, 2024 15:40
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.

7 participants