Nayms

The `TokenizedVaultFacet.payDividendFromEntity()` can be called by anyone. The `LibTokenizedVault._payDividend()` function called under the hood would have the `_from` input as the `entityId`. Inside the `_payDividend()` function, it calls `_internalTransfer(_from, dividendBankId, _dividendTokenId, _amount)` to collect funds from the `_from` input (in the case here, the `entity`) to the virtual `dividendBankId`. The users can call this function to transform the assets of their entity to become dividends. Since anyone can call this, all entity members will be incentivized to immediately move all assets as dividends. The situation worsens as the `entity.asset` is supposed to be the same as the `simplePolicy.asset`. The attacker can immediately transform the funds for the insurance into a dividend.

The `LibMarket._matchToExistingOffers()` function can overflow on the following lines: ``` result.remainingSellAmount -= currentSellAmount result.remainingBuyAmount -= currentBuyAmount; ``` It can overflow because the calculation logic on L199-214 for the `currentSellAmount` and the `currentBuyAmount` only takes care of one `result.remainingSellAmount` side or the `result.remainingBuyAmount` side. The other side is a derived amount, which can be larger than what is left in the remaining. Once the function overflows, it will revert and block the offer execution, although the trade should be valid. From our analysis, overflow will only happen when the `buyExternalToken` is true. In the case of `buyExternalToken == false`, if the `currentSellAmount` is larger than the `result.remainingSellAmount`, it means it is trying to match a maker's offer that is worse than the taker's offer since the taker needs to sell more to get the same buying amount. However, in the opposite case, the overflow of the `remaingingBuyToken` means the maker's offer is better than the taker expects.

While creating a new `simplePolicy`, the function `_createSimplePolicy()` does not check that the policy `SimplePolicy.asset` variable is the same as `Entity.assetId`. This means a policy can have a different asset than the entity. It is also possible to update an entity's asset to become different than the rest of its policies through the function `_updateEntity()`.

The highest privileges on the system are owned by the users in the group `GROUP_SYSTEM_ADMINS`, which are the users with the role `ROLE_SYSTEM_ADMIN`. It is possible to revoke the role `ROLE_SYSTEM_ADMIN` from a user with the following methods: 1. `ACLFacet.assignRole()`: assign the admin a new role. 1. `ACLFacet.unassignRole()`: unassign the admin the role 1. `ACLFacet.updateRoleGroup()`: assign new groups to the `ROLE_SYSTEM_ADMIN`. However, only members of that group can call that function. Aside from that, additional roles can be added to the group `GROUP_SYSTEM_ADMINS` if a user with the role `ROLE_SYSTEM_ADMIN` calls the function `ACLFacet.updateRoleGroup()`. 1. `NaymsOwnershipFacet.transferOwnership()`: the contract owner can transfer ownership but cannot only revoke ownership. However, it can transfer ownership to the `0x0` address, effectively renouncing his contract ownership. As the current number of users in the system having the role `ROLE_SYSTEM_ADMIN` is not tracked, it is possible to remain without any user having system admin privileges.

The `LibTokenizedVault._getWithdrawableDividendAndDeductionMath()` can return a wrong `_dividendDeduction` value that adds one unexpectedly. Instead of checking against `_withdrawableDividend * _supply` on L276, it is supposed to check against `holderDividend * _supply`. Otherwise, whenever the `_withdrawnSoFar` is not zero, the `_dividendDeduction` would always be plus one.

Some facets inherit the `ReentrancyGuard` contract copied from the OpenZeppelin under `src/utils/`. However, the `ReentrancyGuard` implementation does not integrate with the diamond storage. The `ReentrancyGuard` will mutate a `_status` variable on the storage. The design of the `LibAppStorage.diamondStorage()` function (in the `AppStorage.sol`) uses slot zero instead of a randomized location. The `ReentrancyGuard` will override the data of the `AppStorage`. The current contract will continue to work as expected because the first slot of the `AppStorage` is reserved for the `nonces` mapping. In EVM, the storage slot for mapping will not be used (see: [blog](https://medium.com/coinmonks/solidity-tutorial-all-about-mappings-29a12269ee14#:~:text=In%20Solidity%2C%20mappings%20values%20are,key%20is%20hold%20in%20storage.)). The EVM relies on the slot number rather than the value inside the slot to hash to store and get the mapping value. However, if the variable `uint256 totalSupply` is the first variable in the `AppStorage` struct, then it will be overridden by the `ReentrancyGuard` contract. We set the severity as low because there is an immediate impact with the current codebase. However, this pattern is risky and might bring more considerable risks in the future.

The `LibEntities._createSimplePolicy()` function assigns the stakeholders their roles. However, the stakeholders can be duplicated with the input. Thus, the for-loop might unexpectedly override the later role.

The logic handling dust order is inconsistent between the `_createOffer()`, `_takeOffer()`, and `_executeLimitOffer()` functions of the `LibMarket` contract. The dust condition in the `_createOffer()` function is `<= LibConstants.DUST`; but the condition in the `_takeOffer()` is `< LibConstants.DUST`. Also, in the `_executeLimitOffer()` function, the condition to inject into the sorted list is not an inverse of the dust condition used in the `_createOffer()` function. For instance, when the `remainingBuyAmount` and `remainingSellAmount` are non-zero and `<= LibConstants.DUST` (e.g., one) in the `_executeLimitOffer()` function, the offer will be flagged as `LibConstants.OFFER_STATE_FULFILLED` on `_createOffer()#L247`. In contrast, the `_executeLimitOffer()` will continue to inject the already fulfilled order into the sorted list.

In the function `_assertValidOffer()`: * the first require statement checks that an entity identified by `_entityId` already exists, but the error message is `must belong to entity to make an offer`; * the local boolean variable `sellTokenIsEntity` gets the value `true` if the token ID matches an existing entity. But it does not check that the token ID matches the entity ID. The same issue is for the local boolean `buyTokenIsEntity`.

While creating an entity, the code requires one of two things (see: `LibEntity.validateEntity()`): * `_entity.simplePolicyEnabled` to be false * `_entity.maxCapacity` should be greater than 0 However, the function `_updateAllowSimplePolicy()` can then be used to enable the `simplePolicyEnabled` for an entity with a `maxCapacity` of zero. Meanwhile, it is unclear whether it is acceptable to flag it back from `true` to `false` when the `_entity.maxCapacity` is non-zero.