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

[Feature]: Limit signer ability to update Emily's database #1491

Open
4 tasks
djordon opened this issue Mar 9, 2025 · 0 comments · May be fixed by #1530
Open
4 tasks

[Feature]: Limit signer ability to update Emily's database #1491

djordon opened this issue Mar 9, 2025 · 0 comments · May be fixed by #1530
Assignees
Labels
emily API that communicates with Signers to trigger sBTC operations. immunefi-scope

Comments

@djordon
Copy link
Collaborator

djordon commented Mar 9, 2025

Feature - Limit signer ability to update Emily's database

1. Description

It looks like any signer can add entries into Emily's chainstate, so long as it doesn't lead to a reorg, and the signers probably shouldn't be able to this. Moreover, when a signer submits a update request they also send over a "message" string. This "message" isn't used and should be replaced with an enum or removed.

This ticket is about two related things:

  1. Remove the signers' ability to update Emily's chainstate.
  2. Further restrict the data that gets stored in Emily's because of a request from the signers.

1.1 Context & Purpose

This ticket is just to make sure that things will go smoothly in the event of a malicious signer.

2. Technical Details:

I'm not 100% sure that the signers can update Emily's chainstate, but I think they can by making a PUT /withdrawal request with a "new" chainstate (just a greater height than the currently known height). The same issue might exist for PUT /deposit requests. Emily shouldn't trust a signer's chainstate data, and instead use her view of the chain tip instead of the signers.

Also, before updating the history attribute for a request, Emily checks to see if the exact same event exists. This is done here:

pub fn is_unnecessary(&self, entry: &WithdrawalEntry) -> bool {
entry
.history
.iter()
.rev()
.take_while(|event| event.stacks_block_height >= self.event.stacks_block_height)
.any(|event| event == &self.event)
}

Shouldn't Emily just check to see if the last update status is the same as this one?

2.1 Acceptance Criteria:

  • The signers cannot update Emily's chainstate.
  • String fields like status or status_message in the WithdrawalUpdate struct should be removed or replaced with enums.
  • Remove the last_update_height last_update_block_hash fields from the WithdrawalUpdate. Use Emily's view for these things.
  • Maybe add a timestamp attribute to deposit and withdrawal events.

3. Related Issues and Pull Requests (optional):

#1453

4. Appendix

I suspect the original motivation for allowing a signer to modify Emily's chainstate was that:

  1. Every signer is assumed honest.
  2. Emily needed to know the chainstate for each update so that she knew which updates were affected by a reorg.

But we have been moving away from (1) for quite some time, and for (2) Emily doesn't need to take the signer's word on the chainstate when they send an update. Instead, Emily should use her view of the chainstate when setting the update height and hash.

After #1453, the signers have a limited ability to update the status of deposit and withdrawal requests. After this ticket is completed, the signers should only be able to make one update to any request (assuming no reorgs), and have that update should take a fixed amount of space in Emily's database.

@djordon djordon added the emily API that communicates with Signers to trigger sBTC operations. label Mar 9, 2025
@djordon djordon added this to the sBTC: Withdrawal fine tuning milestone Mar 9, 2025
@djordon djordon added this to sBTC Mar 9, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in sBTC Mar 9, 2025
@djordon djordon moved this from Needs Triage to Todo in sBTC Mar 9, 2025
@djordon djordon changed the title [Feature]: Ensure signers cannot update Emily's chainstate [Feature]: Limit signer ability to update Emily's chainstate Mar 9, 2025
@djordon djordon changed the title [Feature]: Limit signer ability to update Emily's chainstate [Feature]: Limit signer ability to update Emily's database Mar 10, 2025
@Jiloc Jiloc moved this from Todo to In Progress in sBTC Mar 13, 2025
@Jiloc Jiloc moved this from In Progress to In Review in sBTC Mar 17, 2025
@Jiloc Jiloc linked a pull request Mar 19, 2025 that will close this issue
4 tasks
@Jiloc Jiloc linked a pull request Mar 19, 2025 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
emily API that communicates with Signers to trigger sBTC operations. immunefi-scope
Projects
Status: In Review
Development

Successfully merging a pull request may close this issue.

2 participants