Final audit revision #90

taitruong · 2024-02-12T10:51:52Z

Changes:

pass nft contract, instead of overriding info.sender (commit #5059fc0e6831a6005fb7dcc0ee80295f46904984)
remove NFT_CONTRACT_TO_CLASS_ID and CLASS_ID_TO_NFT_CONTRACT and merge into CLASS_ID_AND_NFT_CONTRACT_INFO (commit #d6d63835ec03da75e8a45a36cf4c0401137f1492)

…e into CLASS_ID_AND_NFT_CONTRACT_INFO

…yet, also keep legacy store

taitruong · 2024-02-12T11:06:17Z

packages/ics721/src/execute.rs

+                // we migrate only in case CLASS_ID_AND_NFT_CONTRACT_INFO is not populated yet
+                // TODO once migrated:
+                // - this complete block can be deleted
+                // - legacy map 'e' and 'f' can be deleted
+                let is_empty = query_nft_contracts(deps.as_ref(), None, None)
+                    .map(|nft_contracts| nft_contracts.is_empty())?;
+                if is_empty {
+                    // - get legacy map and migrate it to new indexed map
+                    let legacy_nft_contract_to_class_id: Map<Addr, ClassId> = Map::new("f");
+                    match cw_paginate_storage::paginate_map(
+                        deps.as_ref(),
+                        &legacy_nft_contract_to_class_id,
+                        None,
+                        None,
+                        Order::Ascending,
+                    ) {
+                        Ok(nft_contract_and_class_id) => {
+                            let response = response.add_attribute(
+                                "migrated nft contracts",
+                                nft_contract_and_class_id.len().to_string(),
+                            );
+                            for (nft_contract, class_id) in nft_contract_and_class_id {
+                                let class_id_info = ClassIdInfo {
+                                    class_id: class_id.clone(),
+                                    address: nft_contract.clone(),
+                                };
+                                CLASS_ID_AND_NFT_CONTRACT_INFO.save(
+                                    deps.storage,
+                                    &class_id,
+                                    &class_id_info,
+                                )?;
+                            }
+                            Ok(response)
+                        }
+                        Err(err) => Err(ContractError::Std(err)),
+                    }
+                } else {
+                    Ok(response)
+                }


This is the migration logic for CLASS_ID_AND_NFT_CONTRACT_INFO

taitruong · 2024-02-12T11:11:14Z

packages/ics721/src/testing/contract.rs

+fn test_migrate() {
+    let mut deps = mock_dependencies();
+    let env = mock_env();
+    let info = mock_info(OWNER_ADDR, &[]);
+    let msg = instantiate_msg(None, None);
+    Ics721Contract {}
+        .instantiate(deps.as_mut(), env.clone(), info, msg.clone())
+        .unwrap();
+    let msg = MigrateMsg::WithUpdate {
+        pauser: Some("some_other_pauser".to_string()),
+        outgoing_proxy: Some("outgoing".to_string()),
+        incoming_proxy: Some("incoming".to_string()),
+        cw721_base_code_id: Some(1),
+        cw721_admin: Some("some_other_admin".to_string()),
+    };
+
+    // before migrate, populate legacy
+    let class_id_to_nft_contract: Map<ClassId, Addr> = Map::new("e");
+    class_id_to_nft_contract
+        .save(
+            deps.as_mut().storage,
+            ClassId::new(CLASS_ID_1),
+            &Addr::unchecked(NFT_CONTRACT_1),
+        )
+        .unwrap();
+    class_id_to_nft_contract
+        .save(
+            deps.as_mut().storage,
+            ClassId::new(CLASS_ID_2),
+            &Addr::unchecked(NFT_CONTRACT_2),
+        )
+        .unwrap();
+    let nft_contract_to_class_id: Map<Addr, ClassId> = Map::new("f");
+    nft_contract_to_class_id
+        .save(
+            deps.as_mut().storage,
+            Addr::unchecked(NFT_CONTRACT_1),
+            &ClassId::new(CLASS_ID_1),
+        )
+        .unwrap();
+    nft_contract_to_class_id
+        .save(
+            deps.as_mut().storage,
+            Addr::unchecked(NFT_CONTRACT_2),
+            &ClassId::new(CLASS_ID_2),
+        )
+        .unwrap();
+
+    // migrate
+    Ics721Contract {}
+        .migrate(deps.as_mut(), env.clone(), msg)
+        .unwrap();
+
+    assert_eq!(
+        PO.pauser.load(&deps.storage).unwrap(),
+        Some(Addr::unchecked("some_other_pauser"))
+    );
+    assert_eq!(
+        OUTGOING_PROXY.load(&deps.storage).unwrap(),
+        Some(Addr::unchecked("outgoing"))
+    );
+    assert_eq!(
+        INCOMING_PROXY.load(&deps.storage).unwrap(),
+        Some(Addr::unchecked("incoming"))
+    );
+    assert_eq!(CW721_CODE_ID.load(&deps.storage).unwrap(), 1);
+    assert_eq!(
+        ADMIN_USED_FOR_CW721.load(&deps.storage).unwrap(),
+        Some(Addr::unchecked("some_other_admin"))
+    );
+    let nft_contract_and_class_id_list = query_nft_contracts(deps.as_ref(), None, None).unwrap();
+    assert_eq!(nft_contract_and_class_id_list.len(), 2);
+    assert_eq!(nft_contract_and_class_id_list[0].0, CLASS_ID_1);
+    assert_eq!(nft_contract_and_class_id_list[0].1, NFT_CONTRACT_1);
+    assert_eq!(nft_contract_and_class_id_list[1].0, CLASS_ID_2);
+    assert_eq!(nft_contract_and_class_id_list[1].1, NFT_CONTRACT_2);
+    // test query and indexers for class id and addr are working
+    let nft_contract_1 =
+        query_nft_contract_for_class_id(&deps.storage, CLASS_ID_1.to_string()).unwrap();
+    assert_eq!(nft_contract_1, Some(Addr::unchecked(NFT_CONTRACT_1)));
+    let nft_contract_2 =
+        query_nft_contract_for_class_id(&deps.storage, CLASS_ID_2.to_string()).unwrap();
+    assert_eq!(nft_contract_2, Some(Addr::unchecked(NFT_CONTRACT_2)));
+    let class_id_1 =
+        query_class_id_for_nft_contract(deps.as_ref(), NFT_CONTRACT_1.to_string()).unwrap();
+    assert_eq!(class_id_1, Some(ClassId::new(CLASS_ID_1)));
+    let class_id_2 =
+        query_class_id_for_nft_contract(deps.as_ref(), NFT_CONTRACT_2.to_string()).unwrap();
+    assert_eq!(class_id_2, Some(ClassId::new(CLASS_ID_2)));
+}


test migration by populating new CLASS_ID_AND_NFT_CONTRACT_INFO with legacy data from NFT_CONTRACT_TO_CLASS_ID. Note: CLASS_ID_TO_NFT_CONTRACT is not used, since it is redundant to NFT_CONTRACT_TO_CLASS_ID

taitruong · 2024-02-12T11:55:19Z

packages/ics721/src/state.rs

-pub const CLASS_ID_TO_NFT_CONTRACT: Map<ClassId, Addr> = Map::new("e");
-/// Maps cw721 contracts to the classID they were instantiated for.
-pub const NFT_CONTRACT_TO_CLASS_ID: Map<Addr, ClassId> = Map::new("f");


removing redundant states...

taitruong · 2024-02-12T11:57:27Z

packages/ics721/src/state.rs

+pub const CLASS_ID_AND_NFT_CONTRACT_INFO: IndexedMap<&str, ClassIdInfo, ClassIdInfoIndexes> =
+    IndexedMap::new(
+        "m",
+        ClassIdInfoIndexes {
+            class_id: UniqueIndex::new(|d| d.class_id.clone(), "class_id_info__class_id"),
+            address: UniqueIndex::new(|d| d.address.clone(), "class_id_info__address"),
+        },
+    );


... and replaced by new state using IndexedMap

Why indexed map instead of the other two maps? What is benefit of putting it into new IndexedMap?

See answer below

taitruong · 2024-02-12T12:07:30Z

packages/ics721/src/execute.rs

        &self,
        deps: DepsMut,
        env: Env,
-        info: MessageInfo,
+        nft_contract: &Addr,
        token_id: TokenId,
        nft_owner: String,
        msg: Binary,
    ) -> Result<Response<T>, ContractError> {


explicit arg for nft_contract instead of using info...

taitruong · 2024-02-12T12:11:50Z

packages/ics721/src/execute.rs

-                        match deps.api.addr_validate(&msg.collection) {
-                            Ok(collection_addr) => {
-                                // set collection address as (initial) sender
-                                info.sender = collection_addr;


before info.sender was overridden using collection...

taitruong · 2024-02-12T12:12:18Z

packages/ics721/src/execute.rs

+                    .map(|msg| match deps.api.addr_validate(&msg.collection) {
+                        Ok(nft_contract) => self.receive_nft(
+                            deps,
+                            env,
+                            &nft_contract,


...after change, collection is directly passed.

yeah much better

humanalgorithm · 2024-02-15T14:29:46Z

contracts/sg-ics721/src/execute.rs

@@ -48,7 +48,9 @@ impl Ics721Execute for SgIcs721Contract {
                // therefore, we use ics721 creator as owner
                creator: ics721_contract_info.creator,
                description: "".to_string(),
-                image: "https://arkprotocol.io".to_string(),
+                // use Stargaze icon as placeholder
+                image: "ipfs://bafkreie5vwrm5zts4wiq6ebtopmztgl5qzyl4uszyllgwpaizyc5w2uycm"


Maybe this constant should be in a variable somewhere or in an external file?

moved to STARGAZE_ICON_PLACEHOLDER

humanalgorithm · 2024-02-15T14:34:42Z

packages/ics721/src/execute.rs

+        let instantiate =
+            if CLASS_ID_AND_NFT_CONTRACT_INFO.has(deps.storage, class.id.to_string().as_str()) {
+                vec![]
+            } else {


This is a huge block doing a lot of calculations. Can we put this in a function somewhere?

moved to create_instantiate_msg()

humanalgorithm · 2024-02-15T14:35:44Z

packages/ics721/src/execute.rs

-                    ))
+                    );
+
+                // we migrate only in case CLASS_ID_AND_NFT_CONTRACT_INFO is not populated yet


Maybe we can put this in a function so its clear that its legacy code to be deleted? It would make it more clear which code specifically is to be removed after migration.

moved to migrate_legacy()

humanalgorithm · 2024-02-15T14:37:39Z

packages/ics721/src/query.rs

-    }
+pub fn query_class_id_for_nft_contract(deps: Deps, contract: String) -> StdResult<Option<ClassId>> {
+    let contract = deps.api.addr_validate(&contract)?;
+    load_class_id_for_nft_contract(deps.storage, &contract)


taitruong · 2024-02-16T09:43:14Z

contracts/sg-ics721/src/state.rs

@@ -2,6 +2,9 @@ use cosmwasm_schema::cw_serde;
 use cosmwasm_std::ContractInfoResponse;
 use sg721_base::msg::CollectionInfoResponse;

+pub const STARGAZE_ICON_PLACEHOLDER: &str =
+    "ipfs://bafkreie5vwrm5zts4wiq6ebtopmztgl5qzyl4uszyllgwpaizyc5w2uycm";


waiting for @jhernandezb providing another link

taitruong · 2024-02-16T09:50:27Z

packages/ics721/src/execute.rs

-                NFT_CONTRACT_TO_CLASS_ID.save(deps.storage, info.sender.clone(), &class.id)?;
-                CLASS_ID_TO_NFT_CONTRACT.save(deps.storage, class.id.clone(), &info.sender)?;


@humanalgorithm as noted in audit:

In the cw-ics721 package, specifically in state.rs at lines 22 and 25, there are two cw-storage-plus::Maps
named CLASS_ID_TO_NFT_CONTRACT and NFT_CONTRACT_TO_CLASS_ID. These maps are responsible for tracking the class_id and collection_addr in a reciprocal manner. However, this setup leads to redundant operations since any modification to these values necessitates updates in both maps.

So both maps hold redundant data - which requires redundant operations like here.

That's funny, I originally proposed the IndexedMap in the original implementation but my suggestion got shot down.

taitruong · 2024-02-16T09:53:23Z

packages/ics721/src/state.rs

+            class_id: UniqueIndex::new(|d| d.class_id.clone(), "class_id_info__class_id"),
+            address: UniqueIndex::new(|d| d.address.clone(), "class_id_info__address"),


@humanalgorithm IndexedMap allows multiple indices. Here we need to put an index on ClassId and collection address.

taitruong · 2024-02-16T09:54:34Z

packages/ics721/src/query.rs

+        .idx
+        .address


@humanalgorithm here query is done on address index

taitruong · 2024-02-16T09:54:52Z

packages/ics721/src/query.rs

+        .idx
+        .class_id


@humanalgorithm here query is done on class id index

Art3miX

Where is execute_admin_clean_and_burn_nft function being used? can't find it

taitruong · 2024-02-18T14:58:49Z

Where is execute_admin_clean_and_burn_nft function being used? can't find it

This is related to #83 and not part of this PR. There was a bug on packet receive in case of failure in sub msgs and states (especially OUTGOING_CLASS_TOKEN_TO_CHANNEL) not being reverted since it was NOT in a sub msg - but in in main packet receive msg.

In short: this msg can be called by admin using ExecuteMsg::AdminCleanAndBurnNft for fixing states. Please also check counterpart msg AdminCleanAndUnescrowNft:

    /// Admin msg in case something goes wrong.
    /// As a minimum it clean up states (incoming channel and token metadata), and burn NFT if exists.
    AdminCleanAndBurnNft {
        owner: String,
        token_id: String,
        class_id: String,
        collection: String,
    },

    /// Admin msg in case something goes wrong.
    /// As a minimum it clean up state (outgoing channel), and transfer NFT if exists.
    /// - transfer NFT if exists
    AdminCleanAndUnescrowNft {
        recipient: String,
        token_id: String,
        class_id: String,
        collection: String,
    },

Art3miX

LFG

taitruong added 7 commits February 1, 2024 22:28

rename

2da4752

pass nft contract, instead of overriding info.sender

5059fc0

remove NFT_CONTRACT_TO_CLASS_ID and CLASS_ID_TO_NFT_CONTRACT and merg…

d6d6383

…e into CLASS_ID_AND_NFT_CONTRACT_INFO

test migration

36e0567

migrate only in case CLASS_ID_AND_NFT_CONTRACT_INFO is not populated …

86537cb

…yet, also keep legacy store

fix optional minter

f7b1b90

typo

7ecfb36

taitruong commented Feb 12, 2024

View reviewed changes

use Stargaze icon as placeholder

2d19b4d

humanalgorithm reviewed Feb 15, 2024

View reviewed changes

taitruong added 2 commits February 15, 2024 21:48

move to constant

e8e3c1f

move to dedicated function

bbc19e8

taitruong commented Feb 16, 2024

View reviewed changes

docs

9ef9bd7

humanalgorithm approved these changes Feb 16, 2024

View reviewed changes

humanalgorithm requested review from shanev, jhernandezb and tasiov February 16, 2024 10:30

shanev approved these changes Feb 17, 2024

View reviewed changes

humanalgorithm requested a review from Art3miX February 17, 2024 03:39

Art3miX reviewed Feb 17, 2024

View reviewed changes

Art3miX approved these changes Feb 18, 2024

View reviewed changes

taitruong mentioned this pull request Feb 18, 2024

Audit final revision compared to initial audit (based on branch is v0.1.5, commit f1dfefc71c3ace567a5b79e98100ee17d9cfcc5d) #88

Closed

taitruong added this pull request to the merge queue Feb 19, 2024

Merged via the queue into public-awesome:main with commit 65b9b78 Feb 19, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Final audit revision #90

Final audit revision #90

taitruong commented Feb 12, 2024 •

edited

Loading

taitruong Feb 12, 2024

taitruong Feb 12, 2024

taitruong Feb 12, 2024

taitruong Feb 12, 2024

humanalgorithm Feb 15, 2024 •

edited

Loading

taitruong Feb 16, 2024

taitruong Feb 12, 2024

taitruong Feb 12, 2024

taitruong Feb 12, 2024

humanalgorithm Feb 15, 2024

humanalgorithm Feb 15, 2024

taitruong Feb 16, 2024

humanalgorithm Feb 15, 2024

taitruong Feb 15, 2024

humanalgorithm Feb 16, 2024

humanalgorithm Feb 15, 2024 •

edited

Loading

taitruong Feb 15, 2024

humanalgorithm Feb 16, 2024

humanalgorithm Feb 15, 2024

taitruong Feb 15, 2024

taitruong Feb 16, 2024

taitruong Feb 16, 2024

humanalgorithm Feb 16, 2024

taitruong Feb 16, 2024

taitruong Feb 16, 2024

taitruong Feb 16, 2024

taitruong Feb 16, 2024

Art3miX left a comment

taitruong commented Feb 18, 2024

Art3miX left a comment

		NFT_CONTRACT_TO_CLASS_ID.save(deps.storage, info.sender.clone(), &class.id)?;
		CLASS_ID_TO_NFT_CONTRACT.save(deps.storage, class.id.clone(), &info.sender)?;

		class_id: UniqueIndex::new(\|d\| d.class_id.clone(), "class_id_info__class_id"),
		address: UniqueIndex::new(\|d\| d.address.clone(), "class_id_info__address"),

Final audit revision #90

Final audit revision #90

Conversation

taitruong commented Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humanalgorithm Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humanalgorithm Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Art3miX left a comment

Choose a reason for hiding this comment

taitruong commented Feb 18, 2024

Art3miX left a comment

Choose a reason for hiding this comment

taitruong commented Feb 12, 2024 •

edited

Loading

humanalgorithm Feb 15, 2024 •

edited

Loading

humanalgorithm Feb 15, 2024 •

edited

Loading