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

[Bug]: Ensure integrity of deposit requests submitted #1116

Open
1 task
djordon opened this issue Dec 13, 2024 · 8 comments · May be fixed by #1197
Open
1 task

[Bug]: Ensure integrity of deposit requests submitted #1116

djordon opened this issue Dec 13, 2024 · 8 comments · May be fixed by #1197
Assignees
Labels
bug Something isn't working emily API that communicates with Signers to trigger sBTC operations. sbtc signer binary The sBTC Bootstrap Signer. transaction library Common library for handling transaction manipulation.

Comments

@djordon
Copy link
Collaborator

djordon commented Dec 13, 2024

Bug - Ensure integrity of deposit requests

1. Description

When someone submits deposit request data to Emily, we do not have a way of ensuring that the sender is the same entity that created the deposit UTXO. This means that someone can create properly formed deposit requests to Emily that do not match any actual deposits on bitcoin, with the outcome being that actual deposits will be deemed invalid.

1.1 Context & Purpose

We may want to make some design decisions to increase the integrity of data submitted to Emily, so that we know that data stored there comes from the correct source.

2. Technical Details:

There are three solutions that come to mind and all are partly undesirable:

  1. Require a signature in the deposit request object sent to Emily. The signature must be verifiable with a public key that locked one of the inputs into the deposit transaction.
  2. Put all deposit information on chain as part of an OP_RETURN output.
  3. Give Emily access to bitcoin-core to do a full verification of the deposit request.
  4. Require a stacks transaction from an account that mirrors one of the scriptPubKeys locking an input from the deposit transaction.

Option (1) Is the easiest lift for us, but seems impractical: what is going to construct this signature? I don't see many wallets adding such functionality. Option (2) is doable, but it also requires wallets to support the creation of OP_RETURN outputs. Option (3) is like option (1) but better. Option (4) is quite cumbersome.

I think (2) is the way to go, and allows us to remove functionality from Emily. We can fit the required data into an OP_RETURN output if we are creative.

2.1 Acceptance Criteria:

  • It not possible for someone other than the depositor to inform the sBTC system of their deposits.

3. Related Issues and Pull Requests (optional):

4. Appendix

Option (2) above is more tricky than it looks, and it requires us to be a little creative because we only have 80 bytes to work with in an OP_RETURN output. A straightforward porting of the data in our current deposit transactions will show that we don't have enough space. So an alternative layout for the deposit is something like this:

0                    20             21        23               44                    64
|--------------------|--------------|---------|----------------|---------------------|
  signer pubkey hash   sbtc version   max fee   stacks address   reclaim pubkey hash

Meaning:

  1. "signer pubkey hash" is the HASH160 of the signers x-only aggregate key,
  2. "sbtc version" is just a version byte for the layout.
  3. "max fee" is the users max fee interpreted as multiples of 10000 sats.
  4. "stacks address" is the recipient address. It is always a standard address.
  5. "reclaim pubkey hash" is the HASH160 of the x-only public key locking the reclaim script.

The locktimes are fixed per sbtc version and for version 0 the lock time is 144. Reclaim scripts in general must have a fixed form given the sbtc version.

This was fun.

@djordon djordon added transaction library Common library for handling transaction manipulation. emily API that communicates with Signers to trigger sBTC operations. labels Dec 13, 2024
@djordon djordon added this to the sBTC: Nice to have milestone Dec 13, 2024
@djordon djordon added this to sBTC Dec 13, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage in sBTC Dec 13, 2024
@djordon djordon added the bug Something isn't working label Dec 13, 2024
@djordon djordon changed the title [Feature]: Ensure integrity of deposit requests submitted to Emily [Bug]: Ensure integrity of deposit requests submitted to Emily Dec 13, 2024
@djordon djordon added the sbtc signer binary The sBTC Bootstrap Signer. label Dec 14, 2024
@djordon djordon changed the title [Bug]: Ensure integrity of deposit requests submitted to Emily [Bug]: Ensure integrity of deposit requests submitted Dec 14, 2024
@aldur
Copy link
Collaborator

aldur commented Dec 14, 2024

When someone informs Emily of a deposit, they're somehow providing something akin to the proof for a Merkle Tree. If that's the case, can't Emily verify the params and reject invalid requests?

(Sorry on mobile but happy to expand on this with proper info)

@djordon
Copy link
Collaborator Author

djordon commented Dec 14, 2024

When someone informs Emily of a deposit, they're somehow providing something akin to the proof for a Merkle Tree. If that's the case, can't Emily verify the params and reject invalid requests?

Yes, it could, and Emily would need access to bitcoin-core to do a full verification of the proof. Emily currently does a partial verification. We've been tossing around the idea of having Emily verify things against bitcoin-core. I'll add this as an option.

@aldur
Copy link
Collaborator

aldur commented Dec 15, 2024

I can be convinced otherwise but I think that anything else than that would add complexity for little gain. We know how to solve this properly, we can evaluate how much of a heavy lift it is to add this to Emily.

Does it need to have access to Bitcoin core though? I'd assume the verification path can be much simpler, because we want it to be pretty specific.

@cylewitruk
Copy link
Member

cylewitruk commented Dec 15, 2024

Hm, wouldn't option 2 also let us extract (what look like) sbtc deposits directly from bitcoin blocks and then only ask emily for these, and use the intersection vs. asking Emily for everything it views as pending every block?

@djordon
Copy link
Collaborator Author

djordon commented Dec 15, 2024

Does it need to have access to Bitcoin core though? I'd assume the verification path can be much simpler, because we want it to be pretty specific.

I believe that it essentially does. There are two kinds of verification:

  1. Verify that the received data is properly formatted deposit data.
  2. Verify that the received data maps to what is on chain.

Emily does (1) but not (2), and for (2) you need Bitcoin Core or you need the user to supply a bunch of information. I think the information is something like: the header of the block that confirmed the deposit, the transaction hex, and some merkle proof of inclusion of the transaction into the block. This is off the top of my head, I'm not entirely sure that this is correct.

Hm, wouldn't option 2 also let us extract (what look like) sbtc deposits directly from bitcoin blocks and then only ask emily for these, and use the intersection vs. asking Emily for everything it views as pending every block?

Yes it would. Also, option 2 would let us extract sbtc deposits directly from bitcoin blocks without speaking to Emily at all. I'm not saying that we should do that, just that it's possible.

Hmmm... but after thinking about this some more, I'm not as sure that option 2 is well-tailored to the problem stated in the ticket. It's more complex than it needs to be for the Emily integrity issue.

Option (3) seems like the only way forward here, unless we require all deposits to have an OP_RETURN output and I wasn't advocating for that. Maybe I'm missing another option. It seems like we cannot leave Emily open to the public for anything besides some GET requests because without Bitcoin Core she cannot validate all the data that she receives. She needs a trusted source. If she has access to bitcoin-core then we'd want to stop users from DoS'ing it, so we may need tighter rate limits.

I can be convinced otherwise but I think that anything else than that would add complexity for little gain. We know how to solve this properly, we can evaluate how much of a heavy lift it is to add this to Emily.

Yeah after thinking about it some more, I agree. But, I think I just like option (2). It appeals to me as a nice feature because it allows me to bridge into sBTC directly from the L1. And I think "the decentralized, trust minimized, bridge from Bitcoin into Stacks" should have L1 support.

On the other hand it adds some complexity, requires leather work, and maybe no one besides me cares. I'd like to think through the work required so I'll open a design ticket and things will be clearer afterwards.

@cylewitruk
Copy link
Member

I think I just like option (2). It appeals to me as a nice feature because it allows me to bridge into sBTC directly from the L1. And I think "the decentralized, trust minimized, bridge from Bitcoin into Stacks" should have L1 support.

I +1 this; removing the requirement of Emily is very appealing, but yeah probably deserves its own ticket.

Option (3) seems like the only way forward here

I think that this would necessarily add 1 block to the deposit processing time, right?

@aldur
Copy link
Collaborator

aldur commented Dec 16, 2024

the header of the block that confirmed the deposit, the transaction hex, and some merkle proof of inclusion of the transaction into the block. This is off the top of my head, I'm not entirely sure that this is correct.

Thanks!

It seems that here we are trying to solve many things at once. I wonder if we can reduce scope by picking the right problem to solve.

Going back to the issue:

This means that someone can create properly formed deposit requests to Emily that do not match any actual deposits on bitcoin, with the outcome being that actual deposits will be deemed invalid.

Let's say there's a deposit request d. If we think of d as a taproot address, we can think of d = h(sender, amount, recipient, lock-time, sbtc-signers-aggregate-key, etc). That deposit d must be included in a transaction with ID txid.

Now, IIUC Emily gets some inputs to d and verifies them, and txid. But right now can't verify that txid spends to d.

The problem is that if someone provides d' (and not d) for a given txid with valid inputs, Emily will happily accept it but this will break the deposit request for d.

In that case, what I propose to solve this is to have the user provide all inputs.

Even if it's not the same user, as long as d is computed deterministically and the user provides all required to compute txid (including d and its inputs), it's fine!

This will prevent an attacker from invalidating d by providing d' for the same txid.

The nice thing about this is that it is completely stateless. Essentially, Emily reproduces the computation that the bridge performs to obtain d in the first place, then the computation that the wallets performs to get txid.

Now IIUC this should not require the Bitcoin block header. That's useful for a different problem, ie prevent someone from providing a d or a txid that has never made it to the blockchain. But we can tackle this separately imho.

@cylewitruk
Copy link
Member

cylewitruk commented Dec 16, 2024

If the goal is only to ensure that the scripts are valid and are a part of the (potential) transaction for the given txid, then we shouldn't need bitcoin-core. But we would need a bit more information to compute the txid -- it might be easier to have them send the hex-encoded transaction at that point, and we validate that we compute the same txid and that both script paths are valid given the taproot in the output.. I think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working emily API that communicates with Signers to trigger sBTC operations. sbtc signer binary The sBTC Bootstrap Signer. transaction library Common library for handling transaction manipulation.
Projects
Status: In Review
Development

Successfully merging a pull request may close this issue.

4 participants