Escher contest - HollaDieWaldfee's results

A decentralized curated marketplace for editioned artwork.

General Information

Platform: Code4rena

Start Date: 06/12/2022

Pot Size: $36,500 USDC

Total HM: 16

Participants: 119

Period: 3 days

Judge: berndartmueller

Total Solo HM: 2

Id: 189

League: ETH

Escher

Findings Distribution

Researcher Performance

Rank: 7/119

Findings: 7

Award: $1,530.63

QA:
grade-a

🌟 Selected for report: 1

🚀 Solo Findings: 0

Lines of code

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

Vulnerability details

Impact

A LPDA sale can be set up in a way that the price drops below zero.

If the price drops below zero, the call to LPDA.getPrice (https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L117) reverts.

This will cause calls to LPDA.buy and LPDA.refund to revert.

So users cannot get their refunds and if not all NFTs were sold (newId == temp.finalId (https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L81)) before, the artist cannot get his revenue from the sold NFTs.

A malicious artist can make the price go below zero intentionally to cause a loss for the buyers. He should set the amount that is to be sold so low that he can be sure they will get sold. Thereby only the buyers will lose.

Of course this issue can happen by accident as well. Then both the buyers and the artist can loose funds.

Proof of Concept

Add the following test to the LPDATest contract in LPDA.t.sol:

function test_priceBelow0() public {
    // set up a LPDA Sale
    LPDA.Sale memory saleData = LPDA.Sale({
        currentId: uint48(0),
        finalId: uint48(10),
        edition: address(edition),
        startPrice: uint80(100),
        finalPrice: uint80(0),
        dropPerSecond: uint80(10),
        startTime: uint96(block.timestamp),
        saleReceiver: payable(address(69)),
        endTime: uint96(block.timestamp + 1 days)
    });

    sale = LPDA(lpdaSales.createLPDASale(saleData));

    edition.grantRole(edition.MINTER_ROLE(), address(sale));

    sale.buy{value: 1 ether}(1);

    vm.warp(block.timestamp + 100);

    // the refund will revert
    sale.refund();
}

Observe that the call to sale.refund() will revert because the price will drop to 0 in 10 seconds.
So after >=11 seconds the LPDA.getPrice will revert.

Tools Used

VSCode

When creating a LPDA Sale, the LPDAFactory should check that the lowest possible price is above zero:

require(sale.dropPerSecond * (sale.endTime - sale.startTime) >= sale.startPrice);

#0 - c4-judge

2022-12-11T11:38:10Z

berndartmueller marked the issue as duplicate of #392

#1 - c4-judge

2023-01-02T19:54:54Z

berndartmueller changed the severity to 3 (High Risk)

#2 - c4-judge

2023-01-02T19:54:59Z

berndartmueller marked the issue as satisfactory

Awards

49.6134 USDC - $49.61

Labels

bug
3 (High Risk)
satisfactory
duplicate-441

External Links

Lines of code

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

Vulnerability details

Impact

When the last NFT is sold in the LPDA Sale by calling LPDA.buy (https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L58-L89), the revenue will be sent to the saleReceiver (artist).
Users that have bought NFTs for a price higher than the finalPrice can get a refund.

There are now two possibilities we must differentiate:

  1. The sum of all refunds is at least as big as the revenue.
    In this case anyone including the artist can call LPDA.buy with msg.value=0 and _amount=0 and withdraw the revenue a second time, effectively stealing the refunds

  2. The sum of all refunds is not as big as the revenue.
    This means there must be additional ETH in the contract in order to be able to withdraw the revenue a second time. So the artist calls LPDA.buy with msg.value = price * amountSold - address(LPDA).balance and _amount=0. This will allow the artist to withdraw all pending refunds.

So in summary, any pending refunds can be transferred to the artist after the last NFT got minted.

Proof of Concept

I implemented a scenario using foundry. You can add the test to the LPDATest contract in the LPDA.t.sol file.

The scenario is this:

  1. Two NFTs are sold with a start price of 0.5 ETH. Every second the price drops 0.01 ETH.
  2. The victim buys 1 NFT for 0.5 ETH
  3. After 25 seconds, the victim buys the second NFT for 0.25 ETH
  4. The final price is now 0.25 ETH and the victim should be able to get a refund of 0.25 ETH
  5. However the artist calls LPDA.buy himself (with sufficient msg.value) and withdraws the refunds
function test_stealRefunds() public {
    deal(address(69), 1 ether);
    // set up a LPDA Sale
    LPDA.Sale memory saleData = LPDA.Sale({
        currentId: uint48(0),
        finalId: uint48(2),
        edition: address(edition),
        startPrice: uint80(0.5 ether),
        finalPrice: uint80(0),
        dropPerSecond: uint80(0.01 ether),
        startTime: uint96(block.timestamp),
        saleReceiver: payable(address(this)),
        endTime: uint96(block.timestamp + 50)
    });

    sale = LPDA(lpdaSales.createLPDASale(saleData));

    edition.grantRole(edition.MINTER_ROLE(), address(sale));

    vm.prank(address(69));
    sale.buy{value: 0.5 ether}(1); // price of NFT is now 0.5 ETH

    vm.warp(block.timestamp + 25); // price of NFT is now 0.25 ETH

    uint256 artist_balance_before = address(this).balance;

    vm.prank(address(69));
    sale.buy{value: 0.25 ether}(1); // final price of NFTs is now 0.25 ETH
    // so the saleReceiver, which is address(this) receives 2 * 0.25 ETH = 0.5 ETH

    uint256 artist_balance_after = address(this).balance;
    console.log(artist_balance_after - artist_balance_before);

    // address(69) should be able to refund 0.25 ETH
    // however address(this) now calls buy() with msg.value = 0.25 ETH along with _amount=0

    artist_balance_before = address(this).balance;
    sale.buy{value:0.25 ether}(0);
    artist_balance_after = address(this).balance;
    console.log(artist_balance_after - artist_balance_before);
    // address(this) has now stolen the remaining 0.25 ETH

    // and balance of sale is 0
    console.log(address(sale).balance);
}

Tools Used

VSCode

The LPDA.buy function should check that _amount > 0.
If the amount is greater 0, the function call will always revert once the finalId is reached, so the revenue cannot be withdrawn multiple times.

#0 - c4-judge

2022-12-12T09:25:38Z

berndartmueller marked the issue as duplicate of #16

#1 - c4-judge

2023-01-04T10:13:53Z

berndartmueller marked the issue as satisfactory

#2 - C4-Staff

2023-01-10T22:21:33Z

JeeberC4 marked the issue as duplicate of #441

Findings Information

🌟 Selected for report: HollaDieWaldfee

Also found by: 0xNazgul, cccz, hansfriese, obront

Labels

bug
2 (Med Risk)
primary issue
selected for report
M-01

Awards

375.912 USDC - $375.91

External Links

Lines of code

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

Vulnerability details

Impact

On the code4rena page of this contest there is a "SALES PATTERNS" section that describes the flow of how to use Sales:

https://code4rena.com/contests/2022-12-escher-contest

It contains this statement:

If the artist would like sales and royalties to go somewhere other than the default royalty receiver, they
must call setTokenRoyalty with the following variables

So an artist should be able to call the setTokenRoyalty function on the Escher721 contract.

However this function cannot be called. It does not exist. There exists a _setTokenRoyalty in ERC2981.sol from openzeppelin. This function however is internal (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/3d7a93876a2e5e1d7fe29b5a0e96e222afdc4cfa/contracts/token/common/ERC2981.sol#L94).

So there is no setTokenRoyalty function that can be called by the artist. So an artist cannot set a royalty individually for a token as is stated in the documentation.

Proof of Concept

I tried calling setTokenRoyalty from inside Escher721.t.sol with the following test:

function test_setDefaultRoyalty() public {
    edition.setTokenRoyalty(1,address(this), 5);
}

This won't even compile because the setTokenRoyalty function does not exist.

Tools Used

VSCode

In order to expose the internal _setTokenRoyalty function to the artist, add the following function to the Escher721 contract:

function setTokenRoyalty(
    uint256 tokenId,
    address receiver,
    uint96 feeNumerator
) public onlyRole(DEFAULT_ADMIN_ROLE) {
    _setTokenRoyalty(tokenId,receiver,feeNumerator);
}

#0 - c4-judge

2022-12-14T09:13:36Z

berndartmueller marked the issue as duplicate of #181

#1 - c4-judge

2023-01-03T15:50:49Z

berndartmueller marked the issue as selected for report

#2 - C4-Staff

2023-01-10T22:22:01Z

JeeberC4 marked the issue as not a duplicate

#3 - C4-Staff

2023-01-10T22:22:15Z

JeeberC4 marked the issue as primary issue

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L109 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L85 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L86 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L105 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEdition.sol#L92

Vulnerability details

Impact

Note: While the code that is referenced here is also mentioned in the automated findings of this contest, the automation tool has mistakenly recognized these instances as ERC20 operations. So I outline in this report that these are not ERC20 operations and I will give a correct estimation of the impact.

The transfer function allows the callee to spend only 2300 Gas. This was introduced in order to mitigate Reentrancy attacks.

However using the transfer function introduces issues for two reasons:

  1. Function call reverts if callee runs code upon receiving ETH that uses more than 2300 Gas
  2. Gas costs may change significantly in the future so using transfer may break code that runs successfully now

For the two reasons stated above, it must be assumed that calling the transfer function can fail which leads to DOS in the affected contracts.

Instances where transfer is used:
https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L109

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

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

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

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

Proof of Concept

I will show the impact for the first instance of this issue (https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L109):

If the feeReceiver employs code upon receiving ETH that uses up more than 2300 Gas, the call to the FixedPrice._end function will revert.

This means the last NFT cannot be minted, the feeReceiver cannot receive fees and the saleReceiver cannot receive the revenue generated by selling NFTs.

Tools Used

VSCode

Use call() instead of transfer(), and be sure to respect the CEI pattern and/or add re-entrancy guards to the functions that make use of call().

Fix (for first instance):

(bool success, ) = ISaleFactory(factory).feeReceiver().call{value: address(this).balance / 20}("");
require(success, "Transfer Failed");

#0 - c4-judge

2022-12-10T00:31:06Z

berndartmueller marked the issue as duplicate of #99

#1 - c4-judge

2023-01-03T12:48:53Z

berndartmueller marked the issue as satisfactory

Findings Information

Awards

28.357 USDC - $28.36

Labels

bug
2 (Med Risk)
satisfactory
duplicate-390

External Links

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/Escher721.sol#L51 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L50 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L92 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEdition.sol#L75

Vulnerability details

Impact

According to the Sale contracts the only time when a Sale should be able to be cancelled is before the startTime (FixedPrice.cancel, OpenEdition.cancel, LPDA.cancel).

However the artist can mint a token (https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/Escher721.sol#L51) with some id that is for sale which will cancel the sale and any tokens with an id >= the id of the minted token cannot be sold.

This is a problem because by creating a sale, the artists commits to the rules of the sale and he should not be able to step back from the decision he has made.

E.g. if an artist decides to sell 10 tokens at a price of $100 each, he should not be able to cancel the sale after the sale has already started.

Proof of Concept

  1. The artist creates a FixedPrice sale with 10 tokens for sale
  2. The startTime has passed which means that the artist cannot call cancel anymore.
  3. The artist mints the token with id 1
  4. Now the sale is effectively cancelled

Tools Used

VSCode

There should be a mechanism in place such that tokens that are for sale cannot be minted.
Some sort of lock might be created by the artist (owner of the Escher721 contract) for the mint functionality which is checked by the Sale contract and can only be released by the Sale contract once the sale is finished.

In the case of a FixedPrice and LPDA sale which deal with a fixed amount of tokens, the tokens could be minted before the sale and transferred to the Sale contract instead of minting the tokens during the sale. However this does not work for OpenEdition Sales.

#0 - c4-judge

2022-12-13T11:34:21Z

berndartmueller marked the issue as duplicate of #390

#1 - c4-judge

2023-01-02T20:05:32Z

berndartmueller marked the issue as satisfactory

Findings Information

🌟 Selected for report: adriro

Also found by: 0xA5DF, HollaDieWaldfee

Labels

bug
2 (Med Risk)
satisfactory
duplicate-399

Awards

594.9857 USDC - $594.99

External Links

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L50 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L92 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEdition.sol#L75 https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/25aabd286e002a1526c345c8db259d57bdf0ad28/contracts/access/AccessControlUpgradeable.sol#L165

Vulnerability details

Impact

A Sale contract (FixedPrice, OpenEdition, LPDA) mints NFTs to the buyers.
In order to mint NFTs, the Sale contract must be granted the MINTER_ROLE first by calling ERC721.grantRole (i.e. the artist must allow the Sale to mint tokens).

The issue is that this role can be revoked at any time by calling ERC721.revokeRole.

According to the Sale contracts the only time when a Sale should be able to be cancelled is before the startTime (FixedPrice.cancel, OpenEdition.cancel, LPDA.cancel).

Cancelling an ongoing Sale is especially bad for a LPDA Sale. In a LPDA Sale the price will drop continously and the price that is paid for the last NFT will be the price that the artist receives for all NFTs.

So when the LPDA is going on without any Sale occurring, the artist can decide to cancel the sale by revoking the MINTER_ROLE from the Sale and avoid a scenario where the NFTs are sold for a low price.

By starting a Sale, the artist should be committed to selling the NFTs under the conditions specified in the Sale (As is also clearly intended by allowing to call the cancel function only before the startTime).

Proof of Concept

Add the following Test to the LPDATest contract in LPDA.t.sol:

function test_BuyRevoke() public {
    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);

    // cancel the Sale by revoking the MINTER_ROLE
    edition.revokeRole(edition.MINTER_ROLE(), address(sale));

    sale.buy{value: 1 ether}(1);
}

Observe that the second sale.buy causes a revert.

Tools Used

VSCode

There should be a timelock applied to the revokeRole function. The artist should activate the timelock before creating the Sale.

The Sale should then check that timelock.endTime >= sale.endTime.

In the case of a FixedPrice sale (which currently does not have an endTime) you might introduce an endTime as well. So the FixedPrice Sale either ends when all NFTs are sold or the endTime is reached (whichever is first).

#0 - c4-judge

2022-12-13T13:53:05Z

berndartmueller marked the issue as duplicate of #399

#1 - c4-judge

2023-01-03T10:31:45Z

berndartmueller marked the issue as satisfactory

Awards

480.3105 USDC - $480.31

Labels

bug
grade-a
QA (Quality Assurance)
Q-11

External Links

Escher Low Risk and Non-Critical Issues

Summary

RiskTitleFileInstances
L-00Spamming Buy events by buying 0 NFTs-3
L-01Do not use selfdestruct to transfer ETH-2
L-02Use safeMint instead of mint-3
L-03Missing zero address checks-6
L-04openzeppelin Ownable does not use 2-Step-Process to transfer ownership-7
L-05Missing sanity checks when creating Sales-2
L-06NFTs with tokenId 0 cannot be sold-3
N-00Unlocked pragma-12

[L-00] Spamming Buy events by buying 0 NFTs

The FixedPrice.buy, OpenEdition.buy and LPDA.buy functions can be called with _amount=0 and msg.value=0.

This will cause a Buy event to be emitted.

So a malicious user can create Buy events, thereby negatively impacting the operation of any components that listen for these events.

E.g. a UI might display all Buy events. If there are a lot of Buy events with zero amount, users must sort through them in the UI.

You can fix this by adding require(_amount > 0) to the buy functions.

These are the 3 instances:
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/LPDA.sol#L58-L89

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

[L-01] Do not use selfdestruct to transfer ETH

OpenEdition._end and FixedPrice._end use selfdestruct to transfer ETH to the saleReceiver address.

Transferring ETH via selfdestruct does not call any logic that is implemented on the receiver side.

A receiver might implement the receive() function that keeps track of incoming payments. By using selfdestruct this logic will not be executed.

So instead of calling selfdestruct, the saleReceiver should be required to implement some payable function that can then be called by the Sale.

You should also make sure that a failed call to this function does not impact the operation of the Sale from the buyer perspective.

After all the ETH balance is transferred to the saleReceiver you can then call selfdestruct with any address.

These are the 2 instances:

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/OpenEdition.sol#L122

[L-02] Use safeMint instead of mint

It is best practice to use safeMint over mint. If the receiving address is a contract, safeMint requires that it implements the onERC721Received function correctly which makes sure that the contract can handle the ERC721 token it receives.

Make sure to respect the CEI pattern when using safeMint or use a reentrancy-guard, as safeMint calls an external function.

There are 3 instances where mint is used:
https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L66

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

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

And the Escher721 contract must also make safeMint accessible to addresses with the MINTER_ROLE.

[L-03] Missing zero address checks

There are 6 instances of missing zero address checks.
I will list all of them with a quick description of the impact.

feeRecevier

Accidentally setting feeReceiver for a factory to the zero address will cause a loss of fees for all Sales created with that factory.
https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPriceFactory.sol#L43

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

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

tokenUriDelegate

If the tokenUriDelegate is the zero address, the NFTs do not have an URI associated with them.
https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/Escher721.sol#L58

_sale.saleReceiver

If there the saleReceiver is the zero address, all sale revenue will be lost.

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

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

[L-04] openzeppelin Ownable does not use 2-Step-Process to transfer ownership

Many of the contracts in scope inherit from Ownable or OwnableUpgradeable. The problem is that these two contracts do not implement a 2-Step-Process for transferring ownership which increases the risk of accidentally transferring ownership to a wrong address, thereby losing ownership of the contract.

You should consider using Ownable2Step (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol) and Ownable2StepUpgradeable (https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/access/Ownable2StepUpgradeable.sol) instead.

These are the instances where Ownable or OwnableUpgradeable are inherited from:

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

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

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

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

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

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

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/uris/Base.sol#L7

[L-05] Missing sanity checks when creating Sales

The functions FixedPriceFactory.createdFixedSale, OpenEditionFactory.createOpenEdition and LPDAFactory.createLPDASale all employ sanity checks on the Sale struct paramter.

However the checks in FixedPriceFactory.createdFixedSale and OpenEditionFactory.createOpenEdition are less thorough. E.g. they don't check that the price is greater 0. So there should be more sanity checks added (as well as the zero address checks mentioned above).

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

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

[L-06] NFTs with tokenId 0 cannot be sold

tokenIds for ERC721 tokens start at 0. However the buy functions in the Sale contracts only allow buying tokens starting from id 1. This should be changed such that tokens with id 0 can be sold as well.

There are 3 instances of this:
https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L65

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

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

[N-00] Unlocked pragma

The files in scope use Solidity version ^0.8.17.
Consider using a locked version like 0.8.17 to make sure the files only compile with this specific version.

There are 12 instances of this.
https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/Escher.sol#L2

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

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/Escher721Factory.sol#L2

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/uris/Base.sol#L2

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/uris/Generative.sol#L2

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/uris/Unique.sol#L2

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

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

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

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

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

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

#0 - c4-judge

2023-01-04T10:53:20Z

berndartmueller marked the issue as grade-a

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter