Update Reviews

The vault tracks rewards using a global accumulator (rewardPerShare) and a per-account checkpoint (_rewardDebt) that represents how much of the global rewards an account has already entitled. Deposits and withdrawals update this checkpoint appropriately and claiming rewards increase it by the amount paid. However, when vault shares are transferred between accounts, the contract does not adjust either party’s _rewardDebt. Consequently, shares can be moved to an address with zero (or lower) debt, that address inherits the full entitlement and can immediately claim rewards that were already claimed by the previous holder, effectively draining the whole rewards pool.

Update _rewardDebt during share transfers (_update) by proportionally moving debt from sender to recipient.

Disabled-collateral flags within getSupportedCollaterals are ignored, so every asset listed in the mainnet defaults goes live immediately.

The configuration loop always calls addCollateral and approveCowVaultRelayer for every entry, and the implementation hard-codes enabled: true regardless of the provided struct. Although SNX will believe those tokens are disabled, they are actually depositable and have CoW approvals from the start.

Skip the addCollateral+approval steps whenever enabled is false.

In SLPVault, the addRewards function pulls USDT via safeTransferFrom, which requires the USDT holder (depositContract) to approve the vault as a spender. The SynthetixDepositContract does not expose any function to set or update USDT allowances and only the token holder can grant allowance. With no on-chain path to increase the approval, the allowance remains zero and addRewards consistently reverts, effectively disabling reward distribution.

In SynthetixDepositContract expose an owner-only function allowing to set an approval for SLPVault.

The deployment script fails mid-execution on Sepolia (and any network with unset environment variables) due to placeholder address(0) collateral entries. The Sepolia configuration defaults SEPOLIA_USDT and SEPOLIA_WETH to address(0) when environment variables are unset, while setting non-zero allowances (type(uint256).max). During configuration, the loop correctly guards addCollateral calls but executes approveCowVaultRelayer(address(0), allowance) unconditionally when allowance > 0. This reverts with TokenNotSupported() because address(0) was never added as collateral.

Since each function call is a separate broadcast transaction, earlier operations (role grants, collateral additions) remain committed on-chain while the deployment aborts, leaving the contract half-configured and requiring manual cleanup or redeployment. The same issue exists for setPriceFeed and setOracleStaleTimeoutForToken calls which also lack collateralToken != address(0) guards. Note that the default address(0) also applies in PostDeployment.s.sol.

Validate that collateralToken != address(0) during deployment.

The deployment script silently defaults all operational roles to the owner address when environment variables are unset. All relayer, watcher, teller, and guardian slots fall back to the owner value, which in production is intended to be a timelock contract that cannot perform fast-path operational functions like requestWithdrawal, castWatcherVotes, disburseWithdrawals, or emergency pauses. The default watcher configuration assigns the same owner address to all three watcher slots.

Since grantRole is idempotent, granting WATCHER_ROLE to the same address three times results in only one unique role member. When the script subsequently attempts to set WATCHER_QUORUM = 2, the setWatcherQuorum function validates that the quorum does not exceed getRoleMemberCount(WATCHER_ROLE) and reverts with InvalidInput as 2 > 1.

This revert occurs after multiple configuration transactions have already been mined, including role grants, collateral additions, and price feed configurations, leaving the contract in a partially deployed and unusable state requiring manual recovery.

Require explicit, non-zero, unique addresses for every operational role and validate that the watcher quorum does not exceed the number of unique watcher addresses before broadcasting contract calls.

The deployment script never grants MANAGER_ROLE to any address. After the deployer transfers OWNER_ROLE to the configured owner (intended to be a timelock) and renounces their own OWNER_ROLE, no “fast” account retains the ability to grant or revoke operational roles (RELAYER_ROLE, WATCHER_ROLE, TELLER_ROLE, GUARDIAN_ROLE, AUTHORIZED_TRADER_ROLE).

During initialization, the contract sets MANAGER_ROLE as the role admin for all five operational roles. This design allows quick operational changes without timelock delays. However, since the deployment script only grants OWNER_ROLE to the final owner and never grants MANAGER_ROLE to anyone, all operational role management becomes locked to the timelock's execution process.

Grant MANAGER_ROLE to an appropriate address before transferring OWNER_ROLE

The deployment script performs "full setup" but never grants AUTHORIZED_TRADER_ROLE to any address, leaving the CoW Protocol integration unusable after deployment. The script configures CoW-related parameters including slippage tolerance, price feeds, and vault relayer approvals, but omits granting the role required to actually execute trades.

The ERC-1271 signature validation in isValidSignature recovers the EOA signer from the embedded signature and validates that the signer holds AUTHORIZED_TRADER_ROLE. Without this role granted to any address, all CoW swap orders will be rejected.

Add AUTHORIZED_TRADER_ROLE configuration to the deployment script.

Function executeRoleGrants is hard-coded to grant only RELAYER/WATCHER/TELLER/GUARDIAN. No path exists to grant MANAGER_ROLE (admin for every operational role) or AUTHORIZED_TRADER_ROLE (required by isValidSignature). Once OWNER_ROLE lives in the timelock, role rotations and CoW trades are delayed.

Extend the role configuration arrays/env vars to collect manager and trader addresses, and enqueue the corresponding grantRole calls before the deployer relinquishes OWNER_ROLE.

The script never checks timelock.isOperationReady or whether a matching operation was scheduled; it immediately broadcasts execute calls. If the operation ID isn’t scheduled or the delay hasn’t elapsed, the call reverts and is silently swallowed (try-catch) leaving the system unconfigured.

Before each execute, query isOperationReady/isOperationPending and report missing scheduling or the remaining delay.

Function verifyDeployment confirms the timelock owns OWNER_ROLE and checks a few global parameters, then merely logs counts for collaterals/roles without asserting anything. It never ensures MANAGER_ROLE or AUTHORIZED_TRADER_ROLE are granted, nor that watcher quorum is achievable (quorum <= unique watchers). Because it only compares against env vars, any change to env values between execution and verification can produce false positives.

Extend verification to assert the exact addresses for every required role (including manager/trader), ensure watcher quorum <= distinct watcher count, and compare on-chain config against the stored deployment plan instead of mutable env vars.

Because every relayer/watcher/teller/guardian slot defaults to the same timelockAdmin address when its env var is unset, it’s easy to deploy with a single actor holding every operational role. AccessControl only stores unique members, so the three watcher entries collapse to one user; the queued setWatcherQuorum(2) call then reverts mid-run and the system launches half-configured.

Require explicit, non-zero, non-timelock addresses for each operational role.

The vault does not maintain or expose global aggregates and does not subtract pending liabilities from pricing. convertToShares and previewDeposit quote mints from the gross sUSD balance (totalAssets) and total share supply, ignoring sUSD already owed to withdrawals. As a result, deposit pricing and on‑chain views do not reflect liabilities. Users can mint against assets that are already committed to pending withdrawals and UIs cannot display a clear liability ratio. This can mislead depositors and amplify timing‑based gaming around the withdrawal queue.

Track and expose global aggregates and use net assets and circulating supply in convertToShares/previewDeposit to make mint pricing liability-aware.

The vault uses a step‑wise jump reward accumulator - the rewardPerShare increases discretely when addRewards is executed by the TELLER. A depositor’s _rewardDebt is set at the pre‑jump level, what allows an attacker to front‑run addRewards call with a deposit just before the reward distribution. Then, their new shares immediately capture rewardPerShare delta and can be claimed at once and the withdrawn process can be initiated right away. Consequently, MEV bots can monitor and front-run the TELLER’s addRewards calls, unfairly diluting distributions away from longer-term holders.

Consider making reward eligibility time-based or epoch-based to ensure fair distribution. Alternatively, implement a cooldown mechanism where each deposit sets an eligibility timestamp and pending rewards are computed only over the portion of the user’s balance that has passed the cooldown.

In SLPVault, the createWithdrawalRequests fixes amountSUSD at request creation, but the vault does not recompute the payout at disbursement. While the request is pending, shares remain in totalSupply and convertToShares computes deposit price using the gross sUSD balance, ignoring owed sUSD. If NAV per share drifts before disbursement, paying the fixed amountSUSD while burning the corresponding shares at the current supply transfers value between the withdrawer and all remaining holders.

Considering that fair = shares * (assets_current / supply_current):

• If amountSUSD > fair (overpay) - remaining holders are diluted by amountSUSD − fair (their per‑share NAV drops).
• If amountSUSD < fair (underpay) - remaining holders receive a windfall of fair − amountSUSD (their per‑share NAV rises).

Fixing amountSUSD at creation introduces unintended value transfers - NAV drift between creation and disbursement can overpay or underpay withdrawals, shifting value between remaining holders and new minters and exposing timing-based gaming and value extraction.

Recompute the sUSD payout at disbursement as convertToAssets(shares) using the current NAV, optionally enforcing a slippage tolerance.

The maxWithdraw function uses the full share balance (balanceOf(owner)) as the basis for the withdrawable amount. It ignores the user’s reserved shares, which back an existing withdrawal request. Also, it ignores the fact that a user with an active withdrawal request (_userActiveWithdrawalId[owner] != 0) cannot create another request. Because withdrawals in SLPVault are asynchronous and go through a separate request lifecycle, a user can only withdraw:

• at most their unreserved (available) shares,
• only if they do not already have an active request.

Despite this, maxWithdraw returns the assets corresponding to the entire balance, even when most or all of those shares are reserved and the user is blocked from initiating additional withdrawals. This breaks the vault expectation that maxWithdraw reflects what the user can actually withdraw.

Update maxWithdraw to be request and reservation aware, so it reflects what a user can actually withdraw.

Function disputeWithdrawals moves a request from Approved to Disputed, but _isWithdrawalExpired only checks the Approved and Validated states. Once a request is disputed it is immune to the expiry timer forever, so _reservedShares[user] and _userActiveWithdrawalId remain locked until a privileged role intervenes. A single dispute can therefore freeze a user’s shares indefinitely, enabling DoS against the vault.

Clearly document this risk.

In SLPVault, the deposit function mints at the spot rate and lacks a minSharesOut slippage check. Between quote and execution, the mint rate can worsen if price-per-share changes due to asymmetric state changes (e.g. mispriced withdrawal disbursals) or direct sUSD donations. Because the function does not validate the minted amount against a user‑supplied tolerable value, a depositor can receive fewer shares than expected with no way to revert based on price movement.

Add a slippage check that verifies the computed number of shares is within a user-specified minimum before proceeding with the deposit.

Function createWithdrawalRequests lets the relayer provide any (shares, amountSUSD, amountUSDT) tuple as long as the user owns at least that many shares and the vault currently holds the requested tokens. There is no check that amountSUSD corresponds to convertToAssets(shares) (plus rewards)

During disburseWithdrawals, the teller blindly burns shares and sends the prefilled amountSUSD/amountUSDT. In the most extreme case, it is possible to burn a token holder’s 1 wei of shares and pay them the entire vault's sUSD balance.

Consider enforcing the share<>asset conversion on chain during the withdrawal flow. Otherwise, ensure off-chain checks are appropriately performed.

When a relayer queues a withdrawal it must only show that the vault currently holds enough balance in the contracts for the requested amountUSD: if (contractBalanceUSDT < amountUSDT)

However, this does not consider the user's pending rewards. Consequently, users can claim rewards between request creation and disbursement to DoS the disburseWithdrawal queue.

Consider pending rewards in the balance check, although this would be a soft-check as pending rewards can increase by disbursal time. For a stricter check, explictly reserve USDT at request creation and include the withdrawer’s pending rewards in that reservation.

Any address can deposit the minimum 1 sUSD, then request a withdrawal through off-chain flow. Because createWithdrawalRequests processes every user individually and enforces no per-batch cap or fee, an attacker who spins up hundreds of funded wallets can force the RELAYER/WATCHER/TELLER roles to handle thousands of tiny requests on mainnet.

Each batch becomes prohibitively expensive, and the backlog delays honest withdrawals long enough that they hit withdrawalExpiryTimeout and expire, leaving users stuck in Status.Expired even though the problem is pure spam. This also griefs the Relayer as the contract is expensive and cost grows with more withdrawals.

Enforce a minimum withdrawal amount and consider a withdrawal fee.

Reward debt is calculating using (shares * rewardPerShare) / 1e18 which rounds down. However, this rounds in the user's favor since the smaller the debt, the more rewards they can claim. For safety, the reward debt calculation should round up.

Round up when calculating reward debt.

Reward per share is 6 decimals and is used in reward and reward debt calculations. It may be prudent to scale this value to higher precision (e.g. 18 decimals) to avoid unintended rounding scenarios with rewards/debt.

Consider magnifying reward per share and reward debt to a higher decimal precision before payout (ensure on payout precision goes back to 6 decimals for USDT)

addRewards updates rewardPerShare using floor division, so each distribution leaves an unallocated remainder that is not accumulated and included in the next reward update. While per-user rounding in pendingRewards/claimRewards rolls into future claims, this distribution-level remainder becomes permanent USDT dust that accumulates in the contract and creates a drift between USDT received and USDT ever claimable.

To minimize adding complexity, consider simply document this behavior. If a fix is implemented, add an only-owner sweep function that transfers only the USDT surplus above the explicitly tracked unclaimed rewards, and any non-core stray tokens. An alternative would be accumulating the remainder from each addRewards call in a dedicated variable and include it in the next reward update so that all USDT transferred into the vault via addRewards is eventually claimable by users.

The first deposit can be withdrawn below the minimum deposit amount, hence the initial supply is not locked and the classic "dead shares" fix to first-depositor is not implemented.

Consider having the protocol lock up the initial deposit and prevent the shares from going below that initial deposit on withdrawal so that supply is locked. Furthermore, consider increasing the first deposit amount.

The script claims to check "all role assignments" but it never queries DEFAULT_ADMIN_ROLE (0x00). This isn't a large gap since it is never explicitly granted but should be noted.

Consider including the DEFAULT_ADMIN_ROLE in the check and any future roles.

The cancelWithdrawal functions in both SLPVault and SynthetixDepositContract effectively allow user-initiated cancellation only from a single status (SLPVault: Status.Approved, SynthetixDepositContract: Status.Requested) and only when the request is not expired. After the early expiry check, the current logic redundantly excludes all other states (Validated, Disputed, and all final states: Disbursed, Denied, Cancelled, Expired), which is functionally equivalent to a single explicit state precondition and adds unnecessary complexity that can obscure intent and increase maintenance risk.

In SLPVault, replace the compound condition with a simple precondition requiring Status.Approved after the expiry check and in SynthetixDepositContract, require Status.Requested.

Function resolveDisputedWithdrawal in SynthetixDepositContract enforces per-token guardian approval limits, preventing a single guardian from green-lighting payouts above a configured cap. The analogous function in SLPVault simply flips a disputed request back to Validated with no size checks.

A compromised guardian can therefore approve a withdrawal that empties the entire vault, bypassing the usual watcher quorum, while the deposit contract would have blocked that action.

Consider adding guardian approval limits as in the SynthetixDepositContract.

createWithdrawalRequests iterates a batch supplied by the relayer, and if a user occurs twice, the second occurrence would trigger _userActiveWithdrawalId[user] != 0 and the function reverts, aborting the entire transaction.

If a malicious caller is able to force the relayer to include their address twice will be able to brick the whole batch, preventing any other users’ withdrawals from being queued.

Ensure the Relayer has safety checks to ensure a user cannot be included twice.

Currently getWithdrawalRequestsByUser loops through alll existent withdrawal requests and validates matches with the passed _user. Although this functions according to interface spec, it would be more efficient to having a mapping for (user => userRequestIds). Currently, if a user has sparse requests, the caller to getWithdrawalRequestsByUser has to keep bumping _offset and rescanning large gaps just to find entries.

Consider a mapping to allow for more efficient by-user querying.

Function requestWithdrawal no longer has access control, hence an attacker can pass a _user and create a withdrawal. Because a user can only have one withdrawal at a time, the attacker can trigger a withdrawal for just 1 wei and continuously block the user from actually withdrawing their funds.

Ensure the _user is msg.sender.

The requestWithdrawal function incorrectly uses previewWithdraw(_shares) to calculate the sUSD amount users should receive, however function previewWithdraw should accept assets as the parameter. Consequently, users can receive a fraction of the intended withdrawal.

For example:

• Share price = 2 sUSD / share
• User withdraws 100 shares
• Should receive: 100 shares * 2 = 200 sUSD
• Actually receives: 100 shares * 0.5 = 50 sUSD, loss of 150 sUSD

Calculate _amountSUSD with uint256 _amountSUSD = convertToAssets(_shares);

Function disburseWithdrawals adds pendingRewards() to the already stored req.amountUSDT from request time, causing double-counting since pendingRewards() calculates total accumulated, not delta since request. Consequently, users will receive double the rewards.

Consider calculating amountUSDT at disbursal time or update the reward debt at request time.

The DeploySynthetixDepositContractWithTimeLock script does not schedule a grant AUTHORIZED_TRADER_ROLE, leaving the CoW Protocol integration unusable after deployment

Schedule a grantRole operation for the AUTHORIZED_TRADER_ROLE in the script.

PostDeployment.s.sol is designed to execute timelock operations scheduled by DeploySynthetixDepositContractWithTimeLock.s.sol, but uses fundamentally different collateral configurations that will cause all execution attempts to fail. The timelock identifies scheduled operations by hashing the complete calldata. When DeploySynthetixDepositContractWithTimeLock.s.sol schedules addCollateral(token, config) with specific parameters (e.g. USDT with userMaximum: 100_000_000 * 1e6), it creates a unique hash.

PostDeployment.s.sol attempts to execute with completely different config values (USDT with userMaximum: 0), generating a different hash. The timelock will reject these as unscheduled operations. The limits between the same collaterals across the two scripts are different, as well the which tokens are enabled are different. Additionally, PostDeployment's use of 0 for all limit fields causes the buildCollateralSetup function to default userMaximum to type(uint256).max, creating unlimited deposit configurations if the script somehow succeeded

Remove duplicate collateral configuration logic from PostDeployment.s.sol and make it use the same configuration source as DeploySynthetixDepositContractWithTimeLock.s.sol