Synthetix Acrux Release Smart Contract Audit
# 1. Introduction
iosiro was commissioned by [Synthetix](https://www.synthetix.io) to conduct a smart contract audit on the Acrux release, including [SIP-48](https://sips.synthetix.io/sips/sip-48) and [SIP-53](https://sips.synthetix.io/sips/sip-53). The audit was performed between 10 and 26 June.
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 risk exposure of the smart contracts, and as a guide to improving the security posture of the smart contracts by remediating the issues that were identified. The results of this audit are only a reflection of the source code reviewed at the time of the audit and of the source code that was determined to be in-scope.
The purpose of this audit was to achieve the following:
* Ensure that the smart contracts functioned as intended.
* Identify potential security flaws.
Assessing the market effect, economics, game theory, or underlying business model of the platform were strictly beyond 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 very high risk with regards to 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. There are a number of techniques that can help to achieve this, some of which are described below.
* Security should be integrated into the development lifecycle.
* Defensive programming should be employed to account for unforeseen circumstances.
* Current best practices should be followed when possible.
<a name="section-2"></a>
# 2. Executive Summary
This report presents the findings of the audit performed by iosiro on the smart contract implementation of the [Acrux release](https://blog.synthetix.io/the-acrux-release/).
#### SIP-48
The purpose of SIP-48 was to ensure that all view functions returned a value, even when the supplied currency rate was stale. A rate was considered stale if it was not updated recently enough. Previously, view functions would revert rather than returning a stale rate, which was problematic for dApps calling and displaying the value.
One low risk issue and several informational issues were identified during the audit. By the conclusion of the audit, all of the issues had been addressed. Overall, the implementation was of a high standard and accorded with the specification provided.
#### SIP-53
SIP-53 introduced the creation of new markets for trading binary options. Binary options trading allows users to bid on a certain price outcome of a given synth during some given timeframe.
Several issues were identified, including one high risk vulnerability. By the conclusion of this audit, all of the issues had been addressed. The implementation accorded with its specification and was of a high standard.
<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 is considered to be out-of-scope. Out-of-scope code that interacts with in-scope code is assumed to function as intended and introduce no functional or security vulnerabilities for the purposes of this audit.
### 3.1.1 Synthetix SIP-48 Smart Contracts
**Project Name:** Synthetix<br/>
**Commits:** [1ed6657](https://github.com/Synthetixio/synthetix/commit/1ed6657a4af2e80d0fcc844ce4e381831ef7b931)<br/>
**Files:** AddressResolver.sol, EtherCollateral.sol, ExchangeRates.sol, Exchanger.sol, FeePool.sol, Issuer.sol, PurgeableSynth.sol, Synthetix.sol
### 3.1.2 Synthetix SIP-53 Smart Contracts
**Project Name:** Synthetix<br/>
**Commits:** [8169a2e](https://github.com/Synthetixio/synthetix/pull/537/commits/8169a2ee452b060178455cb6369ac59363179c42)<br/>
**Files:** AddressListLib.sol, BinaryOption.sol, BinaryOptionMarket.sol, BinaryOptionMarketFactory.sol, BinaryOptionMarketManager.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 Ganache test environment, both manually and through the test suite provided. Manual analysis was used to confirm that the code operated at a functional level, and to verify the exploitability of any potential security issues identified.
### 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. The static analysis results were manually analyzed to remove false-positive results. True positive results would be indicated in this report. Static analysis tools commonly used include Slither, Securify, and MythX. Tools such as the Remix IDE, compilation output, and linters are also used to identify potential issues.
## 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 addressed to a satisfactory level to remove the risk that 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-48
The specification of SIP-48 was based on commit hash [dfebaee](https://github.com/Synthetixio/SIPs/blob/dfebaeeb335025636de920104f71af8cf09f7ecf/SIPS/sip-48.md).
## 4.2 SIP-53
The specification of SIP-53 was based on commit hash [84dc677](https://github.com/Synthetixio/SIPs/blob/84dc677d5e04476c0a985566c2a5d56266edc8c2/SIPS/sip-53.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 were present at the conclusion of the audit.
## 5.2 Medium Risk
No medium risk issues were present at the conclusion of the audit.
## 5.3 Low Risk
No low risk issues were present at the conclusion of the audit.
## 5.4 Informational
No informational issues were present at the conclusion of the audit.
## 5.5 Closed
### 5.5.1 Deposited Balance Vulnerable To Theft (High Risk)
*SIP-53: [BinaryOption.sol#L66](https://github.com/Synthetixio/synthetix/blob/e1c118b55e8ac6c87b417b0adaf90d56352bd413/contracts/BinaryOption.sol#L66)*
#### Description
In specific cases, there was a window in which an attacker could intercept a large majority of claimed options during the trading period. These options could then be exercised to receive the associated sUSD.
This vulnerability relied on rounding errors from bids so that the `exercisableDeposits` amount was slightly less than the `totalSupply` of options. By bidding a sufficiently small bid, say `smallBid`, such that `exercisableDeposits + smallBid < totalSupply`, the attacker could claim an amount of options equal to `exercisableDeposits`. This exploit relied on the attacker with the `smallBid` to be the last claimant to claim their options and the first to exercise their option to ensure that they were awarded the entire pot.
Additionally, when expanding the ternary operation, it was found that the `true` case did not have test coverage.
#### Remedial Action
It is recommended to enforce a minimum bid amount. The amount should be considered sufficiently high to ensure that trying to compound rounding errors to have `exercisableDeposits + minBid < _totalSupply` will be infeasible. It is also recommended to validate the amount a user is owed according to their bids and not the remaining pot.
It is recommended that a test case be added to properly cover the ternary operation.
##### Update
Implemented in [5e61120](https://github.com/Synthetixio/synthetix/commit/5e61120a9be6a8f55b0c5953aa0a850564c75ee7#diff-3bb0999eedef5614bddfd3aece58b08a), [714312a](https://github.com/Synthetixio/synthetix/commit/714312a29ba28d8bfcd843bd69c2cf774b20e901#diff-3bb0999eedef5614bddfd3aece58b08a) and [96875ec](https://github.com/Synthetixio/synthetix/commit/96875ecb1279154bd9b26021113c81805f0758bc#diff-3bb0999eedef5614bddfd3aece58b08a). The ternary operation was removed and thus no test coverage was necessary.
### 5.5.2 Permissive Refund Limit For Market Creators (Medium)
*SIP-53: [BinaryOptionMarket.sol#L427](https://github.com/Synthetixio/synthetix/blob/8169a2ee452b060178455cb6369ac59363179c42/contracts/BinaryOptionMarket.sol#L427)*
#### Description
Market creators were prohibited from fully refunding either their long or short bids, as long as the total capital was above the minimum requirement. However, this still allowed a market creator to leave just more than 0 bids. As such, the control was not effective at preventing a market creator from fully leaving a position.
#### Remedial Action
As the limit was in place to ensure that the creator maintained a reasonable amount of bids on either side of the market, it is recommended that the limit be relative to their entire position.
##### Update
Implemented in [f0de9d3](https://github.com/Synthetixio/synthetix/pull/537/commits/f0de9d30b848061049ba93bbd69ad1b15c299bab).
### 5.5.3 Missing staleness check (Low Risk)
*SIP-48: [Synth.sol#L139](https://github.com/Synthetixio/synthetix/blob/1ed6657a4af2e80d0fcc844ce4e381831ef7b931/contracts/Synth.sol#L139)*
#### Description
The `_transferToFeeAddress` function did not check for stale rates. This could lead to inconsistencies in the fee pool.
Mutative functions that burn, issue, or transfer synths are now responsible for checking whether rates are stale when calling view functions. In particular, the `_transferToFeeAddress` function in `Synth.transfer()` performed an exchange without checking whether the relevant currency was stale.
#### Remedial Action
It is recommended to enforce a staleness check in `_transferToFeeAddress` to properly ensure that rates are current at the time of operation.
##### Update
A staleness check was implemented in `Exchanger._exchange` in commit [4c82060](https://github.com/Synthetixio/synthetix/pull/558/commits/4c820608e66050d041b40fa286d25dba5db2d34a). This sufficiently remediated this vulnerability since `Exchange._exchange` was called in `_transferToFeeAddress`.
### 5.5.4 Options Transfers Should Require `requireSystemActive` (Low)
*SIP-53: [BinaryOption.sol#L127](https://github.com/Synthetixio/synthetix/blob/8169a2ee452b060178455cb6369ac59363179c42/contracts/BinaryOption.sol#L127)*
#### Description
The `_transfer` function used by BinaryOptions did not require `requireSystemActive`. While synths in the Synthetix system required the system to be active, in this case the BinaryOptions were freely traded even when the system was not active.
#### Remedial Action
It is recommended to add the requirement of `requireSystemActive` when transferring synths to ensure that transfers are limited when the system is offline.
##### Update
Implemented in [a9f7023](https://github.com/Synthetixio/synthetix/blob/a9f70232be9bcf111893f19eac1eeedc58d2b976/contracts/BinaryOption.sol#L144).
### 5.5.5 Unused Function Parameters (Informational)
*SIP-48: [Exchanger.sol#L317](https://github.com/Synthetixio/synthetix/blob/1ed6657a4af2e80d0fcc844ce4e381831ef7b931/contracts/Exchanger.sol#L317)*
#### Description
The `remitFee` function passed unused parameters `_issuer` and `_exRates`. While there may not be a direct impact when passing unused parameters, it is recommended to remove these parameters to reduce the function complexity and increase its overall readability.
##### Update
Implemented in [f218abc](https://github.com/Synthetixio/synthetix/pull/558/commits/f218abc8ad2e0463a1444b6815cdb1cac81c5b6d).
### 5.5.6 Missing Test Case (Informational)
*SIP-48: [Issuer.sol#L176](https://github.com/Synthetixio/synthetix/blob/1ed6657a4af2e80d0fcc844ce4e381831ef7b931/contracts/Issuer.sol#L176)*
#### Description
There was no test coverage for an edge case accounted for in `_totalIssuedSynths`. The edge case was intended to check the requested rate of a `currencyKey` other than a synth or SNX.
#### Remedial Action
It is recommended to add test coverage for the edge case in `_totalIssuedSynths` by using other available currency keys, such as `ETH`.
##### Update
Implemented in [4537450](https://github.com/Synthetixio/synthetix/pull/558/commits/4537450f24c52051454918ce705e69defc4e83c8).
### 5.5.7 Unsafe Arithmetic Used (Informational)
*SIP-53: [BinaryOption.sol#L62](https://github.com/Synthetixio/synthetix/blob/8169a2ee452b060178455cb6369ac59363179c42/contracts/BinaryOption.sol#L62)*
#### Description
The `totalExercisable` function made use of `+` instead of `SafeMath.add()`. It was unlikely that the use of `+` would cause an overflow or underflow in `totalExercisable` as the operands were representing options in the system.
#### Remedial Action
It is recommended to make use of `SafeMath.add()` in place of `+`.
##### Update
The `totalExercisable` function was replaced by the `totalClaimable` function as implemented in [4b7fd44](https://github.com/Synthetixio/synthetix/pull/537/commits/4b7fd440c77aff2c48a77288ef3bf89612b5a017#diff-3bb0999eedef5614bddfd3aece58b08aR67). The `totalClaimable` function did not make use of arithmetic, which closed this issue.
### 5.5.8 Design Comments (Informational)
Actions to improve the functionality and readability of the codebase are outlined below.
#### No revert in view functions
*SIP-48: General*
Preventing view functions from reverting increases the usability of them for dApps and scripts that consume it. However, this impacted the auditability of the code as each function using currency rates needs to independently check if a given rate is stale. If incorrectly implemented, it could lead to instances where stale rates are used in mutative functions.
##### Comment
After a discussion with the Synthetix team, the `_exchange` function was modified to include a staleness check. Several factors were considered, such as gas usage and potential for incorrect usage. The modified `_exchange` function was considered to sufficiently remediate issues to the extent possible.
##### Update
Implemented in [4c82060](https://github.com/Synthetixio/synthetix/blob/4c820608e66050d041b40fa286d25dba5db2d34a/contracts/Exchanger.sol#L238).
#### Comment Incorrectly Describes Code
*SIP-48: [Issuer.sol#593](https://github.com/Synthetixio/synthetix/blob/1ed6657a4af2e80d0fcc844ce4e381831ef7b931/contracts/Issuer.sol#L593), [Exchanger.sol#L234](https://github.com/Synthetixio/synthetix/blob/1ed6657a4af2e80d0fcc844ce4e381831ef7b931/contracts/Exchanger.sol#L234)*
There were two comments referencing that `effectiveValue()` would check for stale rates. As this validation was no longer enforced, these comments should be removed.
##### Update
Implemented in [4c82060](https://github.com/Synthetixio/synthetix/pull/558/commits/4c820608e66050d041b40fa286d25dba5db2d34a).
*SIP-53: [BinaryOptionMarket.sol#L452](https://github.com/Synthetixio/synthetix/blob/8169a2ee452b060178455cb6369ac59363179c42/contracts/BinaryOptionMarket.sol#L452)*
BinaryOptionMarket.sol#L452 specified that the price does not need to be checked for staleness if it was updated after maturity. Since `resolve()` can only be called after maturity, the price could be from before the maturity date. The comment should be corrected to say that the price does not need to be checked for staleness if the price was updated close enough to the maturity date.
##### Update
Implemented in [c0a361c](https://github.com/Synthetixio/synthetix/pull/537/commits/c0a361c89fe122958c78c7d43f85c8ef84745ed3).
#### Refactoring Suggestions
It is recommended that certain portions of the code be refactored to improve readability and consistency, as indicated below.
1. SIP-53: BinaryOptionMarket.sol#L432: `refundSansFee` should be replaced with `refundMinusFee`
2. SIP-53: BinaryOption.sol: `claimableBy` returns the quantity of options available at the currency price. A more appropriate name would be `amountClaimableBy`.
3. SIP-53: BinaryOptionMarket.sol: To improve the readability of `destructionReward()`, it should be changed to:
<pre>
<code class="language-solidity" >
function destructionReward() external view returns (uint) {
if(resolved) {
if(_destructible()) {
return _destructionReward(deposited);
}
}
return 0;
}
</code>
</pre>
4. SIP-53: BinaryOptionMarket.sol#L114: `_bids[0]` should use `longBid` and L115 `_bids[1]` should use `shortBid`.
##### Update
1. Implemented in [5ff5cfe](https://github.com/Synthetixio/synthetix/pull/537/commits/5ff5cfe4f364360b3a8664feff772dd0f8c35db7).
2. Implemented in [aaebfeb](https://github.com/Synthetixio/synthetix/pull/537/commits/aaebfebdee7636320c5f4fc2e6aa22fb8d3b477f).
3. `destructionReward()` was removed in commit [4b7fd44](https://github.com/Synthetixio/synthetix/pull/537/commits/4b7fd440c77aff2c48a77288ef3bf89612b5a017#diff-2ad29a4fc4e741d71819e0eab3387573L29).
4. Implemented in [2f37921](https://github.com/Synthetixio/synthetix/commit/2f379213535423b04c8c739b6eb855097944271d).
#### Gas Optimization
*SIP-53: [BinaryOptionMarket.sol#L88](https://github.com/Synthetixio/synthetix/blob/8169a2ee452b060178455cb6369ac59363179c42/contracts/BinaryOptionMarket.sol#L88)*
Market parameters were only validated after a new market was created. If the checks were performed in the market manager prior to creating the contract, a significant amount of gas would be saved in the event of a revert.
##### Update
Market parameter validation was moved into the manager, with the exception of the creator limit checks. This was implemented in [fe62f41](https://github.com/Synthetixio/synthetix/pull/537/commits/fe62f41dd6a69f0e84a4a80c036a6c0442800642).
<pre>
<code class="language-solidity" >
