xSNX Smart Contract Audit
# 1. Introduction
iosiro was commissioned by [xToken](https://xtoken.market/) and the Synthetix [GrantsDAO](https://snxgrants.io/) to conduct a smart contract audit of the xSNX implementation. The audit was performed intermittently between 16 July and 05 August on different versions of the codebase. A review of changes was performed between 10 August and 11 August 2020.
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 [xSNX](https://github.com/xtokenmarket/xsnx).
The purpose of [xSNX](https://github.com/xtokenmarket/xsnx) was to wrap SNX in order to provide a [managed service for earning staking rewards](https://medium.com/xtoken/introducing-xtoken-2fd50d0f943b). Users would only need to deposit ETH or SNX into the contract, which would then be managed by an admin account. This would avoid the need for users to claim rewards on a regular basis, simplifying the process and reducing gas fees.
#### System Description
To enter the system, users exchanged ETH or SNX for xSNX at an exchange rate that was based on the system's net asset value (NAV), which included escrowed SNX earned through staking rewards. When exiting the system, users would burn xSNX in exchange for ETH at an exchange rate that excluded the escrowed SNX.
The system was managed by admin roles, comprising of the owner and a manager address. The owner was rewarded with fees for various actions in the system. The admin roles were responsible for claiming the Synthetix rewards, maintaining the collateralization ratio, and allocating assets between the hedge assets according to the hedge ratio.
For a comprehensive description of the system, see [Section 4 - Design Specification](#section-4).
#### Audit Findings
Overall, the system was found to operate as intended and the implementation was of a fair standard.
* Several high and medium risk issues were identified and closed during the audit.
* Several informational risk issues relating to functional suggestions and best practices were open at the conclusion of the audit.
At an architectural level, there was a large dependency on the admin role within the system. Unlike most DeFi systems that operate without a central managing party, xSNX relied heavily on the admin role. Examples of this include:
* Staking SNX, maintaining the correct collateralization ratio, and allocating assets were all done at the discretion of the admin.
* The ability to exit the system was effectively dependent on the admin performing the necessary rebalances to provide ETH liquidity.
* Due to a lack of validation, admin functionality could potentially be vulnerable to negligence or malicious intent.
The dependency on the admin role presented a centralized risk, which could lead to a system halt in the case of an absent admin, or to potential exploitation in the case of a malicious admin. Some precautions were taken to try limit this exposure, including a mechanism that allowed users to exchange contract funds into ETH to provide liquidity for users to exit the system if the admin was absent for a set period of time.
#### Further Precautions
While the assessment attempted to identify any potential functional issues that may be encountered after deploying to mainnet, given test constraints, the system was only functionally tested in a local test environment using mock implementations of external services. Given the complexity of the system, and its dependency on several external systems (e.g. Curve, Kyber, Synthetix, and Set Protocol), it is recommended that the project is initially launched and tested on mainnet in a limited capacity before user funds are permitted.
Furthermore, the security posture of the smart contracts could be strengthened by:
* Remediating the issues identified in this report and performing a review to ensure that the issues were correctly addressed.
* Performing additional iterations of security audits. An audit is not a guarantee that the code is free from issues, so performing multiple iterations might uncover new issues.
* Implementing a bug bounty program to encourage the responsible disclosure of security vulnerabilities in the smart contracts.
<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 xSNX Smart Contracts
**Project Name:** xSNX<br/>
**Starting Audit Commit:** [00d7a44](https://github.com/xtokenmarket/xsnx/tree/00d7a4462234d5f08f118b5651ef88c39f321171)<br/>
**Final Audit Commit:** [c2f6a5a](https://github.com/xtokenmarket/xsnx/tree/c2f6a5a8df805776a637363dc9010740526fe5f9)<br/>
**Files:** TradeAccounting.sol, Whitelist.sol, xSNXCore.sol
### 3.1.2 xSNX Smart Contracts Review
**Project Name:** xSNX<br/>
**Starting Review Commit:** [c40cd3d](https://github.com/xtokenmarket/xsnx/commit/c40cd3d047d57ecb6aedc7bf372092660ab9ba04)<br/>
**Final Review Commit:** [2e300aa](https://github.com/xtokenmarket/xsnx/commit/2e300aaee1844f4c838ee20cf8cebc56f8746808)<br/>
**Files:** TradeAccounting.sol, xSNXCore.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 xSNX
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 xSNX Token
xSNX was an ERC20-compliant token with the following values.
Field | Value
------------ | -------------
Name | xSNX
Symbol | xSNXa
### 4.1.2 User Functionality
The following functionality was exposed publicly for users to interact with the xSNX system.
#### Mint
In order to mint xSNX, users would need to send either ETH or SNX to the contract. In return, users would receive xSNX tokens in proportion to their contribution to the contract's net asset value, including escrowed SNX. Unless the contract required additional ETH to maintain the hedge ratio, ETH funds would be exchanged into SNX. Each time a user minted tokens with ETH, an admin fee was taken in ETH.
#### Burn
Users were able to exit the system by burning their xSNX tokens in exchange for ETH. The exchange rate was calculated based on the number of SNX available to the contract, i.e. excluding escrowed SNX. Given this difference with the minting calculation, the exchange rate would be higher at a given time when entering the system than when exiting, as long as there were tokens in escrow. Each time a user burned tokens, an admin fee was taken in ETH.
#### Vest
A publicly available function was available for anyone to vest the escrowed SNX that were earned as staking rewards.
#### Emergency Liquidation
Due to limited liquid ETH to exit the system, users were able to call the `liquidationUnwind` function if the admin had not claimed within a set period of time. This would allow users to sell the system's Set for sUSD to reduce the Synthetix debt position to unlock its SNX. The SNX would then be exchanged into ETH, which could be withdrawn by users from the system by burning their xSNX.
### 4.1.3 Admin Functionality
The following functionality was exposed to the owner and manager roles.
#### Hedge
The hedge function would stake all the available SNX to receive sUSD. The sUSD would then be distributed according to the hedge ratio between the specified Set and ETH.
#### Claim
The admin was responsible for claiming Synthetix staking rewards. When claiming, it was possible for the admin to sell a portion of the Set holdings for sUSD to burn in order to adjust the collateralization ratio. A portion of the sUSD reward was allocated as an admin fee and the rest was exchanged into ETH.
#### Rebalance
Several rebalance functions were available to reallocate asset or liability components if they exceeded certain thresholds. If the hedge assets exceeded the debt liabilities, a rebalance towards SNX could be called. If the debt liabilities exceeded the hedge assets, a rebalance towards the hedge assets could be called. If the ETH balance was less than a certain threshold, a rebalance towards ETH could be called.
#### Helper Functions
Some helper functions were exposed to allow the admin to manage the system, including:
* Setter functions were available to set fees, approve token transfers, and update contract addresses.
* A withdrawal function was accessible by the owner to withdraw the accrued admin fees.
* View functions were available, including one to help calculate the necessary sUSD to correct the Synthetix collateralization ratio in order to claim staking rewards.
### 4.1.4 Asset Allocation
#### Hedge Ratio
The hedge ratio was defined as 75% Set and 25% ETH. When users entered the system, or when the admin called the hedge or a rebalance function, the ratio was adjusted.
#### Debt Ratio
The system aimed to maintain parity between the hedge assets and debt liabilities. Rebalances could be performed if the ratio drifted more than 5% in either direction.
#### ETH Reserves
A reserve of ETH was maintained according to the hedge ratio (25% ETH, 75% Set), which at a collateralization ratio of 750% was equivalent to approximately 3% of the NAV. The ETH reserve was used to pay out users exiting the system. As only a relatively small amount of the NAV was stored in ETH, it would only be possible for a limited portion of xSNX to be burned at any stage. Thereafter, the ETH reserve would only be replenished when more users entered the system with ETH, or when an admin claimed or rebalanced the hedged assets.
### 4.1.5 External Systems
#### Curve
Due to limited sUSD liquidity in Kyber, [Curve](https://curve.fi) was used to exchange between sUSD-USDC to reduce slippage.
#### Kyber Network
The Kyber Network was used for exchanging between USDC-ETH and USDC-Set.
#### Set Tokens
Set tokens were used as a mechanism to attempt to hedge against the debt accrued by staking SNX for sUSD. Only Sets that used a single active asset at a time were supported.
#### Synthetix
Synthetix was used to stake SNX in order to claim SNX and sUSD rewards. Additionally, the Synthetix exchange rates were used to determine the USD rate of SNX, ETH, and the active Set asset.
### 4.1.6 Design Considerations
Below is a list of notable design decisions.
#### Minimizing Gas Fees
One of the main goals of the system was to reduce gas costs for users. As such, user operations, specifically, minting and burning, were implemented in such a way as to reduce the gas costs. When minting xSNX, the user funds were left in SNX. It was then the admin's responsibility to stake the SNX by calling the hedge function. This means that there was some period between when a user entered the system and when the assets were staked.
#### Set Functionality
The system was designed to integrate with different Set tokens with the idea that users could have a choice in how to counteract the effect of the growing Synthetix debt pool. It should be noted that a simple strategy such as [ETHRSI6040](https://www.tokensets.com/set/ethrsi6040), which allocates assets to either ETH or USDC, could be implemented without the need to integrate with the Set Protocol. It may be beneficial to implement this functionality within xSNX itself, as there could be benefits in maintaining a reserve of ETH and sUSD. However, this approach would not allow for the same extensibility, which was a requirement of the xSNX design.
#### Exit Liquidity
A notable restriction of the system was that during normal operations, only a fraction of the NAV was available for users to exit from the system. Once depleted, this ETH reserve would only be replenished when users entered the system through minting xSNX with ETH, when the admin hedged or claimed, or when the admin rebalanced the hedge assets.
As the xSNX value would increase once the SNX rewards started paying out, an OTC or secondary market could be established to reduce the dependency on the burn functionality to exit from xSNX.
<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 No Admin Validation in `xSNXCore.rebalanceTowardsHedge()`
The `xSNXCore.rebalanceTowardsHedge()` function freely permitted an admin to specify the amount of sUSD to burn and Set to sell in exchange for liquid ETH. While there was a `TradeAccounting.getRebalanceTowardsHedgeUtils()` function available to calculate the values necessary to correct the allocations, it was not used.
The `xSNXCore.unwindStakedPosition()` function already permitted an admin to exchange hedge assets for ETH at their discretion, most likely in the event that all users needed to exit the system. As such, no additional risk is posed by the `xSNXCore.rebalanceTowardsHedge()` function. However, it should be noted that from a functional perspective, it may still be easier to calculate the value on-chain at the expense of a nominal amount of gas.
### 5.4.2 System Potentially Vulnerable to Front-Running
If an attacker monitored the SNX/ETH rate and saw an upcoming price increase, they could attempt to front-run the difference by minting xSNX tokens and then burning them once the increased rate was committed on-chain. Should the profit in ETH exceed the spread between the mint and burn rate, taking into account gas and system fees, as well as slippage, the system might be exploitable. This attack would become less feasible as more SNX was escrowed.
Given the conditions required to exploit this issue, the likelihood of successful exploitation is very low. However, if deemed necessary, it would be possible to implement a per-account lockout period, which should prevent a user from minting, burning, or transferring their xSNX for some short period (e.g. 3 minutes) after minting or burning.
### 5.4.3 Variable Slippage Rate
The `TradeAccounting.SLIPPAGE_RATE` constant was used to model the potential slippage rate of exchanges. It may be helpful to provide the admin with the ability to modify this rate so that it can be adjusted over time to better represent slippage rates if necessary. Validation should then be performed to ensure that it is within an acceptable range to limit the potential effect of its variability.
### 5.4.4 Unit Test Hardening
Unit tests can aid in ensuring the correctness of code. The test quality could be improved by:
1. Implementing tests that encompass the full life-cycle of the system, rather than only testing individual parts of the system.
2. Numerical values should be tested independently off-chain to ensure that they produce the correct values.
3. `assert(true)` statements should be removed or replaced with tests that verify the outcome of the test to be correct.
## 5.5 Closed
### 5.5.1 Owner Able to Withdraw Funds (High Risk)
#### Description
It was found that the owner was able to withdraw the contract's funds. The owner was able to call the `approve` function with an arbitrary ERC-20 token and spender address in [xSNXCore.approveSetTransferProxy()](https://github.com/xtokenmarket/xsnx/blob/dde8c065137367cba963f124d041c3eb2a1f890d/contracts/xSNXCore.sol#L594) and [TradeAccounting.approveCurve()](https://github.com/xtokenmarket/xsnx/blob/dde8c065137367cba963f124d041c3eb2a1f890d/contracts/TradeAccounting.sol#L1031). As such, it would be possible for the owner to withdraw any ERC-20 tokens from the contract through approving an address and then using the `transferFrom` function.
#### Remedial Action
It is recommended that the spender addresses used for the `approve` functions are made immutable to prevent the owner from being able to specify a different spender address.
#### Update
The Set transfer proxy address was made immutable in [b6781b3](https://github.com/xtokenmarket/xsnx/commit/b6781b3ac574f4ad22cee65bbe0e5ba42e1611b1#diff-46ce021cac42f38cde438feff608a8cdL596). The setter functionality for the Curve address was updated to require the owner to propose an address, and then another trusted party to accept the proposal in [645e548](https://github.com/xtokenmarket/xsnx/commit/645e54817adc356d1ed42988819da07ca85d7014#diff-3113cc0448a5da5d8f0843db5a15697fR1028).
### 5.5.2 Incorrect Mint NAV Calculation (High Risk)
#### Description
When minting xSNX, the NAV of the contract was calculated in order to determine the relative amount of xSNX to mint. However, when minting with ETH, the NAV calculation included the ETH from the function call. As such, the NAV was overrepresented and the amount of tokens issued was reduced.
#### Remedial Action
It is recommended that the minting NAV calculation is modified to exclude the incoming ETH.
#### Update
Fixed in [53ed59e](https://github.com/xtokenmarket/xsnx/commit/53ed59ecb16da4dce6472e0db8c8fdf0eb10acc1#diff-3113cc0448a5da5d8f0843db5a15697fR292).
### 5.5.3 Incorrect Unit Multiplication (High Risk)
#### Description
In the `TradeAccounting.calculateSusdToBurnForRedemption()` function, the `snxToSell` variable was set to `getContractOwnedSnxValue()`, which was already in terms of USD. The `valueOfSnxToSell` was then set to `snxToSell` [multiplied by the SNX price in USD](https://github.com/xtokenmarket/xsnx/blob/00d7a4462234d5f08f118b5651ef88c39f321171/contracts/TradeAccounting.sol#L708), which resulted in an incorrect value for the calculation.
#### Remedial Action
It is recommended that the multiplication by the SNX price is removed.
#### Update
Fixed in [b6781b3](https://github.com/xtokenmarket/xsnx/commit/b6781b3ac574f4ad22cee65bbe0e5ba42e1611b1#diff-3113cc0448a5da5d8f0843db5a15697fR772).
### 5.5.4 Improper Validation on Address Setters (Medium Risk)
#### Description
The `TradeAccounting.setCallerAddress` function permitted the owner to change the address pointing to the xSNXCore instance after it had already been set. If the address was changed, the xSNXCore instance would lose access to much of the functionality it required to operate.
Additionally, the `TradeAccounting.setAddressResolverAddress` function could be used by the owner to set a malicious instance of the Synthetix State or Exchange Rates contract. Doing so would allow the owner to siphon user funds out by temporarily adjusting the exchange rates to buy xSNX at a reduced rate and sell at an increased rate.
#### Remedial Action
As the `TradeAccounting.setCallerAddress()` function was only supposed to be called once, it is recommended that validation is added to ensure that the address is not already set before allowing the owner to set the address.
Furthermore, it is recommended that the `TradeAccounting.setAddressResolverAddress()` function is removed.
#### Update
The `TradeAccounting.setCallerAddress()` address was fixed in [645e548](https://github.com/xtokenmarket/xsnx/commit/645e54817adc356d1ed42988819da07ca85d7014#diff-3113cc0448a5da5d8f0843db5a15697fR989).
The `TradeAccounting.setAddressResolverAddress()` function was removed in [b6781b3](https://github.com/xtokenmarket/xsnx/commit/b6781b3ac574f4ad22cee65bbe0e5ba42e1611b1#diff-3113cc0448a5da5d8f0843db5a15697fL992).
### 5.5.5 Excessive Admin Capabilities (Medium Risk)
#### Description
Certain admin functions were found to expose users to some degree of risk as the admin was allowed to pass in arbitrary values for managing contract funds.
1. `xSNXCore.rebalanceTowardsSnx()` could retrieve the `setToSell` amount and `currentSetAsset` address by calling `TradeAccounting.getRebalanceTowardsSnxUtils()` directly.
2. There was no validation in `xSNXCore.rebalanceSetToEth()`. This function should only be callable when the ETH balance is less than `hedgeAssets/ETH_TARGET`. Furthermore, the `redemptionQuantity` and `activeAsset` values could be calculated on-chain with `tradeAccounting.calculateSetToSellForRebalanceSetToEth()` and `xSNXCore.getAssetCurrentlyActiveInSet()` respectively.
3. The `xSNXCore.setFeeDivisors()` function did not validate the fee rates being set. As such, the owner could set arbitrarily high rates. It is recommended that the fee rates are restricted to an accepted range through validation.
It should be noted that the lack of validation on these operations was in part due to the intention to reduce gas costs, and the contract size and complexity.
#### Remedial Action
In order to reduce admin privileges, it is recommended that values are calculated on-chain where possible.
#### Update
1. The amount to Set to sell is now calculated on-chain as of [c40cd3d](https://github.com/xtokenmarket/xsnx/commit/c40cd3d047d57ecb6aedc7bf372092660ab9ba04#diff-46ce021cac42f38cde438feff608a8cdR321).
2. The `activeAsset` value in `xSNXCore.rebalanceSetToEth()` is now calculated on-chain in [645e548](https://github.com/xtokenmarket/xsnx/commit/645e54817adc356d1ed42988819da07ca85d7014#diff-46ce021cac42f38cde438feff608a8cdR387). The amount of Set to sell for ETH is calculated on-chain as of [c40cd3d](https://github.com/xtokenmarket/xsnx/commit/c40cd3d047d57ecb6aedc7bf372092660ab9ba04#diff-46ce021cac42f38cde438feff608a8cdR377).
3. Validation logic was added when setting fees in [c40cd3d](https://github.com/xtokenmarket/xsnx/commit/c40cd3d047d57ecb6aedc7bf372092660ab9ba04#diff-46ce021cac42f38cde438feff608a8cdR570).
### 5.5.6 Unable to Withdraw Profits through `liquidationUnwind()` (Low Risk)
#### Description
When unwinding assets through `xSNXCore.liquidationUnwind()`, the Set assets were sold for sUSD to burn any remaining debt to unlock SNX to sell for ETH. In the event of an absent admin, excess sUSD remaining in the contract after burning all of the debt would be locked.
#### Remedial Action
It is recommended that the sUSD balance is checked after unwinding the staked position. Any remaining sUSD should be exchanged into ETH and distributed to users.
#### Update
Fixed in [c40cd3d](https://github.com/xtokenmarket/xsnx/commit/c40cd3d047d57ecb6aedc7bf372092660ab9ba04#diff-46ce021cac42f38cde438feff608a8cdR456).
### 5.5.7 Use of `transfer` Function (Informational)
*[TradeAccounting.sol#L174](https://github.com/xtokenmarket/xsnx/blob/00d7a4462234d5f08f118b5651ef88c39f321171/contracts/TradeAccounting.sol#L174), [xSNXCore.sol#L536](https://github.com/xtokenmarket/xsnx/blob/00d7a4462234d5f08f118b5651ef88c39f321171/contracts/xSNXCore.sol#L536)*
#### Description
The `TradeAccounting.swapTokenToEther()` and `xSNXCore.withdrawFees()` functions made use of the `transfer` function to send ether. While `transfer` is commonly used to prevent reentrancy attacks due to its 2300 gas limit, it relies on the receiving contract to have a fallback function below this limit. As demonstrated in EIP-1884, which changed the gas cost of the `SLOAD` operation, gas costs can change. This could lead to a case where a contract has its fallback function increased above the 2300 limit, resulting in it becoming incompatible with the system.
#### Remedial Action
It is recommended that the `call` function be used to send ETH instead of `transfer`.
#### Further Reading
[Consensys On Avoiding `transfer()`](https://diligence.consensys.net/blog/2019/09/stop-using-soliditys-transfer-now/).
##### Update
Implemented in [c34c275](https://github.com/xtokenmarket/xsnx/commit/c34c27512d7db8dff7950f4a862e1aed0a329618).
### 5.5.8 Incorrect Function Visibility (Informational)
*[TradeAccounting.sol#L712](https://github.com/xtokenmarket/xsnx/blob/645e54817adc356d1ed42988819da07ca85d7014/contracts/TradeAccounting.sol#L712)*
### Description
The `TradeAccounting.calculateSusdToBurnToFixRatio()` function visibility was public. The intended visibility was confirmed to be internal.
#### Remedial Action
It is recommended that the function's visibility be changed to internal.
##### Update
Implemented in [c40cd3d](https://github.com/xtokenmarket/xsnx/commit/c40cd3d047d57ecb6aedc7bf372092660ab9ba04#diff-3113cc0448a5da5d8f0843db5a15697fR703).
### 5.5.9 Lack of Fallback Function Validation (Informational)
#### Description
The xSNX fallback function was found to permit anyone to send ETH directly to the contract. As it was possible to mint xSNX with ETH, it is advised that this fallback function be restricted to avoid users accidentally depositing ETH directly into the contract.
#### Remedial Action
The xSNXCore fallback function should be restricted to only receive ETH funds from the TradeAccounting address.
#### Update
The remedial action was implemented in [0acfd24](https://github.com/xtokenmarket/xsnx/commit/0acfd24bbd9011e5a2dfbf1f490d3ac5232d0416#diff-46ce021cac42f38cde438feff608a8cdR595).
### 5.5.10 Design Comments (Informational)
Actions to improve the functionality and readability of the codebase are outlined below.
#### Gas Optimizations
1. `TradeAccounting.calculateAssetChangesForRebalanceToHedge()` called the `getIssuanceRatio()` function twice. The duplicate instance of `issuanceRatio` should be removed.
2. The validation in [`xSNXCore.vest()`](https://github.com/xtokenmarket/xsnx/blob/dde8c065137367cba963f124d041c3eb2a1f890d/contracts/xSNXCore.sol#L462) is redundant and could be removed.
##### Update
The duplicate `issuanceRatio` calculation was fixed in [19706b5](https://github.com/xtokenmarket/xsnx/commit/19706b5b09c4ec37c8f3f2e2a85a3f920e49204e#diff-3113cc0448a5da5d8f0843db5a15697fL808).
The validation from `xSNXCore.vest()` was removed in [b6781b3](https://github.com/xtokenmarket/xsnx/commit/b6781b3ac574f4ad22cee65bbe0e5ba42e1611b1#diff-46ce021cac42f38cde438feff608a8cdL462).
#### Code Cleanup
It is recommended that the following steps are taken to tidy certain aspects of the codebase.
1. The `IKyberNetworkProxy` import in `xSNXCore` can be removed as it was unused.
2. There were no transfers made from `TradeAccounting` to `xSNXCore` using `transferFrom`, so the `xSNXCore.approveTradeAccounting` function can be removed.
3. The following private state variables were not used and should be removed: `xSNXCore.PERCENT`, `TradeAccounting.DEC_6`, `TradeAccounting.MIN_CURVE_RETURN`, `TradeAccounting.CURVE_UPDATE_WAITING_PERIOD`, `TradeAccounting.ETH_ADDRESS` and `TradeAccounting.isCurveSet`.
4. The `100 sUSD Debt` value in the `isRebalanceTowardsSnxRequired` example in TradeAccounting was listed as `($105)`. This should be changed to `($100)`.
It is recommended that the following steps are taken to tidy certain portions of the codebase.
##### Update
The `IKyberNetworkProxy` was removed in [ee46062](https://github.com/xtokenmarket/xsnx/commit/ee46062934b02e93d6f83167e9867a3a6f67c813#diff-46ce021cac42f38cde438feff608a8cdL14).
The `xSNXCore.approveTradeAccounting` function was removed in [b6781b3](https://github.com/xtokenmarket/xsnx/commit/b6781b3ac574f4ad22cee65bbe0e5ba42e1611b1#diff-46ce021cac42f38cde438feff608a8cdL589).
The private state variables were removed in [c34c275](https://github.com/xtokenmarket/xsnx/commit/c34c27512d7db8dff7950f4a862e1aed0a329618).
The debt comment was corrected in [c34c275](https://github.com/xtokenmarket/xsnx/commit/c34c27512d7db8dff7950f4a862e1aed0a329618#diff-3113cc0448a5da5d8f0843db5a15697fR60).
#### Refactoring Suggestions
It is recommended that the following variables be renamed to improve readability.
1. `TradeAccounting.caller` should be `TradeAccounting.xSNXInstance`.
2. The `snxBalanceAdded` parameter in `TradeAccounting.calculateTokensToMintWithSnx()` should be `snxAddedToBalance`.
##### Update
The remedial action was implemented in [c34c275](https://github.com/xtokenmarket/xsnx/commit/c34c27512d7db8dff7950f4a862e1aed0a329618).
#### <a name="gas"></a> Gas Optimizations
Integrating the whitelist into xSNXCore rather than TradeAccounting would avoid the need to make external calls on mint and burn, which would save on user gas costs. It should be noted that the whitelist was not integrated into xSNXCore to reduce the contract's bytecode.
#### Update
The whitelist was removed in [c40cd3d](https://github.com/xtokenmarket/xsnx/commit/c40cd3d047d57ecb6aedc7bf372092660ab9ba04#diff-46ce021cac42f38cde438feff608a8cd).
#### Unnecessary Paused Check in `rebalanceSetToEth`
Despite being an admin function, `rebalanceSetToEth` was found to use the `whenNotPaused` modifier. As it may be necessary for the admin to call this function while the system is paused, this modifier should be removed.
#### Update
The modifier was removed in [c40cd3d](https://github.com/xtokenmarket/xsnx/commit/c40cd3d047d57ecb6aedc7bf372092660ab9ba04#diff-46ce021cac42f38cde438feff608a8cdL384).
#### Conditional Burn in `_unwindStakedPosition`
It may be beneficial to integrate conditional logic into `_unwindStakedPosition` to allow skipping the process of redeeming Set and exchanging it for sUSD to burn. Checking if a 0 value is passed in for `_totalSusdToBurn` would allow for transferrable SNX to be sold without having to first make expensive calls that may not be necessary.
#### Update
The suggestion was implemented in [c40cd3d](https://github.com/xtokenmarket/xsnx/commit/c40cd3d047d57ecb6aedc7bf372092660ab9ba04#diff-46ce021cac42f38cde438feff608a8cdR393).
