Repository: DA0-DA0/dao-contracts (rust)

Issue:
Validate cw20 and cw721 contract addresses upon adding
`cw-core` does not validate that the addresses being added to either token lists are actually valid token contracts.

The frontend was supposed to validate these addresses, but was not. As of https://github.com/DA0-DA0/dao-ui/pull/774, the frontend now validates these.

It's still a good idea that we enforce this on the contract side. It can be done by ensuring CW20s respond to `token_info` and CW721s respond to `contract_info`.

Pull request description:
Basic validation for cw20 and cw721 contracts added to treasury.
This performs some basic validation for cw20 and cw721 tokens added to the cw-core treasury. This is in response to an issue reported where a user added a cw721 and broke the `Cw20Balances` query.

Resolves #427 

Patch:
diff --git a/contracts/cw-core/src/contract.rs b/contracts/cw-core/src/contract.rs
index a83d12e7..68d0cb47 100644
--- a/contracts/cw-core/src/contract.rs
+++ b/contracts/cw-core/src/contract.rs
@@ -346,11 +346,14 @@ pub fn execute_update_proposal_modules(
         .add_submessages(to_add))
 }
 
+/// Updates a set of addresses in state applying VERIFY to each item
+/// that will be added.
 fn do_update_addr_list(
     deps: DepsMut,
     map: Map<Addr, Empty>,
     to_add: Vec<String>,
     to_remove: Vec<String>,
+    verify: impl Fn(&Addr, Deps) -> StdResult<()>,
 ) -> Result<(), ContractError> {
     let to_add = to_add
         .into_iter()
@@ -363,6 +366,7 @@ fn do_update_addr_list(
         .collect::<Result<Vec<_>, _>>()?;
 
     for addr in to_add {
+        verify(&addr, deps.as_ref())?;
         map.save(deps.storage, addr, &Empty {})?;
     }
     for addr in to_remove {
@@ -382,7 +386,17 @@ pub fn execute_update_cw20_list(
     if env.contract.address != sender {
         return Err(ContractError::Unauthorized {});
     }
-    do_update_addr_list(deps, CW20_LIST, to_add, to_remove)?;
+    do_update_addr_list(deps, CW20_LIST, to_add, to_remove, |addr, deps| {
+        // Perform a balance query here as this is the query performed
+        // by the `Cw20Balances` query.
+        let _info: cw20::BalanceResponse = deps.querier.query_wasm_smart(
+            addr,
+            &cw20::Cw20QueryMsg::Balance {
+                address: env.contract.address.to_string(),
+            },
+        )?;
+        Ok(())
+    })?;
     Ok(Response::default().add_attribute("action", "update_cw20_list"))
 }
 
@@ -396,7 +410,12 @@ pub fn execute_update_cw721_list(
     if env.contract.address != sender {
         return Err(ContractError::Unauthorized {});
     }
-    do_update_addr_list(deps, CW721_LIST, to_add, to_remove)?;
+    do_update_addr_list(deps, CW721_LIST, to_add, to_remove, |addr, deps| {
+        let _info: cw721::ContractInfoResponse = deps
+            .querier
+            .query_wasm_smart(addr, &cw721::Cw721QueryMsg::ContractInfo {})?;
+        Ok(())
+    })?;
     Ok(Response::default().add_attribute("action", "update_cw721_list"))
 }
 
diff --git a/packages/cw-paginate/README.md b/packages/cw-paginate/README.md
new file mode 100644
index 00000000..ecf08f35
--- /dev/null
+++ b/packages/cw-paginate/README.md
@@ -0,0 +1,32 @@
+# CosmWasm Map Pagination
+
+This package provides generic convienence methods for paginating keys
+and values in a CosmWasm `Map` or `SnapshotMap`. If you use these
+methods to paginate the maps in your contract you may [make larry0x
+happy](https://twitter.com/larry0x/status/1530537243709939719).
+
+## Example
+
+Given a map like:
+
+```rust
+pub const ITEMS: Map<String, String> = Map::new("items");
+```
+
+You can use this package to write a query to list it's contents like:
+
+```rust
+pub fn query_list_items(
+    deps: Deps,
+    start_after: Option<String>,
+    limit: Option<u32>,
+) -> StdResult<Binary> {
+    to_binary(&paginate_map(
+        deps,
+        &ITEMS,
+        start_after,
+        limit,
+        cosmwasm_std::Order::Descending,
+    )?)
+}
+```
diff --git a/packages/cw-paginate/src/lib.rs b/packages/cw-paginate/src/lib.rs
index b1eb6319..7806056b 100644
--- a/packages/cw-paginate/src/lib.rs
+++ b/packages/cw-paginate/src/lib.rs
@@ -1,3 +1,44 @@
+//! # CosmWasm Map Pagination
+//
+//! This package provides generic convienence methods for paginating keys
+//! and values in a CosmWasm `Maop` or `SnapshotMap`. If you use these
+//! methods to paginate the maps in your contract you may [make larry0x
+//! happy](https://twitter.com/larry0x/status/1530537243709939719).
+//
+//! ## Example
+//
+//! Given a map like:
+//
+//! ```rust
+//! # use cw_storage_plus::Map;
+//!
+//! pub const ITEMS: Map<String, String> = Map::new("items");
+//! ```
+//
+//! You can use this package to write a query to list it's contents like:
+//
+//! ```rust
+//! # use cosmwasm_std::{Deps, Binary, to_binary, StdResult};
+//! # use cw_storage_plus::Map;
+//! # use cw_paginate::paginate_map;
+//!
+//! # pub const ITEMS: Map<String, String> = Map::new("items");
+//!
+//! pub fn query_list_items(
+//!     deps: Deps,
+//!     start_after: Option<String>,
+//!     limit: Option<u32>,
+//! ) -> StdResult<Binary> {
+//!     to_binary(&paginate_map(
+//!         deps,
+//!         &ITEMS,
+//!         start_after,
+//!         limit,
+//!         cosmwasm_std::Order::Descending,
+//!     )?)
+//! }
+//!  ```
+
 use cosmwasm_std::{Deps, Order, StdResult};
 use cw_storage_plus::{Bound, Bounder, KeyDeserialize, Map, SnapshotMap};
 


Test patch:
diff --git a/contracts/cw-core/src/tests.rs b/contracts/cw-core/src/tests.rs
index 2f925bad..37ef94e2 100644
--- a/contracts/cw-core/src/tests.rs
+++ b/contracts/cw-core/src/tests.rs
@@ -868,7 +868,7 @@ fn test_admin_permissions() {
         },
         &[],
     );
-    assert!(res.is_err());
+    res.unwrap_err();
 
     // Proposal mdoule can't call ExecuteAdminMsgs
     let res = app.execute_contract(
@@ -887,7 +887,7 @@ fn test_admin_permissions() {
         },
         &[],
     );
-    assert!(res.is_err());
+    res.unwrap_err();
 
     // Update Admin can't be called by non-admins
     let res = app.execute_contract(
@@ -898,7 +898,7 @@ fn test_admin_permissions() {
         },
         &[],
     );
-    assert!(res.is_err());
+    res.unwrap_err();
 
     // Nominate admin can be called by core contract as no admin was
     // specified so the admin defaulted to the core contract.
@@ -918,7 +918,7 @@ fn test_admin_permissions() {
         },
         &[],
     );
-    assert!(res.is_ok());
+    res.unwrap();
 
     // Instantiate new DAO with an admin
     let (core_with_admin_addr, mut app) =
@@ -941,7 +941,7 @@ fn test_admin_permissions() {
         },
         &[],
     );
-    assert!(res.is_err());
+    res.unwrap_err();
 
     // Admin can call ExecuteAdminMsgs, here an admin pasues the DAO
     let res = app.execute_contract(
@@ -960,7 +960,7 @@ fn test_admin_permissions() {
         },
         &[],
     );
-    assert!(res.is_ok());
+    res.unwrap();
 
     let paused: PauseInfoResponse = app
         .wrap()
@@ -985,7 +985,7 @@ fn test_admin_permissions() {
         },
         &[],
     );
-    assert!(res.is_ok());
+    res.unwrap();
 
     let nomination: AdminNominationResponse = app
         .wrap()
@@ -1226,7 +1226,7 @@ fn test_admin_nomination() {
         },
         &[],
     );
-    assert!(res.is_ok());
+    res.unwrap();
 
     let paused: PauseInfoResponse = app
         .wrap()
@@ -1553,6 +1553,25 @@ fn test_instantiate_with_items() {
 fn test_cw20_receive_auto_add() {
     let (gov_addr, mut app) = do_standard_instantiate(true, None);
 
+    let cw20_id = app.store_code(cw20_contract());
+    let another_cw20 = app
+        .instantiate_contract(
+            cw20_id,
+            Addr::unchecked(CREATOR_ADDR),
+            &cw20_base::msg::InstantiateMsg {
+                name: "DAO".to_string(),
+                symbol: "DAO".to_string(),
+                decimals: 6,
+                initial_balances: vec![],
+                mint: None,
+                marketing: None,
+            },
+            &[],
+            "another-token",
+            None,
+        )
+        .unwrap();
+
     let voting_module: Addr = app
         .wrap()
         .query_wasm_smart(gov_addr.clone(), &QueryMsg::VotingModule {})
@@ -1621,12 +1640,27 @@ fn test_cw20_receive_auto_add() {
         }]
     );
 
-    // Test removing and adding some new ones.
+    // Test removing and adding some new ones. Invalid should fail.
+    let err: ContractError = app
+        .execute_contract(
+            Addr::unchecked(gov_addr.clone()),
+            gov_addr.clone(),
+            &ExecuteMsg::UpdateCw20List {
+                to_add: vec!["new".to_string()],
+                to_remove: vec![gov_token.to_string()],
+            },
+            &[],
+        )
+        .unwrap_err()
+        .downcast()
+        .unwrap();
+    assert!(matches!(err, ContractError::Std(_)));
+
     app.execute_contract(
         Addr::unchecked(gov_addr.clone()),
         gov_addr.clone(),
         &ExecuteMsg::UpdateCw20List {
-            to_add: vec!["new".to_string()],
+            to_add: vec![another_cw20.to_string()],
             to_remove: vec![gov_token.to_string()],
         },
         &[],
@@ -1643,13 +1677,32 @@ fn test_cw20_receive_auto_add() {
             },
         )
         .unwrap();
-    assert_eq!(cw20_list, vec![Addr::unchecked("new")]);
+    assert_eq!(cw20_list, vec![another_cw20]);
 }
 
 #[test]
 fn test_cw20_receive_no_auto_add() {
     let (gov_addr, mut app) = do_standard_instantiate(false, None);
 
+    let cw20_id = app.store_code(cw20_contract());
+    let another_cw20 = app
+        .instantiate_contract(
+            cw20_id,
+            Addr::unchecked(CREATOR_ADDR),
+            &cw20_base::msg::InstantiateMsg {
+                name: "DAO".to_string(),
+                symbol: "DAO".to_string(),
+                decimals: 6,
+                initial_balances: vec![],
+                mint: None,
+                marketing: None,
+            },
+            &[],
+            "another-token",
+            None,
+        )
+        .unwrap();
+
     let voting_module: Addr = app
         .wrap()
         .query_wasm_smart(gov_addr.clone(), &QueryMsg::VotingModule {})
@@ -1692,7 +1745,7 @@ fn test_cw20_receive_no_auto_add() {
         Addr::unchecked(gov_addr.clone()),
         gov_addr.clone(),
         &ExecuteMsg::UpdateCw20List {
-            to_add: vec!["new".to_string(), gov_token.to_string()],
+            to_add: vec![another_cw20.to_string(), gov_token.to_string()],
             to_remove: vec!["ok to remove non existent".to_string()],
         },
         &[],
@@ -1709,7 +1762,7 @@ fn test_cw20_receive_no_auto_add() {
             },
         )
         .unwrap();
-    assert_eq!(cw20_list, vec![Addr::unchecked("new"), gov_token]);
+    assert_eq!(cw20_list, vec![another_cw20, gov_token]);
 }
 
 #[test]
@@ -1733,6 +1786,21 @@ fn test_cw721_receive() {
         )
         .unwrap();
 
+    let another_cw721 = app
+        .instantiate_contract(
+            cw721_id,
+            Addr::unchecked(CREATOR_ADDR),
+            &cw721_base::msg::InstantiateMsg {
+                name: "ekez".to_string(),
+                symbol: "ekez".to_string(),
+                minter: CREATOR_ADDR.to_string(),
+            },
+            &[],
+            "cw721",
+            None,
+        )
+        .unwrap();
+
     app.execute_contract(
         Addr::unchecked(CREATOR_ADDR),
         cw721_addr.clone(),
@@ -1770,12 +1838,28 @@ fn test_cw721_receive() {
         .unwrap();
     assert_eq!(cw721_list, vec![cw721_addr.clone()]);
 
-    // OK to add already added. Remove happens after add.
+    // Try to add an invalid cw721.
+    let err: ContractError = app
+        .execute_contract(
+            Addr::unchecked(gov_addr.clone()),
+            gov_addr.clone(),
+            &ExecuteMsg::UpdateCw721List {
+                to_add: vec!["new".to_string(), cw721_addr.to_string()],
+                to_remove: vec![cw721_addr.to_string()],
+            },
+            &[],
+        )
+        .unwrap_err()
+        .downcast()
+        .unwrap();
+    assert!(matches!(err, ContractError::Std(_)));
+
+    // Add a real cw721.
     app.execute_contract(
         Addr::unchecked(gov_addr.clone()),
         gov_addr.clone(),
         &ExecuteMsg::UpdateCw721List {
-            to_add: vec!["new".to_string(), cw721_addr.to_string()],
+            to_add: vec![another_cw721.to_string(), cw721_addr.to_string()],
             to_remove: vec![cw721_addr.to_string()],
         },
         &[],
@@ -1792,7 +1876,7 @@ fn test_cw721_receive() {
             },
         )
         .unwrap();
-    assert_eq!(cw20_list, vec![Addr::unchecked("new")]);
+    assert_eq!(cw20_list, vec![another_cw721]);
 }
 
 #[test]
@@ -1816,6 +1900,21 @@ fn test_cw721_receive_no_auto_add() {
         )
         .unwrap();
 
+    let another_cw721 = app
+        .instantiate_contract(
+            cw721_id,
+            Addr::unchecked(CREATOR_ADDR),
+            &cw721_base::msg::InstantiateMsg {
+                name: "ekez".to_string(),
+                symbol: "ekez".to_string(),
+                minter: CREATOR_ADDR.to_string(),
+            },
+            &[],
+            "cw721",
+            None,
+        )
+        .unwrap();
+
     app.execute_contract(
         Addr::unchecked(CREATOR_ADDR),
         cw721_addr.clone(),
@@ -1859,7 +1958,7 @@ fn test_cw721_receive_no_auto_add() {
         gov_addr.clone(),
         &ExecuteMsg::UpdateCw721List {
             to_add: vec![
-                "new".to_string(),
+                another_cw721.to_string(),
                 cw721_addr.to_string(),
                 cw721_addr.to_string(),
             ],
@@ -1879,7 +1978,7 @@ fn test_cw721_receive_no_auto_add() {
             },
         )
         .unwrap();
-    assert_eq!(cw20_list, vec![Addr::unchecked("new"), cw721_addr]);
+    assert_eq!(cw20_list, vec![another_cw721, cw721_addr]);
 }
 
 #[test]
@@ -2227,6 +2326,6 @@ fn test_execute_stargate_msg() {
         },
         &[],
     );
-    // TODO: Once cw-multi-test supports executing stargate/ibc messages we can change this test assert
-    assert!(res.is_err());
+    // TODO: Once cw-multi-test supports executing stargate/ibc messages we can change this result unwrap
+    res.unwrap_err();
 }
diff --git a/contracts/cw-proposal-multiple/src/tests.rs b/contracts/cw-proposal-multiple/src/tests.rs
index a44490c4..193cba6d 100644
--- a/contracts/cw-proposal-multiple/src/tests.rs
+++ b/contracts/cw-proposal-multiple/src/tests.rs
@@ -274,7 +274,9 @@ where
                 };
                 assert_eq!(vote, expected)
             }
-            ShouldExecute::No => assert!(res.is_err()),
+            ShouldExecute::No => {
+                res.unwrap_err();
+            }
             ShouldExecute::Meh => (),
         }
     }
diff --git a/contracts/cw-proposal-single/src/tests.rs b/contracts/cw-proposal-single/src/tests.rs
index 8413fc87..c4d32180 100644
--- a/contracts/cw-proposal-single/src/tests.rs
+++ b/contracts/cw-proposal-single/src/tests.rs
@@ -822,7 +822,9 @@ where
                 };
                 assert_eq!(vote, expected)
             }
-            ShouldExecute::No => assert!(res.is_err()),
+            ShouldExecute::No => {
+                res.unwrap_err();
+            }
             ShouldExecute::Meh => (),
         }
     }