Escher contest - Ch_301's results

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDAFactory.sol#L29-L42

Vulnerability details

Impact

• The LPDA will fail to buy() (mint) new NFTs even if block.timestamp >= temp.startTime , block.timestamp >= temp.endTime and newId <= temp.finalId
• Both the feeReceiver and saleReceiver will never receive their funds if the price dropped more than the start price and still no one buy() the finalId

Proof of Concept

Please copy the following test on LPDA.t.sol

• First, update the endTime on LPDABase contract

            //endTime: uint96(block.timestamp + 1 days)
            endTime: uint96(block.timestamp + 12 days)

Then use this POC

    function test_Arithmetic_underflow() public {
        test_Buy();

        // the test_Buy() will mint only one NFT
        uint blanceNFT = IEscher721(address(edition)).balanceOf(address(this));
        assertEqUint(blanceNFT, 1);
        
        // time travel
        vm.warp(lpdaSale.endTime + 11 days);
        uint price = sale.getPrice();

    }

This test will [FAIL. Reason: Arithmetic over/underflow] even if the endTime still not

Recommended Mitigation Steps

Add this require on LPDAFactory.createLPDASale()

 require(sale.startPrice > (sale.dropPerSecond * (sale.endTime - sale.startTime)), "INVALID DROP PER SECOND");

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDAFactory.sol#L29-L42 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPriceFactory.sol#L29-L38 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEditionFactory.sol#L29-L38

Vulnerability details

Impact

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

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 with ID == 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

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L58-L89

Vulnerability details

Impact

The users would keep minting from LPDA even if the end time was reached

Proof of Concept

Please copy the following test on LPDA.t.sol

import {IEscher721} from "../src/interfaces/IEscher721.sol";

    function test_Reverts_When_Ended_Buy() public {
        test_Buy();

        // the test_Buy() will mint only one NFT
        uint blanceNFT = IEscher721(address(edition)).balanceOf(address(this));
        assertEqUint(blanceNFT, 1);

       // let’s reach the endTime
        vm.warp(lpdaSale.endTime + 1 days);

        sale.buy{value: 9 ether}(9);

        // check if the mint was successful 
        blanceNFT = IEscher721(address(edition)).balanceOf(address(this));
        assertEqUint(blanceNFT, 10);
    }

Recommended Mitigation Steps

Add

require(block.timestamp >= temp.endTime, "TOO LATE");