fix: bound set_admin acceptance window (B1)#40
Conversation
341e6f8 to
af05839
Compare
| /// 120_960 ledgers past the current ledger) panics [`MoonlightError::AcceptanceWindowTooLong`]. | ||
| /// `live_until_ledger == 0` is exempt from the ceiling and cancels a pending transfer (the | ||
| /// library requires the cancel call to name the current pending address). | ||
| pub fn set_admin(e: &Env, new_admin: Address, live_until_ledger: u32) { |
There was a problem hiding this comment.
Receiving the arg from the external invocation is a good approach here to ensure there is consistency between simulation and submission.
One common mistake would be to dynamically calculate a value based on the current ledger, which could cause the transaction to be simulated on block n and submitted on block n+1, which could invalidate it due to mismatching values.
| const INSTANCE_BUMP_AMOUNT: u32 = 7 * DAY_IN_LEDGERS; | ||
| const INSTANCE_LIFETIME_THRESHOLD: u32 = INSTANCE_BUMP_AMOUNT - DAY_IN_LEDGERS; | ||
|
|
||
| // B1: cap the acceptance window of a pending ownership transfer in-contract. Without this the |
There was a problem hiding this comment.
I assume the B1 stands for the audit finding, right?
Is there an added benefit of having such reference in the comment?
My gut feeling is that this will become historical context that won't be explicit for future readers when simply looking into the source. The comment context seems detailed enough to explain the section.
…(B1) set_admin now takes a required live_until_ledger and forwards it to ownable::transfer_ownership instead of hardcoding max_live_until_ledger(), which pinned every pending ownership transfer to the network-max (~180 day) window. A non-zero window beyond an in-contract 7-day ceiling (7 * DAY_IN_LEDGERS = 120_960 ledgers past the current ledger) panics the new MoonlightError::AcceptanceWindowTooLong; live_until_ledger == 0 is exempt and preserves the library's cancel path. The two-step accept_admin is unchanged. The standard operating window for a real handover is 3 days (51_840 ledgers), documented on set_admin; there is deliberately no default so max can never silently return. Addresses RV audit finding B1.
af05839 to
e769ee7
Compare
What
Both
channel-authandprivacy-channelhadset_admin(new_admin)callownable::transfer_ownership(e, &new_admin, e.ledger().max_live_until_ledger()), hardcoding the acceptance window of a pending ownership transfer to the network max (~180 days). A pending transfer therefore never auto-expired, so aPendingOwnerkey compromised while a transfer lingered stayed exploitable for months, and the library's0-cancel path was unreachable.Approved fix (RV audit finding B1):
set_admin(e, new_admin, live_until_ledger: u32)— required parameter, forwarded totransfer_ownership. No default, somaxcan never silently return.live_until_ledgerbeyondcurrent_ledger + 7 * DAY_IN_LEDGERS(120_960 ledgers) panics the newMoonlightError::AcceptanceWindowTooLong(code 1014). Panics rather than silently clamping.live_until_ledger == 0is exempt from the ceiling and preserves the library's cancel path (cancel names the current pending address).accept_adminand cancel semantics are unchanged — the ceiling is a tighter check layered on top of the library's existing< current/> maxvalidation.current_ledger + 3 * DAY_IN_LEDGERS= 51_840 ledgers), documented onset_admin; sensible range 24h–7d. Both contracts get the identical change.Tests
current + 120_960accepted,+1rejects withAcceptanceWindowTooLong;0cancels a pending transfer (formerly-pending admin can no longer accept).cargo test --workspace: 66 passed / 0 failed. Both WASMs build (stellar contract build). fmt/clippy clean on changed code.Notes
set_admin. Downstream cascade (handled separately after merge, NOT in this PR): regenerate SDK/TS bindings, bump SDK + consumers, update ops/deploy scripts that callset_admin, redeploy + re-init the WASMs.Refs audit finding B1.