🛡️ Insufficient information in Events

[ccamel]

Note

Severity: Critical
target: v5.0.0 - Commit: cde785fbd2dad71608d53f8524e0ef8c8f8178af
Ref: OKP4 CosmWasm Audit Report v1.0 - 02-05-2024 - BlockApex

Description

• Law Stone Contract: The break_stone function in the Law Stone contract fails to emit detailed events for actions undertaken within the function. Notably, actions such as unpinning or forgetting objects lack corresponding information in event emissions, which are crucial for auditing and tracing the state changes within the contract.
• Objectarium Contract: The functions within the Objectarium contract consistently emit events that only include the "action" and "id" parameters. This results in the omission of critical information such as pin counts, the pin status of objects, and the remaining pins after an object is forgotten. This lack of detail in event logs hampers effective monitoring and debugging of the
  system.

Recommendation

• For the Law Stone Contract: Enhance the break_stone function to include detailed event emissions for all significant actions, particularly for unpinning and forgetting operations. Each event should detail the affected objects and the resultant state changes.
• For the Objectarium Contract: Modify event emissions to include additional data such as pin counts, the current pin status of each object, and updates following any "forget" operations. This improvement will enable better insight into the contract's operation and facilitate data tracking and integrity checks.

[bdeneux]

This comment initiates the process of determining the necessary information to return for each execute message in the Objectarium. As the event result from the Objectarium is automatically transferred when invoked from the Law stone contract, it's crucial to start with the Objectarium.

Adding event information is crucial for data tracking and integrity checks. However, it's important to avoid including excessive or redundant information, as this can lead to increased data storage costs. By redundant information, we mean data that can be easily inferred from the input. For example, when storing an object with a specified compression algorithm, it's unnecessary to return the used compression algorithm as an event output, since this information is already known.

So for the next purpose, in addition to the standard attributes (action and object_id), I plan to include only those values that are not easily predictable or value that have been updated as a result of the message execution.

💾 store_object

The current event attributes returned is :

{ 
    "action": "store_object",
    "id": <object_id>
}

Adding information like pin_count and compression is not necessary because this information can be easily determined in advance through the input. I suggest adding the compressed_size and size attributes (the latest is also determinable but indirectly).

Storing an object results in updating the bucket statistics for these three elements: size, compressed_size, and object_count. It could be interesting to add this information to the event result. Here is the final result:

{ 
    "action": "store_object",
    "id": <object_id>,
    "size": <size>,
    "compressed_size": <compressed_size>,
    "bucket_stat": {
        "size": <total_bucket_size>,
        "compressed_size": <total_compresse_size>,
        "object_count": <total_object_count>
    }
}

📌 pin_object

The current event attributes returned is :

{ 
    "action": "pin_object",
    "id": <object_id>
}

In the result of a pin_object action, a new number of pins is populated for the given object.

The proposed event result is:

{ 
    "action": "pin_object",
    "id": <object_id>,
    "pin_count": <new_pin_count>
}

📌 unpin_object

The current event attributes returned is :

{ 
    "action": "unpin_object",
    "id": <object_id>
}

Following the pin_object message, only pin_count is updated here:

{ 
    "action": "unpin_object",
    "id": <object_id>,
    "pin_count": <new_pin_count>
}

🗑 forget_object

The current event attributes returned is :

{ 
    "action": "forget_object",
    "id": <object_id>
}

When forgetting an object, if no error occurs, only bucket stat information is updated (in addition to the object deletion). Therefore, we need to return the new bucket stats:

{ 
    "action": "forget_object",
    "id": <object_id>,
    "bucket_stat": {
        "size": <total_bucket_size>,
        "compressed_size": <total_compresse_size>,
        "object_count": <total_object_count>
    }
}

Next step

I will write another comment for the law_stone event attributes, but adding these will enhance the law_stone since it acts on the Objectarium result.

@amimart @ccamel

[amimart]

Many thanks @bdeneux for such wide analysis! It helps to imagine all the possibilities we have here.

It's clear we can make the events carry a lot of information, we also need to avoid flooding events because this has a cost on the long term. That's why I'd tend to avoid exposing information that could be recomposed by tracking all the past events, for instance pin_count information could be omitted if you track all the pinning actions.

Following this I think the only message that should expose more information is the store_object, here's what I propose:

{ 
    "action": "store_object",
    "id": <object_id>,
    "pinned": <pinned>,
    "size": <size>,
    "compressed_size": <compressed_size>
}

It's worth mentioning that it's authorized to store an already stored object, and in that case we may need to expose different values for those events..

Regarding the law stone break_stone message I don't see anything to add because as you said we take benefit of the objectarium events.

Let me know what you think :)

[bdeneux]

@amimart I agree with the proposed changes. Let's proceed with adding the following attributes to the store_object response:

{ 
    "action": "store_object",
    "id": <object_id>,
    "pinned": <pinned>,
    "size": <size>,
    "compressed_size": <compressed_size>
}

However, we need to carefully consider the scenario where an object already exists in the storage. In my opinion, it would be beneficial to include the pinned attribute in this case, as the only possible action is to pin the object.

Additionally, while working on this implementation, I noticed an inconsistency when the compression algorithm of the existing object differs from the one being stored (refer to issue #583). Depending on the resolution of this issue, the data returned in the event might need to be adjusted (for example, the new compressed size). Therefore, I suggest we first decide on the approach for handling differing compression algorithms before finalizing the attributes to be returned in the event.

[ccamel]

Sounds like a plan. 🚀

[amimart]

However, we need to carefully consider the scenario where an object already exists in the storage. In my opinion, it would be beneficial to include the pinned attribute in this case, as the only possible action is to pin the object.

I agree, we could set pinned: true if not already pinned IMHO