Escher contest - cccz's results

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/Escher721.sol#L64-L69

Vulnerability details

Impact

The documentation says that authors can specify metadata and royalties for NFTs with specific IDs via setTokenURI()/setTokenRoyalty()

1. Set up the token URI by calling setTokenURI with the token ID and the URI (arweave recommended)
2. If the artist would like sales and royalties to go somewhere other than the default royalty receiver, they must call setTokenRoyalty with the following variables:
...

Authors can only call setDefaultRoyalty to set the default royalty, and setBaseURI to set the baseURI. This is inconsistent with the documentation, thus limiting authors to setting different metadata and royalties for different NFTs

Proof of Concept

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/Escher721.sol#L64-L69 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/uris/Base.sol#L10-L12

Tools Used

None

Recommended Mitigation Steps

Consider implementing setTokenURI()/setTokenRoyalty() to allow authors to specify metadata and royalties for NFTs with different IDs

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L109-L110

Vulnerability details

Impact

FixedPrice._end/LPDA.buy/LPDA.refund/OpenEdition.finalize use payable.transfer to send eth to the user. This is unsafe as transfer has hard coded gas budget and can fail when the user is a smart contract. Whenever the user either fails to implement the payable fallback function or cumulative gas cost of the function sequence invoked on a native token transfer exceeds 2300 gas consumption limit the native tokens sent end up undelivered and the corresponding user funds return functionality will fail each time.

Proof of Concept

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L109-L110 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L85-L86 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L105-L106 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEdition.sol#L92-L93

Tools Used

None

Recommended Mitigation Steps

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L62-L67

Vulnerability details

Impact

When the author creates a sale of Escher721, the id of the NFT bought by the user is incremented.

        uint48 newId = uint48(_amount) + sale_.currentId;
        require(newId <= sale_.finalId, "TOO MANY");

        for (uint48 x = sale_.currentId + 1; x <= newId; x++) {
            nft.mint(msg.sender, x);
        }

And in Escher721 contract, there may be more than one minter, if other minter mint the NFT with id in the sale range, the sale will fail when minting the NFT with that id in the buy function, and because Escher721 does not support burn NFT, it cannot be recovered, thus the ETH in the Fixed Price and LPDA sales is locked in the contract.

        uint48 newId = uint48(_amount) + sale_.currentId;
        require(newId <= sale_.finalId, "TOO MANY");

        for (uint48 x = sale_.currentId + 1; x <= newId; x++) {
            nft.mint(msg.sender, x);
        }

Due to the high impact and low likelihood, I consider the severity to be medium

Proof of Concept

Consider the following scenarios.

An author creates a FixedPrice sale for NFTs with IDs 1 to 100 in Escher721 at 1 ETH A minter in Escher721 minted an NFT with ID 100. When the user buys the NFT in the FixedPrice sale, the sale cannot be ended because the NFT with ID 100 has already been minted, resulting in the ETH sent by the user being locked in the contract.

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/Escher721.sol#L51-L53 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L62-L67 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L73-L88

Tools Used

None

Recommended Mitigation Steps

Consider allowing only one minter to exist for Escher721, transferring the minter to the sale contract when the author creates the sale, and then to the author when the sale ends

[Low-01] PRNG in Generative.setSeedBase

Impact

Generative.setSeedBase uses block.timestamp/block.number/blockhash to generate random numbers, which results in the generated random numbers being predictable, and since the generated random numbers are generally used to determine the properties of NFTs, users can predict the random numbers and thus purchase rarer NFTs.

Proof of Concept

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/uris/Generative.sol#L19-L25

Recommended Mitigation Steps

Consider using chainlink VRF for random number generation https://docs.chain.link/vrf/v2/introduction/

[Low-02] _safeMint() should be used rather than _mint() wherever possible

Impact

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver.

Proof of Concept

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L66-L67 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L74-L75 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEdition.sol#L67-L68

Recommended Mitigation Steps

Use _safeMint instead of _mint

[Low-03] LPDAFactory.createLPDASale should check startPrice >= dropPerSecond * (end-start)

Impact

LPDAFactory.createLPDASale should check startPrice >= dropPerSecond * (end-start), if startPrice < dropPerSecond * (end-start), then LPDA.getPrice() may revert due to overflow, and buy()/refund() will fail, making ETH locked in the contract

   function getPrice() public view returns (uint256) {
        Sale memory temp = sale;
        (uint256 start, uint256 end) = (temp.startTime, temp.endTime);
        if (block.timestamp < start) return type(uint256).max;
        if (temp.currentId == temp.finalId) return temp.finalPrice;

        uint256 timeElapsed = end > block.timestamp ? block.timestamp - start : end - start;
        return temp.startPrice - (temp.dropPerSecond * timeElapsed);
    }

Proof of Concept

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L117-L125

Recommended Mitigation Steps

Consider checking startPrice >= dropPerSecond * (end-start) in LPDAFactory.createLPDASale

[Low-04] Unable to create sale for NFT with id 0

Impact

When creating a sale, the currentId is at least 0, but in the buy function, only the NFT with ID currentId + 1 will be minted, which makes it impossible to create a sale for an NFT with ID 0

    function buy(uint256 _amount) external payable {
        Sale memory sale_ = sale;
        IEscher721 nft = IEscher721(sale_.edition);
        require(block.timestamp >= sale_.startTime, "TOO SOON");
        require(_amount * sale_.price == msg.value, "WRONG PRICE");
        uint48 newId = uint48(_amount) + sale_.currentId;
        require(newId <= sale_.finalId, "TOO MANY");

        for (uint48 x = sale_.currentId + 1; x <= newId; x++) {
            nft.mint(msg.sender, x);
        }

Proof of Concept

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L57-L67

Recommended Mitigation Steps

Consider allowing NFTs with ID 0 to be minted in the buy function