OpenSea security record

The `auctions` mapping is first indexed by the `contractAddr` followed by the `tokenId`. However, in the case of ERC1155 tokens, there may be more than 1 supply of each `tokenId`. This is apparent in the `token.safeTransferFrom(_from, _to, _tokenID, 1, DEFAULT_MESSAGE)` call, where the 1 indicates the amount of `_tokenID` tokens to transfer. Suppose a `(contractAddr, tokenId)` pair has a supply larger than 1, and an auction is launched for one instance of this token. If a new auction is started for a different instance, the `auctionInfo[contractAddr][tokenId]` data will be overwritten, wiping previous auction data, and eventually locking 1 instance of the token in the contract forever.

A reentrancy vulnerability is a scenario where an attacker can repeatedly call a function from itself, unexpectedly leading to potentially disastrous results. Here's a basic example representing the very attack which impacted The DAO in 2016: ``` function withdraw_with_reentrancy(uint256 _amount) public { msg.sender.call.value(_amount)(); // ATTACKER CAN CALL AGAIN BEFORE // FUNCTION TERMINATES because `call` // allows msg.sender to execute any code using fallback function balances[msg.sender] -= _amount; // Balance only updated AFTER funds withdrawn } ``` Specifically, re-entrancy is present in `launchAuction()`, `launchReservedAuction()` due to the use of `transferToken()`.

Functions `NFTAuction.vipBid()` and `NFTAuction.Bid()` only check if the bid is within the allowed time window of the auction, but does not take into account the status of the auction (Unsold), allowing one to bid on cancelled auctions so long as it falls in the allowed time frame.

In contract `NFTStore.sol` the native `.transfer()` function is used to transfer ETH, which only forwards `2300` gas and reverts on error. As during `vipBuy()` and `Buy()` funds are always forwared to all shares, a malicious `shareAddr` could prevent the sell operation, by consuming more than `2300` gas on ETH receival or otherwise err. The same applies for the `storeAddress` and `product.seller` address.

In order to validate the proper deployment and initialization of the contracts, it is a good practice to emit events. Also, any important state transition 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 events that could be emitted to improve the application management: 1. `ERC721PresetHTC.setContractUri()` does not emit an event reflecting changes made to the state variable `contractUri`. (**Update:** Fixed) 1. `ERC721PresetHTC.setBaseUri()` does not emit an event reflecting changes made to the state variable `_baseTokenURI`. (**Update:** Fixed) 1. `ERC721PresetHTC._setTokenURI()` does not emit an event reflecting changes made to the state variable `_tokenURIs[]`. (**Update:** Fixed) 1. `NFTAuction.setPriceLimit()` does not emit an event reflecting changes made to the state variables `priceThreshold` and `priceLimit`. (**Update:** Fixed) 1. `NFTAuction.setMerkleRoot()` does not emit an event reflecting changes made to the state variable `merkleRoots[]`. (**Update:** Fixed) 1. `NFTStore.setStoreData()` does not emit an event reflecting changes made to the state variables `storeAddress` and `storeDefaultProfit`. (**Update:** Fixed) 1. `NFTStore.setMerkleRoot()` does not emit an event reflecting changes made to the state variable `merkleRoots[]`. (**Update:** Fixed)

In the `finalizeAuction()` function, the caller is the `seller` of the auction and also has the `LAUNCH_ROLE`. The seller should not have access to launch new auctions, but should still be able to finalize their previous auctions. If before calling `finalizeAuction()` the admin revokes `LAUNCH_ROLE` for the `seller` the auction remains in open status and funds get locked in the contract. This is the result of mixing seller and launcher roles in one `LAUNCH_ROLE` role.

Avoid leaving a contract uninitialized. An uninitialized contract can be taken over by an attacker. This applies to both a proxy and its implementation contract, which may impact the proxy.

Storage variable `storeDefaultProfit` in contract `NFTStore.sol` is stored as an `uint256` variable, although, by business, it should not be able to exceed `10**4` (`BASE_PERCENT`). In L98 a primitive cast operation is performed `uint16(storeDefaultProfit)`, potentially leading to truncation, as the aforementioned bound is not enforced during `__NFTMarket_init()` and `storeDefaultProfit` could exceed `type(uint16).max`.

These two functions return 0 when the external call is not successful. This may lead to burned funds, particularly when `getAddrDataFromContract()` is used to transfer creator profit in the `acceptOffer()` function. Further, it is not clear why the functions `getUintDataFromContractI()` and `getAddrDataFromContract()` exist at all. It seems the relevant functionality could be represented through an interface that expects the function signatures `creatorOf(uint256)` and `creatorProfitOf(uint256)`.

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".