Fix orphaned held assets on reversible-transfer recovery#611
Conversation
Make `release_held_funds_with_fee` transactional so a failed delivery rolls back the fee burn, and release funds before mutating metadata in `cancel_transfer`. Prevents recovery from permanently burning fees and desyncing the hold when the recipient cannot receive the asset (#93134).
n13
left a comment
There was a problem hiding this comment.
Review verdict: ✅ Approve (LGTM)
Correct, minimal, and well-tested fix for audit #93134. (Posting as a review comment because GitHub blocks formal self-approval — this PR was authored by the same account.)
Why the fix is correct
The orphaning stems from release_held_funds_with_fee doing two non-atomic steps — burn_held(fee) then transfer_on_hold(remaining). The critical path is recover_funds, which swallows the error and continues, so the extrinsic still returns Ok. Under FRAME's per-extrinsic storage layer, a partial execution (fee burned, transfer failed) was therefore being committed: the hold shrank by the fee while pending.amount still claimed the full amount → desynced/orphaned hold. Wrapping the helper in #[frame_support::transactional] makes burn+transfer all-or-nothing, so a failed delivery rolls back the burn and leaves the hold + metadata intact and retryable. This is exactly the right fix and is correctly scoped to the one helper with that pattern.
cancel_transfer reorder
Releasing before mutating metadata is functionally redundant on the cancel path — that path propagates the error via ?, so the default per-extrinsic transaction already rolls everything back. But it's harmless, makes the helper correct in isolation, and keeps ordering consistent with recover_funds. Good defensive change.
Verification
recover_funds_is_atomic_when_release_failsis a strong regression test: blocking the guardian's asset account forcestransfer_on_holdto fail, and it asserts the hold and total issuance are unchanged (proving the burn rolled back), that the pending transfer + sender index survive, and that recovery is idempotent.- Ran locally: the new test passes, and the full pallet suite is 49 passed, 0 failed.
- Confirmed the scheduled-execution path (
do_execute_transfer) uses a plainrelease(no fee burn), so it isn't subject to this bug.
Non-blocking nit
- The "retryable via
cancel" framing: while the guardian's asset account stays blocked,cancelalso routes funds to the guardian and would fail the same way, so "retryable" only holds once the blocking condition clears. Pre-existing semantics — just noting.
Fixes audit finding #93134 (Cancellation orphans held assets).
release_held_funds_with_feeis now#[transactional], so a failed delivery rolls back the fee burn instead of leaving held funds partially burned and desynced frompending.amount.cancel_transfernow releases funds before mutating metadata.Adds
recover_funds_is_atomic_when_release_fails(blocked guardian asset account); full pallet suite passes and the runtime builds.