forest: SignedMessage::new_from_part result in 'Secp signature verification failed' error because of Network value

Describe the bug Cannot build SignedMessage using new_from_part because the recovered address has the wrong network.

See here : https://github.com/ChainSafe/forest/blob/main/crypto/src/signature.rs#L150

To Reproduce See this issue with the test case using testnet and mainnet address : https://github.com/Zondax/filecoin-signing-tools/issues/422

Expected behaviour I expect the recovered address to have the same network value than in the unsigned message.

Other information and links All the step https://github.com/ChainSafe/forest/blob/4eb74f908375d7ea0f97fa8e47a3da0e2e619357/vm/message/src/signed_message.rs#L29 https://github.com/ChainSafe/forest/blob/main/crypto/src/signature.rs#L105 https://github.com/ChainSafe/forest/blob/main/crypto/src/signature.rs#L136 https://github.com/ChainSafe/forest/blob/main/crypto/src/signature.rs#L150 https://github.com/ChainSafe/forest/blob/main/crypto/src/signature.rs#L187 https://github.com/ChainSafe/forest/blob/main/crypto/src/signature.rs#L200

About this issue

Original URL
State: closed
Created 2 years ago
Comments: 17 (13 by maintainers)

Commits related to this issue

#1419 - fix signature verification fiasco — committed to ChainSafe/forest by LesnyRumcajs 2 years ago
#1419 - fix signature verification fiasco (#1437) — committed to ChainSafe/forest by LesnyRumcajs 2 years ago

Most upvoted comments

Hey, great discussion here!

The network version is intended to be purely cosmetic – it is only the actual address payload that matters. So in general, yeah, you wanna disregard the t vs f for everything except UX. Lotus does this with one exception – the internal wallet keystore is indexed by the address string, including the network version (but this is fully internal and not consensus-relevant).
Yeah, currently the entirety of the replay protection is “never reuse an address between a testnet and mainnet”. I agree that that is not good, it’s not suuuper easy to solve, though. We should discuss it.

arajasek on Feb 11, 2022

posted this https://github.com/filecoin-project/FIPs/discussions/301 and proposed an FIP.

q9f on Feb 16, 2022

@q9f This is what a transaction looks like

ec2 on Feb 11, 2022

If this is the case, we might want to bring this up during the next core dev call…

q9f on Feb 9, 2022

We are all new here 🤣

The problem is, as far as I understand it, that we default to mainnet in case we are not running a configured client/daemon.

How do the crypto crates function if they are only tasked to recover a signature? The TODO in the address crate suggests that someone stumbled upon that before us already and suggested there need to be more flexibility.

q9f on Feb 9, 2022

Hi @q9f

Is the signature wrong or the recovered address? I’m new to Filecoin and try to understand how different networks are handled.

I verified the signature independently and it is a valid one. I tried then to get the recovered address and I had the same payload value but a different network.

Address { network: Testnet, payload: Secp256k1([69, 19, 240, 77, 214, 36, 247, 249, 201, 132, 33, 237, 114, 215, 9, 174, 159, 3, 201, 184]) }
Address { network: Mainnet, payload: Secp256k1([69, 19, 240, 77, 214, 36, 247, 249, 201, 132, 33, 237, 114, 215, 9, 174, 159, 3, 201, 184]) }
thread 'api::tests::conversion_signed_messages' panicked at 'called `Result::unwrap()` on an `Err` value: "Secp signature verification failed"', signer/src/api.rs:1005

Looks like we set all addresses to a mainnet default

I also notice that. I am not sure what I did that would make it work for testnet but not mainnet. It does bug me.

rllola on Feb 9, 2022