code-423n4 / 2024-03-phala-network-findings

0 stars 0 forks source link

`MB's` value is incorrect #33

Closed c4-bot-5 closed 6 months ago

c4-bot-5 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

In runtime.rs#L125, the MB should be 64KiB * 16, but it's defined as 16. Which causes schedule.limits.memory_pages much smaller than expected.

Proof of Concept

As shown in runtime.rs#L125 and runtime.rs#L128:

123     pub DefaultSchedule: Schedule<PinkRuntime> = {
124         let mut schedule = Schedule::<PinkRuntime>::default();
125         const MB: u32 = 16;  // 64KiB * 16 <<<--- Here
126         // Each concurrent query would create a VM instance to serve it. We couldn't
127         // allocate too much here.
128         schedule.limits.memory_pages = 4 * MB; <<<--- Here
129         schedule.instruction_weights.base = 8000;
130         schedule.limits.runtime_memory = 2048 * 1024 * 1024; // For unittests
131         schedule.limits.payload_len = 1024 * 1024; // Max size for storage value
132         schedule
133     };

Tools Used

Recommended Mitigation Steps

diff --git a/phala-blockchain/crates/pink/runtime/src/runtime.rs b/phala-blockchain/crates/pink/runtime/src/runtime.rs
index d3bed97..a59ebaa 100644
--- a/phala-blockchain/crates/pink/runtime/src/runtime.rs
+++ b/phala-blockchain/crates/pink/runtime/src/runtime.rs
@@ -122,7 +122,7 @@ parameter_types! {
     pub const MaxDebugBufferLen: u32 = 128 * 1024;
     pub DefaultSchedule: Schedule<PinkRuntime> = {
         let mut schedule = Schedule::<PinkRuntime>::default();
-        const MB: u32 = 16;  // 64KiB * 16
+        const MB: u32 = 1024*1024;  // 64KiB * 16
         // Each concurrent query would create a VM instance to serve it. We couldn't
         // allocate too much here.
         schedule.limits.memory_pages = 4 * MB;

Assessed type

Other

c4-pre-sort commented 6 months ago

141345 marked the issue as primary issue

c4-pre-sort commented 6 months ago

141345 marked the issue as sufficient quality report

141345 commented 6 months ago

set the wrong const value, 64KiB * 16 as 16

kvinwang commented 6 months ago
125         const MB: u32 = 16;  <<<--- the uint is pages
128         schedule.limits.memory_pages = 4 * MB; <<<--- assigned to memory_pages

A wasm page is of 64KiB.

c4-sponsor commented 6 months ago

kvinwang (sponsor) disputed

c4-judge commented 6 months ago

OpenCoreCH marked the issue as unsatisfactory: Invalid