Lines of code
<https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPriceFactory.sol#L29-L38&gt;
<https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEditionFactory.sol#L29-L38&gt;

Vulnerability details

Impact

• Possibility of IDs collision
• The ether will be locked on FixedPrice orLPDA

Proof of Concept

On the same Escher721
Let’s say the first collection start from id = 0 and end on id = 10
So the next collection of the same Escher721 (We can say the next part of the same collection) should start withID == 10 or grater

Case 1:
The creator can creat the second part of his collection by invoking createOpenEdition , createLPDASale or createFixedSale. And passing _sale.currentId == 8 so no one can use this clone to mint the NFT because the ERC721Upgradeable will revert

require(!_exists(tokenId), "ERC721: token already minted");

This is not a big problem

Case 2:
The creator can create the second part of his collection by invoking createLPDASale or createFixedSale. passing _sale.currentId == 11 and sale.finalId == 20
And create another part (this is the third one) and passing _sale.currentId == 18 and sale.finalId == 30
Please copy the following test on FixedPrice.t.sol to keep tracking the Case 2

    FixedPrice public sale_second_part;
    FixedPrice public sale_third_part;

     function test_create_two_part_in_same_time() public {
        
        sale = FixedPrice(fixedSales.createFixedSale(fixedSale));
        // authorize the fixed price sale to mint tokens
        edition.grantRole(edition.MINTER_ROLE(), address(sale));
        //mint all the possible NFTs
        sale.buy{value: 10 ether}(10);

        // check if the mint all was successful 
        uint blanceNFT = IEscher721(address(edition)).balanceOf(address(this));
        console.log("You have 1: ", blanceNFT);
        assertEqUint(blanceNFT, 10);

        //Now the creator will launch  the second part...
            fixedSale = FixedPrice.Sale({
            currentId: uint48(11),
            finalId: uint48(20),
            edition: address(edition),
            price: uint96(uint256(1 ether)),
            saleReceiver: payable(address(69)),
            startTime: uint96(block.timestamp)
        });
        sale_second_part = FixedPrice(fixedSales.createFixedSale(fixedSale));
        edition.grantRole(edition.MINTER_ROLE(), address(sale_second_part));

        //Now mint only 7 NFTs
        sale_second_part.buy{value: 7 ether}(7);

        // check if the mint was successful 
        blanceNFT = IEscher721(address(edition)).balanceOf(address(this));
        console.log("You have 2: ", blanceNFT);
        assertEqUint(blanceNFT, 17);

        //Now the creator will launch the third part...     
         fixedSale = FixedPrice.Sale({
            currentId: uint48(18),
            finalId: uint48(30),
            edition: address(edition),
            price: uint96(uint256(1 ether)),
            saleReceiver: payable(address(69)),
            startTime: uint96(block.timestamp)
        });
        sale_third_part = FixedPrice(fixedSales.createFixedSale(fixedSale));
        edition.grantRole(edition.MINTER_ROLE(), address(sale_third_part));

        //Now mint only 1 NFTs
        sale_third_part.buy{value: 1 ether}(1);

        // check if the mint was successful 
        blanceNFT = IEscher721(address(edition)).balanceOf(address(this));
        console.log("You have 3: ", blanceNFT);
        assertEqUint(blanceNFT, 18);

        //new let's try mint another one on the second part sale_second_part 
        vm.expectRevert("ERC721: token already minted");
        sale_second_part.buy{value: 1 ether}(1);
        
        // check if the mint  was unsuccessful 
        blanceNFT = IEscher721(address(edition)).balanceOf(address(this));
        console.log("You have 4: ", blanceNFT);

        // now no one can mint the rest of NFTs on the second part ==> all the ether will be locked there 
    }

As we can see, no one can mint the rest of NFTs on the second part. So this line

if (newId == sale_.finalId) _end(sale);

this line Will never be executed means all the ether will be locked there forever (or until these users burn their NFTs and Allow to complete the mint of the rest IDs until finalId)

Recommended Mitigation Steps

Add some tracking for the IDs and the opened mint to abode the IDs collision

---

The text was updated successfully, but these errors were encountered:

All reactions