near / near-sdk-contract-tools

Helpful functions and macros for developing smart contracts on NEAR Protocol.
https://crates.io/crates/near-sdk-contract-tools
Apache License 2.0
41 stars 12 forks source link

Unsafe storage keys due to potential collision #137

Closed frol closed 9 months ago

frol commented 9 months ago

It was reported by @hanakannzashi to https://github.com/near/near-sdk-rs/issues/1141, but I checked near-sdk-contract-tools and realized that it has the same issue:

Hard-coded Storage key is unsafe, this may lead to colliding key-values stored in collections if user has other collections and uses 0 storage key for something else. It can either fail or even go unnoticed if the schema is the same:

#[derive(BorshSerialize, BorshStorageKey)]
enum StorageKey {
    MyKey {
        account_hash: Vec<u8>
    },
    // other variants
}

https://github.com/near/near-sdk-contract-tools/blob/1efbd10059cb7472c80eac4b482c9d005d00af24/src/standard/nep171/mod.rs#L72-L75

encody commented 9 months ago

First and foremost, this is a valid concern. Thank you for the report.

Here is how near-sdk-contract-tools currently handles storage keys for most components (e.g. NEP-141):

  1. A default storage key prefix is defined here. In the case of NEP-141, this default prefix is "~$141".
  2. All storage entries managed by a component use that prefix to construct their keys.
  3. All components allow the user to override the default storage prefix. Here is a test that demonstrates that functionality.

With that being said, there are not currently any safeguards that prevent (or even warn a user against) duplicating/overloading a storage key. That seems like a difficult problem to solve, though, without creating a complex compile-time key management system and prohibiting the use of env::storage_* methods.

frol commented 9 months ago

Ah, I see, near-sdk-contract-tools deals with the storage keys much better than near-contract-standards. Given that the chances are that there will be a collision with ~$141 prefix are much lower than a simple enum-index key, I believe this issue can be closed as resolving it, as was mentioned above, would require some generic solution on near-skd-rs side.