[Feature] Implement operation sign_hash on Ledger and Trezor signers

[zerosnacks]

Component

signers

Describe the feature you would like

Related: foundry-rs/foundry#9944

cast wallet sign --trezor --no-hash 0x1c8aff950685c2ed4bc3174f3472287b56d9517b9c948127319a09a7a36deac8

and

cast wallet sign --ledger --no-hash 0x1c8aff950685c2ed4bc3174f3472287b56d9517b9c948127319a09a7a36deac8

currently yield the error: "Error: operation sign_hash is not supported by the signer`"

as they are not implemented:

alloy/crates/signer-ledger/src/signer.rs

Lines 82 to 89 in 88fc403

	#[cfg_attr(target_arch = "wasm32", async_trait(?Send))]
	#[cfg_attr(not(target_arch = "wasm32"), async_trait)]
	impl Signer for LedgerSigner {
	async fn sign_hash(&self, _hash: &B256) -> Result<Signature> {
	Err(alloy_signer::Error::UnsupportedOperation(
	alloy_signer::UnsupportedSignerOperation::SignHash,
	))
	}

alloy/crates/signer-trezor/src/signer.rs

Lines 39 to 47 in 88fc403

	#[cfg_attr(target_arch = "wasm32", async_trait(?Send))]
	#[cfg_attr(not(target_arch = "wasm32"), async_trait)]
	impl Signer for TrezorSigner {
	#[inline]
	async fn sign_hash(&self, _hash: &B256) -> Result<Signature> {
	Err(alloy_signer::Error::UnsupportedOperation(
	alloy_signer::UnsupportedSignerOperation::SignHash,
	))
	}

Additional context

For safety, the hash is just a simple keccak256 of "hello"

0x1c8aff950685c2ed4bc3174f3472287b56d9517b9c948127319a09a7a36deac8 == keccak256("hello")

[neithanmo]

@zerosnacks if possible, can I look at this? I am familiar with ledger signing mechanism, I would like to see how this is handled in alloy

[zerosnacks]

Sure, that would be great! Assigned @neithanmo

[klkvr]

Not sure this is possible, iiuc it's common for hardware wallets to only allow signing transactions or EIP-712 messages. Allowing to blindly sign random hashes is not great in terms of security

[zerosnacks]

I agree we should be hesitant to implement this as we want users to move away from blind signing. Alternative way to resolve this - if we choose not to implement it - is to provide a better error as we choose it to be not implemented rather than we lack support for it. Right now it can appear to users that their device does not support an operation.

[neithanmo]

I agree, actually, some ledger applications require enabling blind-signing mode to sign hashes otherwise, this could get rejected. depending on the context here, another alternative is to use EIP-191 or EIP-712 as @klkvr mentioned

[neithanmo]

A good documentation and an clear error message seem a good approach, making clear that it is not the device neither us, but a design choice based on security and best practices