ickb / v1-core

iCKB scripts and utilities for interacting with them
MIT License
2 stars 0 forks source link

load_cell_data issue #3

Closed XuJiandong closed 2 months ago

XuJiandong commented 2 months ago

The load_cell_data function can load all cell data at once. However, the cell data might be very large, potentially consuming all available memory in certain scenarios. It is recommended to use the following low-level syscall to load only a portion of the cell data:

https://github.com/nervosnetwork/ckb-std/blob/d4526dc41bc4bc1cb55b8bea0358ce4f24ead7b6/src/syscalls/native.rs#L396-L401

phroi commented 2 months ago

@XuJiandong this is a point valid for all projects, why is this not documented as a failure mode in ckb-std load_cell_data docs?

phroi commented 2 months ago

Running grep returns the following uses:

user@host:~/ickb/v1-core/scripts$ grep --recursive --line-number -i 'load_cell_data' contracts/
contracts/ickb_logic/src/utils.rs:3:use ckb_std::{ckb_constants::Source, high_level::load_cell_data};
contracts/ickb_logic/src/utils.rs:15:    let data = load_cell_data(index, source)?;
contracts/ickb_logic/src/utils.rs:43:    load_cell_data(index, source)
contracts/owned_owner/src/entry.rs:7:        load_cell_data, load_cell_lock_hash, load_cell_type_hash, load_script_hash, QueryIter,
contracts/owned_owner/src/entry.rs:71:    let owned_distance = load_cell_data(index, source)?;
contracts/utils/src/lib.rs:44:    let data = load_cell_data(index, source)?;
phroi commented 2 months ago

contracts/ickb_logic/src/utils.rs:15: let data = load_cell_data(index, source)?;

This calls happens in extract_receipt_data.

Right away after the call the data length is checked with:

    let data = load_cell_data(index, source)?;
    if data.len() != UNION_ID + DEPOSIT_QUANTITY + DEPOSIT_AMOUNT {
        return Err(Error::Encoding);
    }

Also using again grep on extract_receipt_data, the function that wraps the load_cell_data call, the calls to this function happens only in:

user@host:~/ickb/v1-core/scripts$ grep --recursive --line-number -i 'extract_receipt_data' contracts/
contracts/ickb_logic/src/utils.rs:14:pub fn extract_receipt_data(index: usize, source: Source) -> Result<(u32, u64), Error> {
contracts/ickb_logic/src/entry.rs:11:use crate::utils::extract_receipt_data;
contracts/ickb_logic/src/entry.rs:56:                let (deposit_quantity, deposit_amount) = extract_receipt_data(index, source)?;
contracts/ickb_logic/src/entry.rs:118:                let (deposit_quantity, deposit_amount) = extract_receipt_data(index, source)?;

These calls to extract_receipt_data happen only if the cell has already been verified as a Receipt.

Putting together that:

Then no memory exhaustion should occur.

If it does, it means that the Receipt is invalid and the cell likely malicious. At this point the validation will fail as it should. A generic error code for attacker is fine.

phroi commented 2 months ago

contracts/ickb_logic/src/utils.rs:43: load_cell_data(index, source)

This calls happens in is_deposit_cell.

Right away after the call the data content is checked with:

    load_cell_data(index, source)
        .map(|data| data.as_ref() == DAO_DEPOSIT_DATA)

Also using again grep on is_deposit_cell, the function that wraps the load_cell_data call, the call to this function happens only in:

contracts/ickb_logic/src/utils.rs:42:pub fn is_deposit_cell(index: usize, source: Source) -> bool {
contracts/ickb_logic/src/celltype.rs:19:    utils::is_deposit_cell,
contracts/ickb_logic/src/celltype.rs:104:            if is_deposit_cell(self.index, self.source) {

These calls to is_deposit_cell happen only if the cell has already been verified as a DAO cell.

Putting together that:

Then no memory exhaustion should occur.

If it does, it means that the DAO cell is invalid and likely malicious. At this point the validation will fail as it should. A generic error code for attacker is fine.

phroi commented 2 months ago

contracts/owned_owner/src/entry.rs:71: let owned_distance = load_cell_data(index, source)?;

This calls happens in extract_owned_metapoint.

Right away after the call the data content is checked with:

    let owned_distance = load_cell_data(index, source)?;
    if owned_distance.len() != 4 {
        return Err(Error::Encoding);
    }

Also using again grep on extract_owned_metapoint, the function that wraps the load_cell_data call, the call to this function happens only in:

user@host:~/ickb/v1-core/scripts$ grep --recursive --line-number -i 'extract_owned_metapoint' contracts/
contracts/owned_owner/src/entry.rs:36:                    let metapoint = extract_owned_metapoint(index, source)?;
contracts/owned_owner/src/entry.rs:69:fn extract_owned_metapoint(index: usize, source: Source) -> Result<MetaPoint, Error> {

This call to extract_owned_metapoint happens only if the cell has already been verified as an Owner cell.

Putting together that:

Then no memory exhaustion should occur.

If it does, it means that the Owner cell is invalid and likely malicious. At this point the validation will fail as it should. A generic error code for attacker is fine.

phroi commented 2 months ago

contracts/utils/src/lib.rs:44: let data = load_cell_data(index, source)?;

This calls happens in extract_udt_cell_data.

Using again grep on extract_udt_cell_data, the function that wraps the load_cell_data call, the call to this function happens only in:

user@host:~/ickb/v1-core/scripts$ grep --recursive --line-number -i 'extract_udt_cell_data' contracts/
contracts/limit_order/src/entry.rs:7:use utils::{extract_metapoint, extract_udt_cell_data, has_empty_args, MetaPoint};
contracts/limit_order/src/entry.rs:161:    let (udt_amount, order_raw_data) = extract_udt_cell_data(index, source)?;
contracts/ickb_logic/src/entry.rs:7:    extract_accumulated_rate, extract_udt_cell_data, extract_unused_capacity, has_empty_args,
contracts/ickb_logic/src/entry.rs:65:                total_udt_ickb += extract_udt_cell_data(index, source)?.0;
contracts/ickb_logic/src/entry.rs:131:                total_udt_ickb += extract_udt_cell_data(index, source)?.0;
contracts/utils/src/lib.rs:43:pub fn extract_udt_cell_data(index: usize, source: Source) -> Result<(u128, Vec<u8>), SysError> {

In some of these calls (with likely malicious cells) memory exhaustion could actually occur.

phroi commented 2 months ago

@XuJiandong, given that I need to address this last case, I'll switch to your suggestion. Since I'm modifying the code in this way, I'll also address the previous ones too.

Maybe I'll also relax the strict equality on Owner cells too.