NFTonPulse

Note that in [ERC-20 standard](https://eips.ethereum.org/EIPS/eip-20) it is stated: > The [transfer] function SHOULD throw if the message caller’s account balance does not have enough tokens to spend. and: > Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned! Hence, the following lines in the function `NFTMarketOffers.acceptOffer()` are problematic: ``` IERC20(offer.erc20Address).transferFrom( offer.offerer, msg.sender, acceptQuantity * offer.singleOfferPrice - commission - royalty ); // Transfers commission IERC20(offer.erc20Address).transferFrom(offer.offerer, owner(), commission); ``` Given that there is no check to ensure the offerer has sufficient balance, this renders the above lines of code more dangerous. Similar lines can be found in the `NFTMarketAuction.settleAuction()`function, though there is a check for both balance and allowance, reducing the probability of a failed transfer. As these lines are implicitly assuming that the contract at `priceTokenAddress` will throw if the `transferFrom()` fails, which is not strictly required by the ERC20 standard. Relatedly, there is no handling to ensure that the return value of the `transferFrom()` call is `true`. Of course, the impact of a failed transfer that returns `false` rather than reverting would mean that auctions can be settled and the NFT transferred to the bidder without a payment occurring on their part.

Various transfers of Ether ignore the success of a transfer. Therefore, the Ether will be stored in a contract that was not intended to store Ether. The lack of a withdraw function will cause the Ether transferred to these contracts to be locked forever. Return values for `call()` should be checked to ensure success. This occurs in: * `NFT721.redeem()` * `NFT1155.redeem()` * `NFTMarketSimple.sendAssets()`

The `NFTVoucher` for `NFT1155` contract does not include a nonce. In the case of `NFT721`, there is no need for a nonce, since the `tokenId` acts as an implicit nonce. Since you cannot mint more than one token of the same ID. However, for ERC1155, you can mint multiple tokens with the same `tokenID`. This means that the voucher can be replayed multiple times to mint more than once. The function attempts to mitigate that by adding the check: ``` require( _tokensMinted[voucher.tokenId] + amount <= voucher.amount, 'Exceeded mint limit' ); ``` However, this only allows a max amount of `voucher.amount` for `voucher.tokenId` which might not be the desired behavior with ERC1155 tokens.

In the docs it is stated: > If user wants to buy a direct sale item that has a sale price of 100 weis and commission is 1%, the users needs to pay 101 weis. However, whenever a commission fee is taken in the code, it is always subtracted from the amount that the user pays to the seller (i.e. the seller pays the commission fee, not the buyer). To take one example from `NFTMarketOffers.acceptOffer()` we have: ``` uint256 commission = getPriceAfterPercent( offer.singleOfferPrice, acceptQuantity, commissionPercent ); // Transfers royalty uint256 royalty = handleErc20Royalty( offer.nftContract, offer.tokenId, offer.erc20Address, offer.offerer, acceptQuantity * offer.singleOfferPrice ); // Transfers price IERC20(offer.erc20Address).transferFrom( offer.offerer, msg.sender, acceptQuantity * offer.singleOfferPrice - commission - royalty ); // Transfers commission IERC20(offer.erc20Address).transferFrom(offer.offerer, owner(), commission); ``` Where `handleRoyalty()` function makes the user pay `royalty`. Adding up all the user's payments gives: ``` royalty + (offer.singleOfferPrice - commission - royalty) + commission ``` which amounts to paying the price of their offer rather than their offer plus commission.

The voucher design allows for the case where anyone can create a voucher and sign it, since the marketplace only checks if the voucher signer is the `voucher.creator`. But it does not check if the signer is authorized to sign vouchers.

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 events that could be emitted to improve application management: 1. MarketTools 1. `addToWhitelist()`: newly whitelisted tokens. 1. `removeFromWhitelist`: tokens removed from the whitelist 1. `setCommisionPercent()`: new commission fee. 1. `setSimpleCommisionPercent()`: new simple commission fee.

Instead of specifically checking for approval on the token needed for the auction, `MarketTools.checkNFTAllowance()` checks to see if the owner has permitted them to use all the NFTs. Narrowing the scope of the allowance would conform to the cybersecurity principle of least privilege.

When the first listing in one of the market contracts is created, the counter is incremented before assigning the ID to the market item. This means that listing IDs start at `1`. In most of the functions, the validation to check if a listing exists is: ``` require(listingId <= _listingIds.current(), "Listing doesn't exist"); ``` The statement will be valid for `listingId = 0` for all cases, even if no listings were created yet.

It is important to validate inputs, even if they only come from trusted addresses, to avoid human error. We recommend adding the following validation checks: 1. MarketTools 1. `constructor()`: * `erc20TokenAddress != address(0)` or `erc20TokenAddress.isContract()` (alternatively call the `addToWhitelist()` function from the constructor). 1. `addToWhitelist()`: * `erc20TokenAddress != address(0)` or `erc20TokenAddress.isContract()`. 1. `transferNFT()`: * `sender != address(0)` * `receiver != address(0)` * `amount != 0` 1. NFTMarketAuction 1. `acceptBid()`: * `listingId > 0` (see issue: "Listing Ids Start at 1") 1. NFTMarketOffers 1. `cancelOffer()` * `offerId <= _offerIds.current()`(No need to check if the offerer is address(0) if id exists) 1. NFTMarketSimple 1. `buySimple()` * `listingId > 0` * require the seller to have balance >= quantity

Both `NFT721` and `NFT1155` allow users to overpay for NFTs when redeeming a voucher.