Synthetix Aspidiske Release Smart Contract Audit

# 1. Introduction
iosiro was commissioned by [Synthetix](https://www.synthetix.io) to conduct a smart contract audit of their Aspidiske Release, which included [SIP-252](https://sips.synthetix.io/sips/sip-252)

The audit of SIP-252 was performed across the following periods:

* 15 July to 11 August 2022 with two auditors.
* 5 October to 18 October 2022 with two auditors.

A total of 29 resource days were used to audit across this period.

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 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 a smart contract audit performed by iosiro of Synthetix's Aspidiske release.

[SIP-252](https://sips.synthetix.io/sips/sip-252) introduced a mechanism to allow the liquidation of escrowed SNX in the system when a position's collateralization ratio is below the liquidation ratio threshold. The addition of escrow entry liquidations allows all collateral held by a staker to be liquidated, which further improves the system's ability to maintain its health. Escrow entries can only be liquidated by forced (i.e. non-self) liquidations if the transferrable SNX in a position is insufficient to increase their collateralization ratio to the requisite amount. Furthermore, the liquidator and flagger fee mechanism applied during forced liquidations was updated to improve the calculations of penalizations and in some cases close accounts that were previously left open.

Two high risk issues relating to accounts with Synthetix Escrow and flag removal logic were identified, as well as one medium risk and various other low risk and informational issues. At the end of the audit, the code was found to be of a high standard and conformed to the specification provided.

<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.2 Synthetix SIP-252 smart contracts
**Project Name:** Synthetix<br/>
**Initial commit:** [1678d32](https://github.com/Synthetixio/synthetix/pull/1825/commits/1678d32e7730a6767314bcdc6619d5e74f106997)<br/>
**Intermediary commit:** [73e7463](https://github.com/Synthetixio/synthetix/pull/1825/commits/73e746386c10043e58173203a821953a71a6558f)<br/>
**Final commit:** [159a5da](https://github.com/Synthetixio/synthetix/commit/159a5dad87a81da90f85b57f1c6a64f7b2c51c56)<br/>

**Files:** BaseRewardEscrowV2.sol, ImportableRewardEscrowV2.sol, BaseSynthetix.sol, Issuer.sol, Liquidator.sol, RewardEscrowV2.sol, RewardEscrowV2Storage.sol

## 3.2 Methodology

A variety of techniques were used in order to perform the audit. These techniques are briefly 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 to identify 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 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-252

The specification of SIP-252 was based on commit hash [10fae03](https://github.com/Synthetixio/SIPs/blob/10fae033df93516aacb0d95fc05e7dae93589f28/content/sips/sip-252.md).

<a name="section-5"></a>
# 5. Detailed findings
The following section includes in-depth descriptions of the findings of the audit.

## 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.

## 5.5 Closed

### 5.5.1 Potential issues for accounts with a Synthetix Escrow balance (high risk)
*[Issuer.sol#L783](https://github.com/iosiro/synthetix-audits/blob/67264e57cee93b49e50ad40c6bacf4d27578b0e1/contracts/Issuer.sol#L783)*

#### Description
The implementation neglects that a user could have SNX in the SynthetixEscrow contributing to their c-ratio. When paying out liquidator rewards, the transaction could fail, as the necessary amount of SNX is not liquid. As a result, accounts that fall into this category could be prevented from being liquidated. This issue is also present in the containing `if` statement.[Issuer.sol#L779](https://github.com/iosiro/synthetix-audits/blob/67264e57cee93b49e50ad40c6bacf4d27578b0e1/contracts/Issuer.sol#L779). Furthermore, liquidating an account like this could result in all of its debt being removed despite having additional funds available. One solution would be to perform a partial liquidation in this case, e.g. calculate how much debt was actually paid off and then return that. The problem is only relevant to L1 as L2 does not have escrowed entries in this contract. Around 723,907 SNX is currently unclaimed.

#### Remedial Action
The `_collateral` function should be updated to no longer include the Synthetix Escrow balance held by accounts as a form of collateral.

#### Update
The suggestion was implemented in [f4d754c](https://github.com/Synthetixio/synthetix/pull/1908/commits/f4d754cede9d3eba6095c5029c1b33b76369985e#diff-42e8646281c6615c6c2862c05b1230947b7a701cb84c2219eb0f5eb7f3fdb974R345).

### 5.5.2 Incorrect flag removal logic (high risk)
*[Issuer.sol#L721](https://github.com/iosiro/synthetix-audits/blob/67264e57cee93b49e50ad40c6bacf4d27578b0e1/contracts/Issuer.sol#L721)*

#### Description
As partial liquidations are possible with self-liquidation (and potentially forced liquidations if the change is implemented), the collateralization ratio of the position should be checked before removing the flag. The risk with this issue is that a user could continuously remove any liquidation flag set on their account by self-liquidating any SNX position (i.e. a non-zero value of `debtToRemove`) that will then reset the flag timer, preventing the account from being liquidated.

#### Remedial Action
Flag removal should be limited to only forced liquidations.

#### Update
The suggestion was implemented in [bdad565](https://github.com/Synthetixio/synthetix/pull/1908/commits/bdad565ca599f15170397571880c41ef2efc7444#diff-42e8646281c6615c6c2862c05b1230947b7a701cb84c2219eb0f5eb7f3fdb974R713).

### 5.5.3 Potential blocking case for liquidations (medium risk)
*[BaseSynthetix.sol#L392](https://github.com/iosiro/synthetix-audits/blob/67264e57cee93b49e50ad40c6bacf4d27578b0e1/contracts/BaseSynthetix.sol#L392)*

#### Description
A liquidation will revert if the liquidated account has exactly enough collateral to pay the liquidator rewards, e.g. `_collateral(account) = (rewardsSum)` [Issuer.sol#L788](https://github.com/iosiro/synthetix-audits/blob/67264e57cee93b49e50ad40c6bacf4d27578b0e1/contracts/Issuer.sol#L788), or when liquidating an account with all of its collateral in the rewards escrow.

#### Remedial Action
The revert should be replaced by an if statement around the liquidator rewards transfer logic and call to `notifyRewardAmount`.

#### Update
The suggestion was implemented in [2fdcddf](https://github.com/Synthetixio/synthetix/pull/1908/commits/2fdcddf679695ae37f9679af40aa2b7c209792de#diff-a1a22e5bc9662603174b49c6f186d70298df0371555e15e9d113394418bb9dcaR405).

### 5.5.4 Overcorrection for self-liquidations (medium)
*[Issuer.sol#L751](https://github.com/iosiro/synthetix-audits/blob/67264e57cee93b49e50ad40c6bacf4d27578b0e1/contracts/Issuer.sol#L751)*

#### Description
Using `balanceOf` instead of the `_collateral` function results in self-liquidations overcorrecting if there are escrowed entries.

#### Remedial Action
The function should use the `_collateral` function to ensure that the target collateralization ratio is reached after a self-liquidation.

### 5.5.5 Liquidator fee not paid when collateral is insufficient (low risk)
*[BaseSynthetix.sol#L399](https://github.com/Synthetixio/synthetix/blob/d95ebe9713eeb9ed71e8c1983d60f9e80ef446fc/contracts/BaseSynthetix.sol#L399)*

#### Description
When the liquidated account has a liquidatable collateral amount equal to the sum of the liquidator and flagger fees, the liquidated collateral is paid as staker rewards and not to the flagger and liquidator. This is not in line with the [SIP-252](https://sips.synthetix.io/sips/sip-252/) specification, which states that in such a case, the collateral should be paid to the liquidator and flagger.

#### Remedial Action
The `totalRedeemed > flagReward.add(liquidateReward)` condition should rather use `>=` to ensure that the liquidator and flagger fees are paid when their sum is equal to the redeemed collateral amount.

#### Update
This issue was resolved in [c2dc94a](https://github.com/Synthetixio/synthetix/pull/1825/commits/c2dc94a008e2160fc86f10542c5ad582dc727979#diff-a1a22e5bc9662603174b49c6f186d70298df0371555e15e9d113394418bb9dcaR399).

### 5.5.6 Incorrect liquidation penalty returned by helper function (informational)
*[Liquidator.sol#L97](https://github.com/Synthetixio/synthetix/blob/66615b3c1a724f5442adae3b34c8905ebbebee99/contracts/Liquidator.sol#L97)*

#### Description
The `liquidationPenalty()` helper in `Liquidator.sol` uses `getLiquidationPenalty()` to load the penalty value from `FlexibleStorage.sol` which returns a different value than `getSnxLiquidationPenalty()` used by `Issuer.sol` when performing a liquidation. The helper function is not used during the liquidation process, so this difference has no risk to the system.

#### Remedial Action

The `liquidationPenalty()` helper function should be updated to use the `getSnxLiquidationPenalty()` function instead to ensure the correct value is provided to callers attempting to perform liquidation calculations off-chain.

#### Update
This issue was resolved in [9bdc006](https://github.com/Synthetixio/synthetix/pull/1825/commits/9bdc0066bd430fadd10865a5468b6819d39c93f6).

‍

Disclaimer