Nayms 2024 (Retainer) security record

In the function `_unstake()`, the stake balance of the entity is not reduced for the current interval, namely `s.stakeBalance[vTokenId][_entityId]` is not reduced by the amount unstaked. This is problematic because a user can stake and unstake in the current interval, but this only increases the stake balance of the entity for the current interval. This means the actual amount of tokens staked in `_entityId` is not accurately represented by the `stakeBalance` mapping. This in turn leads to wrong accounting in `_payRewards()` if the entity admin decides to pay rewards at the current interval after the user's unstaking. In particular, some rewards will be unclaimable because a portion of the rewards is allocated to the "unstaked" user. Furthermore, an attacker can deny other users from claiming their rewards by staking a huge amount and then immediately unstaking.

As staking happens on behalf of a users' entity and due to missing checks, it is possible for users of a staking entity to stake into their entity on its behalf. In particular, this could lead to following accounting issues: 1. `LibTokenizedVaultStaking._stake()`: Double accounting in `L124`, `L129` and `L142-L147`. 2. `AppStorage.stakeCollected[][]`: Variable is used to track a staker's last collected interval as well as a staking entity's last paid out reward interval, leading to incorrect last interval accounting for users of a staking entity.

The Nayms platform uses a `bytes32` identifier to distinguish the different objects in the system and store relational attributes between them in mappings, for instance, the token balances of each object, or the access permissions between objects. The mapping `existingObjects(bytes32 => bool)` stores the fact that an object with a specific identifier exists or not. However, not all objects identified by an identifier are in this mapping. For instance, roles and groups have an identifier but are not considered as existing in the system. This could lead to collisions when we can create an object `b` with the same identifier as the object `a` which is already used by the system but is not an existing object according to the mapping. One example of a negative consequence is illustrated in the Exploit section below. Another example is the fact that, when the first address is granted a `sysAdmin` role in `LibInitDiamond.setSystemAdmin()`, the associated identifier of this address becomes an existing id (mapping `existingObjects`). However, when other `sysAdmin` roles are granted via `LibACL._assignRole()`, the addresses receiving that role do not become existing objects. Finally, the concept of object type was introduced in the last update of the codebase and adds to the identifier of an object `12` bytes to define the type of the object. However, objects that were created before this update still exist in the system but do not have in their identifier these `12` bytes to define their type. As a result, if in the future, some features check that a given object must be of a given type (for example: a policy), legacy objects created before the update will not be compatible with this new feature.

In `_unstake()`, we only set `s.stakeBoost[vTokenId][_stakerId] = 0;` back to zero, but we do not set the stake boost for the next `vTokenId` back to zero. When a user stake in the current interval, part of the stake boost will be allocated to the next `vTokenId` proportional to the time it was staked between the current interval and the next interval, as shown in the following lines: ``` uint256 boostTotal = (_amount * _getA(_entityId)) / _getD(_entityId); uint256 boostNext; if (_isStakingInitialized(_entityId)) { boostNext = (boostTotal * (block.timestamp - _calculateStartTimeOfCurrentInterval(_entityId))) / s.stakingConfigs[_entityId].interval; } uint256 boost = boostTotal - boostNext; // give to the staker s.stakeBoost[vTokenId][_stakerId] += boost; s.stakeBoost[nextVTokenId][_stakerId] += boostNext; ``` Since the stake boost is never reset for the next `vTokenId`, an attacker can inflate the stake boost for the next interval by repeatedly staking and unstaking in the current interval. Then, at the start of the next interval, an attacker can simply stake a small amount, but he will be able to claim more rewards than should be allowed due to the inflated boost.

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 `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 function `ACLFacet.updateRoleGroup()` allows system administrators to change the group membership for a given role. While the following check prevents changing the membership to group `LC.GROUP_SYSTEM_ADMINS`, it can be bypassed: ``` require(!LibString.eq(_group, LC.GROUP_SYSTEM_ADMINS), "system admins group is not modifiable"); ``` In details, this check can be bypassed by providing a `_group` string with the value `LC.GROUP_SYSTEM_ADMINS` padded with enough zero bytes such that the function `eq()` returns `false` but the function `LibACL._updateRoleGroup()` still uses the value of `LC.GROUP_SYSTEM_ADMINS` (i.e. `System Admins\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000` - The string `System Admins` padded with null-bytes to 32 bytes length, which may be passed like this or may contain arbitrary other bytes afterwards).

The function `TokenizedVaultFacet.internalTransferFromEntity()` is no longer protected by an "onlyEntityAdmin" modifier. Since the fact that the parent of `msg.sender` is not checked to be an entity, any address being the child of an object id owning tokens can trigger a transfer of any token to an arbitrary other internal address. This could lead to unexpected consequences in the current version of the system and future ones as well.