Check length of polynomials

[xoloki]

This change fixes a DoS that results from incorrectly sized polynomials. A polynomial that is too large will result in signatures that fail to verify, and one that is too small will lead to a panic when initializing the signature aggregator.

Fixes #87

[djordon]

The logic looks good, but shouldn't we add tests here too?

[xoloki]

The logic looks good, but shouldn't we add tests here too?

Indeed, I’ll add some tomorrow

[xoloki]

Ok @djordon new generic test added and called from v1 and v2.

[djordon]

Looks good.

Suppose at the end of all of this, there is a bad party member who uses an incorrect polynomial length. Does WSTS associate the party id to their public key or is that all left to the caller?

[xoloki]

Looks good.

Suppose at the end of all of this, there is a bad party member who uses an incorrect polynomial length. Does WSTS associate the party id to their public key or is that all left to the caller?

Oh, very good question. It looks like SignerStateMachine was trying to be clever, and looked for bad public shares before calling compute_secrets. This allowed it to handle both v1 and v2, since in the v1 case we need to associate the party_id with a signer_id.

So we need to check that the poly length equals the threshold there too.

[xoloki]

Thinking about this more, maybe we shouldn't do a separate check for BadPublicShares, and just let it fall out of compute_secrets like BadPrivateShares:

Here's the duplicated BadPublicShares check:
https://github.com/Trust-Machines/wsts/blob/main/src/state_machine/signer/mod.rs#L346
https://github.com/Trust-Machines/wsts/blob/main/src/state_machine/signer/mod.rs#L371

Here's the actual compute_secrets call which only handles BadPrivateShares:
https://github.com/Trust-Machines/wsts/blob/main/src/state_machine/signer/mod.rs#L400

Here's where we map the party_id to a signer_id:
https://github.com/Trust-Machines/wsts/blob/main/src/state_machine/signer/mod.rs#L405

Thoughts?

[xoloki]

I went ahead and fixed it in the separate check, then added a test. Are you okay with merging as is @djordon ?