Sythentix Kwenta SNX Conversion Smart Contract Audit

</nav>
<article class="report">
<h1 id="introduction">Introduction</h1>
<p>iosiro was commissioned by Synthetix to perform an audit of a SNX to KWENTA token conversion smart contract. The audit was conducted between the 26th and 27th of November, using 3 audit days and 2 auditors.</p>
<h4 id="overview">Overview</h4>
<p>The audit uncovered 2 issues, of which 1 posed a significant risk as it could allow an attacker to get more SNX from the contract than intended. All issues were promptly resolved by the team by the end of the audit, as can be seen in the following table.</p>
<table class="findings-count">
<thead>
<tr>
<th> </th>
<th>Critical</th>
<th>High</th>
<th>Medium</th>
<th>Low</th>
</tr>
</thead>
<tbody>
<tr>
<td>Open</td>
<td>0</td>
<td>0</td>
<td>0</td>
<td>0</td>
</tr>
<tr>
<td>Resolved</td>
<td>0</td>
<td>1</td>
<td>0</td>
<td>1</td>
</tr>
<tr>
<td>Closed</td>
<td>0</td>
<td>0</td>
<td>0</td>
<td>0</td>
</tr>
</tbody>
</table>
<h4 id="scope">Scope</h4>
<p>The assessment focused on the source files listed below, with all other files considered out of scope. Any out-of-scope code interacting with the assessed code was presumed to operate correctly without introducing functional or security vulnerabilities.</p>
<ul>
<li><strong>Project name:</strong> <code>kwenta-snx-conversion</code></li>
<li><strong>Initial audit commit:</strong> <a href="https://github.com/Kwenta/kwenta-snx-conversion/tree/ea0806d128f9d2f91d66af5fbf8e3cc687d6699b">ea0806d</a></li>
<li><strong>Final review commit:</strong> <a href="https://github.com/Kwenta/kwenta-snx-conversion/tree/0ab9dcf1723e2b7d6610e55b5ee6498a8c89baec">0ab9dcf</a></li>
<li><strong>Files:</strong> <code>Conversion.sol</code>, <code>IConversion.sol</code></li>
</ul>
<p>A specification is available in the <a href="#specification">Specification section</a> of this report.</p>
<h1 id="disclaimer">Disclaimer</h1>
<p>This report aims to provide an overview of the assessed smart contracts' risk exposure and a guide to improving their security posture by addressing identified issues. The audit, limited to specific source code at the time of review, sought to:</p>
<ul>
<li>Identify potential security flaws.</li>
<li>Verify that the smart contracts' functionality aligns with their documentation.</li>
</ul>
<p>Off-chain components, such as backend web application code, keeper functionality, and deployment scripts were explicitly not in-scope of this audit.</p>
<p>Given the unregulated nature and ease of cryptocurrency transfers, operations involving these assets face a high risk from cyber attacks. Maintaining the highest security level is crucial, necessitating a proactive and adaptive approach that accounts for the experimental and rapidly evolving nature of blockchain technology. To encourage secure code development, developers should:</p>
<ul>
<li>Integrate security throughout the development lifecycle.</li>
<li>Employ defensive programming to mitigate the risks posed by unexpected events.</li>
<li>Adhere to current best practices wherever possible.</li>
</ul>
<h1 id="methodology">Methodology</h1>
<p>The audit was conducted using the techniques described below.</p>
<dl>
<dt>Code review</dt>
<dd>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.</dd>
<dt>Dynamic analysis</dt>
<dd>The contracts were compiled, deployed, and tested in a test environment, both manually and through the test suite provided. Dynamic analysis was used to identify additional edge cases, confirm that the code was functional, and to validate the reported issues.</dd>
<dt>Automated analysis</dt>
<dd>Automated tooling was used to detect the presence of various types of security vulnerabilities. Static analysis results were reviewed manually and any false positives were removed. Any true positive results are included in this report.</dd>
</dl>
<h1 id="audit-findings">Audit findings</h1>
<p>The table below provides an overview of the audit's findings. Detailed write-ups are provided below.</p>
<table class="findings">
<thead>
<tr>
<th>ID</th>
<th>Issue</th>
<th>Risk</th>
<th>Status</th>
</tr>
</thead>
<tbody>
<tr>
<td><a href="#IO-SNX-KWN-001">IO-SNX-KWN-001</a></td>
<td>Users can vest more SNX than intended</td>
<td class="rating-high">High</td>
<td class="status-resolved">Resolved</td>
</tr>
<tr>
<td><a href="#IO-SNX-KWN-002">IO-SNX-KWN-002</a></td>
<td>Conversion does not use safeTransfer and safeTransferFrom</td>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
</tr>
</tbody>
</table>
<p>Each issue identified during the audit has been assigned a risk rating. The rating is determined based on the criteria outlined below.</p>
<dl>
<dt>Critical risk</dt>
<dd>The issue could result in the theft of funds from the contract or its users.</dd>
<dt>High risk</dt>
<dd>The issue could result in the loss of funds for the contract owner or its users.</dd>
<dt>Medium risk</dt>
<dd>The issue resulted in the code being dysfunctional or the specification being implemented incorrectly.</dd>
<dt>Low risk</dt>
<dd>A design or best practice issue that could affect the ordinary functioning of the contract.</dd>
<dt>Informational</dt>
<dd>An improvement related to best practice or a suboptimal design pattern.</dd>
</dl>
<p>In addition to a risk rating, each issue is assigned a status:</p>
<dl>
<dt>Open</dt>
<dd>The issue remained present in the code as of the final commit reviewed and may still pose a risk.</dd>
<dt>Resolved</dt>
<dd>The issue was identified during the audit and has since been satisfactorily addressed, removing the risk it posed.</dd>
<dt>Closed</dt>
<dd>The issue was identified during the audit and acknowledged by the developers as an acceptable risk without actioning any change.</dd>
</dl>
<a name="IO-SNX-KWN-001"></a><h2 id="io-snx-kwn-001-users-can-vest-more-snx-than-intended" class="break-before"><strong>IO-SNX-KWN-001</strong> Users can vest more SNX than intended</h2>
<table class="metadata">

<tbody>
<tr>
<td class="rating-high">High</td>
<td class="status-resolved">Resolved</td>
<td><a href="https://github.com/Kwenta/kwenta-snx-conversion/blob/ea0806d128f9d2f91d66af5fbf8e3cc687d6699b/src/Conversion.sol#L87">Conversion.sol#L87</a></td>
</tr>
</tbody>
</table>
<p>A user’s vest-able SNX is calculated by multiplying <code>owedSNX</code> with the proportion of time that has elapsed since the vesting period started relative to the duration of the period. SNX that was already claimed is subtracted from this calculated amount. In the case where the elapsed time is larger than the vesting duration, the proportion is larger than 100% but the resultant amount is capped to the <code>owedSNX</code> amount for the account.</p>
<p>When vesting, the user’s claimed amount is incremented by the capped value returned by <code>vestableAmount()</code>. This results in subsequent calls to <code>vestableAmount()</code> to calculate an owed amount, being at the higher proportion less the claimed amount of 100%. As such, this would allow the user to claim the proportional value beyond 100%.</p>
<p>The below snippet shows an example of an exploitable scenario:</p>
<ul>
<li>TimeElapsed / Vesting Duration = 1.5</li>
<li>snxOwed = 1000</li>
<li>claimed = 0</li>
<li>vestable1 = (1000 * 1.5) - 0 = 1500 (but gets capped to 1000)</li>
<li>claimed = 1000</li>
<li>vestable2 = (1000 * 1.5) - 1000 = 500</li>
<li>totalVested = 1000 + 500 = 1500</li>
</ul>
<p>A PoC was developed to demonstrate how this issue could be exploited - <a href="https://gist.github.com/iosiro-security/8afa1690fc4016d21ca5131aca71c5bd">https://gist.github.com/iosiro-security/8afa1690fc4016d21ca5131aca71c5bd</a></p>
<h3 id="recommendation">Recommendation</h3>
<p><code>vestableAmount()</code> should return 0 when <code>claimedSNX[caller] >= owedSNX[caller]</code>. Additionally, the function should be further refactored to explicitly handle the case where the elapsed time is greater than the <code>LINEAR_VESTING_DURATION</code> duration, and cap the proportion to the unclaimed SNX in that case, as shown in this <a href="https://gist.github.com/iosiro-security/515340f962c0c7b65d56fcac6d0e55f2">Github gist</a></p>
<h3 id="client-response">Client response</h3>
<p>Fixed in <a href="https://github.com/Kwenta/kwenta-snx-conversion/commit/4a766c74f26344a8a281b6f939dc0af5a3213ab2">4a766c7</a> and <a href="https://github.com/Kwenta/kwenta-snx-conversion/pull/3/commits/01730be97b9258fab27a0b601785c4e22c01b275">01730be</a>.</p>
<a name="IO-SNX-KWN-002"></a><h2 id="io-snx-kwn-002-conversion-does-not-use-safetransfer-and-safetransferfrom" class="break-before"><strong>IO-SNX-KWN-002</strong> Conversion does not use safeTransfer and safeTransferFrom</h2>
<table class="metadata">

<tbody>
<tr>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
<td><a href="https://github.com/Kwenta/kwenta-snx-conversion/blob/ea0806d128f9d2f91d66af5fbf8e3cc687d6699b/src/Conversion.sol#L108">Conversion.sol#L108</a>, <a href="https://github.com/Kwenta/kwenta-snx-conversion/blob/ea0806d128f9d2f91d66af5fbf8e3cc687d6699b/src/Conversion.sol#L123">Conversion.sol#L123</a></td>
</tr>
</tbody>
</table>
<p>The <code>safeERC20</code> versions of the token transfer functions should be used instead of <code>transferFrom()</code> and <code>transfer()</code> in the <code>lockAndConvert()</code> and <code>vest()</code> functions. This is to ensure that reverts are thrown on any errors during token transfer.</p>
<h3 id="recommendation-1">Recommendation</h3>
<p><code>safeERC20</code> versions of the token transfer functions should be used.</p>
<h3 id="client-response-1">Client response</h3>
<p>Fixed in <a href="https://github.com/Kwenta/kwenta-snx-conversion/commit/e1a183751d1c750989fc2c7ecc6405671c2866f8">e1a1837</a>.</p>
<h1 id="code-quality-improvement-suggestions">Code quality improvement suggestions</h1>
<p>Code improvement suggestions without security implications are listed below.</p>
<table class="codequality">
<thead>
<tr>
<th>#</th>
<th>Location</th>
<th>Details</th>
</tr>
</thead>
<tbody>
<tr>
<td>1</td>
<td><a href="https://github.com/Kwenta/kwenta-snx-conversion/blob/ea0806d128f9d2f91d66af5fbf8e3cc687d6699b/src/Conversion.sol#L121">Conversion.sol#L121</a></td>
<td>Check that the <code>amountVested</code> value is zero and revert if so. This will prevent zero token amount transfers and corresponding <code>SNXVested</code> events from being emitted.</td>
</tr>
</tbody>
</table>
<h3 id="client-response-2">Client response</h3>
<ol>
<li>Fixed in <a href="https://github.com/Kwenta/kwenta-snx-conversion/commit/0ab9dcf1723e2b7d6610e55b5ee6498a8c89baec">0ab9dcf</a>.</li>
</ol>
<h1 id="specification">Specification</h1>
<p>The following section outlines the system's intended functionality at a high level, based on its implementation in the codebase. Any perceived points of conflict should be highlighted with the auditing team to determine the source of the discrepancy.</p>
<p>A KWENTA to SNX conversion contract was implemented to allow KWENTA token holders to deposit KWENTA to the contract address and receive a set ratio of SNX, as per the terms of the <a href="https://github.com/Kwenta/kwenta-state-log/blob/master/kips/kip-138.md">Kwenta acquisition by Synthetix</a>. Specifically, KWENTA token holders could receive 17 SNX for every KWENTA token. The SNX tokens were subject to a 90 day lock up period and a 9 month vesting period, both of which were enforced by the conversion contract. Once the vesting period was complete, the remaining KWENTA token holders could atomically swap their tokens for the equivalent SNX through this contract. The contract also allows the synthetix treasury address to withdraw SNX from the contract after 2 years.</p>
<h1 id="test-coverage-report">Test coverage report</h1>
<p>The coverage report of the provided tests as on the final day of the audit is given below.</p>
<table>
<thead>
<tr>
<th>File</th>
<th>% Lines</th>
<th>% Statements</th>
<th>% Branches</th>
<th>% Funcs</th>
</tr>
</thead>
<tbody>
<tr>
<td>src/Conversion.sol</td>
<td>100.00% (35/35)</td>
<td>100.00% (43/43)</td>
<td>100.00% (9/9)</td>
<td>100.00% (6/6)</td>
</tr>
</tbody>
</table>
<p>The test coverage was comprehensive.</p>

</article>
</main>
</div>

‍