API crash preventing correct processing of deposits
Description
Brief/Intro
The Signers network uses a multi-signature mechanism to execute sBTC deposits. For example, in a 3-5 multi-signature, as long as 3 coordinators are honest, the Signers network can operate normally.
However, in the current implementation, as long as a malicious coordinator calls the Emily::update_deposits api to update the status of deposits from Pending to Confirmed, all Signers will no longer be able to process these deposits.
Vulnerability Details
The emily/handler/src/api/handlers/deposit.rs::update_deposits code is as follows.
#[instrument(skip(context))]
pub async fn update_deposits(
context: EmilyContext,
body: UpdateDepositsRequestBody,
) -> impl warp::reply::Reply {
debug!("In update deposits");
// Internal handler so `?` can be used correctly while still returning a reply.
async fn handler(
context: EmilyContext,
body: UpdateDepositsRequestBody,
) -> Result<impl warp::reply::Reply, Error> {
// Get the api state and error if the api state is claimed by a reorg.
//
// Note: This may not be necessary due to the implied order of events
// that the API can receive from stacks nodes, but it's being added here
// in order to enforce added stability to the API during a reorg.
let api_state = accessors::get_api_state(&context).await?;
api_state.error_if_reorganizing()?;
// Validate request.
let validated_request: ValidatedUpdateDepositsRequest = body.try_into()?;
// Infer the new chainstates that would come from these deposit updates and then
// attempt to update the chainstates.
let inferred_chainstates = validated_request.inferred_chainstates()?;
for chainstate in inferred_chainstates {
// TODO(TBD): Determine what happens if this occurs in multiple lambda
// instances at once.
crate::api::handlers::chainstate::add_chainstate_entry_or_reorg(&context, &chainstate)
.await?;
}
// Create aggregator.
let mut updated_deposits: Vec<(usize, Deposit)> =
Vec::with_capacity(validated_request.deposits.len());
// Loop through all updates and execute.
for (index, update) in validated_request.deposits {
let updated_deposit =
accessors::pull_and_update_deposit_with_retry(&context, update, 15).await?;
updated_deposits.push((index, updated_deposit.try_into()?));
}
updated_deposits.sort_by_key(|(index, _)| *index);
let deposits = updated_deposits
.into_iter()
.map(|(_, deposit)| deposit)
.collect();
let response = UpdateDepositsResponse { deposits };
Ok(with_status(json(&response), StatusCode::CREATED))
}
// Handle and respond.
handler(context, body)
.await
.map_or_else(Reply::into_response, Reply::into_response)
}
It does not check the identity of the caller. According to the sBTC architecture, all coordinators have the authority to call the API. Therefore, any coordinator can maliciously call the API to destroy the status of deposits in Emily.
A malicious coordinator can update all deposits to Confirmed, and then all other coordinators will not process them anymore.
Impact Details
This will cause the Signers network to no longer be able to process user deposits.
Fix
Emily should keep all the opinions of the coordinator on updating the deposits status and execute them when the opinions meet the threshold. Just like the Signer, it should follow the principle of multi-signature.
References
None
Proof of Concept
Patch docker/sbtc/Dockerfile, fix database bug
RUN git clone https://github.com/stacks-network/sbtc.git
WORKDIR /code/sbtc
-ARG GIT_BRANCH=main
+ARG GIT_BRANCH=immunefi_attackaton_0.9
RUN git checkout $GIT_BRANCH
# Run an install that we'll cache the result of and then build the code
make devenv-up
make devenv-down
docker compose -f docker/docker-compose.yml --profile default --profile bitcoin-mempool --profile sbtc-signer build
make devenv-up
Wait for a while, run the demo
./signers.sh demo
Wait for a while, run the PoC. This PoC simulates user behavior to create 10 deposits, and then simulates a malicious coordinator calling Emily::update_deposits to update the status of these deposits to Confirmed
cargo run -p signer --bin poc5
You will find that these deposits are not executed.