feat: harden caller authorization and precondition guards in admin_pause_escrow (closes #349) - #439
Merged
godamongstmen897 merged 16 commits intoSep 1, 2026
Conversation
…ocks#298) Audited tax_withholding_deductions's existing validation against this issue's requirement ("assert that bad setups are rejected immediately with descriptive error types") and found the existing coverage already extensive: NotInitialized, NotFunded, InvalidMilestone, InvalidRatio, InvalidAmount (zero balance, overflow), and InvalidStatus for Released/Refunded milestones are all implemented and tested (test.rs, tax_withholding_tests.rs). One real gap: the status check only excluded Released/Refunded, not Disputed — unlike raise_dispute_inner and resolve_dispute elsewhere in this file, which both treat Disputed as its own case via an exhaustive match. A disputed milestone's funds are meant to be frozen pending resolve_dispute; tax_withholding_deductions could still compute and persist a TaxWithholdingRecord for one, moving money around that freeze. No existing test combined raise_dispute with tax_withholding_deductions, so this was uncovered on both the implementation and test side. Fixes the gap by converting the two equality checks to an exhaustive match over MilestoneStatus (so a future new variant fails to compile here instead of silently falling through as allowed), matching the established pattern elsewhere in this file, and adds a test exercising it via the existing dual-signature test fixture. Not touched: admin_tax_withholding_deductions, a separately-implemented sibling function (per its own doc comment, "arrived from a separate PR under the same name") that has no status check at all — a larger gap, but outside this issue's named scope (Module/Component: tax_withholding_deductions).
Add two early-exit guards at the top of admin_set_yield_rate before any auth check or ledger read/write: 1. load_job_meta() -> NotInitialized if the contract has not been initialized yet. Runs first so callers on an uninitialised contract get a clear, typed error rather than a storage miss. 2. assert_not_paused() -> Paused if an emergency pause is active. Yield-rate changes while the contract is suspended could silently affect the next accrual cycle once the pause lifts, so they are rejected here. The ordering (preconditions -> pause -> auth -> validation) ensures that an unauthorized caller never learns whether the admin key exists from the error variant alone. Also adds admin_set_yield_rate_tests.rs with 14 focused tests: - Unauthorized caller returns Unauthorized; no storage mutated - Unauthorized caller with zero rate: same - Paused contract returns Paused; no storage mutated - Unauthorized caller on paused contract returns Paused (not Unauthorized) - Unpause restores normal operation - Uninitialized contract returns NotInitialized; no storage mutated - Rate > 10000 returns InvalidRatio; no storage mutated - Rate = u32::MAX: same - Happy path: zero, 1, 500, 9999, 10000 all accepted and persisted - Repeated calls update YieldRateBps to the latest value
|
@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! 🚀 |
# Conflicts: # contracts/milestone-escrow/src/lib.rs
main guards tax_withholding_deductions with two equality checks that reject Released and Refunded. This branch replaces them with an exhaustive match that also rejects Disputed, which is the whole point of Goldii-locks#298: a Disputed milestone's funds are frozen pending arbitration, so computing and persisting a tax split for it here would move money around a dispute that resolve_dispute is meant to gate. Took the branch's side. It strictly widens main's guard -- everything main rejected is still rejected -- and being exhaustive means a future MilestoneStatus variant fails to compile here rather than silently falling through as permitted. 544 tests passing / WASM release build OK
…8-tax-withholding-validation fix(tax_withholding_deductions): reject Disputed milestones (Goldii-locks#298)
The conflict was just a module declaration; it now sits under #[cfg(test)] alongside the others, matching what Goldii-locks#429 established. Two compile fixes in the new suite: - setup_funded_escrow was not in scope. Imported from crate::test, the same way admin_override_cancel_tests.rs does it. - DataKey::YieldRateBps does not exist. admin_set_yield_rate persists the rate as the yield_rate field of the YieldConfig entry under DataKey::YieldConfig, so read_yield_rate reads that instead. None still means "never written", which is what the no-mutation cases want. Three of the new tests then failed, and they were right to. They pause with emergency_pause and expect admin_set_yield_rate to return Paused, but it called only assert_not_paused, which reads DataKey::Paused -- the flag admin_pause_escrow sets. The emergency pause is a separate, stronger freeze recorded under DataKey::Ep, and nothing was checking it here, so a yield-rate change went straight through an emergency pause while the weaker admin pause blocked it. admin_set_yield_rate now rejects under either flag. That is the hardening this PR set out to add; the tests had simply reached for the pause that was not wired up. No existing test asserted the old behaviour. 558 tests passing / WASM release build OK
…set-yield-rate-guards feat: harden caller auth and precondition guards in admin_set_yield_rate
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
…-pause-admin-override-storage-footprint Goldii-locks#399 - fix(emergency_pause_admin_override): remove unnecessary EmergencyPauseLock overhead
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
5 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 \�dmin_pause_escrow\ (closes #349).
Problem
\�dmin_pause_escrow\ could mutate instance storage in two ways before returning a business-rule error:
Fix
Added \Self::assert_emergency_pause_not_locked(&env)?\ as the second statement, before any write. Moved the already-paused check outside the closure and immediately before any storage write — on an already-paused escrow the function now returns \Ok(())\ with zero storage mutations and no event. Removed the closure entirely; the three storage writes are now unconditional linear statements. Added the \EmergencyPauseInProgress\ error to the docstring. Added a dedicated test file (\�dmin_pause_escrow_tests.rs) covering: uninitialized contract, non-admin caller, emergency-pause lock held (with no-mutation assertions), idempotent already-paused (event count assertion), and the happy path with event assertion.
Notes
This PR is orthogonal to #335 and #347.
Before merging: verify that CI passes all \cargo test\ and \clippy\ checks on this branch.