iosiro was commissioned by Nexus Mutual to conduct a smart contract audit of their v2 contracts.
Three auditors performed the initial audit from 30 November to 16 December 2022. The changes made to the codebase since the initial audit, which included the fixes for the issues reported, were reviewed from 13 February 2023 to 22 February 2023 and again on 6 March 2023. This report summarises the findings of the initial audit and the additional issues identified and remediated during the review.
This report is organized into the following sections.
- Section 2 - Executive summary: A high-level description of the findings of the audit.
- Section 3 - Audit details: A description of the scope and methodology of the audit.
- Section 4 - Design specification: An outline of the intended functionality of the smart contracts.
- Section 5 - Detailed findings: Detailed descriptions of the findings of the audit.
The information in this report should be used to better understand the smart contracts' risk exposure 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 that considers blockchain technologies' new and experimental nature. 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.
The audits uncovered seven high-risk issues and two medium-risk issues. By the conclusion of the audits, none of the high-risk or medium-risk issues remained unresolved.
The first high-risk issue allowed an attacker to forcefully lock other users' funds within a staking pool if the users had set an allowance for the TokenController contract.
Two of the high-risk issues pertained to the allocation of staking capacity when buying cover. Both prevented the system from deallocating capacity in the staking pools when editing cover. A related issue was identified during the retest, which could have prevented users who had edited their cover from receiving payouts.
During the review, it was found that the users were prevented from claiming rewards more than once, which could result in inconsistent reward distributions.
The remaining two high-risk issues were due to the cover premium not being sent to the pool when buying cover using ETH, as well as changes to the contracts that would prevent the migration of any existing cover to the new system.
One of the medium-risk issues allowed users to extract their NXM tokens despite being locked in governance voting. Another medium-risk issue was due to the incorrect calculation of the premium when editing an existing cover.
The remaining medium-risk issue was due to all expired cover being included in the calculation of the MCR. The inflated MCR would affect the NXM token price and impact the premium for any additional cover bought by members.
Two low-risk issues and multiple informational items were also identified. The low-risk issues were generally related to additional security checks that should be performed. The informational items included possible gas improvements and overall design considerations.
The source code considered in-scope for the assessment is described below. 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.
Project Name: Nexus Mutual
Initial Commit: 6522da1
Review Commit:: 9bb3650
Final Commit: 2415e08
Files: All contracts under the contracts/modules folder were considered in-scope for the audit.
A variety of techniques were used to perform the audit. These techniques are briefly described below.
The source code was manually inspected to identify potential security flaws. Code review is a helpful approach for detecting security flaws, discrepancies between the specification and implementation, design improvements, and high-risk areas of the system.
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 to identify security issues that could be exploited.
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.
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 the contract owner or system users losing funds.
- 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.
The Nexus Mutual V2 codebase is functionally separated into modules. The system is intended to be used by members of the mutual fund. The native token, NXM, can only be bought and sold through Nexus Mutual, and only by members. It is possible to mint wrapped NXM, which can be freely traded on-chain by any account, but this is not relevant to the functioning of Nexus Mutual.
The different modules are summarized as follows:
-
Assessment: The assessment module allows accounts with cover to submit claims following potential incidents. Members of the mutual can then vote on the assessments, either approving or declining the claim. The vote weights are a function of the amount of NXM the member has staked through the
Assessmentcontract. -
Capital: The capital module is where members will primarily buy and sell their NXM. It also holds the funds that will be paid out to successful claims and keeps track of the minimum amount of capital required by the system to service any potential claims based on the amount of coverage that has been issued.
-
Cover: The cover module allows members to buy new cover and edit their existing cover. It also allows for the creation of staking pools and management of cover products.
-
Governance: The governance module allows governance proposals to be submitted, voted on, and executed. Members participating in governance can have one of four roles: Advisory Board, Owner, Member, or Unassigned. Each role has a different set of governance privileges.
-
Legacy: The legacy module provides a set of contracts that allow interoperability between V1 and V2. This code is intended to facilitate the transition to V2 rather than to allow long-term usage of V1.
-
Staking: The staking module allows members to stake their NXM, which will be used to allocate cover capacity to users buying cover. Users stake their NXM in tranches, which implicitly locks their NXM for the specified staking period.
-
Token: The token module contains the implementation of the NXM token and the token controller. The
TokenControllercontract is responsible for routing NXM throughout the system and issuing rewards to stakers.
Through Cover::buyCover(), users can buy insurance cover for the various products the mutual supports. The user specifies the amount of cover required, the period, their chosen payment asset, and the various staking pool allocations for the desired cover. The cover's premium is the sum of the premiums calculated across the selected staking pools. The premiums are greatly influenced by the capacity made available by each staking pool for the desired product and the current demand for the specific type of cover requested.
When buying cover, the user is issued with an ERC721 token that represents their cover. The token is transferable, but cover can only be claimed by members of the mutual. All claims are handled by the set of contracts included in the Assessments module.
As part of the V2 release, the Cover contract contains the Cover::migrateCovers() function to transfer covers from the legacy system. This process will invalidate the previous covers and issue the owners with equivalent cover within the new system.
Members are encouraged to stake their NXM tokens in exchange for rewards in NXM tokens. Members will stake their NXM up to a desired tranche using the StakingPool::depositTo() function. Each tranche lasts 91 days, and staking can be performed for up to 8 tranches in the future. Each staker is issued an ERC721 token to represent their stake, which is transferable.
The stake can then either be withdrawn using the StakingPool::withdraw() function, or the staker can extend their staking window before expiry to additional tranches using the StakingPool::extendDeposit() function. Staked amounts can be withdrawn when the pool manager is not be locked in governance voting and the tranche for the stake has expired. Stakers are not permitted to shorten their staking duration.
The assessments module allows users to submit claims through either IndividualClaims::submitClaim() or YieldTokenIncidents::submitIncident(), depending on the product the user has cover for. Stakers of the assessments module are then considered assessors, and assessors can vote on the claim's validity, given any supporting evidence supplied during the claim submission. If the outcome of the assessment is favorable towards the claimant, the claimant can then call IndividualClaims::redeemClaimPayout() or YieldTokenIncidents::redeemPayout() to redeem their claim payout.
The governance contracts manage the membership of the mutual and the interdependencies of the system contracts. Members can create proposals that all members can vote to accept or deny. Specific proposal categories require approval from the Advisory Board, a small group of members that provides additional oversight to the system's operation.
A significant change in the V2 upgrade is how members join the mutual. New members are now required to perform off-chain Know Your Customer (KYC) validation before attempting to join using the MemberRoles::join() function. An off-chain application service will sign attestations which must be submitted alongside the joining request and fee. The previous implementation required users to first pay the joining fee and then apply for membership and thus had to support reimbursements for users whose applications were unsuccessful. The new joining mechanism greatly simplifies the on-chain logic.
The following section includes in-depth descriptions of the findings of the audit.
No high-risk issues were present at the conclusion of the review.
No medium-risk issues were present at the conclusion of the review.
The PriceFeedOracle::_getAssetToEthRate() function did not check for stale rates. This could lead to inconsistencies in the system's asset pricing. Since all significant system actions require valid asset prices to calculate asset values in terms of ETH, an invalid or stale asset price could lead to an incorrect calculation of the pool value and NXM token price.
A staleness check should be performed in PriceFeedOracle::_getAssetToEthRate() to ensure that rates are current during operation. As a general guideline, validation should ensure that the last price update occurred within the heartbeat interval of the relevant Price Feed Aggregator.
The heartbeat interval of each Chainlink Price Feed is published on Chainlink's documentation website and is visible when toggling the "Show more details" checkbox:
https://docs.chain.link/data-feeds/price-feeds/addresses
Nexus Mutual has acknowledged and accepted the risk associated with potentially stale oracle rates. Instead of on-chain mitigations, the team has indicated that they will use off-chain monitoring and alerting solutions.
The amount of cover per allocation is rounded up to the nearest allocation unit at Cover.sol#L346. This breaks the separation of concerns between the contracts. Instead, StakingPool::requestAllocation() should be responsible for converting the value and returning coverAmountInNXM to the premium, which can then be validated within Cover.
The ERC20::decimals() property is of type pure view in most implementations and does not access state. Since the property is always used in conjunction with the balanceOf() function, calls to decimals() will only result in static calls to hot addresses, which only costs 100 gas instead of a cold SLOAD that costs 2100 gas.
At the time of the audit, all investment assets in the Pool and supported cover assets implemented decimals() as either public constant or pure view. Therefore it would be more gas efficient to not store each asset's decimals locally but rather to defer to the tokens' implementations.
In the Cover::buyCover() function, the AllocationRequest struct is populated with a zero-valued coverId property. This is because the coverId parameter passed to the function was not initialized. As a result, in the subsequent call to StakingPool::getActiveAllocationsWithoutCover(), the coverId will always be zero, leading to incorrect staking pool allocations.
The coverId should be set after a new coverId has been obtained or after the coverId of the existing cover is validated.
The issue was addressed per the recommendation in commit 983fd4c.
When creating a new staking pool using Cover::createStakingPool(), the caller can specify an arbitrary manager of that staking pool and an initial deposit into the staking pool. The StakingPool::depositTo() function is called internally, which transfers the initial stake from the manager using TokenController::operatorTransfer().
As no permissions are required to specify a manager, a malicious actor could create a staking pool and provide another user's address as its manager. If this address holds NXM tokens and has provided the TokenController a transfer allowance, the initial deposit amount specified by the malicious user would be transferred from the victim's address to the staking pool and locked for up to two years.
The initial deposit functionality should be removed, or it should be impossible to create staking pools where the creator is not the manager.
The issue was addressed by removing the initial deposit when creating a staking pool in commit c700161.
When updating a cover, the total amount is reallocated even if allocations have already been made. As a result, the coverTrancheAllocations mapping is never updated with new entries. Thereby, the Cover::getActiveAllocationsWithoutCover() function always returns the same result as Cover::getActiveAllocations(), effectively ignoring any existing cover.
The issue was validated by modifying the Cover::getActiveAllocationsWithoutCover() function to be public and comparing the values returned before and after editing the cover.
The following Foundry test case was developed to illustrate the issue:
uint coverId = type(uint).max;
buyCoverParams.coverId = coverId;
for (uint i = 0; i < 2; i++) {
coverId = cover.buyCover(buyCoverParams, allocations);
buyCoverParams.coverId = coverId;
activeAllocations = stakingPool.getActiveAllocationsWithoutCover(0, coverId, block.timestamp, block.timestamp + 60 days);
totalActiveAllocations = 0;
for (uint i = 0; i < activeAllocations.length; i++) {
totalActiveAllocations += activeAllocations[i];
}
console.log("getActiveAllocationsWithoutCover: ", totalActiveAllocations);
// i == 0, prints 4
// i == 1, prints 8
vm.warp(block.timestamp + 1 days);
}Logic should be added to update the coverTrancheAllocations mapping whenever creating or editing cover.
Functionality was added to accurately track allocations when editing cover in commit ed319b.
When a user opts to pay their cover premium using ETH, the payment is never transferred to the pool. Furthermore, there is no mechanism to transfer the ETH from the Cover contract to the Pool. As a result, the funds are effectively lost and cannot be used for their intended purpose.
The Cover::retrievePayment() function should be revised to ensure the ETH premium is sent to the Pool.
--- a/contracts/modules/cover/Cover.sol
+++ b/contracts/modules/cover/Cover.sol
@@ -422,6 +422,12 @@ contract Cover is ICover, MasterAwareV2, IStakingPoolBeacon, ReentrancyGuard {
require(ok, "Cover: Returning ETH remainder to sender failed.");
}
+ // send premium
+ if (premiumInPaymentAsset > 0) {
+ (bool ok, /* data */) = address(pool).call{value: premiumInPaymentAsset}("");
+ require(ok, "Cover: Sending ETH to Pool.");
+ }
+
// send commission
if (commission > 0) {
(bool ok, /* data */) = address(commissionDestination).call{value: commission}("");The issue was addressed per the recommendation in commit 0e97b06.
The TokenController contract is deployed using a delegate proxy and will be upgraded as part of the Nexus V2 release. This contract's storage layout was modified, resulting in a mismatch of the existing coverInfo mapping's storage slot. As a result, all existing coverInfo entries would become inaccessible, preventing migration of all existing cover.
The Foundry test case provided below was developed to record the storage slots accessed when interacting with the existing implementation versus the new implementation.
function testStorage() public {
TokenController newControllerImpl = new TokenController(address(0),address(0));
TokenController oldControllerImpl = TokenController(0xcafea8cF7044dcfe97fb33D32DA71D0f3fe3053f);
vm.record();
oldControllerImpl.coverInfo(0);
vm.accesses(address(oldControllerImpl));
vm.record();
newControllerImpl.coverInfo(0);
vm.accesses(address(newControllerImpl));
}The results of the test case are shown below:
[PASS] testStorage() (gas: 2977180)
Traces:
[2977180] CoverTest::testStorage()
├─ [2927025] → new TokenController@0x210503c318855259983298ba58055A38D5FF63E0
│ └─ ← 14617 bytes of code
├─ [0] VM::record()
│ └─ ← ()
├─ [2555] 0xcafea8cF7044dcfe97fb33D32DA71D0f3fe3053f::coverInfo(0) [staticcall]
│ └─ ← 0, false, false
├─ [0] VM::accesses(0xcafea8cF7044dcfe97fb33D32DA71D0f3fe3053f)
│ └─ ← [0x5eff886ea0ce6ca488a3d6e336d6c0f75f46d19b42c06ce5ee98e42c96d256c7], []
├─ [0] VM::record()
│ └─ ← ()
├─ [2612] TokenController::coverInfo(0) [staticcall]
│ └─ ← 0, false, false
├─ [0] VM::accesses(TokenController: [0x210503c318855259983298ba58055A38D5FF63E0])
│ └─ ← [0x13da86008ba1c6922daee3e07db95305ef49ebced9f5467a0b8613fcc6b343e3], []
└─ ← ()
As can be observed, the storage slot index between the two calls differs. The slot indexes can be manually calculated for coverInfo[0] to confirm the issue.
# Array index 0 of mapping at slot 8
$ cast index uint256 0 8
0x5eff886ea0ce6ca488a3d6e336d6c0f75f46d19b42c06ce5ee98e42c96d256c7
# Array index 0 of mapping at slot 10
$ cast index uint256 0 10
0x13da86008ba1c6922daee3e07db95305ef49ebced9f5467a0b8613fcc6b343e
The storage slot position of coverInfo should be maintained. At the time of the audit, the _unused4 storage slot corresponded with the existing coverInfo slot.
The issue was addressed per the recommendation in commit 4d3bdf9.
When claiming staking rewards, the rewardsShares of a user's deposit are zeroed, preventing further accumulation of rewards. Furthermore, the incorrect value was stored in lastAccNxmPerRewardShare, which could result in an underflow when attempting to claim rewards again.
Stakers should be able to claim rewards continuously. The total amount of rewards received should be independent of whether a staker withdraws rewards repeatedly during their staking period or after it expires.
When claiming rewards, the deposit's rewardsShares should remain unaffected, and the lastAccNxmPerRewardShare should be set to accNxmPerRewardShareToUse rather than the current reward rate.
The issue was addressed per the recommendation in commit abb73b8.
When editing some cover it is possible to modify the period or staking pool allocation amounts; however, the previous cover segment can still be claimed against until the expiration of the cover's grace period. Consequently, the staking pools might no longer have the required capacity reserved to burn, resulting in a revert.
The following Foundry test case was developed to illustrate the issue:
pragma solidity ^0.8.16;
import "./Bootstrap/Deployment.t.sol";
contract CoverTest is SystemDeployment {
function setUp() public {
bootstrap();
}
function testEditCover() public {
uint currentTranche = block.timestamp / 91 days;
stakingPools[1].depositTo(100 ether, currentTranche + 4, 1, address(0));
PoolAllocationRequest[] memory allocations = new PoolAllocationRequest[](1);
allocations[0] = PoolAllocationRequest({
poolId: 1,
coverAmountInAsset: 3 ether,
skip: false
});
BuyCoverParams memory params = BuyCoverParams({
coverId: 0,
owner: address(this),
productId: 0,
coverAsset: uint8(1),
amount: 1 ether,
period: 120 days,
maxPremiumInAsset: 100 ether,
paymentAsset: 1,
commissionRatio: 0,
commissionDestination: address(0),
ipfsData: ""
});
uint coverId = cover.buyCover(params, allocations);
vm.warp(block.timestamp + 30 days);
// Reduce cover from 30 ether to 3 ether
allocations[0].coverAmountInAsset = 300 ether;
params.coverId = coverId;
params.period = 90 days; // 30 + 90
// Edit cover
cover.buyCover(params, allocations);
// Enter grace period
vm.warp(block.timestamp + 1 days);
// Claim against segment 0 - which had cover amount == 30 ether
individualClaims.submitClaim{value: 1 ether}(uint32(coverId), 1, 30 ether, "");
{
uint[] memory assessmentIds = new uint[](1);
bool[] memory votes = new bool[](1);
string[] memory ipfsAssessmentDataHashes = new string[](1);
assessmentIds[0] = 0;
votes[0] = true;
ipfsAssessmentDataHashes[0] = "";
assessment.castVotes(assessmentIds, votes, ipfsAssessmentDataHashes, 1 ether);
}
// Close voting period
vm.warp(block.timestamp + 10 days);
// Encounter underflow
individualClaims.redeemClaimPayout(0);
}
receive() payable external {
}
}The above test case reverts when attempting call redeemClaimPayout() due to a underflow at StakingPool.sol#L1051.
Cover editing should be revised to ensure that provision is made for the original cover segment's grace period. This can be achieved by creating an allocation for the difference and which expires at the end of the original segment's grace period.
Alternatively, claiming against a previous cover segment during its grace period should not be permitted.
The a revert was added to buyCover to prevent editing of existing cover as per the recommendation in commit c48f33c.
To avoid double-voting attacks, NXM tokens can only be transferred when the owner of the tokens is not locked in governance voting. This is done by checking the msg.sender using the NXMToken::isLockedForMV() function when calling StakingPool::depositTo().
When creating a new staking pool with an initial deposit, the StakingPool::depositTo() function verifies that msg.sender is not locked in governance voting, but draws the initial stake from the pool manager. As msg.sender will in this instance be the Cover contract, this check will pass even if the manager address is locked in governance voting. This would allow a malicious account to vote in governance but transfer their NXM tokens to a new staking pool.
If msg.sender within StakingPool::depositTo() is the Cover contract, the isLockedForMV() check should be performed for the manager and not msg.sender.
The issue was addressed by removing the initial deposit when creating a staking pool in commit c700161.
Whenever cover is bought, the cover amount is added to totalActiveCoverInAsset for the cover asset. The values stored in the mapping are used to calculate the Minimal Capital Requirement (MCR) of the mutual and should only track cover that is still active. No logic was present in the Cover contract to reduce the amount stored when the cover expires. This will increase the MCR and, over a long time, could result in users needing help to buy or sell their NXM and inflate the cover premiums.
Logic should be implemented to reduce the amount of expired cover from the totalActiveCoverInAsset mapping.
Deallocation of expired cover using buckets was noted as TODO in the source code, which is understood to be a known issue. Once the mechanism for tracking active cover using buckets is completed, it is recommended that the functionality is thoroughly tested and audited.
The functionality was implemented and audited as part of the final review.
When editing the existing cover, the premium paid on the remaining portion of the initial cover is subtracted from the new premium. This refund is calculated as the ratio between the time left and the initial cover period. Unfortunately, the calculation incorrectly calculates the initial period due to missing parentheses.
The affected code is provided below for reference:
vars.previousPremiumInNXM = previousPoolAllocation.premiumInNXM;
vars.refund =
previousPoolAllocation.premiumInNXM
* (allocationRequest.previousExpiration - block.timestamp) // remaining period
/ allocationRequest.previousExpiration - allocationRequest.previousStart; // previous period
allocationRequest.previousExpiration - allocationRequest.previousStart should be enclosed in parentheses to preserve the order of operations.
Fixed in commit e95b493.
The manner in which the premium for existing cover is recalculated contains an off-by-one error. This can, under specific circumstances, result in an arithmetic underflow.
When buying cover, the start time is set to block.timestamp + 1, but the premium is calculated using block.timestamp, making the premium greater than initially calculated.
When buying cover, the previous premium should be calculated using the timestamp at which the cover segment starts instead of the current block.timestamp.
The Cover contract was modified to start cover segments at the current block.timestamp instead of one second later in commit fee2480.
The sum of allocations is not validated to satisfy the amount specified in Cover::buyCover(). Although the amount parameter is validated to be greater than 0, it is redundant in the current implementation.
The sum of the allocations should be validated to match the amount specified, to ensure that the desired cover amount is bought.
require(totalCoveredAmountInPayoutAsset == params.amount, "Insufficient cover");Validation that the total sum of allocations when buying cover is greater or equal to the specified amount was added in commit 40d5fbc.
The calculations at Pool.sol#L141 should be replaced with priceFeed.getEthForAsset(...) to avoid reimplementing existing logic:
--- a/contracts/modules/capital/Pool.sol
+++ b/contracts/modules/capital/Pool.sol
@@ -138,10 +138,7 @@ contract Pool is IPool, MasterAwareV2, ReentrancyGuard {
return 0; // ETH
}
- uint rate = priceFeedOracle.getAssetToEthRate(assetAddress);
- require(rate > 0, "Pool: Zero rate");
-
- return assetBalance * rate / (10 ** uint(assetDecimals)); // ETH
+ return priceFeedOracle.getEthForAsset(assetAddress, assetBalance);
}Fixed in commit 289f01a.
The functions MCR::min() and MCR::max() should be removed and the existing implementations in the SafeMath library should be used instead. This will prevent unnecessary code duplication.
Fixed in commit 80bad1e.