Investigation: Possibility of generating non-normalized signatures in sign function.

[netrome]

Context

Currently we have an assertion in the encode_signature function in fuel-crypto/src/secp256/signature_format.rs. However, this assertion is frequently failing during fuzz tests, as observed in #733.

The assertion fails when the encode_signature function receives a non-normalized signature. The signature is produces in the sign function in fuel-crypto/src/secp256/backend/k1/secp256k1.rs.

pub fn sign(secret: &SecretKey, message: &Message) -> [u8; 64] {
    let signature = CONTEXT.sign_ecdsa_recoverable(&message.into(), &secret.into());
    let (recovery_id, signature) = signature.serialize_compact();

    // encode_signature cannot panic as we don't generate reduced-x recovery ids.
    let recovery_id = SecpRecoveryId::try_from(recovery_id)
        .expect("reduced-x recovery ids are never generated");
    encode_signature(signature, recovery_id)
}

It is not obvious that we guarantee that the signature creation here always return normalized signatures. We should further investigate if this is a guaranteed invariant from the rest of the code, and take appropriate action based on the findings.

Definition of done

We have clarity on whether we can expect the sign function to always return normalized signatures in production, and based on the findings we have either a follow-up task defined to fix this, or the code clearly maintains this invariant so that it is not encountered in fuzz tests.

[netrome]

This also seems to be the cause of the validity errors in fuel-tx/src/transaction/validity.rs encountered during fuzzing, as the address recovery seems to assume that the signature was normalized.

[netrome]

Found the culprit. While our code is correct, and secp256k1 doesn't generate non-normalized signatures normally, using --cfg=fuzzing disables any cryptography in the current version of secp256k1 which breaks our assumptions during fuzz testing. In newer versions they have changed this to be togglable using the specific --cfg=secp256k1_fuzz flag instead.

See the rust-secp256k1 readme for more details.

The fix is just as simple as a version bump of secp256k1. I'll do that directly in the https://github.com/FuelLabs/fuel-vm/pull/733 PR.