Skip to content

fix(primitives): unlink on-chain and off-chain signer keys#693

Open
alessandromazza98 wants to merge 3 commits into
mainfrom
ale/nethermind-audit-1st-issue
Open

fix(primitives): unlink on-chain and off-chain signer keys#693
alessandromazza98 wants to merge 3 commits into
mainfrom
ale/nethermind-audit-1st-issue

Conversation

@alessandromazza98
Copy link
Copy Markdown
Contributor

@alessandromazza98 alessandromazza98 commented Apr 27, 2026

Problem

Signer::from_seed_bytes passed the same 32-byte input to both PrivateKeySigner::from_bytes and EdDSAPrivateKey::from_bytes. Leaking the on-chain key directly leaks the master seed and trivially yields the off-chain key (and vice versa). The nethermind audit flagged this as a linkability issue: compromise of one signer key implies compromise of the other.

Solution

Keep the single-seed public API but treat the input strictly as a master seed and derive two independent 32-byte subkeys with domain separation:

onchain_seed = SHA-256("world-id-protocol/signer/onchain/v1" || master_seed)
offchain_seed = SHA-256("world-id-protocol/signer/offchain/v1" || master_seed)

Because SHA-256 is one-way, recovering master_seed from either subkey is infeasible, so leaking one signing key no longer lets an attacker derive the other. The domain tags carry a /v1 suffix so the derivation can be rotated later if needed. Derived seed bytes are wrapped in zeroize::Zeroizing<[u8; 32]> so secret material is wiped from the stack on drop.

Important Notes

This is a deterministic derivation breaking change: the same master_seed now produces different on-chain/off-chain keys than before.


Note

High Risk
Changes cryptographic key derivation for both on-chain and off-chain signing keys, which is security-critical and a deterministic breaking change that alters derived addresses/keys for existing seeds. Also updates multiple integration tests/dev tooling assumptions (e.g., funding derived addresses), so regressions could surface in account registration/signing flows.

Overview
Unlinks on-chain and off-chain signer keys derived from the same seed. Signer::from_seed_bytes now treats the input as a 32-byte master seed and derives independent on-chain and off-chain subkeys via a domain-separated SHA-256(tag || master_seed) KDF, with derived secret bytes wrapped in zeroize::Zeroizing and new unit tests covering determinism and non-linkability.

Updates downstream tests and tooling to match the new derivation: e2e/gateway tests now derive pubkey/address via Signer (not raw seed bytes), issuer registration tests pre-fund the derived on-chain address before calling register, and the OPRF dev client derives the returned EdDSA private key from Signer so it matches the registered off-chain pubkey.

Reviewed by Cursor Bugbot for commit 12b735d. Bugbot is set up for automated code reviews on this repo. Configure here.

@alessandromazza98 alessandromazza98 requested a review from a team as a code owner April 27, 2026 10:05
kilianglas
kilianglas previously approved these changes Apr 27, 2026
@alessandromazza98
Copy link
Copy Markdown
Contributor Author

@paolodamico What do you think of this? The alternative to fix the issue mentioned in the Nethermind audit (which is that it's best practice to not use the same seed for the two keys) is to provide two different seeds in the input of the fn. But this would change the external API essentially.

@paolodamico
Copy link
Copy Markdown
Collaborator

I'm not convinced this is the best approach. Layering custom key derivation brings a very specific implementation expectation for key management. it further makes it difficult to update this mechanism. I think instead we should consider simply that the initializer must provide two keys, and they can be the same for backwards compatibility. another downside I see with KDF is that we're not gaining much in terms of security, the root seed will be loaded into memory every time and that is still an attack vector that can be compromised.

furthermore, it'd be ideal that you can instantiate an Authenticator with only one of the keys. for example, most of the time you'll only need the off-chain signer. in that scenario, the on-chain key doesn't ever have to be loaded into application memory. this reduces the surface area of the keys.

@alessandromazza98
Copy link
Copy Markdown
Contributor Author

I'm not convinced this is the best approach. Layering custom key derivation brings a very specific implementation expectation for key management. it further makes it difficult to update this mechanism. I think instead we should consider simply that the initializer must provide two keys, and they can be the same for backwards compatibility. another downside I see with KDF is that we're not gaining much in terms of security, the root seed will be loaded into memory every time and that is still an attack vector that can be compromised.

furthermore, it'd be ideal that you can instantiate an Authenticator with only one of the keys. for example, most of the time you'll only need the off-chain signer. in that scenario, the on-chain key doesn't ever have to be loaded into application memory. this reduces the surface area of the keys.

I see, it's ok to me to modify this PR so that you need to pass 2 seeds to the function. I don't have a strong opinion there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants