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.

Sign up for GitHub

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

Jump to bottom

[Feature]: Explicit duplicate deposit txid check is missing #518

Open
evonide opened this issue Sep 12, 2024 · 2 comments · May be fixed by #1425
Open

[Feature]: Explicit duplicate deposit txid check is missing #518

evonide opened this issue Sep 12, 2024 · 2 comments · May be fixed by #1425
Labels
emily API that communicates with Signers to trigger sBTC operations. flagged by AR Issue / bug / suggestion filed by Asymmetric Research immunefi-scope P3 Low Priority

Comments

@evonide
Copy link

evonide commented Sep 12, 2024

(Low) Explicit duplicate deposit txid check is missing

1. Description

When creating a deposit via Emily API you can specify a txid and vout to point to the UTXO that’s supposed to be used for depositing.

sbtc/emily/handler/src/api/handlers/deposit.rs

Lines 194 to 244 in 033e2cb

pub async fn create_deposit(
context: EmilyContext,
body: CreateDepositRequestBody,
) -> impl warp::reply::Reply {
// Internal handler so `?` can be used correctly while still returning a reply.
async fn handler(
context: EmilyContext,
body: CreateDepositRequestBody,
) -> Result<impl warp::reply::Reply, Error> {
// Set variables.
let api_state = accessors::get_api_state(&context).await?;
api_state.error_if_reorganizing()?;
let chaintip = api_state.chaintip();
let stacks_block_hash: String = chaintip.key.hash;
let stacks_block_height: u64 = chaintip.key.height;
let status = Status::Pending;
// Make table entry.
let deposit_entry: DepositEntry = DepositEntry {
key: DepositEntryKey {
bitcoin_txid: body.bitcoin_txid,
bitcoin_tx_output_index: body.bitcoin_tx_output_index,
},
parameters: DepositParametersEntry {
reclaim_script: body.reclaim,
..Default::default()
},
history: vec![DepositEvent {
status: StatusEntry::Pending,
message: "Just received deposit".to_string(),
stacks_block_hash: stacks_block_hash.clone(),
stacks_block_height,
}],
status,
last_update_block_hash: stacks_block_hash,
last_update_height: stacks_block_height,
..Default::default()
};
// Validate deposit entry.
deposit_entry.validate()?;
// Add entry to the table.
accessors::add_deposit_entry(&context, &deposit_entry).await?;
// Respond.
let response: Deposit = deposit_entry.try_into()?;
Ok(with_status(json(&response), StatusCode::CREATED))
}
// Handle and respond.
handler(context, body)
.await
.map_or_else(Reply::into_response, Reply::into_response)
}

Potential duplicates or reuse of an existing txid shouldn't be a problem due to the database's design which has a primary (and hence unique) key over (txid, output_index). However, there should be explicit checks inside the code to verify that a TX has not been seen before in addition to that to avoid potential long-term regressions.
@djordon djordon added the emily API that communicates with Signers to trigger sBTC operations. label Sep 24, 2024
@will-corcoran will-corcoran added the flagged by AR Issue / bug / suggestion filed by Asymmetric Research label Sep 27, 2024
@aldur aldur added the P3 Low Priority label Sep 27, 2024
@aldur
Copy link
Collaborator

aldur commented Feb 21, 2025 •
edited
Loading

@Jiloc please comment so that @evonide can check if fixed/acknowledge and close this (if appropriate).

@Jiloc
Copy link
Collaborator

Jiloc commented Feb 23, 2025

At the time this issue was opened, duplicate deposits were indeed prevented by the DynamoDB constraints on (txid, output_index), but the concern was still valid since the logic allowed overriding and replacing the original deposit data.

Since then, we have introduced validation against the raw transaction hex (#1197), which means that even though the code still technically "allows" an override for pending and reprocessing deposits (except for stacks_block_* fields), it is now practically impossible to modify an existing deposit with different data. This was originally designed to allow correcting deposit and reclaim scripts when needed, but with the current validation mechanism, a conflicting deposit request simply wouldn't pass.

So this is not a security concern as of now, but adding an explicit duplicate error would make this behavior clearer and more explicit

@Jiloc Jiloc linked a pull request Feb 23, 2025 that will close this issue
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
emily API that communicates with Signers to trigger sBTC operations. flagged by AR Issue / bug / suggestion filed by Asymmetric Research immunefi-scope P3 Low Priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[chore] duplicate check on create_deposit stacks-network/sbtc
6 participants
@aldur @Jiloc @matteojug @djordon @evonide @will-corcoran