feat(commitment_nft): initialize auth model — document single deployer assumption and operational checklist by Tijesunimi004 · Pull Request #460 · Commitlabs-Org/Commitlabs-Contracts

Tijesunimi004 · 2026-03-31T03:40:55Z

Closes #220

What changed

Security fix: initialize now calls admin.require_auth() before writing any state. Previously the function had no auth check, meaning any address could have called it and registered a different admin (front-run the deployer). Now only a transaction signed by the admin key can succeed.

Documentation (lib.rs):

Module-level //! block covering: auth model, single deployer assumption, trust boundary table (admin / core / whitelisted minter / NFT owner / public), and the flag-based reentrancy guard.
Expanded initialize Rustdoc with NatSpec-style Parameters, Errors, and Security Notes sections. Notes explain why require_auth() is placed after the AlreadyInitialized guard (so a retry does not leak contract state or incur unnecessary auth cost).

Tests (tests.rs):

test_initialize_requires_admin_auth — calls mock_all_auths() then checks e.auths() to assert that admin's authorization was recorded during initialize. Directly tests the single deployer assumption.
test_initialize_stores_authorizing_admin — asserts get_admin() returns the address that signed initialize.
setup_contract() now calls e.mock_all_auths() so all existing tests that call initialize without explicit auth setup continue to pass.
Test snapshots updated to reflect the added admin auth invocation.

Docs (docs/CONTRACT_FUNCTIONS.md):

initialize access-control column corrected from "None (single-use)" to "admin require_auth (single-use)" with a note on front-run prevention.
mint signature updated to include the caller parameter.
New commitment_nft deployment checklist section: seven ordered steps (upload WASM → deploy → initialize atomically → verify admin → set_core_contract → optional whitelist → smoke-test) with an explicit note on the single deployer assumption and the admin key rotation path.

Security notes

Trust boundary: only the single transaction that both deploys and initializes the contract can set the admin. The window for front-running is closed by require_auth() + atomic deployment.
Reentrancy guard is unaffected — initialize does not use it because emergency and pausable controls are meaningless before initialization.
No arithmetic paths were changed; no new cross-contract calls introduced.

Tests

All 73 tests pass:

test result: ok. 73 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

…r assumption and operational checklist - Add admin.require_auth() to initialize, enforcing the single deployer assumption: only the transaction signed by the admin key can initialize the contract. - Add module-level //! documentation covering the auth model, trust boundaries (admin / core / whitelisted minter / owner / public), and the reentrancy guard. - Expand initialize Rustdoc with NatSpec-style summary, params, errors, and security notes explaining the front-run mitigation. - Add two targeted tests: test_initialize_requires_admin_auth (verifies admin.require_auth() is recorded via e.auths()) and test_initialize_stores_authorizing_admin (verifies the stored admin matches the address that signed initialize). - Add mock_all_auths() to setup_contract() so all existing tests continue to work after require_auth() was added to initialize. - Update CONTRACT_FUNCTIONS.md: fix initialize access-control column from "None" to "admin require_auth"; update mint signature to include the caller parameter; add deployment operational checklist for commitment_nft (upload WASM → deploy → initialize atomically → verify → set_core_contract → smoke-test). - Update test snapshots to reflect the added auth invocation recorded for admin during initialize.

drips-wave · 2026-03-31T03:46:19Z

@Tijesunimi004 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

Copilot

Pull request overview

This PR strengthens the initialization/auth story for commitment_nft by requiring admin authorization during initialize, and expands the documentation and test suite to reflect the intended “single deployer” operational model.

Changes:

Enforce admin.require_auth() during initialize to ensure the admin key authorizes the one-time initialization.
Add/expand auth-model documentation (including a deployment checklist) and update the documented ABI table (notably mint(caller, ...)).
Update tests and Soroban snapshot fixtures to reflect the new initialize auth invocation and the mint signature usage.

Reviewed changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
docs/CONTRACT_FUNCTIONS.md	Updates the function matrix (incl. `initialize` access control, `mint` signature) and adds a deployment checklist.
contracts/commitment_nft/src/lib.rs	Adds module-level auth/reentrancy documentation and adds `admin.require_auth()` to `initialize`.
contracts/commitment_nft/src/tests.rs	Adds tests asserting `initialize` auth behavior; updates mint helpers/call sites to pass `caller`.
contracts/commitment_nft/src/test_zero_address.rs	Updates tests to use the current contract/client names and the `mint(caller, ...)` signature.
contracts/commitment_nft/test_snapshots/tests/test_transfer_not_owner.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_transfer_nonexistent_token.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_token_exists.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_settle.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_settle_not_expired.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_settle_already_settled.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_owner_of.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_owner_of_nonexistent_token.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_mint.1.json	Snapshot update reflecting `initialize` auth recording and updated invocation args.
contracts/commitment_nft/test_snapshots/tests/test_mint_without_initialize_fails.1.json	Snapshot update reflecting updated invocation args formatting.
contracts/commitment_nft/test_snapshots/tests/test_mint_multiple.1.json	Snapshot update reflecting `initialize` auth recording and updated invocation args.
contracts/commitment_nft/test_snapshots/tests/test_metadata_timestamps.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_is_expired.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_is_expired_nonexistent_token.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_is_active.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_is_active_nonexistent_token.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_initialize_twice_fails.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_initialize_stores_authorizing_admin.1.json	New snapshot for admin-storage behavior test.
contracts/commitment_nft/test_snapshots/tests/test_initialize_requires_admin_auth.1.json	New snapshot for auth-recording behavior test.
contracts/commitment_nft/test_snapshots/tests/test_get_nfts_by_owner.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_get_metadata.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_get_metadata_nonexistent_token.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_get_all_metadata.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_get_all_metadata_empty.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_get_admin.1.json	Snapshot update reflecting `initialize` auth recording.
contracts/commitment_nft/test_snapshots/tests/test_balance_updates_after_transfer.1.json	Snapshot update reflecting `initialize` auth recording and ledger nonce differences.
contracts/commitment_nft/test_snapshots/tests/test_balance_of_after_minting.1.json	Snapshot update reflecting `initialize` auth recording.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-31T03:47:25Z

contracts/commitment_nft/src/lib.rs

+    ///   guard, so a second caller cannot learn whether the contract is already
+    ///   initialized without paying for auth on a no-op.


The Security Notes bullet about placing admin.require_auth() after the AlreadyInitialized guard is backwards: because the guard returns before auth, anyone can call initialize and learn whether the contract is initialized without paying auth. If the goal is to avoid auth cost on retries, keep the current ordering but update the doc; if the goal is to avoid leaking initialization status, require_auth() would need to run before the guard (with the tradeoff of auth cost on retries).

Suggested change

/// guard, so a second caller cannot learn whether the contract is already

/// initialized without paying for auth on a no-op.

/// guard. This means any caller can probe whether the contract has

/// already been initialized without paying for auth, but avoids auth

/// cost on legitimate retry attempts.

Copilot · 2026-03-31T03:47:26Z

contracts/commitment_nft/src/lib.rs

+//! All state-mutating functions use a flag-based reentrancy guard
+//! (`DataKey::ReentrancyGuard`). The guard is set on entry and cleared before
+//! every return path (including errors), so nested calls from the same
+//! contract invocation are rejected with `ReentrancyDetected`.


Module docs claim "All state-mutating functions use" the ReentrancyGuard, but several mutating entrypoints (e.g. initialize, pause/unpause, set_core_contract, admin/whitelist management) do not use the guard. Please either narrow the statement (e.g., only NFT lifecycle functions like mint/transfer/settle are guarded) or extend the guard consistently so the documentation matches behavior.

Suggested change

//! All state-mutating functions use a flag-based reentrancy guard

//! (`DataKey::ReentrancyGuard`). The guard is set on entry and cleared before

//! every return path (including errors), so nested calls from the same

//! contract invocation are rejected with `ReentrancyDetected`.

//! All NFT lifecycle state-mutating functions (such as `mint`, `transfer`,

//! and `settle`) use a flag-based reentrancy guard (`DataKey::ReentrancyGuard`).

//! The guard is set on entry and cleared before every return path (including

//! errors), so nested calls from the same contract invocation are rejected

//! with `ReentrancyDetected`.

Copilot · 2026-03-31T03:47:26Z

docs/CONTRACT_FUNCTIONS.md

+| set_core_contract(core_contract) -> Result                                                                                                            | Set authorized core contract.      | Admin require_auth.              | Emits CoreContractSet event.                |
+| get_core_contract() -> Result<Address>                                                                                                                | Fetch core contract address.       | View.                            | Fails if not initialized.                   |
+| get_admin() -> Result<Address>                                                                                                                        | Fetch admin address.               | View.                            | Fails if not initialized.                   |
+| mint(caller, owner, commitment_id, duration_days, max_loss_percent, commitment_type, initial_amount, asset_address, early_exit_penalty) -> Result<u32> | Mint NFT for a commitment.         | caller must be admin, core contract, or whitelisted minter. | Validates inputs and uses reentrancy guard. |


The mint access-control description implies the caller is authenticated, but the current on-chain implementation only compares the supplied caller address to admin/core/whitelist and does not require caller authorization. That means any invoker could pass caller=<admin> and satisfy the role check. To make the doc accurate (and the control effective), the contract should authenticate the minter identity (e.g., caller.require_auth() and/or derive the caller from Env::invoker() instead of a user-supplied parameter) and then the docs can describe the enforced rule.

Copilot · 2026-03-31T03:47:26Z

docs/CONTRACT_FUNCTIONS.md

-| get_core_contract() -> Result<Address>                                                                                                         | Fetch core contract address.       | View.               | Fails if not initialized.                   |
-| get_admin() -> Result<Address>                                                                                                                 | Fetch admin address.               | View.               | Fails if not initialized.                   |
-| mint(owner, commitment_id, duration_days, max_loss_percent, commitment_type, initial_amount, asset_address, early_exit_penalty) -> Result<u32> | Mint NFT for a commitment.         | No require_auth.    | Validates inputs and uses reentrancy guard. |
+| initialize(admin) -> Result                                                                                                                           | Set admin and token counters.      | admin require_auth (single-use). | Returns AlreadyInitialized on repeat. admin must sign the transaction; prevents front-running. |


The initialize notes currently state that requiring admin to sign "prevents front-running". admin.require_auth() ensures the chosen admin controls that key, but by itself it doesn't prevent a third party from calling initialize first with their own address as admin; the actual mitigation for takeover is deploying+initializing atomically (as you describe later in the checklist). Consider rephrasing this note to avoid implying require_auth() alone closes the window.

Copilot · 2026-03-31T03:47:26Z

docs/CONTRACT_FUNCTIONS.md

+
+1. **Upload WASM** — submit the compiled WASM to Stellar and record the hash.
+2. **Deploy contract** — instantiate the contract from the uploaded WASM hash.
+3. **Call `initialize(admin)`** — in the same transaction as deployment (or immediately after, before any other transaction). The `admin` key must sign this transaction. This is the single deployer assumption: the window between `deploy` and `initialize` must be zero or negligible to prevent front-running.


Checklist step 3 allows calling initialize "immediately after" deployment, but later you state deploy+initialize must be atomic (same transaction/XDR) to eliminate the front-run window. If atomicity is a security requirement, consider removing or clearly qualifying the "immediately after" guidance so operators don't treat it as safe on public networks.

…el-document-single-deployer-assumption-an

Copilot AI review requested due to automatic review settings March 31, 2026 03:40

Copilot started reviewing on behalf of Tijesunimi004 March 31, 2026 03:41 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

Merge branch 'master' into feature/commitment-nft-initialize-auth-mod…

a08926b

…el-document-single-deployer-assumption-an

1nonlypiece merged commit 3f6254e into Commitlabs-Org:master Apr 1, 2026
0 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(commitment_nft): initialize auth model — document single deployer assumption and operational checklist#460

feat(commitment_nft): initialize auth model — document single deployer assumption and operational checklist#460
1nonlypiece merged 2 commits intoCommitlabs-Org:masterfrom
Tijesunimi004:feature/commitment-nft-initialize-auth-model-document-single-deployer-assumption-an

Tijesunimi004 commented Mar 31, 2026 •

edited

Loading

Uh oh!

drips-wave bot commented Mar 31, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		/// guard, so a second caller cannot learn whether the contract is already
		/// initialized without paying for auth on a no-op.

-    ///   guard, so a second caller cannot learn whether the contract is already
-    ///   initialized without paying for auth on a no-op.
+    ///   guard. This means any caller can probe whether the contract has
+    ///   already been initialized without paying for auth, but avoids auth
+    ///   cost on legitimate retry attempts.

-//! All state-mutating functions use a flag-based reentrancy guard
-//! (`DataKey::ReentrancyGuard`). The guard is set on entry and cleared before
-//! every return path (including errors), so nested calls from the same
-//! contract invocation are rejected with `ReentrancyDetected`.
+//! All NFT lifecycle state-mutating functions (such as `mint`, `transfer`,
+//! and `settle`) use a flag-based reentrancy guard (`DataKey::ReentrancyGuard`).
+//! The guard is set on entry and cleared before every return path (including
+//! errors), so nested calls from the same contract invocation are rejected
+//! with `ReentrancyDetected`.

Conversation

Tijesunimi004 commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Security notes

Tests

Uh oh!

drips-wave bot commented Mar 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Tijesunimi004 commented Mar 31, 2026 •

edited

Loading