Deposit Contract

The Deposit contract never posts on-chain PnL between counterparties. A winning trader's withdrawal can exceed their on-chain user balance and is still disbursed as long as the contract holds enough tokens, while no corresponding debit is applied to the losing trader. This is not zero-sum and allows profits to be realized without charging the other side of the trade, eventually leading to contract insolvency without appropriate risk engine/off-chain withdrawal validations.

Furthermore, the losing trader's user balance is closer to the config.userMaximum, so if they would like to deposit more collateral to save their position from being liquidated, they may be prevented from doing so, although their true, off-chain balance is less than the user balance the on-chain mapping contains due to negative PnL.

Have the relayer post an order settlement to the contract, such that the user balance of both the maker and taker are adjusted by the PnL.

Mapping tokenWithdrawalsPaused is intended to prevent withdrawals for specific tokens. Function requestWithdrawal checks tokenWithdrawalsPaused[token] but function disburseWithdrawal does not. Consequently assets that are intended to be paused may still leave the Deposit contract.

Consider validation again tokenWithdrawalsPaused in function disburseWithdrawal.

The contract implements an expiry mechanism for withdrawal requests using function _isWithdrawalExpired(), which returns true if a request is in Requested or Validated states and its withdrawal timeout has elapsed. However, disburseWithdrawal() does not check expiry. A teller can disburse a withdrawal that has been in the Validated state past the withdrawalExpiryTimeout. As a result, withdrawals that are technically expired can still be executed and paid out.

Consider if expired withdrawals should be disbursable. If not, attempt to cancel the request when disbursing.

When a withdrawal is requested/validated, the contract does not “lock” or put aside those funds. The user’s on-chain balance isn’t reserved since only at disbursal is userBalance decreased. Meanwhile, the off-chain component can still potentially treat the same balance as available margin which enables double-use of the same collateral (pending withdrawal and trading margin). This may lead to some scenarios where if a trader's position quickly turns into a loss, the Teller may disburse more funds than should be allocated to the user based on their latest position.

Consider setting aside funds that have been requested for withdraw from being used as margin.

According to technical documentation, "A user can call cancelWithdrawal(uint256 _id) to cancel their own request, but only if it is still in the Requested, Approved, or Disputed states."

Function cancelWithdrawal explicitly validates against Validated and Disputed withdrawal requests and reverts with InvalidStateForAction if so. Consequently, user cancellation will be prevented in these states, going against technical spec.

Either update the technical spec to reflect the latest contract behavior, or allow for cancellation in these states. Note that race conditions with off-chain component will have to be appropriately handled if cancellations are supported when the Teller can already disburse funds (in Validated state) for example.

The Deposit contract lacks a fee on deposits. Consequently, a malicious user may take the entire deposit capacity (the globalMaximum) and prevent other users from depositing collateral. Depending on the size of the userMaximum, the attacker can simply split up deposits among multiple addresses.

Consider adding a configurable deposit fee.

The documentation stats that PnL is settled in USDT, so the contract must keep healthy USDT liquidity. However, the Deposit contract’s accounting treats totalDeposited[token] as a uint that only increases on user deposits and decreases on disbursement. Trading PnL or CoW conversions that add USDT to the vault do not increase totalDeposited[USDT].

For non-USDT collateral depositors, their PnL is denominated in USDT, but no matching USDT was ever credited to totalDeposited[USDT]. When such a user requests a USDT withdrawal, the system subtracts from totalDeposited[USDT] even though that counter was never incremented. This can drive totalDeposited[USDT] below zero, trigger an underflow revert and blocking withdrawals.

Calculate the user’s USDT PnL off-chain, price that amount into the collateral token deposited by the user (accounting for any valuation haircut as necessary), and relay the withdrawal for that amount. If USDT disbursal is needed, support a withdrawal for that amount.

Function requestWithdrawal supports multiple withdrawal entries and validates that each entry does not exceed the contract balance.

If there are multiple entries with the same token, the amount that is request for that token can exceed the actual contract balance.

For example:

(1) Contract balance = 10 WETH (2) Two withdrawal entries of 10 WETH (3) Each entry passes the validation but contract does not have enough balance to support the total withdrawal of 20 WETH.

Furthermore, the validation does not take into account tokens that have already been requested for withdrawal for a different beneficiary. This may lead to potential issues with on-chain solvency.

Consider adding more validation on-chain to account for requested tokens from other user withdrawal requests and/or add more off-chain validation such that each token only has one entry.

There are multiple functions for >1 actor to dispute withdrawals:

(1) batchDisputeWithdrawals by RELAYER_ROLE (2) watcherDisputeWithdrawal by WATCHER_ROLE (3) batchWatcherDisputeWithdrawals by WATCHER_ROLE (4) disputeWithdrawal callable by RELAYER_ROLE and WATCHER_ROLE

All of this functionality can be combined into a single function.

Consider consolidating disputes into a single function such as disputeWithdrawal callable by both roles.

According to documentation, _userBalance represents "[t]he user's deposited amount (can be negative if losses exceed deposits)". However, according to the tests within SuccessfulTraderWithdrawalTest, trading profit is what leads to the user balance being negative upon withdrawal disbursement. Furthermore, if a user withdraws profit so that their user balance is -X and then deposits X more tokens, their user balance appears as 0 which may cause issues with off-chain infrastructure and may bypass the config.userMaximum validation.

Adjust the documentation to reflect trader profit leading to negative balance, or adjust the implementation so profit increases user balance.

There is currently no subaccount validation on the Deposit contract. For example, a user may end up depositing to a subAccountId that does not exist for their address. An AssetDeposited event will be emitted with the non-existent subaccount, and it is crucial that the off-chain component appropriately handles this case.

Ensure the off-chain component handles currently non-existent subaccounts.

A Teller will attempt to disburse funds through function disburseWithdrawal after withdrawal validation. However, if the token being transferred has blacklist functionality such as USDT and the user has been blacklisted, this will prevent the disbursal until the withdrawal turns stale and is rejected. In the meantime while the withdrawal is not yet expired, it is important that the Teller is appropriately configured so it is not prevented from disbursing other withdrawal requests.

Ensure the Teller is capable of moving on to other withdrawal requests if a particular request is reverting. Furthermore, validate that the amount being transferred is greater than 0 within both disburseWithdrawal and batchDisburseWithdrawals.

In order for a request to become validated, the watcherQuorum number of votes must be reached and this parameter is configured within function setWatcherQuorum. The watcherQuorum can be set to a value larger than the number of watches and hence the Requested -> Validated state transition will not be possible.

Ensure the quorum amount is non-zero but no greater than the number of watchers.

Function _isWithdrawalExpired() does not treat Disputed requests as expirable, and users cannot cancel from Disputed. Only a Guardian (or owner override) can resolve them. If Guardians (multi-sigs) are inactive, the request is stuck, and since each user may only have one active withdrawal at a time, the affected user is blocked from creating new withdrawals.

Clearly document this risk and ensure availability of Guardians.

The contract enforces a minimum deposit amount with CollateralConfig.userMinimum, but no equivalent check exists for withdrawals. A relayer can therefore create arbitrarily small withdrawal requests based on off-chain user inputs, which can lead to spam and unnecessary gas wastage.

Consider adding the ability to set a minimum withdrawal amount.

The overrideRequestStatus function is intended to be used as an "[e]mergency function", but is gated by the whenNotPaused modifier. Consequently, if the contract is paused during an emergency, the Owner will be unable to override the status.

Consider removing the whenNotPaused modifier from function overrideRequestStatus.

The contract validates userMaximum and globalMaximum independently when adding or updating collateral configs. However, it does not enforce that userMaximum <= globalMaximum. This allows a misconfigured token where userMaximum exceeds the global cap, preventing user deposits.

Consider validating that userMaximum <= globalMaximum when setting configs.

The PERMIT2 contract has the following PermitTransferFrom:

struct PermitTransferFrom {
        TokenPermissions permitted;
        // a unique value for every token owner's signature to prevent signature replays
        uint256 nonce;
        // deadline on the permit signature
        uint256 deadline;
    }

which differs from SNX’s struct:

struct PermitTransferFrom {
        TokenPermissions permitted;
        address spender;
        uint256 nonce;
        uint256 deadline;
    }

Hence the spender is not necessary and will trigger failure in PERMIT2 functionality when a signature is provided to permitTransferFrom.

Update the PermitTransferFrom struct to the PERMIT2 version.

Function batchDisputeWithdrawals does not finalize expired withdrawal requests with Status.Expires, which is asymmetrical with batchRejectWithdrawals which does first check if a request is expired and setting its status as expired if so.

Add a check within batchDisputeWithdrawals to check whether the withdrawal is expired and set it as expired if so.

Watcher votes are not cleared when a withdrawal request is disputed. If a request in Requested accumulates watcher votes and is then moved to Disputed, those prior votes remain stored. Should the request later return to Requested (through overrideRequestStatus for example), the old votes still apply, allowing the request to be quickly re-validated without a fresh round of voting. This potentially undermines the intended integrity of the dispute process.

Consider resetting watcher votes and watcherCount if the request moves away from a Requested state.

A withdrawal request can reach the Validated state without any watcher votes. Specifically, if a request in Status.Requested is disputed, a Guardian can call resolveDisputedWithdrawal(_approve = true), which directly transitions the request to Status.Validated. This bypasses the watcher quorum mechanism and allows withdrawals to be validated solely through Guardian approval, which may be unexpected behavior since spec states "[a] configurable quorum (M-of-N) of Watchers must approve a request before it can be processed."

Clearly document this behavior if intended under the GUARDIAN_ROLE section, clarifying that resolved requests can be processed for disbursal. Otherwise, transition the state to Status.Requested and let the watchers then cast votes.

Currently there is no validation on the CoW order within function isValidSignature beyond validation that the signer is an authorized trader, leading to serious trust assumptions for authorized traders.

Orders can be submitted that have an unexpected output token, where the receiver is not the Deposit contract nor an authorized receiver leading to drainage, etc. The documentation for SLP Collateral Exchange has example code including validations:

Consider adding more validations within function isValidSignature. Otherwise, clearly document trust assumptions.

User balance and totalDeposited is incremented by the item.amount, but this may differ from the actual amount received by the Deposit contract if a fee-on-transfer token was used as collateral.

Carefully select which tokens can be used as collateral, avoiding fee-on-transfer tokens, ERC777, etc.

Function deposit uses modifier whenNotPaused which reverts if (depositsGloballyPaused && withdrawalsGloballyPaused)

Within the deposit function body, depositsGloballyPaused is specifically validated with if (depositsGloballyPaused) revert DepositsGloballyPaused();

Consequently, the whenNotPaused modifier is not necessary since it would only be triggered if depositsGloballyPaused = true anyway. The same modifier is also unnecessary on function requestWithdrawal since if (withdrawalsGloballyPaused) is validated within the function body.

Consider removing whenNotPaused from functions deposit and requestWithdraw.

The boolean expression for finalized states - req.status == Status.Disbursed || req.status == Status.Denied || req.status == Status.Cancelled || req.status == Status.Expired - is repeated throughout the contract. Consider consolidating the validation in a single function to prevent inconsistencies if future updates/upgrades occur.

Consider consolidating finalized states into one function.

The existing Deposit contract technical doc has inconsistencies with the latest code, including but not limited to:

(1) Function approveWithdrawal does not exist although in doc it is stated that "the Relayer calls this function to transition the request's status from Requested to Approved."

(2) validateWithdrawal function stated in technical doc does not exist and watchers cast votes with castWatcherVote instead.

(3) The spec mentions event WithdrawalTimeoutSet but the Deposit contract uses event WithdrawalExpiryTimeoutSet

Update the spec to match the latest Deposit contract.

Function cancelStaleWithdrawals finalizes the request with reason code ReasonCodes.REASON_NONE, while other cancellations on expired withdrawal requests through the Deposit contract use REASON_EXPIRED.

Consider if cancelStaleWithdrawals should set the reason code as REASON_EXPIRED.

Function updateCollateralConfig rejects withdrawalMinimum > userMaximum, but addCollateral rejects withdrawalMinimum > userMinimum, so after updating the collateral’s config the withdrawal floor can be raised above the deposit floor and users may be trapped.

Use the same if (_config.withdrawalMinimum > _config.userMinimum) revert InvalidInput(); validation in function updateCollateralConfig.

Function disburseWithdrawal implies a payout/disbursal upon successful execution, but execution can exit without disbursing anything if the underlying request is cancelled, which is not communicated in the NatSpec within ISynthetixDepositContract.

Expand the NatSpec on disburseWithdrawal to document that that no disbursal is expected when the request has been cancelled and that disburseWithdrawal can finalize the status of the request.

The deployment and post-deployment scripts schedule/execute addCollateral with only the token address, but the Deposit contract requires a CollateralConfig argument. This causes all scheduled collateral additions to revert at execution time.

Update addCollateralData in DeploySynthetixDepositContract and addData in PostDeployment to supply a CollateralConfig rather than a token address.

According to spec, several situations will cause the system to have to sell non-USDT collaterals back into USDT, such as full trader liquidations. When accounting for use cases such as this, it is important to note that CowSwap is not atomic and may provide partial fills.

Ensure the rest of the collateral exchange system is capable of supporting delayed/partial swaps.

A user can submit a valid withdrawal request off-chain, which will then be relayed on-chain to request the withdrawal and later disburse it. If prior to disbursal the withdrawals were paused for a particular token, the entire batch would be DoS’d.

Ensure off-chain simulation architecture is resilient enough to adjust _requestIds accordingly.

MockToken.mint uses the _decimals parameter to scale the initial supply, but decimals() is overridden to always return 6. WETH is instantiated with 18 as _decimals and a WETH-oriented config in 1e18, but decimals() still returns 6. This inconsistency will skew deposit/limit checks and can lead to confusing or incorrect test behavior.

Align decimals() with the intended token decimals.

The comment for the MIN_ENVELOPE_LENGTH is incorrect: /// 12 head words (12*32) + bytes length (32) + padded 65-byte sig (96) = 384

The calculation above (12 * 32 + 32 + 96) does not equal 384.

Update the comment to accurately reflect the MIN_ENVELOPE_LENGTH calculation.

Request ID’s can be reused in multiple privileged functions when they have already been finalized. For example, the Watcher can call castWatcherVotes on an already expired/validated request id as a no-op that will waste gas.

Ensure the off-chain infrastructure is not reusing finalized id’s in the typical withdrawal lifecycle.

Multiple parameters in the Deposit contract act globally for all tokens included slippageToleranceBps, oracleStaleTimeout, etc. It may be prudent to have more granular configurations per-token instead.

Consider having these parameters be part of a collateral’s config.

Considering that swaps make the totalDeposited and userBalance for a token out-of-line and are one of the ways funds leave the Deposit contract, it may be prudent to have a specific swap toggle without relying on disable an entire sellToken's config.

Consider having a pause variable specifically used in isValidSignature to prevent swaps.

The error InvalidTradeParams() is raised for multiple reasons including but not limited to: slippage exceeded through swap, order buy/sell amount being 0, incorrect order sellTokenBalance, etc. It may be better to have specific reverts for specific issues to make for easier debugging.

Consider making reverts more specific.