fix: Check permissioned domain on private vault withdrawal - #7977
fix: Check permissioned domain on private vault withdrawal#7977tyalymov wants to merge 3 commits into
Conversation
A private vault restricts who may take part in it, but VaultWithdraw only ever checked that the destination was allowed to hold the underlying asset. A participant could therefore withdraw to an account the domain owner never admitted, and the funds left the domain. Under fixCleanup3_4_0, a withdrawal from a private vault to a third party now requires both the submitter and the destination to be members of the vault's permissioned domain, read from the share issuance as VaultDeposit does. Withdrawing to self is not checked, so that losing vault access cannot strand funds already deposited. The asset issuer is always allowed to receive, which keeps the return path for frozen assets open even for a submitter who lost access. Public vaults, VaultClawback and LoanBrokerCoverWithdraw are unaffected.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
The change adds a permissioned-domain check to VaultWithdraw::preclaim, gated behind fixCleanup3_4_0, mirroring VaultDeposit's existing domain check. The logic is sound: it correctly skips the check for self-withdrawals and for payouts to the asset issuer (with the exemption bypassing both parties' domain checks, as explicitly justified in the MR description for the frozen-asset-return case), reads the domain from the share issuance for consistency with VaultDeposit, and returns tecNO_AUTH when a private vault has no domain configured. The new test covers destination inside/outside the domain, the issuer, the vault owner, self-withdrawal (implicit and explicit), a submitter who lost domain access, and confirms public vaults are unaffected. I did not find correctness, security, or resource-management issues in the added code — no gaps in the pattern were found elsewhere in the diff (VaultClawback/LoanBrokerCoverWithdraw are explicitly and correctly out of scope per the description).
Comments now describe the rule rather than the gate that carries it: the rules.enabled condition already tells the reader which amendment applies, and naming it in prose only rots once the amendment activates. The two domain checks are also spelled out instead of looping over an initializer list of the two accounts. With a three-line body and exactly two elements the loop saved nothing and asked the reader to think about temporaries.
…hdraw_destination_domain_check
There was a problem hiding this comment.
Solid, well-scoped change: adds a permissioned-domain check to VaultWithdraw on the way out, gated behind fixCleanup3_4_0, mirroring the existing VaultDeposit check. The self-withdrawal and issuer-recipient exemptions are implemented consistently with the description, and the new test covers the described matrix (domain member, non-member, issuer, owner, self, and a submitter who lost domain access) plus a public-vault control case. Only one edge case caught my eye and is called out below for confirmation rather than as a confirmed bug; everything else in the diff looks correct.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
High Level Overview of Change
A withdrawal from a private vault to a third party now requires both the submitter
and the destination to be members of the vault's permissioned domain. Gated behind
fixCleanup3_4_0.Context of Change
A private vault restricts who may take part in it, but
VaultWithdrawonly everchecked that the destination was allowed to hold the underlying asset, through
requireAuth. There was no domain check anywhere in the withdrawal path, so aparticipant could withdraw to an account the domain owner never admitted and the
funds left the domain.
VaultDepositalready applies the equivalent check on theway in; this closes the gap on the way out. The domain is read from the share
issuance, the same source
VaultDeposit::preclaimuses.Two cases deliberately skip the check:
funds that were already deposited, so redeeming your own shares stays available
regardless of domain membership.
for frozen assets open. This exemption covers both parties rather than just the
recipient: otherwise a submitter who had lost domain access could not return a
frozen asset to its issuer, which is precisely the situation that path exists for.
Reviewers may want to look closely at this one, it is the one judgement call in
the change rather than a direct reading of the access-control rules.
Public vaults,
VaultClawbackandLoanBrokerCoverWithdraware unaffected.VaultClawbacksends to the issuer through a separate path and does not usesfDestinationat all.API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Consensus-affecting: a withdrawal that succeeds today can return
tecNO_AUTHoncefixCleanup3_4_0is enabled. Behavior is unchanged while the amendment is off.Test Plan
New test
testVaultWithdrawPrivateDestinationDomaininsrc/test/app/Vault_test.cpp,run twice, with and without
fixCleanup3_4_0. Every account in it holds the asset,so anything blocked is blocked by the domain check alone. Covered: destination inside
the domain; destination outside it; destination is the issuer; destination is the vault
owner; withdrawal to self with and without an explicit
Destination; a submitter wholost domain access withdrawing to self, to a domain member, and to the issuer; and a
public vault paying out to a non-member.