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

Prevent ANYONECANPAY-signed inputs replay #83

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

darosior
Copy link
Member

In order for watchtowers to be able to fee-bump Cancel and Emergencies transactions and to get around transactions pining, stakeholders exchange ANYONECANPAY | ALL signatures.

It was known that the malleability introduced could cause some issues, however they were minors and mostly considered in the realm of fee bumping (eg stripping the fee bumping input). Now, some design decisions make it actually practical to DOS the operation by merging Cancel or Emergencies transactions together (effectively burning an entire input to fees) under certain conditions.

Consider the Cancel transaction.
Two deposit transactions paying to the same address for the same amount will create a transaction chain ultimately paying to the Cancel output (same address and amount, as we reuse keys across the transaction chain and have static fees).
The ACP | ALL will not (as would a ACP | SINGLE) commit to the input index used, it can therefore be freely placed as second, third, or whatever input in a transaction given that this transaction contains a single output to the Cancel utxo.
Thus, the signature would be valid as a second input of a second transaction. It would effectively result in burning an entire vault.

Therefore we need a way to commit to a specifc Unvault txo (or Deposit for an Emergency) per transaction or to the index in the transaction. There are many ways to do so:

  • We could require another set of signatures, this time ACP | SINGLE which would prevent it to be used as nth input in another transaction.
  • We could abuse the nSequence and nLockTime unused bits (like Lightning does) to commit to part of the bits of the spent outpoint. However we could not stuff more than 48 bits which would make bruteforce way more practical when considering an attack from a miner.
  • We could tweak our keys or keys usage.
  • We could use an OP_RETURN output committing to the spent txid. Albeit the concern of abusing the block chain it is the most straightforward solution, making it a good candidate at this stage of development. This is what this pull request implements.

Due to exchanging ALL | ANYONECANPAY signatures, Unvault and Deposit
inputs could be replayed in other Cancel or Emergency transactions,
effectively merging those transactions.

Even though unlikely and in most cases requiring internal corruption
this would be a massive DOS, so prevent them by comitting to specific
vaults.

There exists more complicated way of comitting to each vault but we go
with a simple-to-reason-about solution for now.

Reported-by: Stepan Snigirev <snigirev.stepan@gmail.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
@darosior
Copy link
Member Author

darosior commented Apr 2, 2021

Hmm so i'm now really hesitating with tweaking the public keys à la BIP32 or Lightning Network with the tweak being the unvault outpoint, it'd be a huge win... At the expense of surely making coin tracking more complicated.

@darosior
Copy link
Member Author

darosior commented Apr 2, 2021

We could also "just" change the Cancel descriptor from

wsh(thresh(4, stk_a, stk_b, stk_c, stk_d))

To

wsh(thresh(5, stk_a, stk_b, stk_c, stk_d, hash160(H)))

With H = hash160(unvault_txid | unvault_vout). Probably better to go this way at the expense of ~83 witness units for not playing the cryptographers with no real idea of what we are doing.

@@ -145,6 +153,10 @@ transactions are never meant to be used.

The Emergency `scriptPubKey` is not known to the managers.

In addition to the deposit output, the Emergency transaction features a data-carrying output committing
to the Deposit transaction it's used for in order for the `ANYONECANPAY | ALL` signature to not be replayed
Copy link
Member

Choose a reason for hiding this comment

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

"...committing to the Deposit transaction id and the output index (vout) it is used for..."

@JSwambo
Copy link
Member

JSwambo commented Apr 29, 2021

Is there any chance that the Unvault Tx will batch deposit UTxOs in a future version? If so should we consider committing to the vout too in the Cancel and Unvault-Emergency Txs now?

What about the Spend Tx? Are they always signed with ALL?

@darosior
Copy link
Member Author

To be honest i don't think we should do this for the Cancel, but rather #83 (comment) .

What about the Spend Tx? Are they always signed with ALL?

Yes. I mean it's left to the managers, they could sign with NONE if they want, but they could also leak their keys.

@JSwambo
Copy link
Member

JSwambo commented Apr 29, 2021

Maybe I'm wrong, but it seems like there is nothing stopping an attacker from choosing H to be the same when constructing both Cancel Tx 1 and Cancel Tx 2. What verification do we have that the H are unique? What verification is there that the H actually has committed to the correct previous Unvault Txid?
Do we expect the stakeholders to verify this on the HW screen? How do they know what to compare with?

@darosior
Copy link
Member Author

darosior commented Apr 29, 2021

What verification do we have that the H are unique?

They would not exchange signatures for the same transaction.
Say A, B, C are stakeholders and C is trying to sneak an invalid commitment to the Unvault txid (or outpoint for that matter). A and B would verify C's signature against their own transaction (with the right commitment) which would raise an error.

Now, for the enforcement of the verification.. I wonder if it's possible to do on the HW and asked @stepansnigirev by mail some time ago. It seems possible since it's an information internal to the transaction, but i would have to implement it to really be sure of what it takes.

FWIW this solution would:

  • mess up our "encapsulation" in revault_tx but in my opinion it's better than messing up onchain transactions.
  • be free with Taproot.
  • is fine to only check on the watchonly wallets / watchtowers for now

@JSwambo
Copy link
Member

JSwambo commented Apr 29, 2021

What verification do we have that the H are unique?

They would not exchange signatures for the same transaction.

Right, so we assume at least 1 of the stakeholders' wallets is not compromised. Notably, this has nothing to do with their HW, unless we have some HW feature that reliably let's stakeholders check the H is consistent with the associated unvault Tx.

It seems possible since it's an information internal to the transaction, but i would have to implement it to really be sure of what it takes.

Well, isn't it information internal to the previous Tx?

@darosior
Copy link
Member Author

we assume at least 1 of the stakeholders' wallets is not compromised

One of their HW* if it can be generated by the HW. If you are the last honest stakeholder standing, your HW won't send rogue signatures to the other participants.

Also, there is the brittle argument of the watchtower ACK that may one day be checked on the HW.

Well, isn't it information internal to the previous Tx?

Your transaction refers the outpoints it spends, therefore it's part of your transaction's input.

@JSwambo
Copy link
Member

JSwambo commented Apr 29, 2021

we assume at least 1 of the stakeholders' wallets is not compromised

if it can be generated by the HW.

That's what I was getting at with ....

unless we have some HW feature that reliably let's stakeholders check the H is consistent with the associated unvault Tx.

If that feature is robust, then the assumption becomes 'at least one of the stakeholders' HW is not compromised'.

Your transaction refers the outpoints it spends, therefore it's part of your transaction's input.

ah yeah of course!

@JSwambo
Copy link
Member

JSwambo commented Apr 29, 2021

Also, there is the brittle argument of the watchtower ACK that may one day be checked on the HW.

Are you suggesting the WT could refuse to ACK if it notices the inconsistency (H incorrect)?

@darosior
Copy link
Member Author

That's what I was getting at with ....

Yes but the premise is wrong, they don't need to verify it but to trust the HW (they don't re-compute the signature by hand each time they sign, they just assume that the HW does its job right).

Are you suggesting the WT could refuse to ACK if it notices the inconsistency (H incorrect)?

Well watchtowers do verify signatures before ACKing

@JSwambo
Copy link
Member

JSwambo commented Apr 29, 2021

Yes but the premise is wrong, they don't need to verify it but to trust the HW

Ok. If the check is fully automated, then the trust is on the HW software. That's much better in terms of UX. Is this in the revault-compatibility HW features list somewhere?

@darosior
Copy link
Member Author

Is this in the revault-compatibility HW features list somewhere?

#83 (comment) :

I wonder if it's possible to do on the HW and asked stepansnigirev by mail some time ago.

Copy link
Member

@JSwambo JSwambo left a comment

Choose a reason for hiding this comment

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

given my small comment,
ACK 147faea

@darosior
Copy link
Member Author

My last approach is clearly not going to work for HWs (they can't bear storing an infinite amount of descriptors..). I'm now thinking of stuffing a short-channel-id in nLockTime and nSequence à la Lightning.
The issue is that we only have 6 bytes available where we need 8 of them for the scid (and arguably even 9)..

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