Escher contest - obront'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

Vulnerability details

Impact

LPDA auctions are run using the getPrice() function, which calculates the current price based on the formula:

price = startPrice - (timeElapsed * dropPerSecond);

However, there is no edge case handling for the situation where startPrice > (timeElapsed * dropPerSecond). As a result, if the dropPerSecond variable is set too large, the auction will eventually start to revert on getPrice().

If this happens before a mint sells out, it will cause buy() to revert. This means that the contract will never finish selling out. Since there is no way to remove funds from a non-completed auction, all funds will be permanently locked in the contract.

It will additionally cause refund() to revert, so that users who bought so far will not be able to redeem their overpayment as intended.

Proof of Concept

• An LPDA auction is started with startTime = x, endTime = x + 20, dropPerSecond = 10, startPrice = 100
• getPrice() is calculated as 100 - (10 * (endTime - startTime))
• After 10 seconds have passed, this value will underflow and getPrice() will begin to revert
• With getPrice() reverting, buy() cannot be called
• Without all NFTs being minted, there is no way for the creator to withdraw funds, and all funds from mints so far will remain stuck in the contract
• getPrice() will also cause refund() to revert, causing similar problems

Tools Used

Manual Review

Recommended Mitigation Steps

Two possible safety precautions:

Option 1: Add a check in LPDAFactory.sol#createLPDASale() that ensures that the dropPerSecond isn't too great and won't cause a price underflow:

require(sale.startPrice - (sale.dropPerSecond * (sale.endTime - sale.StartTime)) > 0);

Option 2: Manually check for this case in the getPrice() function so that it returns a 0 in the case that the drops per second bring the price down lower than zero:

uint priceDiscount = temp.dropPerSecond * timeElapsed;
if (temp.startPrice > priceDiscount) {
  return temp.startPrice - priceDiscount;
} else {
  return 0
}

Lines of code

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

Vulnerability details

Impact

The documentation describes the ability to set the royalties on a token-by-token basis using the setTokenRoyalty function. However, this function doesn't exist in the contract.

Proof of Concept

In the docs, it explains that the flow for setting up a sale includes:

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:

• id: the token ID of the sale
• receiver: the address to receive paymnts
• feeNumerator: the desired royalty %

However, this function does not exist. While the inherited ERC2981.sol file does have a _setTokenRoyalty() internal function, Escher has not implemented any external function to access it.

Instead, it implements a setDefaultRoyalty() function, which doesn't have the same granularity laid out in the docs.

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a setTokenRoyalty() function in Escher721.sol that calls the _setTokenRoyalty() internal function from ERC2981.sol.

Lines of code

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

Vulnerability details

Impact

In LPDA.sol, the refund() function calculates the difference between what a user spent in the auction and their actual cost (calculated as final price * amount). It then refunds the difference to the user.

This refund is performed with a .transfer call that doesn't allow the user to specify the address. In the event that the original transactions came from a smart contract that uses more than 2300 gas in its receive() or fallback() function, the refund will revert and the user will not be able to access their fees.

Proof of Concept

• User calls buy() with 50 ETH for 5 NFTs, expecting to get a refund
• receipts[msg.sender] is set with their quantity and balance
• The final NFT sells for 5 ETH
• The user's refund should (10-5) * 5 = 25 ETH
• If the user's initial transaction came from a smart contract that can't receive ETH without using more than 2300 gas, the .transfer call will fail
• The user will have no alternative way to access their funds, because they are only able to withdraw them to this contract address, which is immutable

Tools Used

Manual Review

Recommended Mitigation Steps

Two options:

1. Allow a to input to the refund() function, which would allow users to send their refund to another address if they aren't able to accept it.
2. Use a low level call to send the refund, so that the hard gas cap doesn't apply.

Lines of code

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

Vulnerability details

Impact

In an LPDA auction, the price falls from startPrice down to startPrice - (dropPerSecond * (endTime - startTime). At this point, the auction continues, but the price no longer falls.

If the auction stops at a price that doesn't clear the market, it'll remain open indefinitely.

But, as long as the auction remains open, the owners are not able to withdraw their funds and the feeReceiver doesn't receive their fees.

The result is that an NFT collection can be mostly minted and active, but the owners are not able to collect their earnings to fund the project.

Proof of Concept

The only place in the LPDA.sol contract where funds are transferred to the feeReceiver and saleReceiver are in the final block of the buy() function:

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

For this block to execute, the NFTs need to be minted up to the point where newId == sale.finalId. If this doesn't happen, this block can never execute.

There are only two ways for this to happen:

1. cancel() is called, which can only happen before a sale starts.
2. Enough NFTs are minted that currentId increases to the point that currentId + amount reaches finalId.

If the latter doesn't happen, the funds will be stuck.

[Note that this doesn't impact refunds, as users will be able to call refund() at any time to get their refund up to the current price.]

Tools Used

Manual Review

Recommended Mitigation Steps

Add an end() function that can be called after the endTime to finalize the LPDA and process the payouts.

Lines of code

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

Vulnerability details

Impact

After an LPDA auction is ended, the final price is locked in as follows:

sale.finalPrice = uint80(price);

This unsafe downcasting casts price into an 80 bit integer. This may be sufficient for most situations, but only represents 1.208MM ETH, which is not out of the question for how much an NFT might one day sell for.

Proof of Concept

If an NFT were to sell for 1.209MM ETH:

• price = 1209 * 10 ** 18;
• sale.finalPrice = uint80(price);
• because of the unsafe downcasting, this value overflows to 74 ETH
• refund values are set based on this finalPrice, which is inaccurate

Tools Used

Manual Review

Recommended Mitigation Steps

Use safe downcasting by checking that the value of price is less than the max uint80 value before downcasting:

require(price <= type(uint80).max, "CANT DOWNCAST");

This option would stop any NFTs from being purchased above this price. A simplier solution might be to rearrange the storage struct so that finalPrice is sized at uin96, which would fit within the existing slot and not cost any extra gas.

Lines of code

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

Vulnerability details

Impact

The selfdestruct opcode may be phased out. This possibility is discussed in EIP4760 (written by Vitalik and Dankrad).

This opcode is used to send Eth to the saleReceiver, which risks changes to the network invalidating the immutable implementation contracts deployed by Escher.

Proof of Concept

The _end() function in both OpenEdition.sol and FixedPrice.sol both use selfdestruct() to send their remaining funds to the saleReceiver.

Tools Used

Manual Review

Recommended Mitigation Steps

Use a low level call to transfer funds to the saleReceiver instead of self destructing.