Treasury Updates

In the Treasury Market, the saddle() function either allocates rewards or initiates debt repayment based on the user's account debt:

• Zero debt: Rewards are allocated to depositRewards[accountId][config.token].
• Non-zero debt: Debt repayment is prioritized.

Only one of these actions occurs per saddle() call. Rewards are recorded during saddle() but not cleared during unsaddle(), relying on the next saddle() to overwrite or reset them. This assumption introduces vulnerabilities.

Issue 1: Reward Persistence Across Debt States

Steps:

1. User saddles with zero debt, earning rewards in depositRewards.
2. User unsaddles post vesting, claims rewards, but depositRewards remains uncleared.
3. User saddles again with non-zero debt (e.g., accrued via delegation to other pools).
4. Since debt > 0, saddle() prioritizes repayment and does not reset prior depositRewards.
5. User loops this process of saddling with non zero debt and unsaddle.

This could have been critical, however since the minimum delegation time is expected to be 7 days, the damage could be capped to first loop itself.

We tested this on a mainnet fork with poolId = 1 and confirmed that delegation is still possible for other pools, allowing the scenario described above.

Synthetix also handles cases where a user could saddle with a debt after their first saddle here, which enables this attack vector.

Issue 2: Reward Leakage via Config Manipulation

Even if Synthetix restricts debt accrual in other pools post-Treasury Market migration, reward leakage persists.

Steps:

1. User saddles while Token A is in depositRewardConfigurations, accruing rewards.
2. User unsaddles, but depositRewards for Token A is not reset.
3. SNX removes Token A from the config.
4. User saddles again—no reset occurs for Token A (it’s not in the active config).
5. Token A is re-added to the config later.
6. User claims Token A rewards accrued from the initial period, despite not staking during the new reward period. This undermines the time-based reward vesting logic.

Root Cause

The protocol assumes depositRewards is reset during saddle(), but this reset:

• Only applies to currently active reward tokens.
• Ignores tokens removed and re-added during unsaddle periods.
• Fails to account for debt changes between saddle-unsaddle cycles.

Consider reset of depositRewards[accountId] fully during unsaddle() to prevent stale reward data from persisting across states or config changes.

When LiquidationModule.liquidateToTreasury() is called, the treasury account is credited the liquidated collateral twice

// move the collateral

Account.load(treasuryAccountId).increaseAvailableCollateral(

collateralType,

liquidationData.collateralLiquidated

);

treasuryAccountEpoch.collateralAmounts.set(

// solhint-disable-next-line numcast/safe-cast

bytes32(uint256(treasuryAccountId)),

// solhint-disable-next-line numcast/safe-cast

treasuryAccountEpoch.collateralAmounts.get(bytes32(uint256(treasuryAccountId))) +

liquidationData.collateralLiquidated

);

The first part of the code deposits the collateral into the treasury account and the second one delegates it to the appropriate vault in the treasury pool. Normally, when delegations happen, the available collateral of the given user is decreased, otherwise it can be used again. As we can see from the code above, this step is missed here and in result the treasury account receives twice the collateral. This can lead to serious backing problems for Synthetix.

Consider removing the increaseAvailableCollateral call to achieve correct collateral delegation to the pool.

A new check has been added to burnUsd() to make sure the locked collateral cannot be burned.

if (

account.collaterals[usdToken.getAddress()].getTotalLocked() >

account.collaterals[usdToken.getAddress()].amountAvailableForDelegationD18

) {

revert InsufficientAvailableCollateral(

account.collaterals[usdToken.getAddress()].amountAvailableForDelegationD18,

account.collaterals[usdToken.getAddress()].getTotalLocked()

);

}

The burn transaction will revert if we leave our available amount lower than what's locked. However, if we look at CollateralModule.withdraw(), we can see the locked amount is applied on the total user collateral amount, i.e available + already delegated. Because of this, the burnUsd will be reverting in cases where it should succeed. For example:

• totalDeposited = 3000; assigned = 1000; locked = 800; availableForDelegation = 2000
• The unavailableCollateral = 1000, because 1000 (assigned) > 800 (locked)
• Therefore, we should be able to use all of the availableForDelegation = 2000 collateral
• However, burnUsd() will require at least 800 to be left in availableForDelegation, which makes it a total of 1800 locked amount instead of 1000.

This can result in the user being liquidated in situations where they need to repay their debt by burning sUsd and not being able to undelegate their collateral.

Change the check in burnUsd to calculate unavailableCollateral taking into account the total amount.

The owner can remove reward configurations from depositRewardConfigurations. This can be abused in the following way:

• A user frontruns the owner's transaction to saddle a large amount of funds which will record a big reward in their depositRewards for all the tokens (including the one to be removed)
• The owner transaction is executed and the given token is removed from depositRewardConfigurations
• After the minimum delegation time expires, the user can unsaddle. The removed token is not insidedepositRewardConfigurations, the rest of it is the same as when the user saddled. Because of this, the user receives their fair share of rewards for the rest of the tokens, but their data in depositRewards for the removed token is not changed, so it continues accruing rewards.
• If that token is ever re-added, the user can frontrun the admin transaction again to saddle and receive their rewards even though they aren't eligible for it.

You can introduce a new array containing the active reward loans for each users and use it in the unsaddle() flow, by either paying the user the tokens or clearing the removed ones.

When the liquidated collateral is delegated by adding it to treasuryAccountEpoch.collateralAmounts, liquidateToTreasury() the checks which usually run in delegateCollateral are not executed. This can lead to unexpected behaviors, including, but not limited to:

• delegating not enabled collateral if the liquidated collateral is not enabled on the treasury pool
• exceeding the pool collateral limit
• performing insufficient delegation, i.e below the minimum limit

If the team considers these behaviours as acceptable, then document them. Otherwise, checks can be added for each desired validation, but keep in mind that each check added increases the probability of DOS for liquidateToTreasury().

Since setDepositRewardConfigurations reverts if availableDepositRewards[token] > 0, anyone can front-run it by calling fundForDepositReward with even 1 wei, causing a revert and blocking config updates.

Code Reference

• Make fundForDepositReward permissioned or
• Call SNX Core to set maxDepositable = 0 temporarily to allow removal. Code Reference or
• Add a function in the peripheral contract that calls removeFromDepositReward(token, market.availableDepositRewards(token)) to remove the entire balance before making the call to set. or
• Use Front-running resistant RPC

When a user unsaddles, any available rewards will be unconditionally claimed. However, if the rewards are still not deposited and the amount recored in availableDepositRewards is less than what's to be claimed, the unsaddle will revert. The reward mechanism serve the user as a benefit, but in this case it blocks their withdrawal which may create big inconvenience if the user needs the funds immediately.

Consider adding forceUnsaddle boolean parameter to the unsaddle() function which if true skips any rewards that cannot be paid out.

The rewardAmount computed in saddle() depends on the result returned by the oracleManager to convert the collateral token to reward token. The result of rewardAmount should be in rewardTokenDecimals. Currently, the calculation for it will result in collateralTokenDecimals + oracleDecimals - 18.

We should have `collateralTokenDecimals + oracleDecimals - 18 = rewardDecimals => The following should hold true for each oracle:

oracleDecimals = rewardDecimals - collateralTokenDecimals + 18

Make sure each oracle returns the price in the correct decimal precision.

The owner can remove reward configurations from depositRewardConfigurations. Because of this, any unclaimed rewards for users will be lost when they unsaddle since the loop won't execute any logic for the corresponding token.

Make sure that in the event of reward token removal users are informed in advance so they can take the appropriate actions on time.

The rewards allocated to users are deposited into the market as collateral by the treasury. These rewards are withdrawn when users claim them during the unsaddle process.

The withdrawMarketCollateral function internally calls convertTokenToSystemAmount to convert the token amount into the system's internal accounting units.

 function convertTokenToSystemAmount(
        Data storage self,
        uint256 tokenAmount
    ) internal view returns (uint256 amountD18) {
      -------
                // ensure no precision is lost when converting to 18 decimals
                if (tokenAmount % (10 ** (decimals - 18)) != 0) {
                    revert PrecisionLost(tokenAmount, decimals);
                }
--------
}

However, if the token in question has more than 18 decimals, this conversion may revert due to precision loss during the conversion. This could cause unexpected reverts during user unsaddles.

If tokens with more than 18 decimals are used, ensure to truncate any precision beyond 18 decimals before invoking withdrawMarketCollateral.

The approvals for the reward tokens in the TreasuryMarket are never revoked, which means that if a token like USDT is used, its configuration cannot be edited and if once removed, it cannot be added again because the approval will try to change the already non-zero allowance value to another non-zero allowance value, which is not supported by these weird tokens.

Consider using an approach like safeApproveWithRetry() which will reset the approval to 0 first if needed.

In liquidateToTreasury() the treasuryAccountId is fetched from the _CONFIG_TREASURY_ACCOUNT_ID config and is checked to not be 0, but there is no check to ensure the account exists.

Check for account existence.

uint128 treasuryAccountId = uint128(Config.readUint(_CONFIG_TREASURY_ACCOUNT_ID, 0));

if (treasuryAccountId == 0) {
revert ParameterError.InvalidParameter("treasuryAccountId", "not set");
}
+ Account.exists(treasuryAccountId);

In liquidateToTreasury() the treasuryPoolId is fetched from the _CONFIG_TREASURY_POOL_ID config and is checked to not be 0, but there is no check to ensure the pool exists.

When loading the pool, instead of Pool.load(), use Pool.loadExisting(). This will revert if the pool doesn't exist.

The _decimalPow() helper function in the TreasuryMarket contract raises a given base to an exp by multiplying base by itself exp times. However, it uses mulDecimal for the multiplication, which works by multiplying the two numbers and removing 18 decimals after that. After the first cur = cur.mulDecimal(base) is executed, the new cur will be in base decimals. This will result in a wrong result from the second iteration and return a wrong result. Currently, the code uses this function only in _loanedAmount() and the base there has 18 decimals of precision, but if you decide to use the function with a different base in the future, it can result in unexpected behaviors.

Never use the function as it is for any base which is not in 18 decimals precision.

The comment on L239 appears misplaced. It likely refers to logic on L232.

Consider moving the comment to the appropriate line (L232) or removing it if no longer relevant.