Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
1nonlypiece merged 2 commits into from
Apr 1, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b07723
feat(commitment_nft): initialize auth model — document single deploye…
Tijesunimi004 Mar 31, 2026
a08926b
Merge branch 'master' into feature/commitment-nft-initialize-auth-mod…
1nonlypiece Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 79 additions & 4 deletions contracts/commitment_nft/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,41 @@
//! # Commitment NFT Contract
//!
//! Mints and manages Soroban NFTs that represent user commitments. Each NFT
//! records commitment parameters (duration, max loss, type, amount) and tracks
//! whether the underlying commitment is still active.
//!
//! ## Auth Model and Single Deployer Assumption
//!
//! `initialize` must be called exactly once, immediately after deployment,
//! by the entity that holds the private key of the `admin` address it
//! registers. This is the **single deployer assumption**: the transaction that
//! deploys the contract should also invoke `initialize` in the same
//! transaction or atomically thereafter, so no third party can front-run it
//! with a different admin. `admin.require_auth()` is called inside
//! `initialize` to enforce this — the transaction must be signed by the admin
//! key.
//!
//! Subsequent admin operations (pausing, whitelisting minters, upgrading)
//! require the same `admin.require_auth()` check via the `require_admin`
//! helper.
//!
//! ## Trust Boundaries
//!
//! | Caller role | What they can do |
//! |-------------|-----------------|
//! | Admin | Initialize, pause/unpause, set core contract, manage minter whitelist, upgrade, migrate, emergency mode |
//! | Core contract (`set_core_contract`) | Call `mint` as an authorized minter |
//! | Whitelisted minter (`add_authorized_contract`) | Call `mint` |
//! | NFT owner | `transfer` (inactive NFTs only) |
//! | Anyone | All view functions (`get_metadata`, `owner_of`, etc.) |
//!
//! ## Reentrancy
//!
//! 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`.
Comment on lines +34 to +37
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

#![no_std]
//! Commitment NFT contract.
//!
Expand Down Expand Up @@ -180,7 +218,42 @@ pub struct CommitmentNFTContract;

#[contractimpl]
impl CommitmentNFTContract {
/// Initialize the NFT contract
/// Initialize the commitment NFT contract.
///
/// # Single Deployer Assumption
///
/// This function enforces the single deployer assumption: the caller must
/// control the `admin` address being registered. `admin.require_auth()` is
/// called before any state is written, ensuring the transaction must be
/// signed by the admin key. This prevents front-running attacks where a
/// third party deploys the contract and calls `initialize` with a different
/// admin before the legitimate deployer acts.
///
/// The recommended deployment pattern is to invoke `initialize` in the same
/// transaction as the contract upload, leaving no window for front-running.
///
/// This function may only be called once. Any subsequent call returns
/// [`ContractError::AlreadyInitialized`].
///
/// # Parameters
///
/// - `admin`: Address that will hold admin authority over the contract.
/// Must authorize this transaction via `require_auth`.
///
/// # Errors
///
/// - [`ContractError::AlreadyInitialized`] — contract has already been
/// initialized.
///
/// # Security Notes
///
/// - `admin.require_auth()` is invoked after the `AlreadyInitialized`
/// guard, so a second caller cannot learn whether the contract is already
/// initialized without paying for auth on a no-op.
Comment on lines +251 to +252
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
/// - Emergency mode and pausable state are **not** checked here because
/// those controls are meaningless before initialization completes.
/// - Storage keys written: `Admin`, `TokenCounter` (0), `TokenIds` ([]),
/// and the `paused` key (`false`).
pub fn initialize(e: Env, admin: Address) -> Result<(), ContractError> {
// Reject zero address for admin
if is_zero_address(&e, &admin) {
Expand All @@ -192,10 +265,12 @@ impl CommitmentNFTContract {
return Err(ContractError::AlreadyInitialized);
}

// Store admin address
e.storage().instance().set(&DataKey::Admin, &admin);
// Enforce single deployer assumption: the admin key must sign this
// transaction. Placed after the AlreadyInitialized check so that a
// retry attempt incurs no auth cost.
admin.require_auth();

// Initialize token counter to 0
e.storage().instance().set(&DataKey::Admin, &admin);
e.storage().instance().set(&DataKey::TokenCounter, &0u32);

// Initialize empty token IDs vector (persistent storage for scalability)
Expand Down
53 changes: 52 additions & 1 deletion contracts/commitment_nft/test_snapshots/tests/test_balance_of_after_minting.1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,25 @@
"nonce": 0
},
"auth": [
[],
[
[
"CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
{
"function": {
"contract_fn": {
"contract_address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM",
"function_name": "initialize",
"args": [
{
"address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4"
}
]
}
},
"sub_invocations": []
}
]
],
[],
[],
[],
Expand Down Expand Up @@ -1320,6 +1338,39 @@
4095
]
],
[
{
"contract_data": {
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
"key": {
"ledger_key_nonce": {
"nonce": 801925984706572462
}
},
"durability": "temporary"
}
},
[
{
"last_modified_ledger_seq": 0,
"data": {
"contract_data": {
"ext": "v0",
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
"key": {
"ledger_key_nonce": {
"nonce": 801925984706572462
}
},
"durability": "temporary",
"val": "void"
}
},
"ext": "v0"
},
6311999
]
],
[
{
"contract_code": {
Expand Down
57 changes: 54 additions & 3 deletions contracts/commitment_nft/test_snapshots/tests/test_balance_updates_after_transfer.1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,25 @@
"nonce": 0
},
"auth": [
[],
[
[
"CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
{
"function": {
"contract_fn": {
"contract_address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM",
"function_name": "initialize",
"args": [
{
"address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4"
}
]
}
},
"sub_invocations": []
}
]
],
[
[
"CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
Expand Down Expand Up @@ -1031,6 +1049,39 @@
6311999
]
],
[
{
"contract_data": {
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
"key": {
"ledger_key_nonce": {
"nonce": 5541220902715666415
}
},
"durability": "temporary"
}
},
[
{
"last_modified_ledger_seq": 0,
"data": {
"contract_data": {
"ext": "v0",
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
"key": {
"ledger_key_nonce": {
"nonce": 5541220902715666415
}
},
"durability": "temporary",
"val": "void"
}
},
"ext": "v0"
},
6311999
]
],
[
{
"contract_data": {
Expand Down Expand Up @@ -1102,7 +1153,7 @@
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAITA4",
"key": {
"ledger_key_nonce": {
"nonce": 5541220902715666415
"nonce": 4837995959683129791
}
},
"durability": "temporary"
Expand All @@ -1117,7 +1168,7 @@
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAITA4",
"key": {
"ledger_key_nonce": {
"nonce": 5541220902715666415
"nonce": 4837995959683129791
}
},
"durability": "temporary",
Expand Down
53 changes: 52 additions & 1 deletion contracts/commitment_nft/test_snapshots/tests/test_get_admin.1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,25 @@
"nonce": 0
},
"auth": [
[],
[
[
"CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
{
"function": {
"contract_fn": {
"contract_address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM",
"function_name": "initialize",
"args": [
{
"address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4"
}
]
}
},
"sub_invocations": []
}
]
],
[]
],
"ledger": {
Expand Down Expand Up @@ -94,6 +112,39 @@
4095
]
],
[
{
"contract_data": {
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
"key": {
"ledger_key_nonce": {
"nonce": 801925984706572462
}
},
"durability": "temporary"
}
},
[
{
"last_modified_ledger_seq": 0,
"data": {
"contract_data": {
"ext": "v0",
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
"key": {
"ledger_key_nonce": {
"nonce": 801925984706572462
}
},
"durability": "temporary",
"val": "void"
}
},
"ext": "v0"
},
6311999
]
],
[
{
"contract_code": {
Expand Down
53 changes: 52 additions & 1 deletion contracts/commitment_nft/test_snapshots/tests/test_get_all_metadata.1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,25 @@
"nonce": 0
},
"auth": [
[],
[
[
"CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
{
"function": {
"contract_fn": {
"contract_address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM",
"function_name": "initialize",
"args": [
{
"address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4"
}
]
}
},
"sub_invocations": []
}
]
],
[],
[],
[],
Expand Down Expand Up @@ -816,6 +834,39 @@
4095
]
],
[
{
"contract_data": {
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
"key": {
"ledger_key_nonce": {
"nonce": 801925984706572462
}
},
"durability": "temporary"
}
},
[
{
"last_modified_ledger_seq": 0,
"data": {
"contract_data": {
"ext": "v0",
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAFCT4",
"key": {
"ledger_key_nonce": {
"nonce": 801925984706572462
}
},
"durability": "temporary",
"val": "void"
}
},
"ext": "v0"
},
6311999
]
],
[
{
"contract_code": {
Expand Down
Loading
Loading