Decentraland Marketplace V2

# Decentraland Marketplace V2  ###### tags: `Public`,`Results` ## Project Info * repo - <https://github.com/decentraland/marketplace-contracts/pull/56> * commit - `86de52dd82978746537a7c65be28df6fc1fc1885` * scope - MarketplaceV2.sol + RoyaltiesManager.sol * SLOC-P - 450 * documentation - <https://github.com/decentraland/proposals/blob/master/dsp/dsp-0013/0013.md> ## Project assessment There are four non-code aspects of the project that we assess as part of the audit: * documentation - OK * compilation - OK * tests - 91/91 OK, coverage 75% * dependency management - OK  ## Code Issues :::info Most issues are described in format * `<contract>.<function>` lines (\<severity\>, \<type\>) - description Severity levels are: * critical - loss of funds or complete fail of project * medium - the contracts don't behave as expected, but assets are safe * low - doesn't really affect operations, but might lead to a more serious problem in the future versions of the code Type loosely describes what is this issue about: * code - the code doesn't follow the best practices. Overall idea/design is fine, but the implementation could be improved. * logic - the code's logic is incorrect, as it is a bug, or a missed edge case, or improper integration with other contract * gas - an opportunity to save gas * docs - documentation issue ::: * `MarketplaceV2` (med, logic) - the contract is `Pausable`, however `pause` and `unpause` functions (with proper access control over calling corresponding internal functions) are not implemented. Thus, there is no real possibility to make an actual pause. * `NativeMetaTransaction.executeMetaTransaction` (low, code) - function is marked as payable, but no ether transfer specified in the contract. Consider removing `payable` keyword to prevent locked money issue. * `RoyaltiesManager.getRoyaltiesReceiver` L46 (low, code) - consider returning a `struct` instead of separate values in `IERC721CollectionV2` interface to avoid comma notation, which is error-prone. * `RoyaltiesManager` (low, code) - since royalties is relatively new area, it is not yet standardized. Nevertheless, implementing one of the proposed standards (i.e. [EIP 2981](https://eips.ethereum.org/EIPS/eip-2981)) might be useful for future integration. * `RoyaltiesManager.getRoyaltiesReceiver` L32 (low, code) - avoid returning non-initialized values. In this case return value might be explicitly set to `address(0)`. * `MarketplaceV2` L20 (low, gas) - Variable `acceptedToken` is set during contract deployment and never changes later. Declare it as `immutable` to reduce gas consumption. * `MarketplaceV2._requireERC721` L469 (low, code) - redundant `isContract()` check, since the compiler includes this check before external calls. * `MarketplaceV2` (low, code) - Consider declaring functions as `external` instead of `public` when possible to improve code readability and optimize gas consumption. * `MarketplaceV2` (low, docs) - most of NatSpec comments do not include `@return` parameter, consider adding it to description.