Incorrect Withdrawal Implementation May Lead to Lock of Unclaimed NFTs

The SimpleERC721MerkleDistributor contract allows claiming of ERC-721 tokens instead of ERC-20. It inherits most of its functions from TokenTableMerkleDistributor. The main difference is that _send() and withdraw() functions handle ERC-721 token minting and transfers instead of ERC-20 transfers.

The claiming process in this case relies on minting the NFTs directly from the token contract. In the ERC-20 versions the project owner is equipped with withdraw(), which allows to recover all non-claimed tokens.

In this SimpleERC721MerkleDistributor contract, however the overloaded withdraw() attempts to transfer existing NFTs from the contract. This will not work because the unclaimed NFTs are not actually minted to that contract.

function withdraw(bytes memory extraData) external virtual override onlyOwner {
    uint256[] memory tokenIds = abi.decode(extraData, (uint256[]));
    for (uint256 i = 0; i < tokenIds.length; i++) {
        IERC721(_getBaseMerkleDistributorStorage().token).safeTransferFrom(address(this), owner(), tokenIds[i]);
    }
}

function _send(address recipient, address token, uint256 amount) internal virtual override {
    for (uint256 i = 0; i < amount; i++) {
        IERC721SafeMintable(token).safeMint(recipient);
    }
}

In the case where the minting permissions are granted to the distributor contract, but not to the project owner, the owner cannot mint the unclaimed tokens for himself and they might end up locked (or rather never minted).

Change the withdraw() function in SimpleERC721MerkleDistributor so that it mints the NFTs instead of transferring them. Warning! If that change is implemented the withdraw() function must also be placed into the SimpleNoMintERC721MerkleDistributor contract as it inherits from SimpleERC721MerkleDistributor. If that is not done, then withdrawals will not work for SimpleNoMintERC721MerkleDistributor.

TokenTable: Revised withdraw logic in 0e4cd1d1c27dfbb98080728da8955a10d1143a9c.