Nayms 2

In the standard Diamond Pattern, upgrades work as follows: facets (a contract implementing a particular behavior) can be added, replaced, or removed using the function: ``` { DiamondCutFacet.diamondCut( FacetCut[] calldata _diamondCut, address _init, bytes calldata _calldata ) } ``` </br>The second and third parameters are used (if both have a non-zero value) to execute a `delegateCall()` from the current contract to any arbitrary contract deployed at address `_init`, to a specific function and with specific parameters defined in `_calldata`. The current two-phase diamond facet cut process implemented in `PhasedDiamondCutFacet.diamondCut()` makes sure for security purposes that the following actions associated with an upgrade of the diamond are executed by two distinct addresses: * allow in advance in `GovernanceFacet.sol` an upgrade to be executed (action only doable by a `sysAdmin`); * execute an allowed upgrade (action only doable by the owner of the diamond contract); </br>An upgrade is allowed only if its hash (which is the hash of the parameter `_diamondCut`) can be found in the storage mapping `upgradeScheduled`, the associated value being the expiration timestamp of that upgrade authorization. However, that authorization in advance lets the contract owner use any values for the parameters `_init` and `_calldata`. As a result, it becomes possible to override any value anywhere in the storage of the Diamond. The owner just has to deploy an arbitrary contract at the address `_init` and call a function that overrides memory slots. For example, he could: 1. drain all ERC20 funds deposited to the system; 2. update the sysAdmins of Nayms; 3. modify balances of the internal ledger; </br>Note that this list is non-exhaustive. Also, the contract owner is considered a trusted user.

The Nayms system currently restricts the number of `dividendDenomiation` to be at most `maxDividendDenominations`, which is currently set as `1`. However, in `LibTokenizedVault`, this restriction is checked before the value is updated, so the check can be bypassed. Specifically, we have the following snippet in `_payDividend()`: ```solidity! if (s.dividendDenominationIndex[_to][_dividendTokenId] == 0) { if (s.dividendDenominations[_to].length > LibAdmin._getMaxDividendDenominations()) { revert("exceeds max div denominations"); } s.dividendDenominationIndex[_to][_dividendTokenId] = uint8(s.dividendDenominations[_to].length); s.dividendDenominationAtIndex[_to][uint8(s.dividendDenominations[_to].length)] = _dividendTokenId; s.dividendDenominations[_to].push(_dividendTokenId); } ``` This means we can add more than `1` dividend token id in the list `dividendDenominations`. However, this has a harmful unintended side effect, where a same `dividendTokenId` can be added to the array. Suppose we called `_payDividend(_to = Alice, _dividendTokenId = WETH)` for the first time. Note that after the function call, `s.dividendDenominationIndex[Alice][WETH]` will still be `0` because `uint8(s.dividendDenominations[Alice].length) = 0`, and now `s.dividendDenominations[Alice] = [WETH]`. Now, if we call `_payDividend(_to = Alice, _dividendTokenId = WETH)` again, it will pass both checks in `_payDividend`, and append `WETH` to the array `s.dividendDenominations[Alice]` again. This leads to a double-counting issue in `_normalizeDividends` as `s.withdrawnDividendPerOwner[_tokenId][dividendDenominationId][_to] += dividendDeductionIssued;` will be executed twice in the loop, adding the same value twice to the same `dividendDenominationId`. Furthermore, since this value is used to calculate the dividend payout for an entity, the entity may receive a payout lower than expected because the system mistakenly assumes that the entity withdrew more dividends than it actually had. To illustrate the above issue, consider the following example: 1. Alice starts with `500 WETH` in its internal balance; 2. Alice calls `startTokenSale(ALICE, 100, 100 WETH)`, selling `100 ALICE` tokens for `100 WETH`; 3. Now Alice owns `100 ALICE` but they are locked; 4. Alice pays `100 WETH` as a dividend; 5. Alice pays `100 WETH` as a dividend; 6. Bob buys all `100 ALICE` from Alice. Here, during the transfer, Alice would have withdrawn the `200 WETH` dividend owed to her, so her balance is `600 WETH` (`300` + `200` for dividend + `100` from Bob's purchase); 7. Bob transfers all `100 ALICE` back to Alice; 8. Alice pays `500 WETH` as a dividend; 9. Alice tries to withdraw the `500 WETH` dividend but can only withdraw `100 WETH` due to the double counting issue and the other issue described in this report about Entity Token Holders not receiving future dividends;

There are currently some miscalculations related to dividend payout accounting. In particular, capital providers might not be able to get future dividends if they trade entity tokens with each other on the marketplace. The cause is in the function `LibTokenizedVault._internalTrasfer(_from, _to, ...)` where the values in the mapping `s.withdrawnDividendPerOwner` only ever increase. However, this means `s.withdrawnDividendPerOwner` will be increased for the previous owner even when they no longer own the entity token, as the tokens now belong to the new owner. Furthermore, following the comments in `DIVIDEND PAYOUT LOGIC` in `LibTokenizedVault`, `totalWithdrawnDividendPerOwner` is supposed to be reduced for the previous owner, but this is not the case in the actual implementation.

The function `LibHelpers._getAddressFromId(bytes32 id)` downcasts 32 bytes to an address, discarding the lower 12 bytes. In other terms, it will return the same address for different ids: * `id_a = 0x0102030405060708091011121314151617181920000000000000000000000000`; * `id_b = 0x0102030405060708091011121314151617181920FFFFFFFFFFFFFFFFFFFFFFFF`; </br>That function is used by several functions: * the function `UserFacet.getAddressFromExternalTokenId()` returns the same address for `id_a` and `id_b`; * it could lead to limit offer aliasing in the function `LibMarket._executeLimitOffer()`, in the special case where the parameters `_sellToken` or `_buyToken` represent one of two existing entities having the same 20 upper bytes. * the function `LibAdmin._isSupportedExternalToken()` returns true for `id_a` and `id_b`, even if only `id_a` represents an existing entity. As a result: * `AdminFacet.isSupportedExternalToken()` returns true for `id_a` and `id_b`; * simple policies and entities can be created with different asset ids such that `id != LibHelpers._getIdForAddress(LibHelpers._getAddressFromId(id))`. That could result later in unexpected behavior. * `LibTokenizedVault._payDividend()` could have unexpected behavior; </br>External endpoints that could be impacted are: * `MarketFacet.executeLimitOffer()` - executable by any user from an entity; * `TokenizedVaultFacet.payDividendFromEntity()` - executable only by the admin of an entity; * `EntityFacet.startTokenSale()` - executable only by sysMgr;

The function `LibACL._assignRole()` may grant an object ID the `ROLE_SYSTEM_ADMIN`, in which case the state variable `sysAdmins` is increased. While there exists the `LibACL._unassignRole()` function which revoke from a given object ID the admin role and correspondingly decreases the `sysAdmins` counter variable, it is possible to reassign an existing admin to another role using `LibACL._assignRole()`, which however will not decrease the `sysAdmins` counter, leading to incorrect accounting of the number of currently active `ROLE_SYSTEM_ADMIN` roles. The concrete impact would be that the check in `LibACL.sol#L60`, which checks that at least one `ROLE_SYSTEM_ADMIN` role remains in the system when unassigning a role, could falsely assume one or more `ROLE_SYSTEM_ADMIN` roles remain in the system, while in fact this is no longer the case and thereby leave the protocol in an inoperable state.

The lack of crucial checks when updating an Entity allows for an invalid state to persist on an Entity, ultimately denying the ability of this Entity to write new policies. In the function `LibEntity._updateEntity()`, `validateEntity()` is called to check that the `utilizedCapacity` of the new Entity does not exceed the `maxCapacity` for that Entity. In the case where there is a new `collateralRatio` being assigned to the Entity, there is a `newUtilizedCapacity` calculated for the Entity. This `newUtilizedCapacity` is not checked to ensure that it does not exceed the specified `maxCapacity`. As a result, an Entity can be updated to have a `utilizedCapacity` that exceeds the `maxCapacity`, which would deny the ability of this Entity to pass the checks required to write new policies. Currently, the balance of the Entity and the updated values are only checked if there is an update to the `collateralRatio` of the Entity. In the case where the `collateralRatio` of the Entity is not being updated, you can have an updated `utilizedCapacity` that exceeds the internal balance of that Entity. More generally, it appears that the `utilizedCapacity` of an Entity can be updated, even if the `collateralRatio` has not been updated. Logically, the utilized capacity of an Entity should only be updated if the collateral ratio is changed.

The payment of a premium can be done with the function `LibFeeRouter._payPremiumCommissions()`. Premium commissions can be defined at 2 levels: 1. at the level of the policy: a list of `commissionReceivers` is defined, each having a defined `commissionBasisPoints` fee assigned; 2. at the level of the system: three actors (NaymsLtd, NDF, STM) have dedicated `commissionBasisPoints` fees assigned: `premiumCommissionNaymsLtdBP`, `premiumCommissionNDFBP`, `premiumCommissionSTMBP`; </br>For the policy level, the function `LibEntity._validateSimplePolicyCreation()` makes sure that the sum of all commissionBasisPoints is `< 10000`. For the system level, the function `InitDiamond.initialize()` is used to initially define the three variables. Also, the function `LibFeeRouter._updatePolicyCommissionsBasisPoints()` is used to update the three variables and make sure that the sum is `< 10000`. However, none of the operations to create a policy and update system-level premium commissions makes sure that there is no policy in the system for which the sum of policy-level and system-level `commissionBasisPoints` is `> 10000`. In that situation, more than the premium amount to be paid could be spent by the entity. Alternatively, the payment could also fail if there is no sufficient amount to be spent. The likelihood is low, as only `sysAdmin` and `sysManager` can execute these functions. Also, in the same function, the event `PremiumCommissionsPaid` only records the sum of system-level commissions that have been paid. It does not record what has been paid for the policy-level commissions.

While the function `LibAdmin._addSupportedExternalToken()` does check whether or not the to-be-checked address `_tokenAddress` is an already supported external token, it does not check whether or not it refers to a wrapped participation token (see contract `./src/erc20/ERC20Wrapper.sol` - Note: that contract was out-of-scope for this audit). Adding such a token could lead to incorrect accounting of the underlying balances.

In order to validate the proper deployment and initialization of the contracts, it is a good practice to emit events. Also, any important state transition can be logged, which is beneficial for monitoring the contract, and also tracking eventual bugs, or hacks. Below we present a non-exhaustive list of events that could be emitted to improve the application management: 1. `LibObject._createObject()` (all 3 functions impacted): Changes made to `LibAppStorage.diamondStorage().existingObjects[]`, `LibAppStorage.diamondStorage().objectParent[]` and `LibAppStorage.diamondStorage().objectDataHashes[]`. 2. `LibObject._setDataHash()`: Changes made to `LibAppStorage.diamondStorage().objectDataHashes[]`. 3. `LibObject._setParent()`: Changes made to `LibAppStorage.diamondStorage().objectParent[]`. 4. `LibObject._enableObjectTokenization()`: Changes made to `LibAppStorage.diamondStorage().objectTokenSymbol[]` and `LibAppStorage.diamondStorage().objectTokenName[]`. 5. `LibAdmin._lockFunction()`: Changes made to `LibAppStorage.diamondStorage().locked[]`. 6. `LibAdmin._unlockFunction()`: Changes made to `LibAppStorage.diamondStorage().locked[]`. 7. `LibFeeRouter._payPremiumCommissions()`: The emitted event `PremiumCommissionsPaid()` does not take into account the policy-specific commissions (see lines `L21-L25`). 8. `LibFeeRouter._updateTradingCommissionsBasisPoints()`: Changes made to `LibAppStorage.diamondStorage().tradingCommission*` state variables. 9. `LibFeeRouter._updatePolicyCommissionsBasisPoints()`: Changes made to `LibAppStorage.diamondStorage().premiumCommission*` state variables.

The two-phase upgrade mechanism of the diamond contract is a way to prevent any upgrade can be executed by a single address, meaning the `newOwner` having also a `sysAdmin` role. Checks make sure in `NaymsOwnershipFacet.transferOwnership()` that is not possible to set a new contract owner if that owner also has a `sysAdmin` role. </br>However, the opposite can technically be done, meaning: 1. set a new contract owner using `NaymsOwnershipFacet.transferOwnership()`, only doable by a `sysAdmin` (due to the modifier `assertSysAdmin`); 2. executes `ACLFacet.assignRole()` to assign the role `sysAdmin` to the new contract owner; However, only a `sysAdmin` can perform that action.