Escher contest - Chom's results

Lines of code

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

Vulnerability details

Impact

LPDA price is not calculated correctly such that it won't end at the final price as expected

Proof of Concept

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

Assume startPrice = 5, dropPerSecond = 1, finalPrice = 3 and timeElapsed = 10 (Possible as there isn't any check on end - start)

temp.startPrice - (temp.dropPerSecond * timeElapsed) = 5 - (1 * 10) = 5 - 10

It should end at finalPrice = 3 but in fact it end at negative price (reverted)

Recommended Mitigation Steps

    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.dropPerSecond * timeElapsed) >= temp.startPrice - temp.finalPrice ? temp.finalPrice : temp.startPrice - (temp.dropPerSecond * timeElapsed);
    }

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L110 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L50-L53 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEdition.sol#L75-L78 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEdition.sol#L120-L123

Vulnerability details

Impact

selfdestruct will be banned after "The Purge" phase of ethereum merge (EIP-4758). FixedPrice and OpenEdition sale can't be ended after EIP-3758.

Proof of Concept

https://eips.ethereum.org/EIPS/eip-4758

After EIP-4758 is merged, _end, which uses selfdestruct may not be working as intended. It won't selfdestruct the contract, so if the creator cancel the sale using cancel function, it won't cancel the sale.

    function cancel() external onlyOwner {
        require(block.timestamp < sale.startTime, "TOO LATE");
        _end(sale);
    }

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

It will just transfer the fund but never end the sale as intended as selfdestruct is replaced with SENDALL

Similar pattern apply for OpenEdition too.

Recommended Mitigation Steps

Use ordinary flag variable instead

    bool public isSaleEnded;

    function cancel() external onlyOwner {
        require(block.timestamp < sale.startTime, "TOO LATE");
        _end(sale);
    }

    function buy(uint256 _amount) external payable {
        require(!isSaleEnded, "Sale ended");
        ...
    }

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

Similar pattern can be applied to OpenEdition.

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L50-L53 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L14-L24 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L92-L96 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L58-L89

Vulnerability details

Impact

The sale can't be canceled after the sale start. Moreover, the sale won't end, which contradict the fact that typical NFT sales nowadays have an end time. Even your OpenEdition have an end time.

Proof of Concept

    struct Sale {
        // slot 1
        uint48 currentId;
        uint48 finalId;
        address edition;
        // slot 2
        uint96 price;
        address payable saleReceiver;
        // slot 3
        uint96 startTime;
    }

FixedPrice sale doesn't have an end time.

LPDA has an end time but it just trigger the end of LPDA price slope but not actual sale end time

    struct Sale {
        // slot 1
        uint72 price;
        uint24 currentId;
        address edition;
        // slot 2
        uint96 startTime;
        address payable saleReceiver;
        // slot 3
        uint96 endTime;
    }

In contrast, OpenEdition have an end time.

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

Moreover, cancel can't be used after the sale has started.

As a result, once the sale has started, it never ends, which is not a best practice as typical NFT sales nowadays have an end time.

Recommended Mitigation Steps

Implement an end time logic to the FixedPrice sale

    struct Sale {
        // slot 1
        uint48 currentId;
        uint48 finalId;
        address edition;
        // slot 2
        uint96 price;
        address payable saleReceiver;
        // slot 3
        uint96 startTime;
        uint96 endTime;
    }

    ...

    /// @notice buy from a fixed price sale after the sale starts
    /// @param _amount the amount of editions to buy
    function buy(uint256 _amount) external payable {
        Sale memory sale_ = sale;
        IEscher721 nft = IEscher721(sale_.edition);
        require(block.timestamp >= sale_.startTime, "TOO SOON");
        require(block.timestamp < sale_.endTime, "TOO LATE");
        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);
    }

Similar pattern apply to LPDA sale too.