[Bug] Transferring mUSDC permanently locks the position

[collinsezedike]

Description

MeridianVault's internal share accounting and the mUSDC token's real balances can drift apart, permanently locking a position.

withdraw() checks the caller has enough shares by reading the vault's own DataKey::Balance(caller) map (packages/contracts/vault/src/lib.rs:255-258), then later burns from the real mUSDC token balance via TokenClient::new(&env, &musdc).burn(&caller, &shares) (packages/contracts/vault/src/lib.rs:278). Nothing keeps these two in sync: mUSDC is a standard transferable token, and a plain transfer() moves the real token balance without touching DataKey::Balance.

If A transfers mUSDC to B:

• B's DataKey::Balance entry is still 0, so B's withdrawal fails immediately with InsufficientShares, even though B now holds the real tokens.
• A's DataKey::Balance entry still shows the old amount, so A's withdrawal passes the InsufficientShares check, but the subsequent burn() call fails since A no longer holds the real tokens, and the whole transaction reverts.

Neither party can ever withdraw that position through the vault again.

Steps to Reproduce

1. Deposit USDC into the vault as address A, receiving mUSDC shares.
2. Transfer some or all of that mUSDC to address B using mUSDC's standard token transfer().
3. Have B call vault.withdraw() for the transferred shares.
4. Have A call vault.withdraw() for the same shares.

Expected Behavior

mUSDC stays freely transferable (it's a legitimate share token, tradeable and usable as collateral elsewhere), and a transferred position withdraws normally through its new holder.

Actual Behavior

B's withdrawal fails with InsufficientShares (internal map still shows 0). A's withdrawal passes the internal check but reverts on the burn() call since A no longer holds the tokens. The position becomes permanently unreachable through the vault's normal withdraw() flow for both parties.

Environment

Field	Value
Network	testnet
Wallet	N/A
Protocol affected	None
Browser (if frontend)	N/A
Node.js version	N/A
pnpm version	N/A

Possible Cause / Fix

Drop the internal DataKey::Balance map entirely and derive share ownership directly from mUSDC's real token balance (TokenClient::balance()) on every withdraw() call. That removes the second, redundant source of truth that causes the drift, rather than patching around it: making mUSDC non-transferable would fix the lockup by deleting a legitimate feature, and adding transfer hooks to keep the internal map in sync would add a second system that has to be maintained by hand indefinitely, and is exactly the kind of thing that reintroduces this bug the next time a code path forgets the hook.

This will also touch DataKey::Entry(address) and DataKey::Principal(address), which are keyed off the same caller and currently assume the depositor is the same address that eventually withdraws. Once mUSDC is correctly transferable end to end, a transferred position's cost basis and entry time need to move with the token too, or they'll stay wrong for the new holder even after the withdrawal lockup itself is fixed. Scope that into this same fix rather than leaving it as a follow-up gap.

Additional Context

Found during an independent audit of the vault contract, verified directly against current source (packages/contracts/vault/src/lib.rs) rather than taken on the audit's word alone.

[Chigybillionz]

Hi, I’ve reviewed this issue and I’m confident I can fix it. I’d like to take it on.

[collinsezedike]

@Chigybillionz this is one of your oldest open applications here, and it's a real one, a transfer that permanently locks a user's position. Given you're already assigned to #513 and have applied to several others, can you confirm you can take this on now? Happy to assign once I hear back.

[determined-001]

Hi, I would like to work on this issue. I will fix the withdrawal lockup by removing the internal DataKey::Balance map and replacing it with a direct call to TokenClient::balance() on the mUSDC token contract to derive share ownership at withdrawal time, eliminating the second source of truth that drifts out of sync whenever mUSDC is transferred between addresses. I will also update DataKey::Entry and DataKey::Principal to move with the token on transfer so the new holder's cost basis and entry time are correct rather than orphaned at the original depositor's address, scoping this into the same fix rather than leaving it as a follow-up gap that would leave the new holder with incorrect accounting even after the withdrawal lockup itself is resolved. I will write tests covering the exact reproduction steps from the issue including deposit, transfer to a second address, and withdrawal by both the original and new holder, and verify all cases resolve correctly on testnet before submitting. Can I be assigned?

[collinsezedike]

@determined-001 DataKey::Entry/DataKey::Principal are per-address single values. If a holder transfers only part of their mUSDC balance, not all of it, how does your fix split cost basis and entry time between the original holder (who keeps some shares) and the new holder (who gets the rest)?

[determined-001]

@collinsezedike Good question, and worth calling out: unlike DataKey::Balance (which can be replaced outright by querying TokenClient::balance() on demand), DataKey::Entry and DataKey::Principal are historical facts (when and at what cost basis shares were acquired), not something derivable after the fact from a balance snapshot. A partial transfer genuinely needs to split that history between sender and receiver at the moment it happens, there's no way around observing the transfer itself.

Plan for the split, on a partial transfer of amount from A to B:

• Principal (cost basis): pro-rate by the fraction moved. principal_moved = A.principal * (amount / A.balance_before_transfer). Subtract that from A's principal, add it to B's. This is a proportional/weighted-average split, the same approach used for average-cost accounting, not a FIFO lot system (FIFO would need per-lot tracking, a much bigger change and probably overkill for what these fields are used for here).
• Entry time: if B had no existing position, B simply inherits A's entry time as-is for the moved shares. If B already holds shares, merge as a principal-weighted average of B's existing entry time and A's, so a large incoming transfer moves B's effective entry time more than a small one does. A's own entry time is untouched by a partial transfer out, only the remaining principal shrinks; the shares that stay with A still entered when A originally deposited.
• Full-balance transfers stay the simple case already covered: A's Entry/Principal for that address get cleared, B's fields either get set fresh (if B had none) or merged the same way as above.

The catch: doing this correctly requires observing every transfer as it happens, so contrary to the DataKey::Balance fix, mUSDC's transfer() can't stay a bare-bones token call for this specific bookkeeping. Since the vault side already owns mint/burn on mUSDC, I'd add the split logic directly inside mUSDC's transfer() (a synchronous update in the same contract, not an external hook/callback to a third contract), so it's inside the one contract we already control rather than a second system bolted on. That's different from the "don't add transfer hooks" note in the issue write-up, but that note was about Balance, which really is redundant and derivable; Entry/Principal aren't, so they need this.

Let me know if average-cost-weighted splitting is the right call here, or if you'd rather have exact per-lot (FIFO) tracking instead. I can scope either into the same PR, but wanted to confirm the approach before writing it.

[collinsezedike]

@determined-001 The partial-transfer split holds up, pro-rata principal and weighted-average entry time is the right call over FIFO here. Assigning this to you.