Staking Pool Fixes and NXM Batch Withdrawal Changes Smart Contract Audit

<defs>

</g>

</symbol>

</defs>

</svg>

</svg>

</div>

</svg>

<div class="frontpage-title">Staking pool fixes and NXM batch withdrawals</div>

<div class="frontpage-subtitle">Nexus Mutual, 21 August 2024</div>

<h1>Contents</h1>

<ul>

<li>

<a href="#introduction">Introduction</a></li>

<li>

<a href="#disclaimer">Disclaimer</a></li>

<li>

<a href="#methodology">Methodology</a></li>

<li>

<a href="#audit-findings">Audit findings</a><ul>

<li>

<a href="#io-nxm-spb-001-censoring-assessors-by-front-running">IO-NXM-SPB-001 Censoring assessors by front-running</a></li>

<li>

<a href="#io-nxm-spb-002-validate-ipfshash">IO-NXM-SPB-002 Validate ipfsHash</a></li>

<li>

<a href="#io-nxm-spb-003-overwriting-of-ipfshash">IO-NXM-SPB-003 Overwriting of ipfsHash</a></li>

</ul>

</li>

<li>

<a href="#code-quality-improvement-suggestions">Code quality improvement suggestions</a></li>

<li>

<a href="#specification">Specification</a><ul>

<li>

<a href="#staking-pool-rewards">Staking pool rewards</a></li>

<li>

<a href="#batch-nxm-withdrawals">Batch NXM withdrawals</a></li>

<li>

<a href="#skip-deallocations-when-burning-in-the-grace-period">Skip deallocations when burning in the grace period</a></li>

<li>

<a href="#staking-pool-metadata">Staking pool metadata</a></li>

</ul>

</li>

</ul>

</nav>

<h1 id="introduction">Introduction</h1>

<p>iosiro was commissioned by Nexus Mutual to perform a smart contract audit of several feature enhancements and bug fixes. The audit was conducted between 12 August 2024 and 19 August 2024 by one auditor for a total of five days.</p>

<h4 id="overview">Overview</h4>

<p>During the audit, a medium-risk issue was identified and subsequently remediated - this issue would have allowed malicious actors to censor assessors. Several code quality and design suggestions were also provided. A breakdown of the issues reported and their status at the conclusion of the audit is provided below:</p>

<thead>

<tr>

<th>Critical</th>

<th>Medium</th>

<th>Informational</th>

</tr>

</thead>

<tbody>

<tr>

</tr>

<tr>

<td>Resolved</td>

</tr>

<tr>

<td>Closed</td>

</tr>

</tbody>

</table>

<h4 id="scope">Scope</h4>

<p>The assessment focused on source file 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 issues or security vulnerabilities.</p>

<ul>

<li><strong>Project name:</strong> Nexus Mutual</li>

<li><strong>Initial audit commit:</strong> <a href="https://github.com/NexusMutual/smart-contracts/tree/7087907b5b6d8bfb83a12755e8f2ce82f56fcb4f">7087907</a></li>

<li><strong>Final review commit:</strong> <a href="https://github.com/NexusMutual/smart-contracts/tree/e62eed05b4e159b105701488458e9e40b94440e9">e62eed0</a></li>

<li><strong>Files:</strong> <code>contracts/modules/assessment/Assessment.sol</code>, <code>contracts/modules/capital/SwapOperator.sol</code>, <code>contracts/modules/cover/Cover.sol</code>, <code>contracts/modules/staking/StakingExtrasLib.sol</code>, <code>contracts/modules/staking/StakingPool.sol</code>, <code>contracts/modules/staking/StakingProducts.sol</code>, <code>contracts/modules/token/TokenController.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>

<thead>

<tr>

<th>Issue</th>

<th>Status</th>

</tr>

</thead>

<tbody>

<tr>

<td>Censoring assessors by front-running</td>

<td class="rating-medium">Medium</td>

<td class="status-resolved">Resolved</td>

</tr>

<tr>

<td>Validate ipfsHash</td>

<td class="status-resolved">Resolved</td>

</tr>

<tr>

<td>Overwriting of ipfsHash</td>

<td class="rating-informational">Informational</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>

<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>

<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>

<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-NXM-SPB-001"></a><h2 id="io-nxm-spb-001-censoring-assessors-by-front-running" class="break-before"><strong>IO-NXM-SPB-001</strong> Censoring assessors by front-running</h2>

<tbody>

<tr>

<td class="rating-medium">Medium</td>

<td><a href="https://github.com/NexusMutual/smart-contracts/blob/eef8c8672b8bebb963a60860fb9ab61d42515193/contracts/modules/assessment/Assessment.sol#L156">Assessment.sol#L156</a></td>

</tr>

</tbody>

</table>

<p>It is possible to front-run calls to <code>Assessment::castVotes(...)</code> with <code>Assessment::unstakeAllFor(...)</code>, preventing assessors from casting votes. An attacker could leverage this to censor specific accounts that might vote against or for a particular claim.</p>

<p>The following unit test was added to illustrate the issue:</p>

<pre tabindex="0" class="chroma"><code><span class="line"><span class="cl"><span class="gh">diff --git a/test/unit/Assessment/unstakeAllFor.js b/test/unit/Assessment/unstakeAllFor.js

</span></span></span><span class="line"><span class="cl"><span class="gh">index b22027d4..08e2719d 100644

</span></span></span><span class="line"><span class="cl"><span class="gh"></span><span class="gd">--- a/test/unit/Assessment/unstakeAllFor.js

</span></span></span><span class="line"><span class="cl"><span class="gd"></span><span class="gi">+++ b/test/unit/Assessment/unstakeAllFor.js

</span></span></span><span class="line"><span class="cl"><span class="gi"></span><span class="gu">@@ -65,6 +65,22 @@ describe('unstakeAllFor', function () {

</span></span></span><span class="line"><span class="cl"><span class="gu"></span>await expect(unstakeForAtExpiry).to.be.revertedWithCustomError(assessment, 'StakeLockedForAssessment');

</span></span><span class="line"><span class="cl">});

</span></span><span class="line"><span class="cl">

</span></span><span class="line"><span class="cl"><span class="gi">+ it('should not be possible to front-run user votes', async function () {

</span></span></span><span class="line"><span class="cl"><span class="gi">+ const fixture = await loadFixture(setup);

</span></span></span><span class="line"><span class="cl"><span class="gi">+ const { assessment, individualClaims } = fixture.contracts;

</span></span></span><span class="line"><span class="cl"><span class="gi">+ const [user, otherUser] = fixture.accounts.members;

</span></span></span><span class="line"><span class="cl"><span class="gi">+ const amount = parseEther('100');