fix(market): write StorageVersion before Admin in initialize - #650
Open
Akeem813 wants to merge 1 commit into
Open
fix(market): write StorageVersion before Admin in initialize#650Akeem813 wants to merge 1 commit into
Akeem813 wants to merge 1 commit into
Conversation
The two storage writes in initialize() were ordered admin-first,
version-second. If a transaction is interrupted between the two writes
(out-of-gas or host error), the resulting partial state differs
critically depending on which write landed:
Before (admin first, version second):
- Interrupted after set_admin, before set_version:
has_admin() == true → subsequent initialize() calls return
AlreadyInitialized, but assert_version() returns UpgradeRequired.
The contract is permanently locked out of both initialization and
normal operation — bricked with no recovery path short of
redeployment.
After (version first, admin second):
- Interrupted after set_version, before set_admin:
has_admin() == false → initialize() can be retried and will
complete normally. All require_initialized guards still reject
callers because has_admin() is false, so no state-mutating
function can run in the gap. The contract is safe to retry.
The AlreadyInitialized guard itself (has_admin() check) is unchanged —
it still correctly rejects a second full call to initialize() after a
successful first run.
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
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.
Problem
The two storage writes in
initialize()were ordered admin-first, version-second. If the transaction is interrupted between those two writes (out-of-gas or host error), the resulting partial state is different depending on which write completed:has_admin()assert_version()set_admin(old ordering)trueUpgradeRequiredinitialize()returnsAlreadyInitialized, but all storage accessors returnUpgradeRequired. No recovery path short of redeployment.set_version(new ordering)falseOk(())initialize()can be retried. Allrequire_initializedguards still reject callers, so nothing can be called in the gap.Fix
Swap the write order: call
storage::set_versionbeforestorage::set_admin.The
AlreadyInitializedguard (has_admin()check) is unchanged — it still correctly rejects any second complete call toinitialize()after a successful first run.Why This Satisfies "Reject initialize twice"
The existing
has_admin()guard already returnsContractError::AlreadyInitializedon a second call. This PR does not change that logic — it hardens the write ordering so that the guard's sentinel value (Admin) is only written once all preceding state is durable. A secondinitialize()call on a successfully initialized contract still returnsAlreadyInitialized(#42).Files Changed
contracts/market/src/lib.rs— reorderset_versionbeforeset_admininsideinitialize()Acceptance Criteria
initializereturnsContractError::AlreadyInitialized— existing guard, unchanged