Any function generated by `define_mask_fn!` maybe panic

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-03-phala-network/blob/main/phala-blockchain/crates/pink/runtime/src/contract.rs#L59

Vulnerability details

Impact

The blockchain maybe panic when use any function generated by define_mask_fn! with a special min_mask_bits

Proof of Concept

In the macro define_mask_fn!

macro_rules! define_mask_fn {
    ($name: ident, $bits: expr, $typ: ty) => {
        /// Mask given number's lowest bits.
        ///
        /// Given a number 0x1000beef, in binary representation:
        ///     0b_10000_00000000_10111110_11101111
        /// We want to mask it to 0x100fffff.
        /// Rough steps:
        ///     0b_10000_00000000_10111110_11101111
        ///        ^
        ///      1. we find the most left bit position here
        ///     0b_10000_00000000_10111110_11101111
        ///                  ^^^^^^^^^^^^^^^^^^^^^^
        ///               2. than calculate these bits need to be mask
        ///     0b_10000_00001111_11111111_11111111
        ///                  ^^^^^^^^^^^^^^^^^^^^^^
        ///               3. mask it
        fn $name(v: $typ, min_mask_bits: u32) -> $typ {
            // Given v = 0x1000_beef
            // Suppose we have:
            // bits = 64
            // v =0b010000_00000000_10111110_11101111
            let pos = $bits - v.leading_zeros();
            //    0b010000_00000000_10111110_11101111
            //      ^
            //     here, pos=30
            // shift right by 9
            let pos = pos.max(9) - 9;
            // Now pos =  0b_10000_00000000_00000000
            //               ^
            //              now here, pos=21
            // If min_mask_bits = 16
            //                  0b_10000000_00000000
            //                     ^
            //                  min_mask_bits here
            let pos = pos.clamp(min_mask_bits, $bits - 1);
            let cursor: $typ = 1 << pos;
            //            0b_10000_00000000_00000000
            //               ^
            //               cursor here
            let mask = cursor.saturating_sub(1);
            // mask =  0b_00001111_11111111_11111111
            // v | mask =
            //    0b10000_00001111_11111111_11111111
            //  = 0x100fffff
            v | mask
        }
    };
}

Focus on

let pos = pos.clamp(min_mask_bits, $bits - 1);

min_mask_bits is a args of the generated function, $bits is a args of the macro like 128, 64

But There is no check if min_mask_bits <= $bits - 1

According to the rust docs https://docs.rs/num/latest/num/fn.clamp.html, function clamp will panic if the left value is bigger than right value

for example

mask_low_bits64(64_u64, 64);

this will cause panic

in the code, we can see a function like make_deposit

https://github.com/code-423n4/2024-03-phala-network/blob/main/phala-blockchain/crates/pink/runtime/src/contract.rs#L77

fn mask_deposit(deposit: u128, deposit_per_byte: u128) -> u128 {
    const MIN_MASKED_BYTES: u128 = 256;
    let min_masked_value = deposit_per_byte
        .saturating_mul(MIN_MASKED_BYTES)
        .saturating_sub(1);
    println!("{}", min_masked_value);
    let min_mask_bits = 128 - min_masked_value.leading_zeros();
    println!("{}", min_mask_bits);
    mask_low_bits128(deposit, min_mask_bits)
}

we can panic it with

mask_deposit(0_u128, u128::MAX);

In fact, we can't control the min_mask_bits args in current codebase. But in future updates, if someone misuses it, it will cause serious problems

Tools Used

manual audit

Recommended Mitigation Steps

Requires min_mask_bits to be less than or equal to $bits - 1

Assessed type

DoS

[c4-pre-sort]

141345 marked the issue as duplicate of #86

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid