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

[rllola]

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 : Zondax/filecoin-signing-tools#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

forest/vm/message/src/signed_message.rs

Line 29 in 4eb74f9

pub fn new_from_parts(

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

[q9f]

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

It appears you use r, s, and recovery id which is just the y-parity as opposed to Bitcoin or Ethereum that use v? Does the public key match but only the address won't?

[q9f]

Looks like we set all addresses to a mainnet default:

https://github.com/ChainSafe/forest/blob/main/vm/address/src/lib.rs#L59
https://github.com/ChainSafe/forest/blob/main/vm/address/src/lib.rs#L107

[rllola]

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.

[LesnyRumcajs]

Looks like we set all addresses to a mainnet default:

@q9f Are you sure about that? Seems we set the default network here:

forest/forest/src/daemon.rs

Lines 32 to 39 in 356757e

	#[cfg(feature = "testnet")]
	address::NETWORK_DEFAULT
	.set(address::Network::Testnet)
	.unwrap();
	#[cfg(not(feature = "testnet"))]
	address::NETWORK_DEFAULT
	.set(address::Network::Mainnet)
	.unwrap();

Disclaimer - I'm new here and I don't have a full picture (yet!). :)

[q9f]

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.

[rllola]

Would it fix this problem once this one is closed ? #1402

I had this problem in the past and after talking to one of the dev they added this function for me to be able to change the network from the default : https://github.com/ChainSafe/forest/blob/main/vm/address/src/lib.rs#L167

[q9f]

#1402 is much more involved, and while it is arguably slightly related, it wouldn't necessarily address this issue.

[LesnyRumcajs]

Do you think we could perform only a partial comparison here for verification?

forest/crypto/src/signature.rs

Line 153 in 674fba5

if &rec_addr == addr {

That is to say, compare only the payload here? So far the generated comparison operator is checking both fields.

forest/vm/address/src/lib.rs

Lines 65 to 68 in 7727b33

	pub struct Address {
	network: Network,
	payload: Payload,
	}

If I understand correctly this is our reference:
https://github.com/filecoin-project/lotus/blob/39755a294aaae75d5b59992292142f4d755fa750/lib/sigs/secp/init.go#L50

And given the Address is defined as
https://github.com/filecoin-project/go-address/blob/5769c0de15a02ef38055aeb277ff159db822c5bc/address.go#L39

I believe it should be fine as it compares only the payload (right?). Or are there other implications I should be more aware of?

[q9f]

Or are there other implications I should be more aware of?

Well, it's all about assumptions. Traditionally, you do not compare addresses at all, instead you compare the uncompressed public keys and derive the address in a later step for convenience.

The way it is implemented in Forest leaves room for speculation.

1. Should you be able to pass the network version to the recovery function?
2. Should you compare the uncompressed public keys instead of the addresses and avoid network selection completely?
3. Should you ignore the network prefix and just compare the payload, potentially breaking assumptions?

I would do 1 or 2, but not 3 as this might break things we are not aware of.

[rllola]

I am a bit concerned by the fact that a testnet signed transaction can be broadcasted in mainnet. Shouldn't something in the way we sign integrate the notion of network ?

[q9f]

I am a bit concerned by the fact that a testnet signed transaction can be broadcasted in mainnet. Shouldn't something in the way we sign integrate the notion of network ?

Replay protection should definitely be part of the transaction protocol specification. This is less of a client-specific issue.

In Ethereum, you basically sign the chain ID and encode it in the v value of the signature instead of using a recovery ID.

How does Filecoin deal with this?

[LesnyRumcajs]

Ah yes, I see, the network was in the address string itself, my bad.

In general it looks to me like the same issue exists in the reference Lotus implementation, the logic looks the same, is this correct? So whichever approach we'd take it'd be best to be aligned with Lotus on this one?

[q9f]

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

[arajasek]

Hey, great discussion here!

1. 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).
2. 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.

[q9f]

Thank you!

1. 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).

In this case, we will disregard the prefix and just match the payload, as @LesnyRumcajs suggested. 👍🏼

2. 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.

What does a transaction in Filecoin look like and where can I read more about that?

[ec2]

@q9f This is what a transaction looks like

[q9f]

posted this filecoin-project/FIPs#301 and proposed an FIP.