Synthetix v3 Smart Contract Audit
# 1. Introduction
iosiro was commissioned by Synthetix to conduct a smart contract audit of their Synthetix v3 implementation. The audit was performed in multiple phases:
* **Phase 1**: Coverage of the core v3 system by 5 auditors between 2023-01-03 and 2023-02-03, using 84 resource days.
* **Phase 2**: Coverage of changes made in [SIP-318](https://sips.synthetix.io/sips/sip-318), [SIP-319](https://sips.synthetix.io/sips/sip-319), [SIP-320](https://sips.synthetix.io/sips/sip-320) and [SIP-321](https://sips.synthetix.io/sips/sip-321) by 2 auditors between 2023-04-11 and 2023-04-26, using 17 resource days.
This report is organized into the following sections.
- **[Section 2 - Executive summary:](about:blank#section-2)** A high-level description of the findings of the audit.
- **[Section 3 - Audit details:](about:blank#section-3)** A description of the scope and methodology of the audit.
- **[Section 4 - Design specification:](about:blank#section-4)** An outline of the intended functionality of the smart contracts.
- **[Section 5 - Detailed findings:](about:blank#section-5)** Detailed descriptions of the findings of the audit.
The information in this report should be used to understand the smart contracts’ risk exposure better and as a guide to improving the security posture of the smart contracts by remediating the issues identified. The results of this audit reflect the in-scope source code reviewed at the time of the audit.
The purpose of this audit was to achieve the following:
- Identify potential security flaws.
- Ensure that the smart contracts function according to the documentation provided.
Assessing the off-chain functionality associated with the contracts, for example, backend web application code, was outside of the scope of this audit.
Due to the unregulated nature and ease of transfer of cryptocurrencies, operations that store or interact with these assets are considered high risk from cyber attacks. As such, the highest level of security should be observed when interacting with these assets. This requires a forward-thinking approach, which takes into account the new and experimental nature of blockchain technologies. Strategies that should be used to encourage secure code development include:
- Security should be integrated into the development lifecycle, and the level of perceived security should not be limited to a single code audit.
- Defensive programming should be employed to account for unforeseen circumstances.
- Current best practices should be followed where possible.
# 2. Executive summary
This report presents the findings of an audit performed by iosiro of the Synthetix v3 implementation. The audit focused primarily on the core components of Synthetix v3, namely accounts, vaults, pools, liquidations, and market management functionality. The router proxy pattern, oracle manager, and utils were only covered to the extent necessary to audit the core components and were not comprehensively reviewed during this audit.
Following the initial audit in Q1 2023, a second phase of auditing was conducted at the start of Q2 2023, covering changes made in [SIP-318](https://sips.synthetix.io/sips/sip-318), [SIP-319](https://sips.synthetix.io/sips/sip-319), [SIP-320](https://sips.synthetix.io/sips/sip-320) and [SIP-321](https://sips.synthetix.io/sips/sip-321). Market implementations (e.g. Legacy Market and Spot Market) will undergo separate audits.
A detailed description of the concept and implementation of the system can be found in the [Synthetix v3 is on Mainnet](https://mirror.xyz/noahlitvin.eth/ZkqFOBsJP4WX-pDmVVWEJhB2dPoWED_6QqcriKULcME) blog post.
The following issues were identified during the audit:
- 6 high-risk issues, all of which have been addressed.
- 11 medium-risk issues, all of which have been addressed.
- 9 low-risk issues, 8 of which have been addressed.
- 9 informational issues, 6 of which have been addressed.
These issues can, in part, be attributed to the size and complexity of the codebase, as well as it being the first audit of the completely new system. While the system used many novel patterns, the code was generally found to conform to best practices and be of a high standard.
# 3. Audit details
## 3.1 Scope
The source code considered in-scope for the assessment is described below. Code from all other files was considered to be out-of-scope. Out-of-scope code that interacts with in-scope code was assumed to function as intended and not introduce any functional or security vulnerabilities for the purposes of this audit.
### 3.1.1 synthetix-v3
Solidity files in the following directories at the commits specified were in-scope for the audit.
**Initial Commit:** [d7cb00a](https://github.com/Synthetixio/synthetix-v3/tree/d7cb00a91a06f03f80d5088aebfee93c6fd44eda/)
- [protocol/synthetix/contracts/modules/](https://github.com/Synthetixio/synthetix-v3/tree/d7cb00a91a06f03f80d5088aebfee93c6fd44eda/protocol/synthetix/contracts/modules)
- [protocol/synthetix/contracts/storage/](https://github.com/Synthetixio/synthetix-v3/tree/d7cb00a91a06f03f80d5088aebfee93c6fd44eda/protocol/synthetix/contracts/storage)
**Final Commit:** [f471a2d](https://github.com/Synthetixio/synthetix-v3/tree/f471a2dce7176351a952a00df362543c177891e8)
- [protocol/synthetix/contracts/modules/](https://github.com/Synthetixio/synthetix-v3/tree/f471a2dce7176351a952a00df362543c177891e8/protocol/synthetix/contracts/modules)
- [protocol/synthetix/contracts/storage/](https://github.com/Synthetixio/synthetix-v3/tree/f471a2dce7176351a952a00df362543c177891e8/protocol/synthetix/contracts/storage)
## 3.2 Methodology
The audit was conducted using a variety of techniques described below.
### 3.2.1 Code review
The source code was manually inspected to identify potential security flaws. Code review is a useful approach for detecting security flaws, discrepancies between the specification and implementation, design improvements, and high-risk areas of the system.
### 3.2.2 Dynamic analysis
The contracts were compiled, deployed, and tested in a test environment, both manually and through the test suite provided. Manual analysis was used to confirm that the code was functional and discover security issues that could be exploited. The coverage report of the provided tests as on the final day of the audit is given below.
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Lines |
------------------------------------------------|----------|----------|----------|----------|----------------------------|
contracts/modules/ | 100 | 100 | 100 | 100 | |
InitialModuleBundle.sol | 100 | 100 | 100 | 100 | |
contracts/modules/account/ | 100 | 100 | 100 | 100 | |
AccountTokenModule.sol | 100 | 100 | 100 | 100 | |
contracts/modules/associated-systems/ | 100 | 100 | 100 | 100 | |
AssociatedSystemsModule.sol | 100 | 100 | 100 | 100 | |
contracts/modules/common/ | 100 | 100 | 100 | 100 | |
OwnerModule.sol | 100 | 100 | 100 | 100 | |
UpgradeModule.sol | 100 | 100 | 100 | 100 | |
contracts/modules/core/ | 100 | 84.78 | 100 | 91.11 | |
AccountModule.sol | 100 | 83.33 | 100 | 84.44 | 40-46, 200 |
AssociateDebtModule.sol | 100 | 100 | 100 | 100 | |
CollateralConfigurationModule.sol | 100 | 100 | 100 | 100 | |
CollateralModule.sol | 100 | 80.77 | 100 | 96.49 | 54, 148 |
FeatureFlagModule.sol | 100 | 100 | 100 | 100 | |
IssueUSDModule.sol | 100 | 81.25 | 100 | 95.12 | 71, 130 |
LiquidationModule.sol | 100 | 87.5 | 100 | 85.25 | 91, 241-257 |
MarketCollateralModule.sol | 100 | 85.71 | 100 | 97.87 | 169 |
MarketManagerModule.sol | 100 | 83.33 | 100 | 95.24 | 79, 86, 126 |
MulticallModule.sol | 100 | 75 | 100 | 75 | 26, 29 |
PoolConfigurationModule.sol | 100 | 100 | 100 | 100 | |
PoolModule.sol | 100 | 90 | 100 | 91.11 | 37, 90, 207-219 |
RewardsManagerModule.sol | 100 | 73.33 | 100 | 89.23 | 69, 77, 161, 214, 249, 255 |
UtilsModule.sol | 100 | 100 | 100 | 41.67 | 33-41, 60, 61 |
VaultModule.sol | 100 | 100 | 100 | 91.67 | 193, 195, 196, 200 |
contracts/modules/usd/ | 100 | 66.67 | 100 | 48 | |
USDTokenModule.sol | 100 | 66.67 | 100 | 48 | 29, 36, 59, 73, 104-132 |
contracts/storage/ | 100 | 88.31 | 100 | 97.37 | |
Account.sol | 100 | 100 | 100 | 100 | |
AccountRBAC.sol | 100 | 81.25 | 100 | 85.71 | 71, 75, 104 |
Collateral.sol | 100 | 100 | 100 | 100 | |
CollateralConfiguration.sol | 100 | 86.36 | 100 | 92.5 | 139, 177, 238 |
CollateralLock.sol | 100 | 100 | 100 | 100 | |
Config.sol | 100 | 100 | 100 | 100 | |
Distribution.sol | 100 | 100 | 100 | 100 | |
DistributionActor.sol | 100 | 100 | 100 | 100 | |
Market.sol | 100 | 91.18 | 100 | 98.88 | 310 |
MarketConfiguration.sol | 100 | 100 | 100 | 100 | |
MarketCreator.sol | 100 | 100 | 100 | 100 | |
MarketPoolInfo.sol | 100 | 100 | 100 | 100 | |
OracleManager.sol | 100 | 100 | 100 | 100 | |
Pool.sol | 100 | 90.63 | 100 | 100 | |
RewardDistribution.sol | 100 | 83.33 | 100 | 96.77 | 80 |
RewardDistributionClaimStatus.sol | 100 | 100 | 100 | 100 | |
ScalableMapping.sol | 100 | 87.5 | 100 | 94.74 | 65 |
SystemAccountConfiguration.sol | 100 | 100 | 100 | 100 | |
SystemPoolConfiguration.sol | 100 | 100 | 100 | 100 | |
Vault.sol | 100 | 50 | 100 | 93.55 | 134, 167 |
VaultEpoch.sol | 100 | 100 | 100 | 100 | |
Unit tests covered the majority of functionality but did not exhaustively test all code paths.
### 3.2.3 Automated analysis
Tools were used to automatically detect the presence of several types of security vulnerabilities, including reentrancy, timestamp dependency bugs, and transaction-ordering dependency bugs. Static analysis results were reviewed manually and any false positives were removed. Any true positive results are included in this report.
Static analysis tools commonly used include Slither, Securify, and MythX. Tools such as the Remix IDE, compilation output, and linters could also be used to identify potential areas of concern.
## 3.3 Risk ratings
Each issue identified during the audit has been assigned a risk rating. The rating is determined based on the criteria outlined below.
- **High risk**: The issue could result in a loss of funds for the contract owner or system users.
- **Medium risk**: The issue resulted in the code specification being implemented incorrectly.
- **Low risk**: A best practice or design issue that could affect the security of the contract.
- **Informational**: A lapse in best practice or a suboptimal design pattern that has a minimal risk of affecting the security of the contract.
- **Closed**: The issue was identified during the audit and has since been satisfactorily addressed, removing the risk it posed.
# 4. Design specification
The following section provides a high-level description of the system in-scope.
## 4.1 Oracle manager
The oracle manager was responsible for interacting with various third-party oracles, as well as allowing for validation of the responses.
## 4.2 Synthetix
### 4.2.1 Accounts and permissions
Accounts in the system were represented by ERC-721 tokens. The owner of the account token could transfer the token to another account to transfer ownership or delegate permissions to addresses for more granular access control.
### 4.2.2 Vaults
Vaults held collateral deposited into the system. Each vault instance represented a different collateral type for the pool it supported.
### 4.2.3 Pools
Pools were used to manage the distribution of credit and debt between depositors and markets. Accounts could delegate credit from their vault to a selected pool. The pool's owner could then decide how to allocate the delegated credit between markets.
### 4.2.4 Liquidations
If an account had insufficient collateral to support its debt, it would be subject to liquidation. The debt and collateral of a liquidated account would be socialized between all other accounts in the vault, excluding the liquidation fee paid. If the vault itself became undercollateralized, it would be subject to liquidation. During a vault liquidation, the liquidator would burn the debt owed by the vault and receive the vault’s remaining collateral as payment.
### 4.2.5 Market management
Markets could be registered to gain access to credit (liquidity) provided by depositors. Registered markets had the ability to report a balance of debt, deposit snxUSD, and withdraw snxUSD depending on the amount of liquidity delegated to them by pools.
### 4.2.6 Feature flags
Feature flags allowed the owner to configure whether specific parts of the system are enabled, disabled, or enabled for specific users.
### 4.2.7 Issue USD
The Issue USD module allowed accounts to mint and burn USD tokens against their provided collateral. In doing so, an equivalent amount of debt would be allocated to them.
### 4.2.8 Rewards
The Reward Distributor allowed approved actors to distribute rewards to accounts in a vault proportionately to their debt versus the total vault debt.
## 4.3 Utils
Utility contracts, such as UUPS proxy, ownership, and various token types were included in the project. These contracts had to be adapted from standard implementations to be compatible with the storage pattern used by the proxy.
## 4.4 Router
A novel proxy pattern, known as the Router Proxy, was used. Details on how this proxy works can be found in Synthetix’s [Router Proxy documentation](https://www.notion.so/PUBLIC-The-Router-Proxy-Architecture-3f4b68912c2241e8ae86baf9cf852d72).
## 4.5 Additional SIPs
* The specification of SIP-318 was based on commit hash [db6c224](https://github.com/Synthetixio/SIPs/blob/db6c2244f05aa4a9bb6b3113dd5b4015ec9b8c27/content/sips/sip-318.md).
* The specification of SIP-319 was based on commit hash [2ffd310](https://github.com/Synthetixio/SIPs/blob/2ffd31028b2c5b0ee7b40bd4191b3fdaeebc319f/content/sips/sip-319.md).
* The specification of SIP-320 was based on commit hash [2ffd310](https://github.com/Synthetixio/SIPs/blob/2ffd31028b2c5b0ee7b40bd4191b3fdaeebc319f/content/sips/sip-320.md).
* The specification of SIP-321 was based on commit hash [7ea466a](https://github.com/Synthetixio/SIPs/blob/7ea466afad0267b0fefab07feb62c51600b9827c/content/sips/sip-321.md).
# 5. Detailed findings
The following section details the findings of the audit.
## 5.1 High risk
All high-risk issues identified during the audit were closed.
## 5.2 Medium risk
All medium-risk issues identified during the audit were closed.
## 5.3 Low risk
### 5.3.1 Consider allowing pools to bypass the waiting period when increasing market capacity
*[PoolModule.sol#L121](https://github.com/Synthetixio/synthetix-v3/blob/0113c9db2c62a506bee1c2dfff304e6779fa57b8/protocol/synthetix/contracts/modules/core/PoolModule.sol#L121)*
#### Description
Pools must observe the waiting period between configurations regardless of whether a new configuration would increase or decrease capacity to a market. As with [Issue 5.5.23](#issue-5523), this may be less flexible than is ideal.
#### Recommendation
During the delegation period, it may be desirable to allow pools to bypass the limit if they are increasing their capacity to a market, and then reset the delegation timer. It should be noted that this case will likely be more complex than issue 5.5.10 to implement.
#### Synthetix response
To be addressed in the future.
## 5.4 Informational
### 5.4.1 Pools are collateral agnostic
Supported collateral types are configured on a system-wide level such that any pool can have a vault for any collateral type. This limits the risk management tools available to pool managers. Despite having a choice of pools, risk-averse participants do not have a reliable way of limiting their exposure to individual collateral types.
Ultimately, all collateral types can be used to issue system debt. However, debt not linked to a market can only be issued in proportion to the issuance ratio for the given collateral type. A more stringent issuance ratio is expected to be set for volatile collateral types, reducing their effect on the system debt.
Markets are not subject to the issuance ratio; instead, the `maxDebtPerShare` is used to limit the risk exposure of each pool. However, pools remain fully exposed to the collateral's volatility. Cautious participants may wish to delegate to pools that limit the contagion of a volatile collateral type.
Consider the following scenario: a pool is created with the express purpose of being risk-averse. This pool is only associated with well-established and robust markets, and the manager configures conservative `maxDebtPerShare` values. At some point, the Spartan Council performs a risk analysis on an algorithmic stablecoin and elects to enable it as a collateral type with an 800% issuance and 400% liquidation ratio. These ratios limit the amount of system debt that can be created to 12% of the collateral's value.
Over time, this stablecoin becomes the predominant (51%) collateral type within the pool. After achieving dominance, it depegs from its stable value. In this scenario, even though the Spartan Council identified and catered for the risk of the token, and the impact on the overall system debt should be minimal, the debt created by markets will be distributed to all the vaults within the pool. Consequently, the participants in the risk-averse pool will be liquidated, including participants who have not used the depegged stablecoin for collateral.
In summary, some participants will be more risk averse than the Spartan Council and may wish to limit their exposure to new collateral types, and this is not possible under the current design.
### 5.4.2 Potential for atomic cascading liquidations
The Synthetix v2 implementation uses a two-stage liquidation mechanism. An account is first flagged for liquidation and only after a set period can the account be liquidated. At the time of liquidation, the liquidated account’s debt and collateral are distributed to the rest of the stakers in the system. As an account can only be liquidated when its collateralization ratio is less than the liquidation ratio, these socialized liquidations will always slightly reduce the collateralization ratio of all other participants in the system.
In Synthetix v3, the same concepts apply, except that account liquidations are only socialized across members of the same vault and there is no flagging stage. As a result, it is possible to perform multiple liquidations atomically. Each liquidation would reduce the collateralization ratio of other accounts in the vault. Ordinarily, this would likely not be of much concern; however, in the case of a distressed market, malicious behavior may be incentivized. If the vault was to be slightly above the liquidation threshold, a malicious actor could theoretically attempt to strategically perform liquidations to put other accounts in liquidations, creating a cascading effect, which could even result in the vault itself being liquidatable — which would likely be highly profitable for the executor.
### 5.4.3 Design comments
Actions to improve the functionality and readability of the codebase are outlined below.
### Consider using time of unlock instead of time of last interaction
*General*
Instead of storing the "time of last interaction", a more fair pattern may be to store the "time of unlock." The current pattern means that accounts and pools are at the mercy of markets, which can arbitrarily change the duration of the locking period.
#### Synthetix response
This change could make pool configuration more expensive and may have cross-chain implications. It will be considered more fully in future.
### Consider separating market minimum delegation and pool reconfiguration
*General*
Currently, pool configuration changes can only be made after the longest minimum delegation time of the pool's attached markets has elapsed. This may be unintuitive and could cause unnecessary delays. Consider using a separate value to determine the minimum time between pool reconfigurations.
### Possibly unintended consequences of lock period offsets between pools and accounts
*General*
Pools and accounts will have lock periods that are offset from each other. This may introduce some unintended consequences. For example, accounts could be forced into undesirable markets by pools for some period of time and some malicious accounts may look to front-run the removal of a market to bypass the lock-in period.
## 5.5 Closed
### 5.5.1 Collateral locks do not prevent withdrawal (high risk)
*CollateralModule.sol*
#### Description
Addresses with the ADMIN permission can create collateral locks on an account to prevent withdrawal of a specified amount of a specified collateral type before a specified expiry date. However, the `CollateralModule.withdraw(...)` function does not consider the locked collateral amount when validating the amount to withdraw, rendering the locks ineffective.
#### Recommendation
When validating the amount to withdraw, the `CollateralModule.withdraw(...)` function should take the total locked amount into account when determining how much collateral can be withdrawn. One possible code change to achieve this is provided below:
```diff
diff --git a/protocol/synthetix/contracts/modules/core/CollateralModule.sol b/protocol/synthetix/contracts/modules/core/CollateralModule.sol
index 4f0f0402..aa29cd1e 100644
--- a/protocol/synthetix/contracts/modules/core/CollateralModule.sol
+++ b/protocol/synthetix/contracts/modules/core/CollateralModule.sol
@@ -72,7 +72,8 @@ contract CollateralModule is ICollateralModule {
.load(collateralType)
.convertTokenToSystemAmount(tokenAmount);
- if (account.collaterals[collateralType].availableAmountD18 < systemAmount) {
+ (uint256 totalDeposited, uint256 totalAssigned, uint256 totalLocked) = account.getCollateralTotals(collateralType);
+ if (totalDeposited - totalAssigned < systemAmount || totalDeposited - totalLocked < systemAmount) {
revert InsufficientAccountCollateral(systemAmount);
}
```
The resolution of this issue can be confirmed by adding the following unit test to `CollateralModule.deposit.test.ts`:
```diff
diff --git a/protocol/synthetix/test/integration/modules/core/CollateralModule/CollateralModule.deposit.test.ts b/protocol/synthetix/test/integration/modules/core/CollateralModule/CollateralModule.deposit.test.ts
index 28fb5b04..5f439ae4 100644
--- a/protocol/synthetix/test/integration/modules/core/CollateralModule/CollateralModule.deposit.test.ts
+++ b/protocol/synthetix/test/integration/modules/core/CollateralModule/CollateralModule.deposit.test.ts
@@ -3,12 +3,13 @@ import assertEvent from '@synthetixio/core-utils/utils/assertions/assert-event';
import assertRevert from '@synthetixio/core-utils/utils/assertions/assert-revert';
import { ethers as Ethers } from 'ethers';
import { ethers } from 'hardhat';
+import { fastForwardTo, getTime } from '@synthetixio/core-utils/utils/hardhat/rpc';
import { addCollateral, verifyCollateral } from './CollateralModule.helper';
import { bootstrap } from '../../../bootstrap';
-describe('CollateralModule', function () {
- const { signers, systems } = bootstrap();
+describe('CollateralModule', function () {
+ const { signers, systems, provider } = bootstrap();
let Collateral: Ethers.Contract, oracleNodeId: string;
@@ -90,7 +91,7 @@ describe('CollateralModule', function () {
describe('when depositing collateral', () => {
const depositAmount = ethers.utils.parseUnits('1', 6);
const systemDepositAmount = ethers.utils.parseEther('1');
-
+
before('deposit some collateral', async () => {
const tx = await systems()
.Core.connect(user1)
@@ -147,6 +148,29 @@ describe('CollateralModule', function () {
});
});
+ describe('when attempting to withdraw locked collateral', () => {
+ var lockedTill;
+ it('get current time', async() => {
+ const currentTime = await getTime(provider());
+ lockedTill = currentTime + 60*60*24*30;
+ });
+ it('reverts', async () => {
+ await systems().Core.connect(user1).createLock(1, Collateral.address, depositAmount, lockedTill );
+
+ // Collateral uses 6 decimals and system operates with 18,
+ // so scale up the expected amount.
+ const errorAmount = depositAmount.mul(ethers.BigNumber.from(10).pow(12));
+
+ await assertRevert(
+ systems().Core.connect(user1).withdraw(1, Collateral.address, depositAmount),
+ `InsufficientAccountCollateral("${errorAmount}")`,
+ systems().Core
+ );
+ });
+ it('unlock collateral', async () => {
+ await fastForwardTo(lockedTill, provider());
+ });
+ });
describe('when withdrawing collateral', () => {
before('withdraw some collateral', async () => {
const tx = await systems()
```
#### Update
The issue was fixed in the following commits: [d918209](https://github.com/Synthetixio/synthetix-v3/pull/1338/commits/d9182096af370a3de800efa7385a6605f82413b0), [87b7774](https://github.com/Synthetixio/synthetix-v3/pull/1338/commits/87b7774650aabd8982955cdbacbdcd73fc905fc6).
### 5.5.2 `IssueUSDModule` does not reduce connected markets' capacities (high risk)
*IssueUSDModule.sol*
#### Description
Accounts can issue USDToken against the collateral delegated to any pool. The debt created in this manner is assigned to the account and tracked as consolidated debt. The same collateral used to back the USDToken can be reused by any connected market, resulting in uncollateralized debt.
The unit test linked below was developed to demonstrate the issue:
[https://gist.github.com/iosiro-security/a94cf5522a0d04e667a9b0a03db35efb#file-issue-usd-capacity-diff](https://gist.github.com/iosiro-security/a94cf5522a0d04e667a9b0a03db35efb#file-issue-usd-capacity-diff)
It is important to note that if the order of operations is swapped so that the market first depletes its capacity, it is not possible to call `mintUsd()`. This is due to debt being tracked in `netIssuanceD18`, which is included in `totalDebt`, resulting in the account's collateralization ratio being below the target threshold.
This exploit is possible because vault collateral is used to determine the capacity available to the market without accounting for assigned debt. The code in question is given below for reference:
```solidity
usdWeightD18 = ((epochData.accountsDebtDistribution.totalSharesD18 * scaleModifierD27) /
1e27).mulDecimal(collateralPriceD18)
```
#### Recommendation
It is not clear how to modify the existing calculations to account for assigned debt. Using a revised debt tracking structure, the credit capacity of each market can be calculated while taking `assignedDebt` into account.
```solidity
usdWeightD18 = ((epochData.accountsDebtDistribution.totalSharesD18 * scaleModifierD27) /
1e27).mulDecimal(collateralPriceD18) - totalAssignedDebt;
```
Currently:
- `accountsDebtDistribution` tracks the total debt distributed by markets
- `unconsolidatedDebtD18` tracks the change in debt yet to be assigned to specific accounts
- `totalConsolidatedDebtD18` tracks the debt at points in time.
If debt was always distributed and not assigned, the debt tracked by `accountsDebtDistribution.getValuePerShare() * accountsDebtDistribution.totalSharesD18` would be equal to the sum of `totalConsolidatedDebtD18` and `unconsolidatedDebtD18`.
For this reason, it may make more sense to track the total debt in a vault using the distribution and the `totalAssignedDebt` and `assignedDebt` per account instead of consolidated debt. This would result in the following system-wide calculation changes:
```
totalDebt = accountsDebtDistribution.getValuePerShare() * accountsDebtDistribution.totalSharesD18;
debtOf(accoundId) = ((totalDebt - totalAssignedDebt) / accountsDebtDistribution.totalSharesD18) + assignedDebt(accountId);
```
#### Update
The issue was fixed as of [7ed26ea](https://github.com/Synthetixio/synthetix-v3/commit/7ed26ea0a792e6e78a6eb6c74a121f953af115a2).
### 5.5.3 Temporary market conditions can permanently prevent pools from rejoining (high risk)
*Market.sol*
#### Description
If a Market's reported debt exceeds the `maxDebtShareValue` of all of its associated pools at any point in time, all the pools will be moved from in-range to out-of-range. If the reported debt was to reduce again, the Market will fail to reassociate the pools, leaving it permanently without in-range pools.
The unit test at the link below was developed to demonstrate the issue:
[https://gist.github.com/iosiro-security/a94cf5522a0d04e667a9b0a03db35efb#file-inrange-pools-diff](https://gist.github.com/iosiro-security/a94cf5522a0d04e667a9b0a03db35efb#file-inrange-pools-diff)
The root cause of the issue is that once all pools have been moved to the out-of-range heap, the `poolsDebtDistribution` will be empty and the `bumpPools(...)` function will always return early:
```
if (maxDistributedD18 == 0 || self.poolsDebtDistribution.totalSharesD18 == 0) {
return (0, false);
}
```
#### Recommendation
The conditions under which `bumpPools(...)` returns early should be revised such that it only returns early when there is no change in the debt distribution.
```
diff --git a/protocol/synthetix/contracts/storage/Market.sol b/protocol/synthetix/contracts/storage/Market.sol
index db5ec882..c6c3a08d 100644
--- a/protocol/synthetix/contracts/storage/Market.sol
+++ b/protocol/synthetix/contracts/storage/Market.sol
@@ -413,7 +417,7 @@ library Market {
int256 maxDistributedD18,
uint256 maxIter
) internal returns (int256 actuallyDistributedD18, bool exhausted) {
- if (maxDistributedD18 == 0 || self.poolsDebtDistribution.totalSharesD18 == 0) {
+ if (maxDistributedD18 == 0) {
return (0, false);
}
```
This will, however, require modification to the calculations performed in `getTargetValuePerShare(...)` to prevent a division by zero condition.
#### Update
This issue was resolved as of [46c2604](https://github.com/Synthetixio/synthetix-v3/commit/46c2604e92edab3d9215c872cf6022192f79b211).
### 5.5.4 Reentrancy in `MarketCollateralModule.depositMarketCollateral()` (high risk)
*MarketCollateralModule.sol*
#### Description
If a collateral type that implemented a user-controlled post-transfer hook was enabled, a malicious market would be able to exploit a reentrancy condition to deposit collateral beyond the collateral type's `maximumDepositableD18`. As the `amountD18` property of the collateral entry is only updated after the collateral's transfer, a reentrant call to `depositMarketCollateral()` will perform the `collateralEntry.amountD18 + systemAmount > maxDepositable` check against the old value of `amountD18`.
#### Recommendation
A possible solution would be to check that the change in the collateral balance after the transfer matches what the market originally intended to deposit (`tokenAmount`). Alternatively, the balance-checking mechanism could be added to the `ERC20Helper.safeTransfer()` and `ERC20Helper.safeTransferFrom()` methods to apply the idea system-wide.
#### Update
This issue was resolved as of [8a34425](https://github.com/Synthetixio/synthetix-v3/commit/8a3442539699597a59e37860a8233c4abc0033a7).
### 5.5.5 Division by zero in `Pool.currentAccountCollateralizationRatio()` (high risk)
*Pool.sol*
#### Description
Whenever an account has negative or zero debt, the `currentAccountCollateralizationRatio()` function will attempt to divide by zero, causing the transaction to revert. This will cause calls to functions such as `VaultModule.getPositionCollateralizationRatio()` and `VaultModule.getPosition()` to revert, disallowing users with zero or negative debt to view their positions.
#### Recommendation
`currentAccountCollateralizationRatio()` should be adjusted to return a collateralization ratio of zero, or some other suitable value, when a user's debt is negative or zero. Care should be taken to ensure that the result of this function is only used to display to users, as using the return value for other state-changing operations could introduce other vulnerabilities.
#### Update
This issue was resolved as of [d858041](https://github.com/Synthetixio/synthetix-v3/commit/d8580417103064978da43452fbb19c6f11fa3684).
### 5.5.6 Potential reentrancy in `claimRewards()` (high risk)
*RewardsManagerModule.sol*
#### Description
The `claimRewards()` function may be susceptible to reentrancy attacks if a reward token that implements a callback or alternative mechanism for reentering the function is added. This is because `payout()` is called with an amount set by the `updateReward()` function, which is based in part on the value of `pendingSendD18`. If reentering the function, the `pendingSendD18` would remain constant between calls, as the value is only zeroed out after the call, allowing multiple payouts of the same amount to succeed.
#### Recommendation
`RewardsManagerModule.sol#L164` should be moved above `RewardsManagerModule.sol#L157` to ensure that the `pendingSend18` is zeroed out before an external call is made.
#### Update
This issue is resolved as of [510d243](https://github.com/Synthetixio/synthetix-v3/commit/510d2438041fa6fd89aadcd9cccb657bc93eeccc).
### 5.5.7 `AccountModule._isPermissionValid()` does not consider the `_REWARDS_PERMISSION` permission (medium risk)
*AccountModule.sol*
#### Description
The `AccountModule._isPermissionValid()` function, which is used when granting permissions, does not include the `_REWARDS_PERMISSION` permission. As a result, users will not be able to grant other users the ability to claim rewards, as `_REWARDS_PERMISSION` will be invalid.
#### Recommendation
The if statement should be adjusted to include `permission != AccountRBAC._REWARDS_PERMISSION`.
#### Update
This issue is resolved as of [5c927da](https://github.com/Synthetixio/synthetix-v3/commits/5c927da6677474f26722b70813d87886d5db48cc).
### 5.5.8 `_postTransfer` hook is not called for ERC-721 mint and burn operations (medium risk)
*ERC721.sol*
#### Description
In the `_mint()` and `_burn()` implementations in `ERC721.sol`, the `_beforeTransfer()` hook is called, while the `_postTransfer()` hook is not. This operates as intended for the AccountTokenModule.sol implementation; however, it does not seem like expected behavior for a generic ERC-721 implementation, as developers would likely assume that both pre- and post-transfer hooks are called when minting and burning. The [OpenZeppelin ERC721 implementation](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L285) calls both hooks when minting and burning.
#### Recommendation
If this functionality is intended, it should be documented. If not, the `_postTransfer()` hook should be called at the end of the mint and burn operations, and the AccountTokenModule.sol should be reassessed to correctly integrate the change.
#### Update
This issue was resolved as of [37dfda9](https://github.com/Synthetixio/synthetix-v3/commits/37dfda97fee4b1397235c5e8c0f0c0fe9049dab0).
### 5.5.9 Logic for `!exhausted` never reached in `Market.distributeDebtToPools()` (medium risk)
*Market.sol*
#### Description
`Market.distributeDebtToPools()` in the current implementation is always called with a large value for `maxIter` (`9999999999`), virtually guaranteeing that `Market.bumpPools()` iterates over all in-range or out-of-range pools. As a result, `bumpPools()` is unlikely to ever return `exhausted` as false, leaving the code in `distributeDebtToPools()`, that only executes when `!exhausted`, unreachable. This implies that the code might never be hit during coverage tests, and could be considered redundant if `maxIter` is sufficiently large.
#### Recommendation
It is recommended that either the logic that executes if `exhausted` is `false` is removed, or that additional tests are developed to ensure the system behaves as intended when `exhausted` is false.
#### Update
This issue was resolved as of [08c008c](https://github.com/Synthetixio/synthetix-v3/commit/08c008ce6acb3d5aa3e4a16c40d46a19943a5de0).
### 5.5.10 Incorrect calculation of`getSystemMaxDebtPerShare` (medium risk)
*Pool.sol*
#### Description
The calculation of `getSystemMaxDebtPerShare` produces values significantly lower than intended. As a consequence, the capacity provided to connected markets when more than one market is attached to a pool is considerably less than it should be.
The issue arises due to the market weight being applied twice, quadratically reducing the market’s available capacity.
Furthermore, when the system’s `minimumLiquidityRatio` is unset, the calculation is inconsistent with the design specification. It is expected that the `minimumLiquidityRatio` should default to 100%, when not configured.
#### Recommendation
The `getSystemMaxDebtPerShare` function should be revised to ensure that the correct values are returned under all configurations. Instead of a separate code branch for when `minimumLiquidityRatio` is unset, the value should instead default to 100% and follow the code branch for when the value is specified.
It is also advisable to expand the test suite to include multiple market configurations with varying values for `minimumLiquidityRatio` and `maxDebtPerShare`.
#### Update
The `getSystemMaxDebtPerShare` function was modified as per the recommendations provided in commit [6925e5b](https://github.com/Synthetixio/synthetix-v3/commit/6925e5b9d27096686512b62e3d55a453c2e8df4a)
<!-- SIP319 med -->
### 5.5.11 Fee not included in issuance ratio validation (medium risk)
*[IssueUSDModule.sol#L81](https://github.com/Synthetixio/synthetix-v3/blob/bd9363413b39ed4da83584fa2e8d08cc91104b13/protocol/synthetix/contracts/modules/core/IssueUSDModule.sol#L81)*
#### Description
The `IssueUSDModule.mintUsd()` function validates the account's issuance ratio in accordance with the new debt created. However, it did not include the fee in this calculation, which could lead to an account exceeding the issuance ratio following the mint operation.
#### Recommendation
`feeAmount` should be included when validating the account's issuance ratio.
#### Update
The issue was fixed in commit [d3263d6](https://github.com/Synthetixio/synthetix-v3/blob/d3263d6d19784dc0e63e617c108a2be39fd29bc2/protocol/synthetix/contracts/modules/core/IssueUSDModule.sol#L82).
### 5.5.12 Withdrawal does not consider fee in issuance check (medium risk)
*[MarketManagerModule.sol#L171](https://github.com/Synthetixio/synthetix-v3/blob/bd9363413b39ed4da83584fa2e8d08cc91104b13/protocol/synthetix/contracts/modules/core/MarketManagerModule.sol#L171)*
#### Description
The `MarketManagerModule.withdrawMarketUSD()` function does not consider the `feeAmount` when determining whether the market's balance allows for the withdrawal.
#### Recommendation
`feeAmount` should be included when calculating whether the market contains enough withdrawable snxUSD to facilitate the withdrawal.
#### Update
The issue was fixed in commit [d3263d6](https://github.com/Synthetixio/synthetix-v3/commit/d3263d6d19784dc0e63e617c108a2be39fd29bc2#diff-7bd783d312b0b8283ad40be6debcae0d791229bd655c394f7d434882299ae403R189).
### 5.5.13 Missing fee address validation (medium risk)
*[IssueUSDModule.sol#L102](https://github.com/Synthetixio/synthetix-v3/blob/bd9363413b39ed4da83584fa2e8d08cc91104b13/protocol/synthetix/contracts/modules/core/IssueUSDModule.sol#L102), [IssueUSDModule.sol#L144](https://github.com/Synthetixio/synthetix-v3/blob/bd9363413b39ed4da83584fa2e8d08cc91104b13/protocol/synthetix/contracts/modules/core/IssueUSDModule.sol#L144), [MarketManagerModule.sol#L149](https://github.com/Synthetixio/synthetix-v3/blob/bd9363413b39ed4da83584fa2e8d08cc91104b13/protocol/synthetix/contracts/modules/core/MarketManagerModule.sol#L149), [MarketManagerModule.sol#L186](https://github.com/Synthetixio/synthetix-v3/blob/bd9363413b39ed4da83584fa2e8d08cc91104b13/protocol/synthetix/contracts/modules/core/MarketManagerModule.sol#L186)*
#### Description
Functions which minted fees did not ensure that the address these fees were minted was non-zero. Should the fee address be set to zero, all functions including fee mints would revert, resulting in a denial of service.
#### Recommendation
A check should be included alongside the `amount > 0` check to ensure that the fee address is non-zero.
### Update
The issue was fixed in commit [d3263d6](https://github.com/Synthetixio/synthetix-v3/commit/d3263d6d19784dc0e63e617c108a2be39fd29bc2#diff-7bd783d312b0b8283ad40be6debcae0d791229bd655c394f7d434882299ae403R203).
<!-- SIP321 med -->
### 5.5.14 No approval for account transfer in `createAccount()` (medium risk)
*[AccountModule.sol#L82](https://github.com/Synthetixio/synthetix-v3/blob/52e020d1caa28ac64603be12634a06ecfd48a95c/protocol/synthetix/contracts/modules/core/AccountModule.sol#L82)*
#### Description
Functionality was included in the `createAccount()` function to transfer existing accounts with high IDs to the requesting user, on the assumption that accounts created with high IDs were made for the purpose of griefing. However, as the calling contract would not have authorisation to perform this transfer, the invocation of this functionality would lead to a revert. No test case for this functionality was included.
#### Recommendation
As per [issue 5.5.15](#issue-5515), a different pattern for account creation should be considered.
#### Update
The account transfer functionality was removed in commit [bc57a45](https://github.com/Synthetixio/synthetix-v3/commit/bc57a45f858873ec314179b93549869923d16af7#diff-93aed9b72a7b2f0645073239d4f7984e5741f878e6239247a7d4e990e4a520e9L80).
<a name="issue-5515"></a>
### 5.5.15 Arbitrary account transfers (medium risk)
*[AccountModule.sol#L82](https://github.com/Synthetixio/synthetix-v3/blob/52e020d1caa28ac64603be12634a06ecfd48a95c/protocol/synthetix/contracts/modules/core/AccountModule.sol#L82)*
#### Description
Functionality was included in the `createAccount()` function to transfer existing accounts with high IDs to the requesting user, on the assumption that accounts created with high IDs were made for the purpose of griefing. However, this may not be a safe assumption, and could lead to the theft of legitimate accounts.
#### Recommendation
Accounts created beyond the `uint128.max/2` offset should not be transferred to `msg.sender`. Instead, in the event of a collision, the offset could be incremented by a value that is hard to predict ahead of time, such as `msg.sender` cast to `uint64`, to reach a sparse region of storage. Afterwards, new accounts can iterate sequentially until the unlikely event of another collision.
#### Update
Synthetix v3 will be redeployed in future, obviating the need to account for already-created account IDs. Thus, functionality dealing with this edge-case was removed in commit [bc57a45](https://github.com/Synthetixio/synthetix-v3/commit/bc57a45f858873ec314179b93549869923d16af7#diff-93aed9b72a7b2f0645073239d4f7984e5741f878e6239247a7d4e990e4a520e9L80).
### 5.5.16 `createAccount()` does not check account creation feature flag (medium risk)
*[AccountModule.sol#L74](https://github.com/Synthetixio/synthetix-v3/blob/52e020d1caa28ac64603be12634a06ecfd48a95c/protocol/synthetix/contracts/modules/core/AccountModule.sol#L74)*
#### Description
The `createAccount()` function did not check the value of the `_CREATE_ACCOUNT_FEATURE_FLAG` feature flag before executing. This could allow users to create new accounts while account creation is disabled.
#### Recommendation
The `createAccount()` function should ensure that the account creation feature flag is enabled before executing.
#### Update
Fixed in commit [bc57a45](https://github.com/Synthetixio/synthetix-v3/commit/bc57a45f858873ec314179b93549869923d16af7#diff-93aed9b72a7b2f0645073239d4f7984e5741f878e6239247a7d4e990e4a520e9R75).
### 5.5.17 Potential reentrancy in `createAccount()` (medium risk)
*[AccountModule.sol#L88](https://github.com/Synthetixio/synthetix-v3/blob/52e020d1caa28ac64603be12634a06ecfd48a95c/protocol/synthetix/contracts/modules/core/AccountModule.sol#L88)*
#### Description
In `AccountModule.createAccount()`, the `accountId` variable is incremented after an external call to `accountTokenModule.safeMint()`. This could lead to reentrancy.
#### Recommendation
`systemAccountConfiguration.accountIdOffset` should be incremented directly after setting `accountId` to avoid any potential reentrancy concerns.
#### Update
Fixed in commit [bc57a45](https://github.com/Synthetixio/synthetix-v3/commit/bc57a45f858873ec314179b93549869923d16af7#diff-93aed9b72a7b2f0645073239d4f7984e5741f878e6239247a7d4e990e4a520e9R84).
### 5.5.18 `convertTokenToSystemAmount(...)` relies on optional ERC-20 `decimals()` function (low risk)
*CollateralConfiguration.sol*
#### Description
`ConvertTokenToSystemAmount` did not account for ERC-20 tokens that do not implement the `decimals()` function, or for tokens with zero decimals.
#### Recommendation
The `configureCollateral()` function should validate the decimals property when adding a new collateral type and implement defined behavior for when the property is absent.
#### Update
The recommendation was implemented over the following commits: [234c48c](https://github.com/Synthetixio/synthetix-v3/pull/1406/commits/234c48cd2cfe06c632f0d594b58b0b6021ba7097), [ce69ab1](https://github.com/Synthetixio/synthetix-v3/pull/1406/commits/ce69ab146009078bac7f62744d817bb21008fe49), [1e978d4](https://github.com/Synthetixio/synthetix-v3/pull/1406/commits/1e978d4f5d887fc8dfb7863143e8f38182f6d422), [999cd17](https://github.com/Synthetixio/synthetix-v3/pull/1406/commits/999cd17bd6872c6fd250f03b2a139dd6826e69d6). Tokens without a `decimals()` function are assumed to have 18 decimals.
### 5.5.19 Pool can be removed (low risk)
*VaultModule.sol*
#### Description
Under certain edge-cases, a pool may be removed while it still contains collateral from the account.
#### Recommendation
In `VaultModule._updatePosition()`, the line
```solidity
else if (collateral.pools.contains((poolId)))
```
should include an additional condition, `newCollateralAmount == 0`, to prevent a pool from being removed while it still contains collateral from the account.
This fix would render the `_ensureAccountCollateralsContainsPool()` function unnecessary.
#### Update
This recommendation was implemented in [f72f58c](https://github.com/Synthetixio/synthetix-v3/commit/f72f58c9f49cee39092edf93f0d0ac04b39f92a8).
### 5.5.20 New collateral amount can be the same as current (low risk)
*VaultModule.sol*
#### Description
`VaultModule.delegateCollateral()` could be called with a `newCollateralAmount` value equal to the `currentCollateralAmount`. This case is not explicitly handled in the code and could lead to unexpected behavior.
#### Recommendation
Consider reverting if `newCollateralAmount == currentCollateralAmount`.
#### Update
The recommendation was implemented in [5b21cfb](https://github.com/Synthetixio/synthetix-v3/pull/1397/commits/5b21cfb18734559c7b4105b984595a0a685070ca).
### 5.5.21 Non-functional implementation of test utility function `assertBn.near(...)` (low risk)
*assert-bignumber.ts*
#### Description
A utility function for testing that a number was within a certain range was found to produce false positives for all values. This was because the exception to be thrown when the assertion failed was not being passed up to the calling script, and thus the assertion would always be true.
#### Recommendation
The diff below suggests alterations to `assert-bignumber.ts` and `assert-bignumber.test.ts`, allowing it to pass up the `BigNumberAssertionError` for values not within the expected range.
```diff
diff --git a/utils/core-utils/src/utils/assertions/assert-bignumber.ts b/utils/core-utils/src/utils/assertions/assert-bignumber.ts
index 562f0204..7c94a52a 100644
--- a/utils/core-utils/src/utils/assertions/assert-bignumber.ts
+++ b/utils/core-utils/src/utils/assertions/assert-bignumber.ts
@@ -82,21 +82,20 @@ export = {
* Assert if `a` is within a small range of `b`
*/
near(a: BigNumberish, b: BigNumberish, tolerance: BigNumberish = 10000) {
- return () => {
- const abn = BigNumber.from(a);
- const bbn = BigNumber.from(b);
- const tolerancebn = BigNumber.from(tolerance);
+
+ const abn = BigNumber.from(a);
+ const bbn = BigNumber.from(b);
+ const tolerancebn = BigNumber.from(tolerance);
- const lower = bbn.sub(tolerancebn);
- const upper = bbn.add(tolerancebn);
+ const lower = bbn.sub(tolerancebn);
+ const upper = bbn.add(tolerancebn);
- if (abn.lt(lower) || abn.gt(upper)) {
- throw new BigNumberAssertionError({
- actual: abn,
- expected: bbn,
- operator: 'near',
- });
- }
- };
+ if (abn.lt(lower) || abn.gt(upper)) {
+ throw new BigNumberAssertionError({
+ actual: abn,
+ expected: bbn,
+ operator: 'near',
+ });
+ }
},
};
diff --git a/utils/core-utils/test/utils/assertions/assert-bignumber.test.ts b/utils/core-utils/test/utils/assertions/assert-bignumber.test.ts
index 11cee3ee..2005880a 100644
--- a/utils/core-utils/test/utils/assertions/assert-bignumber.test.ts
+++ b/utils/core-utils/test/utils/assertions/assert-bignumber.test.ts
@@ -64,4 +64,10 @@ describe('utils/assertions/assert-bignumber.ts', function () {
it('#isZero', function () {
bn.isZero(0);
});
+
+ it("#near", function() {
+ bn.near(200000, 200001);
+ bn.near(200001, 200000);
+ not('near', 200000, 5000);
+ });
});
```
#### Update
The recommendation above was implemented in [dfc4f10](https://github.com/Synthetixio/synthetix-v3/commit/dfc4f1058a69871c775efa2332b42e40d659ef5a#diff-e8d018c3e97a0f207ef83c9b914bd8389967714e843a305c733d1ff217b12c4cL85).
<!-- SIP319 low -->
### 5.5.22 `setConfig()` does not validate system values (low risk)
*[UtilsModule.sol#L58](https://github.com/Synthetixio/synthetix-v3/blob/bd9363413b39ed4da83584fa2e8d08cc91104b13/protocol/synthetix/contracts/modules/core/UtilsModule.sol#L58)*
#### Description
The `UtilsModule.setConfig()` function does not enforce any validation of system values.
#### Recommendation
Configuration values, such as the fee ratios and fee address, should be required to implement relevant validation.
#### Synthetix response
Validation of configuration values is intended to be performed in the functions which use them. It should be possible to set, for example, `feeAddress` to any address, or to set `feePercentage` to a high value such as 200%.
<!-- SIP320 low -->
### 5.5.23 Minimum delegation time can be zero (low risk)
*[MarketManagerModule.sol#L239](https://github.com/Synthetixio/synthetix-v3/blob/0113c9db2c62a506bee1c2dfff304e6779fa57b8/protocol/synthetix/contracts/modules/core/MarketManagerModule.sol#L239)*
#### Description
Markets can set a minimum delegation time of zero, which could open them up to exploitation. The potential for exploitation is market-dependent and in some cases it may be desirable for markets to have 0 values; however, this should be assessed on a case-by-case basis.
#### Recommendation
To avoid this manner of market exploitation, a global minimum delegation time could be enforced in `setMarketMinDelegateTime()`.
#### Synthetix response
As some markets will not need a minimum delegation time, this configuration has not been restricted. Synthetix may implement the provided recommendation in future, should it be practically necessary to mitigate front-running.
<a name="issue-5523"></a>
### 5.5.24 Consider allowing account delegation increases during waiting period (low risk)
*[VaultModule.sol#L66](https://github.com/Synthetixio/synthetix-v3/blob/0113c9db2c62a506bee1c2dfff304e6779fa57b8/protocol/synthetix/contracts/modules/core/VaultModule.sol#L66)*
#### Description
The minimum delegation time applies to both decreases and increases in collateral delegation. This may be less flexible than is ideal.
#### Recommendation
While accounts should be prohibited from reducing their delegated collateral for the full period, it may be desirable for accounts to still be able increase their collateral delegation during this period, and in so doing, reset their `lastDelegation` time.
#### Update
Fixed in commit [d3263d6](https://github.com/Synthetixio/synthetix-v3/commit/d3263d6d19784dc0e63e617c108a2be39fd29bc2#diff-006a0f2fc707929806dfc5fcc194297e0de6efdd9fb2f7e583e916b820f28bc2R89).
### 5.5.25 Per-market minimum delegation is unresponsive to global minimum changes (low risk)
*[Pool.sol#L448](https://github.com/Synthetixio/synthetix-v3/blob/0113c9db2c62a506bee1c2dfff304e6779fa57b8/protocol/synthetix/contracts/storage/Pool.sol#L448)*
#### Description
When the global minimum delegation time changes, markets are not forced to adhere to the new limit unless they update their own minimum delegation configuration after the change. This may prevent updates to the global minimum from having the intended effect.
#### Recommendation
`requireMinDelegationTimeElapsed()` could be adjusted to use the globally configured minimum delegation time in the event that the market's minimum delegation time is out of bounds.
#### Update
Fixed in commit [d3263d6](https://github.com/Synthetixio/synthetix-v3/commit/d3263d6d19784dc0e63e617c108a2be39fd29bc2#diff-31e200943c6f6c72f621b7e712c638d50dfa860f81ec71a6703a90b1355e838eR379).
### 5.5.26 `removeRewardsDistributor(...)` does not delete reward entry storage (informational)
*RewardsManagerModule.sol*
#### Description
The function `RewardsManagerModule.removeRewardsDistributor(...)` does not delete the storage of the reward entry. Thus, when adding the distributor again, the previous distribution will still be valid, which may lead to unexpected behavior.
#### Recommendation
The function should delete the reward entry from storage unless this is intentional behavior. If so, the behavior should be documented.
#### Update
In [05ae8d8](https://github.com/Synthetixio/synthetix-v3/pull/1439/commits/05ae8d8d2933bf4fd4e89cb4e4698ab911a80ccb), the function was altered to zero out the removed distributor in storage.
### 5.5.27 Permissions can be removed for the account owner (informational)
*AccountModule.sol*
#### Description
The account permissions model allows for an account’s owner to revoke permissions or have permissions removed from them by an address with ADMIN control over the account. This behavior may be unexpected.
#### Recommendation
The removal or revocation of permissions from the account owner could be prevented.
#### Update
Synthetix feedback: A user could revoke their ADMIN permission, but since the owner can do anything, they could eventually grant the ADMIN permission again at a later time.
<!-- SIP319 info -->
### 5.5.27 Fee amount not emitted by relevant events (informational)
*[IssueUSDModule.sol#L106](https://github.com/Synthetixio/synthetix-v3/blob/bd9363413b39ed4da83584fa2e8d08cc91104b13/protocol/synthetix/contracts/modules/core/IssueUSDModule.sol#L106), [IssueUSDModule.sol#L156](https://github.com/Synthetixio/synthetix-v3/blob/bd9363413b39ed4da83584fa2e8d08cc91104b13/protocol/synthetix/contracts/modules/core/IssueUSDModule.sol#L156), [MarketManagerModule.sol#L153](https://github.com/Synthetixio/synthetix-v3/blob/bd9363413b39ed4da83584fa2e8d08cc91104b13/protocol/synthetix/contracts/modules/core/MarketManagerModule.sol#L153), [MarketManagerModule.sol#L190](https://github.com/Synthetixio/synthetix-v3/blob/bd9363413b39ed4da83584fa2e8d08cc91104b13/protocol/synthetix/contracts/modules/core/MarketManagerModule.sol#L190)*
#### Description
The `feeAmount` is not emitted in any events emitted to log actions where it is charged.
#### Recommendation
Consider including the fee amount as a parameter in events describing fee-charging actions.
#### Update
New fee payment events added in commit [d3263d6](https://github.com/Synthetixio/synthetix-v3/commit/d3263d6d19784dc0e63e617c108a2be39fd29bc2#diff-00034a7ee192cb4bdc2cdc12964006b3a6f4492a12ec5f56a86f50978f94e12fR45).
### 5.5.28 Mint and burn fees may complicate access to collateral (informational)
*General*
#### Description
Charging a fee for minting and burning USD may make it difficult for accounts to access their underlying collateral. For example, in a pool that is only exposed to minting and burning snxUSD, if a 3% fee is applied to minting and burning, 100 snxUSD of debt will require ~106 snxUSD to unlock, which will require the exiting account to acquire additional snxUSD from another source. While this may support the peg, it could make for a poor user experience.
#### Synthetix response
The current tradeoff between user experience and protocol functionality is acceptable.
<!-- SIP320 info -->
### 5.5.29 `loadExisting()` used in place of `load()` (informational)
*[VaultModule.sol#L65](https://github.com/Synthetixio/synthetix-v3/blob/0113c9db2c62a506bee1c2dfff304e6779fa57b8/protocol/synthetix/contracts/modules/core/VaultModule.sol#L65)*
#### Description
In `VaultModule.delegateCollateral()`, the function `Pool.load()` is used to access a pool that may not exist. Using `Pool.loadExisting()` could prevent issues that may arise from attempting to access a non-existent pool.
#### Recommendation
Use `Pool.loadExisting()` instead of `Pool.load()`.
#### Update
Changed to `Pool.loadExisting()` in commit [d3263d6](https://github.com/Synthetixio/synthetix-v3/commit/d3263d6d19784dc0e63e617c108a2be39fd29bc2#diff-006a0f2fc707929806dfc5fcc194297e0de6efdd9fb2f7e583e916b820f28bc2R65).
### 5.5.30 Design comments (informational)
#### Refactoring suggestions
Recommendations for improving code clarity, consistency, and concision are provided below.
1. Vault.sol: Cache array lengths in `updateReward()`
2. Consider separating the vault liquidation ratio value from the position liquidation value for finer control of system parameters.
3. FeatureFlagModule.sol: Feature flags could include a `denyAll` attribute, which would allow for specific features to be explicitly paused.
4. RewardsManagerModule.sol: `claimRewards()` can validate the collateralization ratio to only pay out rewards if the position is currently healthy (similarly to the v2 staking rewards).
##### Update
1. Implemented in [0be6b6e](https://github.com/Synthetixio/synthetix-v3/pull/1394/commits/0be6b6ea6fd58e7baf450cc822c6c8f1add47cad).
2. Synthetix feedback: Interesting suggestion, but currently not seen as adding practical value.
3. Implemented in [a822cc7](https://github.com/Synthetixio/synthetix-v3/pull/1398/commits/a822cc79f7ddff2e458f0d2105615de91ee6ce03).
4. Synthetix feedback: We plan to introduce this kind of logic at a reward distributor level so it can be fine-tuned.
#### Naming and comment clarity
The following commenting and naming inconsistencies were found in the codebase:
1. RewardsManagerModule.sol: `getClaimableRewards()` modifies state and should be renamed to reflect its behavior, e.g. `updateRewards()`.
2. Collateral.sol: The comment above `CollateralLock.Data[]` locks reads "Note: Locks apply to collateral delegation (see VaultModule), and not to withdrawing collateral." This is incorrect and should be reconciled with the docstring for `CollateralModule::createLock()`.
3. VaultModule.sol: In `delegateCollateral()`, the variable `newCollateralAmount` should be named `newCollateralAmountD18`.
##### Update
1. Implemented in [5a84c44](https://github.com/Synthetixio/synthetix-v3/pull/1387/commits/5a84c4413a1188cf49e25c078ff7fd781ddd9ced).
2. Implemented in [d4704db](https://github.com/Synthetixio/synthetix-v3/pull/1391/commits/d4704db0a5df5cb1497c2ccb20d214df6a7eb92c).
3. Implemented in [2f9650d](https://github.com/Synthetixio/synthetix-v3/pull/1392/commits/2f9650de2e8d1bd441dd64d220ff199ed8021a19).
