Repository: DA0-DA0/dao-contracts
Pull request #186 (2022-03-03 21:21:36)

Remove usage of Addr in contract messages.
The Addr type is not validated during deserialization yet it still
implements both serialization and deserialization. This means that an
invalid Addr can be constructed in the event that it is passed to a
contract via a message.

This removes all usage of Addr in message types. Before this change
two bugs are present:

1. For the DAO and Multisig contracts tokens can be added to the
treasury which are invalid.
2. For the staking contract this allows setting the admin of the
contract to an invalid address.


Patch:
diff --git a/contracts/cw3-dao/src/contract.rs b/contracts/cw3-dao/src/contract.rs
index 07fe23c7..9ab780b1 100644
--- a/contracts/cw3-dao/src/contract.rs
+++ b/contracts/cw3-dao/src/contract.rs
@@ -143,9 +143,9 @@ pub fn instantiate(
                 admin: Some(env.contract.address.to_string()),
                 label,
                 msg: to_binary(&stake_cw20::msg::InstantiateMsg {
-                    admin: Some(env.contract.address),
+                    admin: Some(env.contract.address.to_string()),
                     unstaking_duration,
-                    token_address: cw20_addr.addr(),
+                    token_address: cw20_addr.addr().to_string(),
                 })?,
             };
 
@@ -454,13 +454,13 @@ pub fn execute_update_staking_contract(
     deps: DepsMut,
     env: Env,
     info: MessageInfo,
-    new_staking_contract: Addr,
+    new_staking_contract: String,
 ) -> Result<Response<Empty>, ContractError> {
     // Only contract can call this method
     if env.contract.address != info.sender {
         return Err(ContractError::Unauthorized {});
     }
-    let new_staking_contract = deps.api.addr_validate(new_staking_contract.as_str())?;
+    let new_staking_contract = deps.api.addr_validate(&new_staking_contract)?;
 
     // Replace the existing staking contract
     STAKING_CONTRACT.save(deps.storage, &new_staking_contract)?;
@@ -474,8 +474,8 @@ pub fn execute_update_cw20_token_list(
     deps: DepsMut,
     env: Env,
     info: MessageInfo,
-    to_add: Vec<Addr>,
-    to_remove: Vec<Addr>,
+    to_add: Vec<String>,
+    to_remove: Vec<String>,
 ) -> Result<Response<Empty>, ContractError> {
     // Only contract can call this method
     if env.contract.address != info.sender {
@@ -491,12 +491,14 @@ pub fn execute_update_cw20_token_list(
         });
     }
 
-    for token in &to_add {
-        TREASURY_TOKENS.save(deps.storage, token, &Empty {})?;
+    for token in to_add {
+        let token = deps.api.addr_validate(&token)?;
+        TREASURY_TOKENS.save(deps.storage, &token, &Empty {})?;
     }
 
-    for token in &to_remove {
-        TREASURY_TOKENS.remove(deps.storage, token);
+    for token in to_remove {
+        let token = deps.api.addr_validate(&token)?;
+        TREASURY_TOKENS.remove(deps.storage, &token);
     }
 
     Ok(Response::new().add_attribute("action", "update_cw20_token_list"))
@@ -771,9 +773,9 @@ pub fn reply(deps: DepsMut, env: Env, msg: Reply) -> Result<Response, ContractEr
                         admin: Some(env.contract.address.to_string()),
                         label: env.contract.address.to_string(),
                         msg: to_binary(&stake_cw20::msg::InstantiateMsg {
-                            admin: Some(env.contract.address),
+                            admin: Some(env.contract.address.to_string()),
                             unstaking_duration,
-                            token_address: cw20_addr,
+                            token_address: cw20_addr.to_string(),
                         })?,
                     };
                     let msg = SubMsg::reply_on_success(msg, INSTANTIATE_STAKING_CONTRACT_REPLY_ID);
diff --git a/contracts/cw3-dao/src/msg.rs b/contracts/cw3-dao/src/msg.rs
index e16b70d1..5a01606a 100644
--- a/contracts/cw3-dao/src/msg.rs
+++ b/contracts/cw3-dao/src/msg.rs
@@ -1,7 +1,7 @@
 use crate::error::ContractError;
 use crate::query::ThresholdResponse;
 use crate::state::Config;
-use cosmwasm_std::{Addr, CosmosMsg, Decimal, Empty, Uint128};
+use cosmwasm_std::{CosmosMsg, Decimal, Empty, Uint128};
 use cw20::{Cw20Coin, Cw20ReceiveMsg};
 use cw20_base::msg::InstantiateMarketingInfo;
 use cw3::Vote;
@@ -163,12 +163,12 @@ pub enum ExecuteMsg {
     UpdateConfig(Config),
     /// Updates token list
     UpdateCw20TokenList {
-        to_add: Vec<Addr>,
-        to_remove: Vec<Addr>,
+        to_add: Vec<String>,
+        to_remove: Vec<String>,
     },
     /// Update Staking Contract (can only be called by DAO contract)
     /// WARNING: this changes the contract controlling voting
-    UpdateStakingContract { new_staking_contract: Addr },
+    UpdateStakingContract { new_staking_contract: String },
     /// Wrapper called for automatically adding cw20s
     /// to our tracked balances
     Receive(Cw20ReceiveMsg),
diff --git a/contracts/cw3-multisig/src/contract.rs b/contracts/cw3-multisig/src/contract.rs
index c1042a50..285b3486 100644
--- a/contracts/cw3-multisig/src/contract.rs
+++ b/contracts/cw3-multisig/src/contract.rs
@@ -364,8 +364,8 @@ pub fn execute_update_cw20_token_list(
     deps: DepsMut,
     env: Env,
     info: MessageInfo,
-    to_add: Vec<Addr>,
-    to_remove: Vec<Addr>,
+    to_add: Vec<String>,
+    to_remove: Vec<String>,
 ) -> Result<Response<Empty>, ContractError> {
     // Only contract can call this method
     if env.contract.address != info.sender {
@@ -381,12 +381,14 @@ pub fn execute_update_cw20_token_list(
         });
     }
 
-    for token in &to_add {
-        TREASURY_TOKENS.save(deps.storage, token, &Empty {})?;
+    for token in to_add {
+        let token = deps.api.addr_validate(&token)?;
+        TREASURY_TOKENS.save(deps.storage, &token, &Empty {})?;
     }
 
-    for token in &to_remove {
-        TREASURY_TOKENS.remove(deps.storage, token);
+    for token in to_remove {
+        let token = deps.api.addr_validate(&token)?;
+        TREASURY_TOKENS.remove(deps.storage, &token);
     }
 
     Ok(Response::new().add_attribute("action", "update_cw20_token_list"))
diff --git a/contracts/cw3-multisig/src/msg.rs b/contracts/cw3-multisig/src/msg.rs
index b4cb12bb..6172a915 100644
--- a/contracts/cw3-multisig/src/msg.rs
+++ b/contracts/cw3-multisig/src/msg.rs
@@ -2,7 +2,7 @@ use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 
 use crate::{error::ContractError, state::Config};
-use cosmwasm_std::{Addr, CosmosMsg, Decimal, Empty};
+use cosmwasm_std::{CosmosMsg, Decimal, Empty};
 use cw20::Cw20ReceiveMsg;
 use cw3::Vote;
 use cw4::{Member, MemberChangedHookMsg};
@@ -159,8 +159,8 @@ pub enum ExecuteMsg {
     UpdateConfig(Config),
     /// Updates token list
     UpdateCw20TokenList {
-        to_add: Vec<Addr>,
-        to_remove: Vec<Addr>,
+        to_add: Vec<String>,
+        to_remove: Vec<String>,
     },
     /// Wrapper called for automatically adding cw20s
     /// to our tracked balances
diff --git a/contracts/stake-cw20/src/contract.rs b/contracts/stake-cw20/src/contract.rs
index 93c4df86..65db79b3 100644
--- a/contracts/stake-cw20/src/contract.rs
+++ b/contracts/stake-cw20/src/contract.rs
@@ -44,9 +44,10 @@ pub fn instantiate(
         None => None,
     };
 
+    let token_address = deps.api.addr_validate(&msg.token_address)?;
     let config = Config {
         admin,
-        token_address: msg.token_address,
+        token_address,
         unstaking_duration: msg.unstaking_duration,
     };
     CONFIG.save(deps.storage, &config)?;
@@ -75,7 +76,7 @@ pub fn execute(
 pub fn execute_update_config(
     info: MessageInfo,
     deps: DepsMut,
-    new_admin: Option<Addr>,
+    new_admin: Option<String>,
     duration: Option<Duration>,
 ) -> Result<Response, ContractError> {
     let mut config: Config = CONFIG.load(deps.storage)?;
@@ -89,7 +90,7 @@ pub fn execute_update_config(
                 });
             }
 
-            config.admin = new_admin;
+            config.admin = new_admin.map(|a| deps.api.addr_validate(&a)).transpose()?;
             config.unstaking_duration = duration;
 
             CONFIG.save(deps.storage, &config)?;
@@ -446,8 +447,8 @@ mod tests {
     ) -> Addr {
         let staking_code_id = app.store_code(contract_staking());
         let msg = crate::msg::InstantiateMsg {
-            admin: Some(Addr::unchecked("owner")),
-            token_address: cw20,
+            admin: Some("owner".to_string()),
+            token_address: cw20.to_string(),
             unstaking_duration,
         };
         app.instantiate_contract(
@@ -553,7 +554,10 @@ mod tests {
         admin: Option<Addr>,
         duration: Option<Duration>,
     ) -> AnyResult<AppResponse> {
-        let msg = ExecuteMsg::UpdateConfig { admin, duration };
+        let msg = ExecuteMsg::UpdateConfig {
+            admin: admin.map(|a| a.to_string()),
+            duration,
+        };
         app.execute_contract(info.sender, staking_addr.clone(), &msg, &[])
     }
 
diff --git a/contracts/stake-cw20/src/msg.rs b/contracts/stake-cw20/src/msg.rs
index 32303b7b..e0b65b18 100644
--- a/contracts/stake-cw20/src/msg.rs
+++ b/contracts/stake-cw20/src/msg.rs
@@ -9,8 +9,8 @@ pub use cw_controllers::ClaimsResponse;
 
 #[derive(Serialize, Deserialize, JsonSchema, Debug, Clone, PartialEq)]
 pub struct InstantiateMsg {
-    pub admin: Option<Addr>,
-    pub token_address: Addr,
+    pub admin: Option<String>,
+    pub token_address: String,
     pub unstaking_duration: Option<Duration>,
 }
 
@@ -23,7 +23,7 @@ pub enum ExecuteMsg {
     },
     Claim {},
     UpdateConfig {
-        admin: Option<Addr>,
+        admin: Option<String>,
         duration: Option<Duration>,
     },
 }


Test patch:
diff --git a/contracts/cw3-dao/src/tests.rs b/contracts/cw3-dao/src/tests.rs
index 4e38a2f6..cb6ec621 100644
--- a/contracts/cw3-dao/src/tests.rs
+++ b/contracts/cw3-dao/src/tests.rs
@@ -904,14 +904,8 @@ fn test_token_add_limited() {
 
     // Attempt to add a bunch of nonesense tokens
     let update_token_list_msg = ExecuteMsg::UpdateCw20TokenList {
-        to_add: (0..20)
-            .into_iter()
-            .map(|i| Addr::unchecked(i.to_string()))
-            .collect(),
-        to_remove: (20..31)
-            .into_iter()
-            .map(|i| Addr::unchecked(i.to_string()))
-            .collect(),
+        to_add: (0..20).into_iter().map(|i| i.to_string()).collect(),
+        to_remove: (20..31).into_iter().map(|i| i.to_string()).collect(),
     };
     let wasm_msg = WasmMsg::Execute {
         contract_addr: dao_addr.clone().into(),
@@ -2032,7 +2026,7 @@ fn test_update_staking_contract() {
 
     // Nobody can call call update staking contract method directly
     let update_staking_contract_msg = ExecuteMsg::UpdateStakingContract {
-        new_staking_contract: Addr::unchecked("Better_Staking_Contract"),
+        new_staking_contract: "Better_Staking_Contract".to_string(),
     };
     let res = app.execute_contract(
         Addr::unchecked(VOTER1),
@@ -2604,8 +2598,8 @@ fn treasury_queries() {
 
     // Manually add token to list by voting
     let update_token_list_msg = ExecuteMsg::UpdateCw20TokenList {
-        to_add: vec![Addr::unchecked("NEW"), Addr::unchecked("NEWNEW")],
-        to_remove: vec![other_cw20_addr],
+        to_add: vec!["NEW".to_string(), "NEWNEW".to_string()],
+        to_remove: vec![other_cw20_addr.to_string()],
     };
     let wasm_msg = WasmMsg::Execute {
         contract_addr: dao_addr.clone().into(),
diff --git a/contracts/cw3-multisig/src/tests.rs b/contracts/cw3-multisig/src/tests.rs
index 2a1a9f01..fe042e13 100644
--- a/contracts/cw3-multisig/src/tests.rs
+++ b/contracts/cw3-multisig/src/tests.rs
@@ -867,14 +867,8 @@ fn test_token_add_limited() {
 
     // Attempt to add a bunch of nonesense tokens
     let update_token_list_msg = ExecuteMsg::UpdateCw20TokenList {
-        to_add: (0..20)
-            .into_iter()
-            .map(|i| Addr::unchecked(i.to_string()))
-            .collect(),
-        to_remove: (20..31)
-            .into_iter()
-            .map(|i| Addr::unchecked(i.to_string()))
-            .collect(),
+        to_add: (0..20).into_iter().map(|i| i.to_string()).collect(),
+        to_remove: (20..31).into_iter().map(|i| i.to_string()).collect(),
     };
     let wasm_msg = WasmMsg::Execute {
         contract_addr: multisig_addr.clone().into(),
@@ -2284,8 +2278,8 @@ fn treasury_queries() {
 
     // Manually add token to list by voting
     let update_token_list_msg = ExecuteMsg::UpdateCw20TokenList {
-        to_add: vec![Addr::unchecked("NEW"), Addr::unchecked("NEWNEW")],
-        to_remove: vec![other_cw20_addr],
+        to_add: vec!["NEW".to_string(), "NEWNEW".to_string()],
+        to_remove: vec![other_cw20_addr.to_string()],
     };
     let wasm_msg = WasmMsg::Execute {
         contract_addr: multisig_addr.clone().into(),