Escher contest - 0xRobocop's results

Lines of code

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

Vulnerability details

Impact

The impact is that the contract will be on Denial Of Service, not allowing anyone to get ether out, neither the creator or people via refund().

Proof of Concept

The functions buy(uint256 _amount) and refund() of the LPDA.sol contract both call the function getPrice():

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

We can see that if the block.timestamp is greater than the start time and we are not reached the final id, the returned value is calculated by startPrice minus the difference of time multiplied by dropPerSecond. The largest difference of time that we can have is end-start. The Denial of Service can happen because solidity 0.8.0 or greater have overflows and underflows checks by the default. There is no restriction on having certain values on the parameters of startTime, endTime and dropPersecond that ensure this is avoided. For example i provide the next test:

First modify the sale with the following:

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.21 ether) / 1 days),
            startTime: uint96(block.timestamp),
            saleReceiver: payable(address(69)),
            endTime: uint96(block.timestamp + 5 days)
        });
    }

Then i wrote this test:

function test_LPDAD_DoS_Due_To_Incorrect_Parameters() public {
        // make the lpda sales contract
        sale = LPDA(lpdaSales.createLPDASale(lpdaSale));
        // authorize the lpda sale to mint tokens
        edition.grantRole(edition.MINTER_ROLE(), address(sale));
        //lets buy an NFT
        sale.buy{value: 1 ether}(1);
        assertEq(address(sale).balance, 1 ether);

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

        // try to buy
        vm.expectRevert();
        sale.buy{value: uint256((0.9 ether + lpdaSale.dropPerSecond) * 9)}(9);

        // Even refunds will fail
        vm.warp(block.timestamp + 2 days);
        vm.expectRevert();
        sale.refund();
    }

It shows that the sale cannot continue (so it cannot end) nor people can get refunds, due to an underflow on return temp.startPrice - (temp.dropPerSecond * timeElapsed);

Tools Used

Manual and Tests

Recommended Mitigation Steps

There is a view function already on the contract function lowest_price() that already takes into account the largest time difference that is end-start so given a startTime and endTime it can tell you which is the minimum dropPerSecond that will make an underflow. The LPDAFactory must ensure that the dropPerSecond passed by the creator is below this minimum.

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPriceFactory.sol#L30 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDAFactory.sol#L30 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEditionFactory.sol#L30

Vulnerability details

Impact

A user expect that all the sales created by the sales factories (Fixed, Open and LPDA) are for Escher721 editions contracts from curated creators. They also expect what the logic of this contract is, since they are created using clones on the Escher721Factory. Both of these assumptions can be broken, and the users can be tricked to buy from a not curated edition and whose logic is arbitrary.

Proof of Concept

The 3 sales factories (Fixed Sale, Open, LPDA) include the following require:

require(IEscher721(_sale.edition).hasRole(bytes32(0x00), msg.sender), "NOT AUTHORIZED");

This is to check that the user creating the sale has the admin role of the Escher721 edition for which the sale is being created for. The problem is that there is no check that the Escher721 edition contract passed as parameter was indeed deployed by the Escher721Factory contract. Any user can create a contract with a function hasRole(bytes32, address) that returns true and will bypass the check.

Tools Used

Manual

Recommended Mitigation Steps

Add a mapping(address => bool) public isEditionDeployedByFactory on the Escher721Factory so the sales factories can know if a given Escher721 edition was indeed deployed by the Escher721Factory.

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

Use of selfdestruct in FixedPrice.sol and OpenEdition.sol

Lines of code

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

Vulnerability details

Impact

The ether earned by the creator and the fees for the platform can get frozen.

Proof of Concept

The only way the creator can get out the ether earned by the sale (and also the platform to get the feest) is by the next code snippet from the buy funtion at the LPDA.sol contract:

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

The conditional is that the final id token has been sold. The Denial of Service can happen if by other ways the collection mints a token id that overlaps with the range of {currentId,finalId} of the LPDA sale. I prodive a test to show this behavior:

function test_LPDAD_DoS_Due_To_Minting_Overlap() public {
        // make the lpda sales contract
        sale = LPDA(lpdaSales.createLPDASale(lpdaSale));
        // authorize the lpda sale to mint tokens
        edition.grantRole(edition.MINTER_ROLE(), address(sale));
        //lets buy an NFT
        sale.buy{value: 1 ether}(1);
        assertEq(address(sale).balance, 1 ether);

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

        // token id: 2 overlaps with the range of 1-10 of the LPDA sale.
        edition.mint(address(sale), 2);

        // try to buy
        vm.expectRevert('ERC721: token already minted');
        sale.buy{value: uint256((0.9 ether + lpdaSale.dropPerSecond) * 9)}(9);
    }

Tools Used

Manual and Tests

Recommended Mitigation Steps

Use a pull payment system for the creator that can be used as a way to pull ether out without depending on selling all the range of token ids for the sale.

Escher.sol:

1.- uri is the same for the creator token and curator token.

At LoC 21 the function setURI(string memory _newuri) is used to set the uri of the collection. This function will set the same uri for all the tokens. The openzeppelin documentation clearly says that is the responsibility of the client fetching the uri to replace the '/id/' part of the uri. The documentation of Escher does not say if this is the behavior they want, or they prefer a different uri for each token id in case they use arweave for example.

2.- curators can grant themselves the creator role.

At LoC 27 the function addCreator(address _account) allows only to the curators to grant an account the role of creator. In the current implementation, a curator can give himself the role of creator:

function addCreator(address _account) public onlyRole(CURATOR_ROLE) {
        _grantRole(CREATOR_ROLE, _account);
    }

Escher lacks documentation if this is the behavior they want or not.

Escher721.sol:

3.- Creator of an Escher721 edition cannot set royalty information for tokens individually.

The documentation says that a creator can call the function setTokenRoyalty() with three parameters: id, receiver, feeNumerator. This is to set the royalty information for the given id. But the Escher721.sol only declares an external function for setDefaultRoyalty() which sets the same royalty information for all the tokens. The ERC2981 contract inherited from openzeppelin only contains an internal version of setTokenRoyalty(), so it needs to declare an external function which invokes the internal one.