xSNX Relaunch Smart Contract Audit
# 1. Introduction
iosiro was commissioned by [xToken](https://xtoken.market/) to conduct a second smart contract audit of changes introduced into the platform since the first [xSNX Smart Contract Audit](https://iosiro.com/audits/xsnx-smart-contract-audit). The audit was performed intermittently between 16 September and 26 September.
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 second audit performed by iosiro on the xSNX smart contracts. The scope of the audit was limited to the implementation of a new proxy, refactoring code to accommodate the bytecode limit, and a bug fix for a [flash loan attack reported by samczsun](https://medium.com/xtoken/xsnxa-false-start-post-mortem-f26a7a735383). For a full description of the scope see section [3.1. Scope](#section-3).
The purpose of the xSNX proxy was to provide the admin with the ability to upgrade the logic smart contracts. Users should be aware that as with any upgradeable system, when performing an upgrade, there is the potential for new issues to be introduced into the system, whether it be by malice or simply negligence. However, when performing an upgrade, a second trusted address would need to confirm the new contract address, which requires at least two parties to collude.
The fix implemented to address the flash loan attack was found to properly address the reported vulnerability.
One high risk, one medium risk, and several low risk issues were identified during the audit, as well as one informational issue. At the conclusion of the audit, only one informational issue was open.
<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.
[comment]: <> (PR 24 - main PR)
**Project Name:** xSNX<br/>
**Commits:** [7bdf24f](https://github.com/xtokenmarket/xsnx/commit/7bdf24fbb3127d47e33c9a09a65d46d2166b8319)<br/>
**Files:** TradeAccounting.sol, Proxy.sol, TradeAccountingProxy.sol, xSNXCoreProxy.sol, xSNXCore.sol
[comment]: <> (PR 21)
**Project Name:** xSNX<br/>
**Commits:** [8f57e12](https://github.com/xtokenmarket/xsnx/commit/8f57e12dafd50bb2dc71c96ba95ddd544cdcbe1a)<br/>
**Files:** TradeAccounting.sol, xSNXCore.sol
[comment]: <> (PR 22)
**Project Name:** xSNX<br/>
**Commits:** [0c6d2e0](https://github.com/xtokenmarket/xsnx/commit/0c6d2e02afea8ca4243270365180788e2076674f)<br/>
**Files:** xSNXCore.sol
[comment]: <> (PR 23)
**Project Name:** xSNX<br/>
**Commits:** [9414169](https://github.com/xtokenmarket/xsnx/commit/9414169dc59c8d3afbac3e67d0e53583571cca86)<br/>
**Files:** TradeAccounting.sol
[comment]: <> (PR 25)
**Project Name:** xSNX<br/>
**Commits:** [bb650fd](https://github.com/xtokenmarket/xsnx/commit/bb650fd58971108fb435b6c86636ea56256fcc05)<br/>
**Files:** TradeAccounting.sol
[comment]: <> (PR 27)
**Project Name:** xSNX<br/>
**Commits:** [3dbfd8d](https://github.com/xtokenmarket/xsnx/commit/3dbfd8df5ea31bdabf8413566d723ff113213be5)<br/>
**Files:** TradeAccounting.sol, xSNX.sol, xSNXAdmin.sol
[comment]: <> (PR 28)
**Project Name:** xSNX<br/>
**Commits:** [0778765](https://github.com/xtokenmarket/xsnx/commit/0778765cc61be62d92f0865a2b4d1bb150e0bded)<br/>
**Files:** TradeAccounting.sol, xSNX.sol, xSNXAdmin.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 Phase II Changes
The specification given below was derived by iosiro from the code in the target commit hashes. Any discrepancies between this specification and expected behaviour should be brought to the attention of iosiro.
### 4.1.1 Proxy Upgrade Pattern
The proxy contract follows the unstructured storage proxy upgrade pattern. Functionality was added for the proxy admin to propose a new delegate contract. One of the two cosigners could then confirm the proposed delegate contract. Further functionality for the proxy admin to transfer administrator rights to another address was implemented as well.
Proxies and logic contracts need to be written carefully to avoid storage collisions. The xSNX proxy avoids storage collisions by using randomized slots for its state variables.
### 4.1.2 Flash Loan Attack
A [flash loan attack](https://medium.com/xtoken/xsnxa-false-start-post-mortem-f26a7a735383) was reported by samczsun after the initial xSNX launch. As the original `mint` function used the Synthetix oracle to determine the amount of xSNX to mint when transferring ETH, it was possible for a discrepancy to exist between the value of SNX purchased and the value of ETH transferred to the contract when minting tokens with ETH. This allowed an attacker to crash the Kyber ETH/SNX rate with a flash loan to reduce the amount of SNX received by the contract during minting, and then mint xSNX at the correct rate. This would effectively reduce the price per token of xSNX as the NAV would be diluted by more tokens while holding proportionately less value.
A [fix](https://github.com/xtokenmarket/xsnx/commit/8f57e12dafd50bb2dc71c96ba95ddd544cdcbe1a) was implemented to determine the amount of xSNX to mint based on the number of SNX tokens purchased through Kyber.
<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 open at the conclusion of the audit.
## 5.2 Medium Risk
No medium risk issues were open at the conclusion of the audit.
## 5.3 Low Risk
No low risk issues were open at the conclusion of the audit.
## 5.4 Informational
### 5.4.1 Transparent Proxy Pattern Not Used
#### Description
The proxy contract did not differentiate between function calls from a user and function calls from the proxy admin. In the event of function identifier collisions, the user may erroneously call a function in the proxy instead of the function in the delegated contract.
#### Remedial Action
Use of the transparent proxy pattern is recommended. A transparent proxy ensures that any function called by the user will be delegated to the logic contract, while any function called by the proxy admin will be executed on the proxy contract.
Moreover, since there are cosigners who may need to interact with the proxy contract directly, it is recommended to ensure that all cosigner function calls are strictly delegated to the proxy contract as well.
#### Further Information
[OpenZeppelin: The Transparent Proxy Pattern](https://blog.openzeppelin.com/the-transparent-proxy-pattern/).
## 5.5 Closed
<a name="section-5.5.1"></a>
### 5.5.1 Withdrawal Function Incorrectly Implemented (High Risk)
*[xSNX.sol#L230](https://github.com/xtokenmarket/xsnx/blob/21f351b11fbbf78cc7c56a676d0f1ea275370174/contracts/xSNX.sol#L230)*
#### Description
The `withdrawNativeToken` function was incorrectly implemented, causing the function to transfer the owner's tokens instead of the contract's tokens. As such it was not possible for the admin to withdraw xSNX tokens locked in the token contract.
The reason for this issue was that the `withdrawNativeToken` function called the `transfer` function directly, which preserved the `msg.sender` as the owner. Instead, the contract should make an external call to itself to correctly set `msg.sender` to its own address.
#### Remedial Action
It is recommended that the `withdrawNativeToken` function be updated to make an external call to the transfer function in order to correctly set the `msg.sender` value.
#### Update
Implemented in [4d24db4](https://github.com/xtokenmarket/xsnx/commit/4d24db4639efbd22572f90bc2e836d6f9e5fb2c9#diff-af8399fba6a09de89f6477ab65645292R233).
### 5.5.2 No Recovery Withdrawal Function (Medium Risk)
#### Description
The `xSNX` contract did not implement a recovery function to accommodate withdrawals in case of erroneous xSNX transfers directly to the token contract.
#### Remedial Action
It is recommended that a function is added to the xSNX contract to allow the admin to withdraw any xSNX tokens held by the contract.
#### Update
Implemented in [21f351b](https://github.com/xtokenmarket/xsnx/commit/21f351b11fbbf78cc7c56a676d0f1ea275370174#diff-af8399fba6a09de89f6477ab65645292R230). However, a high risk issue was found with the changes made in this commit, which is detailed in [Withdrawal Function Incorrectly Implemented](#section-5.5.1).
### 5.5.3 Missing Validation on Delegate Address (Low Risk)
#### Description
The proxy did not validate whether a delegate address was a contract or an externally owned account, which meant that the delegate address could be set to a non-contract address. This may pose a risk under certain conditions, since `delegatecall` will successfully return if the destination has no code. This successful return could result in the unexpected execution of application logic.
#### Remedial Action
It is recommended the delegate is validated to determine whether it is a contract. The OpenZeppelin [Address library](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L26) contains an example of a function that checks whether or not a given address is a contract or an externally owned account.
#### Update
Implemented in [49f06ba](https://github.com/xtokenmarket/xsnx/commit/49f06ba77f0fe800f39c3075e76cbb22028a70fe#diff-4aab7aae8e6d340234b80c5d61b73af6R120).
### 5.5.4 Missing Validation in Constructor (Low Risk)
*[Proxy.sol#L46](https://github.com/xtokenmarket/xsnx/blob/cc45123524e0d7ca7f6380c32ec7802b7bce41c0/contracts/proxies/Proxy.sol#L46)*
#### Description
The constructor in the `Proxy` contract did not include validation preventing the implementation address from being set to the zero address. An error in the contract deployment could thus cause the implementation address to be set as the zero address.
#### Remedial Action
It is recommended that a `require` statement is added at the beginning of the constructor to ensure the implementation address is not the zero address.
#### Update
Implemented in [891f79d](https://github.com/xtokenmarket/xsnx/commit/891f79dd06f96b210293ee81b5e7cb9373c3500b#diff-4aab7aae8e6d340234b80c5d61b73af6R46).
### 5.5.5 Possible Outdated Synthetix Contracts Used (Low Risk)
#### Description
The `TradeAccounting` contract did not use the Synthetix `AddressResolver` to fetch the latest Synthetix address. This could lead to using an outdated version of the Synthetix contract being used.
#### Remedial Action
All references to the Synthetix contract should use the Synthetix `AddressResolver` contract to dynamically fetch the latest Synthetix address.
#### Update
Implemented in [21f351b](https://github.com/xtokenmarket/xsnx/commit/21f351b11fbbf78cc7c56a676d0f1ea275370174#diff-3113cc0448a5da5d8f0843db5a15697fR699) and [862a9b7](https://github.com/xtokenmarket/xsnx/commit/862a9b70939054c6e9104d76e0a9079441b31e58).
### 5.5.6 Design Comments (Informational)
It is recommended that certain portions of the code be refactored to improve readability and consistency, as indicated below.
#### Fix Spelling and Grammatical Errors
Language mistakes were identified in the codebase. Fixing these mistakes can help improve the end-user experience by providing clear information on errors encountered, and improve the maintainability and auditability of the codebase.
1. [Proxy.sol#L73](https://github.com/xtokenmarket/xsnx/blob/cc45123524e0d7ca7f6380c32ec7802b7bce41c0/contracts/proxies/Proxy.sol#L73): `comfirms` should be `confirms`
#### Update
Fixed in [891f79d](https://github.com/xtokenmarket/xsnx/commit/891f79dd06f96b210293ee81b5e7cb9373c3500b#diff-4aab7aae8e6d340234b80c5d61b73af6R73).
