Escher contest - adriro'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: 8/119

Findings: 7

Award: $1,010.70

QA:
grade-b
Gas:
grade-b

🌟 Selected for report: 3

🚀 Solo Findings: 0

Lines of code

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

Vulnerability details

The dutch auction in the LPDA contract is implemented by configuring a start price and price drop per second.

A bad set of settings can cause an issue where the elapsed duration of the sale multiplied by the drop per second gets bigger than the start price and underflows the current price calculation.

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

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

This means that if temp.dropPerSecond * timeElapsed > temp.startPrice then the unsigned integer result will become negative and underflow, leading to potentially bricking the contract and an eventual loss of funds.

Impact

Due to Solidity 0.8 default checked math, the subtraction of the start price and the drop will cause a negative value that will generate an underflow in the unsigned integer type and lead to a transaction revert.

Calls to getPrice will revert, and since this function is used in the buy to calculate the current NFT price it will also cause the buy process to fail. The price drop will continue to increase as time passes, making it impossible to recover from this situation and effectively bricking the contract.

This will eventually lead to a loss of funds because currently the only way to end a sale and transfer funds to the sale and fee receiver is to buy the complete set of NFTs in the sale (i.e. buy everything up to the sale.finalId) which will be impossible if the buy function is bricked.

PoC

In the following test, the start price is 1500 and the duration is 1 hour (3600 seconds) with a drop of 1 per second. At about ~40% of the elapsed time the price drop will start underflowing the price, reverting the calls to both getPrice and buy.

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

import "forge-std/Test.sol";
import {FixedPriceFactory} from "src/minters/FixedPriceFactory.sol";
import {FixedPrice} from "src/minters/FixedPrice.sol";
import {OpenEditionFactory} from "src/minters/OpenEditionFactory.sol";
import {OpenEdition} from "src/minters/OpenEdition.sol";
import {LPDAFactory} from "src/minters/LPDAFactory.sol";
import {LPDA} from "src/minters/LPDA.sol";
import {Escher721} from "src/Escher721.sol";

contract AuditTest is Test {
    address deployer;
    address creator;
    address buyer;

    FixedPriceFactory fixedPriceFactory;
    OpenEditionFactory openEditionFactory;
    LPDAFactory lpdaFactory;

    function setUp() public {
        deployer = makeAddr("deployer");
        creator = makeAddr("creator");
        buyer = makeAddr("buyer");

        vm.deal(buyer, 1e18);

        vm.startPrank(deployer);

        fixedPriceFactory = new FixedPriceFactory();
        openEditionFactory = new OpenEditionFactory();
        lpdaFactory = new LPDAFactory();

        vm.stopPrank();
    }
    
    function test_LPDA_getPrice_NegativePrice() public {
        // Setup NFT and create sale
        vm.startPrank(creator);

        Escher721 nft = new Escher721();
        nft.initialize(creator, address(0), "Test NFT", "TNFT");

        // Duration is 1 hour (3600 seconds), with a start price of 1500 and a drop of 1, getPrice will revert and brick the contract at about 40% of the elapsed duration
        uint48 startId = 0;
        uint48 finalId = 1;
        uint80 startPrice = 1500;
        uint80 dropPerSecond = 1;
        uint96 startTime = uint96(block.timestamp);
        uint96 endTime = uint96(block.timestamp + 1 hours);

        LPDA.Sale memory sale = LPDA.Sale(
            startId, // uint48 currentId;
            finalId, // uint48 finalId;
            address(nft), // address edition;
            startPrice, // uint80 startPrice;
            0, // uint80 finalPrice;
            dropPerSecond, // uint80 dropPerSecond;
            endTime, // uint96 endTime;
            payable(creator), // address payable saleReceiver;
            startTime // uint96 startTime;
        );
        LPDA lpdaSale = LPDA(lpdaFactory.createLPDASale(sale));

        nft.grantRole(nft.MINTER_ROLE(), address(lpdaSale));

        vm.stopPrank();

        // simulate we are in the middle of the sale duration
        vm.warp(startTime + 0.5 hours);

        vm.startPrank(buyer);

        // getPrice will revert due to the overflow caused by the price becoming negative
        vm.expectRevert();
        lpdaSale.getPrice();

        // This will also cause the contract to be bricked, since buy needs getPrice to check that the buyer is sending the correct amount
        uint256 amount = 1;
        uint256 price = 1234;
        vm.expectRevert();
        lpdaSale.buy{value: price * amount}(amount);

        vm.stopPrank();
    }
}

Recommendation

Add a validation in the LPDAFactory.createLPDASale function to ensure that the given duration and drop per second settings can't underflow the price.

require((sale.endTime - sale.startTime) * sale.dropPerSecond <= sale.startPrice, "MAX DROP IS GREATER THAN START PRICE");

#0 - c4-judge

2022-12-11T11:35:36Z

berndartmueller marked the issue as primary issue

#1 - c4-sponsor

2022-12-22T15:56:02Z

stevennevins marked the issue as sponsor confirmed

#2 - c4-judge

2023-01-02T19:58:55Z

berndartmueller marked the issue as selected for report

Awards

1.3417 USDC - $1.34

Labels

bug
2 (Med Risk)
satisfactory
duplicate-328

External Links

Lines of code

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

Vulnerability details

The LPDA sale type is created with a configurable end time which is not taked into account and doesn't play any role in finalizing the auction.

The ending condition for the sale in this contract can be seen in lines 81-87:

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

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

This means that the only way to end an auction and distribute funds is when the last NFT in the sale is sold. The sale's endTime should be taked into account and the owner should be able to finalize the auction provided the sale has reached this end time.

Impact

Since the auction end time is not taked into account to finalize and end the sale, the owner or sale beneficiary will need to rely on all NFTs from the sale being sold in order to end the sale and withdraw funds.

This will make it impossible to withdraw funds until the last NFT from the sale is sold, even if the auction has already ended.

PoC

  1. Create a sale using LPDAFactory.createLPDASale with startTime = t1 and endTime = t2.
  2. During t1 to t2 not all NFTs from the collection are sold. Meaning that at time t2, sale.currentId < sale.finalId.
  3. When t2 is reached the auction has reached it's end time, but the owner of the sale can't finalize it and withdraw their funds.

Recommendation

Add a finalize function (similar to the OpenEdition sale) to able to end the auction and withdraw funds to beneficiaries. In this case, the finalPrice should be set to the price that corresponds to the sale.endTime time.

contract LPDA is Initializable, OwnableUpgradeable, ISale {
    ....

    bool finalized;

    function finalize() external {
        Sale memory _sale = sale;
        require(block.timestamp >= _sale.endTime, "TOO SOON");
        require(!finalized, "ALREADY FINALIZED");
        
        uint256 price = getPrice(); // this should properly return price because timeElapse == end - start
        
        // Update storage state
        sale.finalPrice = price;
        sale.finalId = _sale.currentId;
        finalized = true;
        
        // Distribute funds
        uint256 totalSale = price * amountSold;
        uint256 fee = totalSale / 20;
        ISaleFactory(factory).feeReceiver().transfer(fee);
        _sale.saleReceiver.transfer(totalSale - fee);
        
        // emit event
        _end();
    }
}

#0 - c4-judge

2022-12-12T09:00:05Z

berndartmueller marked the issue as duplicate of #328

#1 - c4-judge

2023-01-02T20:22:46Z

berndartmueller marked the issue as satisfactory

Findings Information

Awards

36.8641 USDC - $36.86

Labels

bug
2 (Med Risk)
disagree with severity
downgraded by judge
primary issue
selected for report
sponsor disputed
edited-by-warden
M-10

External Links

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPrice.sol#L66 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEdition.sol#L67 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L74

Vulnerability details

In all of the three sale types of contracts, the buy function will mint tokens with sequential ids starting at a given configurable value.

If an external entity mints any of the token ids involved in the sale, then the buy procedure will fail since it will try to mint an already existing id. This can be the creator manually minting a token or another similar contract that creates a sale.

Taking the FixedPrice contract as the example, if any of the token ids between sale.currentId + 1 and sale.finalId is minted by an external entity, then the buy process will be bricked since it will try to mint an existing token id. This happens in lines 65-67:

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPrice.sol#L65-L67

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

The implementation of the mint function (OZ contracts) requires that the token id doesn't exist:

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC721/ERC721Upgradeable.sol#L293

function _mint(address to, uint256 tokenId) internal virtual {
    require(to != address(0), "ERC721: mint to the zero address");
    require(!_exists(tokenId), "ERC721: token already minted");
    ...

Impact

If any of the scenarios previously described happens, then the contract will be bricked since it will try to mint an already existing token and will revert the transaction. There is no way to update the token ids in an ongoing sale, which means that the buy function will always fail for any call.

This will also cause a loss of funds in the case of the FixedPrice and LPDA contracts since those two require that all token ids are sold before funds can be withdrawn.

PoC

The following test reproduces the issue using the FixedPrice sale contract type.

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

import "forge-std/Test.sol";
import {FixedPriceFactory} from "src/minters/FixedPriceFactory.sol";
import {FixedPrice} from "src/minters/FixedPrice.sol";
import {OpenEditionFactory} from "src/minters/OpenEditionFactory.sol";
import {OpenEdition} from "src/minters/OpenEdition.sol";
import {LPDAFactory} from "src/minters/LPDAFactory.sol";
import {LPDA} from "src/minters/LPDA.sol";
import {Escher721} from "src/Escher721.sol";

contract AuditTest is Test {
    address deployer;
    address creator;
    address buyer;

    FixedPriceFactory fixedPriceFactory;
    OpenEditionFactory openEditionFactory;
    LPDAFactory lpdaFactory;

    function setUp() public {
        deployer = makeAddr("deployer");
        creator = makeAddr("creator");
        buyer = makeAddr("buyer");

        vm.deal(buyer, 1e18);

        vm.startPrank(deployer);

        fixedPriceFactory = new FixedPriceFactory();
        openEditionFactory = new OpenEditionFactory();
        lpdaFactory = new LPDAFactory();

        vm.stopPrank();
    }
    
    function test_FixedPrice_buy_MintBrickedContract() public {
        // Suppose there's another protocol or account that acts as a minter, it can even be another sale that overlaps ids.
        address otherMinter = makeAddr("otherMinter");

        // Setup NFT and create sale
        vm.startPrank(creator);

        Escher721 nft = new Escher721();
        nft.initialize(creator, address(0), "Test NFT", "TNFT");

        uint48 startId = 10;
        uint48 finalId = 15;
        uint96 price = 1;

        FixedPrice.Sale memory sale = FixedPrice.Sale(
            startId, // uint48 currentId;
            finalId, // uint48 finalId;
            address(nft), // address edition;
            price, // uint96 price;
            payable(creator), // address payable saleReceiver;
            uint96(block.timestamp) // uint96 startTime;
        );
        FixedPrice fixedSale = FixedPrice(fixedPriceFactory.createFixedSale(sale));

        nft.grantRole(nft.MINTER_ROLE(), address(fixedSale));
        nft.grantRole(nft.MINTER_ROLE(), otherMinter);

        vm.stopPrank();

        // Now, other minter mints at least one of the ids included in the sale
        vm.prank(otherMinter);
        nft.mint(makeAddr("receiver"), startId + 2);

        // Now buyer goes to sale and tries to buy the NFTs
        vm.startPrank(buyer);

        // The following will revert. The contract will be bricked since it will be impossible to advance with the ids from the sale.
        uint256 amount = finalId - startId;
        vm.expectRevert("ERC721: token already minted");
        fixedSale.buy{value: price * amount}(amount);

        vm.stopPrank();
    }
}

Recommendation

The quick solution would be to mint the tokens to the sale contract, but that will require a potentially high gas usage if the sale involves a large amount of tokens.

Other alternatives would be to "reserve" a range of tokens to a particular minter (and validate that each one mints over their own range) or to have a single minter role at a given time, so there's just a single entity that can mint tokens at a given time.

#0 - c4-judge

2022-12-13T10:48:48Z

berndartmueller marked the issue as primary issue

#1 - mehtaculous

2022-12-22T17:20:31Z

This is considered more of a Medium risk and a solution would be to have the ERC-721 contract manage the sequential IDs and have the sales contracts simply request minting of the next token ID

#2 - c4-sponsor

2022-12-22T17:22:45Z

mehtaculous marked the issue as sponsor disputed

#3 - c4-sponsor

2022-12-22T17:25:08Z

mehtaculous marked the issue as disagree with severity

#4 - berndartmueller

2023-01-02T20:04:20Z

Due to the external requirement of this finding, I'm downgrading it to Medium severity.

#5 - c4-judge

2023-01-02T20:04:34Z

berndartmueller changed the severity to 2 (Med Risk)

#6 - c4-judge

2023-01-02T20:05:10Z

berndartmueller marked the issue as selected for report

Findings Information

🌟 Selected for report: adriro

Also found by: 0xA5DF, HollaDieWaldfee

Labels

bug
2 (Med Risk)
primary issue
selected for report
sponsor confirmed
M-11

Awards

773.4814 USDC - $773.48

External Links

Lines of code

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

Vulnerability details

The OpenEdition type of sale has a start time and an end time. The creator (or owner of the contract) can cancel a sale using the cancel function only if it hasn't started yet (i.e. start time is after current block timestamp).

However, the NFT creator can still revoke the minting permissions to the sale contract if he wishes to pull out of the sale. This will prevent anyone from calling the buy and prevent any further sale from the collection.

Impact

The owner of the sale contract can still virtually cancel a sale after it has started by simply revoking the minting permissions to the sale contract.

This will cause the buy function to fail because the call to mint will revert, effectively making it impossible to further purchase NFTs and causing the effect of canceling the sale.

PoC

In the following test, the creator of the sale decides to pull from it in the middle of the elapsed duraction. After that, he only needs to wait until the end time passes to call finalize and end the sale.

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

import "forge-std/Test.sol";
import {FixedPriceFactory} from "src/minters/FixedPriceFactory.sol";
import {FixedPrice} from "src/minters/FixedPrice.sol";
import {OpenEditionFactory} from "src/minters/OpenEditionFactory.sol";
import {OpenEdition} from "src/minters/OpenEdition.sol";
import {LPDAFactory} from "src/minters/LPDAFactory.sol";
import {LPDA} from "src/minters/LPDA.sol";
import {Escher721} from "src/Escher721.sol";

contract AuditTest is Test {
    address deployer;
    address creator;
    address buyer;

    FixedPriceFactory fixedPriceFactory;
    OpenEditionFactory openEditionFactory;
    LPDAFactory lpdaFactory;

    function setUp() public {
        deployer = makeAddr("deployer");
        creator = makeAddr("creator");
        buyer = makeAddr("buyer");

        vm.deal(buyer, 1e18);

        vm.startPrank(deployer);

        fixedPriceFactory = new FixedPriceFactory();
        openEditionFactory = new OpenEditionFactory();
        lpdaFactory = new LPDAFactory();

        vm.stopPrank();
    }
    
    function test_OpenEdition_buy_CancelSaleByRevokingRole() public {
        // Setup NFT and create sale
        vm.startPrank(creator);

        Escher721 nft = new Escher721();
        nft.initialize(creator, address(0), "Test NFT", "TNFT");

        uint24 startId = 0;
        uint72 price = 1e6;
        uint96 startTime = uint96(block.timestamp);
        uint96 endTime = uint96(block.timestamp + 1 hours);

        OpenEdition.Sale memory sale = OpenEdition.Sale(
            price, // uint72 price;
            startId, // uint24 currentId;
            address(nft), // address edition;
            startTime, // uint96 startTime;
            payable(creator), // address payable saleReceiver;
            endTime // uint96 endTime;
        );
        OpenEdition openSale = OpenEdition(openEditionFactory.createOpenEdition(sale));

        nft.grantRole(nft.MINTER_ROLE(), address(openSale));

        vm.stopPrank();

        // simulate we are in the middle of the sale duration
        vm.warp(startTime + 0.5 hours);

        // Now creator decides to pull out of the sale. Since he can't cancel the sale because it already started and he can't end the sale now because it hasn't finished, he revokes the minter role. This will cause the buy transaction to fail.
        vm.startPrank(creator);

        nft.revokeRole(nft.MINTER_ROLE(), address(openSale));

        vm.stopPrank();

        vm.startPrank(buyer);

        // Buyer can't call buy because sale contract can't mint tokens, the buy transaction reverts.
        uint256 amount = 1;
        vm.expectRevert();
        openSale.buy{value: price * amount}(amount);

        vm.stopPrank();

        // Now creator just needs to wait until sale ends
        vm.warp(endTime);

        vm.prank(creator);
        openSale.finalize();
    }
}

Recommendation

One possibilty is to acknowledge the fact the creator has still control over the minting process and can arbitrarily decide when to cancel the sale. If this route is taken, then the recommendation would be to make the cancel function unrestricted.

Preminting the NFTs is not a solution because of high gas costs and the fact that the amount of tokens to be sold is not known beforehand, it's determined by the actual amount sold during the sale.

A more elaborated solution that would require additional architecture changes is to prevent the revocation of the minting permissions if some conditions (defined by target sale contract) are met.

#0 - c4-judge

2022-12-13T13:52:52Z

berndartmueller marked the issue as primary issue

#1 - c4-sponsor

2022-12-22T21:27:53Z

stevennevins marked the issue as sponsor confirmed

#2 - c4-judge

2023-01-03T10:31:48Z

berndartmueller marked the issue as selected for report

Findings Information

🌟 Selected for report: ForkEth

Also found by: CRYP70, Ch_301, Chom, Lambda, adriro, csanuragjain, minhquanym

Labels

bug
2 (Med Risk)
satisfactory
duplicate-474

Awards

131.7499 USDC - $131.75

External Links

Lines of code

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

Vulnerability details

The buy function present in the LPDA contract has a missing validation to check that the current sale hasn't finished at the time the function is called.

The contract has a round of validations around line 62:

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

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");
    ...

It correctly checks the sale's start time but fails to check the end time.

Impact

A buyer is allowed to still call the buy method after the sale has ended.

PoC

The following test reproduces the issue:

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

import "forge-std/Test.sol";
import {FixedPriceFactory} from "src/minters/FixedPriceFactory.sol";
import {FixedPrice} from "src/minters/FixedPrice.sol";
import {OpenEditionFactory} from "src/minters/OpenEditionFactory.sol";
import {OpenEdition} from "src/minters/OpenEdition.sol";
import {LPDAFactory} from "src/minters/LPDAFactory.sol";
import {LPDA} from "src/minters/LPDA.sol";
import {Escher721} from "src/Escher721.sol";

contract AuditTest is Test {
    address deployer;
    address creator;
    address buyer;

    FixedPriceFactory fixedPriceFactory;
    OpenEditionFactory openEditionFactory;
    LPDAFactory lpdaFactory;

    function setUp() public {
        deployer = makeAddr("deployer");
        creator = makeAddr("creator");
        buyer = makeAddr("buyer");

        vm.deal(buyer, 1e18);

        vm.startPrank(deployer);

        fixedPriceFactory = new FixedPriceFactory();
        openEditionFactory = new OpenEditionFactory();
        lpdaFactory = new LPDAFactory();

        vm.stopPrank();
    }
    
    function test_LPDA_buy_EndTimeNotChecked() public {
        // Setup NFT and create sale
        vm.startPrank(creator);

        Escher721 nft = new Escher721();
        nft.initialize(creator, address(0), "Test NFT", "TNFT");

        uint48 startId = 0;
        uint48 finalId = 1;
        uint80 startPrice = 1e6;
        uint80 dropPerSecond = 1;
        uint96 startTime = uint96(block.timestamp);
        uint96 endTime = uint96(block.timestamp + 1 hours);

        LPDA.Sale memory sale = LPDA.Sale(
            startId, // uint48 currentId;
            finalId, // uint48 finalId;
            address(nft), // address edition;
            startPrice, // uint80 startPrice;
            0, // uint80 finalPrice;
            dropPerSecond, // uint80 dropPerSecond;
            endTime, // uint96 endTime;
            payable(creator), // address payable saleReceiver;
            startTime // uint96 startTime;
        );
        LPDA lpdaSale = LPDA(lpdaFactory.createLPDASale(sale));

        nft.grantRole(nft.MINTER_ROLE(), address(lpdaSale));

        vm.stopPrank();

        // simulate we wait until after the sale's end time
        vm.warp(endTime + 1 hours);

        vm.startPrank(buyer);

        // The following succeeds even if we are past the end time
        uint256 amount = 1;
        uint256 price = lpdaSale.getPrice();
        lpdaSale.buy{value: price * amount}(amount);

        vm.stopPrank();
    }
}

Recommendation

Check that the current block timestamp is before the sale's end time in the buy function:

 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");
+        require(block.timestamp < temp.endTime, "TOO LATE");
         ...

#0 - c4-judge

2022-12-11T19:17:10Z

berndartmueller marked the issue as duplicate of #474

#1 - c4-judge

2023-01-02T20:31:07Z

berndartmueller marked the issue as satisfactory

Awards

31.1555 USDC - $31.16

Labels

bug
grade-b
QA (Quality Assurance)
Q-23

External Links

Use _disableInitializers() in Escher721 contract

The Escher721 contract is used by cloning the implementation. Consider adding the _disableInitializers() call in the constructor as precautionary measure.

Remove _burn override in Escher721 contract

The contract overrides the _burn function only to call super. Consider removing this unneeded override. Project will compile fine with Solidity 0.8.17 and all tests pass if this override is removed.

Rename Start event used in sales contracts

Consider renaming the Start event for the Sales contract since technically sales have a "start time" that can be in the future and the event is emitted at creation time.

Validate sale parameters in FixedPriceFactory contract

In createFixedSale function:

  • Validate sale.price > 0
  • Validate sale.saleReceiver != address(0)

Invalid initialization of storage variable amountSold in LPDA contract

The amountSold variable is initialized during the variable definition uint48 public amountSold = 0;. This is equivalent to using the constructor, which in this case is invalid since this contract is created using clones.

Assigning this a low severity since the assigned value is 0 which matches the default value and the issue shouldn't have a negative effect. However, consider removing this initialization.

Add a getter for sale endTime in LPDA contract

Add a getter to expose the endTime value of the sale.

function endTime() public view returns (uint256) {
    return sale.endTime;
}

Buy event is emitted with old data in OpenEdition.buy function

The Buy event is emitted using the temp variable which has the old sale.currentId (value is updated in line 69 directly to storage).

Validate sale parameters in OpenEditionFactory contract

In createOpenEdition function:

  • Validate sale.price > 0
  • Validate sale.saleReceiver != address(0)

Fee beneficiary can DoS and block the sale end process

The sale contracts uses the transfer function to distribute fees to the fee receiver defined in its factory contract. If the fee receiver is a contract which executes some logic on receive or fallback, it is possible that the call fails due to gas costs (transfer calls are bounded to 2300 units of gas) or directly by a revert on the call. If the transfer call fails, then the whole procedure will revert.

Reporting this as a low severity because fee receiver address will likely be in control of the protocol owner and can eventually be updated in the factory contracts.

#0 - c4-judge

2023-01-04T10:41:04Z

berndartmueller marked the issue as grade-b

Findings Information

Awards

35.0246 USDC - $35.02

Labels

bug
G (Gas Optimization)
grade-b
sponsor disputed
G-12

External Links

State is stored by duplicate in Escher contract

The Escher contract is an ERC1155 NFT that represents roles (admin, creator, curator) as soulbound NFTs. However, this contract also inherits from OZ's AccessControl which also tracks roles using storage. When an account gets a role, it is minted an NFT and also granted the role using the AccessControl mechanism.

This means that state is duplicated, each account that has a role in the Escher contract will have a representation as an NFT (_balances mapping in OZ ERC1155) and as a role (_roles mapping in OZ AccessControl).

One of these is unneeded. For example, this can be implemented by using just the ERC1155 representation. The IAccessControl interface could be implemented by minting the NFT to grant a role, burning the NFT to revoke a role, and checking the NFT balance to implement the "has role" behavior.

Unneeded sale variable constructor initialization in Sales contract

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPrice.sol#L46 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L53 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEdition.sol#L45

Each of the three sales contracts (FixedPrice, LPDA and OpenEdition) will initialize the sale variable in the constructor. This is not needed as these contracts are used with cloned implementations (rest of the body of the constructor is ok).

sale variable is re-read from storage when ending the sale in the FixedPrice contract

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

The sale variable is re-read from storage (previously read at line 58) when the sale is ended during the buy process.

Optimize struct fields in LPDA.Sale

Timestamps (startTime and endTime) can be defined as uint48 to save one storage slot. Using uint48 would allow a precision up to the year 8921556.

struct Sale {
    // slot 1
    uint48 currentId;
    uint48 finalId;
    address edition;
    // slot 2
    uint80 startPrice;
    uint80 finalPrice;
    uint80 dropPerSecond;
    // slot 3
    uint48 startTime;
    uint48 endTime;
    address payable saleReceiver;
}

Optimize struct fields in OpenEdition.Sale

Similarly, timestamps here (startTime and endTime) can be defined as uint48 to save one storage slot. Using uint48 would allow a precision up to the year 8921556.

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

#0 - c4-sponsor

2022-12-22T22:50:24Z

mehtaculous marked the issue as sponsor disputed

#1 - c4-judge

2023-01-04T10:58:51Z

berndartmueller marked the issue as grade-b

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