Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: CandyMachine bitmask sizes are incorrect #915

Open
sushi-shi opened this issue Dec 14, 2022 · 0 comments
Open

[Bug]: CandyMachine bitmask sizes are incorrect #915

sushi-shi opened this issue Dec 14, 2022 · 0 comments
Labels

Comments

@sushi-shi
Copy link
Contributor

sushi-shi commented Dec 14, 2022

Which package is this bug report for?

candy-machine

Which Type of Package is this bug report for?

Rust Contract

Issue description

In initialize.rs bitmask size is calculated as:

let vec_start = CONFIG_ARRAY_START
    + 4
    + (candy_machine.data.items_available as usize) * CONFIG_LINE_SIZE;
let as_bytes = (candy_machine
    .data
    .items_available
    .checked_div(8)
    .ok_or(CandyError::NumericalOverflowError)? as u32)
    .to_le_bytes();
for i in 0..4 {
    data[vec_start + i] = as_bytes[i]
}

Which returns incorrect results for anything not divisible by 8.

But in the end it works out fine because the next 4 bytes are the size of the second bitmask (which is never even initialized) and only the first its byte is being overwritten by add_config_lines.rs:

let bit_mask_vec_start = CONFIG_ARRAY_START
    + 4
    + (candy_machine.data.items_available as usize) * CONFIG_LINE_SIZE
    + 4;

let mut new_count = current_count;
for i in 0..fixed_config_lines.len() {
    let position = (index as usize)
        .checked_add(i)
        .ok_or(CandyError::NumericalOverflowError)?;
    let my_position_in_vec = bit_mask_vec_start
        + position
            .checked_div(8)
            .ok_or(CandyError::NumericalOverflowError)?;
    let position_from_right = 7 - position
        .checked_rem(8)
        .ok_or(CandyError::NumericalOverflowError)?;
    let mask = u8::pow(2, position_from_right as u32);

    let old_value_in_vec = data[my_position_in_vec];
    data[my_position_in_vec] |= mask;
...

So that the second bitmask is left intact (because of the last +4)

pub fn get_good_index(
    arr: &mut RefMut<&mut [u8]>,
    items_available: usize,
    index: usize,
    pos: bool,
) -> Result<(usize, bool)> {
    let mut index_to_use = index;
    let mut taken = 1;
    let mut found = false;
    let bit_mask_vec_start = CONFIG_ARRAY_START
        + 4
        + (items_available) * CONFIG_LINE_SIZE
        + 4
        + items_available
            .checked_div(8)
            .ok_or(CandyError::NumericalOverflowError)?
        + 4;

    while taken > 0 && index_to_use < items_available {
...

So we get memory layout where only xx byte is written incorrectly:

| CM_STATE |  BM1_LEN  | BM1_DATA |  BM2_LEN  | BM2_DATA |
|          |  |  |  |  |          |xx|  |  |  |          |
                                    ^
                                    |
                                    +-- BM1_DATA

Incidentally, I am also unsure what this line is for in get_config_line:

    if arr[CONFIG_ARRAY_START + 4 + index_to_use * (CONFIG_LINE_SIZE)] == 1 {
        return err!(CandyError::CannotFindUsableConfigLine);
    }

It feels like it tried to check whether found index exists in the first bitmask, but from what I understand it should never fail, because the first byte it takes is length of a name: String (by Borsh specification).
And if so, then it is possible to mint an NFT without name and metadata fields. Just like so (check minted NFT metadata):
https://explorer.solana.com/tx/3pzh8K4HhzZdV1eyCmjubbpk5eFf7AjMirJjPfJuHemVd8CpPYkaM8wLuw8jLBy33GLMdUocSzCaajeSBCMcwxBo?cluster=devnet
This happens when I comment out code in sugar for uploading config lines and mint tokens afterwards.

Relevant log output

No response

Priority this issue should have

Low (slightly annoying)

@sushi-shi sushi-shi added the bug label Dec 14, 2022
@samuelvanderwaal samuelvanderwaal self-assigned this Dec 22, 2022
@samuelvanderwaal samuelvanderwaal removed their assignment Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants