Synthetix 3.2.0 Smart Contract Audit
# 1. Introduction
iosiro was commissioned by Synthetix to conduct a smart contract audit of changes to the Synthetix V3 system. The initial audit was performed by two auditors between 3 August 2023 and 11 August 2023, as well as a review of changes between 23 August 2023 and 25 August 2023. In total, the audit spanned over 12 resource days.
This report is organized into the following sections.
- **[Section 2 - Executive summary:](#section-2)** A high-level description of the findings of the audit.
- **[Section 3 - Audit details:](#section-3)** A description of the scope and methodology of the audit.
- **[Section 4 - Design specification:](#section-4)** An outline of the intended functionality of the smart contracts.
- **[Section 5 - Open findings:](#section-5)** Detailed descriptions of audit findings that remained present at the conclusion of the audit.
- **[Section 6 - Closed findings:](#section-6)** Detailed descriptions of audit findings that were addressed during the audit.
The information in this report should be used to better understand the smart contracts' risk exposure and as a guide to improving the security posture of the smart contracts by remediating the identified issues. 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 this audit's scope.
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 considering blockchain technologies' new and experimental nature. 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.
<a name="section-2"></a>
# 2. Executive summary
This report details the results of an audit conducted by iosiro on the enhancements made to the Synthetix V3 system. The audit encompassed the following Synthetix Improvement Proposals (SIPs):
1. **[SIP-323](https://sips.synthetix.io/sips/sip-323/)**: Adopt "s" Prefix for Synthetix V3 Asset Tickers
1. **[SIP-326](https://sips.synthetix.io/sips/sip-326/)**: Additional Pool Configuration (Issuance Ratio)
1. **[SIP-331](https://sips.synthetix.io/sips/sip-331/)**: Adding View Function to Retrieve Market Address by Market ID
1. **[SIP-332](https://sips.synthetix.io/sips/sip-332/)**: Mint and Burn with Account Balance
1. **[SIP-333](https://sips.synthetix.io/sips/sip-333/)**: Pool Collateral Caps
1. **[SIP-334](https://sips.synthetix.io/sips/sip-334/)**: Add Optional Bytes Field for Oracle Manager Requests
1. **[SIP-335](https://sips.synthetix.io/sips/sip-335/)**: Allow Revoking ERC20 Approvals
Additionally, several minor alterations in the codebase, primarily focusing on enhancing code quality and implementing security measures, were examined.
The audit findings are summarized as follows:
- Two high-risk issues that have been resolved.
- Four low-risk concerns were identified, all of which have been suitably addressed.
- Various suggestions to improve code quality.
<a name="section-3"></a>
# 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 this audit.
Solidity files in the following directories at the commits specified were in-scope for the audit.
* **Initial commit**: [9377efe](https://github.com/Synthetixio/synthetix-v3/tree/9377efeb6e8773318ef01be45762aca31aed6f1f)
- [protocol/oracle-manager/contracts/modules/](https://github.com/Synthetixio/synthetix-v3/tree/9377efeb6e8773318ef01be45762aca31aed6f1f/protocol/oracle-manager/contracts/modules)
- [protocol/oracle-manager/contracts/storage/](https://github.com/Synthetixio/synthetix-v3/tree/9377efeb6e8773318ef01be45762aca31aed6f1f/protocol/oracle-manager/contracts/storage)
- [protocol/synthetix/contracts/modules/](https://github.com/Synthetixio/synthetix-v3/tree/9377efeb6e8773318ef01be45762aca31aed6f1f/protocol/synthetix/contracts/modules)
- [protocol/synthetix/contracts/storage/](https://github.com/Synthetixio/synthetix-v3/tree/9377efeb6e8773318ef01be45762aca31aed6f1f/protocol/synthetix/contracts/storage)
* **Final commit**: [ce5edb2f](https://github.com/Synthetixio/synthetix-v3/tree/ce5edb2f96d466fe42a0f714e8d0334adc112b7a)
- [protocol/oracle-manager/contracts/modules/](https://github.com/Synthetixio/synthetix-v3/tree/ce5edb2f96d466fe42a0f714e8d0334adc112b7a/protocol/oracle-manager/contracts/modules/)
- [protocol/oracle-manager/contracts/storage/](https://github.com/Synthetixio/synthetix-v3/tree/ce5edb2f96d466fe42a0f714e8d0334adc112b7a/protocol/oracle-manager/contracts/storage/)
- [protocol/synthetix/contracts/modules/](https://github.com/Synthetixio/synthetix-v3/tree/ce5edb2f96d466fe42a0f714e8d0334adc112b7a/protocol/synthetix/contracts/modules/)
- [protocol/synthetix/contracts/storage/](https://github.com/Synthetixio/synthetix-v3/tree/ce5edb2f96d466fe42a0f714e8d0334adc112b7a/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 valuable 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 the code's functionality and discover security issues that could be exploited.
### 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 identify potential areas of concern.
## 3.3 Summary of findings
The table below provides an overview of the audit's findings. Detailed write-ups are provided in [Section 5](section-5) and [Section 6](#section-6).
<table><thead><tr><th>#</th><th>Issue</th><th>Risk</th><th>Status</th></tr></thead><tbody><tr><td><a href="#issue-6-1-1">6.1.1</a></td><td>New collateral types are unrestricted</td><td class="risk-high">High</td><td class="status-closed">Closed</td></tr><tr><td><a href="#issue-6-1-2">6.1.2</a></td><td>Collateral balances are incorrectly updated when calling <code>burnUsd</code></td><td class="risk-high">High</td><td class="status-closed">Closed</td></tr><tr><td><a href="#issue-6-3-1">6.3.1</a></td><td>Incorrect precision when validating <code>liquidationRatioD18</code> and <code>issuanceRatioD18</code></td><td class="risk-low">Low</td><td class="status-closed">Closed</td></tr><tr><td><a href="#issue-6-3-2">6.3.2</a></td><td>Undocumented breaking changes</td><td class="risk-low">Low</td><td class="status-closed">Closed</td></tr><tr><td><a href="#issue-6-3-3">6.3.3</a></td><td>Incorrect event emitted</td><td class="risk-low">Low</td><td class="status-closed">Closed</td></tr><tr><td><a href="#issue-6-3-4">6.3.4</a></td><td>Mismatched array lengths</td><td class="risk-low">Low</td><td class="status-closed">Closed</td></tr><tr><td><a href="#issue-6-4-1">6.4.1</a></td><td>Design comments</td><td class="risk-informational">Informational</td><td class="status-closed">Closed</td></tr></tbody></table>
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.
In addition to a risk rating, each issue is assigned a status:
- **Open**: The issue remained present in the code as of the final commit reviewed and may still pose a risk.
- **Closed**: The issue was identified during the audit and has since been satisfactorily addressed, removing the risk it posed.
<a name="section-4"></a>
# 4. Design specification
The following section provides a high-level description of the system in-scope.
### SIP-323
Upon the initial deployment of Synthetix V3, the **snxUSD** symbol was designated for the new stablecoin. To uphold the Synthetix brand continuity and reduce confusion, the specification proposes an update to the ERC20 name and symbol of the token to align with the existing **sUSD** token.
This implementation leveraged pre-existing features within the Core system to renew the token's name and symbol, ensuring no changes to balances, total supply, or token decimals.
For further details, refer to [SIP-323](https://sips.synthetix.io/sips/sip-323).
### SIP-324
The proposal is limited to a configuration change to introduce an SNX-only pool managed by the Spartan Council. The pool should support no other collateral types, and it should not be possible for accounts to associate debt with the pool.
No smart contract code changes were introduced in SIP-324. Nevertheless, the specification was reviewed and considered through the audit as a number of the other SIPs aim to facilitate an SNX-only pool.
For further details, refer to [SIP-324](https://sips.synthetix.io/sips/sip-324).
### SIP-326
This SIP offers enhanced granular control over the issuance ratios for each collateral type in a pool. The design permits only configurations that are stricter than system-wide defaults and can be set by the pool's owners.
It was observed during the audit that [SIP-326](https://sips.synthetix.io/sips/sip-326/) had significant similarities to [SIP-333](https://sips.synthetix.io/sips/sip-333/), prompting a combined review.
For further details, refer to [SIP-326](https://sips.synthetix.io/sips/sip-326/).
### SIP-331
The specification proposes the addition of a view function to obtain the address of a market.
For further details, refer to [SIP-331](https://sips.synthetix.io/sips/sip-331/).
### SIP-332
SIP-332 recommends a slight alteration to the stablecoin issuance mechanism in Synthetix V3. The aim is to have stablecoin issuances conform to the same withdrawal cooldowns and restrictions as other collateral types. This was proposed in anticipation of incorporating cross-chain pools, a feature unavailable during the audit period.
Instead of transferring tokens to and from the caller, stablecoin issuance and burning now increase or decrease the caller’s stablecoin collateral balance.
For further details, refer to [SIP-332](https://sips.synthetix.io/sips/sip-332/).
### SIP-333
SIP-333 grants pool owners the authority to dictate the type of collateral allocated to their pools. The audit revealed that the original specification would put pools at risk when a new collateral type is introduced. Consequently, the design was adapted so that only explicitly configured collateral types are allowed when the pool owner activates the feature.
For further details, refer to [SIP-333](https://sips.synthetix.io/sips/sip-333/).
### SIP-334
SIP-334 suggests a change to the Synthetix protocol's Oracle Manager. The enhancement allows passing custom key-value pairs to the Oracle Manager as parameters in requests, then passing them to downstream Oracle nodes that support more dynamic behavior based on input.
For further details, refer to [SIP-334](https://sips.synthetix.io/sips/sip-334/).
### SIP-335
SIP-335 introduces a subtle refinement to Synthetix V3's ERC20 framework. This upgrade aims to adjust the validation procedure within the `_approve` function to follow the ERC20 standard more closely, enabling the revocation of pre-set allowances.
For further details, refer to [SIP-335](https://sips.synthetix.io/sips/sip-335/).
<a name="section-5"></a>
# 5. Open findings
The following section details the findings of the audit which remained open at the conclusion of the final review.
## 5.1 High risk
No high-risk issues identified during the audit were present at the conclusion of the audit.
## 5.2 Medium risk
No medium-risk issues identified during the audit were present at the conclusion of the audit.
## 5.3 Low risk
No low-risk issues identified during the audit were present at the conclusion of the audit.
## 5.4 Informational
No informational issues identified during the audit were present at the conclusion of the audit.
<a name="section-6"></a>
# 6 Closed findings
The following section details findings of the audit which were addressed during the audit.
## 6.1 High risk
<a name="issue-6-1-1"></a>
### 6.1.1 New collateral types are unrestricted
*[Pool.sol#L531](https://github.com/Synthetixio/synthetix-v3/blob/4a02874b70bbd7b6a8f9c121a21058b634e92374/protocol/synthetix/contracts/storage/Pool.sol#L531)*
#### Description
The default behavior for any new collateral type was to allow delegation and issuance. This was concerning in the context of [SIP-324](https://sips.synthetix.io/sips/sip-324/), as the implementation of a strict SNX-only pool would require that all new collateral types to be explicitly disabled immediately upon deployment.
Furthermore, it would not be possible for pools not owned by the Spartan Council to effectively limit the collateral types that can be delegated, rendering the feature ineffective.
#### Recommendation
An alternative approach would be to add a `limitCollateralTypes` property for each pool that, when enabled, limits delegation to only the collateral types explicitly enabled by the pool owner. The `collateralTypeDisabled` mapping should be replaced with `collateralTypeEnabled`, inverting the logic.
#### Update
A `collateralDisabledByDefault` flag was introduced as a configurable property for each pool. When set, the delegation is only permitted for the collateral types that have a non-zero `collateralLimitD18` configured. This aligns with the recommendation and prevents new collateral types from being enabled for restrictive pools.
<a name="issue-6-1-2"></a>
### 6.1.2 Collateral balances are incorrectly updated when calling `burnUsd`
*[IssueUSDModule.sol#L158](https://github.com/Synthetixio/synthetix-v3/blob/d5cc1c2daf5b988de8be20fd3915b2933db75189/protocol/synthetix/contracts/modules/core/IssueUSDModule.sol#L158)*
#### Description
The changes to `burnUsd` did not align with the specification provided and would cause accounts to have incorrect balances. The following issues were explicitly identified:
1. **Incorrect collateral reference**: The contract mistakenly interacts with `collateralType` instead of the intended `USD_TOKEN`.
2. **Burning tokens from the wrong address**: Instead of burning the sUSD tokens from the system, the tokens are burned from the `msg.sender`.
3. **Increasing collateral balance**: The user's collateral should be reduced instead of increased as the tokens are burned from the system, not the user.
#### Recommendation
The `burnUsd` function should be revised to mirror the implementation of `mintUsd`:
1. Tokens should be burned from the system, e.g. `address(this)`.
2. The account's `USD_TOKEN` collateral balance should be adjusted.
3. The collateral balance should be decreased and not increased.
To achieve the desired implementation, `USD_TOKEN` must be enabled as a system collateral type.
#### Update
The `burnUsd` function was reworked to match the specification and to mirror the implementation of `mintUsd`.
## 6.3 Medium risk
No medium risk findings were identified during the audit.
## 6.3 Low risk
<a name="issue-6-3-1"></a>
### 6.3.1 Incorrect precision when validating `liquidationRatioD18` and `issuanceRatioD18`
*[CollateralConfiguration.sol#L145](https://github.com/Synthetixio/synthetix-v3/blob/94bce32a43162edaaccd66db8990e5c5ad5f6613/protocol/synthetix/contracts/storage/CollateralConfiguration.sol#L145)*
#### Description
The validation checks added to the setter for `CollateralConfiguration` have the incorrect decimal precision to denote `100%`. Values are represented using 18 decimal unsigned integers instead of absolute percentages. The validation should also be inclusive of the value to match the error message.
The code snippet below is provided for reference:
```solidity
if (config.issuanceRatioD18 < 100) {
revert ParameterError.InvalidParameter("issuanceRatioD18", "must be greater than 100%");
}
if (config.liquidationRatioD18 < 100) {
revert ParameterError.InvalidParameter("liquidationRatioD18", "must be greater than 100%");
}
if (config.issuanceRatioD18 < config.liquidationRatioD18) {
revert ParameterError.InvalidParameter("issuanceRatioD18", "must be greater than liquidationRatioD18");
}
```
#### Recommendation
Instead of specifying the percentage, the values should be validated to be greater than `1 ether`.
#### Update
The validations were modified to make use of the correct threshold values.
<a name="issue-6-3-2"></a>
### 6.3.2 Undocumented breaking changes
*[IExternalNode.sol#L11](https://github.com/Synthetixio/synthetix-v3/blob/96f34e384b9fa6207a34373fbac56d75e8311ced/protocol/oracle-manager/contracts/interfaces/external/IExternalNode.sol#L11)*
#### Description
The `process` function signature of the `ExternalNode` interface has been modified. If `ExternalNodes` are already deployed using the previous function signature, this will be a breaking change for those nodes.
#### Recommendation
The SIP states explicitly that the change should not be a breaking change. To reflect the actual change, the SIP should be updated to note that no `ExternalNode` has been registered on-chain, and thus, a breaking change is acceptable.
#### Update
The SIP was updated to reflect the implementation.
<a name="issue-6-3-3"></a>
### 6.3.3 Incorrect event emitted
*[PoolModule.sol#L268](https://github.com/Synthetixio/synthetix-v3/blob/a80ab2d0dc204fe77803fc62d9ec86e0f0638985/protocol/synthetix/contracts/modules/core/PoolModule.sol#L268)*
#### Description
The `setPoolCollateralIssuanceRatio` function incorrectly emits `PoolCollateralDisabled` instead of its own or other suitable event.
Furthermore, including and especially indexing the `msg.sender` provides little benefit from a monitoring perspective. Suitable events are already emitted when changing the pool owner.
#### Recommendation
Instead of introducing a new event that matches the implementation of `setPoolCollateralIssuanceRatio`, existing events could be combined and reused. The following event signature would be more compact while still allowing sensitive owner actions to be effectively monitored:
```solidity
PoolCollateralUpdated(uint128 index poolId, address indexed collateral, uint256 issuanceRatioD18, bool enabled)
```
#### Update
Due to the overlap between SIP-326 and SIP-333, the setter functions were combined into a single function, `setPoolCollateralConfiguration`, which emitted a more suitable event.
<a name="issue-6-3-4"></a>
### 6.3.4 Mismatched array lengths
*[NodeModule.sol#L154](https://github.com/Synthetixio/synthetix-v3/blob/4a02874b70bbd7b6a8f9c121a21058b634e92374/protocol/oracle-manager/contracts/modules/NodeModule.sol#L154)*
#### Description
The process function's implementation in `MockExternalNode` assumes that the `runtimeKey` and `runtimeValues` arrays are of equal lengths, but this is not validated. Arrays of different lengths could cause reverts or the omission of runtime keys.
#### Recommendation
The `_process()` should incorporate a check to ensure that the `runtimeKeys` and `runtimeValues` arrays are always of equal length.
#### Update
Validation of the arrays' lengths was added as per the recommendation.
## 6.4 Informational
<a name="issue-6-4-1"></a>
### 6.4.1 Design comments
The following changes are recommended to improve the readability and maintainability of the code base:
- Standardize the configuration properties introduced in SIP-326 and SIP-333. SIP-333 introduced a `PoolCollateralConfiguration` struct, whereas SIP-326 introduced a new mapping for each configurable property.
- Apart from the disparity in naming between the `requireSufficientCollateralCapacity` function and the error it reverts with, namely `SurpassedPoolMaxCollateralDeposit`, the term `capacity` has an implied meaning within the context of the system. Consider renaming the function to `checkPoolCollateralLimit` and the error to `PoolCollateralLimitExceeded`.
- When calling `burnUsd`, the transaction will revert due to an underflow if the user’s sUSD collateral balance is less than the amount of debt being burned. A check should be added to `decreaseAvailableCollateral` and revert with a suitable error message.
- The `maxDepositD18` property should be more descriptive. The functionality restricts the total amount of collateral that can be delegated to a pool and the amount of collateral that can be deposited within the system. Alternative variable names such as `collateralLimitD18` or `maxTotalDelegatedD18` would be more suitable.
#### Update
The suggestions provided were incorporated in the final commit of the audit.
