dloop Art Registry Smart Contract security record

Input validation is missing in multiple places (line numbers are for the commit `42be24b`): 1. `DLoopArtwork.sol`, `L22`: The `artworkId` parameter is not checked to be greater than zero, and this violates the requirement: `The artwork id must always be a positive integer`. **Fixed** in `2ccd432`. 1. `DLoopArtwork.sol`, `L25` and `L72`: `dataType` is not currently validated, and therefore, it can be any `bytes32` value. However, `0x0` is used as a special value in the check on `L43` in `DloopMintable.sol`, which suggests that `dataType` should be checked to differ from `0x0` in `DLoopArtwork.sol`. **Fixed** in `2ccd432`. 1. `DloopAdmin.sol`, `L30`: `addAdmin(address account)` should check that `account` is a non-zero address. **Fixed** in `2ccd432`. 1. `DLoopArtwork.sol`: `L26` and `L73`: the `data` parameter is not validated. It could be an empty array, which means it will not contain any hashes. Also, it could be a really long array that may potentially lead to risks of exceeding block gas limits. It is **strongly recommended** to enforce a minimum and a maximum length or, if known, an exact format. Similarly, validation of `data` is missing in `DloopMintable.sol` on `L44` and `L53`. **Fixed** in `2ccd432`. 1. `DloopManagedToken.sol`, `L14`: `_setManaged(...)` does not check if the `tokenId` exists. While in the usage contexts token existence seems to be the case, it is still recommended to add a check into the `_setManaged(...)` method itself. **Fixed** in `2ccd432`.

When tokens are managed, minters can move tokens on behalf of a regular user, and it is expected. However, in the current implementation, the regular user is also able to move their managed tokens to a new holder, independently from minters (without authorization from minters). Those tokens remain managed after a transfer is complete, but the new token holder may be unaware of the fact that their tokens are still managed, and therefore, the tokens could be taken away from them by the minters. The specification does not seem to be very clear with regards to this subject. The Google doc says "the platform can move the token internally between managed user accounts". The PDF on Dropbox, while stating that managed tokens are "Only dloop ecosystem controlled / internal tokens", also states that "Technically, the user is able to transfer the tokens too".

The `splitTokenId(...)` method, `L42-44` (commit `42be24b `): after the bitwise shifts, it is suggested to check if the resulting `artworkId`, `editionNumber`, and `artistProofNumber` are still valid and meet the criteria that were originally imposed in `createTokenId(...)`.

It is currently possible for an admin to remove all minters using the `removeMinter(...)` method. If no active minters are present, the `onlyMinter` modifier reverts for any Ethereum address, and therefore, the `mintEdition(...)` and `addEditionData(...)` methods are going to revert. Consider the case when the minter role is enabled (`_minterRoleEnabled` is set to `true`) but there are no active minters. This case could be misleading to both regular users and admins, because when the minter role is enabled, there could be a false expectation that there is at least one active minter. However, there is currently no easy way for admins or users to verify if there are any active minters: `_minterMap` is a map that cannot be traversed, and there is no variable storing the number of minters. So, users would need to know the actual minter address or iterate over past blocks to find `MinterAdded` events.

Every Solidity file specifies in the header a version number of the format `pragma solidity (^)0.5.*`. 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".