Escher contest - yixxas's results

Lines of code

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

Vulnerability details

Impact

The way getPrice() calculation works in LPDA is with return temp.startPrice - (temp.dropPerSecond * timeElapsed). This is sound as price is decreasing at a constant rate based on temp.dropPerSecond. However, the lack of input sanitation is crucially dangerous here. If at some point in time after sale has started, temp.dropPerSecond * timeElapsed > temp.startPrice, getPrice() will always revert due to the underflow error here.

The sale will never end because no one is able to call buy() -> saleReceiver and feeReceiver() will never be able to claim their proceeds. refund() can never be called by users. Whatever funds that are transferred to the contract will be stuck forever. We will also not be able to cancel() as sale has started.

Proof of Concept

We first see in createLPDASale() that there is no restrictions placed on dropPerSecond relative to startPrice, startTime and endTime.

LPDAFactory.sol#L29-L42

    function createLPDASale(LPDA.Sale calldata sale) external returns (address clone) {
        require(IEscher721(sale.edition).hasRole(bytes32(0x00), msg.sender), "NOT AUTHORIZED");
        require(sale.saleReceiver != address(0), "INVALID SALE RECEIVER");
        require(sale.startTime >= block.timestamp, "INVALID START TIME");
        require(sale.endTime > sale.startTime, "INVALID END TIME");
        require(sale.finalId > sale.currentId, "INVALID FINAL ID");
        require(sale.startPrice > 0, "INVALID START PRICE");
        require(sale.dropPerSecond > 0, "INVALID DROP PER SECOND");

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

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

getPrice() will always reach the reverting line uint256 timeElapsed = end > block.timestamp ? block.timestamp - start : end - start if sale has started and sale has not completely been sold out.

LPDA.sol#L117-L125

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

Say we our sale is set to last 24 hours ( 86400 seconds ). We set startPrice = 1 ether, dropPerSecond = 1e14. After 10000 seconds, we have dropPerSecond * timeElapsed = 1e14 * 10000 = 1e18.

Any call to getPrice() after the 10000th seconds will revert. Assuming that there are still NFTs remaining to be sold, anyone that bought anything in the first 10000th seconds will not be able to claim back their rightful funds as they should in a LPDA auction as they should, as refund() relies on getPrice() which always reverts.

LPDA.sol#L58-89

    function buy(uint256 _amount) external payable {
        uint48 amount = uint48(_amount);
        Sale memory temp = sale;
        IEscher721 nft = IEscher721(temp.edition);
        require(block.timestamp >= temp.startTime, "TOO SOON");
        uint256 price = getPrice();
        require(msg.value >= amount * price, "WRONG PRICE");

        amountSold += amount;
        uint48 newId = amount + temp.currentId;
        require(newId <= temp.finalId, "TOO MANY");

        receipts[msg.sender].amount += amount;
        receipts[msg.sender].balance += uint80(msg.value);

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

        sale.currentId = newId;

        emit Buy(msg.sender, amount, msg.value, temp);

        if (newId == temp.finalId) {
            sale.finalPrice = uint80(price);
            uint256 totalSale = price * amountSold;
            uint256 fee = totalSale / 20;
            ISaleFactory(factory).feeReceiver().transfer(fee);
            temp.saleReceiver.transfer(totalSale - fee);
            _end();
        }
    }

No one is also able to buy anymore NFTs after this period because buy() checks the price with getPrice(). Sellers of the NFTs will not be able to retrieve their earnings from sales because the sale never ends. Likewise, feeReceiver will not be able to collect any fees from the sales that are done. cancel() does not save us as it cannot be called after sale has started.

Tools Used

Manual review

Recommended Mitigation Steps

Do not allow a sale to be created where (endTime - startTime) * dropPerSecond > startPrice. Add the require check require( (endTime-startTime) * dropPerSecond <= startPrice ) in createLPDASale.

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L107-L111 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L57-L74 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L50-L53

Vulnerability details

Impact

If a FixedPrice sale is not popular, and its NFTs are not completely sold out, there is no way for seller or fee receiver to withdraw their funds. One can argue that the saleReceiver() can purposefully buy out all the NFTs to trigger _end(), but this requires paying a 5% fee on the remaining NFTs to the feeReceiver() ( assuming they are different addresses ), which is a sizable amount.

Proof of Concept

The only way for feeReceiver() or saleReceiver() to receive funds is in the _end() function, which is triggered in buy() after all NFTs are sold out. cancel() will not work once a sale has started. We note that this is a very likely scenario to happen since NFTs are not going to be sold out all the time in a fixed price sale.

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend that we add a _sale.endTime, to add a limit in which the fixed price sale should end. Once block.timestamp passes this limit, we should be able to trigger _end() for the seller or fee receiver to get their funds.

Lines of code

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

Vulnerability details

Impact

When a user buy() editions, they can choose the number of editions they want to purchase. However, due to an explicit downcast on a uint256 value, they can make a payment for a large number of editions, but receive a much lesser amount of editions. feeReceiver() and saleReceiver() will receive a disproportionately large amount of funds. Huge loss of asset for user.

Proof of Concept

we see in the code snippet below, buy() takes in uin256 _amount. This value is multiplied with sale_.price to verify that it is equal to the amount of ether user has sent in. However, in the next line we have, uint48 newId = uint48(_amount) + sale_.currentId. _amount is downcasted to uint48.

FixedPrice.sol#L57-L74

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

Let me illustrate with a scenario that causes massive loss to user.

A fixed sale is started with x = 100000000 number of edition ids. ( Exact amount of supply does not matter ) sale_.price is set at 1000 wei. sale_.currentId = 0.

Buyer is not aware of this, decides to buy type(uint48).max + 1 = 281,474,976,710,656 with the correct amount of msg.value = 281,474,976,710,656 * 1000 = 281,474,976,710,656,000 and this is about 0.28 ether, a realistic amount. Now, because _amount is downcasted, we have uint48 newId = uint48(_amount) + sale_.currentId = 1 + 0 = 1.

End result is that only one NFT would be minted to buyer despite paying for type(uint48).max + 1 of them. Buyer loses his 0.28 ether.

Tools Used

Manual review.

Recommended Mitigation Steps

We should downcast the _amount at the start of the function and use this same value throughout just like how it is done in the other sale methods.

    function buy(uint256 _amount) external payable {
+       _amount = uint48(_amount);
        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;
+        uint48 newId = _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);
    }

Judge has assessed an item in Issue #173 as M risk. The relevant finding follows:

L-03 - Depreciating-soon selfdestruct is used to transfer funds to seller after sale ends.

L-01 - Event will emit the wrong value when a explicit downcast happens

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

As explained in one of my reported issue, input of uint256 _amount being explicitly downcasted to a uint48 here will cause issues. One of the side effects is that event emitted will be a uint256 value, but the amount that is minted will be much smaller than that. This can create confusion for indexers. Fixing that issue would fix this too by setting _amount = uint48(_amount) right at the start.

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

L-02 - setDefaultRoyalty can be changed anytime with no limits to it.

This makes it difficult for royalties receivers to trust the artist, since royalties amount can be change anytime to 0. Artist can get all the profits to himself. I recommend to allow royalties to be set only once.

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

    function setDefaultRoyalty(
        address receiver,
        uint96 feeNumerator
    ) public onlyRole(DEFAULT_ADMIN_ROLE) {
        _setDefaultRoyalty(receiver, feeNumerator);
    }

L-03 - Depreciating-soon selfdestruct is used to transfer funds to seller after sale ends.

The selfdestruct function is being used in OpenEdition.sol and FixedPrice.sol to transfer funds to users. This is not recommended as future forks of Ethereum is likely going to change that. See EIP-4758. Once removed, it will break the usage of it.

I recommend using a normal transfer instead of selfdestruct to transfer funds to users.

L-04 - Creating a sale in FixedPrice factory and OpenEdition factory have very little input checks

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEditionFactory.sol#L29 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPriceFactory.sol#L29

It is lacking 2 important 0 checks in both of them. saleReceiver() address can be set to 0. This will cause the proceeds of sales to go to the 0 address, which are essentially lost. The variable startPrice can also be set to 0. Since default value in solidity is 0, sale creators may accidentally create a 0 priced sale, which is unlikely to be something that is wanted, and have all their NFTs be sold for free.

I recommend adding this 2 input checks.