Escher contest - nameruse's results

Lines of code

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

Vulnerability details

Impact

While i'm assuming there are checks for this on UI or off-chain, this wasn't mentioned in the docs or the comments, so its worth flagging and possibly implementating the mitigation step as a precaution since it is also cheap on gas.

The price can theoritically drop below the sale.startprice which can lead to getPrice() function call to always revert which will in turn always revert a buy function call under these conditions.

More specifically on line 125 of the code above:

priceDropPerSec * timeElapased can be larger than start price.

There is no steps taken in code to check or mitigate it and hence this can be potential source of DoS.

Proof of Concept

The below test using forge fails due to revert

       function test_LPDABug() public {

        lpdaSale = LPDA.Sale({
            currentId: uint48(0),
            finalId: uint48(10),
            edition: address(edition),
            startPrice: uint80(uint256(1 ether)),
            finalPrice: uint80(uint256(0.1 ether)),
            dropPerSecond: uint80(uint256(1.1 ether) / 1 days),
            startTime: uint96(block.timestamp),
            saleReceiver: payable(address(69)),
            endTime: uint96(block.timestamp + 1 days)
        });
        // make the lpda sales contract
        sale = LPDA(lpdaSales.createLPDASale(lpdaSale));
        // authorize the lpda sale to mint tokens
        edition.grantRole(edition.MINTER_ROLE(), address(sale));

        vm.warp(block.timestamp + 1 days);

        sale.getPrice();
    }

Tools Used

VSCode Forge for test

Recommended Mitigation Steps

Updating the return statement of line 125 of LDPA.sol to something like the below to return final price incase the price drop ends up being bigger than start price

uint256 dropAmount = temp.dropPerSecond * timeElapsed;
return dropAmount > temp.startPrice ?  temp.final : Pricetemp.startPrice - dropAmount;

Lines of code

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

Vulnerability details

Impact

Currently for a FixedPrice sale that started, the only way for it to end is when the entire token id range in the FixedPrice instance gets sold.

The sales receiver and fee receiver only receive their fund once the FixedPrice sale ends.

As such, in a situation where the all token ids in that FixedPrice sale proxy don't get sold then funds will be held on the smart contract without being able to retreive them.

There is a cancellation functionality that can be called by FixedPrice proxy but that can only be called before the scheduled start time for the sale and not after it started.

Proof of Concept

Below is the snippet of the cancellation functionality with a requirment for the block.timstamp being less than the start time Link to code

    /// @notice Owner can cancel current sale
    function cancel() external onlyOwner {
        require(block.timestamp < sale.startTime, "TOO LATE");
        _end(sale);
    }

While the Snippet for the ending the sale and transferring the fund to the sale receiver and fee receiver is as per below. Link to code

    function _end(Sale memory _sale) internal {
        emit End(_sale);
        ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);
        selfdestruct(_sale.saleReceiver);
    }

This seems to the only way to extract funds from the contract as there is no other withrdaw functionality visible in the code

As for ending the FixedPrice sale through once the all token ids in that FixedPrice sale instance are purchase, please ind the code below Link to code

    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);
        }

        sale.currentId = newId;

        emit Buy(msg.sender, _amount, msg.value, sale);

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

The last line in the code (line 73) called the end functions once tokens are fully sold.

This and the cancellation function are the only two references to the end function and thus once the FixedPrice Sale starts it can only end when all tokens are sold and only then can the sales receives and fee receiver withdraw their fund from the contract

Tools Used

VScode

Recommended Mitigation Steps

There are two way around this:

1-Allowing sales cancellation to occur after the sale started by removing the required check

2-Adding a withdaw function with the onlyOwner modifier that can transfer the current balance to the sale receiver and the fee receiver addreses

Summary

Overall the codebase is quite readable, well organized. For the most part the codebase closley matched the documenation but there where a few mismatches that were mentioned below. Project structure is intuitive and simple to follow. Contract structure is well ogranized and makes good use of libraries and external contracts. Variable naming is mostly easily readble and intuitive. Comments were used rigourously and were beneficial and accurate in describing the object they were comenting on. Events are used in a benefical manner to track protocol outputs.

Low Quality issues

[LQ-01] No external/public function for {_setTokenRoyalty}

No external/public function for {_setTokenRoyalty} as mentioned in the sale flow of the docs. Currently royalty can only be set through default and all token ids.

instances:1

File: Escher721.sol Line: N/A Link

[LQ-02] Improved flow to grant the proxy minter role

Better flow to grant the proxy minter role (escher721 minting function) during initialize instead of having to call it separately. As either way it will have to be called in order for the buy function to work

instances:3

File: FixedPrice.sol Line: 78-82 Link

    function initialize(Sale memory _sale) public initializer {
        sale = _sale;
        _transferOwnership(_sale.saleReceiver);
        emit Start(_sale);
    }

File: LPDA.sol Line: 110-114 Link

    function initialize(Sale memory _sale) public initializer {
        sale = _sale;
        _transferOwnership(_sale.saleReceiver);
        emit Start(_sale);
    }

File: OpenEdition.sol Line: 82-86 Link

    function initialize(Sale memory _sale) public initializer {
        sale = _sale;
        _transferOwnership(_sale.saleReceiver);
        emit Start(_sale);
    }

Non-Critical issues

[NC-01]

For better readability, adding comment that curators will be added by admins manually calling AccessControl "grantRole" function.

Comment could be added above the addCreator function.

instances:1

File: Escher.sol Line: 27 Link

[NC-02]

Typo in the comment. comment mentions "set the prices" i believe it should be "set the pieces"

instances:1

File: Generative.sol Line: 11

[NC-03]

For better readability, fees parameter could be named feesAdress as fees implies an amount

instances:3

File: FixedPriceFactory.sol Line: 40-44 Link

    /// @notice set the fee receiver for fixed price editions
    /// @param fees the address to receive fees
    function setFeeReceiver(address payable fees) public onlyOwner {
        feeReceiver = fees;
    }

File: LPDAFactory.sol Line: 44-47 Link

    /// @notice set the fee receiver for fixed price editions
    /// @param fees the address to receive fees
    function setFeeReceiver(address payable fees) public onlyOwner {
        feeReceiver = fees;
    }

File: OpenEditionFactory.sol Line: 40-44 Link

    /// @notice set the fee receiver for fixed price editions
    /// @param fees the address to receive fees
    function setFeeReceiver(address payable fees) public onlyOwner {
        feeReceiver = fees;
    }

[NC-04]

Fixed price sale in the comment should be Open Edition sale

instances:2

File: FixedPriceFactory.sol Line: 27 && 40

[NC-05]

Missing check to on Zero address for _sale.saleReceiver. Mostly for a precaution as a zero address can lead to situation where sale funds are lost.

instances:2

File: OpenEditionFactory.sol Line: 29-38 Link

    function createOpenEdition(OpenEdition.Sale calldata sale) external returns (address clone) {
        require(IEscher721(sale.edition).hasRole(bytes32(0x00), msg.sender), "NOT AUTHORIZED");
        require(sale.startTime >= block.timestamp, "START TIME IN PAST");
        require(sale.endTime > sale.startTime, "END TIME BEFORE START");


        clone = implementation.clone();
        OpenEdition(clone).initialize(sale);


        emit NewOpenEditionContract(msg.sender, sale.edition, clone, sale);
    }

File: FixedPriceFactory.sol Line: 29-38 Link

    function createOpenEdition(OpenEdition.Sale calldata sale) external returns (address clone) {
        require(IEscher721(sale.edition).hasRole(bytes32(0x00), msg.sender), "NOT AUTHORIZED");
        require(sale.startTime >= block.timestamp, "START TIME IN PAST");
        require(sale.endTime > sale.startTime, "END TIME BEFORE START");


        clone = implementation.clone();
        OpenEdition(clone).initialize(sale);


        emit NewOpenEditionContract(msg.sender, sale.edition, clone, sale);
    }