Synthetix Caph Release Smart Contract Audit
# 1. Introduction
iosiro was commissioned by [Synthetix](https://www.synthetix.io) to conduct a smart contract audit of their Caph Release, which included [SIP-2004](https://sips.synthetix.io/sips/sip-2004) and [SIP-2005](https://sips.synthetix.io/sips/sip-2005). Two auditors performed the audit between 22 March 2023 and 6 April 2023, consuming 24 audit 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 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 Caph release.
[SIP-2004](https://sips.synthetix.io/sips/sip-2004) implemented several changes that improved the trader experience using Synthetix Perps v2. Changes included introducing a delayed order to close the caller's position, removing the commit fee for delayed orders, making the liquidation buffer market specific, and improving the slippage protection control.
Two medium risk and several informational issues were identified related to these changes. The medium risk issues included:
* Under certain circumstances, opening a delayed order for closing a position could be performed on the wrong account. This was resolved suitably prior to the completion of the audit.
* One parameter in functions relating to delayed orders changed from being a slippage control specified as a percentage to an absolute value for an acceptable price. As both values were of type `uint`, the function signature remained the same. As a result, integrators and bots could call the function with incorrect values and perform undesirable trades. The team accepted the risk, as the main platforms integrating with Perps V2 had been notified of the change. All informational issues were resolved upon the completion of the audit.
[SIP-2005](https://sips.synthetix.io/sips/sip-2005) mitigated the potential to sandwich attack liquidations. It used a two-stage liquidation process, namely flagging and liquidation. Liquidation could be spontaneous if the position was small enough and the market had an acceptable skew. If those conditions were not met, liquidations could be forced by an "endorsed address", a new special role in the system. Endorsed addresses did not receive a reward for the liquidation and were intended to be protocol-controlled addresses used to perform liquidations safely.
Two low risk and several informational issues were identified with these specific changes. Both low risk issues were deemed acceptable by the team, as the endorsed accounts are intended to mitigate against such issues. Should the endorsed account be unable to fulfill their responsibility, stakers may be at risk. All informational issues were suitably resolved upon 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 Synthetix SIP-2004 and SIP-2005 smart contracts
**Project Name:** Synthetix<br/>
**Final Commit:** [098b7f5](https://github.com/Synthetixio/synthetix/commit/098b7f58a65fab5c2608d1d7e9c8bd56fdcc50d3)<br/>
**Files:** EmptyFuturesMarketManager.sol, FuturesMarketManager.sol, MixinPerpsV2MarketSettings.sol, PerpsV2ExchangeRate.sol, PerpsV2Market.sol, PerpsV2MarketBase.sol, PerpsV2MarketData.sol, PerpsV2MarketDelayedExecution.sol, PerpsV2MarketDelayedIntent.sol, PerpsV2MarketDelayedOrders.sol, PerpsV2MarketDelayedOrdersBase.sol, PerpsV2MarketDelayedOrdersOffchain.sol, PerpsV2MarketLiquidate.sol, PerpsV2MarketProxyable.sol, PerpsV2MarketSettings.sol, PerpsV2MarketState.sol, PerpsV2MarketStateLegacyR1.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.
#### SIP-2004 and SIP-2005 Test Coverage
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/ | 100 | 84.81 | 93.16 | 96.22 | |
EmptyFuturesMarketManager.sol | 100 | 100 | 0 | 0 |... 71,72,76,80 |
FuturesMarketManager.sol | 100 | 93.33 | 100 | 100 | |
MixinPerpsV2MarketSettings.sol | 100 | 100 | 100 | 100 | |
PerpsV2ExchangeRate.sol | 100 | 75 | 100 | 100 | |
PerpsV2Market.sol | 100 | 100 | 100 | 100 | |
PerpsV2MarketBase.sol | 100 | 95.83 | 100 | 98.8 | 315,570 |
PerpsV2MarketData.sol | 100 | 100 | 100 | 100 | |
PerpsV2MarketDelayedExecution.sol | 100 | 86.36 | 100 | 98.46 | 265 |
PerpsV2MarketDelayedIntent.sol | 100 | 87.5 | 100 | 100 | |
PerpsV2MarketLiquidate.sol | 100 | 100 | 100 | 100 | |
PerpsV2MarketProxyable.sol | 100 | 90 | 100 | 98.73 | 344 |
PerpsV2MarketSettings.sol | 100 | 89.29 | 93.44 | 96.4 | 95,144,151,179 |
PerpsV2MarketState.sol | 100 | 76.47 | 93.55 | 92.68 |... 397,409,412 |
PerpsV2MarketStateLegacyR1.sol | 100 | 50 | 61.9 | 68.75 |... 145,153,157 |
Test coverage should be improved to complete coverage of the uncovered lines.
## 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-2004
The specification of SIP-2004 was based on commit hash [4b9b0dc](https://github.com/Synthetixio/SIPs/blob/dd44f3e760fd8a84073db6e0e59fe435d909dae1/content/sips/sip-2004.md).
## 4.2 SIP-2005
The specification of SIP-2005 was based on commit hash [d4973d4](https://github.com/Synthetixio/SIPs/blob/dd44f3e760fd8a84073db6e0e59fe435d909dae1/content/sips/sip-2005.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 Dangerous redefinition of order variable (medium-risk)
*[PerpsV2MarketProxyable.sol#L104](https://github.com/Synthetixio/synthetix/blob/2129ac7b1f27f33f91779bcda6f01e5a97310827/contracts/PerpsV2MarketProxyable.sol#L104)*
#### Description
Previously, when submitting an order, the order would include a `priceImpactDelta` parameter, specified as a percentage value between 0 and 1, which effectively indicated the slippage in price that would be acceptable to the user. The behavior of this functionality has been changed to the user specifying `desiredFillPrice`, which is an absolute value for a price deemed to be acceptable. As both of these variables are of type `uint` the function signature will remain the same. Integrators with the system, such as other DeFi platforms or bot traders, risk specifying incorrect values for this parameter, resulting in either undesirable prices or transaction reverts. Any legacy orders affected by this and that cannot be executed will become canceled, and lose their `commitFee`.
#### Recommendation
It is recommended that orders placed with a `desiredFillPrice` use a new function signature and that the legacy logic is retained to fulfill any outstanding orders until it can be fully deprecated.
#### Update
The risk has been accepted due to the Synthetix team directly communicating with the main integrators of the system, such as Polynomial and Lyra.
### 5.5.2 Missing `onlyProxy` modifier (medium-risk)
*[PerpsV2MarketDelayedIntent.sol#L36](https://github.com/Synthetixio/synthetix/blob/2129ac7b1f27f33f91779bcda6f01e5a97310827/contracts/PerpsV2MarketDelayedIntent.sol#L36), [PerpsV2MarketDelayedIntent.sol#L40](https://github.com/Synthetixio/synthetix/blob/2129ac7b1f27f33f91779bcda6f01e5a97310827/contracts/PerpsV2MarketDelayedIntent.sol#L40)*
#### Description
The `onlyProxy` modifier was not used on the `submitCloseOffchainDelayedOrderWithTracking()` and `submitCloseDelayedOrderWithTracking()` functions. This could result in closing incorrect positions as the `messageSender` can be influenced by bypassing the proxy implementation.
#### Recommendation
A simple solution would consist of adding the `onlyProxy notFlagged(messageSender)` modifiers to the `_submitCloseDelayedOrder()` internal function as it is required by all of the relevant external functions.
#### Update
Fixed in [07722aa](https://github.com/Synthetixio/synthetix/pull/1994/commits/07722aa10af6e18fd13de166355766d2580008d7#).
### 5.5.3 Delay during liquidation could put stakers at risk of paying liquidation fees (low-risk)
*[PerpsV2MarketLiquidate.sol#L213](https://github.com/Synthetixio/synthetix/blob/2129ac7b1f27f33f91779bcda6f01e5a97310827/contracts/PerpsV2MarketLiquidate.sol#L213)*
#### Description
Flagged positions not available for spontaneous liquidation will encounter a delay between flagging and liquidation. During this delay, the position’s margin could decrease below the liquidation margin and require stakers to pay the associated fees to the flagger and liquidator.
#### Recommendation
An alternative approach would be to cap the flagger fee to the margin amount minus the liquidator fee.
Endorsed accounts should limit this risk as they are intended to operate continuously. However, it should be noted that should the endorsed accounts be unable to fulfill their responsibility, stakers in the system will be at risk.
#### Update
The Synthetix team acknowledged the behavior and accepted the risk.
### 5.5.4 Small liquidations could be accumulated and sandwiched by the liquidator (low-risk)
*[PerpsV2MarketLiquidate.sol#L125](https://github.com/Synthetixio/synthetix/blob/2129ac7b1f27f33f91779bcda6f01e5a97310827/contracts/PerpsV2MarketLiquidate.sol#L125)*
#### Description
SIP-2005 may be vulnerable to unexpected manipulation. Max Liquidation Delta (MLD) is per user, while Max Premium/Discount (MPD) is market-based. This means that under certain market conditions, a large PD could block all liquidations for a market, accumulating until the PD lowers, at which point all flagged positions can be liquidated at the same time. Furthermore, MPD is an effective control if liquidations are on the opposite side to the skew (e.g. market skewed short, long positions being liquidated). However, suppose the liquidations happen on the same side as the skew (e.g. market skewed short, short positions being liquidated). In that case, the skew is reduced after each liquidation, so there would be greater potential for potential exploitation. For example, consider a market skewed short, with many small short position liquidations blocked by MPD:
1. liquidator opens long (making skew slightly smaller).
2. liquidate short position (making skew smaller).
3. liquidator closes long and profits from PD (making skew slightly bigger again).
4. repeat (1).
In this scenario, a liquidator would theoretically be able to perform cascading atomic liquidations as long as the liquidations happened in the opposite direction to the skew.
#### Recommendation
To limit the effect of cascading or accumulated liquidations, an accumulator could be used to track and throttle the total amount liquidated in a single block or time period.
#### Update
The Synthetix team acknowledged the behavior and accepted the risk. Endorsed accounts are intended to mitigate the likelihood of scenarios such as this, and order fees should limit the profitability of such an event.
### 5.5.5 Design comments (informational)
#### Missing close order function without tracking
There are no functions to submit a delayed order for closing a position without a tracking code.
#### Update
The function has not been added to reduce name pollution and reduce code size. An empty tracking code has the same effect.
#### Gas savings and code optimizations
1. [FuturesMarketManager.sol#L444](https://github.com/Synthetixio/synthetix/blob/2129ac7b1f27f33f91779bcda6f01e5a97310827/contracts/FuturesMarketManager.sol#L444) Adding addresses to the set is idempotent, rendering the check if the address is already added in the linked contract unnecessary.
2. [PerpsV2MarketDelayedExecution.sol#L311](https://github.com/Synthetixio/synthetix/blob/2129ac7b1f27f33f91779bcda6f01e5a97310827/contracts/PerpsV2MarketDelayedExecution.sol#L311) Only calling `_updatePositionMargin()` if `toRefund > 0` would prevent calling the function unnecessarily and save gas.
3. [PerpsV2MarketDelayedExecution.sol#L237](https://github.com/Synthetixio/synthetix/blob/2129ac7b1f27f33f91779bcda6f01e5a97310827/contracts/PerpsV2MarketDelayedExecution.sol#L237), [PerpsV2MarketDelayedExecution.sol#L281](https://github.com/Synthetixio/synthetix/blob/2129ac7b1f27f33f91779bcda6f01e5a97310827/contracts/PerpsV2MarketDelayedExecution.sol#L281) To save gas and reduce code complexity, the `notFlagged(account)` modifier could be removed from the `_cancelDelayedOrder()` and `_executeDelayedOrder()` functions as delayed orders for a flagged account are deleted.
4. [PerpsV2MarketLiquidate.sol#L157](https://github.com/Synthetixio/synthetix/blob/2129ac7b1f27f33f91779bcda6f01e5a97310827/contracts/PerpsV2MarketLiquidate.sol#L157) The position is loaded into memory twice during the flagging of an account, the second instance can be removed.
#### Update
1. Fixed in [07722aa](https://github.com/Synthetixio/synthetix/pull/1994/commits/07722aa10af6e18fd13de166355766d2580008d7#diff-d92292c1de0c1b6c0561950cee640248293d86c5e0a942565ecf3277b43a7e42L444).
2. Fixed in [07722aa](https://github.com/Synthetixio/synthetix/pull/1994/commits/07722aa10af6e18fd13de166355766d2580008d7#diff-89092ba91e86d3b46d91d79586941e116fbb9e41a914a79749b1249761d05338R311).
3. Fixed in [07722aa](https://github.com/Synthetixio/synthetix/pull/1994/commits/07722aa10af6e18fd13de166355766d2580008d7#).
4. Fixed in [07722aa](https://github.com/Synthetixio/synthetix/pull/1994/commits/07722aa10af6e18fd13de166355766d2580008d7#diff-812f0300cb7c3cd7034d8541845a3de5700f859d5bb029690ee0a5786622dd09L157).
#### Link state directly from constructor
*[PerpsV2MarketState.sol](https://github.com/Synthetixio/synthetix/blob/2129ac7b1f27f33f91779bcda6f01e5a97310827/contracts/PerpsV2MarketState.sol)*
In the constructor, it would be possible to query if `manager.marketForKey()` is non-zero and then get the state address from `manager.marketForKey().marketState,` instead of passing in `_legacyState` to determine if there’s a legacy state for that market key.
#### Update
Synthetix team responded that it is intentionally done that way to validate the state is being linked. Comments were added to make the decision explicit in [07722aa](https://github.com/Synthetixio/synthetix/pull/1994/commits/07722aa10af6e18fd13de166355766d2580008d7#diff-f3c7219e89c2ee76dc32e9f31e6a0ba324558c2dcadd623dab2e7e03649d6e20R104).
#### Initialize state from constructor
*[PerpsV2MarketState.sol](https://github.com/Synthetixio/synthetix/blob/2129ac7b1f27f33f91779bcda6f01e5a97310827/contracts/PerpsV2MarketState.sol)*
The `linkOrInitializeState` could be made an internal function and called directly from the constructor, which would avoid the need to use the `onlyIfInitialized` modifier.
#### Update
Synthetix team responded that it is intentionally done that way to separate contract deployment from configuration and linking. Added comments to make the decision explicit in [07722aa](https://github.com/Synthetixio/synthetix/pull/1994/commits/07722aa10af6e18fd13de166355766d2580008d7#diff-f3c7219e89c2ee76dc32e9f31e6a0ba324558c2dcadd623dab2e7e03649d6e20R113).
#### View functions do not consider the legacy state
*[PerpsV2MarketState.sol](https://github.com/Synthetixio/synthetix/blob/2129ac7b1f27f33f91779bcda6f01e5a97310827/contracts/PerpsV2MarketState.sol)*
The `getPositionAddressesPage`, `getDelayedOrderAddressesPage`, `getPositionAddressesLength` only return correct values for the newly deployed state contracts, without consideration for any state in the legacy state of a market.
#### Update
Synthetix team responded that helper functions are for migration and analytics for the isolated contract. Comments were added to make the decision explicit in [07722aa](https://github.com/Synthetixio/synthetix/pull/1994/commits/07722aa10af6e18fd13de166355766d2580008d7#diff-f3c7219e89c2ee76dc32e9f31e6a0ba324558c2dcadd623dab2e7e03649d6e20R216).
