Audit Fixes and Improvements (#742)

* Add missing hooks to dao-voting-native-staked, start adding tests * Fix group contract attribute key emits "address" * Fix absoluteCount can be configured to be greater than the total NFT supply * Fix absoluteCount threshold for a new token is not validated * Fix issuer contract can be blacklisted * Fix Stargaze collection info bug, and absolute count validation sg721 has two addresses that control it. The `minter` which can mint NFTs, and the `creator` which can update collection metadata and royalties. We now set both of those to be the DAO. * Fix incorrect events are emitted for whitelist * Fix lack of denom validation * Fix BEFORE_SEND_HOOK_FEATURES_ENABLED is not exposed through smart queries * Fix misleading from attribute when burning funds * Fix broken tests from denom validation * Fix inconsistent attribute namings and orderings * Fix INITITIAL_NFTS spelling XD * Fix counterintuitive variable namings * Better API for cw-tokenfactory-issuer Many of these methods are not needed thanks to Authz. * Appease clippy gods * Reduce unneeded gas consumption by setting admin directly * Fix unneeded reply_always when instantiating new token * Remove unused function * Improve comments * Use “Migrate only if newer” pattern * Clean up and fix issuer test-tube tests * Combine all dao-related hooks into a single package * Improve code resuse by consilidating stake hooks * Clean up types * Remove need for unused TokenFactoryQuery, use shared stake hooks * Fix stake hook tests, update schema * Incorporate NFT staking hooks into dao-hooks package * Add cw4::MemberChangedHookMsg and clean up dao-hooks pkg * Make clippy happy, use only migrate newer across all voting contracts * cargo fmt * clippy on latest rust version * Improve unstaking_duration validation reuse * Allowlist should apply to transfers from *or to* an address For example, a DAO may wish to white list a Token Staking contract (to allow users to stake their tokens in the DAO) or a Merkle Drop contract (to allow users to claim their tokens). * Add info on renouncing Token Factory Admin * Remove outdated comments * Remove commented out metadata test Osmosis test tube doesn't support metadata queries anymore, we assume metadata is set properly by the Cosmos SDK and this is well tested upstream. * More informative error to address BlockBeforeSend hook executing on minting or burning This is intended functionality. If a token is frozen, a DAO needs to both grant a minter allowance as well as adding the minter to the allowlist to allow for token transfers when the token is frozen. The new error message should make next steps clear if this edge case is encountered. * Use tagged versions or upstream git repos for deps * Better code documentation for cw-tokenfactory-issuer * Make it possible to unset BeforeSendHook, or set to a different contract DAOs may wish to disable the BeforeSendHook at some point, or potentially set it to a custom cosmwasm contract if they wish to customize functionality a bit more. Modifies the SetBeforeSendHook method to allow for this, and adds tests that it's indeed possible to set it to nil. * Default to BeforeSendHook features being disabled These are powerful features, but many may not want them as they can be abused. DAOs that want these features will have to explicitly enable them via a governance prop. * BeforeSendHook refactor and tests * Cleanup TODO, verify correct error * Accurate comment * Improve active threshold validation reuse * Don't create issuer for existing tokens * Reorg so we can have only one native token voting contract * No need to have an issuer for existing tokens * Update Schema * Fix up integration tests * Remove owner from dao-voting-cw721-staked The concept of ownership doesn't really make sense for voting contracts. IMO the owner should always be the DAO. * Improve TF docs * Implement two-step ownership transfer for cw_tokenfactory_issue * Tests for renouncing ownership * Fix package name. * Fix integration tests * set_before_update_hook -> set_before_send_hook * Address remaining TODOs --------- Co-authored-by: Jake Hartnell <[email protected]>
DA0-DA0 · Sep 15, 2023 · 3518a28 · 3518a28
1 parent bf3ef9c
commit 3518a28
Show file tree

Hide file tree

Showing 144 changed files with 3,825 additions and 6,968 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -54,8 +54,8 @@ cw721 = "0.18"
 cw721-base = "0.18"
 env_logger = "0.10"
 once_cell = "1.18"
-osmosis-std = { git = "https://github.com/osmosis-labs/osmosis-rust", branch = "autobuild-v17.0.0-rc0" }
-osmosis-test-tube = "17.0.0-rc0"
+osmosis-std = "0.19.0"
+osmosis-test-tube = "19.0.0"
 proc-macro2 = "1.0"
 prost = "0.11"
 quote = "1.0"
@@ -71,9 +71,7 @@ sg-multi-test = "3.1.0"
 syn = { version = "1.0", features = ["derive"] }
 test-context = "0.1"
 thiserror = { version = "1.0" }
-# TODO use upstream when PR merged and new release tagged: https://github.com/CosmWasm/cw-multi-test/pull/51
-token-bindings = { git = "https://github.com/CosmosContracts/token-bindings.git", rev = "0cd084b68172ffc9af29eb37fb915392ce351954" }
-token-bindings-test = { git = "https://github.com/CosmosContracts/token-bindings.git", rev = "0cd084b68172ffc9af29eb37fb915392ce351954" }
+token-bindings = "0.11.0"
 wynd-utils = "0.4"
 
 # One commit ahead of version 0.3.0. Allows initialization with an
@@ -94,28 +92,27 @@ cw721-controllers = { path = "./packages/cw721-controllers", version = "2.2.0" }
 cw721-roles = { path = "./contracts/external/cw721-roles", version = "*" }
 dao-cw721-extensions = { path = "./packages/dao-cw721-extensions", version = "*" }
 dao-dao-core = { path = "./contracts/dao-dao-core", version = "2.2.0" }
-dao-interface = { path = "./packages/dao-interface", version = "2.2.0" }
 dao-dao-macros = { path = "./packages/dao-dao-macros", version = "2.2.0" }
+dao-hooks = { path = "./packages/dao-hooks", version = "*" }
+dao-interface = { path = "./packages/dao-interface", version = "2.2.0" }
 dao-pre-propose-approval-single = { path = "./contracts/pre-propose/dao-pre-propose-approval-single", version = "2.2.0" }
 dao-pre-propose-approver = { path = "./contracts/pre-propose/dao-pre-propose-approver", version = "2.2.0" }
 dao-pre-propose-base = { path = "./packages/dao-pre-propose-base", version = "2.2.0" }
 dao-pre-propose-multiple = { path = "./contracts/pre-propose/dao-pre-propose-multiple", version = "2.2.0" }
 dao-pre-propose-single = { path = "./contracts/pre-propose/dao-pre-propose-single", version = "2.2.0" }
 dao-proposal-condorcet = { path = "./contracts/proposal/dao-proposal-condorcet", version = "2.2.0" }
-dao-proposal-hooks = { path = "./packages/dao-proposal-hooks", version = "2.2.0" }
 dao-proposal-multiple = { path = "./contracts/proposal/dao-proposal-multiple", version = "2.2.0" }
 dao-proposal-single = { path = "./contracts/proposal/dao-proposal-single", version = "2.2.0" }
 dao-proposal-sudo = { path = "./test-contracts/dao-proposal-sudo", version = "2.2.0" }
+dao-proposal-hook-counter = { path = "./test-contracts/dao-proposal-hook-counter", version = "2.2.0" }
 dao-testing = { path = "./packages/dao-testing", version = "2.2.0" }
-dao-vote-hooks = { path = "./packages/dao-vote-hooks", version = "2.2.0" }
 dao-voting = { path = "./packages/dao-voting", version = "2.2.0" }
 dao-voting-cw20-balance = { path = "./test-contracts/dao-voting-cw20-balance", version = "2.2.0" }
 dao-voting-cw20-staked = { path = "./contracts/voting/dao-voting-cw20-staked", version = "2.2.0" }
 dao-voting-cw4 = { path = "./contracts/voting/dao-voting-cw4", version = "2.2.0" }
 dao-voting-cw721-roles = { path = "./contracts/voting/dao-voting-cw721-roles", version = "*" }
 dao-voting-cw721-staked = { path = "./contracts/voting/dao-voting-cw721-staked", version = "2.2.0" }
-dao-voting-native-staked = { path = "./contracts/voting/dao-voting-native-staked", version = "2.2.0" }
-dao-voting-token-factory-staked = { path = "./contracts/voting/dao-voting-token-factory-staked", version = "2.2.0" }
+dao-voting-token-staked = { path = "./contracts/voting/dao-voting-token-staked", version = "2.2.0" }
 
 # v1 dependencies. used for state migrations.
 cw-core-v1 = { package = "cw-core", version = "0.1.0" }
@@ -129,6 +126,6 @@ cw4-voting-v1 = { package = "cw4-voting", version = "0.1.0" }
 voting-v1 = { package = "dao-voting", version = "0.1.0" }
 stake-cw20-v03 = { package = "stake-cw20", version = "0.2.6" }
 
-# TODO remove when upstream PR merged and new release tagged: https://github.com/CosmWasm/cw-multi-test/pull/51
+# TODO remove when new release is tagged upstream
 [patch.crates-io]
-cw-multi-test = { git = "https://github.com/JakeHartnell/cw-multi-test.git", branch = "bank-supply-support" }
+cw-multi-test = { git = "https://github.com/CosmWasm/cw-multi-test.git", rev = "d38db7752b9f054c395d6108453f8b321e4cab02" }
diff --git a/ci/integration-tests/src/helpers/helper.rs b/ci/integration-tests/src/helpers/helper.rs
@@ -118,15 +118,16 @@ pub fn create_dao(
         .unwrap();
 
     let ProposalCreationPolicy::Module { addr: pre_propose } = chain
-	.orc
-	.query(
-	    "dao_proposal_single",
-	    &dao_proposal_single::msg::QueryMsg::ProposalCreationPolicy {}
-	).unwrap()
-	.data()
-	.unwrap()
+        .orc
+        .query(
+            "dao_proposal_single",
+            &dao_proposal_single::msg::QueryMsg::ProposalCreationPolicy {},
+        )
+        .unwrap()
+        .data()
+        .unwrap()
     else {
-	panic!("expected pre-propose module")
+        panic!("expected pre-propose module")
     };
     chain
         .orc

diff --git a/ci/integration-tests/src/tests/dao_voting_cw721_staked_test.rs b/ci/integration-tests/src/tests/dao_voting_cw721_staked_test.rs
@@ -1,7 +1,6 @@
 use cosm_orc::orchestrator::{ExecReq, SigningKey};
 use cosmwasm_std::{Binary, Empty, Uint128};
 use cw_utils::Duration;
-use dao_interface::state::Admin;
 use test_context::test_context;
 
 use dao_voting_cw721_staked as module;
@@ -38,7 +37,6 @@ pub fn instantiate_cw721_base(chain: &mut Chain, key: &SigningKey, minter: &str)
 
 fn setup_test(
     chain: &mut Chain,
-    owner: Option<Admin>,
     unstaking_duration: Option<Duration>,
     key: &SigningKey,
     minter: &str,
@@ -50,7 +48,6 @@ fn setup_test(
             CONTRACT_NAME,
             "instantiate_dao_voting_cw721_staked",
             &module::msg::InstantiateMsg {
-                owner,
                 nft_contract: module::msg::NftContract::Existing {
                     address: cw721.clone(),
                 },
@@ -169,7 +166,7 @@ fn cw721_stake_tokens(chain: &mut Chain) {
     let user_addr = chain.users["user1"].account.address.clone();
     let user_key = chain.users["user1"].key.clone();
 
-    let CommonTest { module, .. } = setup_test(chain, None, None, &user_key, &user_addr);
+    let CommonTest { module, .. } = setup_test(chain, None, &user_key, &user_addr);
 
     mint_and_stake_nft(chain, &user_key, &user_addr, &module, "a");
 
@@ -202,13 +199,8 @@ fn cw721_stake_max_claims_works(chain: &mut Chain) {
     let user_addr = chain.users["user1"].account.address.clone();
     let user_key = chain.users["user1"].key.clone();
 
-    let CommonTest { module, .. } = setup_test(
-        chain,
-        None,
-        Some(Duration::Height(1)),
-        &user_key,
-        &user_addr,
-    );
+    let CommonTest { module, .. } =
+        setup_test(chain, Some(Duration::Height(1)), &user_key, &user_addr);
 
     // Create `MAX_CLAIMS` claims.
 

diff --git a/contracts/dao-dao-core/schema/dao-dao-core.json b/contracts/dao-dao-core/schema/dao-dao-core.json
@@ -1029,7 +1029,7 @@
                     ]
                   },
                   "channel_id": {
-                    "description": "exisiting channel to send the tokens over",
+                    "description": "existing channel to send the tokens over",
                     "type": "string"
                   },
                   "timeout": {
@@ -1147,7 +1147,7 @@
             "minimum": 0.0
           },
           "revision": {
-            "description": "the version that the client is currently on (eg. after reseting the chain this could increment 1 as height drops to 0)",
+            "description": "the version that the client is currently on (e.g. after resetting the chain this could increment 1 as height drops to 0)",
             "type": "integer",
             "format": "uint64",
             "minimum": 0.0
@@ -1398,7 +1398,7 @@
                     }
                   },
                   "label": {
-                    "description": "A human-readbale label for the contract",
+                    "description": "A human-readable label for the contract.\n\nValid values should: - not be empty - not be bigger than 128 bytes (or some chain-specific limit) - not start / end with whitespace",
                     "type": "string"
                   },
                   "msg": {

diff --git a/contracts/external/cw-tokenfactory-issuer/Cargo.toml b/contracts/external/cw-tokenfactory-issuer/Cargo.toml
@@ -37,8 +37,9 @@ test-tube = []
 cosmwasm-schema = { workspace = true }
 cosmwasm-std = { workspace = true }
 cosmwasm-storage = { workspace = true }
-cw-storage-plus = { workspace = true }
 cw2 = { workspace = true }
+cw-ownable = { workspace = true }
+cw-storage-plus = { workspace = true }
 osmosis-std = { workspace = true }
 prost = { workspace = true }
 schemars = { workspace = true }

diff --git a/contracts/external/cw-tokenfactory-issuer/README.md b/contracts/external/cw-tokenfactory-issuer/README.md
@@ -6,27 +6,28 @@ This repo contains a set of contracts that when used in conjunction with the x/t
 - Creating a new Token Factory token or using an existing one
 - Granting and revoking allowances for the minting and burning of tokens
 - Updating token metadata
-- Freezing and unfreezing transfers, with a whitelist to allow some address to continue to transfer
-- Force transfering tokens via the contract owner
 - Updating the contract owner or Token Factory admin
+- And more! (see [Advanced Features](#advanced-features))
 
 It is intended to work on multiple chains supporting Token Factory, and has been tested on Juno Network and Osmosis.
 
-The contract has an owner (which can be removed or updated via `ExecuteMsg::ChangeContractOwner {}`), but it can delegate capabilities to other acccounts. For example, the owner of a contract can delegate minting allowance of 1000 tokens to a new address.
+The contract has an owner (which can be removed or updated via `ExecuteMsg::UpdateOwnership {}`), but it can delegate capabilities to other acccounts. For example, the owner of a contract can delegate minting allowance of 1000 tokens to a new address. 
 
-The contract is also the admin of the newly created Token Factory denom. For minting and burning, users then interact with the contract using its own ExecuteMsgs which trigger the contract's access control logic, and the contract then dispatches tokenfactory sdk.Msgs from its own contract account.
+Ownership functionality for this contract is implemented using the `cw-ownable` library.
 
-NOTE: this contract contains a `SudoMsg::BlockBeforeSend` hook that allows for the blacklisting of specific accounts as well as the freezing of all transfers if necessary. This feature is not enabled on every chain using Token Factory, and so blacklisting and freezing features are disabled if `MsgBeforeSendHook` is not supported. DAOs wishing to leverage these features on chains after support is added can call `ExecuteMsg::SetBeforeSendHook {}`.
+The `cw_tokenfactory_issuer` contract is also the admin of newly created Token Factory denoms. For minting and burning, users then interact with the contract using its own ExecuteMsgs which trigger the contract's access control logic, and the contract then dispatches tokenfactory sdk.Msgs from its own contract account.
 
 ## Instantiation
-When instantiating `cw-tokenfactory-issuer`, you can either create a `new_token` or an `existing_token`.
+
+When instantiating `cw-tokenfactory-issuer`, you can either create a `new` or an `existing`.
 
 ### Creating a new Token Factory token
+
 To create a new Token Factory token, simply instantiate the contract with a `subdenom`, this will create a new contract as well as a token with a denom formatted as `factory/{contract_address}/{subdenom}`.
 
 Example instantiate message:
 
-``` json
+```json
 {
   "new_token": {
     "subdenom": "test"
@@ -37,13 +38,67 @@ Example instantiate message:
 All other updates can be preformed afterwards via this contract's `ExecuteMsg` enum. See `src/msg.rs` for available methods.
 
 ### Using an Existing Token
+
 You can also instantiate this contract with an existing token, however most features will not be available until the previous Token Factory admin transfers admin rights to the instantiated contract and optionally calls `ExecuteMsg::SetBeforeSendHook {}` to enable dependent features.
 
 Example instantiate message:
-``` json
+
+```json
 {
   "existing_token": {
     "denom": "factory/{contract_address}/{subdenom}"
   }
 }
 ```
+
+## Renouncing Token Factory Admin
+Some DAOs or protocols after the initial setup phase may wish to render their tokens immutable, permanently disabling features of this contract.
+
+To do so, they must execute a `ExcuteMessage::UpdateTokenFactoryAdmin {}` method, setting the Admin to a null address or the bank module for your respective chain.
+
+For example, on Juno this could be:
+
+``` json
+{
+  "update_token_factory_admin": {
+    "new_admin": "juno1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq"
+  }
+}
+```
+
+The Token Factory standard requires a Token Factory admin per token, by setting to a null address the Token is rendered immutable and the `cw-tokenfactory-issuer` will be unable to make future updates. This is secure as the cryptography that underlies the chain enforces that even with the largest super computers in the world it would take an astonomically large amount of time to compute the private key for this address.
+
+### Advanced Features
+
+This contract supports a number of advanced features which DAOs or token issuers may wist to leverage:
+- Freezing and unfreezing transfers, with an allowlist to allow specified addresses to allow transfer to or from
+- Denylist to prevent certain addresses from transferring
+- Force transfering tokens via the contract owner
+
+**By default, these features are disabled**, and must be explictly enabled by the contract owner (for example via a DAO governance prop).
+
+Moreover, for these features to work, your chain must support the `MsgBeforeSendHook` bank module hook. This is not yet available on every chain using Token Factory, and so denylisting and freezing features are not available if `MsgBeforeSendHook` is not supported.
+
+On chains where `MsgBeforeSendHook` is supported, DAOs or issuers wishing to leverage these features must set the before send hook with `ExecuteMsg::SetBeforeSendHook {}`.
+
+This method takes a `cosmwasm_address`, which is the address of a contract implement a `SudoMsg::BlockBeforeSend` entrypoint. Normally this will be the address of the `cw_tokenfactory_issuer` contract itself, but it is possible to specify a custom contract. This contract contains a `SudoMsg::BlockBeforeSend` hook that allows for the denylisting of specific accounts as well as the freezing of all transfers if necessary. 
+
+Example message to set before send hook:
+``` json
+{
+  "set_before_send_hook": {
+    "cosmwasm_address": "<address of your cw_tokenfactory_issuer contract>"
+  }
+}
+```
+
+DAOs or issuers wishing to leverage these features on chains without support can call `ExecuteMsg::SetBeforeSendHook {}` when support is added.
+
+If a DAO or issuer wishes to disable and removed before send hook related functionality, they simply need to call `ExecuteMsg::SetBeforeSendHook {}` with an empty string for the `cosmwasm_address` like so:
+``` json
+{
+  "set_before_send_hook": {
+    "cosmwasm_address": ""
+  }
+}
+```