c4-bot-5 commented 3 months ago

Lines of code

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

Vulnerability details

Impact

function get_side_effects() uses unbounded decoding of pink events within the ContractEvent::ContractEmitted match arm. Incase, Pink events are highly nested, the decoding process can recursively consume stack space until it exhausts the available stack, resulting in a stack overflow.

Proof of Concept

vulnerable part of the function :

if event.topics.len() == 1
    && event.topics[0].0 == pink::PinkEvent::event_topic()
{   
@>    match pink::PinkEvent::decode(&mut &data[..]) {
        Ok(event) => {
            pink_events.push((address.clone(), event.clone()));
            is_private_event = event.is_private();
        }
        Err(_) => {
            error!("Contract emitted an invalid pink event");
        }
    }
}

The problem is when function attempts to decode Pink events from the data, it does so without any limit on the depth of the decoding process. This means that if the Pink events within the data are structured in a highly nested manneer the decoding process will recursively traverse through each nested level.

Step by Step details how it can happen

Recursive call to decode pink event consume stack space
Nesting is deep enough or there is large number of pink events
space requires for recursive calls exceeded the available stack size
space is exhausted, no further recursive calls are made, results in stack overflow

Tools Used

Manual Review

Recommended Mitigation Steps

A recommended solutions is to set a depth limit for decoding pink events. decode_with_depth_limit method should be used instead of decode to ensure the decoding process doesn't exceed a certain depth and prevent stack overflow. Mitigationn can be done as follows :

if event.topics.len() == 1
    && event.topics[0].0 == pink::PinkEvent::event_topic()
{   
-    match pink::PinkEvent::decode(&mut &data[..]) {
+    match pink::PinkEvent::decode_with_depth_limit(&mut &data[..], MAX_DEPTH) {
        Ok(event) => {
            pink_events.push((address.clone(), event.clone()));
            is_private_event = event.is_private();
        }
        Err(_) => {
            error!("Contract emitted an invalid pink event");
        }
    }
}

would look upon judges and sponsor to Adjust the MAX_DEPTH constant based on complexity of Pink events.