feat: one-shot factory initialization and upgrade state preservation (#1141, #1149) - #1204
Open
temitope-007 wants to merge 1 commit into
Open
temitope-007 wants to merge 1 commit into
temitope-007 wants to merge 1 commit into
Conversation
Closes two initialization/upgrade safety gaps. Creditra#1141 Prevent replay of factory initialization calls gateway-contract/contracts/auction_contract/ set_factory_contract was an unbounded setter, so the same initialization call could be submitted repeatedly and a stale but still-valid authorization could re-point the factory long after deployment. Registration is now one-shot: the first success sets a persistent FactoryInitialized marker and every later call reverts FactoryAlreadyInitialized (15) without mutating state. The barrier is checked before the auth check and before any write, so a replay leaves storage byte-identical. The barrier keys off the marker, not the presence of an address, so a rotation that re-points the slot cannot reopen the initialization window and let an unprivileged address claim the factory role a second time. Deliberate replacement - the case the old setter served implicitly - moves to rotate_factory_contract, which requires both the outgoing and the incoming factory to authorize, so a hand-over can neither be forced on an unwilling successor nor taken unilaterally. is_factory_initialized exposes deployment state without provoking a rejection. Compatibility: set_factory_contract keeps its signature and first-time behaviour; only replacement through it is withdrawn, and that path is replaced by the dedicated entrypoint above. Creditra#1149 Preserve credit-line state through upgrade migrations contracts/credit/src/upgrade_migration.rs upgrade bumped the schema version blind: nothing recorded what state looked like beforehand, so a swap that lost or re-interpreted credit-line records was undetectable, and the "previous" wasm hash written to the event was a zero sentinel, leaving no rollback target. upgrade now records an UpgradeCheckpoint - schema versions, CreditLineCount, TotalUtilized, wasm hashes - before the schema bump and the wasm swap (UP-1); capturing them afterwards would compare the new binary against itself. verify_upgrade_migration compares the live aggregates against the checkpoint and clears it on success. On mismatch it reverts UpgradeStateMismatch and deliberately retains the checkpoint (UP-4), because silently repairing a mismatch would destroy the evidence that records were lost. A second upgrade while a checkpoint is outstanding reverts UpgradeVerificationPending (UP-2), so a failed migration cannot be compounded or its evidence overwritten. rollback_upgrade restores the pre-upgrade schema version; it deliberately does not re-swap the wasm, and the returned checkpoint carries previous_wasm_hash so an operator redeploys the known-good binary explicitly. The checkpoint view carries only aggregates and hashes, never per-borrower data. Repository repairs required to build (byte-identical merge duplicates on main, unrelated to either issue but blocking every build): - Cargo.lock listed package `creditra-credit` twice, so cargo could not parse the lockfile at all. - contracts/credit/src/lib.rs defined `pub fn init` twice in one #[contractimpl], producing 9 compile errors. Validation: - cargo test -p gateway-auction --test factory_init_replay 10 passed, 0 failed - cargo test -p creditra-credit --test upgrade_migration_state 12 passed, 0 failed - cargo test -p gateway-auction --test factory_auth 14 passed, 0 failed - cargo test -p gateway-auction --test err_stab 21 passed, 0 failed - cargo check -p gateway-auction --lib / -p creditra-credit --lib 0 errors - cargo test -p gateway-auction --lib 89 passed / 6 failed, identical to the baseline measured with the two repairs and without these changes; the six liquidation_grace_window failures are pre-existing. Closes Creditra#1141 Closes Creditra#1149
|
@temitope-007 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! 🚀 |
Author
|
@greatest0fallt1me check out the PR |
6 tasks
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.
Summary
Two initialization/upgrade safety gaps, both of which allowed a repeated or
partially-failed operation to leave the protocol in a state nobody could detect
or recover from.
gateway-contract/contracts/auction_contract/src/lib.rstests/factory_init_replay.rscontracts/credit/src/upgrade_migration.rscontracts/credit/tests/upgrade_migration_state.rs#1141 — Prevent replay of factory initialization calls
The gap
set_factory_contractwas an unbounded setter. The same initialization callcould be submitted repeatedly, and a stale but still-valid authorization could
re-point the factory long after deployment — and the factory controls
init_auction,close_auction, andsettle_default_liquidation.The fix
Registration is one-shot. The first success sets a persistent
FactoryInitializedmarker; every later call revertsFactoryAlreadyInitialized(15) and mutates nothing. The barrier is checkedbefore the auth check and before any write, so a replayed call leaves
storage byte-identical.
The barrier keys off the marker, not the presence of an address. A rotation
that re-points the slot therefore cannot reopen the initialization window and
let an unprivileged address claim the factory role a second time — the property
rotation_does_not_reopen_initializationpins.Deliberate replacement — the case the old setter served implicitly — moves to
rotate_factory_contract, which requires both the outgoing and theincoming factory to authorize, so a hand-over can neither be forced on an
unwilling successor nor taken unilaterally.
Compatibility
set_factory_contractkeeps its signature and its first-time behaviour. Theonly behavioural withdrawal is replacement through that entrypoint, which is
replaced by the dedicated, strictly-stronger
rotate_factory_contract.is_factory_initializedlets a client distinguish "not yet initialized" from"initialized" without provoking a rejection.
#1149 — Preserve credit-line state through upgrade migrations
The gap
upgradebumped the schema version blind:silently re-interpreted credit-line records was undetectable;
leaving no rollback target;
already gone wrong.
The fix
upgradenow records anUpgradeCheckpoint— schema versions,CreditLineCount,TotalUtilized, wasm hashes, timestamps — before theschema bump and the wasm swap (UP-1); capturing them afterwards would
compare the new binary against itself and prove nothing.
verify_upgrade_migrationcompares live aggregates against the checkpoint,clearing it on success. On mismatch it reverts
UpgradeStateMismatchanddeliberately retains the checkpoint (UP-4) — silently repairing a
mismatch would destroy the evidence that records were lost.
UpgradeVerificationPending(UP-2), so a failed migration cannot becompounded or its evidence overwritten.
rollback_upgraderestores the pre-upgrade schema version. It deliberatelydoes not re-swap the wasm: a binary that just failed its own migration
cannot be trusted to deploy its replacement, and Soroban has no atomic
revert-to-previous-hash primitive. The returned checkpoint carries
previous_wasm_hashso an operator redeploys the known-good binaryexplicitly. Documenting that boundary is the point — an operator must not
believe rollback is a complete undo.
Aggregates are used rather than a per-borrower scan because the scan cost is
unbounded in the number of credit lines, while these two accumulators are
already maintained as invariants over every credit-line mutation: if any record
were lost or misread, at least one of them moves.
Acceptance criteria
first_registration_succeeds_and_marks_initialized,upgrade_records_checkpoint_of_pre_upgrade_state; duplicate:second_registration_is_rejected,replaying_same_factory_is_rejected,second_upgrade_is_blocked_until_resolved; invalid:rotate_before_initialization_is_rejected,verify_without_checkpoint_is_rejected,rollback_without_checkpoint_is_rejected; boundary:checkpoint_captures_zero_state_on_fresh_contract.rotation_requires_both_partiesproves a successor cannot seize the role;both upgrade mutators remain admin-gated.
rejected_replay_leaves_factory_unchangedandrejected_second_upgrade_leaves_checkpoint_intactassert the state isbyte-identical after a rejected retry.
purely additive (new entrypoints and one new storage key).
discriminants (
factory_errors_are_stable,upgrade_errors_are_classified);the checkpoint view carries only aggregates and hashes, never borrower
addresses or amounts (
checkpoint_exposes_only_aggregate_state).Validation
cargo test -p gateway-auction --test factory_init_replaycargo test -p creditra-credit --test upgrade_migration_statecargo test -p gateway-auction --test factory_auth(pre-existing)cargo test -p gateway-auction --test err_stab(pre-existing)cargo check -p gateway-auction --lib/-p creditra-credit --libcargo test -p gateway-auction --libThe six
liquidation_grace_windowfailures are pre-existing. I measured thebaseline by applying only the two repository repairs below to clean
main,without either feature, and got the identical 89/6 — so these changes
introduce no new failures.
A note on test methodology for #1149
upgradecannot be driven to completion in-process: it ends inupdate_current_contract_wasm, which needs a genuinely uploaded wasm binary,and a failure there reverts the whole invocation including the checkpoint
write. That revert is the correct atomic behaviour, so rather than weaken it
for testability, the tests call the same
record_checkpointthe entrypointcalls, in the contract's own storage context, and exercise every downstream
path through the client. The UP-2 guard is still proven at the entrypoint by
second_upgrade_is_blocked_until_resolved, because that guard runs before theswap is ever reached.
Repository repairs required to build
Two byte-identical merge duplicates on
mainblock every build. Neitherrelates to these issues, but nothing compiles without them, so no validation of
any kind was possible until they were fixed. Both are removals of an exact
duplicate — no semantic choice was involved:
Cargo.locklisted packagecreditra-credittwice, so cargo could notparse the lockfile at all.
contracts/credit/src/lib.rsdefinedpub fn inittwice in one#[contractimpl], producing 9 compile errors.Happy to split these into a separate PR if maintainers prefer.
Security and failure-mode note
For #1141 the ordering is the security property: the replay barrier precedes
the auth check, so a replayed call cannot even consume an authorization, and
the marker is never cleared so the one-shot window cannot be reopened by any
later rotation. For #1149 the checkpoint is both the evidence and the recovery
target, which is why a failed verification retains it and why a stacked upgrade
is refused rather than allowed to overwrite it. Every rejection path validates
before its first write, and a Soroban panic reverts the transaction, so a
rejected initialization, upgrade, verification, or rollback leaves storage
unchanged.
Closes #1141
Closes #1149