Comprehensive access-control boundary audit, initialize hardening, and sponsor-controlled deadline extension - #34
Merged
chonilius merged 12 commits intoJul 20, 2026
Conversation
|
@miraclesonly is attempting to deploy a commit to the chonilius' projects Team on Vercel. A member of the Team first needs to authorize it. |
Function-by-function audit across all three contracts (~20 public entrypoints) comparing intended access level against what's actually enforced in code, for MergeFi#30. Surfaces one real mismatch (initialize has no access control in any contract, fixed in the following commits) and cross-references two already-tracked findings (MergeFi#5, MergeFi#21) instead of duplicating fixes for them.
Focused deep-dive for MergeFi#30 on escrow::refund's permissionless-after- deadline path: whether "anyone can call it" is realistically load- bearing given caller incentives (it isn't, for altruistic strangers — no rational disinterested party pays fees for zero reward — but it is for the bankruptcy/custody-remoteness case, backend/admin permanently unavailable), and whether a malicious caller can grief a sponsor by triggering it early (no: funds always go to the sponsor, and the deadline is the sponsor's own prior commitment). Documents the design decision landing in the next few commits: a sponsor-only, monotonic extend_deadline, chosen over an admin/backend-controlled grace period because the latter would undermine the exact guarantee refund's permissionlessness exists to provide.
initialize performed zero require_auth() calls — the only guard was storage().instance().has(&DataKey::Admin), which blocks re-initialization but not the first call. Add admin.require_auth() so nobody can name a third-party address as admin without their consent (MergeFi#30). This is a partial mitigation, not a full fix for initializer front-running — see the doc comment and docs/access-control-audit.md for why, and MergeFi#33 for the actual structural fix. test_unauthorized_release_rejected needed its auth-mocking order adjusted: it deferred mock_all_auths() until after setup() specifically to test release's auth boundary, but setup()'s own initialize() call now needs auth too, so mocking has to start before setup() runs.
Same gap and same fix as escrow (MergeFi#30): initialize had no access control at all beyond the re-initialization guard. Add admin.require_auth() — see docs/access-control-audit.md for what this does and does not protect against.
Same gap and same fix as escrow/milestones (MergeFi#30): initialize had no access control at all beyond the re-initialization guard. Add admin.require_auth() — see docs/access-control-audit.md for what this does and does not protect against.
Answers the design question raised in MergeFi#30's refund analysis: a sponsor who wants more time before refund's permissionless path opens now has a real, safe mechanism to signal that, rather than no recourse at all. escrow.sponsor.require_auth() gates it, and new_deadline must be strictly later than both the stored deadline and the current ledger time (InvalidDeadline otherwise) — so it can only ever delay the permissionless window, never shorten it, and only the sponsor whose funds these are can call it. Full reasoning for why this shape was chosen over an admin/backend-controlled grace period is in docs/refund-permissionless-analysis.md.
Covers the claims in docs/access-control-audit.md for MergeFi#30: initialize/fund reject calls with no auth for the relevant address, refund's early (pre-deadline) path requires admin auth, and — explicitly required by the issue — refund's post-deadline path genuinely succeeds with zero auths mocked at all, proving the "anyone" path is real and not accidentally gated by something else.
Covers: only the sponsor can call it; a successful extension actually re-closes the permissionless refund window (old deadline passes, new one hasn't — refund still requires admin auth); non-increasing or not-yet-future deadlines are rejected (InvalidDeadline); and it's blocked once the escrow is already paid or refunded, same as the existing early-refund admin path.
Covers the claims in docs/access-control-audit.md for MergeFi#30: initialize/create_milestone reject calls with no auth for the relevant address, and allocate/release_issue/cancel_milestone all reject calls without admin auth.
Covers the claims in docs/access-control-audit.md for MergeFi#30: initialize/deposit reject calls with no auth for the relevant address, and withdraw rejects calls without admin auth.
Documents extend_deadline's signature/semantics, the initialize auth addition and its limits, cross-references the two new docs added in this PR, and refreshes the "verified in this session" test count (16/16 -> 34/34) to match the new access-control boundary matrix.
Pre-existing breakage unrelated to MergeFi#30, surfaced when rebasing this branch onto main after MergeFi#32 (adversarial-ordering split fix) merged: the test used the now-deprecated env.register_contract and called compute_split directly without the env.as_contract wrapper compute_split's storage access needs, so it panicked on any run ("this function is not accessible outside of a contract") and failed clippy's -D warnings for the deprecated call. Confirmed both failures reproduce on a pristine upstream/main checkout, independent of any change in this branch. Fix is mechanical: env.register_contract(None, ...) -> env.register(..., ()), and wrap the two direct compute_split calls in env.as_contract(&contract_id, || ...) so they run with the right storage context. No behavior/assertion changes.
miraclesonly
force-pushed
the
audit/access-control-refund-analysis
branch
from
July 20, 2026 17:45
7fd2d09 to
cea9372
Compare
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
2 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
Closes #30.
A systematic function-by-function access-control audit across all
three contracts, plus a focused analysis of
refund'spermissionless-after-deadline path (economics, griefing surface, and
the sponsor-control design question the issue raises).
Audit table
Full table in
docs/access-control-audit.md,covering every public function in
escrow,milestones, andmaintenance-pool. One real mismatch found and fixed (below); onealready-tracked issue (#5's
cancel_milestone/release_issueasymmetry) explicitly cross-referenced rather than duplicated, since
it's a state-machine gap, not an access-control one (the admin check
on
release_issueis already correct — it's what state it can acton that's the bug).
Finding:
initializehad no access control in any contractBeyond
storage().instance().has(&DataKey::Admin)(which blocksre-init, not the first call),
initializeperformed zerorequire_auth()calls in all three contracts. Fixed by addingadmin.require_auth()to all three.Important caveat, spelled out in the audit doc and doc comments:
this does not close initializer front-running — an attacker can
still race the legitimate deployer by calling
initializeand namingthemselves as admin, since they trivially satisfy
require_auth()for their own address. That requires a structural fix (an atomic
deploy+init via Soroban's
__constructor), which is a real deploy-flowchange (scripts, Makefile, README, breaks the already-initialized
testnet deployments) — too large to bundle here, so I filed
#33 to track it separately. This PR's fix should be read as closing
the narrower "name a third party as admin without their consent" gap,
not the front-running race.
refundpermissionless-path analysisFull writeup in
docs/refund-permissionless-analysis.md.Short version: calling
refundcosts the caller real transaction feesfor zero reward, so it's not realistically load-bearing via altruistic
strangers — the realistic callers are
mergefi-backendor the sponsorthemselves. Where "anyone can call it" actually earns its keep is the
tail case: if both the backend and the sponsor are unavailable, a
third party with some other reason to care can still return funds to
the sponsor with no coordination needed. That's a genuine
bankruptcy/custody-remoteness guarantee, not a UX nicety. Griefing is
not possible in the "steal funds" sense (always pays the stored
sponsor address, caller controls nothing) — the real gap is that the
sponsor has no way to change their mind about the deadline once set.
Design decision: added
extend_deadline(sponsor-only, monotonic —new_deadlinemust be strictly later than both the current deadlineand current ledger time). Considered and rejected an admin/backend-
controlled grace period, since that would undermine the exact
guarantee refund's permissionlessness exists to provide (recovery that
doesn't depend on the backend). Sponsor-only + monotonic-only
preserves that guarantee while directly answering the issue's question
with a real mechanism instead of "no, and that's fine."
Test plan
(initialize/fund/create_milestone/deposit reject calls without
the relevant address's auth; allocate/release_issue/
cancel_milestone/withdraw reject calls without admin auth;
refund's pre-deadline path requires admin auth; refund's
post-deadline path succeeds with zero auths mocked at all,
proving the permissionless path is real).
extend_deadlinetests: sponsor-only, successfully re-closes thepermissionless window after extension, rejects non-increasing/
not-yet-future deadlines, blocked once paid/refunded.
cargo test --workspace— 34/34 pass (17 escrow, 10milestones, 7 maintenance-pool; up from 16 before this PR).
cargo clippy --workspace --all-targets -- -D warnings— clean.cargo fmt --all --check— clean.cargo build --target wasm32v1-none --release— all threecontracts still build to valid wasm (matches the
wasm-buildCIjob).
Follow-ups filed
initializeto a Soroban constructor to structurallyclose the deploy/init front-running race that
require_auth()alonedoesn't solve.