fix(vault): read share ownership from mUSDC, not an internal map - #549
fix(vault): read share ownership from mUSDC, not an internal map#549determined-001 wants to merge 1 commit into
Conversation
withdraw() checked the caller's shares against the vault's own DataKey::Balance map and then burned from the real mUSDC balance. mUSDC is a normal transferable token, so a plain transfer() moved the second without touching the first and stranded the position for both parties: the recipient failed the share check against a map that still read zero, and the sender passed the same check and then reverted inside burn, holding none of the tokens it was trying to burn. Delete the map rather than try to keep two balances in step. Share ownership now comes from the mUSDC token itself, which is the balance the burn already operates on, so the check and the burn cannot disagree. The explicit check stays so callers still get a typed InsufficientShares rather than a panic out of burn. Entry stamping keys off whether an Entry record exists instead of the caller's share balance: an address holding transferred mUSDC has never deposited, so its first deposit is a real entry rather than a top-up. A full exit still clears the record, so a re-depositor starts a fresh clock. Cost basis and entry time cannot follow a transfer. They are history, not a holding, so unlike the balance they are not derivable from a snapshot, and moving them means observing the transfer as it happens. mUSDC is a Stellar Asset Contract: its transfer is the built-in implementation, and the vault is its admin (mint and burn), not the owner of its code, so there is no hook to observe. Rather than leave that silently wrong, an address holding no mUSDC now reports zero for both, so records stranded by a transfer-out are never shown as a live position, and a transferred-in holder reports no recorded basis instead of a fabricated one. Off-chain, computePosition treats a zero basis alongside a positive balance as "none recorded" and reports no yield, instead of counting the whole position as profit. Fund safety is unaffected either way: every holder can withdraw exactly what they hold. Only the yield figure for a transferred position is unknown, and the vault contract docs now say so and what closing it would require. closes drydocs#504
|
@determined-001 is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
One more, not anchorable since coordinator.ts isn't touched by this PR: fetchCoordinatorPosition in packages/stellar-sdk-helpers/src/coordinator.ts:117-122 computes earned = Math.max(0, deposited - principalUnits) directly off get_principal, without the hasBasis guard this PR just added to computePosition in positions.ts. A coordinator-deployed vault's transferred-in holder still gets shown their entire balance as yield there, the same bug this PR fixes in the sibling function reading the identical get_principal call. Worth fixing alongside this PR since it's the same class of bug against the same new contract behavior.
| /// | ||
| /// Making basis follow a transfer needs a share token the vault controls | ||
| /// the code of; see `apps/docs/architecture/vault.md`. | ||
| pub fn get_principal(env: Env, address: Address) -> i128 { |
There was a problem hiding this comment.
The if get_position(...) == 0 { return 0 } guard only protects against a fully-empty holder. It doesn't clear the stale record itself, only withdraw()'s full-exit branch does that. An address that deposits, then moves its shares out via a plain transfer() instead of withdraw(), keeps its old Principal/Entry records sitting in storage. If that same address later receives an unrelated transfer-in (or makes a genuine new deposit), get_position is now nonzero again, the guard no longer applies, and get_principal/get_entry_time return the old stale basis and entry time, not the 0 this PR documents as the guarantee for a transferred-in holder. A genuine re-deposit compounds it: deposit() only skips the entry stamp when !has(&entry_key) (still true here, since the key was never deleted) and adds the new amount on top of the stale Principal, mixing the two deposits' cost bases together.
Needs the plain-transfer-out case covered too, not just the withdraw-to-zero case, either by having get_principal/get_entry_time also clear the stale record inline once they detect a zero-position holder with leftover storage, or by re-checking get_position freshness at both read sites before trusting the stored value.
2 participants
closes #504
Summary
withdraw()checked the caller's shares against the vault's ownDataKey::Balancemap, then burned from the real mUSDC balance. A plaintransfer()moves the second without touching the first, so after A transfers to B:InsufficientSharesagainst a map that still said zero, despite B holding the tokens.burn, because A no longer held the tokens.The position was unreachable through
withdraw()for both parties.This removes
DataKey::Balanceentirely and reads share ownership from the mUSDC token — the same balance the burn operates on, so the check and the burn can no longer disagree. That is the fix the issue asked for: delete the redundant source of truth rather than keep two in step.Changes
DataKey::Balanceis gone.withdrawreadsTokenClient::balance(caller);get_positionreturns the token balance. The explicit share check stays so callers still get the typedInsufficientSharesrather than a panic out ofburn.Entryrecord, not the balance. An address holding transferred mUSDC has never deposited, so its first deposit is a real entry, not a top-up. A full exit still clears the record, so a re-depositor starts a fresh clock.get_entry_time/get_principalreport0for an address holding no mUSDC, so a record left behind by a transfer-out is never shown as a live position.computePositiontreats a zero basis alongside a positive balance as "no basis recorded" and reports no yield, instead of counting a transferred-in holder's entire balance as profit.apps/docs/architecture/vault-contract.md, plus the storage table and per-function notes.The
Entry/Principalsplit is not implementable as agreed — please readIn the issue thread I proposed splitting cost basis and entry time inside mUSDC's
transfer(), on the grounds that "the vault side already owns mint/burn on mUSDC", and you signed that off. That premise was wrong, and I'd rather say so than quietly ship something else under the same heading.mUSDC is a Stellar Asset Contract (
scripts/deploy-testnet.sh:71,stellar contract asset deploy --asset "MUSDC:$DEPLOYER_ADDRESS"). Itstransferis the built-in SAC implementation. The vault is the SAC's admin, which lets it mint and burn — it does not give it the SAC's code, and there is no hook. So there is no point at which the vault can observe a transfer and move the sender's basis, which is exactly what the pro-rata split needed.Basis and entry time are history, not holdings, so unlike the balance they can't be derived from a snapshot after the fact. That leaves them not following a transfer. What I did instead is make that state honest rather than wrong:
0basis, meaning "nothing recorded", and the UI shows no yield rather than 100% yield;0for basis and entry time instead of a phantom position;No path loses funds or blocks a withdrawal; the gap is confined to the yield figure for a transferred position.
Options for closing it properly, your call:
TOTAL_PRINCIPALand derive any holder's basis asbalance * TOTAL_PRINCIPAL / TOTAL_SH, which follows a transfer automatically. Cheap, but it degrades the accurate per-depositor figure everyone gets today into a vault-wide average, so I did not want to make that trade without you.I've defaulted to (3) in this PR so the lockup fix isn't held up behind a token redeployment. Say which you want and I'll open the follow-up.
Verification
cargo test -p meridian-vault— full vault suite green, including 9 new tests.InsufficientShares, no revert). Plus partial transfer with both holders withdrawing their halves, proportional basis retirement after a partial transfer-out, both reporting behaviours above, entry stamping for a transferred-in depositor, and a full-exit-then-redeposit regression guard for the new entry-stamp condition.npm run test,npm run coverage,npm run typecheck,npm run typecheck:api,npm run lint,npx prettier --check .— all green.Not verified on testnet: the live vault predates this contract (#514 covers the redeployment), so this ships as source plus tests, like the other vault changes in flight.