**Audit Commit:** [a63b6996](https://github.com/MystenLabs/fastcrypto/tree/a63b6996298d78184cf6573de629d46553f66bae)

**Audited Files:**
* [fastcrypto/src/groups/bls12381.rs](https://github.com/MystenLabs/fastcrypto/blob/a63b6996298d78184cf6573de629d46553f66bae/fastcrypto/src/groups/bls12381.rs)

**Medium**
1. `GTElement::from_byte_array` allows for multiple byte representations to desearialize to the same element. This is caused due to the use of `blst_fp_from_bendian` which performs a `mod` reduction for byte array with value greater than `p`. This can lead to undefined behaviour for the higher level usage of the library which expects unique byte representation.
*(TODO: provide a concrete example and fix)*

**Informational**
1. Currently the `mul` implementation for `GTElement` performs all the scalar arithematic in `blst_fr`. This increases the complexity of the implementation (requires several type conversions to `blst_scalar` and use unsafe functions). The implementation is simplified if the scalar arithematic is performed in `blst_scalar` instead. Below is a reference `mul` implementation using `blst_scalar`.

     fn mul(self, rhs: Scalar) -> Self {
        let mut n = blst_scalar::default();
        unsafe{
            blst_scalar_from_fr(&mut n, &rhs.0);
        }

        let bytes_len = size_in_bytes(&n);
        let bits_len = size_in_bits(&n, bytes_len);

        if bits_len == 0 {
            return Self::zero();
        } 
        if bits_len == 1 {
            return self;
        }

        let mut y: blst_fp12 = blst_fp12::default();
        let mut x = self.0;

        for i in 0..(bits_len - 1) {
            // Get the bit at the ith position.
            if n.b[i / 8] & (1 << (i % 8)) == 1 {
                y *= x;
            }
            unsafe {
                blst_fp12_sqr(&mut x, &x);
            }
        }
        y *= x;
        Self::from(y)
    }

2. `multi_scalar_mul` implementation for both `G1Element` and `G2Element` perform unsafe type conversion from `&[Self]` to `&[blst_p1]` or `&[blst_p2]` respectively. The lifetime of the output slice is not well defined and is inferred from its usage. It is advisable to tie the lifetime of the input slice to the output slice. A potential way to do this is by wrapping the unsafe component into a seperate function. The rust compiler implicitly ties the lifetime of the input params to the output for such function.

fn from_blst_p1_slice(points: &[G1Element]) -> &[blst_p1] {
    // SAFETY: the cast from `&[G1Element]` to `&[blst_p1]` is safe because 
    // G1Element is a transparent wrapper around blst_p1. The lifetime of
    // output slice is the same as the input slice.
    unsafe { std::slice::from_raw_parts(points.as_ptr() as *const blst_p1, points.len()) }
}

3. Add documentation for the usage of `blst_fr`. The blst codebase is pretty sparse, but HAC chapter 14 is a good option (esp 14.3.2 which provides the main motivation for using montgomery forms). The underlying functions are also shared with blst_fp with the main difference being that fp values are expected to be opaque (as few things operate directly on the coordinates) whilst blst_fr is used to represent scalars which are oftentimes used in computations.