fix: defined panic instead of UB when admin was never set; flux gate - #19
Open
willemneal wants to merge 4 commits into
Open
fix: defined panic instead of UB when admin was never set; flux gate#19willemneal wants to merge 4 commits into
willemneal wants to merge 4 commits into
Conversation
The repo has a v0.1.0 git tag and GitHub release, but admin-sep@0.1.0 was never published to crates.io. release-plz refuses to proceed on that mismatch. Bumping to 0.2.0 (correct anyway after the breaking soroban-sdk v27 update) lets release-plz treat it as a first release. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Administratable::admin() used unsafe unwrap_unchecked on the storage
read — if no admin was ever set (constructor never called set_admin),
that is undefined behavior in wasm and could hand back a garbage
Address for require_admin to authorize against. Replace with
.expect("admin-sep: admin not set"): a defined, loud panic.
Also enable Flux refinement checking (metadata-only, no dependency —
inert for normal builds and crates.io): cargo flux -p admin-sep checks
19/19 functions including the contracttrait-generated code.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M52tZC7sppbbo1HDBpj9KK
willemneal
commented
Aug 17, 2026
| #[contracttrait] | ||
| pub trait Administratable { | ||
| fn admin(env: &Env) -> soroban_sdk::Address { | ||
| unsafe { admin_from_storage(env).unwrap_unchecked() } |
Member
Author
There was a problem hiding this comment.
set_admin is called in a contracts constructor so this is safe.
Member
Author
There was a problem hiding this comment.
Agreed — with set_admin wired into every consuming contract's constructor (and its first call skipping the auth check for exactly that purpose), the entry always exists before a read, so the unchecked unwrap is sound in intended use. Reverted to unwrap_unchecked and moved the requirement into a trait-level Safety invariant doc + SAFETY: comment instead, so the obligation is stated where future consumers (the trait is public API) can see what they must uphold. Gates re-run: tests, clippy, cargo flux (19/19) all green.
…iant Per review: set_admin runs in every consuming contract's constructor, so the storage entry exists before any admin() read — the unsafe read is sound in intended use. Keep it, and encode the requirement as a trait-level safety invariant + SAFETY comment instead of a runtime check, so future consumers know what they must uphold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M52tZC7sppbbo1HDBpj9KK
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Out of the Flux verification sweep (see nidohq/soroban-flux):
Safety invariant documented:
Administratable::admin()'s default impl reads the admin withunwrap_unchecked— sound because every consuming contract callsset_adminfrom its constructor (whose first call skips the auth check for exactly this purpose), so the entry exists before any read. That requirement is now stated as a trait-level Safety invariant doc plus aSAFETY:comment on the unsafe block, since the trait is public API and future consumers must uphold it. (An earlier revision swapped in.expect(...); reverted per review — the constructor wiring makes the unchecked read sound.)Flux gate:
[package.metadata.flux] enabled = true— metadata only, no dependency, inert for normal builds and crates.io.cargo flux -p admin-sepchecks 19/19 functions (including#[contracttrait]-generated code) under flux283ad73/ nightly-2026-02-05.Gates: workspace tests green, clippy
-D warningsgreen, flux green, fmt green.🤖 Generated with Claude Code
https://claude.ai/code/session_01M52tZC7sppbbo1HDBpj9KK