CapsuleNFT security record

Although the inline comment states that the function `CapsuleFactory.updateCapsuleCollectionOwner(...)` can be called only by the `Capsule Collection` as part of the `transferOwnership(...)`, we notice that this function has `external` visibility and, apparently, no access control. This function transfers the ownership from `_previousOwner` to the `_newOwner` without any consentiment from any of the parties. The function is reproduced below. ``` function updateCapsuleCollectionOwner(address _previousOwner, address _newOwner) external { address _capsule = _msgSender(); require(isCapsule[_capsule], Errors.NOT_CAPSULE); require(capsulesOf[_previousOwner].remove(_capsule), Errors.ADDRESS_DOES_NOT_EXIST); require(capsulesOf[_newOwner].add(_capsule), Errors.ADDRESS_ALREADY_EXIST); emit CapsuleOwnerUpdated(_capsule, _previousOwner, _newOwner); } ```

The function `Capsule.lockCollectionCount(...)` does not provide an upper bound on the number of tokens that each collection can have. This can lead to gas concerns in use cases where the application has to go over the entire collection. We could not find any loop over the entire collection in the piece of code audited. However, it is safer to keep some limits in order to avoid Denial-of-Service attacks.

The function `CapsuleFactory.updateCapsuleCollectionTax(...)` can set arbitrary values for the collection tax. Similarly, the function `CapsuleMinter.updateCapsuleMintTax(...)` can also set arbitrary values for the mint tax. That means that there is no upper bound for the taxes, which can lead to reduced gain for users, or even fund losses. It is important to clearly advertise to users the taxes that they will have to face, and also impose thresholds in order to keep the application financially healthy, while guaranteeing the interest of users by providing interesting and competitive gains for users. The function `updateCapsuleCollectionTax(...)` is shown below: ``` function updateCapsuleCollectionTax(uint256 _newTax) external onlyOwner { require(_newTax != capsuleCollectionTax, Errors.SAME_AS_EXISTING); emit CapsuleCollectionTaxUpdated(capsuleCollectionTax, _newTax); capsuleCollectionTax = _newTax; } ``` The function `updateCapsuleCollectionTax(...)` is shown below: ``` function updateCapsuleMintTax(uint256 _newTax) external onlyOwner { require(_newTax != capsuleMintTax, Errors.SAME_AS_EXISTING); emit CapsuleMintTaxUpdated(capsuleMintTax, _newTax); capsuleMintTax = _newTax; } ```

The function `Capsule.mint(...)` calls `IERC721._mint(...)` instead of `IERC721._safeMint(...)`. However, OpenZeppelin discourages the use of `IERC721._mint(...)`. The `_safeMint(...)` flavor of minting causes the recipient of the tokens, if it is a smart contract, to react upon receipt of the tokens. Here is a general consideration to help you decide which one to use: 1. If the application is paying for the minting of tokens, consider use `_mint(...)`. The `_safeMint(...)` might cost you an arbitrary amount of money because of choices made by the recipient of the tokens. This is enough to deter you from considering it; 2. If the other party is paying for the minting of tokens and you expect buyers to be composing functionality with smart contracts, use `_safeMint(...)`. There is marginal benefit in allowing the extra features with certain smart contracts; 3. If the other party is paying and you expect EOA to buy tokens, then use `_mint(...)`. The extra features in `_safeMint(...)` are not expected. The extra cost from using `_safeMint(...)` is non-zero. The function `Capsule.mint(...)` is shown below: ``` function mint(address _account, string memory _uri) external onlyMinter { require(!isCollectionLocked(), 'collection is locked'); _mint(_account, counter); _setTokenURI(counter, _uri); counter++; } ```

- `CapsuleFactoryStorage` inherits from `ICapsuleFactory` but does not implement any function. - `CapsuleMinterStorage` inherits from `ICapsuleMinter` but does not implement any function. This can lead to calls to not-implemented functions.

Ideally, every important state change should be recorded by emitting events describing the past and the new state. A non-exhaustive list of functions that could emit events for improving post-deployment contracts monitoring is presented below: ``` - CapsuleFactory.initialize() - CapsuleFactory.addToWhitelist(...) - CapsuleFactory.removeFromWhitelist(...) - CapsuleFactory.addToBlacklist(...) - CapsuleFactory.removeFromBlacklist(...) - CapsuleFactory.flushTaxAmount(...) - CapsuleFactory.updateCapsuleCollectionTax(...) not showing the past fee - Capsule.lockCollectionCount(...) - CapsuleMinter.initialize(...) - CapsuleMinter.addToWhitelist(...) - CapsuleMinter.removeFromWhitelist(...) - CapsuleMinter.flushTaxAmount() - CapsuleMinter.updateCapsuleMintTax(...) not showing the past mint tax ```

The `Ownable.sol` has the function `renounceOwnership()`. Although it can only be called by the `owner`, such a function leaves the contract without an owner, which surely compromises any ability to manage the contract. Several critical operations can only be performed by the `owner`. In case the `owner` is accidentally removed, the operations of a) adding and removing users in the whitelist and blacklist are blocked, b) the creation of new Capsules, c) the definition of entities responsible for minting Capsules, d) the collection of taxes/fees, e) the minting of new tokens, and f) the operations of lock collections, all this considering only the files that are in the scope of this audit. In practical terms, it would not be an exaggeration to say that the application would become unusable.

Some functions receiving input addresses do not check if they are different from `address(0x0)`, nor that the given addresses refer to a contract (when applicable). Setting addresses incorrectly can lead to incorrect setups that will prevent contracts from working as expected. A non-exhaustive list of functions not sanitizing input addresses is presented below: ``` - CapsuleFactory.addToWhitelist(...) - CapsuleFactory.addToBlacklist(...) (only useful for reducing the storage space) - CapsuleFactory.updateCapsuleCollectionOwner(...) ```

As one can note from the code snippet presented below, the very same address can be added to both `whitelist` and the `blacklist` at the same time. This use case and the code itself is not clear concerning of what happens if an address is added to both lists. The code of both functions is reproduced below. ``` function addToWhitelist(address _user) external onlyOwner { require(whitelist.add(_user), Errors.ADDRESS_ALREADY_EXIST); } function addToBlacklist(address _user) external onlyOwner { require(blacklist.add(_user), Errors.ADDRESS_ALREADY_EXIST); } ``` It is unclear what happens to the system if the same address is added to both lists. Perhaps the best solution is to ensure that the same user cannot integrate both lists. Moreover, it is unclear what happens if a Capsule owner is added to the `blacklist`, and how such an action may damage the users having funds locked in the Capsule. The code of the functions `addToWhitelist(...)` and `addToBlacklist(...)` are reproduced below. ``` function addToWhitelist(address _user) external onlyOwner { require(whitelist.add(_user), Errors.ADDRESS_ALREADY_EXIST); } function addToBlacklist(address _user) external onlyOwner { require(blacklist.add(_user), Errors.ADDRESS_ALREADY_EXIST); } ```