#38671 [BC-Insight] Signer key rotation is not possible due to deadlock between submitting key rotation to Stacks and retrieving it

Submitted on Jan 9th 2025 at 09:32:27 UTC by @n4nika for Attackathon | Stacks

• Report ID: #38671

• Report Type: Blockchain/DLT

• Report severity: Insight

• Target: https://github.com/stacks-network/sbtc/tree/immunefi_attackaton_0.9/signer

• Impacts:

  • A bug in the respective layer 0/1/2 network code that results in unintended smart contract behavior with no concrete funds at direct risk

Description

Summary

Rotating a signer's key is currently impossible since signers build their state machine for DKG based on the last signer set they have in their database. This set, however, is only set AFTER a DKG was successful and a corresponding transaction was sent to Stacks. These two actions basically deadlock each other, preventing key rotations overall.

Finding Description

In transaction_signer::handle_wsts_message, we see the signing set used for DKG is retrieved from the signer's database:

WstsNetMessage::DkgBegin(_) => {
    tracing::info!("handling DkgBegin");

    if !chain_tip_report.sender_is_coordinator {
        tracing::warn!("received coordinator message from non-coordinator signer");
        return Ok(());
    }

    let signer_public_keys = self.get_signer_public_keys(bitcoin_chain_tip).await?; // <------
    // [...]
}

This defaults to the bootstrap signer set from the config if there is no other rotate-keys transaction in the signer's database. This shows us the first requirement, namely that we need to have a rotate-keys transaction confirmed on Stacks, containing a NEW signer set.

Now in order to submit such a transaction, the coordinator must get signatures for a ContractCall::RotateKeysV1 contract call, which get validated by the singers:

async fn validate<C>(&self, ctx: &C, req_ctx: &ReqContext) -> Result<(), Error>
where
    C: Context + Send + Sync,
{
    // [...]
    let Some(latest_dkg) = db.get_latest_encrypted_dkg_shares().await? else {
        return Err(Error::NoDkgShares);
    };
    let latest_public_key = latest_dkg
        .signer_set_public_keys
        .into_iter()
        .collect::<BTreeSet<_>>();
    if self.new_keys != latest_public_key {
        return Err(RotateKeysErrorMsg::SignerSetMismatch.into_error(req_ctx, self));
    }
    // [...]
}

We see, that a requirement here, is that the new signer set must match the latest signer set we have in our database. Now this signer set is ONLY set after a successful DKG.

Putting these together, we see that we have a logical deadlock making it impossible for a key rotation to happen

Severity

I see this as Insight, since key rotations are technically disabled currently. If they should be usable right now, this would be medium in my opinion since it breaks intended functionality.

Mitigation

In order to properly rotate a key, we need additional functionality for signers to add a temporary "proposed" signer set to their storage. This would then need to be retrieved when a DKG is triggered. This should probably be done manually by each individual signer in order to guarantee a quorum of signers agrees with the change (a new signer set can only be adopted, if a quorum of the old signer set agrees on it).

Note: Do NOT allow the coordinator to specify the new set since this would potentially allow them to take over the system.

Proof of Concept

I was asked to submit this without a PoC by djordon