Escher contest - 0xA5DF's results

Lines of code

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

Vulnerability details

LPDA price decreases over time till endTime timestamp. If the LPDA isn't configured properly, the price might underflow when reaching the endTime timestamp (to be specific, it'll underflow if (endTime-startTime)*dropPerSecond > startPrice).

Impact

This will disable all functionality of the LPDA instance and freeze all funds accumulated from all purchases done till the point in time the price underflowed.

Proof of Concept

The following test demonstrates such a case of poor configuration, the test will revert with an underflow error.

File: test/LPDA.poc.t.sol

pragma solidity ^0.8.17;

import "forge-std/Test.sol";
import {EscherTest} from "./utils/EscherTest.sol";
import {LPDAFactory, LPDA} from "src/minters/LPDAFactory.sol";

contract LPDABase is EscherTest {
    LPDAFactory public lpdaSales;
    LPDA.Sale public lpdaSale;

    LPDA public sale;
    event End(LPDA.Sale _saleInfo);

    function setUp() public virtual override {
        super.setUp();
        lpdaSales = new LPDAFactory();
        // set up a LPDA Sale
        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 ether) / 1 days),
            startTime: uint96(block.timestamp),
            saleReceiver: payable(address(69)),
            endTime: uint96(block.timestamp + 2 days)
        });
    }

    function test_Buy_bug() public {
        sale = LPDA(lpdaSales.createLPDASale(lpdaSale));
        // authorize the lpda sale to mint tokens
        edition.grantRole(edition.MINTER_ROLE(), address(sale));
        //lets buy an NFT
        vm.warp(block.timestamp + 3 days);
        sale.buy{value: 1 ether}(1);
        assertEq(address(sale).balance, 1 ether);
    }

}

Recommended Mitigation Steps

Check that the price isn't going to overflow during the creation of the LPDA instance.

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPrice.sol#L107-L111 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L146-L148

Vulnerability details

Impact

This one affects LPDA and Fixed Price contracts, since those end only when the creator sells all of the tokens that he put up for sale. In case that the creator didn't manage to sell all the tokens, they won't able to withdraw the profit for the tokens they've sold.

Proof of Concept

The lack of ability to withdraw the funds before the contest ends is pretty clear from the code (and the lack of a feature can't be demonstrated via a coded PoC).

Consider the following scenario:

• Creator put up for sale 100 NFTs via Fixed Price, for the price of 0.5 ETH
• 4 people buy tokens
• The creator's profit is 1.9 ETH and the protocol fee is 0.1 ETH
• The popularity of this seller has decreased and nobody is willing to buy their art for more than 0.3 ETH
• The funds are now locked in the contract, and the only way for the seller to break them out is to buy his own art
  • That will mean investing 48 ETH, in order to get back only 47.5 ETH (2.5 ETH goes to protocol fees). The seller would forego this option and both the seller's profit and the protocol fees would be locked forever.

Recommended Mitigation Steps

Allow creator to withdraw the sale revenue even before the sale ends. In the case of LPDA - allow only to withdraw lowestPrice * amount, since some funds should be reserved for refunds when/if the price goes down further.

Lines of code

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

Vulnerability details

The sales contracts are supposed to mint a specific range of token IDs, however there's no guarantee that those tokens weren't minted previously or wouldn't be minted by another contract/EOA during the sale. In such case the buy() function will revert when it reaches that already-minted token ID.

Impact

In the case of Open Edition it'll only disable further buying, but in the case of LPDA and Fixed Price it'll also lock all the funds from previous purchases, since the funds can't be withdrawn till all token IDs that are up for sale are purchased.

Proof of Concept

The following test demonstrates a case where 2 sales are created, one has done already one sale

File: test/LPAD.poc.t.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

import "forge-std/Test.sol";
import {EscherTest} from "./utils/EscherTest.sol";
import {LPDAFactory, LPDA} from "src/minters/LPDAFactory.sol";

contract LPDABase is EscherTest {
    LPDAFactory public lpdaSales;
    LPDA.Sale public lpdaSale;

    LPDA public sale;
    LPDA public sale2;


    event End(LPDA.Sale _saleInfo);

    function setUp() public virtual override {
        super.setUp();
        lpdaSales = new LPDAFactory();
        // set up a LPDA Sale
        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(0.1 ether) / 1 days),
            startTime: uint96(block.timestamp),
            saleReceiver: payable(address(69)),
            endTime: uint96(block.timestamp + 1 days)
        });
    }

    function test_Buy_bug() public {
        sale = LPDA(lpdaSales.createLPDASale(lpdaSale));
        lpdaSale.currentId += 1;
        sale2 = LPDA(lpdaSales.createLPDASale(lpdaSale));
        // authorize the lpda sale to mint tokens
        edition.grantRole(edition.MINTER_ROLE(), address(sale));
        edition.grantRole(edition.MINTER_ROLE(), address(sale2));
        //lets buy an NFT
        sale.buy{value: 1 ether}(1);
        sale2.buy{value: 1 ether}(1);
        sale.buy{value: 1 ether}(1);
    }

}

Recommended Mitigation Steps

• Make sure there's only one minter role at a time
• Make sure tokens are minted sequentially, and that currentId parameter is equal to last token minted

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/11ca988e39effe6d316e996c27a876c92e82b4da/src/Escher721.sol#L13 https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/65420cb9c943c32eb7e8c9da60183a413d90067a/contracts/access/AccessControlUpgradeable.sol#L165-L167

Vulnerability details

Impact

Reading the cancel() function it seems that the protocol intends to allow canceling the sale only before the sale starts. However, a creator can effectively cancel a sale after it started by revoking the minting role from the sale contract. This is mostly relevant for the Open Edition sale - since it ends at a certain point in time regardless of the amount sold (meaning, a creator can stop the sale by revoking the minting role and will still be able to collect the funds from purchases done before revoking).

Proof of Concept

The following test demonstrates such a scenario, where the creator revokes the role after the sale has started. The creator is still able to withdraw the profit from the first purchase.

File: test/Revoke.poc.t.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

import {EscherTest} from "./utils/EscherTest.sol";
import {OpenEditionFactory, OpenEdition} from "src/minters/OpenEditionFactory.sol";

contract OpenEditionBase is EscherTest {
    OpenEditionFactory public openSales;
    OpenEdition.Sale public openSale;

    function setUp() public virtual override {
        super.setUp();
        openSales = new OpenEditionFactory();
        openSale = OpenEdition.Sale({
            price: uint72(uint256(1 ether)),
            currentId: uint24(1),
            edition: address(edition),
            startTime: uint96(block.timestamp),
            saleReceiver: payable(address(69)),
            endTime: uint96(block.timestamp + 1 days)
        });
    }
}


contract OpenEditionTest is OpenEditionBase {
    OpenEdition public sale;
    event End(OpenEdition.Sale _saleInfo);

    function test_Revoke_bug() public {
        sale = OpenEdition(openSales.createOpenEdition(openSale));
        // authorize the fixed price sale to mint tokens
        edition.grantRole(edition.MINTER_ROLE(), address(sale));

        // lets buy some NFTs
        sale.buy{value: 1 ether}(1);

        // creator regrets and revokes the minter role
        edition.revokeRole(edition.MINTER_ROLE(), address(sale));

        // Attempting to buy any additional tokens will revert
        vm.expectRevert();
        sale.buy{value: 1 ether}(1);

        // creator is still able to finalize the sale and get the payment from 1st purchase
        vm.warp(openSale.endTime + 1 hours);
        sale.finalize();

    }

}

Recommended Mitigation Steps

• Verify that the sale contract has the minting role when it's created
• Don't allow revoking the minting role from a sale before it ended
  • This can be done by overriding the revokeRole() and adding a check before revoking

Attacker can buy zero amount in order to spam the contract with false events

They buy() function doesn't require the amount to purchase to be greater than zero, meaning a user can run the function with zero amount, which would spam the contract with Buy events without actually buying anything.

It's not possible to mint token #0 with the sales contracts

Code src/minters/LPDA.sol#L73

It's not possible to mint the #0 token, since the sales contract will start minting the temp.currentId + 1 token, and since currentId is an unsigned int it can't be lower then 0. Meaning the minimum token ID that can be minted is 1.

Mitigation

Switch the currentID to be the next token ID to be minted, rather than the last token minted, and update the code accordingly.

Unsafe cast of msg.value can cause loss of ETH in a rare case

Code: src/minters/LPDA.sol#L71

The LPDA.buy() function unsafely casts the msg.value to uint80, in case that msg.value is greater than 2^80 it'll reduce the amount to msg.value % 2^80, and the difference will be locked in the contract. This is not very practical since it requires to send ~1.2M ETH, but it's still better to do a safe cast (i.e. check that msg.value <= type(uint80).max) just in case.

Unsafe cast of _amount at buy()

The buy() function gets a uint256 parameter named _amount and unsafely casts it to uint24/uint48.

As for the OpenEdition and LPDA contracts, this would have only a 'cosmetic' effect (user who'd look at the tx would see that the _amount parameter sent is much greater than the actual amount purchased).

As for the Fixed Price contract, this would also cause a loss of value, since the _amount is casted only for calculating the newID but not for the msg.value check. This can lead to a rare case where a user sets _amount to x + 2^48 and send ETH accordingly, but get only x tokens in return.

Open Edition price is limited to 4.7K ETH

The Sale.price variable of the OpenEdition contract is a uint72, meaning it's max value is ~4.7K ETH. For most sales that would be enough, but in the history of NFT there have been recorded higher prices. The size of the variable can be increased without taking up any additional storage slots, by moving the price variable to the 3rd slot of the Sale struct:

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

Also the LDPA price variable can be increased to uint84 instead of uint80 without taking up any additional slots.

Generative.tokenToSeed() is internal instead of public

Generative.tokenToSeed() function is defined as internal, but has no internal usage. I understood from the devs that it's supposed to serve external contract in generating metadata from a seed. Therefore it needs to be changed to public/external in order to be accessible.

Any user can create a sale with a cloned Escher721 contract

The sale contract factories only check that the creator has the admin role over the _sale.edition contract. But any user can simply clone the code of Escher721, deploy his own version without having the Creator role and then start a sale with it. This might confuse users to thing that the created sale contract is from a legitimate seller approved by the protocol.

Mitigation

The cheapest/easiest solution would be to also check that msg.sender has the Creator role. A more thorough solution would be to handle a registry of deployed contract at Escher721Factory, and at the sale factories check that the edition contract is registered there.