Nexus Mutual Swap Operator Asset to Asset Smart Contract Audit

</nav>
<article class="report">
<h1 id="introduction">Introduction</h1>
<p>iosiro was commissioned by Nexus Mutual to conduct a smart contract audit of the SwapOperator contract's asset-to-asset swap function. Two auditors performed the audit between 12 March 2024 and 15 March 2024, using five resource days.</p>
<h4 id="overview">Overview</h4>
<p>This report presents the findings of an audit performed by iosiro on the Nexus Mutual SwapOperator Asset-To-Asset Swap feature enhancement.</p>
<p>The security posture of the in-scope assets was found to be of a high standard. A low-risk issue related to the maximum fee for small trades was raised. Additionally, several recommendations were provided to enhance the gas performance, readability and maintainability of the code base.</p>
<p>The table below shows the status of reported findings at the conclusion of the assessment.</p>
<table class="findings-count">
<thead>
<tr>
<th></th>
<th>Critical</th>
<th>High</th>
<th>Medium</th>
<th>Low</th>
<th>Informational</th>
</tr>
</thead>
<tbody>
<tr>
<th>Open</th>
<td>0</td>
<td>0</td>
<td>0</td>
<td>0</td>
<td>0</td>
</tr>
<tr>
<th>Resolved</th>
<td>0</td>
<td>0</td>
<td>0</td>
<td>0</td>
<td>0</td>
</tr>
<tr>
<th>Closed</th>
<td>0</td>
<td>0</td>
<td>0</td>
<td>1</td>
<td>0</td>
</tr>
</tbody>
</table>
<p>At a high level, the following actions should be taken to ensure the security posture of the Nexus Mutual protocol remains robust:</p>
<ul>
<li>Performing additional audits regularly, as security best practices, tools, and knowledge change over time. Additional audits throughout the project's lifespan ensure the longevity of the codebase.</li>
<li>Maintaining and expanding the existing bug bounty program to encourage the responsible disclosure of security vulnerabilities in the system.</li>
</ul>
<hr />
<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> SwapOperator Asset-to-Asset Swap</li>
<li><strong>Commits:</strong> <a href="https://github.com/NexusMutual/smart-contracts/tree/bf2679e5402dc6437fb2a661ec11065fc22a92c3">bf2679e</a>, <a href="https://github.com/NexusMutual/smart-contracts/tree/81487e9f553848a8d70a8f448dc710bbc227d906">81487e9</a></li>
<li><strong>Files:</strong> SwapOperator.sol</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 aligned with the provided 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 which takes into account 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-NXM-ATA-001">IO-NXM-ATA-001</a>
</td>
<td>Include the swap fee in the slippage calculation</td>
<td class="rating-low">Low</td>
<td class="status-closed">Closed</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 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>
<p><a name="IO-NXM-ATA-001"></a></p>
<h2 class="break-before" id="io-nxm-ata-001-include-the-swap-fee-in-the-slippage-calculation"><strong>IO-NXM-ATA-001</strong> Include the swap fee in the slippage calculation</h2>
<table class="metadata"><tbody><tr><td class="rating-low">Low</td><td class="status-closed">Closed</td><td><em><a href="https://github.com/NexusMutual/smart-contracts/blob/bf2679e5402dc6437fb2a661ec11065fc22a92c3/contracts/modules/capital/SwapOperator.sol#L124">SwapOperator.sol#L124</a></em></td></tr></tbody></table>
<p>The <code>maxFee</code> is denoted as an absolute, and a minimum trade size is not enforced. A trade size minimum is necessary to avoid disproportionately high fees for smaller transactions when only implementing a maximum fee.</p>
<h3 id="recommendation">Recommendation</h3>
<p>The slippage calculation should be revised to deduct the fee from the sell token's amount. Incorporating the fee directly into the slippage calculation ensures a more accurate representation of the trade's cost-effectiveness, particularly for smaller transactions.</p>
<h3 id="client-response">Client response</h3>
<p>The development team acknowledged the issue but indicated that the risk does not warrant increasing the complexity of the slippage control mechanism.</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>General</td>
<td>Constants used throughout the protocol are often redeclared, for example: <code>ETH</code> and <code>MAX_SLIPPAGE_DENOMINATOR</code>. Protocol-wide constants should be declared in a single source file and imported where needed to mitigate the risk of mismatched values between contracts.</td>
</tr>
<tr>
<td>2</td>
<td><a href="https://github.com/NexusMutual/smart-contracts/blob/bf2679e5402dc6437fb2a661ec11065fc22a92c3/contracts/modules/capital/SwapOperator.sol#L450">Swap<wbr>Operator.sol<wbr>#450</a></td>
<td>The internal functions <code>validateMaxFee(...)</code> and <code>executeAssetTransfer(...)</code> both require the <code>PriceFeedOracle</code>. Passing the address of the <code>PriceFeedOracle</code> as an argument to <code>executeAssetTransfer(...)</code>, <code>performPreSwapValidations(...)</code>, and <code>validateMaxFee(...)</code> avoids unnecessary calls to <code>_pool()</code> and <code>pool.priceFeedOracle()</code>.</td>
</tr>
<tr>
<td>3</td>
<td><a href="https://github.com/NexusMutual/smart-contracts/blob/bf2679e5402dc6437fb2a661ec11065fc22a92c3/contracts/modules/capital/SwapOperator.sol#L377-L381">Swap<wbr>Operator.sol<wbr>#377,381</a></td>
<td>The internal function <code>returnAssetToPool(...)</code> calls <code>_pool()</code>. It is only called from <code>closeOrder(...)</code>, which also calls <code>_pool()</code> on line 357. <code>_pool()</code> could instead be called once, higher in the function, and its value stored, passed to both <code>returnAssetToPool(...)</code> calls and then used again on line 357.</td>
</tr>
<tr>
<td>4</td>
<td><a href="https://github.com/NexusMutual/smart-contracts/blob/bf2679e5402dc6437fb2a661ec11065fc22a92c3/contracts/modules/capital/SwapOperator.sol#L142">Swap<wbr>Operator.sol<wbr>#142</a></td>
<td>The <code>minAmount == 0</code> check could be removed, as checking <code>maxAmount == 0</code> should be sufficient to determine that the token is not enabled.</td>
</tr>
<tr>
<td>5</td>
<td><a href="https://github.com/NexusMutual/smart-contracts/blob/bf2679e5402dc6437fb2a661ec11065fc22a92c3/contracts/modules/capital/SwapOperator.sol">Swap<wbr>Operator.sol</a></td>
<td>The SwapOperator contract uses a mixture of <code>require</code> statements and custom errors. To save gas and improve code consistency, <code>require</code> statements could be replaced with new custom errors.</td>
</tr>
<tr>
<td>6</td>
<td><a href="https://github.com/NexusMutual/smart-contracts/blob/bf2679e5402dc6437fb2a661ec11065fc22a92c3/contracts/modules/capital/SwapOperator.sol#L284">Swap<wbr>Operator.sol<wbr>#284</a></td>
<td>The <code>return</code> statement at the end of <code>executeAssetTransfer(...)</code> is unnecessary, as the function uses a named return.</td>
</tr>
<tr>
<td>7</td>
<td>General</td>
<td><code>maxFee</code> should be renamed to match the convention used for all other constants, e.g. <code>MAX_FEE</code>.</td>
</tr>
<tr>
<td>8</td>
<td><a href="https://github.com/NexusMutual/smart-contracts/blob/bf2679e5402dc6437fb2a661ec11065fc22a92c3/contracts/modules/capital/SwapOperator.sol#L342-L343">Swap<wbr>Operator.sol<wbr>#342,343</a></td>
<td>The comment on line 342 reads "Check how much of the order was filled, and if it was fully filled"; however, the function only retrieves the filled amount and emits it in an event – no logic has been implemented to determine whether the order has been fully filled. If this is intentional, the comment should be altered.</td>
</tr>
</tbody>
</table>
<h1 id="specification">Specification</h1>
<p>The following section outlines the intended functionality of the system at a high level, based on the implementation in the codebase. Any perceived discrepancies with the <a href="https://docs.google.com/document/d/1xqIjJJOZPlvbMqHLTXffpYFLvSCxuy_V5BnvN337u40">official design specifiction provided</a> by Nexus Mutual should be raised with the auditing team.</p>
<p>Initially, the SwapOperator was limited to facilitating exchanges between ETH and supported assets through the <a href="https://swap.cow.fi/">CowSwap</a> protocol. This functionality was expanded to include swaps between different assets to simplify and speed up the process of rebalancing the Pool's assets.</p>
<h2 id="submitting-an-order">Submitting an order</h2>
<p>The SwapOperator enables the swap controller to submit orders calculated off-chain. These orders specify the transaction fee, type of order, and the amounts of the tokens desired. An order is only eligible for submission if the Pool's token balances are outside their target ranges (with the sell token exceeding the maximum desired and the buy token falling below the minimum desired), and the swap will adjust both balances back into these ranges. Governance sets the boundaries, and in conjunction with a cooldown period for each asset, they regulate the actions of the swap controller.</p>
<p>The system accepts both sell and buy order types. The output amount is predetermined for sell orders, whereas for buy orders, the amount of tokens to receive is fixed. Chainlink's oracles are queried to ensure the order's exchange rate falls within an acceptable slippage margin. Notably, the fee (always in the sell token) is not factored into the slippage calculation; a maximum fee limit in Ether is applied instead.</p>
<p>The SwapOperator secures the Ether value of the tokens moved from the Pool for the duration of the order, ensuring that these funds are accounted for in the Pool's total assets.</p>
<p>Orders are settled asynchronously, which means the SwapOperator can handle only one active order at a time. The existing one must be finalized before submitting a new order, regardless of its status—whether it's expired, partially filled, or fully completed.</p>
<h2 id="closing-an-order">Closing an order</h2>
<p>Before an order's expiration, it can only be cancelled and closed by the swap controller. Closing an order involves:</p>
<ul>
<li>Returning any leftover tokens to the Pool.</li>
<li>Withdrawing the order's pre-signature.</li>
<li>Invalidating the order.</li>
</ul>
<p>As an additional security measure, the token allowance granted to the CowSwap protocol is reset to prevent the unintended transfer of assets in future orders.</p>

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

‍