Assessment reportsPublic findings
Back to Zellic site
↗
Assessment reports>Barretenberg Bigfield>Discussion>Misleading names of functions or variables
GeneralOverview
Findings
Critical (7)
Medium (2)
Low (6)
Informational (2)
DiscussionMaturity of the codebase and lack of specificationsConcrete examples for assumptions for bounds and how to document themUndocumented assumption regarding meaning of bigfield fieldsFunctioning of evaluate_non_native_field_multiplicationAssumptions regarding bit width of the modulusBehavior in simulator modeHandling of too large ranges in uint256_t::sliceMissing maximum-bit number checks in constructorFix for decompose_into_bitsOverflow checks for unreduced elementsPrime limb witness indexes equality check in operator- and operator+Inefficient loopsPossible shaper boundsUnnecessary assert in mul_product_overflows_crt_modulusUnnecessary reductons in operator+Unreachable codeMisleading names of functions or variablesImprovements for comments/documentation
Audit ResultsAssessment Results

Misleading names of functions or variables

Some functions or variables have names suggesting a behavior that differs from the actual implementation.

Function from_witness

The function from_witness is implemented as follows:

static bigfield from_witness(Builder* ctx, const bb::field<T>& input)
{
    uint256_t input_u256(input);
    field_t<Builder> low(witness_t<Builder>(ctx, bb::fr(input_u256.slice(0, NUM_LIMB_BITS * 2))));
    field_t<Builder> hi(witness_t<Builder>(ctx, bb::fr(input_u256.slice(NUM_LIMB_BITS * 2, NUM_LIMB_BITS * 4))));
    return bigfield(low, hi);
}

As can be seen, it does not take a witness as argument, but instead it constructs one from a field element, making the function name confusing.

Variables in bigfield constructor taking a byte array

In the constructor bigfield<Builder, T>::bigfield(const byte_array<Builder>& bytes), the naming scheme

const field_t<Builder> hi_8_bytes(bytes.slice(0, 6));
const field_t<Builder> mid_split_byte(bytes.slice(6, 1));
const field_t<Builder> mid_8_bytes(bytes.slice(7, 8));

const field_t<Builder> lo_8_bytes(bytes.slice(15, 8));
const field_t<Builder> lo_split_byte(bytes.slice(23, 1));
const field_t<Builder> lolo_8_bytes(bytes.slice(24, 8));

const auto [limb0, limb1] = reconstruct_two_limbs(ctx, lo_8_bytes, lolo_8_bytes, lo_split_byte);
const auto [limb2, limb3] = reconstruct_two_limbs(ctx, hi_8_bytes, mid_8_bytes, mid_split_byte);

is inconsistent, making it confusing. For example high_high, high_mid, high_low, low_high, low_mid, and low_low would be more consistent.

Switched names negative_prime_modulus_mod_binary_basis and negative_prime_modulus

In bigfield.hpp, negative_prime_modulus_mod_binary_basis and negative_prime_modulus are defined as follows:

static constexpr bb::fr negative_prime_modulus_mod_binary_basis = -bb::fr(uint256_t(modulus_u512));
static constexpr uint512_t negative_prime_modulus = binary_basis.modulus - target_basis.modulus;

The names would fit much better with the value of the other of the two; in particular, the binary basis is uninvolved in the value negative_prime_modulus_mod_binary_basis. The two names appear to have been mistakenly switched.

Zellic © 2025Back to top ↑