🛡️ Insufficient Error Handling in Object Access Within Objectarium

[ccamel]

Note

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

Description

The pin_object function in the Objectarium contract lacks preemptive checks to confirm the existence of an object before attempting operations on it. This oversight leads to attempts to manipulate non-existent objects, resulting in generic errors which are not descriptive of the actual issue. This issue is also evident in similar operations, such as forget_object, where object existence is assumed rather than verified before proceeding with further logic.

Recommendation

Implement and enforce existence checks before performing any operations on objects within the contract.

[bdeneux]

🔎 Analysis

📌 pin_object

The current implementation of the pin_object message already includes implicit error handling for non-existing or invalid object_id. This is managed through storage querying, and the unit tests already verify this behavior. When an error occurs, it returns a StdError::not_found error through ContractError::Std.

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

Line 173 in 2ed6cdc

let object = objects().load(deps.storage, id.clone())?;

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

Lines 1508 to 1526 in 2ed6cdc

	TC {
	// Object not exists
	objects: vec![ObjectId::from(
	"abafa4428bdc8c34dae28bbc17303a62175f274edf59757b3e9898215a428a56",
	)],
	senders: vec![mock_info("bob", &[])],
	expected_count: 0,
	expected_error: Some(ContractError::Std(StdError::not_found(
	not_found_object_info::<Object>(
	"abafa4428bdc8c34dae28bbc17303a62175f274edf59757b3e9898215a428a56",
	),
	))),
	expected_object_pin_count: vec![(
	ObjectId::from(
	"315d0d9ab12c5f8884100055f79de50b72db4bd2c9bfd3df049d89640fed1fa6",
	),
	Uint128::zero(),
	)],
	},

📌 unpin_object

The unpin_object message follows a similar pattern, with error handling for non-existing or invalid object_id.

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

Lines 1773 to 1795 in 2ed6cdc

	TC {
	// Object not exists
	pin: vec![ObjectId::from(
	"315d0d9ab12c5f8884100055f79de50b72db4bd2c9bfd3df049d89640fed1fa6",
	)],
	pin_senders: vec![mock_info("bob", &[])],
	unpin: vec![ObjectId::from(
	"abafa4428bdc8c34dae28bbc17303a62175f274edf59757b3e9898215a428a56",
	)],
	unpin_senders: vec![mock_info("martin", &[])],
	expected_count: 1,
	expected_error: Some(ContractError::Std(StdError::not_found(
	not_found_object_info::<Object>(
	"abafa4428bdc8c34dae28bbc17303a62175f274edf59757b3e9898215a428a56",
	),
	))),
	expected_object_pin_count: vec![(
	ObjectId::from(
	"315d0d9ab12c5f8884100055f79de50b72db4bd2c9bfd3df049d89640fed1fa6",
	),
	Uint128::one(),
	)],
	},

🗑 forget_object

The forget_object message also includes error handling for non-existing or invalid object_id.

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

Lines 2278 to 2299 in 2ed6cdc

	TC {
	pins: vec![
	ObjectId::from(
	"315d0d9ab12c5f8884100055f79de50b72db4bd2c9bfd3df049d89640fed1fa6",
	),
	ObjectId::from(
	"315d0d9ab12c5f8884100055f79de50b72db4bd2c9bfd3df049d89640fed1fa6",
	),
	],
	pins_senders: vec![mock_info("bob", &[]), mock_info("alice", &[])],
	forget_objects: vec![ObjectId::from(
	"abafa4428bdc8c34dae28bbc17303a62175f274edf59757b3e9898215a428a56",
	)],
	forget_senders: vec![mock_info("bob", &[])], // the sender is the same as the pinner, but another pinner is on it so error
	expected_count: 3,
	expected_total_size: Uint128::new(13),
	expected_error: Some(ContractError::Std(StdError::not_found(
	not_found_object_info::<Object>(
	"abafa4428bdc8c34dae28bbc17303a62175f274edf59757b3e9898215a428a56",
	),
	))),
	},

📈 Suggested Improvement

To enhance the clarity of error messages, we could consider replacing the generic Std error with a custom contract error, similar to ContractError::ObjectPinned. For instance, we could introduce ContractError::ObjectNotFound to handle cases where the object_id does not exist.

@ccamel, @amimart

[amimart]

To enhance the clarity of error messages, we could consider replacing the generic Std error with a custom contract error, similar to ContractError::ObjectPinned. For instance, we could introduce ContractError::ObjectNotFound to handle cases where the object_id does not exist.

In my sense I'd rather use existing well known types if they suit, and the StdError::NotFound carry the concerned type so providing a specific ContractError::ObjectNotFound doesn't bring more value I think.

Actually I don't see this issue relevant because the checks and the errors are already here and pretty expressive, I don't see how we can do better.. What do you think? I'd be in favour to close this one

[ccamel]

@amimart I share your point of view. The standard StdError::NotFound exception conveys a sufficient level of information given the context in which it is used. I'd also be in favour of closing the ticket.

[amimart]

Ok let's closing it :)