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

[JakeHartnell]

Summary

Four deploy-blocking issues in cw721-roles and dao-voting-cw721-roles, surfaced while reviewing the contracts for a mainnet membership-NFT DAO. All four are fixed in a single commit. The fixes are surgical: 8 files, +197 / -83.

#	Severity	Where	What's wrong
1	Critical	dao-voting-cw721-roles/src/contract.rs (reply handler)	Two-phase cw_ownable handover never completes. Reply handler initiates TransferOwnership to the DAO core but never arranges for AcceptOwnership, so cw721-roles ownership permanently sticks with the voting module — which returns NoExecute{} from its own execute() entry point. Result: after instantiate, no Mint/Burn/UpdateTokenWeight call on cw721-roles will ever succeed again. The DAO is bricked at the membership layer from block 1.
2	Critical	cw721-roles/src/contract.rs execute_transfer / execute_send	The handlers mutate the tokens map but never touch the MEMBERS weight map or fire MemberChangedHookMsg. Any successful transfer leaves the old owner with voting weight they don't have NFT backing for, and the new owner with the NFT but no weight. The cw4 view returned by dao-voting-cw721-roles becomes permanently incorrect.
3	High	cw721-roles/src/contract.rs	The crate description claims "non-transferable" but soulbound is only enforced by assert_owner(sender). Under the standard DAO-as-owner deployment, a passed proposal can transfer any member NFT to any address. test_minting_and_transfer_permissions and test_send_permissions literally asserted these transfers succeeded — so the bug was locked in by CI.
4	High	both contracts	No migrate() entry point. The standard remediation for any of the above post-deploy is a wasm migration; without migrate() there is no recovery path.

These are correlated: #1 + #4 are why a deployed instance can't recover; #2 + #3 are symptoms of the same gap in transfer-handler completeness.

Why #1 is non-obvious

cw_ownable::Action::TransferOwnership is two-phase since 0.4: the current owner initiates (sets pending_owner), and the new owner must explicitly call AcceptOwnership to finalize. The reply handler at contracts/voting/dao-voting-cw721-roles/src/contract.rs (prior to this PR) emits TransferOwnership as a WasmMsg::Execute, but the new owner — the dao-dao-core that instantiated this voting module — never gets told to accept.

The natural place to chain AcceptOwnership is the ModuleInstantiateCallback channel: dao-dao-core's VOTE_MODULE_INSTANTIATE_REPLY_ID handler already decodes res.data as ModuleInstantiateCallback and dispatches its msgs as the dao-dao-core itself. This PR puts an AcceptOwnership message into that callback, completing the handover atomically inside the original instantiate transaction.

A subtle second bug only surfaces once AcceptOwnership is wired up: the outer cw_ownable::assert_owner(sender) gate at the top of cw721-roles::execute rejects AcceptOwnership calls. AcceptOwnership must be called by the pending owner, not the current owner, but assert_owner requires sender == current_owner. This PR carves out UpdateOwnership(_) to a match arm that runs before the assert_owner gate and delegates to cw_ownable::update_ownership via cw721-base, which auth-checks each Action variant correctly (TransferOwnership/RenounceOwnership require current owner; AcceptOwnership requires pending owner).

Changes

cw721-roles/src/contract.rs

• New ExecuteMsg::UpdateOwnership(_) match arm before assert_owner. Delegates to cw721-base / cw_ownable::update_ownership, which checks the right sender for each action. Without this, the legitimate AcceptOwnership call from the pending owner fails with NotOwner and the entire two-phase handover is unreachable.
• execute_transfer / execute_send now unconditionally return ContractError::Soulbound{}. This subsumes the MEMBERS-desync bug: no transfer path means no desync path. The contract is now soulbound as a code property, matching the Cargo.toml description.
• Approve / ApproveAll / Revoke / RevokeAll also rejected with Soulbound{}. Granting approval implies transferability and is meaningless under soulbound semantics; rejecting explicitly removes a misleading code path.
• New migrate() entry point. No-op state migration; bumps the cw2 version. Standard pattern.

cw721-roles/src/error.rs

• New Soulbound{} variant.

cw721-roles/src/msg.rs

• New MigrateMsg struct.

cw721-roles/src/tests.rs

• test_minting_and_transfer_permissions and test_send_permissions inverted: now assert that even the minter / contract owner is rejected with ContractError::Soulbound{}. Both tests also check MEMBERS and ownership remain unchanged after rejection, catching any future regression that attempts to slip the desync bug back in.

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

• Reply handler now emits dao_interface::state::ModuleInstantiateCallback via .set_data(...) containing an AcceptOwnership execute message targeted at the new cw721-roles. The parent dao-dao-core's VOTE_MODULE_INSTANTIATE_REPLY_ID handler decodes this and dispatches the message, completing the two-phase ownership handover atomically inside the original instantiate transaction.
• The order of operations is now: voting module mints initial NFTs (still owner) → voting module sends TransferOwnership (sets pending_owner = dao_core) → all voting-module-issued messages drain → dao-dao-core's reply handler fires → dispatches AcceptOwnership as itself → cw721-roles ownership flips to the DAO.
• New migrate() entry point.

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

• New MigrateMsg struct.

dao-voting-cw721-roles/src/testing/mod.rs

• setup_test now performs the AcceptOwnership step explicitly, mocking what dao-dao-core does through the callback channel in production (cw_multi_test does not chain reply data into parent reply handlers the same way). The call is .expect()ed — critical, because the obvious alternative (let _ = app.execute_contract(...)) silently swallows errors and would have hidden the very regression this PR fixes.

dao-voting-cw721-roles/src/testing/tests.rs

• test_voting_queries updated to expect cw721-roles ownership belongs to CREATOR_ADDR (the DAO) after handover. Previously it asserted ownership remained with the voting module, which is the bug.

Backwards compatibility

• cw721-roles: anyone currently relying on TransferNft/SendNft/Approve*/Revoke* to succeed under any caller will see those calls fail with Soulbound{}. The crate has always been documented as non-transferable; this PR makes the documentation true. If a downstream deployment was depending on the bug, the right fix downstream is to stop relying on it.
• dao-voting-cw721-roles: any existing test setup that worked around the missing handover by mocking ownership differently will need to either remove that workaround or update assertions to expect the DAO as owner. The setup_test change here demonstrates the pattern. Production deployments instantiated through dao-dao-core will start working correctly with no caller-side change.
• Migration: both contracts gain migrate() entry points with no state migration. The cw2 version bumps to the current crate version. Already-deployed instances cannot be migrated to this code without an admin update, but new instantiates pick up the fixes without ceremony.

Test plan

• cargo check -p cw721-roles -p dao-voting-cw721-roles --lib — clean against development
• In-tree tests updated; the previously-incorrect transfer-success assertions now assert Soulbound{} rejection
• Out-of-tree end-to-end smoke tests (independent crate, no dao-testing dep) instantiate the full dao-voting-cw721-roles → cw721-roles chain via cw_multi_test, fire AcceptOwnership from the simulated DAO, and verify:
  • Ownership lands with the DAO after handover
  • The DAO can mint new NFTs post-handover
  • The voting module loses mint rights post-handover
  • RenounceOwnership by the DAO still works (governance flexibility)
  • Transfer/Send are rejected with Soulbound{} even when called by the minter
  • MEMBERS weight map stays consistent after rejected transfers
  • migrate() succeeds and bumps cw2 version
• Reviewer to run the existing cargo test -p cw721-roles -p dao-voting-cw721-roles in CI to confirm no regressions in the test environment that has dao-testing's full dependency chain available

Operational note (no code change in this PR)

cw721-roles hook dispatch uses SubMsg::new (i.e. ReplyOn::Never), so any registered hook contract that panics or runs out of gas will revert the parent Mint/Burn/UpdateTokenWeight transaction. dao-voting-cw721-roles does not need hooks — it live-queries cw721-roles on every voting-power request and holds no cached membership state. Recommend documenting this in the cw721-roles README or treating AddHook as a power that requires dedicated audit of the hook contract. Not changed here — flagging for a follow-up.

[codecov]

Codecov Report

❌ Patch coverage is 73.77049% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.05%. Comparing base (0178cf5) to head (0432df1).

Files with missing lines	Patch %	Lines
contracts/external/cw721-roles/src/contract.rs	70.37%	8 Missing ⚠️
...acts/voting/dao-voting-cw721-roles/src/contract.rs	65.21%	8 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff               @@
##           development     #930      +/-   ##
===============================================
- Coverage        96.56%   92.05%   -4.52%     
===============================================
  Files              199      159      -40     
  Lines            67407    28797   -38610     
===============================================
- Hits             65094    26510   -38584     
+ Misses            2313     2287      -26     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.