Synthetix Naos Release Smart Contract Audit
# 1. Introduction
iosiro was commissioned by Synthetix to conduct a smart contract audit of their Naos Release, which included [SIP-237](https://sips.synthetix.io/sips/sip-237/). The audit was performed by two auditors between 27 February and 08 March 2023, using 10 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 - Detailed findings:](#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.
<a name="section-2"></a>
# 2. Executive summary
This report presents the findings of the smart contract audit performed by iosiro of Synthetix's Naos Release.
[SIP-237](https://sips.synthetix.io/sips/sip-237/) introduced a mechanism stakers could use to migrate their collateral and debt shares to the Optimism deployment of Synthetix, while leaving their synths on the Mainnet deployment. As a result, stakers could migrate their staking positions to Optimism without reducing synth liquidity on Ethereum.
Several high-risk issues were identified during the audit: a missing token transfer approval, liquid SNX becoming trapped on the Optimism migrator contract, and not tracking debt in-flight between chains. Several low-risk and informational issues were also raised during the audit.
All of the high-risk issues were sufficiently addressed by the conclusion of the audit.
<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 the purposes of this audit.
### 3.1.1 Smart contracts
* **Project name:** Synthetix </br>
* **Initial commit:** [3e2c78a](https://github.com/Synthetixio/synthetix/pull/1896/commits/3e2c78a29371ac7ba3ee01cdbe31119503d0fa59) </br>
* **Final commit:** [62dfb57](https://github.com/Synthetixio/synthetix/pull/2043/commits/62dfb575f30bf29c247d89e89e673633cf9c5bf1) </br>
* **Files:** BaseDebtMigrator.sol, BaseSynthetix.sol, DebtMigratorOnEthereum.sol, DebtMigratorOnOptimism.sol, Issuer.sol
## 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 |
-----------------------------------------|----------|----------|----------|----------|----------------|
BaseDebtMigrator.sol | 100 | 100 | 100 | 100 | |
BaseSynthetix.sol | 100 | 77.27 | 100 | 98.6 | 180,458 |
DebtMigratorOnEthereum.sol | 100 | 69.23 | 100 | 100 | |
DebtMigratorOnOptimism.sol | 100 | 66.67 | 100 | 100 | |
Issuer.sol | 100 | 90.38 | 98.88 | 99.01 | 372,914,992 |
The unit and integration tests covered code changes for this release with basic test cases to ensure its functionality was inline with the specification.
### 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.
<a name="section-4"></a>
# 4. Design specification
The following section outlines the intended functionality of the system at a high level.
## 4.1 SIP-237
The specification of SIP-237 was based on commit hash [93031e9](https://github.com/Synthetixio/SIPs/blob/93031e91b2f48270b8303e9727ff05dd71812561/content/sips/sip-237.md).
<a name="section-5"></a>
# 5. Detailed findings
The following section details the findings of the audit.
## 5.1 High risk
No identified high-risk issues were open at the conclusion of the review.
## 5.2 Medium risk
No identified medium-risk issues were open at the conclusion of the review.
## 5.3 Low risk
### 5.3.1 Accounts flagged for liquidation could get stuck on Mainnet
*[DebtMigratorOnEthereum.sol#L132](https://github.com/Synthetixio/synthetix/blob/3e2c78a29371ac7ba3ee01cdbe31119503d0fa59/contracts/DebtMigratorOnEthereum.sol#L132)*
#### Description
The debt migration contracts did not allow accounts flagged for liquidation to be migrated. Once Synthetix v3 Legacy Market is deployed and liquidations on v2 are disabled, these accounts will be stuck until the position is corrected and the flag is removed by calling `checkAndRemoveAccountInLiquidation()`.
#### Recommendation
Before deploying Synthetix v3 Legacy Market, a migration path for flagged accounts on Mainnet should be created to forcefully migrate flagged accounts to v3 on Optimism for liquidation.
## 5.4 Informational
### 5.4.1 Migrated escrow entries subject to arbitrary 26-week period
*[DebtMigratorOnEthereum.sol#L174](https://github.com/Synthetixio/synthetix/blob/3e2c78a29371ac7ba3ee01cdbe31119503d0fa59/contracts/DebtMigratorOnEthereum.sol#L174)*
#### Description
All escrow entries migrated for an account are combined into a single entry on Optimism with a fixed duration of 26 weeks. This approach will punish stakers whose entries have already or are soon to vest, as these entries will then be locked for a further 26 weeks. Conversely, it was beneficial to new stakers whose entries were closer to the ordinary one year vesting period, as they would have access to the collateral after a shorter duration.
#### Recommendation
Instead of using a fixed duration, an alternative approach would be to calculate the rate of release for each escrow entry (`amount / time`), and calculate an average across all entries. This would result in the same net rate of release across all entries for an account. In order to implement it, changes to `revokeFrom()` and `setZeroAmountUntilTarget()` would be required.
Additionally, a custom `vest()` function could be implemented to allow the migrator to vest all vested entries on behalf of the account to prevent an account from relocking already vested tokens. Alternatively, if no change is deemed necessary, the front-end should determine whether an account has any claimable entries before migrating.
## 5.5 Closed
### 5.5.1 Missing `approve()` for `createEscrowEntry()` on `DebtMigratorOnOptimism` (high risk)
*[DebtMigratorOnOptimism.sol#L51](https://github.com/Synthetixio/synthetix/blob/3e2c78a29371ac7ba3ee01cdbe31119503d0fa59/contracts/DebtMigratorOnOptimism.sol#L51)*
#### Description
Once the migration message had been passed from Mainnet to Optimism, the `DebtMigratorOnOptimism` contract attempted to recreate the migrated account on Optimism. Recreating an account required the use of `createEscrowEntry()` to create an escrow entry for the migrated escrowed SNX, which used `transferFrom()`. Since the `DebtMigratorOnOptimism` never approved `RewardEscrowV2` to transfer the escrowed SNX on its behalf, the call would revert, and the migration would fail.
#### Recommendation
The `DebtMigratorOnOptimism` contract should approve the `RewardEscrowV2` contract to use the SNX required to create the escrow entry.
#### Update
The Synthetix team implemented the recommendation in [dda5042](https://github.com/Synthetixio/synthetix/pull/1896/commits/dda5042763fed865c557ab3d85ad67a9cc855ed4).
### 5.5.2 Staker's liquid SNX unallocated (high risk)
*[DebtMigratorOnOptimism.sol#L89](https://github.com/Synthetixio/synthetix/blob/3e2c78a29371ac7ba3ee01cdbe31119503d0fa59/contracts/DebtMigratorOnOptimism.sol#L89)*
#### Description
During migration, an account's liquid SNX was moved into the bridge escrow but was not allocated to the account on Optimism. The debt shares and escrow were correctly allocated to the account, but the unallocated SNX remained in the `DebtMigratorOnOptimism` contract. Furthermore, in the case of an account that held most of its collateral as liquid SNX, the account would likely be at risk of liquidation on Optimism after migrating.
#### Recommendation
Any liquid SNX migrated by an account should be transferred out of the `DebtMigratorOnOptimism` contract at the end of the migration process.
#### Update
The recommendation was implemented in [dda5042](https://github.com/Synthetixio/synthetix/pull/1896/commits/dda5042763fed865c557ab3d85ad67a9cc855ed4).
### 5.5.3 In-flight debt shares not tracked (high risk)
*[DebtMigratorOnEthereum.sol#L141](https://github.com/Synthetixio/synthetix/blob/3e2c78a29371ac7ba3ee01cdbe31119503d0fa59/contracts/DebtMigratorOnEthereum.sol#L141)*
#### Description
In order to migrate an account, the account's debt shares on Mainnet are burnt, and upon finalization, the shares are re-minted on Optimism. During this time these shares are removed from the total supply of debt shares on the protocol. Since the debt on the protocol remains the same, there is a temporary inflation in the debt value per share on Mainnet. For example, if a whale (or many accounts in a short time) were to migrate, the debt value per share would temporarily be higher on Ethereum, making accounts more susceptible to liquidation.
#### Recommendation
As in the approach used in the synth bridge, the amount of debt shares sent from Mainnet and the amount received on Optimism should be tracked to account for debt shares in-flight. The debt oracle could then use these amounts to correct for any debt in transit.
#### Update
The recommendation was implemented in [5278fce](https://github.com/Synthetixio/synthetix/pull/1896/commits/5278fce3db54859bd24d5f90347775561dd717c1).
### 5.5.4 Only accounts with an escrow amount are allowed to migrate (low risk)
*[DebtMigratorOnEthereum.sol#L146](https://github.com/Synthetixio/synthetix/blob/3e2c78a29371ac7ba3ee01cdbe31119503d0fa59/contracts/DebtMigratorOnEthereum.sol#L146)*
#### Description
Since the migration process on Optimism always attempted to create an escrow entry for the migrated account, the process could fail if the account did not have an escrow balance. As a result, the migrator on Mainnet prohibited the migration of accounts without an escrow balance. This was unnecessary, as accounts with no escrow balance could legitimately seek to migrate their debt across to Optimism.
#### Recommendation
Instead of reverting when attempting to migrate an account with no escrow balance from Mainnet, an `if` should be used on Optimism to check if the `escrow amount > 0` before creating the entry.
#### Update
The recommendation was implemented in [dda5042](https://github.com/Synthetixio/synthetix/pull/1896/commits/dda5042763fed865c557ab3d85ad67a9cc855ed4).
