cw721-roles + dao-voting-cw721-roles: enforce soulbound, complete two…

contracts/external/cw721-roles/src/contract.rs

@@ -8,14 +8,14 @@ use cw4::{
     Member, MemberChangedHookMsg, MemberDiff, MemberListResponse, MemberResponse,
     TotalWeightResponse,
 };
-use cw721::{Cw721ReceiveMsg, NftInfoResponse, OwnerOfResponse};
+use cw721::{NftInfoResponse, OwnerOfResponse};
 use cw721_base::{Cw721Contract, InstantiateMsg as Cw721BaseInstantiateMsg};
 use cw_storage_plus::Bound;
 use cw_utils::maybe_addr;
 use dao_cw721_extensions::roles::{ExecuteExt, MetadataExt, QueryExt};
 use std::cmp::Ordering;
 
-use crate::msg::{ExecuteMsg, QueryMsg};
+use crate::msg::{ExecuteMsg, MigrateMsg, QueryMsg};
 use crate::state::{MEMBERS, TOTAL};
 use crate::{error::RolesContractError as ContractError, state::HOOKS};
 
@@ -55,7 +55,34 @@ pub fn execute(
     info: MessageInfo,
     msg: ExecuteMsg,
 ) -> Result<Response, ContractError> {
-    // Only owner / minter can execute
+    // UpdateOwnership has its own auth model — each Action is auth-checked
+    // separately by cw_ownable::update_ownership (current owner for
+    // TransferOwnership/RenounceOwnership; pending_owner for AcceptOwnership).
+    // It MUST be matched BEFORE the outer assert_owner gate below, otherwise
+    // the legitimate `AcceptOwnership` call from the pending owner (the DAO
+    // core during the dao-voting-cw721-roles bootstrap handover) is rejected
+    // with NotOwner and the cw721-roles ownership never transfers.
+    if let ExecuteMsg::UpdateOwnership(_) = &msg {
+        return Cw721Roles::default()
+            .execute(deps, env, info, msg)
+            .map_err(Into::into);
+    }
+
+    // Soulbound NFTs have no use for approvals — even though TransferNft and
+    // SendNft are blocked below, granting/revoking approvals would suggest
+    // transferability to anything that introspects the contract. Reject
+    // explicitly for design hygiene.
+    if matches!(
+        &msg,
+        ExecuteMsg::Approve { .. }
+            | ExecuteMsg::Revoke { .. }
+            | ExecuteMsg::ApproveAll { .. }
+            | ExecuteMsg::RevokeAll { .. }
+    ) {
+        return Err(ContractError::Soulbound {});
+    }
+
+    // Only owner / minter can execute the remaining mutating handlers.
     cw_ownable::assert_owner(deps.storage, &info.sender)?;
 
     match msg {
@@ -229,55 +256,29 @@ pub fn execute_burn(
 }
 
 pub fn execute_transfer(
-    deps: DepsMut,
+    _deps: DepsMut,
     _env: Env,
-    info: MessageInfo,
-    recipient: String,
-    token_id: String,
+    _info: MessageInfo,
+    _recipient: String,
+    _token_id: String,
 ) -> Result<Response, ContractError> {
-    let contract = Cw721Roles::default();
-
-    let mut token = contract.tokens.load(deps.storage, &token_id)?;
-    // set owner and remove existing approvals
-    token.owner = deps.api.addr_validate(&recipient)?;
-    token.approvals = vec![];
-    contract.tokens.save(deps.storage, &token_id, &token)?;
-
-    Ok(Response::new()
-        .add_attribute("action", "transfer_nft")
-        .add_attribute("sender", info.sender)
-        .add_attribute("recipient", recipient)
-        .add_attribute("token_id", token_id))
+    // Soulbound: NFTs represent on-chain identity / role membership and must
+    // not change hands. Reject all transfers even when called by the contract
+    // owner (the DAO), since a DAO-initiated transfer would also desync the
+    // cw4 voting weight map maintained by mint/burn/update_weight handlers.
+    Err(ContractError::Soulbound {})
 }
 
 pub fn execute_send(
-    deps: DepsMut,
+    _deps: DepsMut,
     _env: Env,
-    info: MessageInfo,
-    token_id: String,
-    recipient_contract: String,
-    msg: Binary,
+    _info: MessageInfo,
+    _token_id: String,
+    _recipient_contract: String,
+    _msg: Binary,
 ) -> Result<Response, ContractError> {
-    let contract = Cw721Roles::default();
-
-    let mut token = contract.tokens.load(deps.storage, &token_id)?;
-    // set owner and remove existing approvals
-    token.owner = deps.api.addr_validate(&recipient_contract)?;
-    token.approvals = vec![];
-    contract.tokens.save(deps.storage, &token_id, &token)?;
-
-    let send = Cw721ReceiveMsg {
-        sender: info.sender.to_string(),
-        token_id: token_id.clone(),
-        msg,
-    };
-
-    Ok(Response::new()
-        .add_message(send.into_cosmos_msg(recipient_contract.clone())?)
-        .add_attribute("action", "send_nft")
-        .add_attribute("sender", info.sender)
-        .add_attribute("recipient", recipient_contract)
-        .add_attribute("token_id", token_id))
+    // See execute_transfer — soulbound, no transfer paths.
+    Err(ContractError::Soulbound {})
 }
 
 pub fn execute_add_hook(
@@ -471,6 +472,18 @@ pub fn query_member(deps: Deps, addr: String, height: Option<u64>) -> StdResult<
     Ok(MemberResponse { weight })
 }
 
+#[cfg_attr(not(feature = "library"), entry_point)]
+pub fn migrate(deps: DepsMut, _env: Env, _msg: MigrateMsg) -> Result<Response, ContractError> {
+    // No state migration logic — just bump the cw2 version so this contract
+    // can be MigrateContract'd by the wasm-level admin (e.g. an x/gov proposal
+    // on chain-governed deployments) if a future bug requires a patched wasm.
+    cw2::set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?;
+    Ok(Response::default()
+        .add_attribute("action", "migrate")
+        .add_attribute("contract_name", CONTRACT_NAME)
+        .add_attribute("contract_version", CONTRACT_VERSION))
+}
+
 pub fn query_list_members(
     deps: Deps,
     start_after: Option<String>,

contracts/external/cw721-roles/src/error.rs

@@ -26,4 +26,7 @@ pub enum RolesContractError {
 
     #[error("The submitted weight is equal to the previous value, no change will occur")]
     NoWeightChange {},
+
+    #[error("cw721-roles NFTs are soulbound and cannot be transferred or sent")]
+    Soulbound {},
 }

contracts/external/cw721-roles/src/msg.rs

@@ -1,5 +1,9 @@
+use cosmwasm_schema::cw_serde;
 use dao_cw721_extensions::roles::{ExecuteExt, MetadataExt, QueryExt};
 
 pub type InstantiateMsg = cw721_base::InstantiateMsg;
 pub type ExecuteMsg = cw721_base::ExecuteMsg<MetadataExt, ExecuteExt>;
 pub type QueryMsg = cw721_base::QueryMsg<QueryExt>;
+
+#[cw_serde]
+pub struct MigrateMsg {}

contracts/external/cw721-roles/src/tests.rs

@@ -219,20 +219,52 @@ fn test_minting_and_transfer_permissions() {
     app.execute_contract(Addr::unchecked(DAO), cw721_addr.clone(), &msg, &[])
         .unwrap();
 
-    // Non-minter can't transfer
-    let msg = ExecuteMsg::TransferNft {
+    // Soulbound: nobody can transfer, not even the DAO (which owns the contract).
+    //
+    // We assert against the error's Display string rather than `.downcast()` to
+    // `RolesContractError`: dao-testing depends on cw721-roles and is also a
+    // dev-dependency of cw721-roles, so the unit-test build ends up with the
+    // contract's error type compiled in two different cargo units. The TypeIds
+    // do not match across those units even though the source is the same, so a
+    // typed downcast inside `src/tests.rs` always returns None.
+    let transfer_msg = ExecuteMsg::TransferNft {
         recipient: BOB.to_string(),
         token_id: "1".to_string(),
     };
-    app.execute_contract(Addr::unchecked(ALICE), cw721_addr.clone(), &msg, &[])
+    let err = app
+        .execute_contract(
+            Addr::unchecked(ALICE),
+            cw721_addr.clone(),
+            &transfer_msg,
+            &[],
+        )
         .unwrap_err();
+    let chain: Vec<String> = err.chain().map(|c| format!("{c}")).collect();
+    assert!(
+        chain
+            .iter()
+            .any(|s| s.contains("not the contract's current owner")),
+        "expected NotOwner error for alice, got chain: {chain:?}",
+    );
 
-    // DAO can transfer
-    app.execute_contract(Addr::unchecked(DAO), cw721_addr.clone(), &msg, &[])
-        .unwrap();
+    let err = app
+        .execute_contract(Addr::unchecked(DAO), cw721_addr.clone(), &transfer_msg, &[])
+        .unwrap_err();
+    let chain: Vec<String> = err.chain().map(|c| format!("{c}")).collect();
+    assert!(
+        chain.iter().any(|s| s.contains("soulbound")),
+        "expected Soulbound error for DAO, got chain: {chain:?}",
+    );
 
+    // Ownership unchanged after rejected transfers.
     let owner: OwnerOfResponse = query_nft_owner(&app, &cw721_addr, "1").unwrap();
-    assert_eq!(owner.owner, BOB);
+    assert_eq!(owner.owner, ALICE);
+
+    // And the cw4 weight map is still in sync — Alice has weight 1, Bob has none.
+    let alice: MemberResponse = query_member(&app, &cw721_addr, ALICE, None).unwrap();
+    assert_eq!(alice.weight, Some(1));
+    let bob: MemberResponse = query_member(&app, &cw721_addr, BOB, None).unwrap();
+    assert_eq!(bob.weight, None);
 }
 
 #[test]
@@ -272,22 +304,37 @@ fn test_send_permissions() {
         )
         .unwrap();
 
-    // Non-minter can't send
-    let msg = ExecuteMsg::SendNft {
+    // Soulbound: SendNft is also rejected, even when called by the DAO/minter.
+    // See `test_minting_and_transfer_permissions` for why we match on the
+    // error chain's Display string instead of `.downcast()`.
+    let send_msg = ExecuteMsg::SendNft {
         contract: cw721_staked_addr.to_string(),
         token_id: "1".to_string(),
         msg: to_json_binary(&Binary::default()).unwrap(),
     };
-    app.execute_contract(Addr::unchecked(ALICE), cw721_addr.clone(), &msg, &[])
+    let err = app
+        .execute_contract(Addr::unchecked(ALICE), cw721_addr.clone(), &send_msg, &[])
         .unwrap_err();
+    let chain: Vec<String> = err.chain().map(|c| format!("{c}")).collect();
+    assert!(
+        chain
+            .iter()
+            .any(|s| s.contains("not the contract's current owner")),
+        "expected NotOwner error for alice, got chain: {chain:?}",
+    );
 
-    // DAO can send
-    app.execute_contract(Addr::unchecked(DAO), cw721_addr.clone(), &msg, &[])
-        .unwrap();
+    let err = app
+        .execute_contract(Addr::unchecked(DAO), cw721_addr.clone(), &send_msg, &[])
+        .unwrap_err();
+    let chain: Vec<String> = err.chain().map(|c| format!("{c}")).collect();
+    assert!(
+        chain.iter().any(|s| s.contains("soulbound")),
+        "expected Soulbound error for DAO, got chain: {chain:?}",
+    );
 
-    // Staking contract now owns the NFT
+    // Alice still owns the NFT.
     let owner: OwnerOfResponse = query_nft_owner(&app, &cw721_addr, "1").unwrap();
-    assert_eq!(owner.owner, cw721_staked_addr.as_str());
+    assert_eq!(owner.owner, ALICE);
 }
 
 #[test]

contracts/voting/dao-voting-cw721-roles/src/contract.rs

@@ -1,8 +1,8 @@
 #[cfg(not(feature = "library"))]
 use cosmwasm_std::entry_point;
 use cosmwasm_std::{
-    to_json_binary, Binary, Deps, DepsMut, Empty, Env, MessageInfo, Reply, Response, StdResult,
-    SubMsg, WasmMsg,
+    to_json_binary, Binary, CosmosMsg, Deps, DepsMut, Empty, Env, MessageInfo, Reply, Response,
+    StdResult, SubMsg, WasmMsg,
 };
 use cw2::set_contract_version;
 use cw4::{MemberResponse, TotalWeightResponse};
@@ -12,9 +12,9 @@ use cw721_base::{
 use cw_ownable::Action;
 use cw_utils::parse_reply_instantiate_data;
 use dao_cw721_extensions::roles::{ExecuteExt, MetadataExt, QueryExt};
-use dao_interface::state::{Admin, ModuleInstantiateInfo};
+use dao_interface::state::{Admin, ModuleInstantiateCallback, ModuleInstantiateInfo};
 
-use crate::msg::{ExecuteMsg, InstantiateMsg, NftContract, QueryMsg};
+use crate::msg::{ExecuteMsg, InstantiateMsg, MigrateMsg, NftContract, QueryMsg};
 use crate::state::{Config, CONFIG, DAO, INITIAL_NFTS};
 use crate::ContractError;
 
@@ -165,6 +165,18 @@ pub fn query_info(deps: Deps) -> StdResult<Binary> {
     to_json_binary(&dao_interface::voting::InfoResponse { info })
 }
 
+#[cfg_attr(not(feature = "library"), entry_point)]
+pub fn migrate(deps: DepsMut, _env: Env, _msg: MigrateMsg) -> Result<Response, ContractError> {
+    // No state migration logic — just bump the cw2 version so this contract
+    // can be MigrateContract'd by the wasm-level admin if a future bug requires
+    // a patched wasm.
+    set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?;
+    Ok(Response::default()
+        .add_attribute("action", "migrate")
+        .add_attribute("contract_name", CONTRACT_NAME)
+        .add_attribute("contract_version", CONTRACT_VERSION))
+}
+
 #[cfg_attr(not(feature = "library"), entry_point)]
 pub fn reply(deps: DepsMut, _env: Env, msg: Reply) -> Result<Response, ContractError> {
     match msg.id {
@@ -208,8 +220,27 @@ pub fn reply(deps: DepsMut, _env: Env, msg: Reply) -> Result<Response, ContractE
                     // Clear space
                     INITIAL_NFTS.remove(deps.storage);
 
-                    // Update minter message
-                    let update_minter_msg = WasmMsg::Execute {
+                    // cw-ownable's TransferOwnership is a two-phase handshake:
+                    // (1) the current owner (this voting module) initiates the
+                    //     transfer, which sets `pending_owner = dao` without
+                    //     changing the actual owner; (2) the new owner (the DAO
+                    //     core) must explicitly accept.
+                    //
+                    // The voting module's own execute() returns NoExecute, so
+                    // the voting module cannot call AcceptOwnership itself.
+                    // Instead we hand the AcceptOwnership message to the DAO
+                    // core via the ModuleInstantiateCallback channel — the DAO
+                    // core's VOTE_MODULE_INSTANTIATE_REPLY_ID handler reads
+                    // `res.data`, decodes it as ModuleInstantiateCallback, and
+                    // dispatches `msgs` as itself. That makes
+                    // info.sender = dao_core when AcceptOwnership lands on
+                    // cw721-roles, satisfying the pending_owner check.
+                    //
+                    // Without this, cw721-roles' owner remains this voting
+                    // module forever, and no future Mint/Burn/UpdateWeight
+                    // proposal can succeed — the DAO would be bricked at the
+                    // membership layer from block 1.
+                    let initiate_transfer_msg = WasmMsg::Execute {
                         contract_addr: nft_contract.clone(),
                         msg: to_json_binary(
                             &Cw721ExecuteMsg::<MetadataExt, ExecuteExt>::UpdateOwnership(
@@ -222,11 +253,27 @@ pub fn reply(deps: DepsMut, _env: Env, msg: Reply) -> Result<Response, ContractE
                         funds: vec![],
                     };
 
+                    let accept_ownership_msg: CosmosMsg = WasmMsg::Execute {
+                        contract_addr: nft_contract.clone(),
+                        msg: to_json_binary(
+                            &Cw721ExecuteMsg::<MetadataExt, ExecuteExt>::UpdateOwnership(
+                                Action::AcceptOwnership {},
+                            ),
+                        )?,
+                        funds: vec![],
+                    }
+                    .into();
+
+                    let callback = ModuleInstantiateCallback {
+                        msgs: vec![accept_ownership_msg],
+                    };
+
                     Ok(Response::default()
                         .add_attribute("method", "instantiate")
                         .add_attribute("nft_contract", nft_contract)
-                        .add_message(update_minter_msg)
-                        .add_submessages(mint_submessages))
+                        .add_message(initiate_transfer_msg)
+                        .add_submessages(mint_submessages)
+                        .set_data(to_json_binary(&callback)?))
                 }
                 Err(_) => Err(ContractError::NftInstantiateError {}),
             }

contracts/voting/dao-voting-cw721-roles/src/msg.rs

@@ -49,6 +49,9 @@ pub struct InstantiateMsg {
 #[cw_serde]
 pub enum ExecuteMsg {}
 
+#[cw_serde]
+pub struct MigrateMsg {}
+
 #[voting_module_query]
 #[cw_serde]
 #[derive(QueryResponses)]