Nexus Mutual Distributor Smart Contract Audit
# 1. Introduction
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.
* **[Section 2 - Executive summary:](#section-2)** A high-level description of the findings of the audit.
* **[Section 3 - Audit details:](#section-3)** A description of the scope and methodology of the audit.
* **[Section 4 - Design specification:](#section-4)** An outline of the intended functionality of the smart contracts.
* **[Section 5 - Detailed findings:](#section-5)** Detailed descriptions of the findings of the audit.
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:
* Identify potential security flaws.
* Ensure that the smart contracts functioned according to the documentation provided.
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:
* 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.
<a name="section-2"></a>
# 2. Executive summary
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.
<a name="section-3"></a>
# 3. Audit details
## 3.1 Scope
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.
### 3.1.1 Smart contracts
* **Project name:** Nexus Mutual
* **Commits:** [7f2c9b5](https://github.com/NexusMutual/smart-contracts/tree/7f2c9b595ff0668041045a19beca6d470a54cae4)
* **Final Review:** [8b93895](https://github.com/NexusMutual/smart-contracts/tree/8b93895cf052e48b0775654a464c3b2fb9dd8371)
* **Files:** Distributor.sol DistributorFactory.sol
## 3.2 Methodology
A variety of techniques, described below, were used to conduct the audit.
### 3.2.1 Code review
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.
### 3.2.2 Dynamic analysis
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.
### 3.2.3 Automated analysis
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.
## 3.3 Risk ratings
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 a loss of funds for the contract owner or system users.
* **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 addressed to a satisfactory level to remove the risk that it posed.
<a name="section-4"></a>
# 4. Design specification
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.
## DistributorFactory
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`.
## 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](https://eips.ethereum.org/EIPS/eip-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.
<a name="section-5"></a>
# 5. Detailed findings
The following section details the findings of the audit.
## 5.1 High risk
No high risk issues were present at the conclusion of the review.
## 5.2 Medium risk
No medium risk issues were present at the conclusion of the review.
## 5.3 Low risk
No low risk issues were present at the conclusion of the review.
## 5.4 Informational
No Informational issues were present at the conclusion of the review.
## 5.5 Closed
### 5.5.1 Deposit not locked for Yield Token Cover (low risk)
*[Quotation.sol#L282](https://github.com/NexusMutual/smart-contracts/blob/7f2c9b595ff0668041045a19beca6d470a54cae4/contracts/modules/cover/Quotation.sol#L282)*
#### Description
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.
#### Recommendation
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.
#### Update
Fixed in: [d1e23f5](https://github.com/NexusMutual/smart-contracts/commit/d1e23f52ad168ee7c9154b5fd107296a24a58076)
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](https://github.com/NexusMutual/smart-contracts/commit/f5fcb5f7f6d9a3ef9824d8bc2e10dab466672542).
### 5.5.2 Missing function events (informational)
*[Distributor.sol#L353](https://github.com/NexusMutual/smart-contracts/blob/7f2c9b595ff0668041045a19beca6d470a54cae4/contracts/modules/distributor/Distributor.sol#L353)*, *[Distributor.sol#L361](https://github.com/NexusMutual/smart-contracts/blob/7f2c9b595ff0668041045a19beca6d470a54cae4/contracts/modules/distributor/Distributor.sol#L361)*, *[Distributor.sol#L385](https://github.com/NexusMutual/smart-contracts/blob/7f2c9b595ff0668041045a19beca6d470a54cae4/contracts/modules/distributor/Distributor.sol#L385)*
### Description
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.
#### Recommendation
Events should be added to the affected functions to emit the state changes that are made.
#### Update
Fixed in: [0843f6b](https://github.com/NexusMutual/smart-contracts/commit/0843f6bac9637cd3affb04cb225d93fb071594b9)
### 5.5.3 Missing validations (informational)
*[Distributor.sol#L74](https://github.com/NexusMutual/smart-contracts/blob/7f2c9b595ff0668041045a19beca6d470a54cae4/contracts/modules/distributor/Distributor.sol#L74)*, *[Distributor.sol#L362](https://github.com/NexusMutual/smart-contracts/blob/7f2c9b595ff0668041045a19beca6d470a54cae4/contracts/modules/distributor/Distributor.sol#L362)*, *[Distributor.sol#L386](https://github.com/NexusMutual/smart-contracts/blob/7f2c9b595ff0668041045a19beca6d470a54cae4/contracts/modules/distributor/Distributor.sol#L386)*
#### Description
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.
#### Recommendation
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.
#### Update
Fixed in: [e9ff85d](https://github.com/NexusMutual/smart-contracts/commit/e9ff85d900ecd5c802cf027a6fa021bf400747e5)
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.
### 5.5.4 Design comments (informational)
Actions to improve the functionality and readability of the codebase are outlined below.
#### Gas optimisations
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](https://eips.ethereum.org/EIPS/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](https://docs.openzeppelin.com/contracts/4.x/api/proxy#Clones) library provides an audited and robust implementation of a minimal proxy.
##### Update
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.
#### Prevent accidental ETH deposits
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.
#### Update
The Nexus team decided not to restrict ETH deposits as it is unlikely that users would accidentally transfer ETH to the contract.
#### Fix spelling and grammatical errors
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.
1. [Distributor.sol#L86](https://github.com/NexusMutual/smart-contracts/blob/7f2c9b595ff0668041045a19beca6d470a54cae4/contracts/modules/distributor/Distributor.sol#L86): `dermining` -> `determining`.
2. [Distributor.sol#L204](https://github.com/NexusMutual/smart-contracts/blob/7f2c9b595ff0668041045a19beca6d470a54cae4/contracts/modules/distributor/Distributor.sol#L204): Repetition of `that`.
3. [Distributor.sol#L233](https://github.com/NexusMutual/smart-contracts/blob/7f2c9b595ff0668041045a19beca6d470a54cae4/contracts/modules/distributor/Distributor.sol#L233): Execute an action on `a` specific cover token.
#### Update
Fixed in: [e9ff85d](https://github.com/NexusMutual/smart-contracts/commit/e9ff85d900ecd5c802cf027a6fa021bf400747e5)
#### Integration tests always use `DEFAULT_FEE_PERCENTAGE`
When making use of the `buyCover` utility function in the tests, the order of arguments at
[distributor.js#L59](https://github.com/NexusMutual/smart-contracts/blob/7f2c9b595ff0668041045a19beca6d470a54cae4/test/integration/Distributor/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)
```
#### Update
Fixed in: [4155d61](https://github.com/NexusMutual/smart-contracts/commit/4155d61a2b7cfebb9af4988662ad5acd430b22ec)
