fix: oracle sync and protocol-wide vault withdrawal protection#2142
fix: oracle sync and protocol-wide vault withdrawal protection#2142alvarengao wants to merge 5 commits intodrift-labs:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughUpdated Devnet USDC oracle config and added both on-chain and client-side guards preventing withdrawals to the spot market vault by disallowing the vault as the destination token account. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (sdk/driftClient)
participant Program as On-chain Program (Withdraw)
participant SpotMarket as SpotMarket Account
participant UserToken as User Token Account
Note over Client,SpotMarket: Client computes spot market and destination associated token address
Client->>SpotMarket: fetch spotMarket (marketIndex)
SpotMarket-->>Client: spotMarket.vault
Client->>UserToken: compare destinationAddr == spotMarket.vault
alt destination == vault
Client-->>Client: throw Error (prevent instruction)
else destination != vault
Client->>Program: send Withdraw instruction (destinationAddr)
Program->>SpotMarket: program-side account constraint checks
alt constraint violation (destination is vault)
Program-->>Client: abort (InvalidSpotPosition)
else
Program-->>UserToken: transfer tokens to destination
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@programs/drift/src/instructions/user.rs`:
- Around line 4757-4758: Add the same account-identity guard to the
WithdrawIsolatedPerpPosition instruction that exists on Withdraw: in the
WithdrawIsolatedPerpPosition constraint list ensure you compare
spot_market_vault.mint == user_token_account.mint and add a constraint that
user_token_account.key() != spot_market_vault.key() returning
ErrorCode::InvalidSpotPosition; this mirrors the protection in Withdraw and
prevents the self-transfer path that handle_withdraw_from_isolated_perp_position
performs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4573388c-50bf-42e5-b76e-53e94bd713b4
📒 Files selected for processing (1)
programs/drift/src/instructions/user.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/src/driftClient.ts`:
- Around line 4379-4383: The current vault-check is bypassable and hardcodes
QUOTE_SPOT_MARKET_INDEX; move this validation into the shared instruction path
by adding a private helper (e.g., assertDestinationIsNotVault(spotMarketVault:
PublicKey, destination: PublicKey, label: string)) and call it from the shared
withdraw-to-user-token instruction builders
(getWithdrawFromIsolatedPerpPositionIx and
getWithdrawFromIsolatedPerpPositionIxsBundle) after deriving spotMarketIndex
from perpMarketAccount.quoteSpotMarketIndex and obtaining
spotMarketAccount.vault; remove use of the hardcoded QUOTE_SPOT_MARKET_INDEX in
this flow so the check always runs and throws if userTokenAccount equals the
spot market vault.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ef9e635-5c3d-4e01-b458-fc398cd5f1f1
📒 Files selected for processing (1)
sdk/src/driftClient.ts
| // Ensure the destination account is not the vault itself to prevent state inconsistency | ||
| const spotMarket = this.getSpotMarketAccount(QUOTE_SPOT_MARKET_INDEX); | ||
| if (spotMarket.vault.equals(userTokenAccount)) { | ||
| throw Error('Destination userTokenAccount cannot be the same as the spot market vault'); | ||
| } |
There was a problem hiding this comment.
Move this validation into the shared ix path and derive the vault from the perp market.
This check is currently bypassable because getWithdrawFromIsolatedPerpPositionIxsBundle() and getWithdrawFromIsolatedPerpPositionIx() are public. It also hardcodes QUOTE_SPOT_MARKET_INDEX, while the actual instruction uses perpMarketAccount.quoteSpotMarketIndex at Line 4475. That matters here because WithdrawIsolatedPerpPosition in programs/drift/src/instructions/user.rs (Lines 4941-4971) has no equivalent on-chain user_token_account != spot_market_vault constraint, so this wrapper is not enough to prevent the invalid vault destination case.
🔧 Suggested fix
public async withdrawFromIsolatedPerpPosition(
amount: BN,
perpMarketIndex: number,
userTokenAccount: PublicKey,
subAccountId?: number,
txParams?: TxParams
): Promise<TransactionSignature> {
- // Ensure the destination account is not the vault itself to prevent state inconsistency
- const spotMarket = this.getSpotMarketAccount(QUOTE_SPOT_MARKET_INDEX);
- if (spotMarket.vault.equals(userTokenAccount)) {
- throw Error('Destination userTokenAccount cannot be the same as the spot market vault');
- }
-
const instructions =
await this.getWithdrawFromIsolatedPerpPositionIxsBundle(
amount,private assertDestinationIsNotVault(
spotMarketVault: PublicKey,
destination: PublicKey,
label: string
): void {
if (spotMarketVault.equals(destination)) {
throw new Error(`Destination ${label} cannot be the same as the spot market vault`);
}
}
public async getWithdrawFromIsolatedPerpPositionIx(
amount: BN,
perpMarketIndex: number,
userTokenAccount: PublicKey,
subAccountId?: number
): Promise<TransactionInstruction> {
const userAccountPublicKey = await getUserAccountPublicKey(
this.program.programId,
this.authority,
subAccountId ?? this.activeSubAccountId
);
const perpMarketAccount = this.getPerpMarketAccount(perpMarketIndex);
const spotMarketIndex = perpMarketAccount.quoteSpotMarketIndex;
const spotMarketAccount = this.getSpotMarketAccount(spotMarketIndex);
this.assertDestinationIsNotVault(
spotMarketAccount.vault,
userTokenAccount,
'userTokenAccount'
);
// existing logic...
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/src/driftClient.ts` around lines 4379 - 4383, The current vault-check is
bypassable and hardcodes QUOTE_SPOT_MARKET_INDEX; move this validation into the
shared instruction path by adding a private helper (e.g.,
assertDestinationIsNotVault(spotMarketVault: PublicKey, destination: PublicKey,
label: string)) and call it from the shared withdraw-to-user-token instruction
builders (getWithdrawFromIsolatedPerpPositionIx and
getWithdrawFromIsolatedPerpPositionIxsBundle) after deriving spotMarketIndex
from perpMarketAccount.quoteSpotMarketIndex and obtaining
spotMarketAccount.vault; remove use of the hardcoded QUOTE_SPOT_MARKET_INDEX in
this flow so the check always runs and throws if userTokenAccount equals the
spot market vault.
alvarengao
changed the title
alvarengao
changed the title
alvarengao
changed the title
|
I have refactored the SDK validation logic to address the feedback. Vault protection is now centralized in the core instruction builders (getWithdrawIx and getWithdrawFromIsolatedPerpPositionIx), using dynamic vault derivation. This ensures that all withdrawal paths are protected against accidental transfers to the protocol vaults without relying on hardcoded indices. Ready for final review. |
1 participant
Description
The oracle address defined in
sdk/src/constants/spotMarkets.tsfor Devnetspot/0(USDC) was pointing to a stale PythLazer feed, diverging from the current on-chain state (spot/6). This updates the index 0 oracle to the activePYTH_STABLE_COIN_PULLfeed to prevent margin calculation failures on Devnet.Fixes: - #2138
Testing
spotMarkets.tscompiles cleanly.Summary by CodeRabbit