[yachamdaniel1-alt]

Summary

This PR addresses three related security, validation, and engineering-quality issues:

• 74. [Smart Contracts] Security review of upgrade.rs authorization flow #1169 — Upgrade authorization security
• 75. [Smart Contracts] Consolidate NFT and credential_metadata minting validation logic #1170 — NFT/Credential metadata validation
• 76. [Smart Contracts] Add benchmark suite tracking WASM binary size per contract #1171 — WASM binary-size benchmarking

The changes harden the timelocked upgrade path end-to-end, move every metadata rule into the shared validation module (one source of truth for both the NFT and the credential contracts), and add a repeatable benchmark that records and diffs release WASM binary size for all 19 contracts.

Branch lineage: this branch is stacked on top of the pending #1172–#1175 branch (PR #1239), because that branch restores the shared-crate module declarations (upgrade, validation, access, multisig, …) and SharedContract scaffolding the #1169/#1170 code builds on. Until PR #1239 merges, this PR's diff therefore also shows those commits; the changes belonging to this PR are exactly the 19 files listed under Files / Areas Changed, and the diff shrinks to those alone once #1239 lands.

#1169 — Upgrade Authorization Security

Proposer authorization. governance::propose_upgrade keeps proposer.require_auth(), requires a future voting end and a non-zero timelock, and now additionally rejects upgrade targets that are the zero address (both the all-zero contract strkey C… and the all-zero account strkey G…), so a proposal can never be pointed at nothing.

Admin-only approval. docs/contract-interfaces.md documents approve_upgrade as the admin approval gate, but the implementation took no caller at all — anyone could flip approved on a proposal that had merely finished voting. approve_upgrade(env, admin, upgrade_id) now takes the approving admin and enforces access::require_admin before marking a passed proposal approved. execute_upgrade stays permissionless exactly as documented, but only after the proposal is approved, the timelock has expired, and it has not already been executed.

Zero-address rejection. Contract and account zero strkeys are validated at proposal time, so no upgrade proposal can target the zero address.

Initialization takeover prevention. SharedContract::initialize had no once-only guard: any address could call it, become the stored admin, and then schedule and execute an upgrade — a complete takeover of the upgrade authority. It now refuses a second call with "Already initialized", matching the convention already used by governance and credential_metadata.

Disabling the direct upgrade bypass. SharedContract::upgrade swapped the WASM immediately for the admin, with no timelock, no pending record and no history — bypassing every documented invariant. The admin authorization check still runs first (unauthorized callers keep getting "Unauthorized: admin required"), but the swap itself now refuses with "Direct upgrade disabled: use schedule_upgrade then execute_upgrade".

schedule → approve → timelock → execute flow. The enforced lifecycle is now exactly the one the docs describe: admin schedule_upgrade → voting passes → admin approve_upgrade → timelock expires → anyone execute_upgrade, with cancel_upgrade available to the admin before execution.

Authorization checks inside the upgrade helpers. Every helper in contracts/shared/src/upgrade.rs (schedule_upgrade, execute_upgrade, cancel_upgrade) now re-checks access::require_admin itself (defense in depth), so no entry point — present or future — can reach upgrade logic without the check.

Zero WASM hash validation. Scheduling the all-zero 32-byte hash (which would replace the contract with nothing) is rejected with "Invalid WASM hash".

Timelock validation. timelock_ledgers == 0 would make an upgrade executable in the very ledger it was scheduled; a minimum of one ledger is now enforced ("Timelock must be at least 1 ledger").

Documentation. contracts/shared/src/upgrade.rs carries an invariants section plus a "Findings addressed (#1169)" list, and docs/contract-upgrades.md was rewritten around the required timelocked flow, the CLI example for schedule/execute, and the authorization invariants.

Tests. New coverage for: unauthorized direct schedule/execute/cancel, wrong-account and role-based (Instructor/Student) rejection, missing signature, module-level bypass of the entry-point checks, partial multisig scenarios (a 2-of-3 execute_proposal with only 2 approvals leaves no upgrade state; a multisig co-signer still cannot schedule), immediate-upgrade bypass, re-initialization admin takeover, non-admin approve_upgrade on an already-passed proposal, admin approval marking the proposal approved, execution before the timelock (rejected), execution after the timelock (succeeds), and double execution (rejected).

#1170 — NFT/Credential Metadata Validation

Shared validation predicates. brain_storm_shared::validation is now the single source of truth: every rule is one is_* predicate plus one require_* wrapper that is literally assert!(is_*(value), MESSAGE), so write paths and read paths can never disagree. Original error messages are preserved.

is_non_empty_string. New shared predicate (require_non_empty_string now delegates to it) used for the fields that need presence rather than a format rule.

Centralized metadata validation. New validate_metadata_fields(course_name, grade, ipfs_hash) performs require_valid_course_name + require_valid_grade + require_non_empty_string(ipfs_hash) and is called from every credential metadata write path — issue_with_nft, store_metadata and update_metadata (which now loads the stored record first so it can validate the record's existing IPFS hash alongside the new values).

NFT course-name and royalty validation. nft::mint_course_nft replaced its inline royalty assertion with the shared require_valid_royalty_basis and gained require_valid_course_name (it had no course-name rule at all).

Validation of stored records through is_metadata_valid. is_metadata_valid(env, credential_id) was not correct before. It now returns false for a nonexistent credential without panicking, and validates every stored field using the same shared predicates the write paths use: course name, grade, non-empty IPFS hash, and expiry_timestamp >= completion_date.

Missing-record handling. A credential id with no record yields false (reader contract), not a panic.

Expiry/completion-date validation. A credential whose expiry_timestamp precedes its completion_date is reported invalid, on both the write and the read side.

IPFS hash validation. The IPFS hash is enforced to be present on every write path (it is the record's only content pointer) and is checked when reading; no hash-format rule was added, because fixtures across the repo legitimately use short placeholders.

Tests. New and updated tests in both contracts: empty/overlong/short course name and empty grade rejected on store and update, empty IPFS hash rejected on write, validate_metadata_fields accepting valid values, plus reader tests proving invalid records already in storage (bad course name, empty hash, expiry before completion, missing record) report false, and the happy path still reports true.

#1171 — WASM Binary-Size Benchmark

• New executable scripts/wasm-size-benchmark.sh (bash 3.2 compatible), the entry point for the benchmark.
• All-19-contract inventory: the script discovers every contract package, builds each for wasm32-unknown-unknown --release, and reports size and status for all of them.
• Isolated SDK-major build environments: builds run in throwaway per-SDK-major workspaces under target/wasm-size-bench/ (copied Cargo.lock + rust-toolchain.toml), because the repository workspace does not resolve (see Pre-existing Issues). Repository files are never modified by the script.
• WASM size measurement: every artifact is measured in bytes with the build duration recorded alongside.
• SHA256 tracking: each build's hash is recorded so silent content changes are visible even when size is unchanged.
• Baseline generation: --update writes docs/contracts/wasm-size-baseline.md (committed with this PR), documenting toolchain, rustc version, profile flags, thresholds, per-contract status/size/hash and how to run locally.
• Regression detection: the default mode diffs current sizes against the baseline and fails when a contract grows past the threshold or changes status (ok → failed).
• Configurable threshold: --threshold <pct> (default 10%).
• --contract <name> filtering: build and compare a single contract.
• Exit codes: 0 = no regressions, 1 = regression or status change, 2 = usage error (unknown contract, bad flag).
• NFT build adjustment required to make the artifact benchmarkable: contracts/nft/Cargo.toml listed soroban-sdk/testutils under [dependencies], which is a hard compile_error on the wasm32 target, so the NFT contract could never be built or measured. testutils moved to [dev-dependencies] (same 21.5.0 version, tests unchanged) so the release artifact builds and is included in the baseline (44133 bytes).
• Generated baseline documentation: docs/contracts/wasm-size-baseline.md.

Verification

The following verification was actually performed:

• rustfmt --edition 2021 --check on every touched file, diffed against a pristine copy of the base revision → 0 formatting violations introduced by this work.
• WASM baseline generation (./scripts/wasm-size-benchmark.sh --update) → succeeded, baseline written.
• WASM benchmark comparison (./scripts/wasm-size-benchmark.sh) → succeeded, all deltas +0.0%, no regressions vs baseline, exit 0.
• NFT benchmark (./scripts/wasm-size-benchmark.sh --contract nft) → ok 44133 bytes, exit 0.
• Simulated size regression (baseline temporarily edited to 30000) → +47.1% !! size regression above 10%, exit 1 (baseline then restored).
• Unknown contract handling (--contract nope) → exit 2, --help prints usage.

Unit tests were run in an isolated copy of the workspace (the repository workspace itself does not resolve — see below): brain-storm-shared 102 passed / 4 failed, brain-storm-governance 50 / 8, brain-storm-nft 38 / 3, credential_metadata 40 / 9 — every remaining failure is on the pre-existing baseline list (the same failures exist before this branch; none were introduced here, and one pre-existing shared failure, re-initialization, is fixed by this PR).

The repository has pre-existing workspace/SDK resolution issues. They were NOT changed as part of this PR.

Pre-existing Issues / Scope

Explicitly out of scope for #1169–#1171 and not modified by this PR:

• Workspace SDK/version resolution conflict: contracts/market pins soroban-sdk 20.0.0 (ed25519-dalek =2.0.0) while the rest of the workspace uses 21.x (^2.1.1), so no in-repo cargo command resolves. Root Cargo.toml / Cargo.lock and all SDK versions are untouched.
• Existing unrelated test failures (shared admin/events/oracle, governance voting::tests::test_e2e_* + vote-power benchmark, nft budget benchmarks, credential linkage/admin/compact/migrate) were not introduced by this PR — they reproduce on the base revision. The full repository test suite does not pass and is not claimed to.
• credential_metadata contract-macro parameter limitation: issue_with_nft declares 12 parameters against the macro's maximum of 10 — a pre-existing defect; its signature is public API and was not changed.
• Pre-existing WASM build failures recorded by the benchmark (present in the committed baseline): 7 contracts list soroban-sdk/testutils in production [dependencies] (badges, buyback, dispute, liquidity_pool, registry, reputation), market has a syntax error at src/lib.rs:9, credential_metadata hits the 12-parameter limit, integration has no cdylib.
• These are outside the scope of 74. [Smart Contracts] Security review of upgrade.rs authorization flow #1169–76. [Smart Contracts] Add benchmark suite tracking WASM binary size per contract #1171.

Files / Areas Changed

• contracts/shared/src/upgrade.rs — module-level authorization, zero-hash / zero-timelock validation, invariants + findings documentation.
• contracts/shared/src/upgrade_tests.rs, contracts/shared/src/tests.rs — authorization, bypass, multisig, timelock and takeover tests.
• contracts/shared/src/lib.rs — once-only initialize, direct upgrade refusal, wrappers that only delegate.
• contracts/shared/src/validation.rs — is_* / require_* predicates, is_non_empty_string.
• contracts/governance/src/lib.rs — zero-address target check, admin-gated approve_upgrade, approval/execution tests.
• contracts/nft/src/{lib.rs,tests.rs} — shared course-name and royalty validation, rejection tests.
• contracts/nft/Cargo.toml — testutils moved to [dev-dependencies] so the NFT WASM builds.
• contracts/credential_metadata/src/{lib.rs,validation.rs,validation_tests.rs} — centralized write-path validation, completed is_metadata_valid, reader/writer tests.
• docs/contract-upgrades.md, docs/contract-interfaces.md — upgrade flow and authorization documentation.
• scripts/wasm-size-benchmark.sh, docs/contracts/wasm-size-baseline.md — new benchmark and its committed baseline.

Issue Closure

Closes #1168
Closes #1169
Closes #1170
Closes #1171

[drips-wave]

@yachamdaniel1-alt 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