Fix vulnerability when processing bls sig transactions #287
Conversation
There was a problem hiding this comment.
Nice fix!
One thing we can consider in future, currently check is done at the beginning of the processing function to constrain computing resources, alternative would be to do the check after verifying bls signature as it the signature would be valid it would means that validator (or someone possesing validator bls private key) signed invalid last commit hash, which could be evidence of misbehaviour.
It's interesting to think about. So, if the signature is valid but is signed over a different LastCommitHash, should it be considered as misbehaviour? I'm not sure, the signer might be innocent because he might not know the existence of other forks.
| @@ -163,6 +163,31 @@ func FuzzWrappedCreateValidator(f *testing.F) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestInvalidLastCommitHash(t *testing.T) { | |||
There was a problem hiding this comment.
Would it be better to make it a Fuzz test?
There was a problem hiding this comment.
I do not see that it would bring that much value. Here we just want test this concrete bug triggering scenario where message is perfectly valid except for lc hash.
Yup exactly, this is not really clear cut that why for now we just reject messages with bad last commit hashes. |
Fixes bug that could enable malicious validator to send valid bls signature over invalid last commit hash. This could lead to corrupting bls multisig.
One thing we can consider in future, currently check is done at the beginning of the processing function to constrain computing resources, alternative would be to do the check after verifying bls signature as it the signature would be valid it would means that validator (or someone possesing validator bls private key) signed invalid last commit hash, which could be evidence of misbehaviour.