Unikura

In `UnikuraMothership.sol`, `_feePercentage` is declared as *The percentage of mintPrice required as sales fee*. However, the function `setFeePercentage()` does not regulate the value of `_feePercentage` range. Also, the concept of `feePercentage` is not defined in the documentation.

In the contract `UnikuraPhygitalCollection`, all the tokens are minted to the sales address upon initialization. Later when a token is minted, the function `mint()` is called, and the function `_mintTransferFrom()` directly operates over the `ERC721AStorage` contract. This function compares `prevOwnershipPacked` with `UnikuraPhygitalCollectionStorage.layout()._salesAddress` to ensure that the token owner is the sales address. The problem here is that the function `setSalesAddress()` only update the `_salesAddress` within `UnikuraPhygitalCollectionStorage.layout()`, but it does not effectively transfer the the minted tokens to the new address and update `ERC721AStorage`. As a result, the `_mintTransferFrom` will fail after `setSalesAddress()` is called. The system has to `mint()` all the tokens again to the new sales address before calling `setSalesAddress()`, which is not only expensive but not comply with the expected system design.

In `UnikuraPhygitalCollection.mint()`, the function accepts payment more than required. The function accepts `msg.value` as payment, and the payment check logic is `require(msg.value >= (totalPrice + totalFee), "Insufficient payment");`. The function does not revert or return the extra payment to the sender when `msg.value` is greater than `totalPrice + totalFee`. Since there is no function for the admin to withdraw the extra payment, the extra payment will be locked in the contract forever.

Since `_velvettFeeRecipient` and `_feePercentage` are not set upon initialization of the `UnikuraMothership` contract, there can be a scenario where the team accidentally forgets to setup those values prior to Physical collection mint, resulting in no fee collection.

In order to validate the proper deployment and initialization of the contracts, it is a good practice to emit events. Also, any important state transitions 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 functions that could emit events to improve application management: 1. `UnikuraMembership` 1. `setSalesAddress()` 2. `setBurnAddress()` 2. `UnikuraPhygitalCollection.setBaseURI()`

The owner of the contracts can call `transferOwnership()` to transfer the ownership to a new address. If an uncontrollable address is accidentally provided as the new owner address, then the contract will no longer have an active owner, and functions with the `onlyOwner` modifier can no longer be executed.

The `UnikuraMembership.mint()` function allows the owner to mint any token id to any address. However, currently, only two token ids are defined, as shown below: ``` uint256 public constant GOLD = 0; uint256 public constant SILVER = 1; ``` However, tokens with ids other than `0` or `1` can be minted, and these tokens appear not to have any meaning in the current code. It is possible that these other ids may have another meaning in the future, and minting them accidentally at this point could lead to unexpected consequences. Also, the constants `0` and `1` are typically used to represent boolean logic `false` and `true`, respectively. Such design can be misleading.

The following validation checks are missing from various contracts. The lack of these checks may lead to the contracts entering an incorrect state. 1. `UnikuraMembership` 1. `mint()`: Validate that `id` can only be `0` or `1` , that is, gold or silver. 2. `setURI()`: Validate that `newUri` is not an empty string. 3. `setSalesAddress()`: Validate that `newSalesAddress` is not the zero address. 4. `setBurnAddress()`: Validate that `newBurnAddress` is not the zero address. 2. `UnikuraMothership`: 1. `setFeePercentage()`: Consider enforcing a maximum value for `feePercentage_`. 2. `setVelvettFeeRecipient()` : Validate that `velvettFeeRecipient_` is not the zero address. 3. `setUnikuraMembershipContract()`: Validate that `unikuraMembershipContract_` is not the zero address. 3. `UnikuraPhygitalCollection` 1. `setBaseURI()`: Validate that `baseURI_` is not an empty string. 2. `setUnikuraMothershipContract()`: Validate that `unikuraMothership_` is not the zero address. 3. `setSalesAddress()`: Validate that `salesAddress_` is not the zero address. 4. `setMintPrice()`: Consider validating that `mintPrice_` is not zero if appropriate.

If the owner renounces their ownership, all ownable contracts will be left without an owner. Consequently, any function guarded by the `onlyOwner` modifier will no longer be able to be executed. The contracts `UnikuraPhygitalCollection`, `UnikuraMothership` and `UnikuraMembership`currently rely on an owner for important administrative functionality.

Every Solidity file specifies in the header a version number of the format `pragma solidity (^)0.8.*`. The caret (`^`) before the version number implies an unlocked pragma, meaning that the compiler will use the specified version _and above_, hence the term "unlocked".