neptune-mutual-blue/protocol

protocol's Issues

Update function documentations

Below are the example of outdated and mistakes in the documentations

• The parameter amount is not included in the function documentation

---

• The parameter coverKey is duplicated

Update the deployment script to add initial liquidity to diversified covers

Current Behavior

When the protocol is being deployed, initial liquidity is added to the dedicated covers but not to the diversified covers.

Change Request

To add initial liquidity to diversified covers during deployment.

[OZ] L-01 Able to close non-empty staking pool

Please note that OpenZeppelin reported this issue.

A staking pool can be closed without checking if there is any remaining liquidity of either the staking token or the reward token. Once the pool is closed, neither deposit nor withdraw functions are allowed. Hence, users won't be able to access their funds. However a recovery agent is still able to retrieve both staking and reward tokens and distribute them as desired.

Consider checking for remaining liquidity before closing a pool

[OZ] M-01 Unenforced staking requirement

Please note that OpenZeppelin reported this issue.

Adding liquidity requires a liquidity provider to have at least a minimum amount of NPM tokens
staked in the vault.

However, the purpose and usefulness of this requirement is unclear, since it can be bypassed.
In particular:

• there is no relationship between the amount of PODs created and the size of the stake
• PODs are transferable to unstaked users, so users can provide liquidity without staking
• staked users can exit their entire staked amount without redeeming any PODs by calling removeLiquidity with parameters podsToRedeem = 0 , npmStakeToRemove = amount, and exit = 1 ; the exit = 1 is crucial as it allows execution of line 234 of VaultLibV1.sol

Consider documenting and enforcing the intended relationship between NPM staking and
liquidity provision.

[OZ] L-04 Imprecise bounds

There are several examples where the time windows or value ranges are defined inconsistently. In particular:

• The getWithdrawalInfoInternal function of the RoutineInvokerLibV1 library considers the end timestamp to be part of the withdrawal period but the mustBeDuringWithdrawalPeriod validation function does not.
• The StakingPoolLibV1 library prevents withdrawals on the block height where withdrawals can start.
• Neither the mustBeBeforeResolutionDeadline function nor the mustBeAfterResolutionDeadline function will succeed on the resolution deadline.
• The flash loan fee calculation requires the loan to be strictly less than the available balance, even though the contract claims to loan out the whole balance.

[OZ] N-01 transfer and send calls are no longer considered best practice

Please note that OpenZeppelin reported this issue.

When transfer or send calls are used to transfer Ether to an address, they forward only a limited amount of gas. This precludes destination addresses with complex fallback functions. Additionally, given that gas prices for EVM operations are sometimes repriced, code execution on the receiving end of these calls cannot be guaranteed in perpetuity.

There are multiple occurrences throughout the code base where transfer or send is used to transfer Ether. For instance:

• On line 41 of StoreBase.sol Ether is transferred via transfer.
• On line 19 of WithRecovery.sol Ether is transferred via transfer.
• On line 23 of BaseLibV1.sol Ether is transferred via transfer.

Rather than using transfer or send, consider using address.call{value: amount}("") or the sendValue function of the OpenZeppelin Address library to transfer Ether.

[OZ] N-19 Unnecessary coupling

Please note that OpenZeppelin reported this issue.

When recording an attestation, the stake is recorded against the who address, but the reporter is set to the message sender. In both invocations, the who parameter is set to the message sender anyway. Nevertheless, in the interest of local reasoning, consider using the who address consistently. A similar observation applies to the first disputer.

Similarly, when calculating the future commitment, the number of months to check should depend on the global limit.

The Cover contract has incorrect ACL requirements.

A governance agent should not be concerned with:

• updating cover creator whitelist
• disabling policy purchase feature

I would suggest refactoring all instance of AccessControlLibV1.mustBeGovernanceAdmin(s) in the Cover.sol file to AccessControlLibV1.mustBeCoverManager(s)

A governance agent shouldn't worry about:

• modifying the cover creator whitelist
• disabling purchase policy feature

In the Cover.sol file, I would advise changing any occurrences of "AccessControlLibV1.mustBeGovernanceAdmin(s)" to "AccessControlLibV1.mustBeCoverManager(s)."

[OZ] L-03 Implicit timing assumptions

Please note that OpenZeppelin reported this issue.

To account for the coverage delay, some valid cxTokens may be excluded from making claims. Any coverage that will become active within 14 days but before the incident resolution will be disregarded. This implicitly assumes that no valid cover starts after either of these deadlines (otherwise it should also be excluded). Since the coverage delay and resolution window are configurable parameters, the assumptions may not hold. Consider calculating exclusions based on the specific parameters that are relevant to the incident being processed.

Add a Feature to "Close" a Report

Current Behavior

When an incident report is submitted, it undergoes a one-week reporting period and a one-week claim period.

Problem

Even if there is a monetary penalty, a motivated spammer (an attacker or a rival of a protected project) might still cause annoyance by submitting a phoney incident report. Thus, policy and liquidity transactions will be inaccessible for at least a week. Also troublesome is the occurrence of an actual incident before the conclusion of the reporting period.

Change Request

Add a function to immediately close (finalize) a report, rather than waiting a week, to avoid the submission of spam incident reports.

[OZ] H-02 Risk of insufficient liquidity

Please note that OpenZeppelin reported this issue.

When purchasing a cover, the protocol ensures it has enough funds to pay out all potential claimants. The computation of the existing commitments includes all covers expiring in the next 3 months, since this is the maximum policy duration. However, some covers may expire in the fourth month and these would be excluded from the calculation. Therefore, the protocol could sell more insurance than it can support, and some valid claimants may be unable to retrieve their payment.

Consider including the extra month in the commitment computation.

[OZ] H-01 Conflated staking pool reward balances

Please note that OpenZeppelin reported this issue.

Each staking pool specifies its own reward token and corresponding balance in the same aggregate contract. When retrieving this value, the token balance of the aggregate contract is returned. Since there could be multiple staking pools with the same reward token, this could include balances from other pools. It could also include any reward token balances that were directly sent to the contract.

Moreover, current user rewards could also be overstated, which would prevent users from claiming the last rewards. Since rewards are claimed when withdrawing stake, anyone could prevent users from unstaking by directly sending reward tokens to the staking pool contract. Any non-zero amount would be sufficient to trigger this scenario. If this occurs, a recovery agent could still retrieve the funds from the aggregate pool contract and distribute them as desired, although it is not clear how they should distribute the remaining rewards.

Consider reading the pool balance from the saved record.

Comments provides wrong Information

The comment and the code in the following file say two different things.

filename; ProtoUtilV1.sol

The comment should be Reverts if the caller is not one of the protocol member

[OZ] N-11 Misleading comments

Please note that OpenZeppelin reported this issue.

Some comments are misleading, and the implementation does not follow the stated intention. For example,

• In line 76, it was stated not to reset the first reporter by incident date. However, the first reporter is not saved by incident date, and it is deleted in line 90 of Finalization.sol . Similarly, the commented out lines do not contain the productKey and don't correspond to any saved value.
• In line 128 of Protocol.sol , it is said that the protocol needs to be paused when the addMember function is invoked but in line 136, it ensures the protocol must not be paused.
• The comments describing the callerMustBeX functions reference the "sender" rather than the caller parameter.
• When initializing the protocol, the burner address must be non-zero but the comment says it isn't necessarily zero.

Consider updating the comments to be aligned with the code implementation

[OZ] L-07 Missing event parameter

Please note that OpenZeppelin reported this issue.

The PoolUpdated event does not include the stakingTarget parameter. Consider including it.

[OZ] N-14 Repeated pause validation

Several functions in the Protocol contract have a whenNotPaused modifier and mustNotBePaused requirement. However, both of these check the pause status of the Protocol contract, so one of them is redundant. Consider removing one of them.

[OZ] N-04 Copied in dependencies

Please note that OpenZeppelin reported this issue.

Dependencies in the lib directory, including openzeppelin-solidity, are copied in without any reference in .gitmodules. This makes it hard to keep track of the latest versions and easy to accidentally change the code inside.

Consider using forge install OpenZeppelin/openzeppelin-contracts for the latest version of the OpenZeppelin contracts.

The file .gitmodules is not checked into git because of .gitignore configuration. Without changing .gitignore, force push the .gitmodules file.

[OZ] M-03 Incorrect policy fee

Please note that OpenZeppelin reported this issue.

There are two discrepancies when calculating a policy fee rate:

• It is always strictly higher than the configured floor.
• The amount of days charged does not account for a non-standard coverage lag period.

Consider updating the calculation accordingly.

Accuracy of `upgradeContractWithKey`

Must ensure that the previous contract is correct contract before updating.

[OZ] M-06 Unexpected deployer privileges

Please note that OpenZeppelin reported this issue.

The deployer address of the Store contract is recorded as a protocol member, which allows it to update the storage arbitrarily. The same address is set as the contract's owner role, which allows it to pause and unpause storage updates. We believe these are intended to be the same role, but they are not programmatically connected. In particular, if the owner address is renounced or transferred, the deployer will still be able to update storage.

Moreover, it is unclear why the Store owner or deployer requires the ability to modify storage arbitrarily.

Consider documenting the role in the Security overview if the role is required. Otherwise, consider renouncing protocol member privileges from the deployer address after the deployment is finished.

Missing documentation for the product status

The product status which is being used in addProduct, updateProduct and isActiveProductInternal functions is not documented properly anywhere except in comments of addProductInternal function

protocol/contracts/libraries/CoverLibV1.sol

Lines 200 to 205 in a98fcce

	// Product Status
	// 0 --> Deleted
	// 1 --> Active
	// 2 --> Retired
	require(values[0] == 1, "Status must be active");

However, the name product status is confusing as it's already used to specify the resolution status of a product in the protocol

protocol/contracts/libraries/CoverUtilV1.sol

Lines 23 to 31 in a98fcce

	enum ProductStatus {
	Normal,
	Stopped,
	IncidentHappened,
	FalseReporting,
	Claimable
	}

addProduct

The first item of the last argument of addProduct function is documented as product status - values[0]. This parameter can be omitted as there is only one value accepted to execute the transaction successfully.

require(values[0] == 1, "Status must be active");

Question

Is there any need of having two different statuses for "Retired" and "Deleted" ? As they both offer almost same functionality as of now.

[OZ] M-04 Parallel access control

Please note that OpenZeppelin reported this issue.

The Protocol contract inherits the OpenZeppelin AccessControl contract, and uses it to define the role hierarchy. It also provides a mechanism for the administrator to grant an existing role to a new address. However, this mechanism functions in parallel to the inherited mechanism for granting roles. This leads to two inconsistencies:

• A role administrator can bypass the whenNotPaused restriction by using the inherited
  mechanism.
• The NS_ROLES_ADMIN can use the new mechanism to grant the NS_ROLES_GOVERNANCE_AGENT, even though they do not directly administer that role.

Consider ensuring consistency between the two mechanisms. Depending on the desired outcome, this could involve relying on the original mechanism, changing the role relationships, or overriding the inherited grantRole function.

About updating cover rules

• Cover creator changes to parameters and policy conditions should be Admin only
• Feature to disable policy purchase temporarily

Protocol, NPM Token, and Oracle Dependency

Neptune Mutual Token (NPM) is planned to be launched months after the protocol is deployed (although this can change). Instead of NPM tokens, we will deploy a voucher. The voucher is a Proof of Authority Token (POT) distributed to a few select individuals and organisations who will participate in our governance portals.

Note that the POT holders are only concerned about the consensus and resolution process. Most, if not all, will not be a policyholder or liquidity provider. Also note that the community members will not receive or be able to acquire any POTs. Therefore, none of the protocol operations should be affected when the zero NPM is supplied. The features that allow setting the minimum (like minStakeToAddLiquidity, etc) should also allow zero values.

Task

Create a list of the affected functions. We can then refactor the codebase to satisfy this requirement.

Initial Requirement

• Refactor RoutineInvoker's updateStateAndLiquidity to work even when IPriceOracle is not present.
• Refactor features related to adding and removing liquidity, minimum NPM stake, etc in such a way that it works even when NPM tokens are not deployed

[OZ] L-10 The info parameter might lose information about an IPFS hash

Please note that OpenZeppelin reported this issue.

The info parameter of the report, dispute, and other functions assume that the length of the IPFS hash is 32 bytes or shorter. However, that is not the case for CIDv1 where the hash can be longer than 32 bytes and also contain prefixes.

This leads to a data availability issue when NPM holders might be unable to retrieve the incident information from the smart contracts. Consequently, they are unable to decide whether to attest or refute the incident.

Consider using a different data structure for storing an IPFS hash.

[OZ] L-11 Incorrect individual liquidity share

Please note that OpenZeppelin reported this issue.

The calculation of an individual's share of liquidity for a particular cover incorrectly uses values[5] instead of values[4] as the number of PODs. Since this is always zero, the returned share of liquidity will always be zero.

This has no implications within the current code base but would mislead external users that rely on it. Consider using the correct number of PODs in the calculation.

Missing params on function documentation [libraries]

• Some parameters are missing from the present documentation in one or more files under contracts/libraries

eg: A function in ProtoUtilV1.sol has missing params s and key on documentation

[OZ] N-06 Duplicate modifier

Please note that OpenZeppelin reported this issue.

The unpause function of the ProtoBase contract has two whenPaused modifiers. Consider removing the first one.

[OZ] N-07 Duplicate token supply tracking

Please note that OpenZeppelin reported this issue.

The NPM token tracks the number of tokens that have been issued. This should be identical to the total supply if the tokens are never burned. It's worth noting that the code base transfers funds to a burner address instead of reducing the supply.

Consider disabling the burn functionality so that the total issued amount does not need to be tracked and updated separately.

[OZ] M-05 Unable to unstake after finalization

Please note that OpenZeppelin reported this issue.

Reporters on the winning camp can unstake their tokens even after the incident has been finalized, albeit with no reward. However, the resolution deadline is not specific to a particular incident and is reset to 0 during finalization. Since the deadline is checked during unstaking, the operation will fail. This means that some successful NPM stakers will be unable to retrieve their funds.

In this scenario, a recovery agent could still retrieve the funds from the Resolution contract and distribute them as desired.

Consider recording the resolution deadline with the incident date so it does not need to be cleared during finalization.

[OZ] L-12 Variable outside store

Please note that OpenZeppelin reported this issue.

In contrast to most of the code base, the last policy identifier is saved directly in the Policy contract. However, to maintain continuity and prevent conflicts, any new version will need to import the old value.

Consider saving it in the Store contract.

Decouple pause and unpause ACL logic in the store contract

Unlike the unpause feature, which is solely available to the owner, the pause feature of the store should be accessible to a separate account in order to programmatically pause the contract. In a hosted environment, this helps to avoid using the owner's private key.

Missing params documentation under `contracts`

• Similar to Issue #114
• Some parameters are missing from the present documentation in one or more files under contracts/core, contracts/interfaces, contracts/pool

Remove redundant `pause` check on the Protocol contract

Numerous instances of Pausable.whenNotPaused and ValidationLibV1.mustNotBePaused are used in functions across the protocol contract. Use ValidationLibV1.mustNotBePaused as an alternative to Pausable.whenNotPaused.

[OZ] L-05 Incorrect NPM threshold

Please note that OpenZeppelin reported this issue.

Some operations require an NPM stake that must not exceed a threshold, currently set to 10 billion. However, the total NPM supply cannot exceed 1 billion, making the threshold nonfunctional. The Neptune team indicated the threshold should only be 10 million. Consider updating the constant accordingly.

[OZ] L-02 Collision between constants

Please note that OpenZeppelin reported this issue.

The NS_POOL_MAX_STAKE and NS_POOL_REWARD_TOKEN constants are defined to be the same string, which introduces the possibility of unexpected storage collisions. In the current code base they are used with non-overlapping data types, which are saved in different mappings. Nevertheless, in the interest of predictability, consider redefining the NS_POOL_MAX_STAKE constant to a unique string.

[OZ] N-10 Incorrect array size

Please note that OpenZeppelin reported this issue.

The getCoverPoolSummaryInternal function creates an array of size 8 but only uses 7 positions. Similarly, the getInfoInternal function creates an array of size 11 but only uses 8 positions.

Consider resizing them accordingly.

[OZ] N-18 Unnecessary complex code

Please note that OpenZeppelin reported this issue.

• The s.getStablecoin() == address(token) == false expression on line 245 of StrategyLibV1.sol can be replaced with s.getStablecoin() != address(token).
• Line 272 of CoverLibV1.sol casts the variable of type address to type address. The casting can be avoided.
• isProtocolMember is defined both in line 257 of ProtoUtilV1.sol and line 85 of StoreBase.sol .

Please ignore the last point: isProtocolMember being defined in multiple places. The Store contract is standalone and can't depend on ProtoUtilV1. Neither can ProtoUtilV1 depend on Store.

[OZ] L-08 No unstaking window

Please note that OpenZeppelin reported this issue.

After an incident is resolved, successful stakers can retrieve their rewards provided the incident has not been finalized. When the incident occurred, they will have at least the claim period. However, if the incident was successfully disputed, there is no claim period and the incident can be finalized immediately before stakers have been provided sufficient time to claim their rewards. Consider including an unstaking window for this scenario.

[OZ] L-06 Lack of input validation

Please note that OpenZeppelin reported this issue.

• The mustNotExceedNpmThreshold function should validate npmStakeToAdd instead of amount .
• The setPolicyRatesByKey function in the PolicyAdmin contract does not check that ceiling is greater than floor, while a similar function setPolicyRates does.
• The initialize function in the Protocol contract does not check the length of the input values array.
• When computing unstaking rewards after an incident resolution, the sum of the toBurn and toReporter rates are not validated to be bounded above by ProtoUtilV1.MULTIPLIER.

Consider including the corresponding validations.

Platform fee is not transferred during claims

The processor contract's claim function should transfer platform fee to the treasury wallet. A fix and a unit test is needed.

[OZ] N-09 Incomplete deletion

Please note that OpenZeppelin reported this issue.

When finalizing an incident, an unused record is deleted. Additionally, the first disputer is not deleted. Consider updating the deletions accordingly

Error in `_withdrawFromDisabled` function

RoutineInvokerLibV1.sol
The function _withdrawFromDisabled allows to withdraw the amount lent to strategies which are currently disabled. Therefore strategy in strategy.withdraw(coverKey) is not active

function _withdrawFromDisabled(
  IStore s,
  bytes32 coverKey,
  address onBehalfOf
) private {
  address[] memory strategies = s.getDisabledStrategiesInternal();

  for (uint256 i = 0; i < strategies.length; i++) {
    ILendingStrategy strategy = ILendingStrategy(strategies[i]);
    uint256 balance = /* ... */

    if (balance > 0) {
      strategy.withdraw(coverKey);
    }
  }
}

But an exception is thrown because of the internal validations of vault contract

Error: VM Exception while processing transaction: reverted with reason string 'Not a strategy contract'
at ValidationLibV1.callerMustBeSpecificStrategyContract (contracts/libraries/ValidationLibV1.sol:148)
at VaultDelegate.preTransferToStrategy (contracts/core/delegates/VaultDelegateBase.sol:76)
at Vault.transferToStrategy (contracts/core/liquidity/VaultStrategy.sol:31)
at AaveStrategy.withdraw (contracts/core/liquidity/strategies/AaveStrategy.sol:149)
at RoutineInvokerLibV1._withdrawFromDisabled (contracts/libraries/RoutineInvokerLibV1.sol:263)
at RoutineInvokerLibV1._invokeAssetManagement (contracts/libraries/RoutineInvokerLibV1.sol:193)
at RoutineInvokerLibV1._invoke (contracts/libraries/RoutineInvokerLibV1.sol:46)
at RoutineInvokerLibV1.updateStateAndLiquidity (contracts/libraries/RoutineInvokerLibV1.sol:30)

The exact functions which are throwing this exception are callerMustBeStrategyContract and callerMustBeSpecificStrategyContract which only allow active strategies

  function callerMustBeStrategyContract(IStore s, address caller) external view {
    bool callerIsStrategyContract = s.getBoolByKey(_getIsActiveStrategyKey(caller));
    require(callerIsStrategyContract == true, "Not a strategy contract");
  }

  function callerMustBeSpecificStrategyContract(
    IStore s,
    address caller,
    bytes32 /*strategyName*/
  ) external view {
    bool callerIsStrategyContract = s.getBoolByKey(_getIsActiveStrategyKey(caller));
    require(callerIsStrategyContract == true, "Not a strategy contract");
  }

[OZ] M-02 Potential token transfer from unrelated account

Please note that OpenZeppelin reported this issue.

The CoverReassurance contract contains a mechanism to retrieve funds from an arbitrary account, as long as the account has provided a non-zero allowance. This would occur whenever a cover owner can front-run another cover owner's reassurance transaction, allowing them to redirect the funds to their own cover.

Even without front-running, there are multiple reasons an account may have a non-zero allowance, including:

• Their addReassurance transaction failed and they didn't revoke the allowance.
• They made an unlimited approval.
• They approved a higher allowance than the amount they eventually transferred.

In all cases, an attacker can retrieve those funds and direct them towards a cover.

A recovery agent could still retrieve the funds from the CoverReassurance contract and distribute them as desired, although it is unclear how they would distinguish a front-running attack from one where a cover owner legitimately transfers funds from a different account.

Consider retrieving the tokens from the message sender rather than an arbitrary account parameter.

[OZ] L-09 Protocol administrator needs to handle external tokens

Please note that OpenZeppelin reported this issue.

The protocol administrator is one of the most critical roles with immense privilege in the operation of the entire protocol. For example, only the administrator can re-initialize the protocol, grant key access control roles, as well as set up all staking and bonding pools.

However, when setting up a staking pool, a non-zero amount of reward tokens are required to be pre-transferred to the administrator account and pulled to the contract. This implies that the administrator needs to receive and approve the transaction a priori. This increases the attack surface and may not fit the intended security assumptions for a critical role.

Consider either using a less critical role to perform staking pool initialization or allowing pool initialization without any token transfer.