An attacker can easily crash the network with a call-bomb, as the memory cost is underestimated

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-03-phala-network/blob/main/phala-blockchain/crates/pink/runtime/src/runtime.rs#L114 https://github.com/code-423n4/2024-03-phala-network/blob/main/phala-blockchain/crates/pink/runtime/src/capi/ecall_impl.rs#L136-L142

Vulnerability details

Impact

The formula used to calculate the maximum allowed memory when calling a contract is wrong, and it can be abused by an attacker to cause an Out of Memory (OOM) error that will crash the node, causing a DoS, impacting the entire chain.

Proof of Concept

The current value of MAX_CODE_LEN is the following:

    const MAX_CODE_LEN: u32 = 2 * 1024 * 1024;

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

There is a formula used to calculate the maximum amount of memory that is not too large to avoid an OOM, which is based on a formula found on substrate tests:

    /*
    According to the cost estimation in ink tests: https://github.com/paritytech/substrate/pull/12993/files#diff-70e9723e9db62816e35f6f885b6770a8449c75a6c2733e9fa7a245fe52c4656cR423
    If we set max code len to 2MB, the max memory cost for a single call stack would be calculated as:
    cost = (MaxCodeLen * 4 + MAX_STACK_SIZE + max_heap_size) * max_call_depth
         = (2MB * 4 + 1MB + 4MB) * 6
         = 78MB
    If we allow 8 concurrent calls, the total memory cost would be 78MB * 8 = 624MB.
    */

https://github.com/code-423n4/2024-03-phala-network/blob/main/phala-blockchain/crates/pink/runtime/src/capi/ecall_impl.rs#L136-L142

Let's write this formula as $f(cost) = y$:

$cost = (MaxCodeLen * 4 + MaxStackSize + MaxHeapSize) * MaxCallDepth$

The issue is that this formula is different from the formula used in substrate tests:

    // This gives us the following formula:
    //
    // `(MaxCodeLen * 18 * 4 + MAX_STACK_SIZE + max_heap_size) * max_call_depth <
    // MAX_RUNTIME_MEM/2`
    //
    // Hence the upper limit for the `MaxCodeLen` can be defined as follows:
    let code_len_limit = MAX_RUNTIME_MEM
        .saturating_div(2)
        .saturating_div(max_call_depth)
        .saturating_sub(max_heap_size)
        .saturating_sub(MAX_STACK_SIZE)
        .saturating_div(18 * 4);

The full expanded formula from the previous code snippet is the following:

$CodeLenLimit = (\frac{MaxRuntimeMem}{2 * MaxCallDepth} - MaxStackSize - MaxHeapSize) * \frac{1}{18 * 4}$

Let's transformate the formula so that we have $f(MaxRuntimeMem) = y$:

$MaxRuntimeMem = (CodeLenLimit * 18 * 4 + MaxStackSize + MaxHeapSize) * (2 * MaxCallDepth)$

If we use this formula with the same numbers used in the pink runtime comment, we have:

cost = (2MB * 18 * 4 + 1MB + 4MB) * (2 * 6) = 149 MB * 12 = 1788 MB = ~1.8 GB

If up to 8 concurrent calls are allowed, this would increase to ~14.4 GB, but the maximum estimation is only 624 MB.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adjusting the cost formula so that it's the same as the one found on substrate tests:

$CodeLenLimit = (\frac{MaxRuntimeMem}{2 * MaxCallDepth} - MaxStackSize - MaxHeapSize) * \frac{1}{18 * 4}$

If the maximum memory cost estimation of a single call is 78 MB, then the MaxCodeLen should be calculated as follows:

$CodeLenLimit = (\frac{78MB}{2 * 6} - 1MB - 1MB) * \frac{1}{18 * 4} = \frac{4.5MB}{18 * 4} = 0.0625MB = 62500B$

-    const MAX_CODE_LEN: u32 = 2 * 1024 * 1024;
+    //@audit this is in bytes, so the theoretical maximum amount <= 62500 is 61.03515625 * 1024
+    const MAX_CODE_LEN: u32 = 61 * 1024;

Assessed type

Math

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[141345]

inconsistent formula used

[kvinwang]

The comments in the substrate explains why it use MaxCodeLen*18*4:

// In worst case, the decoded wasm contract code would be `x16` times larger than the
// encoded one. This is because even a single-byte wasm instruction has 16-byte size in
// wasmi. This gives us `MaxCodeLen*16` safety margin.

In pink-runtime, we reject the code expansion larger than 4x to be uploaded: https://github.com/code-423n4/2024-03-phala-network/blob/main/phala-blockchain/crates/pink/runtime/src/capi/ecall_impl.rs#L145-L146

// Next, the pallet keeps both the original and instrumented wasm blobs for each
// contract, hence we add up `MaxCodeLen*2` more to the safety margin.

pallet-contract no longer store an extra instrumented wasm blob since version polkadot-1.0.0.

// Finally, the inefficiencies of the freeing-bump allocator
// being used in the client for the runtime memory allocations, could lead to possible
// memory allocations for contract code grow up to `x4` times in some extreme cases,
// which gives us total multiplier of `18*4` for `MaxCodeLen`.

pink-runtime don't use the freeing-bump allocator which doesn't free any memory at all.

[c4-sponsor]

kvinwang (sponsor) disputed

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid