Foundation contest - hubble's results

Summary of Findings

Issue#1 : makeOffer for non existent NFT id is allowed and amount is locked Issue#2 : Input check for array lengths missing in adminCancelOffers Issue#3 : Unused inheritance in NFTMarketFees contract Issue#4 : NFT Owner should not be allowed to makeOffer for its own NFT ids

Details Issue#1

Title : makeOffer for non existent NFT id is allowed and amount is locked

Impact

If makeOffer for a non existent NFT id is created, the foundation marketplace allows it and locks the user funds for 24 hours. Its expected that foundation marketplace should not accept such request for Offer for invalid NFT id's and reject transaction.

Proof of Concept

A brief test case is given below with a token id 99999 which is non existent, should fail. but its passing.

  describe("`makeOffer` for a non-existing tokenId", () => {
    const tokenIdNonExisting = 99999;

    beforeEach(async () => {
      tx = await market.connect(collector).makeOffer(nft.address, tokenIdNonExisting, price, { value: price });
      expiry = await getFethExpectedExpiration(tx);
    });

    it("Emits OfferMade", async () => {
      await expect(tx).to.emit(market, "OfferMade").withArgs(nft.address, tokenIdNonExisting, collector.address, price, expiry);
    });

  });

Recommended Mitigation Steps

Check for valid NFT id , by checking if owner exists for an NFT id, and then process any request if valid. Sample test case which should pass after the code fix changes.

  it("revert if Nonexisting NFT token ID", async() => {
      const tokenIdNonExisting = 99999;
      await expect(nft.ownerOf(tokenIdNonExisting)).to.be.revertedWith(
      "VM Exception while processing transaction: reverted with reason string 'ERC721: owner query for nonexistent token'",
    );
  });

Details Issue#2

Title : Input check for array lengths missing in adminCancelOffers

Impact

There is no check for the array length of nftContracts and tokenIds in the function adminCancelOffers of NFTMarketOffer.sol due to which some of the tokenIDs may not get cancelled.

Proof of Concept

Contract : NFTMarketOffer.sol

  function adminCancelOffers(
    address[] calldata nftContracts,
    uint256[] calldata tokenIds,
    string calldata reason
  ) external onlyFoundationAdmin nonReentrant {
    if (bytes(reason).length == 0) {
      revert NFTMarketOffer_Reason_Required();
    }

Recommended Mitigation Steps

Check and revert if nftContracts.length != tokenIds.length

Details Issue#3

Title : Unused inheritance in NFTMarketFees contract

Impact

There is unused inheritance in the NFTMarketFees abstract contract.

Proof of Concept

Contract : NFTMarketFees.sol line#18

abstract contract NFTMarketFees is
  Constants,
  Initializable,
  FoundationTreasuryNode,
  NFTMarketCore,
  NFTMarketCreators,
  SendValueWithFallbackWithdraw
{ ...

Recommended Mitigation Steps

Delete the inheritance use of NFTMarketCore and NFTMarketCreators

Details Issue#4

Title : NFT Owner should not be allowed to makeOffer for its own NFT ids

Impact

The foundation contract allows for the NFT owner to makeOffer for its own NFT id, which may not be a valid business case. The owner can make a very high offer so as to invalidate other offer from an valid Address.

Recommended Mitigation Steps

Check if the address requesting makeOffer for the NFT id is not also the NFT ID owner, and revert in that case.