Nayms 2024 (Retainer) security record

The system assumes that the total supply of supported rebase tokens will not decrease. This can have two main consequences: 1. Since the internal accounting is expected to mimic the external price, we can have at a particular moment some price gaps between the external market price of a token and its internal representation, in both directions (over priced, or under priced). Since price alignment can only happen once a user calls one of the two update functions (`distributeAccruedInterest()` or `rebaseERC20()`), we may have situations when there is a delay in triggering a price update function. This could leave the system exposed to arbitrage or incorrect internal prices. Also, some rebasing tokens can be upgraded or hacked, so it cannot be 100% sure that the supply of a given rebasing token cannot decrease with time. 2. In `LibTokenizedVault`, an underflow is possible in the function `_accruedInterest()`, if `total < depositTotal`. In this case, reflecting the rebasing of the supported token will not be possible until the condition returns `true` again.

In `LibACL`, a counter variable `sysAdminCount` makes sure that we cannot have less than one address with the role `sysAdmin`. However, this can happen in two situations: * if Bob is the only `sysAdmin` (`sysAdminCount = 1`), he grants the role to himself (`sysAdminCount = 2`), he revokes himself (`sysAdminCount = 1`) => no more valid address with the role `sysAdmin`; * if Bob is the only `sysAdmin` (`sysAdminCount = 1`), he grants to a random address owned by no one (`sysAdminCount = 2`), he revokes himself (`sysAdminCount = 1`) => no more valid address with the role `sysAdmin`; Even if the event is unlikely since only an address with the role `sysAdmin` can perform these operations, the impact would be high. **Note:** this issue is close to the issue #6 of the previous audit (link [here](https://certificate.quantstamp.com/full/nayms-2/79da5daf-4326-4f12-a71c-828b72fbea09/index.html)).

When starting a token sale, the sold token amount may be less than the corresponding entities token `objectMinimumSell` amount, which was set during `EntityFacet.enableEntityTokenization()`, resulting in the created offer to be automatically in the `LibConstants.OFFER_STATE_FULFILLED` state. Similarly, the `buyAmount` is not verified to be greater than `objectMinimumSell` of the corresponding `entity.assetId`. Also, offers can be created for not-yet-tokenized entity p-tokens. Leading to their automatic fulfillment on creation.

It is important to validate inputs, even if they only come from trusted addresses, to avoid human error: 1. In the function `LibFeeRouter._addFeeSchedule()`: </br>a. qs-badge-fixed the fact that `_entityId` matches an existing entity is not checked; </br>b. qs-badge-fixed there is no min and max check for the length of the `_receivers`, which could lead to a denial of service; 2. `LibAdmin._approveSelfOnboarding()`: </br>a. qs-badge-fixed `_entityId` can be `address(0)`, the approval will be recorded, but the user will not be able to `onboard()`; </br>b. qs-badge-fixed it is possible to repeat the steps 1) `approveSelfOnboarding()` and 2) `onboard()` for the same user but multiple entities; 3. In `LibMarket._setMinimumSell()`, no check makes sure that `_objectId` represents an existing object. 4. In `LibEntity._validateSimplePolicyCreation()`, no check makes sure that: </br>a. qs-badge-fixed the maturation date is too far in the future; </br>b. qs-badge-fixed the number of policy-level commission receivers is not too high, which could lead to out-of-gas errors when trying to pay the commissions; 5. qs-badge-fixed In the function `LibTokenizedVaultStaking._initStaking()`, no check makes sure that `_entityId` represents an existing entity; 6. In the function `LibTokenizedVaultStaking._payReward()`, p-tokens can be transferred as rewards, instead of only limiting to supported external tokens. 7. qs-badge-fixed In the function `LibTokenizedVaultStaking._validateStakingParams()`, the value of `initDate` can be in a very long time from the current time. 8. qs-badge-fixed In the function `LibTokenizedVaultStaking._validateStakingParams()`, there is no min or max check for the value of `interval`. 9. qs-badge-fixed In the function `LibTokenizedVaultStaking._stake()`, the `_amount` can be `0`, `_stakerId` can be `0` if `msg.sender` has no parent, and `_entityId` can be a non-existent entity. 10. qs-badge-fixed In the function `LibTokenizedVaultStaking._validateStakingParams()`, the following check is insufficient to ensure that the boost multiplier asymptotically converges to 2, which is only true when `_config.a + _config.r == _config.divider`: ``` if (_config.a + _config.r > _config.divider) revert APlusRCannotBeGreaterThanDivider(); ``` 11. In the function `LibTokenizedVaultStaking._validateStakingParams()`, the value of `tokenId` is not checked. In particular, the staking token ID may be: * qs-badge-fixed the zero/empty object ID; * the p-token of another entity; * an external token ID; * the ID of a wrapped internal token; * any other arbitrary non-object ID;

As it presently is constructed, the contract `ERC20Wrapper` is vulnerable to the [allowance double-spend exploit](https://github.com/comitylabs/openzeppelin-contracts/blob/6bd6b76d1156e20e45d1016f355d154141c7e5b9/contracts/token/ERC20/IERC20.sol#L43), as with other ERC20 tokens.

It is important to validate inputs, even if they only come from trusted addresses, to avoid human error. The following functions do not have a proper validation of input parameters: 1. To enforce a maximum limit of `10000` on commission totals, add the line `require(s.tradingCommissionTotalBP <= LibConstants.BP_FACTOR, "commission total must be<=10000bp")`; 2. In `LibACL.updateRoleAssigner()` ensure that the `_assignerGroup` is actually present in the system; 3. `LibEntity._createSimplePolicy()`: the fact that `_stakeholders.roles.length == _stakeholders.entityIds.length` is not checked. The case where `number of roles > number of entityIds` would lead to an exception. However, the case `number of roles < number of entityIds` would let a simplePolicy be created with fewer assigned roles than the number of signing stakeholders. Also, as an extreme case, a policy can be created with 0 assigned roles and 0 stakeholders; 4. `LibEntity._createEntity()` does not check that the parameter `_entityAdmin` exists; 5. `LibEntity._validateSimplePolicyCreation()` does not check that the policy covers a minimum time (`maturationDate`); 6. `LibFeeRouter._updateTradingCommissionsBasisPoints()` does not check that the parameter `bp.tradingCommissionTotalBP` is within a certain minimum/maximum range (i.e. `bp.tradingCommissionTotalBP <= LibConstants.BP_FACTOR`); 7. `UserFacet.setEntity()` does not check that the parameter `_userId` is an existing user; 8. `GovernanceFacet.updateUpgradeExpiration()`: any `uint256` value from `0` to `max(uint256)` can be used; 9. `GovernanceFacet.cancelUpgrade()`: any upgrade can be canceled, even if it is expired or never existed. In that case, any typo in the parameter `id` would be uncaught; 10. `LibObject._enableObjectTokenization()`: an object can be tokenized with a `name == ""`; 11. `EntityFacet.enableEntityTokenization()`: the sysAdmin must make sure that the parameters `_name` and `_symbol` does not have any weird character that could create UX issues on the front-end; 12. `LibEntity._createSimplePolicy()`: the fact that the entity of the signing stakeholder is not the entity issuing the policy is not checked;

The `NaymsOwnershipFacet.transferOwnership()` function can accidentally revoke the role when the `_newOwner` is the same as the current owner. The function first calls `LibACL._assignRole()` and then calls `LibACL._unassignRole()`. If the `_newOwner` is the same as the current owner, the first call `LibACL._assignRole()` will override the same thing, and the role will be immediately revoked by the call of `LibACL._unassignRole()`.

We recommend adding validations to the Nayms system for several reasons. First, the system has multiple layers of function calls between facets, libraries, and storage. While libraries are often designed to be more generalized, facet functions may have more specific responsibilities within a smaller scope. In such cases, it is recommended to add validations to ensure the feature is only doing what the facet function is intended to support and nothing more. Second, Solidity provides empty default values for all variable types, which can create risks when getting struct data from a mapping. It will always provide empty struct data regardless of whether the key-value pair has been set. Finally, adding validations can help prevent accidental wrong inputs that could mutate the system. 1. `EntityFacet.enableEntityTokenization()`: should validate that only an "entity" can tokenize with this function (e.g., checking `s.existingEntities[_entityId] == true`). The current implementation allows tokenizing any object with this function which violates the function naming. 1. `LibEntity._createEntity()`: the fact that the `_entityAdmin` exists and that the `_entityID` does not already exist is not checked (e.g., checking `s.existingEntities[_entityId] == true`). Also, if given empty inputs, it can assign value to an empty key for the `s.existingEntities[]`, `s.entities[]`, `s.objectParent[]`, and `s.roles[]` mappings. The pattern is dangerous. For instance, it can get an unexpected entity with `s.entities[LibObject._getParent(_policyId)]` when the `_policyId` does not exist. 1. `LibEntity._updateEntity()`: the function should validate that `_entityId` is not empty. Otherwise, it can assign value to an empty key for the `s.entities[]` mapping. The pattern is dangerous. For instance, it can get an unexpected entity with `s.entities[LibObject._getParent(_policyId)]` when the `_policyId` does not exist. 1. `LibEntity._updateAllowSimplePolicy()`: the function should validate that `_entityId` is not empty. 1. `LibEntity._startTokenSale()`: the function should validate that `s.existingEntities[_entityId] == true` and that the `entity` has enabled tokenization (e.g., by calling `LibObject._isObjectTokenizable()`). 1. `LibEntity._createSimplePolicy()`: 1. the function should validate that `_entityId` and `_policyId` are not empty bytes. 2. the function should validate the fields of the `_stakeholders`. First, the `_stakeholders.roles.length` should be the same as the `_stakeholders.entityIds.length` (or the length of the `signatures`). Second, the `_stakeholders.entityIds[i]` should be the same as the `_entityId`. Third, depending on the business requirement, all the roles of the `_stakeholders.roles[i]` should probably be the same. Without the validations, it can set the policy role to another entity, and different stakeholders might have different roles. 3. if the length of the parameter `_stakeholders` is too high, it can lead to an out-of-gas error in the for-loop. 1. `LibEntity.validateEntity()`: there is no check making sure that the `utilizedCapacity` of the entity is less than or equal to its `maxCapacity`. 1. `LibEntity. _validateSimplePolicyCreation()`: there is no min duration for the policy, no non-zero check for the `claimsPaid` and the `premiumPaids`, no min/max check for the `sponsorCommissionBasisPoints` and no check that the boolean attribute `canceled` is `false`. 1. `UserFacet.setEntity()`: the `_entityId` should be linked to an existing entity. Also, the function should check whether the user has already been attached to an entity. Otherwise, it can have unexpected outcomes, such as the user not being able to cancel orders for the previous entity anymore. 1. `LibSimplePolicy._payPremium()`: the function can add validation checking that the entity for `_payerEntityId` exists and the policy for `_policyId` exists. Though unlikely, the dummy `simplePolicy` returned as the default value from the mapping might pass through the function and have an unexpected effect. 1. `LibACL._assignRole()`: the `_objectId`, `_contextId`, and `_roleId` inputs should not be empty strings. 1. `LibACL._updateRoleAssigner()`: the `_role` and the `_assignerGroup` should not be empty. Specifically the `_role` input. It can set a value to the `s.canAssign[]` mapping for the empty key. 1. `LibACL._updateRoleGroup()`: the `_role` and `_group` inputs should not be empty strings. Otherwise, it can set the values for empty keys in the `s.groups` mapping. The pattern is dangerous because it can potentially break the `LibACL._canAssign()` function. The `assignerGroup` with the current implementation can be empty bytes when `_roleId` does not exist in the `s.canAssign[]` mapping. The `_isInGroup()` check might pass and falsely return true. 1. `LibAdmin._addSupportedExternalToken()`: the `_tokenAddress` input should not be empty. 1. `LibAdmin`: there is no non-zero address check for the parameter `_tokenAddress` in the functions `_a

The state variable `supportedExternalTokens` is used to record the external tokens supported by the protocol. A new token can be added with the function `_addSupportedExternalToken()`. As the whole list is browsed with a for loop, it can be possible that, once a given size is reached, the function reverts with an out-of-gas error. In this case, it will not be possible anymore to add new supported external tokens.