Commit-Boost / commit-boost-client

Commit-Boost allows Ethereum validators to safely run MEV-Boost and community-built commitment protocols
https://commit-boost.github.io/commit-boost-client/
Apache License 2.0
80 stars 33 forks source link

Signer: sign message with wrong domain mask #156

Closed Akagi201 closed 1 month ago

Akagi201 commented 1 month ago

In crates/common/src/signature.rs, there are two functions to sign a message.

https://github.com/Commit-Boost/commit-boost-client/blob/1dc63e2494a145718261a64baf90331adaef6d29/crates/common/src/signature.rs#L86

pub fn sign_builder_root(
    chain: Chain,
    secret_key: &BlsSecretKey,
    object_root: [u8; 32],
) -> BlsSignature {
    let domain = chain.builder_domain();
    let signing_root = compute_signing_root(object_root, domain);
    sign_message(secret_key, &signing_root)
}

pub fn sign_commit_boost_root(
    chain: Chain,
    secret_key: &BlsSecretKey,
    object_root: [u8; 32],
) -> BlsSignature {
    let domain = compute_domain(chain, COMMIT_BOOST_DOMAIN);
    let signing_root = compute_signing_root(object_root, domain);
    sign_message(secret_key, &signing_root)
}

I found that when a signer_client calls signer_client.request_consensus_signature(signature_req) the commit-boost-signer will only use sign_commit_boost_root to sign the message, so it will use the COMMIT_BOOST_DOMAIN domain mask.

But in helix

https://github.com/gattaca-com/helix/blob/3533496355837a4bb96b330d7e6c49408228ed7e/crates/utils/src/signing.rs#L78

It uses DomainType::ApplicationBuilder which is the same as what sign_builder_root uses. This makes me fail to verify the signature when I calls API to helix.

So why do we use COMMIT_BOOST_DOMAIN to sign the message, not sign_builder_root? How can I pass the signature the correct way?

ltitanb commented 1 month ago

we discussed this on a community call and decided that for safety reason , signatures for external modules would only be on a custom domain #70, this is to ensure there is no equivocation possible with other consensus signatures. I believe that part in helix was done before the change, and would need to be updated

Akagi201 commented 1 month ago

Thanks, understood.