🛡️ Inadequate Input Validation in Objectarium Contract Instantiation

[ccamel]

Note

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

Description

The instantiation process in the Objectarium contract lacks comprehensive input validation, specifically for the parameters associated with bucket configuration and limits. This deficiency may lead to configurations that render the contract functionally ineffective or vulnerable to misuse. The try_new method in the Bucket class currently only checks for an empty bucket name, overlooking critical validations on the numerical limits set for the bucket.

Recommendation

Enhance the validation logic within the Bucket::try_new method to include checks on all parameters.

[bdeneux]

🔎 Analysis

The current Bucket::try_new method only verifies if the bucket name is not empty.

In addition to this, we leverage the implicit validation provided by serde when decoding each JSON attribute to its corresponding type. For example, UInt128 fails validation if the user provides an input such as:

• { "max_objects": "9999999999999999999999999999999999999999999999999999999999999"} -> invalid Uint128 '9999999999999999999999999999999999999999999999999999999999999' - number too large to fit in target type
• { "max_objects": "-1"} -> invalid Uint128 '-1' - invalid digit found in string:

We also utilize JSON and serde validation for u32 inputs in PaginationConfig, and for enumerations HashAlgorithm and CompressionAlgorithm in BucketConfig.

📈 Suggested Improvement

The default serde validation provides basic checks related to the corresponding JSON attribute type, but it does not perform functional checks based on the smart contract usage.

For the BucketLimit object, I don't see any additional checks that could be added, since setting a 0 value is considered as no limit (default value).
However, we could potentially add checks for the coherence of all settings :

• max_total_size should be equal to or greater than (max_object_size * max_object) (only if max_object > 0, otherwise : max_total_size >= max_object_size

While these checks are not mandatory for the integrity and security of the chain, they could help users avoid misconfiguration of axone-objectarium. Let me know if you see other potential checks.

For BucketConfig and PaginationConfig, I don't see any other possible checks.

@ccamel, @amimart

[amimart]

For the BucketLimit object, I don't see any additional checks that could be added, since setting a 0 value is considered as no limit (default value).

@bdeneux Are you sure about the 0 considered as no limit?

Here are all the possible checks I identified, let's discuss on them and converge to a consensus 😉

PaginationConfig

• max_page_size >= default_page_size
• default_page_size > 0

BucketConfig

• accepted_compression_algorithms not empty

BucketLimits

• max_objects > 0
• max_object_size > 0
• max_total_size >= max_object_size

I have no strict opinion about the max_object_pins, being set to 0 allow anyone do remove any objects and I don't see in which case that would be intended.

[bdeneux]

For the BucketLimit object, I don't see any additional checks that could be added, since setting a 0 value is considered as no limit (default value).

@bdeneux Are you sure about the 0 considered as no limit?

@amimart Sorry, you're right, according to the documentation it's not the zero value that indicate no limit but omitted value.

contracts/contracts/axone-objectarium/src/msg.rs

Line 300 in 9d0958d

/// The limits are optional and if not set, there is no limit.

PaginationConfig

✅ Ok with your propositions

BucketConfig

We need to check the behavior but according to the documentation, setting empty array is the same as omit this setting parameter (It's mean that all compressions algorithms are allowed) so is not necessary to check this configuration, except if we want to change the behavior and says that setting empty accepted_compression_algorithm is an incorrect configuration.

contracts/contracts/axone-objectarium/src/msg.rs

Lines 268 to 280 in 9d0958d

	/// The acceptable compression algorithms for the objects in the bucket.
	/// If this parameter is not set (none or empty array), then all compression algorithms are accepted.
	/// If this parameter is set, then only the compression algorithms in the array are accepted.
	///
	/// When an object is stored in the bucket without a specified compression algorithm, the first
	/// algorithm in the array is used. Therefore, the order of the algorithms in the array is significant.
	/// Typically, the most efficient compression algorithm, such as the NoCompression algorithm, should
	/// be placed first in the array.
	///
	/// Any attempt to store an object using a different compression algorithm than the ones specified
	/// here will fail.
	#[serde(default = "CompressionAlgorithm::values")]
	pub accepted_compression_algorithms: Vec<CompressionAlgorithm>,

BucketLimits

✅ Ok for those configurations checks. For the max_objects_pins, there is no interest being set to 0 but it's possible, I don't know too what is the best option 🤷.