feat: harden caller authorization and precondition guards in execute_admin_transfer (closes #347) - #438
Merged
godamongstmen897 merged 66 commits intoSep 1, 2026
Conversation
…tests (Goldii-locks#290) Harden cancel_escrow with two missing validation rules and add a full test suite covering every guard, happy path, post-cancel state, event structure, and milestone isolation. Production changes (lib.rs): - Add EmergencyPaused guard: cancel_escrow now returns Error::Paused when the contract is emergency-paused, consistent with all other user-facing endpoints - Add duplicate-cancel guard: a second call while CancelLock is already active returns Error::EscrowLocked, preventing race conditions and redundant lock-sets Tests added (test.rs) — 20 new tests: Invalid address guards: - test_cancel_escrow_zero_account_address_rejected - test_cancel_escrow_zero_contract_address_rejected Not-initialized guard: - test_cancel_escrow_not_initialized_fails Not-funded guard: - test_cancel_escrow_not_funded_fails Unauthorized guards: - test_cancel_escrow_stranger_unauthorized - test_cancel_escrow_arbiter_unauthorized - test_cancel_escrow_admin_unauthorized Emergency-paused guard: - test_cancel_escrow_while_paused_fails Duplicate-cancel guard: - test_cancel_escrow_duplicate_call_fails - test_cancel_escrow_freelancer_duplicate_after_client_fails Happy paths: - test_cancel_escrow_client_succeeds - test_cancel_escrow_freelancer_succeeds Post-cancel state validation: - test_cancel_escrow_blocks_fund - test_cancel_escrow_blocks_mark_delivered - test_cancel_escrow_blocks_approve_milestone - test_cancel_escrow_blocks_raise_dispute Event validation: - test_cancel_escrow_emits_exactly_one_event - test_cancel_escrow_event_contains_correct_caller Milestone state isolation: - test_cancel_escrow_does_not_mutate_milestones - test_cancel_escrow_all_milestones_released_still_succeeds All 218 tests pass.
…tests (Goldii-locks#293) Expand CancelEscrowInitiatedEvent with full operational context so downstream indexers can reconstruct the complete cancellation state from the event payload alone, without querying contract storage. Production changes (lib.rs): - Expand CancelEscrowInitiatedEvent with six new fields: caller_is_client bool — true if initiator is the client, false if freelancer client Address — registered client address freelancer Address — registered freelancer address token Address — escrow token contract address milestone_count u32 — number of milestones at cancellation time total_amount i128 — aggregate milestone total (pre-release) - Update cancel_escrow event emission to populate all new fields, deriving caller_is_client from (caller == meta.client) No other code changed. Tests added (test.rs) — 14 new tests (396 -> 410): Event count: - test_cancel_escrow_event_emitted_exactly_once contract_id field: - test_cancel_escrow_event_contract_id_correct caller field: - test_cancel_escrow_event_caller_is_client_address - test_cancel_escrow_event_caller_is_freelancer_address caller_is_client role field: - test_cancel_escrow_event_caller_is_client_true_for_client - test_cancel_escrow_event_caller_is_client_false_for_freelancer client / freelancer / token fields: - test_cancel_escrow_event_client_field_correct - test_cancel_escrow_event_freelancer_field_correct - test_cancel_escrow_event_token_field_correct milestone_count field: - test_cancel_escrow_event_milestone_count_single - test_cancel_escrow_event_milestone_count_multiple total_amount field: - test_cancel_escrow_event_total_amount_correct_single_milestone - test_cancel_escrow_event_total_amount_correct_multi_milestone Full indexer round-trip: - test_cancel_escrow_event_full_indexer_parse All 410 tests pass.
Drop the redundant MilestoneReleased(index) temporary-flag write from multisig_admin_override_release. The flag's only reader (is_milestone_released_flag) is dead code, and the persisted Released status on the milestone is the authoritative completion signal. This reduces the distinct storage keys written by the call from three (Milestone, MilestoneReleased, MultisigLocked) to two (Milestone, MultisigLocked), matching multisig_admin_override_refund. Adds test_multisig_admin_override_release_reduced_storage_footprint to assert the temporary flag is no longer written while token transfer, lock clearing, milestone state, and terminal re-entry rejection still hold. Updates the affected snapshot fixtures accordingly. Closes Goldii-locks#392
Guard the refund arithmetic so no input can cause a wrap or an unhandled panic (issue Goldii-locks#395). Negative amount / released_amount and released_amount > amount now return Error::InvalidAmount before any arithmetic runs, complementing the existing checked_sub and remaining<=0 guards. The same guards are applied to the sibling multisig_admin_override_release for consistency. Adds a comprehensive suite to multisig_admin_override_refund_tests asserting i128::MAX / i128::MIN operands return Error::InvalidAmount (rather than panicking) and that valid amounts refund exactly amount - released_amount, identical to prior behavior. Closes Goldii-locks#395
…_approve (closes Goldii-locks#354) Reorder multisig_approve so the signer-membership check runs before any job/token ledger reads (job meta load, cross-contract balance call), matching the guard ordering used elsewhere in the hardening series. Add dedicated tests asserting unauthorized and empty-balance rejections leave the proposal's approval bitmap unmutated.
…d tests (Goldii-locks#353) Also fixes an unrelated pre-existing compile break in admin_override_cancel_tests.rs (missing #![cfg(test)] gate and inaccessible setup_funded_escrow helper) so the crate's test suite can build and run at all.
- annotate test modules with #[cfg(test)] so the wasm build succeeds without dev-dependencies - make setup_funded_escrow pub(crate) and fix test imports so sibling test modules can use it (Address::generate requires testutils trait) - remove redundant admin.require_auth() before require_admin in admin_override_cancel_refund; require_admin already performs the signature check, and env-host 22.1.3 rejects the double auth with Error(Auth, ExistingValue) - fund a terminal-state cancel test through the zero-balance boundary guard in cancel_escrow so its invalid-amount assertion stays intact - add missing admin_override_cancel_tests snapshot files (untracked, would otherwise fail CI on a fresh checkout)
…lance - add Error::EmptyBalance (=32) and assert_nonzero_balance helper - reject emergency_pause_claim_refund while the contract token balance is zero, so an emergency settlement never attempts an empty transfer - fund the initialized-escrow fixture mint via its own token id so the existing claim_refund split-math tests keep passing - add a test asserting EmptyBalance is returned for a paused but unfunded escrow
- annotate test modules with #[cfg(test)] so the wasm build succeeds without dev-dependencies - make setup_funded_escrow pub(crate) and fix test imports so sibling test modules can use it (Address::generate requires testutils trait) - remove redundant admin.require_auth() before require_admin in admin_override_cancel_refund; require_admin already performs the signature check, and env-host 22.1.3 rejects the double auth with Error(Auth, ExistingValue) - fund a terminal-state cancel test through the zero-balance boundary guard in cancel_escrow so its invalid-amount assertion stays intact - add missing admin_override_cancel_tests snapshot files (untracked, would otherwise fail CI on a fresh checkout)
…e on-ledger footprint Rename the emergency-pause instance storage keys to shorter symbols: EmergencyPaused -> Ep (2 chars vs 16) EmergencyPauseLock -> EpLk (4 chars vs 19) Applied consistently across the milestone-escrow contract and the reports copy, and updated the affected test snapshots so the on-ledger symbol assertions match the shorter keys. This is an isolated, self-contained change (Closes Goldii-locks#323).
…ocks#309) Add structured event types and emission for all platform fee allocation functions: set_platform_fee_allocation, lock_platform_fee_allocation, pf_alloc_admin_override, and calculate_platform_fee_split. New event types: - PlatformFeeAllocationSetEvent (topic: pf_set) - PlatformFeeAllocationLockedEvent (topic: pf_lock) - PlatformFeeAllocationOverrideEvent (topic: pf_ovr) - PlatformFeeSplitCalculatedEvent (topic: pf_split) These events enable downstream indexers to track platform fee configuration changes, lock state transitions, admin overrides, and fee split calculations without polling contract storage. Comprehensive test coverage for all 4 event types including: - Happy path event emission verification - Event field validation - Failed operation event suppression - Rounding/conservation verification Co-Authored-By: Codebuff <noreply@codebuff.com>
…x_withholding_deductions (closes Goldii-locks#335)
…admin_transfer (closes Goldii-locks#347)
|
@esthertitilayo-dev 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! 🚀 |
perf(cancel): compact escrow lock storage key
Fix: reduce ledger storage footprint of admin_override_cancel_refund
…hecked-math test: add i128::MIN boundary test for escrow_interest_yield
…erride-release-storage-footprint fix(msadm): reduce multisig_admin_override_release storage footprint
…duce-yield-rate-footprint refactor(storage): consolidate admin_set_yield_rate ledger footprint
…grade-authorization-preconditions refactor(upgrade): enforce auth and state preconditions before WASM upgrade (Goldii-locks#352)
…rden-multisig-approve-guards refactor(multisig): harden auth and precondition guards in multisig_approve (Goldii-locks#354)
feat(Goldii-locks#321): guard emergency_pause_claim_refund on zero balance
…ow-structured-events feat(cancel_escrow): enrich structured event and add indexer parsing …
…79-a0 fix: Harden caller authorization and precondition guards in admin_override_release
…ic-multisig-split-refund Goldii-locks#402 - test(multisig_split_refund): add extreme-value boundary tests for checked arithmetic
…ents_during_platform_fee_allocation feat: emit structured events during platform_fee_allocation (Goldii-locks#309)
…335-admin-tax-withholding-guards feat: harden caller authorization and precondition guards in admin_tax_withholding_deductions (closes Goldii-locks#335)
The arithmetic hardening is sound and merges cleanly. One of the new tests failed: refund_valid_amount_equals_amount_minus_released_amount assertion `left == right` failed: left: 0, right: 1 The event is published -- multisig_admin_override_refund emits msadmref on the success path, and the state and balance assertions above it all passed, so the call had run to completion. The tally was just read too late. env.events().all() reflects the most recent contract invocation, and the test made three more (is_multisig_locked, get_job, token.balance) between the override call and the count. The sibling helper in multisig_split_refund_tests.rs uses the identical idiom and passes because it reads the events first. Moved the event assertion directly after the override call; the state and balance assertions follow unchanged. No production code touched. 518 tests passing / WASM release build OK
…erride-refund-checked-arithmetic fix(msadm): harden multisig_admin_override_refund arithmetic
The storage change is sound: MilestoneTimeExtension moves from persistent to temporary storage, which matches how the rest of this contract already treats deadline-scoped state -- DeliveredAt is temporary too, and extend_ttl is not used anywhere in the file, so no new TTL obligation is introduced. The branch also added `time_extension: 0` to a Milestone literal in test.rs, but Milestone has only amount, released_amount, status and delivered_at: error[E0560]: struct `Milestone` has no field named `time_extension` Nothing else in the branch references such a field -- the extension is keyed storage, not a struct member -- so the line was simply removed. 518 tests passing / WASM release build OK
…timize-storage-keys-footprint-for-milestone fix: optimize milestone_time_extensions storage key footprint
…sed-milestone case
Two things kept this from compiling and passing.
The emergency-pause guard read DataKey::EmergencyPaused, which does not
exist:
error[E0599]: no variant or associated item named `EmergencyPaused`
found for enum `DataKey`
This contract has two separate pause flags -- DataKey::Ep for the
emergency pause (set by emergency_pause_admin_override, read by
ensure_not_paused) and DataKey::Paused for admin_pause_escrow. The
comment and the Error::Paused return both describe the emergency one,
so the guard now reads DataKey::Ep. The branch's own CancelLock check
is left as-is, since it returns EscrowLocked rather than the error
ensure_not_paused would give.
The new test test_cancel_escrow_all_milestones_released_still_succeeds
then failed. Its premise -- "no business rule blocks it" -- is no longer
true: cancel_escrow rejects a zero contract balance with InvalidAmount,
and releasing the only milestone empties the contract, so the balance
guard fired rather than anything to do with milestone status.
The test now mints 1 stroop back to the contract before cancelling, the
same workaround Goldii-locks#429 used for the same guard. That keeps it testing what
it claims -- that Released milestones do not themselves block a cancel --
instead of re-testing the balance guard.
538 tests passing / WASM release build OK
…ow-validation feat(cancel_escrow): add business rule validations and comprehensive …
The storage non-mutation assertions are the point of this PR and are kept in full: the rejected multisig_approval_init calls now also assert the original threshold survives and the would-be signer was never written. The branch predates two tests that have since landed on main, and its copy of test.rs reverts them: test_multisig_approve_unauthorized_fails test_multisig_approve_illegal_source_state_fails Both cover approve-side guards -- an unregistered signer, and a zero-balance source state -- and both assert the approval bitmap is left untouched. Merging as-is took the suite from 538 to 536 while the PR description said it was adding coverage, which is the kind of loss that passes CI without complaint. Restored verbatim from main. The remaining changes to admin_override_cancel_tests.rs are rustfmt reflow plus a #![cfg(test)] attribute, and were left as the branch had them. 538 tests passing / WASM release build OK
…tisig-approval-init-guards fix: add storage non-mutation coverage to multisig_approval_init guar…
The conflict in test.rs was two branches each appending a module declaration at the same spot; both are needed, so both are declared. execute_transfer_swaps_admin_and_emits_event then failed on the event tally (left: 0, right: 1). The event is emitted -- the admin key swap and the pending-transfer removal above it both asserted correctly -- but the count was read after client.get_pending_admin_transfer(), and env.events().all() reflects the most recent contract invocation. Moved the two event assertions directly after execute_admin_transfer; the state assertions follow, unchanged. Same fix as on Goldii-locks#418, which hit this in multisig_admin_override_refund_tests. No production code touched. 543 tests passing / WASM release build OK
godamongstmen897
added a commit
to qa-eden/escrow-contract
that referenced
this pull request
Sep 1, 2026
Dropping the EmergencyPauseLock dance from emergency_pause_admin_override is correct and is the point of Goldii-locks#399: the lock exists to close a reentrancy window around external calls, and this path makes none -- it reads the flag, compares, writes it back, and emits an event. EpLk is still taken by the paths that do call out. Three names had to be corrected against the enum as it actually exists: - DataKey::EmergencyPaused -> DataKey::Ep in lib.rs - DataKey::EmergencyPauseLock -> DataKey::EpLk in the new test suite One test then failed on the event tally. It calls the override twice and expects the count to go 1 then 2, but env.events().all() reports the most recent contract invocation rather than a running total, so the second call reports 1. Adjusted to assert the second call emits exactly one event, with the existing payload check confirming it is the new one. This is the same env behaviour that Goldii-locks#418 and Goldii-locks#438 ran into. 571 tests passing / WASM release build OK
godamongstmen897
added a commit
to esthertitilayo-dev/escrow-contract
that referenced
this pull request
Sep 1, 2026
The branch's own copy of admin_pause_escrow does not parse. A line from
the previous version was left dangling after the new writes:
.set(&DataKey::EmergencyPauseLock, &true);
env.storage().instance().set(&DataKey::Paused, &true);
.set(&DataKey::EpLk, &true); <- orphaned continuation
That produced 90 errors, all downstream of "expected expression, found
`.`". DataKey::EmergencyPauseLock is also not a variant -- the enum
calls it EpLk -- in lib.rs and twice in the new test suite.
Rebuilt the function around the two guards this PR is actually for:
assert_emergency_pause_not_locked before any write, and an early return
when the escrow is already paused so a redundant call mutates nothing.
Because that early return now handles the repeat case, the inner
`if !already_paused` that main used to gate the event is redundant, and
the publish is unconditional inside the lock.
Two of the new tests then failed on event tallies. They read
pause_event_count after is_paused / is_lock_held, and those helpers go
through env.as_contract -- env.events().all() reports the most recent
invocation, not a running total. Reordered so the tally is read first.
The idempotency case now asserts the second call emits nothing at all,
which is what "no-op" means here. Same env behaviour as Goldii-locks#418, Goldii-locks#428
and Goldii-locks#438.
576 tests passing / WASM release build OK
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
13 participants
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
Hardens \execute_admin_transfer\ (closes #347).
Problem
\execute_admin_transfer\ read the \PendingAdminTransfer\ entry and checked the multisig approval state before verifying the contract was initialized. In a never-initialized escrow:
The docstring error list listed \NotInitialized\ but the endpoint could not actually return it.
Fix
Added \Self::load_admin(&env)?\ as the first statement, before the pending-transfer lookup. This returns \NotInitialized\ on an uninitialized contract before any persistent storage is read. Reordered the docstring errors to match actual return order (\NotInitialized\ → \NoPendingAdminTransfer\ → \MultiSigThresholdNotMet). Added a dedicated test file (\execute_admin_transfer_tests.rs) covering: uninitialized contract, no pending transfer, multisig threshold not met (with storage assertions), the happy-path with event assertion, and a behavioral check that the old admin can no longer propose after the transfer.
Notes
This PR is orthogonal to #335 and #349.
Before merging: verify that CI passes all \cargo test\ and \clippy\ checks on this branch.