TangibleTNFT / baskets-foundry

baskets-foundry
1 stars 0 forks source link

[BMR-06M] Bypass of Basket Uniqueness Invariant #28

Closed chasebrownn closed 7 months ago

chasebrownn commented 7 months ago

BMR-06M: Bypass of Basket Uniqueness Invariant

Type Severity Location
Input Sanitization BasketManager.sol:L180, L183

Description:

The BasketManager::deployBasket presently ensures that each basket is uniquely attached to a combination of a Tangible NFT type, an ISO location, and the features it supports.

This uniqueness security trait can be breached by introducing duplicate entries in the _features array, leading to the generation of a different hash but a functionally equivalent Basket.

Impact:

Presently, any Basket deployed with _features of a length less than the featureLimit can be re-deployed with the exact same functional configuration by duplicating any of its features in the _features array breaching a significant invariant of the BasketManager.

Example:

// create unique hash for new basket
hashCache = createHash(_tnftType, _location, _features);

// might not be necessary -> hash is checked when Basket is initialized
require(checkBasketAvailability(hashCache), "Basket already exists");

Recommendation:

We advise the code to prevent duplicate entries in the _features array, ensuring that the hash uniqueness trait is properly upheld by the contract.

chasebrownn commented 7 months ago

This is fixed. We've decided to kill 2 birds with 1 stone. The _features array will not be sorted before calling deployBasket. Upon execution we will then verify the array is sorted and not containing any duplicates. Resolves this issue and https://github.com/TangibleTNFT/baskets-foundry/issues/51