Nexus Mutual Distributor Smart Contract Audit
iosiro was commissioned by Nexus Mutual to conduct a smart contract audit of the Distributor and DistributorFactory smart contracts. The audit was performed between 15 June and 18 June 2021, consuming a total of 3 resource days. A review of the changes made to address the findings of the audit was performed on 29 June 2021.
This report is organized into the following sections.
The information in this report should be used to better understand the risk exposure of the smart contracts, and as a guide to improving the security posture of the smart contracts by remediating issues identified. The results of this audit are only a reflection of the source code reviewed at the time of the audit and of the source code that was determined to be in-scope.
The purpose of this audit was to achieve the following:
Assessing the off-chain functionality associated with the contracts, for example, backend web application code, was out of 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 very high risk with regards to 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:
This report presents the findings of an audit performed by iosiro on the Distributor and DistributorFactory smart contracts. The contract were previously audited and released on the Ethereum mainnet, but changes were required with the introduction of Yield Token Cover.
The contracts in scope were also moved from a separate repository to Nexus Mutual's primary smart contract repository.
The security posture of the Distributor and DistributorFactory were found to be of a high standard. Only informational issues and potential code improvements were identified within the contracts.
A discrepancy was identified in the manner in which the NXM deposit was locked when buying Yield Token Cover. The issue stemmed from the NXM deposit being immediately minted to the member's address, resulting in an effective 10% discount on the cover premiums. This issue was rated as a low risk.
All issues identified during the audit were either remediated or suitably addressed.
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.
A variety of techniques, described below, were used to conduct the audit.
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.
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 discover whether any potential security issues identified could be exploited. The coverage report of the provided Hardhat tests as on the final day of the audit is given below.
| File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Lines |
|---|---|---|---|---|---|
| Distributor.sol | 98.91 | 83.33 | 100 | 98.92 | 266 |
| DistributorFactory.sol | 100 | 100 | 100 | 100 |
Test coverage was comprehensive, with only a single low-risk statement not covered.
Tools were used to automatically detect the presence of several types of security vulnerabilities, including reentrancy, timestamp dependency bugs, and transaction-ordering dependency bugs. The static analysis results were manually reviewed and any false positives were removed from the results. Any true positive results were 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.
The following section outlines the intended functionality of the system at a high level. The specification is based on the implementation in the codebase and any perceived points of conflict should be highlighted with the auditing team to determine the source of the discrepancy.
The factory allowed any user to deploy and pay the joining fee for new Distributor instances. The ownership of the newly created instances would automatically be transferred to the user. However, the instance would still need to follow the usual Know Your Customer (KYC) process and become a member, before it would be able to buy and sell NXM or sell cover to non-members.
The factory exposed a single external function call, newDistributor which would accept the desired fee percentage, address to distribute unlocked deposit, the fees and some fields to uniquely identify cover created via the Distributor.
The Distributor smart contract was developed to allow members to resell cover to non-members and take a fee as well as the NXM deposits as reward.
The major benefit added is the ability to represent cover as Non-Fungible Tokens according to the ERC-721 specification. This enables users to buy and sell cover on various exchanges or transfer it to another address, without the need to become members of the mutual.
Since the Distributor is linked to a member, some functionality is exposed to allow the owner to still participate in the Nexus Mutual ecosystem. Such as buying and selling or transferring any NXM belonging to the contract. Consequently, special consideration was given to ensure that none of the users' premiums were kept in the contract or could be siphoned by the owner.
It should be noted that voting and staking is not supported via the Distributor.
Lastly, the contract exposed a function executeCoverAction that the owner could invoke to perform potentially arbitrary actions for the cover bought. However, the corresponding functionality in the Gateway contract did not support any actions at the time of review and would revert for all actions. Users should take note that the actions available to the owner might change over time, without any changes to Distributor contract's code. It was assumed that the implications for cover holders via Distributors will be carefully considered when enabling actions.
The following section details 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.
No low risk issues were present at the conclusion of the review.
No Informational issues were present at the conclusion of the review.
During the audit, the behavior of Gateway.buyCover(...) for Yield Token Cover was revisited. A discrepancy in the manner in which cover deposits are minted and locked was noticed.
In context of the Distributor contract, this would allow an owner to transfer their membership while users still had cover. Users would still be able to claimTokens using the Distributor, but this is undesired behavior.
Furthermore, it was confirmed that (as per comments in the code) the NXM deposit on cover was immediately minted to the member's wallet for Yield Token Cover. The minted NXM tokens could be transferred, sold or staked as normal. This effectively reduced the premium of Yield Cover Tokens by 10%. During a previous audit, this behavior was assumed to be by design; however, the wider implications have become apparent and should be addressed.
The Quotation._makeCover(...) function should create cover notes for Yield Token Cover, ensuring that the deposit is locked for the period of the cover. This should be achievable by simply removing the if-statement excluding Yield Token Cover.
Fixed in: d1e23f5
The Quotation._makeCover(...) implementation was refactored to always mint Cover Notes and lock the member's deposit when buying cover.
During the review it was identified that deposits were not unlocked when redeeming. This was addressed in f5fcb5f.
Distributor.sol#L353, Distributor.sol#L361, Distributor.sol#L385
The functions setFeePercentage() and setBuysAllowed() and setTreasury() in the Distributor smart contract did not emit any events when executed. Events aid in the visibility of contract state changes. This information can be used on the dApp frontend, and could also be useful for users.
Specifically, being able to view and graph historic changes in feePercentage would be of value to users.
Events should be added to the affected functions to emit the state changes that are made.
Fixed in: 0843f6b
Distributor.sol#L74, Distributor.sol#L362, Distributor.sol#L386
The Distributor smart contract did not validate some of the properties that could be set by the owner or in the constructor.
For feePercentage, an owner was able to set the fee to above 100%. This combined with the fact that no event is emitted to track the feePercentage could result in trusting users (high slippage on maxPriceWithFee) inadvertently overpaying for cover.
The treasury address was also not validated to be a non-zero address. This could result in owners accidentally burning assets if no treasury address is specified.
The following validations should be added to the constructor as well as to each of the applicable set-functions:
feePercentage should have a sensible maximum to prevent abuse by owners.treasury should be validated to not be a zero address to prevent accidental burning of assets.Fixed in: e9ff85d
A check was added to ensure that the treasury address in non-zero as per the recommendations. The team decided to not limit the feePercentage as it was decided to give owners the flexibility. Furthermore, the maxPriceWithFee was regarded as sufficient mitigation and further mitigation would provide little benefit.
Actions to improve the functionality and readability of the codebase are outlined below.
The DistributorFactory redeployed whole new instances of the Distributor smart contract. This increases the byte code size of DistributorFactory and unnecessarily increases the gas cost of deploying new instances of the Distributor.
Making use of a minimal proxy pattern (EIP-1167), whereby a single master copy of the Distributor is deployed, would significantly reduce the gas costs.
Some changes would need to be made to the Distributor smart contract to support the minimal proxy implementation. Primarily that the constructor would need to be replaced with an initialize function.
The OpenZeppelin Clones library provides an audited and robust implementation of a minimal proxy.
The Nexus team decided to not implement a proxy pattern as the deployment gas saving were not sufficient enough to justify the development and proxy gas overheads.
The Distributor smart contract implements a receive function allowing users to send ETH directly to the contract. Developers or users that misunderstand the buyCover function or have an incorrect function signature, might accidentally transfer ETH to the contract that cannot be redeemed.
The master.isInternal(...) function should be used in the receive function to limit ETH deposits to Nexus Mutual contracts. Since the Pool makes use of call opposed to transfer when paying out claims, the small increase in gas usage should not cause the transaction to revert.
The Nexus team decided not to restrict ETH deposits as it is unlikely that users would accidentally transfer ETH to the contract.
Language mistakes were identified in the codebase. Fixing these mistakes can help improve the end-user experience by providing clear information on errors encountered, and improve the maintainability and auditability of the codebase.
dermining -> determining.that.a specific cover token.Fixed in: e9ff85d
DEFAULT_FEE_PERCENTAGEWhen making use of the buyCover utility function in the tests, the order of arguments at
distributor.js#L59 did not allow overloading of the feePercentage :
const priceWithFee = basePrice.muln(DEFAULT_FEE_PERCENTAGE || feePercentage).divn(10000).add(basePrice)
This resulted in all the tests alway using DEFAULT_FEE_PERCENTAGE, even when a custom feePercentage is specified. To address the issue, the feePercentage should be evaluated first:
const priceWithFee = basePrice.muln(feePercentage || DEFAULT_FEE_PERCENTAGE).divn(10000).add(basePrice)
Fixed in: 4155d61
