[HIGH]bug(trade): v17 portfolio selection depends on RPC result ordering

[Bayyan16]

Summary

The v17 trade flow can select a different standalone portfolio account when multiple valid portfolios match the same wallet and market and the RPC response returns those matches in a different order.

The affected helper, findV17Portfolio(), correctly filters program accounts by:

• v17 portfolio magic;
• market public key;
• mutable portfolio owner.

However, after receiving the matching accounts, the current implementation directly parses and returns accounts[0].

No deterministic selection rule is applied before the selected portfolio is passed into the trade instruction as the taker's writable accountA.

As a result, two otherwise identical trade submissions can target different same-owner portfolio accounts when the only changed input is the ordering of matching results returned by getProgramAccounts().

Suggested Severity

High — transaction-target integrity

The report does not claim:

• wallet-signature bypass;
• cross-wallet account substitution;
• unauthorized transaction submission;
• direct private-key compromise.

The selected portfolio must still pass the existing mutable-owner validation.

The security and user-impact concern is that the client can assemble a signed trade against a different valid same-wallet portfolio from the one selected by related deposit, withdrawal, or account-display flows.

Maintainers may adjust the final severity based on the expected production frequency of multiple matching portfolio accounts.

---

Affected Component

• Branch: playground
• File: app/hooks/useTrade.ts
• Function: findV17Portfolio()
• Flow: v17 trade submission
• Affected transaction account: TradeCpi accountA
• Account metadata index: keys[2]

The affected account is the taker's standalone v17 portfolio:

keys: buildAccountMetas(ACCOUNTS_TRADE_CPI, [
  wallet.publicKey, // [0] signerA
  slabPk,           // [1] market
  accountA,         // [2] taker portfolio
  accountB,         // [3] LP portfolio
  matcherProg,      // [4]
  matcherCtx,       // [5]
  matcherDelegate,  // [6]
]);

---

Related Work and Scope Distinction

This issue is not a duplicate of the following reports or changes.

Issue #2204

Issue #2204 concerns unbounded public RPC proxy requests that can cause:

• server memory exhaustion;
• RPC billing exhaustion;
• oversized upstream responses;
• denial of service.

That issue affects:

app/app/api/rpc/route.ts

This report does not concern RPC proxy resource limits.

It concerns client-side selection of one account from a successful and already-filtered getProgramAccounts() response.

Issue #2364

Issue #2364 concerns the close-position flow continuing with cached React/UI position state after a required fresh on-chain read fails.

That issue affects:

app/hooks/useClosePosition.ts

The present report covers a different path:

RPC request succeeds
→ multiple accounts match the owner and market filters
→ useTrade selects array index 0
→ transaction accountA depends on response ordering

No RPC failure or cached-position fallback is required.

PR #128

PR #128 contains broad audit changes, including work around useTrade RPC lifecycle and cancellation.

This report specifically concerns the successful v17 portfolio-discovery path in:

findV17Portfolio()

and the nondeterministic selection of the portfolio forwarded as TradeCpi accountA.

---

Technical Root Cause

The current implementation obtains all portfolio accounts matching the v17 magic, market, and mutable-owner filters:

const accounts = await connection.getProgramAccounts(programId, {
  filters: [
    {
      memcmp: {
        offset: 0,
        bytes: V17_PORTFOLIO_MAGIC.toString("base64"),
        encoding: "base64",
      },
    },
    {
      memcmp: {
        offset: PORTFOLIO_PROVENANCE_MARKET_GROUP_OFF,
        bytes: marketPk.toBase58(),
      },
    },
    {
      memcmp: {
        offset: PORTFOLIO_OWNER_OFF,
        bytes: ownerPk.toBase58(),
      },
    },
  ],
});

The vulnerable selection then uses the first returned element:

if (accounts.length === 0) return null;

const data = Buffer.from(accounts[0].account.data);
const portfolio = parsePortfolioV17(data);

if (!portfolio.owner.equals(ownerPk)) {
  return null;
}

return accounts[0].pubkey;

This code implicitly treats the response index as a canonical account identifier.

The filters establish which accounts are eligible, but they do not establish which eligible account must be selected when more than one match exists.

The issue does not require assuming that every RPC provider deliberately randomizes results.

The unsafe invariant is simply:

selected portfolio = first matching array element

instead of:

selected portfolio = deterministic canonical matching account

---

Inconsistent Selection Across Application Flows

The related v17 deposit flow already canonicalizes matching portfolio accounts by public key:

const sorted = [...accounts].sort((a, b) =>
  a.pubkey
    .toBase58()
    .localeCompare(b.pubkey.toBase58()),
);

const data = sorted[0].account.data;
const portfolio = parsePortfolioV17(data);

if (!portfolio.owner.equals(ownerPk)) {
  return null;
}

return sorted[0].pubkey;

The current behavior can therefore produce the following application-level inconsistency:

Deposit / related portfolio flow
→ selects canonical lowest public key

Trade flow
→ selects whichever valid account appears at index 0

This means different frontend flows can resolve the same wallet and market to different portfolio accounts.

---

Preconditions

The issue requires all of the following:

1. The market uses the v17 standalone portfolio layout.
2. More than one portfolio account matches the same:
   • program;
   • market;
   • mutable owner.
3. The matching accounts are returned in different valid array orders between calls, providers, fixtures, or RPC responses.
4. The user submits a trade.
5. The selected account passes the existing post-fetch owner verification.

The issue is not triggered when only one portfolio matches.

---

Impact

The selected public key becomes the writable taker portfolio in the trade transaction.

If two matching portfolios contain different state, selecting a different account can cause the signed transaction to operate against a different portfolio context, including different:

• collateral or capital state;
• active legs;
• position state;
• health or margin state;
• fee-sweep requirements;
• pre-trade crank behavior.

Possible outcomes include:

• the trade being applied to an unexpected same-wallet portfolio;
• the transaction being rejected because the selected portfolio has incompatible state;
• UI state and transaction target becoming inconsistent;
• a portfolio different from the one used by deposit or display flows receiving the resulting trade state;
• confusing post-transaction balances or positions;
• additional user risk when the selected portfolio has materially different exposure or collateral.

This report does not claim that an attacker can substitute an account belonging to another wallet. The existing post-fetch mutable-owner check remains relevant and must be preserved.

The issue is specifically that multiple valid same-owner targets are not resolved deterministically before transaction construction.

---

Proof of Concept

A focused Vitest regression test was added at:

app/__tests__/hooks/useTrade.v17-portfolio-selection.test.ts

Test Model

The test constructs two distinct valid portfolio public keys:

const portfolioOne = new PublicKey(
  new Uint8Array(32).fill(21),
);

const portfolioTwo = new PublicKey(
  new Uint8Array(32).fill(22),
);

Both mocked accounts satisfy the same discovery conditions:

same v17 account type
same market
same mutable owner
different portfolio public key

The test executes the trade flow twice.

Execution A

The RPC returns:

[
  portfolioTwo,
  portfolioOne,
]

Execution B

The RPC returns:

[
  portfolioOne,
  portfolioTwo,
]

All other inputs remain unchanged:

wallet
market
trade direction
trade size
LP account
limit price
oracle state
matcher configuration

Capturing the Submitted Portfolio

The test captures the transaction passed to sendTx() and reads the final trade instruction:

const sendCall = mocks.sendTx.mock.calls.at(-1)?.[0];

const instructions = sendCall.instructions as Array<{
  keys: Array<{ pubkey: PublicKey }>;
}>;

const tradeInstruction =
  instructions[instructions.length - 1];

The selected taker portfolio is extracted from account metadata index 2:

const selectedAccountA =
  tradeInstruction.keys[2].pubkey;

This corresponds to:

TradeCpi accountA

Vulnerable Behavior

Before the fix:

RPC response [Portfolio B, Portfolio A]
→ selected accountA = Portfolio B

RPC response [Portfolio A, Portfolio B]
→ selected accountA = Portfolio A

Conceptually:

selectedAccountA([B, A]) !== selectedAccountA([A, B]);

Only the RPC result order changes.

The transaction target changes with it.

Regression Invariant

The regression test requires both executions to select the same canonical account:

expect(
  selectedFromReversedOrder.toBase58(),
).toBe(
  selectedFromCanonicalOrder.toBase58(),
);

It also verifies that the selected account equals the canonical public-key ordering:

expect(
  selectedFromCanonicalOrder.toBase58(),
).toBe(
  canonicalPortfolio.toBase58(),
);

Test Command

From the app workspace:

pnpm exec vitest run \
  __tests__/hooks/useTrade.v17-portfolio-selection.test.ts

Vulnerable Result

On the original implementation, the test fails because the two submitted accountA values differ:

Test Files  1 failed
Tests       1 failed

The assertion reports two different portfolio public keys for the canonical and reversed RPC result arrays.

---

Expected Behavior

For a fixed wallet and market, reversing the order of otherwise identical matching accounts must not change the portfolio used as TradeCpi accountA.

selection([Portfolio B, Portfolio A])
=
selection([Portfolio A, Portfolio B])
=
canonical portfolio

The account whose data is parsed and owner-validated must be the same account whose public key is returned to the transaction builder.

---

Actual Behavior

The original implementation selects:

accounts[0]

for both:

Buffer.from(accounts[0].account.data)

and:

return accounts[0].pubkey;

Therefore:

selection([Portfolio B, Portfolio A]) = Portfolio B
selection([Portfolio A, Portfolio B]) = Portfolio A

The selected writable transaction account depends on response ordering.

---

Proposed Fix

Canonicalize the matching accounts before selecting one:

if (accounts.length === 0) {
  return null;
}

const sorted = [...accounts].sort((a, b) =>
  a.pubkey
    .toBase58()
    .localeCompare(b.pubkey.toBase58()),
);

const data = Buffer.from(
  sorted[0].account.data,
);

const portfolio =
  parsePortfolioV17(data);

if (!portfolio.owner.equals(ownerPk)) {
  return null;
}

return sorted[0].pubkey;

Fix Properties

The proposed change:

• sorts a copied array and does not mutate the RPC response;
• applies a deterministic public-key ordering;
• uses the same sorted entry for parsing and return;
• preserves the post-fetch mutable-owner validation;
• aligns trade selection with the related v17 deposit flow;
• does not modify account filters;
• does not modify transaction instruction layout;
• does not modify trade size or direction;
• does not modify margin calculations;
• does not modify limit-price handling;
• does not modify signer requirements;
• does not modify the on-chain program.

---

Regression Validation

The focused regression test passes after the fix:

Test Files  1 passed
Tests       1 passed

The existing useTrade suite also passes:

Test Files  1 passed
Tests       16 passed

Combined result:

Test Files  2 passed
Tests       17 passed

Validation command:

pnpm exec vitest run \
  __tests__/hooks/useTrade.test.ts \
  __tests__/hooks/useTrade.v17-portfolio-selection.test.ts

Additional checks:

pnpm exec tsc --noEmit --pretty false
git diff --check

Both complete without errors.

Non-failing environment warnings observed during testing:

DeprecationWarning: module.register() is deprecated
bigint: Failed to load bindings, pure JS will be used

These warnings do not affect the regression result.

---

Acceptance Criteria

• findV17Portfolio() does not select an unnormalized accounts[0].
• Matching accounts are sorted using a deterministic public-key rule.
• Reversing RPC result order does not change TradeCpi accountA.
• The account data parsed for validation comes from the selected canonical entry.
• The returned public key comes from the same canonical entry.
• The mutable-owner post-fetch validation remains intact.
• Existing useTrade tests remain green.
• A regression test covers canonical and reversed RPC response orders.
• TypeScript validation passes.
• No transaction layout or on-chain behavior is otherwise changed.

[dcccrypto]

Appears already fixed on playground — flagging so this can be closed if a maintainer agrees.

In `app/hooks/useTrade.ts` on `playground`, portfolio selection is made deterministic explicitly for this reason: "getProgramAccounts() does not guarantee stable result ordering. Use the same canonical pubkey ordering as deposit and withdraw" — followed by [...nonLpAccounts].sort((a, b) => a.pubkey.toBase58().localeCompare(b.pubkey.toBase58())), with the market LP portfolio dropped before the sort/pick.

I am not closing it myself since it is not my issue and I may be missing context. But it is worth checking: I have now examined five open HIGH issues on this repo (#2416, #2414, #2362, #2364, #2371) and all five are already fixed on playground, which is where development actually lands (recent PRs #2442/#2436/#2434 all target it; #2431/#2432 merged there).

The tracker appears to be lagging the branch, so the open-HIGH count materially overstates outstanding work. I wasted several cycles writing fixes for defects that were already solved — see #2440.

[Bayyan16]

Verified. #2372 was merged into playground, and the current implementation filters out the LP portfolio, applies canonical Base58 pubkey ordering, and revalidates ownership. The regression test also confirms that reversed RPC ordering resolves to the same accountA. Closing this playground-scoped issue as completed.