Withdrawal Flow

The AutoApproveWatcher calls getAllWithdrawalRequests(offset, limit) on the SynthetixDepositContract to fetch pending withdrawals for automatic approval. However, this function was removed from the SynthetixDepositContract and moved to a separate Lens contract. The relayer only has the SynthetixDepositContract address configured (contract_address), with no Lens contract address available. When getAllWithdrawalRequests is called on the current contract deployment, the call reverts. This causes fetchRequestedIds to return an error, and processRequested fails. Ultimately, all automatic watcher approvals will fail and withdrawals may remain in Requested status until expiry — locking user funds.

Add the Lens contract address to the relayer configuration and update AutoApproveWatcher to query the Lens contract for withdrawal data.

CreateActor() performs a Load, builds a new actor, and then Stores without any synchronization. Concurrent invocations (handleDeposit() handler and handleSubaccountCreate() handler both run in parallel JetStream/NATS goroutines) can each instantiate and start their own actor for the same subaccount before either Store executes. Any initialization that was handled by the losing actor (e.g., deposits applied to the SubAccount before calling CreateActor()) is effectively lost which can lead to deposits not being reflected in the live actor.

Use an atomic “create-if-absent” guard (mutex or sync.Map.LoadOrStore) around CreateActor() so only one actor per subaccount can be created.

The on-chain withdrawal flow supports a dispute mechanism where the Relayer or Watcher can call disputeWithdrawals() to flag suspicious withdrawals for Guardian review, and the Guardian can subsequently call resolveDisputedWithdrawal() to approve or deny them. However, the off-chain system does not implement this functionality:

1. disputeWithdrawals() and resolveDisputedWithdrawal - Contract bindings exist but are never called.
2. When a WithdrawalStatusDisputed event is observed, the code only logs a warning and takes no action: l.logger.Warn("[PROCESSOR] Withdrawal DISPUTED on-chain - no action taken" Ultimately, there is no mechanism to dispute suspicious withdrawals and trigger Guardian review, violating the intended withdrawal flow and spec.

Add logic to support disputeWithdrawals, and resolveDisputedWithdrawal, and the Disputed status.

When GlobalWithdrawalsPaused is emitted, the listener continues to submit disburse transactions for Validated withdrawals without checking the pause state. Those on-chain calls revert while paused. Because the listener returns an error on disbursal failure and cannot skip or defer the failing log, it gets stuck retrying the same Validated log and never advances the block checkpoint. While stuck, subsequent AssetDeposited events are not processed off-chain, so user deposits stop being credited and any other events in later blocks are missed. Although withdrawalsGloballyPaused = true should only pause withdrawals, it effectively disabled deposits due to deposits not being credited off-chain, and can lead to failed margin top-ups and liquidation.

Before submitting disbursals, verify whether withdrawals are paused and queue the disburse to be handled upon unpause.

The listener handles GlobalWithdrawalsPaused events but does not listen for or track TokenSpecificWithdrawalsPaused events. When a guardian pauses withdrawals for a specific token with pauseWithdrawalsFor(), the listener remains unaware. It continues to call SubmitBatchDisbursal for Validated withdrawals containing that token, which reverts on-chain with TokenWithdrawalsPaused. Since the listener returns an error on disbursal failure and cannot skip the failing log, it gets stuck and cannot advance the block checkpoint. This stalls all event processing including deposits for unrelated tokens.

Add handlers for TokenSpecificWithdrawalsPaused and TokenSpecificWithdrawalsUnpaused events in routeEvent and skip disbursals when paused.

In handleWithdrawalRequest, when SubmitWithdrawalRequest returns an error, the handler returns nil which causes the NATS message to be ACKed and permanently removed from the queue. This can simply happen in a period of RPC connectivity issues, in which case no on-chain transaction is made, no PendingTransaction is saved, yet the withdrawal request is ACKed and discarded. User withdrawal requests can be permanently lost due to transient infrastructure failures. A brief RPC outage will silently drop the withdrawal with no retry path, locking user funds. Furthermore, PendingWithdraw was increased when the user requested a withdrawal but never cleared, hence the withdraw quantity is stuck in PendingWithdraw and the user may not re‑request the same amount through the API due to the user's reduced available balance. Note that other consumers are also exposed to the unconditional ACK pattern, such as in cow_order_subscriber the handleCowOrder function and handleMatchingMsg in subaccount/manager.go.

Return an error if the tx was not broadcast so the request will be retried.

The withdrawal handler creates a buffered response channel and closes it if the handler times out after 5 seconds in case <-time.After(5 * time.Second). However, the Actor's _sendWithdrawalResponseIfChannelOpen function only checks for nil, not closed. If the Actor processes the withdrawal after the handler timeout, sending on the closed channel causes a panic, crashing the Actor goroutine. Once the Actor is crashed, it is not automatically restarted and will block all subsequent operations on that subaccount affecting trading, withdrawals, etc.

Don't send a response if the channel is closed or don't close the channel in the handler.

Unlike order placement which enforces rate limits with ctx.OrderRateLimiter.CheckOrderLimit(), withdrawal requests have no rate limiting. An attacker can spam withdrawal requests to exhaust the relayer's NATS consumer capacity and block legitimate withdrawals. The relayer's withdrawal consumer is configured with MaxAckPending: 100 within the withdrawal_subscriber.go file. Each message remains pending until WaitMined() completes (up to 5 minutes) or the transaction fails. When an attacker submits 100 withdrawal requests:

• NATS delivers all 100 messages
• All 100 AckPending slots are consumed immediately
• NATS stops delivering new messages until slots free up
• Legitimate withdrawal #101 is queued indefinitely Even though the on-chain contract only allows one active withdrawal per user, the off-chain queue is shared across all users so the off-chain DoS is possible. An attacker with multiple accounts (or one account repeatedly failing at the contract level) can take total control over the queue and block others from withdrawing their assets indefinitely.

Add withdrawal rate limiting at the API layer.

Any unauthenticated client can open a TCP connection to the public REST API (e.g. /v1/trade or /v1/info) and slowly drip HTTP headers/body bytes without ever finishing the request. In services/api/internal/config/config.go the configuration already exposes ReadTimeout and WriteTimeout, but in services/api/cmd/server/main.go the Echo server is created with echo.New() and started with e.Start(cfg.ServerAddress) without ever assigning those timeout values to the underlying http.Server. The Go net/http server defaults both timeouts to zero, meaning the connection is kept open indefinitely. An attacker can therefore hold thousands of concurrent goroutines by keeping connections half-open, exhausting file descriptors and worker capacity and preventing legitimate traders from placing orders.

Before calling e.Start, set the underlying server timeouts, e.g. e.Server.ReadTimeout = cfg.ReadTimeout and e.Server.WriteTimeout = cfg.WriteTimeout, and optionally cap ReadHeaderTimeout and IdleTimeout. This enforces per-connection limits and prevents Slowloris-style resource exhaustion.

The WebSocket service lacks checks that would enforce the configured maxConnectionsPerIP/maxSubscriptionsPerIP thresholds, and the health monitor is only consulted by the /health/ready endpoint rather than the upgrade handler. An attacker can therefore open thousands of /ws/info or /ws/trade sockets and subscribe to all broadcasters without being rejected, consuming goroutines, Redis/JETStream state, and NATS subscriptions until the service stops accepting new traders. This attack will prevent legitimate trader connections to /ws/trade and order execution.

1. Enable the Redis-backed per-IP connection limit inside handleWebSocketConnection (return HTTP 429 when limitExceeded is true).
2. Enable the per-IP subscription cap inside handleSubscribeRequest (increment the counter atomically and reject when the limit is hit).
3. Gate the WebSocket upgrade path on healthMonitor.ShouldAcceptNewConnections()/MaxTotalConnections (return 503 when the health monitor rejects), so the service can shed load before it runs out of goroutines or NATS subscriptions.

Attacker-controlled HTTP POST data reaches every exposed service without bounds: compose.yml publishes rpcproxy-service on 0.0.0.0:3001, so arbitrary clients can hit Proxy.ServeHTTP in services/rpcproxy/internal/services/rpcproxy/proxy.go, which immediately calls io.ReadAll(r.Body) without wrapping http.MaxBytesReader. Likewise, services/api/cmd/server/main.go mounts public POST /v1/info and POST /v1/trade, and their handlers (services/api/internal/handlers/info_handler.go and services/api/internal/handlers/trade_handler.go) read the entire c.Request().Body with io.ReadAll before validation or auth. Because none of these paths enforce content-length limits or streaming, a single oversized POST to any endpoint forces the Go process to allocate memory proportional to the payload, exhausting RAM and crashing the rpc proxy as well as both trading/info APIs. Ultimately users will not be able to place/cancel orders, adjust margin, withdraw, etc.

Apply strict per-endpoint request-body limits before any io.ReadAll call. Wrap each handler’s Body with http.MaxBytesReader (configurable sane defaults such as 10 MiB for rpcproxy, 5 MiB for /v1/info, 1 MiB for /v1/trade), return HTTP 413 when exceeded, and optionally enforce matching limits at load balancers.

UpdateSubaccountStatus() is supposed to flip a subaccount between ACTIVE/FROZEN, but it loads the entire sub_accounts row and then calls Save(&subAccount) after only changing the Status and UpdatedAt fields:

subAccount.Status = status
subAccount.UpdatedAt = snx_lib_utils_time.Now()

if err := r.db.WithContext(ctx).Save(&subAccount).Error; err != nil {
    return nil, snx_lib_grpc.SubaccountStatus_ACCOUNT_STATUS_UNSPECIFIED, fmt.Errorf("failed to update subaccount status: %w", err)
}

A concurrency issue can occur because the admin gRPC endpoint UpdateSubaccountStatus() runs in its own goroutine, and other service is always mutating the same sub_accounts rows during deposits, transfers, withdrawals, etc. So if a deposit/withdrawal updates balances just before an admin freeze/unfreeze runs, the status call replays its stale snapshot and undoes those balance changes.

Use a field-scoped update to update only the single status field rather than the whole row.

WebSocket trading connections authenticate by submitting an EIP-712 typed data message to /trade and the service calls AccountAuthenticator.ValidateAccountAuth with SupportExpiration explicitly disabled. That function never validates that the provided timestamp is fresh -- it only reserves that timestamp in Redis for 10 minutes as a nonce. Once the Redis key expires, the same signed payload (and the same signature) may be replayed to reauthenticate the victim’s sub-account without requiring a new signature from the wallet. An attacker who captures a single auth message/signature pair (e.g., through a compromised client or a MITM where TLS is not enforced) can wait 10 minutes, replay the same payload, and bypass the victim’s private key to re-open the trading socket, read account streams, or issue trades on their behalf. Ultimately, replay of a captured auth request after the nonce key expires allows remote attackers to impersonate the victim’s wallet address and sub-account, subscribe to sensitive account updates, and send authenticated trade actions without providing a new signature. This gives the attacker full access to the victim’s trading WebSocket session.

Call ValidateTimestamp (or otherwise enforce expiresAfter) before reserving the nonce and store the nonce indefinitely so seen timestamps can never be reused, eliminating the replay window even after the Redis TTL expires.

The off-chain relayer service is currently designed to operate with a single private key for all on-chain operations including:

1. RELAYER_ROLE: Submitting withdrawal requests with requestWithdrawal()
2. WATCHER_ROLE: Casting approval votes with castWatcherVotes()
3. TELLER_ROLE: Disbursing validated withdrawals with disburseWithdrawals() The AutoApproveWatcher component in the relayer service automatically casts watcher votes for pending withdrawals using the same TransactionManager and private key that performs relayer and teller functions. This can be observed in services/relayer/cmd/server/main.go where a single txManager is passed to both the AutoApproveWatcher and other components, and in services/relayer/internal/core/auto_approve_watcher.go where votes are cast using w.txManager.CastWatcherVote(). The on-chain contract, however, was primarily designed for a multi-party model with N-of-M watcher quorum. When a withdrawal receives enough watcher votes to meet the watcherQuorum threshold, it transitions from Requested to Validated status. The deployment configuration in .env.example expects 3 independent watchers with a 2-of-3 quorum, with separate addresses for WATCHER_1, WATCHER_2, and WATCHER_3, and WATCHER_QUORUM=2. This architectural mismatch creates a deployment dilemma: If deployed with intended security (3 watchers, quorum of 2):

• The single relayer can only cast 1 vote per withdrawal
• Withdrawals never reach the required 2-vote quorum
• All withdrawals remain stuck in Requested status until they expire
• The system is non-functional for withdrawals If deployed with single watcher (quorum of 1):
• Compromise of the relayer key grants complete control over all withdrawals
• The watcher quorum system provides limited additional security/health
• The multi-party security model is negated If multiple relayer instances are deployed (attempting a workaround):
• Nonce collisions occur as all instances query PendingNonceAt() independently
• Duplicate transaction submissions occur for the same withdrawal
• Race conditions arise in NATS message consumption
• The system becomes unreliable and prone to failures

Separate watcher and teller from the relayer so they can run with with separate, independent keys.

CreateTradeTypedData falls back to signing only subAccountId/action/nonce/expiresAfter for every requestAction except the few order APIs. TradeHandler ultimately uses that same typed data to validate signatures, so the EIP-712 verification never covers parameters such as withdraw amount, destination, leverage or delegated signer details. As soon as an attacker can intercept or modify a client request (MITM, compromised relay, etc.) they can rewrite these fields after the client has signed the request and still have the request succeed. For example withdrawCollateral uses the user-supplied amount/destination directly when building the WithdrawCollateralRequest that is sent to the trading service, so a tampered request is processed with whatever values the attacker injected while the signature remains valid. Every other handler that does not have a custom typed data (transferCollateral, updateLeverage, add/remove delegated signer, etc.) is affected because their payloads never entered the signed message space. An attacker who can intercept or modify a trade API request (e.g., an HTTPS MITM, proxy, or compromised client) can increase withdrawal amounts, redirect collateral withdrawals to their address, or change leverage/transfer targets without touching the signature or nonce, effectively stealing funds or corrupting subaccount state while still passing signature verification.

Create an EIP-712 schema for each action and include every user-controlled parameter in the typed data so the signature is bound to the actual payload (amount, destination, symbol, permissions, etc.). For now treat any action without a custom schema as invalid (fail fast) and add typed data builders before allowing the handler to run. This ensures any tampering of the JSON body invalidates the signature and stops unauthorized parameter changes.

The subaccount service publishes transfer requests to subaccount.event.transfer.request, waits for a response with 30 second timeout, and subscribes to trading.event.transfer.response to receive responses. No consumer exists for SubaccountEventTransferRequest and the trading service does not publish TradingEventTransferResponse. Every TransferBetweenAccounts call will timeout and fail, hence the transfer feature between subaccounts is not functional.

Implement the missing transfer handler in trading service. Subscribe to subaccount.event.transfer.request. Validate and execute the transfer and then publish trading.event.transfer.response.

Liquidation runs in two phases: Phase 1 – Position liquidation (processMMRBreachState()): cancel open orders, liquidate positions. If checkMarginBreach() ever reports ImmediateSLPTakeoverBreachType here, the engine immediately sets state.Stage = LiquidationStageSLPTakeover and skips to phase 2 (correct behavior). Phase 2 – Debt handoff (handleAllPositionsLiquidated()): once no positions remain, the engine re-checks margins to decide weather any debt must be transferred to SLP. The code only handles breach.Type == MMRBreachType in this step:

breach := le.checkMarginBreach()

if err := le.handleAutoExchange(context.Background()); err != nil {
	...
}

if breach.Type == MMRBreachType {
	// escalate to SLP takeover
	...
	return nil
}

le.logger.Debug("MMR breach resolved through liquidation", ...)
le.state.Reset()
return nil

If phase 1 started as a normal MMRBreachType but, after liquidation, the refreshed check now returns ImmediateSLPTakeoverBreachType, this branch is missed and the engine resets even though AAV < MMR/2 still holds. This scenario is realistic when liquidation realizes larger losses than expected: e.g., slippage, taker fees, funding payments, or negative USDT debt remaining after handleAutoExchange(). Those effects can push the account from “AAV < MMR” to “AAV < MMR/2” once positions are gone. This creates a window where an insolvent account is allowed to submit new orders until the next EnforceAccountHealth()

In handleAllPositionsLiquidated(), treat ImmediateSLPTakeoverBreachType the same way as MMRBreachType (i.e., escalate to SLP takeover) so that accounts below MMR/2 after liquidation don’t get incorrectly reset.

GetSLPActorForMarket() function attempts to find a SLP mapping for market, if no mapping exists. After searching for the defaultSLPActor the code immediately logs the result

m.logger.Debug("No SLP mapping for market", "market", marketSymbol, "defaultSLPActor", defaultSLPActor.subAccount.ID)

However if the loop never finds an SLP actor, defaultSLPActor remains nil, and dereferencing .subAccount.ID panics, so a missing SLP account can crash the trading service. Compare with GetSLPActorForCollateral(), which avoids this by checking for nil before logging

Check defaultSLPActor is not nil before logging, as is done in GetSLPActorForCollateral()

The withdrawal handler applies each WithdrawProcessedEvent to the actor. If the consumer is recreated with DeliverAll or messages are redelivered (e.g. on restart), the same event will be applied twice, double‑adjusting PendingWithdraw/Quantity. The same issue is present with deposits, such that the same DepositProcessedEvent could be applied again, inflating the user's collateral balance.

Track processed request IDs in the Actor and then check if a specific RequestID has already been processed before apply changes in the Actor.

Function SubmitWithdrawalRequest holds signerLock for the entire duration including WaitMined() which has a 5 minute timeout. Because CastWatcherVote, SubmitBatchDisbursal, and subsequent withdrawal submissions all take the same lock, one slow or congested withdrawal will block and delay every other transaction type for the duration.

Consider releasing the lock after SendTransaction.

The Relayer listener converts the on-chain subAccountId, a uint256 in the DepositEntry struct and AssetDeposited event, into a Go uint64: SubaccountID: event.SubAccountId.Uint64(), Subaccount ID values greater than 2^64-1 will silently truncate. While the deposit still credits the same beneficiary, different large on-chain sub-account IDs collapse to the same uint64 within that user, causing misattribution across a user’s own sub-accounts.

Change the type of DepositEntry.subAccountId to uint64.

Function calculateOrderValuesByMarket sums every open order for a symbol, including reduce‑only orders, when computing buy/sell notional for CalculateOrderMarginRequirement. Reduce‑only orders are not meant to reserve additional margin as they only reduce exposure and they are already excluded from order RMR calculations. Including them in calculateOrderValuesByMarket overstates worst‑case notional, causing normal orders to appear riskier, leading to unnecessary rejections or extra order cancellations.

Exclude reduce‑only orders in calculateOrderValuesByMarket.

The off-chain system only validates that withdrawal amounts are positive (amount > 0), but does not enforce the on-chain withdrawalMinimum configured per collateral in the contract's CollateralConfig. This allows dust withdrawals to traverse the entire pipeline before reverting on-chain, allowing for the following griefing vector:a

1. User submits withdrawal for dust amount
2. Request passes all off-chain validation (API, Actor, Subaccount Service)
3. Funds are moved to PendingWithdraw in Actor state
4. Relayer submits on-chain transaction
5. Contract reverts due to withdrawalMinimum check
6. Protocol pays gas for the reverted transaction
7. User repeats with minimal cost and continues to drain relayer funds on ETH

Enforce the on-chain minimum at the API layer to reject invalid withdrawals.

ProcessTransfer() is designed to enforce master to subaccount relationships (either master to sub or sub to its master). But when both accounts are masters (MasterID == 0), no relationship check runs, that means any user who knows another master account’s ID can push collateral directly to it, bypassing wallet ownership and intended account-family boundaries, creating an authorization gap. In a regulated derivatives context, that undermines typical AML/KYC controls: it becomes easier to layer funds by bouncing them across masters, potentially obscuring the source within the protocol.

Add an explicit guard for master to master transfers: either disallow them outright or require both masters to share the same WalletAddress (or other ownership identifier).

When a user initiates a withdrawal, handleWithdrawal deducts from Quantity and adds to PendingWithdraw in memory. If the actor crashes after the in-memory update (before the NATS message is delivered and persisted in publishPnlSettledUpdate), the actor will restart with stale state (PendingWithdraw = 0). Meanwhile, the withdrawal request continues through the pipeline and succeeds on-chain. When handleWithdrawalResponse receives the success confirmation, it checks if PendingWithdraw >= msg.Quantity and sees PendingWithdraw (0) < msg.Quantity (Non-Zero), it logs a warning and sets PendingWithdraw = 0 without making any adjustment to Quantity. Consequently, the user receives funds on-chain, but the actor's internal balance remains inflated by the withdrawn amount. The user can withdraw the phantom collateral again or use it for trading.

Persist before sending withdrawal request so proper collateral recovery can take place.

The handleCollateralChange function enqueues collateral update jobs into the database through AddCollateralJob. However, there is no worker in the codebase that processes these CollateralJobs. As a result, collateral add/remove events emitted on-chain are never reflected in the off-chain state. This can lead to inconsistencies where tokens remain usable off-chain after being removed on-chain, or newly added collateral tokens are never recognized by the off-chain services.

Process the jobs added to the DB.

When a stop-loss or take-profit fires, the actor converts it into a normal market/limit order before sending it to the matching engine.

if err := a.convertConditionalToRegularOrder(order); err != nil {
	a.logger.Debug("Invalid order type", "orderID", order.ID, "orderType", order.Type, "error", err)
	return false
}
// …
if err := a.publishOrder(ctx, requestID, chunkOrder); err != nil {
	a.logger.Error("Failed to send triggered conditional order", "orderID", order.ID, "quantity", firstChunkQuantity, "error", err)
	return false
}

If that send fails (network hiccup, NATS publish error, etc.), the converted order is left in memory as a normal order instead of a conditional one. On the next price tick, the system looks at the same order, sees it is the wrong order type, and will error out.

func (a *Actor) convertConditionalToRegularOrder(order *types.Order) error {
	switch order.Type {
	case types.OrderTypeStopMarket, types.OrderTypeTakeProfitMarket:
		order.Type = types.OrderTypeMarket
	case types.OrderTypeStopLimit, types.OrderTypeTakeProfitLimit:
		order.Type = types.OrderTypeLimit
	default:
		return fmt.Errorf("invalid conditional order type: %v", order.Type)
	}
	return nil
}

The user still sees an active TP/SL, but it will not trigger properly until they delete and re-create it. The impact is that a single error can brick a TP/SL

Do not mutate the conditional order unless the publish succeeds. Either work on a copy, or save the original type and restore it if publish fails.

The OrderRateLimiter is implemented using in-memory storage (sync.Map) rather than distributed storage, despite accepting a Redis client parameter that is discarded (_ any). Each API or WebSocket service instance maintains its own independent rate limit counters. In production deployments with multiple instances behind a load balancer, the effective rate limit scales linearly with instance count:

• 1 instance -> 100 orders/sec per subaccount
• 3 instances -> 300 orders/sec per subaccount
• 10 instances -> 1,000 orders/sec per subaccount An attacker doesn't need to actively exploit this - standard load balancer distribution automatically spreads requests across instances, bypassing the intended rate limit. This could enable order spam, unfair trading advantages, or DoS on downstream systems with resource exhaustion.

Consider implementing distributed rate limiting using the Redis client that is already passed to NewOrderRateLimiter.

When the matching engine sends order fill, accept, or cancel responses, ActorManager.handleMatchingMsg delivers them through actor.Send(), which routes to sendResponse(). This function performs a non-blocking send into a 1,000-slot buffered channel. If the actor's response inbox is full (e.g., actor blocked processing another handler, or flooded with responses), the message is dropped and the caller only logs the error. Consequently, users may have filled orders they never see reflected in their account and positions and balances diverge from the matching engine's state.

Instead of dropping on full, block with timeout or implement a retry mechanism with exponential backoff.

The MarketData gRPC handler accepts user-supplied startTime, endTime, and limit, but it only caps the limit when the client requests more than 1,000 rows. There is no enforcement when limit is zero or negative, so defaults (zero) silently disable the LIMIT clause, and the full requested time range is forwarded to the service. Downstream the service also lacks any guardrails: it happily truncates the provided range to timeframe boundaries and pipes limit directly to the repository, and the repository only applies LIMIT when limit > 0. An attacker can therefore call GetCandlesticks with a narrow timeframe (e.g., 1m) but startTime months or years in the past while leaving limit unset; the code will scan & return every candle in that enormous range, loading entire tables into memory and saturating the database before replying. Ultimately, an unauthenticated client can ask for minute-level candles spanning months/years with limit unset. Each request will load the full historical dataset for that range into memory and stream it back, exhausting the MarketData service’s RAM/CPU and the PostgreSQL connection pool, ultimately taking the service (and any callers that depend on it) offline.

Enforce sensible caps and pagination. Treat limit values <= 0 with default value (e.g 100 or 1000) and reject ranges whose duration exceeds a configured maximum per timeframe. Alternatively, paginate via cursors/streaming and reject requests that would read more than a few thousand rows. Ensure the code always applies a LIMIT, even when the client omits it.

The calculateReduceOnlyAdjustments function correctly counts pending reduce-only orders (in pendingOps) toward totalReduceOnlySize to detect when combined reduce-only exposure exceeds position size. However, it only selects adjustment targets from OpenOrders, not from pending operations. If multiple reduce-only orders are submitted rapidly before the first is accepted by the matching engine, the total reduce size can exceed the existing position, violating the intended behavior detailed with the comment in calculateReduceOnlyAdjustments: "This prevents race conditions where multiple reduce-only orders are placed rapidly" Consider the scenario where a user has a 10BTC position:

• Order A: 10 BTC reduce-only sell is submitted; it sits in pendingOps (not yet in OpenOrders).
• Before A is accepted, Order B: another 10 BTC reduce-only sell is submitted.
• calculateReduceOnlyAdjustments sums size as 10 (pending A) + 10 (new B) = 20 BTC vs 10 BTC position -> 10 BTC excess.
• But the adjustment candidates (reduceOnlyOrders) are built only from OpenOrders; pending orders aren’t added there, so it returns no adjustments.
• Order B is allowed through; later both A and B end up in OpenOrders, totaling 20 BTC reduce-only against a 10 BTC position.
• If A fills first and fully closes the position, B can still fill; with no position left, that fill is treated as opening a new short (reduce-only reserves zero margin), so the user unintentionally flips into new exposure.

When iterating pendingOps in calculateReduceOnlyAdjustments, append eligible pending reduce-only orders to reduceOnlyOrders (not just to totalReduceOnlySize) so they can be adjusted when excess exists.

Function calculateSimulatedRMR within ValidatePreTradeSafety only considers OpenOrders for RMR calculation. Pending orders in pendingOps are not considered which could lead to underestimating the total margin required and pushing the user to immediate safety margin breach.

Consider pending operation in pre-trade safety calculations. Additionally, extend calculateOrderValuesByMarket() to merge pending create operations.

In account_events.go, both handleDeposit and handleWithdrawResponse ACK the JetStream message immediately after actor.Send(), which only enqueues the message. It does not wait for the actor to apply the change. If the actor crashes after the send and before processing, the message is lost (already ACKed) and the state change never occurs. For deposits, on-chain funds can be received but never credited. For withdrawal responses, PendingWithdraw may never be decremented (success) or restored (failure), leaving balances stale.

Do not ACK until the actor confirms processing.

The documentation states that tellers "constantly scan withdrawalRequests that are stale and batch turn them into Rejected". However, the off-chain teller implementation does not invoke cancelStaleWithdrawals() on the contract. The TransactionManager in the relayer service only implements SubmitBatchDisbursal() for calling disburseWithdrawals(). There is no corresponding function to call cancelStaleWithdrawals(), and no scheduled task or event listener triggers stale withdrawal cancellation on-chain. Additionally, the documentation references a Rejected status that does not exist in the contract. The actual status enum is:

• None
• Requested
• Validated
• Disbursed
• Denied
• Disputed
• Cancelled
• Expired

Consider implementing an off-chain task within the teller that periodically identifies withdrawal requests past the withdrawalExpiryTimeout and batch-calls cancelStaleWithdrawals() to transition them to Expired status to free the user's withdrawal slot, and/or allow an on-chain request to clear the prior requested withdrawal if expired. Additionally, update the documentation to reflect the correct status enum values used in the contract.

The partial liquidation portion uses position.Quantity * 25% and is sent to the matcher without rounding to the market’s lot/quantity exponent. The send path truncates extra decimals, so small partials can become zero and be rejected, which immediately triggers SLP takeover. Consider the following scenarios:

• 2-decimal lot (0.01): Position 0.03 BTC -> 25% = 0.0075 -> truncates to 0.00 -> IOC rejected -> SLP takeover
• Integer lot: Position 1 contract -> 25% = 0.25 -> truncates to 0 -> SLP takeover

Before sending the liquidation order, round the partial size to the market’s allowed quantity step (and respect MinTradeAmount), and enforce a minimum of one lot so the size never truncates to zero.

ProcessTransfer() stores a channel in s.transferResponses and schedules a cleanup that removes the entry and closes the channel as soon as the wait (time.After(constants.TransferRequestTimeout)) finishes

	defer func() {
		s.transferMutex.Lock()
		delete(s.transferResponses, requestID)
		close(responseChan)
		s.transferMutex.Unlock()
	}()

If the wait exits due to the timeout, that cleanup runs immediately. Meanwhile the response listener still has the channel pointer and could try to deliver the actual reply after the channel has been closed.

	s.transferMutex.RLock()
	responseChan, exists := s.transferResponses[event.RequestID]
	s.transferMutex.RUnlock()

	if exists {
		responseChan <- &event   // panics if channel was closed by timeout cleanup

If the handler hits a closed channel, Go panics with “send on closed channel,” crashing the NATS subscriber goroutine.

Remove the line close(responseChan) from the deferred cleanup. In this pattern the channel has only one receiver (ProcessTransfer()), the handler either finds the channel (and delivers the response) or doesn’t (and logs that nobody is waiting), so there’s no resource leak because the channel is small (buffer size 1) and short-lived (cleaned by garbage collector once nothing references it)

All liquidation state (MMR breach counts/window timers, current action, partial‑vs‑full attempt) lives only in LiquidationStateDetails in memory. A process crash/restart clears it. A position that had already had a partial cut will be treated as a first attempt again after restart and only partially liquidated instead of proceeding to full liquidation. In‑flight liquidations are also forgotten. A position that was on its 2nd liquidation attempt (should do 100%) would restart at attempt 1 (25% partial). This will further delay the liquidation and potentially exacerbate losses.

Make liquidation logic resumable and idempotent: derive the next step from stored state plus current position health; if prior attempt was partial and still under margin, proceed directly to full liquidation. Avoid re-applying partial cuts.

If handleSLPTakeover() fails transfer of collateral to SLP, yet it still removes collateral, reglardless of transferErr

transferErr := le.actor.slpTransfer.TransferCollateralToSLP(...)
if transferErr != nil {
    le.logger.Warn("Failed to transfer collateral to SLP", ...)
    // still proceeds
}
le.publishCollateralDeletion(collateralSymbol)
collateralSymbolsToDelete = append(collateralSymbolsToDelete, collateralSymbol)

func (le *LiquidationEngine) clearLiquidatedAccount(...) {
    for symbol, position := range le.actor.subAccount.Positions {
        le.publishPositionCloseEvent(symbol, position)
    }
    le.actor.subAccount.Positions = make(map[string]types.Position)
    ...
}

Which publishes close events and zeroes every asset. As a result, collaterals are deleted without being transferred, producing permanent loss when SLP transfer fails. This scenario occurs when slpActor.Send() enqueues the transfer message onto the actor’s inbox channel, and it can fail when that channel is full. The request inbox capacity is 2,000 entries (ActorRequestInboxChannelSize), so a failure scenario is when the SLP actor already has 2,000 pending messages it hasn’t processed yet

Abort or retry the takeover when transfer fails, or track which assets actually transferred and only clear those.

The official Deposit contract product specification at https://www.notion.so/OFFICIAL-Deposit-Contract-2230a80a459e8049a50adcf2fbf46a93 has multiple small differences with the actual implementation of the contract.

• The spec says Owner/Guardian can pause; contract allows only GUARDIAN_ROLE to pause deposits/withdrawals (global or per-token) and only OWNER_ROLE to unpause
• The spec says Watchers can validate/dispute/reject in "Takes Submitted withdrawals and batch turns them into Validated, Disputed or Rejected" but in the contract Watchers can only vote or dispute.
• The spec throughout mentions the states Submitted/Approved/Rejected and no Expired e.g. "while status is Requested or Approved"; the contract uses Requested/Validated/Denied/Disputed/Cancelled/Expired.
• The spec says Teller turns stale requested into Rejected but contract marks stale as Expired, and expiry is enforced opportunistically in multiple handlers
• The spec suggests Teller can reject stale with "Constantly scans withdrawalRequests that are stale and batch turns them into Rejected", however the contract TELLER cannot deny requests -- only expire them with cancelStaleWithdrawals. The function rejectWithdrawals is reserved for the Relayer.
• The spec mentions user can cancel before validation but does not mention thatcontract also blocks cancel once status is Disputed.

Update the Deposit spec language to mimic the on-chain contract and reconcile any discrepancies.

The API handler passes the Symbol field directly to the Trading Service without validating it against supported collaterals. While the Trading Service correctly rejects non-USDT symbols before any state changes occur, the invalid request still traverses the full API -> gRPC -> Trading Service path unnecessarily.

Consider adding symbol validation at the API layer to fail fast.

The AutoApproveWatcher polls every 60 seconds and fetches all withdrawals in Requested status with getAllWithdrawalRequests(). For each withdrawal, it performs validation and submits a castWatcherVotes transaction. However, there is no tracking of which withdrawals this watcher has already voted on. The on-chain castWatcherVotes function silently skips already-cast votes without reverting, but the relayer undergoes unnecessary gas expenditure on no-op transactions.

Consider skipping already voted-on withdrawals.

All withdrawal status notifications are published through function publishWithdrawalEvent without a Nats-Msg-Id. In contrast, deposit events include this header for JetStream deduplication. If the listener replays a block or resends a WithdrawalStatusChanged log, downstream services can receive duplicate withdrawal status notifications.

Add a deterministic Nats-Msg-Id to withdrawal publishes.

File synthetix_deposit_contract.go has numerous stale bindings, including but not limited to: (1) GetAllWithdrawalRequests, GetWithdrawalRequestsByStatus, and GetWithdrawalRequestsByUser are still exposed but were moved to a Lens contract (2) The binding exposes an OracleStaleTimeout() getter yet the contract actually has maxOracleStaleTimeout() (3) The bindings omit slpVault(), setSLPVault, approveSLPVault, getSLPVaultAllowance, and their events SLPVaultSet/SLPVaultApproved and more.

Regenerate synthetix_deposit_contract.go with the latest bindings.

Currently non-usdt collateral is not supported in margin calculations. This is okay for initial deployment, but there should be changes in the off-chain code if a non-USDT collateral was ever enabled on-chain.

Ensure initial deployment only allows USDT collateral and modify the off-chain code to support multi-collateral if other tokens such as sUSDe and cbBTC end up being supported as collateral.

The API layer validates that the destination address is a valid hex format using common.IsHexAddress(), but this check accepts 0x0000000000000000000000000000000000000000 as valid. The downstream services (trading gRPC handler, relayer) perform no additional validation. When a user submits a withdrawal to address(0):

1. API validates with IsHexAddress("0x0000...000") -> passes
2. Request flows through to relayer
3. Relayer calls requestWithdrawal() on contract
4. Contract reverts with ZeroAddress() Currently the off-chain layer only supports one WithdrawalEntry per requestWithdrawal (only one user and one token to withdraw), but if multiples users in requestWithdrawal were ever supported, then an attacker can trivially DoS other user withdrawals by passing a zero address beneficiary in a withdrawal request.

Add explicit zero address rejection at the API layer.

handleWithdrawal allows/denies withdrawals using calculateWithdrawableAmount, which pulls cached position.IMR. Under normal conditions, mark price updates flow through BroadcastTickUpdate() -> actor.handleMarkPriceUpdate() -> marginCalc.UpdateAccountState(), keeping cached IMR values current. However, both withdrawal requests and mark price updates are delivered to the same actor inbox channel (capacity 2000). Under heavy system load when the inbox approaches capacity, mark price updates may be delayed or dropped while withdrawal requests are still processed. In this scenario, withdrawals are evaluated against stale, lower IMR values. Hence users can withdraw funds that should be locked as margin during periods of system load. Note that this backpressure can also lead to other areas to operate under stale values. calculateSymbolRMR() recomputes the worst-case IMR using the latest market.MarkPrice, but subtracts position.IMR. When prices move without a position update, position.IMR remains stale, so worstCaseIMR - currentPositionIMR mixes live and old data. Hence the difference understates or overstates the true RMR depending on how prices have moved. Contrast this with CalculateCumulativeIMR(), which uses the correct value of latest market prices

Recompute IMR/RMR from current mark prices to ensure liveness.

In calculateSimulatedRMR, any error during margin computation (e.g. missing/invalid market data or no applicable tier) is logged and the function returns decimal.Zero, so ValidatePreTradeSafety proceeds with zero RMR. A misconfigured market or transient lookup error effectively disables the pre‑trade margin guardrail instead of blocking the order. Consequently, ValidatePreTradeSafety can approve orders that would otherwise breach margin once the real RMR is computed. Note that this pattern also applies to other paths such as calculatePerSymbolWithdrawalRequirement since GetMarket failure skips symbol's margin and calculateSymbolRMR would also return zero on failure.

Consider propagating the error to reject the order.

In handleDeposit (account_events.go), if two deposit events for a new subaccount arrive concurrently, both GetActor calls can fail before the actor is stored, and both proceed to CreateActor with initial balances. Only one actor is stored and the other deposit isn’t sent to an existing actor and is effectively lost. Consequently, a user’s on-chain deposit can be missing from off-chain state when their first deposits race.

Serialize actor creation per subaccount.

The public API registers /test/deposit (with TestHandler) and HandleTestDeposit simply requires an auth_secret that is compared against SNXAPI_COMPETITION_TIME_DEPOSIT_SECRET The default docker-compose value for that variable is "*" so validateAuthSecret returns success for any submitted auth_secret. Once the handler accepts the request it publishes a RelayerEventDeposit to NATS, which downstream services treat as genuine deposits, letting any attacker credit arbitrary subaccounts/wallets without authentication. There is currently a TODO to remove this, hence this issue is marked as Low severity.

Remove or disable the /test/deposit endpoint in production.

CancelOrder iterates through all requested OrderIds and create a Go routine for each one. Unlike order placement, cancellation does not run through the rate limiter, allowing users to submit large batches of cancellations and spike resource consumption. A user does not necessarily have to submit valid Order ID's to cancel in the request in order to consume resources.

Consider introducing the ability to apply rate limiting on cancellations or enforcing a maximum batch size.

ValidatePreTradeSafety simulates post-fill account state to prevent trades that would immediately breach margin requirements. However, it prices orders optimistically:

• Market orders: Uses markPrice
• Limit orders: Uses limit price Large market orders or aggressive limit orders that walk the book can fill across many price levels at progressively worse prices. The simulation doesn't account for this price impact, hence AAV can be overstated.

Be aware of this risk and/or consider including some form of price impact estimate for pricing.

The subaccount service uses sharded worker pools to process NATS JetStream events for open orders, positions, and trade history. Each worker pool uses consistent hashing (subAccountID % numWorkers) to route all events for a given subaccount to the same worker, ensuring ordering guarantees. However, if a worker goroutine panics, it terminates permanently with no recovery mechanism: When a worker dies:

• Its channel remains open but unread
• New jobs queue up until the buffer fills
• Subsequent jobs are dropped after a 2ms timeout at submission
• Messages are ACKed to NATS even on handler failure, preventing redelivery Since consistent hashing routes all events for specific subaccounts to the same worker, a single worker crash causes all subaccounts where id % numWorkers == deadWorkerID to lose event persistence indefinitely until the entire service restarts. This creates a durability gap where the trading service (in-memory) and subaccount service (PostgreSQL) diverge. If the trading service subsequently restarts, it syncs from the incomplete database state with function SyncSubaccountsFromService(), potentially loading stale positions, orders, or collateral balances.

Add panic recovery within the worker loop to prevent goroutine termination.

When the auto-exchange manager sells collateral to cover a subaccount’s debt, it removes the sold amount twice. The manager immediately subtracts quantityToSell from the source collateral balance:

newQuantity := sourceCollateral.Quantity.Sub(quantityToSell)
if newQuantity.LessThan(decimal.Zero) {
	return fmt.Errorf("insufficient %s balance for auto-exchange", collateralSymbol)
}

then invokes TransferCollateralToSLP():

if m.slpTransfer != nil {
	err := m.slpTransfer.TransferCollateralToSLP(

which also subtracts the same amount after sending the transfer message (by design the transfer helper handles the deduction only after the send succeeds).

// Transfer helper assumes balances are untouched until after a successful send
if err := slpActor.Send(transfer); err != nil {
    ... return err
}
newSourceQuantity = sourceCollateral.Quantity.Sub(amount)
...
st.actor.subAccount.Collaterals[collateralSymbol] = types.Collateral{ ... Quantity: newSourceQuantity }

The two calls stack up and the user ends up losing 2x the sold collateral. No unit test covers this flow; updateCollateralBalances() is only called with slpTransfer == nil, so the duplicate subtraction went unnoticed.

Remove the upfront subtraction in updateCollateralBalances() and let TransferCollateralToSLP() perform subtraction for auto-exchange flows (like it already does for liquidation).

Several handlers parse user-supplied amount strings into arbitrary-precision decimals using decimal.NewFromString() without first validating the input length. This occurs in:

• lib/api/handlers/trade/withdraw_collateral.go at the API layer
• services/trading/internal/handlers/withdrawal_handler.go at the trading service layer While gRPC imposes a default 4MB message size limit, a moderately large numeric string (hundreds of thousands of digits) within that limit could still cause elevated CPU and memory usage during parsing and conversion to big.Int. An attacker could submit withdrawal requests with excessively long amount strings to cause minor resource consumption on the API and trading services.

Add a simple length check before parsing amount strings.

In the subaccount service's withdrawal handling logic, when the NATS client is unavailable, the code logs a warning but continues to return a success response to the caller rather than returning an error. At this point in the flow, the withdrawal has already been persisted to the database in CREATED status, and the trading actor has reserved the requested funds by moving them from available balance to PendingWithdraw. However, without the NATS publish, the withdrawal request never reaches the relayer for on-chain submission, hence the withdrawal remains stuck.

Return an error when NATS is unavailable.

handleSLPTakeover() skips transferring any position or collateral whose SLP actor lookup returns nil, yet it still calls clearLiquidatedAccount:

for symbol, position := range le.actor.subAccount.Positions {
    slpActor := le.actor.manager.GetSLPActorForMarket(symbol)
    if slpActor == nil {
        le.logger.Warn("No SLP actor found for market", ...)
        // Skip this position if no SLP mapping exists
        continue
    }
    ...
}
for collateralSymbol, collateral := range le.actor.subAccount.Collaterals {
    slpActor := le.actor.manager.GetSLPActorForCollateral(collateralSymbol)
    if slpActor == nil {
        le.logger.Warn("No SLP actor found for collateral", ...)
        // Skip this collateral if no SLP mapping exists
        continue
    }
    ...
}
...
le.clearLiquidatedAccount(collateralSymbolsToDelete)

func (le *LiquidationEngine) clearLiquidatedAccount(...) {
    for symbol, position := range le.actor.subAccount.Positions {
        le.publishPositionCloseEvent(symbol, position)
    }
    le.actor.subAccount.Positions = make(map[string]types.Position)
    ...
}

Which publishes close events and zeroes every asset. As a result, skipped positions/collaterals are deleted without being reassigned, producing permanent loss when SLP lookup fails. However the scenario is unlikely in production because SLP actors are pre-seeded and cached, so severity reduced to Medium despite high impact.

Abort or retry the takeover when slpActor == nil, or track which assets actually transferred and only clear those.

clearLiquidatedAccount() receives collateralSymbolsToDelete param but never uses it, so the collateral map is never deleted after liquidation. Each symbol stays in le.actor.subAccount.Collaterals with Quantity = 0, causing future SLP-takeover runs to keep looping those stale entries and logging warnings. Impact is low (bounded CPU/log noise, no financial risk), but the function does not perform what it is intended to do as it should clear the collaterals as the natspec suggests.

// clearLiquidatedAccount clears all positions, collaterals, and balances from the liquidated account
func (le *LiquidationEngine) clearLiquidatedAccount(collateralSymbolsToDelete []string)

Inside clearLiquidatedAccount(), iterate the collateralSymbolsToDelete slice (or simply reinitialize le.actor.subAccount.Collaterals = make(map[string]types.Collateral)) so collateral keys are actually removed once liquidation completes.

handlePositionTPSLEvent() retries only when the DB error string contains “position not found.” Any other failure (deadlock, constraint violation, serialization failure, etc.) is treated as non-retryable: the message is ACK’d immediately and never reprocessed, so TP/SL updates are silently lost.

if !strings.Contains(err.Error(), "position not found") {
    s.logger.Warn("Failed to update position TP/SL with non-retryable error", ...)
    _ = msg.Ack()
    return
}

A transient DB hiccup or unexpected validation error discards a user’s stop-loss/take-profit configuration, leaving positions unmanaged without alerting.

Introduce proper error classification, retry/Nak transient conditions, and only Ack when the payload is definitively invalid. Emit alerts for permanent failures.

InitDB() treats failures while preparing critical collateral columns (ltv, lltv) as warnings and then keeps going. for example:

if err := db.Exec("ALTER TABLE collaterals ADD COLUMN ltv DECIMAL(5,2) DEFAULT 0.8").Error; err != nil {
	logger.Warn("Failed to add ltv column, may already exist", "error", err)
}

AutoMigrate() for the collateral tables runs immediately afterward and expects those constraints to be in place, so the service can run against an inconsistent database.

Treat these preparatory migrations as fatal: return the error (or wrap it) instead of logging and continuing

The watcher rebroadcasts any stuck tx every 15s once it ages past stuckTimeout, each time bumping the gas price. There’s no cap on attempts or gas ceiling. If a tx is repeatedly stuck, the loop will continue rebroadcasting indefinitely, potentially pushing replacement gas prices higher than intended and consuming operator funds unnecessarily.

Consider a max gas price, a cap on rebroadcast attempts, or some larger back-off period.

publishCancelOrder() and publishModifyOrder() skip publishing if a.recoveryMode is true (returns nil, simulating success). But publishOrder() has no such check

	// Skip publishing in recovery mode
	if a.recoveryMode {
		return nil
	}

Therefore during recovery mode new orders could error while cancels/modifies returns nil, causing inconsistent behaviour.

Add recovery mode validation in publishOrder() like the other functions

Liquidations halt when the cached mark price is zero, causing the liquidation flow to error and retry indefinitely. calculateBankruptcyPrice() returns decimal.Zero, and the caller aborts with “invalid bankruptcy price,” so ProcessLiquidationState() retries forever without placing orders.

markPrice := market.MarkPrice
if markPrice.IsZero() {
    le.logger.Error("No mark price available for bankruptcy calculation",
        "subAccountID", le.actor.subAccount.ID,
        "symbol", symbol,
    )
    return decimal.Zero
}

bankruptcyPrice := le.calculateBankruptcyPrice(*position, market, symbol)
if !bankruptcyPrice.IsPositive() {
    le.logger.Warn("Invalid bankruptcy price calculated", ...)
    return decimal.Zero, fmt.Errorf("invalid bankruptcy price: %s", bankruptcyPrice.String())
}

Clearly document this risk and monitor/alert for lacking mark prices.

In calculateReduceOnlyAdjustments() there is a loop to prevent race condition for multiple types.OperationCreateOrder that may exceed position.Quantity which is a cap to ensure that reductions do not exceed position size and end up flipping the trade. This does not take into account pendindtypes.OperationCancelOrder, hence can cause unnecessary adjustments. For example:

1. User has 10 ETH long position
2. User places reduce-only sell 8 ETH (Order A) → added to OpenOrders
3. User cancels Order A → stays in OpenOrders with State = OrderStateCancel
4. User places reduce-only sell 10 ETH (Order B)
5. calculateReduceOnlyAdjustments() sees: 8 (Order A, pending cancel) + 10 (Order B) = 18 > 10
6. System adjusts Order A (which is already being cancelled)

Document that cancellations are not considered when adjusting reduce-only, creating a more conservative behavior.

The safeAddOneIfGreater function is unused throughout the off-chain code. Consider removing the unused code.

Consider removing the unused code.

The WithdrawCollateralResponse struct has timestamp fields that are never populated in Handle_withdrawCollateral.

	response := WithdrawCollateralResponse{
		RequestID:   ctx.RequestId, // Use API request ID for end-to-end correlation
		Symbol:      Symbol(grpcResp.Symbol),
		Amount:      grpcResp.Amount,
		Destination: grpcResp.Destination,
		Status:      grpcResp.Status.String(), // Use actual status from database ("created", "requestedOnChain", "completed", "failed")
	}

Either populate timestamp fields or remove them from response struct.

Unlike SubmitWithdrawalRequest, functions CastWatcherVote and SubmitBatchDisbursal return immediately after SendTransaction without waiting for confirmation. This inconsistency between the functions should be clearly documented, since if there is an issue with the tx the caller would proceed as if it was successful.

Clearly document this behavior.

The off-chain signature verification only supports EOA signatures. isValidSignature(bytes32 hash, bytes signature) is not called anywhere hence smart contract signatures documented in ERC-1271 are not supported.

Clearly document this restriction.

The actor does not check that withdrawal.Quantity is positive.

// Deduct from quantity and move to pending withdrawal
	collateral, exist := a.subAccount.Collaterals[withdrawal.Collateral]
	...
	collateral.Quantity = collateral.Quantity.Sub(withdrawal.Quantity)
	collateral.PendingWithdraw = collateral.PendingWithdraw.Add(withdrawal.Quantity)

A negative value will mint balance. The public REST handler enforces amount > 0, but the gRPC endpoint (HandleWithdrawCollateral()) does not. Attack requires a compromised service that can call gRPC directly, this is unlikely however a sanity check is still recommended.

Validate withdrawal.Quantity is greater than zero.

For the InitialMargin value, the GetSubaccount() function returns only position IMR, but ListSubaccounts() return IMR + RMR, so clients see two different “initial margin” totals for the same subaccount depending on which function they call.

/subaccount/internal/handlers/grpc_handler.go
// GetSubaccount
			InitialMargin: subAccount.CumulativeIMR.String(),
...
// ListSubaccounts
				InitialMargin: subAccount.CumulativeIMR.Add(subAccount.CumulativeRMR).String(),

Have GetSubaccount() report CumulativeIMR + CumulativeRMR so every API reflects the total margin locked by positions plus open orders, consistent with trading-engine calculations.

The gRPC handler GetPositionsBySymbol() allocates LongPositions and ShortPositions slices but never fills them. The loops are empty:

	respLongPositions := make([]*grpc.PositionItem, len(longPositions))
	respShortPositions := make([]*grpc.PositionItem, len(shortPositions))

	for i := 0; i != len(longPositions); i++ {

	}

	for i := 0; i != len(shortPositions); i++ {

	}

So the returned response every entry is nil. Dashboards and other consumers relying on this receive empty data.

Replace the empty loops with a call to the existing convertPositionItems helper, mirroring GetPositions:

respLongPositions := h.convertPositionItems(longPositions)
respShortPositions := h.convertPositionItems(shortPositions)

GetFundingPayments() drops any transaction whose metadata fails to unmarshal.

		if err := json.Unmarshal(tx.Metadata, &metadata); err != nil {
			s.logger.Warn("Failed to parse funding payment metadata", "error", err, "transaction_id", tx.ID)
			continue
		}

Those entries don't reach the aggregation loop, so totalFundingReceived, totalFundingPaid, and the returned FundingHistory silently omit them. Users may see incomplete summaries with no indication that data was skipped.

Either fail the request when metadata parsing fails (so clients can retry/correct) or include the transaction while flagging the row/response as incomplete. At minimum, surface a warning in the API response indicating partial aggregation so consumers know totals may be understated.

Deposits and withdrawals update USDT collateral with six-decimal precision (updateCollateral()), but realized PnL settlements and funding-rate adjustments are rounded to types.MarginPrecision (2 dp) before they ever hit the collateral ledger. Any gain/loss smaller than a cent is silently discarded, so cumulative rounding always favors the protocol and makes on-chain/off-chain accounting diverge.

While rounding in favor of the protocol is best practice for DeFi, it is usually only on the wei factor, not 2 decimal factor. Consider aligning all balance changes on a single precision (6) and document the rounding policy. This avoids the perception of hidden protocol spread.

In mapGRPCToSubAccount() if decimal.NewFromString() returns err, it simply logs it and skips adding the collateral entry. There is a TODO statement:

pendingWithdraw, err := decimal.NewFromString(col.PendingWithdrawalAmount)
		if err != nil {
			m.logger.Error("Failed to parse pending withdrawal amount", "error", err, "collateral", col.Collateral)
			continue // TODO: should we return an error here?
		}

This can cause collateral entry to be skipped resulting in shrinking the available margin.

This is informational as there is already a TODO statement, but it is recommended to return err instead of continue

The handleCancelAllOpenOrdersBySymbol function uses a 30-second timeout that abandons result collection early, causing successful order cancellations completed after the timeout to be lost and incorrectly marked as "failed" in the response. This provides incomplete and inaccurate information about order states.

Consider using sync.WaitGroup to reliably return all states.

In handleOrder (after a.marginCalc.ReserveMarginForOrder call) If publishOrder fails (e.g., NATS error), the function returns an error response but does not unreserve/rollback the margin previously reserved via a.marginCalc.ReserveMarginForOrder. However impact is low because these stale values don't affect actual margin calculations since failed orders are never added to pendingOps or OpenOrders. This is more a state consistency issue that may confuse debugging but doesn't break any system behavior. Contrast to modifyOpenOrder, which does rollback on publish failure.

err = a.publishOrder(ctx, request.RequestID, request.Order)
if err != nil {
    // Rollback margin reservation since order was not successfully submitted
    request.Order.RMR = decimal.Zero
    request.Order.MarginReserved = decimal.Zero
    request.Order.IMR = decimal.Zero
    
    respond.Error(request.Order.ID, request.RequestID, err)
    return
}

There is a block of commented out legacy code in margin_calculator.handleIndexTick()

// // Recalculate values with new price

Remove commented out code before production to reduce confusion.

publishIndexPriceUpdate(ctx context.Context, symbol string, collateral types.Collateral), the parameters symbol and collateral are passed but never used. The function loops over all collaterals in a.subAccount.Collaterals instead. publishPnlSettledUpdate(ctx context.Context, collateralName string, newQuantity, pendingWithdraw decimal.Decimal), the parameters newQuantity and pendingWithdraw are unused. The function pulls values from a.subAccount.Collaterals instead.

Clean up unused params

TransferCollateralToSLP() hard-codes HaircutRate: decimal.NewFromInt(1) so the SLP side always computes post_haircut_value = 0, even when the collateral should retain value. Balances aren’t impacted but logs sees a 100% haircut for every transfer.

transfer := &SLPCollateralTransfer{
    ID:               fmt.Sprintf(collateralTransferIDFormat, ...),
    SourceAccountID:  st.actor.subAccount.ID,
    CollateralSymbol: collateralSymbol,
    Quantity:         amount,
    PricePerUnit:     pricePerUnit,
    HaircutRate:      decimal.NewFromInt(1),
    ...
}

Compute the actual haircut via GetCollateralHaircutRate()

sendOpenOrderEvent() sets both CreatedAt and UpdatedAt to snx_lib_utils_time.Now() for every action (create, modify, cancel, fill, TP/SL insertion, leverage adjustment, etc.).

UpdatedAt:         snx_lib_utils_time.Now(),
CreatedAt:         snx_lib_utils_time.Now(),

Since the subaccount service reuses this helper for all open-order state changes, the true creation time is overwritten when order is modified.

Only write the UpdatedAt for edits to preserve the original CreatedAt timestamp.

The withdraw flow contains error-handling branches that can never be reached, because marginCalc.UpdateAccountState() does not return errors. As a result, withdrawals that fail on-chain are always refunded correctly right now, but the dead code masks a latent risk: if future refactors make UpdateAccountState() start surfacing errors, previously completed withdrawals could become permanently stuck in WITHDRAWAL_STATUS_AWAITING_ACTOR_UPDATE The scenario is described below: If there is an onchain withdraw failure, and then the actor rollback also fails. Funds will be permanently stuck in WITHDRAWAL_STATUS_AWAITING_ACTOR_UPDATE

1. On-chain failure path: When a withdrawal fails on-chain (or the cleanup job detects it expired), handleWithdrawCompletedEvent() marks the row WITHDRAWAL_STATUS_AWAITING_ACTOR_UPDATE, records a failure_reason, and sends a WithdrawProcessedEvent() with Error to trading:

services/subaccount/internal/services/subaccount/event_handlers.go
updates := map[string]any{
	"status":     snx_grpc.WithdrawalStatus_WITHDRAWAL_STATUS_AWAITING_ACTOR_UPDATE,
	"request_id": requestID,
}
if !isSuccess {
	updates["failure_reason"] = errorMsg
}

1. Actor rollback failure: The trading actor should restore PendingWithdraw to available balance for failed withdrawals, but if that restoration path also errors (e.g., marginCalc.UpdateAccountState() fails), _publishWithdrawActorUpdated() is called with a non-empty Error:
2. Dead-end state: handleWithdrawActorUpdatedEvent() sees both the original failure_reason and the actor error, and intentionally leaves the record in WITHDRAWAL_STATUS_AWAITING_ACTOR_UPDATE expecting a retry that does not exist anywhere else in the codebase: As a result, both the withdrawal and the user’s collateral remain permanently stuck; no job or handler reprocesses WITHDRAWAL_STATUS_AWAITING_ACTOR_UPDATE, so funds never return to the user. In the current implementation since marginCalc.UpdateAccountState() does not return errors, it is just an issue with superfluous since the error handling is unreachable. However there is risk that future refactor will surface this risk.

Document the dependency: withdrawals rely on UpdateAccountState() never returning an error. Remove the unreachable branches or on refactor add a reconciliation/cleanup job that can re-drive WITHDRAWAL_STATUS_AWAITING_ACTOR_UPDATE rows if actor rollbacks ever start failing in future

The calculateWithdrawableAmount function includes unrealized PnL when computing how much a user can withdraw, via CalculateBalance(). This is a deliberate design choice that allows traders to withdraw "paper profits" before positions are closed. As documented in dYdX's post-mortem of the SUSHI/YFI incident, allowing withdrawal of unrealized PnL creates potential for:

• Socialized losses: If a trader withdraws unrealized gains and the position later moves against them into insolvency, the protocol's insurance fund (or other users) absorbs the loss.
• Market manipulation incentive: Attackers can pump mark price, withdraw "profits," then let the position liquidate - extracting value that was never truly realized.
• Liquidity drain during volatility: In extreme market conditions, many users withdrawing unrealized gains simultaneously can stress protocol solvency.

Clearly document this design decision and risks. Carefully monitor margin requirements.

When cfg.Auth.Enabled = false, the AccountAuthenticator is never instantiated, causing h.authenticator.Enabled() to return false. In this state, the authenticateRequest function bypasses all signature verification and ownership checks:

if !h.authenticator.Enabled() {
    walletAddress := req.WalletAddress()
    if walletAddress == "" {
        walletAddress = "0x0000000000000000000000000000000000000001"
    }
    return walletAddress, subAccountId, nil
}

When authentication is disabled, no EIP-712 signature verification occurs and any user can impersonate any wallet and act on any subaccount.

Ensure production deployments always have Auth.Enabled = true

The off-chain repo has no automated E2E tests that exercise the full stack (API → trading/subaccount → relayer → Anvil-deployed contracts → event consumption) for on-chain flows. Relayer benchmarks simulate chain events and deposits have sample scripts, but there’s no round trip that starts from the public API with real signatures, drives a contract call on a local chain, and asserts off-chain state updates. Withdrawals are notably untested end-to-end: there’s no flow that submits withdrawCollateral, runs the relayer against SynthetixDepositContract on Anvil, observes WithdrawalRequested, and verifies DB/NATS/WebSocket status transitions.

Add CI-friendly integration tests that spin up Anvil, deploy SynthetixDepositContract, start API/trading/subaccount/relayer, and run both deposit and withdrawal scenarios end-to-end. Use real signed requests, wait for the relayer to submit on-chain transactions, consume emitted events, and assert off-chain persistence and status updates. Add documentation in the README on how to run these tests locally.