Audit Commit: **c961a01**
### Medium:
1. src/groups/secp256r1.rs L98: `rand` doesn't generate `scalar` in a uniformly random way. The mod operation (`from_be_bytes_mod_order`) wraps around skewing the uniform bytes. Using more uniform bytes would reduce the bias.
2. src/groups/secp256r1.rs L112: `from_byte_array` uses mod order (`from_le_bytes_mod_order`) to convert from bytes to scalar allowing for multiple byte representation for small scalars.
3. src/secp256r1/recoverable.rs L167:`sign_recoverable_with_hash` doesn't implement code path to generate signatures with id 2 and 3.
4. src/secp256r1/mod.rs L308: `drop` implementation is incorrect and leaks private keys on the stack.
5. src/secp256r1/conversion.rs L32: `fq_arkworks_to_p256` converts `ark_secp256r1::Fq` to `p256::Scalar` where as the order of the groups are different. This is not expected. Use `p256::FieldElement`.
### Low:
6. src/groups/multiplier/windowed.rs L56: `new` doesn't handle the base case when the cache size is 1 which is a valid power of 2.
7. src/groups/multiplier/windowed.rs L95, L59: If `CACHE_SIZE` is not exact power of 2 this leads to undefined behaviour. Check that `CACHE_SIZE` is exact power of 2.
8. src/groups/multiplier/windowed.rs L65: `mul` is not constant time as the `arkwork` group [addition implementation](https://github.com/arkworks-rs/algebra/blob/c015ea331674368461ff466bc7cbc69806f61628/ec/src/models/short_weierstrass/group.rs#L341) is not constant time.
9. src/groups/multiplier/windowed.rs L213: `compute_multiples` returns incorrect result when `window_size` is 1 (which is stricly positive as checked by the assert statement).
10. src/groups/multiplier/integer_utils.rs L31: Overflow can cause arbitrary read of byte outside the given range. Restrict `end - start` to be strictly less than 32.
11. src/groups/multiplier/integer_utils.rs L37: Addition of `numerator` and `denominator` creates an overflow if either of the values are big enough.
12. src/secp256r1/recoverable.rs L71: `from_bytes` should check that `recovery_id` is in the correct range.
13. src/secp256r1/mod.rs L301: The `zeroize` function is useless as the `drop` is implemented which will always be called. Either make the `drop` call the `zeroize` function or just remove the implementation.
14. src/secp256r1/mod.rs L402: The `H::digest(msg).digest` passed to `rfc6979::generate_k` function should be reduced modulo the group order as required by the function documentation and RFC.
### Information:
15. src/secp256r1/mod.rs L122, L128: Can use `partial_cmp` and `cmp` of the underlying library to avoid inconsistency with `eq` and avoid additional `as_ref`.
16. src/secp256r1/mod.rs L153, L159: Can use `<ProjectivePoint as GroupElement>::ScalarType` instead of `crate::groups::secp256r1::Scalar` for better code consistency.
17. src/secp256r1/mod.rs L229, L296, L348, src/secp256r1/recoverable.rs L92: Replace `get_or_try_init` to `get_or_init`.
18. src/secp256r1/conversion.rs L40: `affine_pt_p256_to_arkworks` can be refactored to return a `ark_secp256r1::Projective` as all of its usages convert returned the `ark_secp256r1::Affine` point to `ark_secp256r1::Projective`. This will reduce type conversions in the core logic.
### General Comments:
19. Private key bytes might be leaked in the stack after use by the code segments other than the above mentioned `drop` issue.