Escher contest - Soosh's results

Lines of code

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

Vulnerability details

The only way to end a sale when it has already started is if all the NFTs have been bought (minted). The only way the NFT creator gets paid from the sale is if the sale ends.

End sale condition example in FixedPrice.sol

if (newId == sale_.finalId) _end(sale);

This means that all NFTs must be bought or the creator will not be able to receive any payment. The creator must either wait for all NFTs to be bought (which could be forever), or buy their own NFTs to end (which requires capital they may not have).

Impact

Creator cannot receive payment for NFTs that sold unless all NFTs have been sold.

I believe it is an incorrect assumption that all NFT projects will be popular and sold out fully. The common case will be that creator will be unable to collect payment for sold NFTs (loss of funds).

Recommendations

Should reconsider the current design to instead allow the creator to receive payment of the NFTs that were already sold.

The current design of all sales contract is that payment is only made once to the creator, when the sale has ended. If this is desired, then FixedPrice and LPDA contracts could follow the OpenEdition contract way of ending the sale after a specific deadline. So even if not all NFTs are sold, the creator can end the sale and receive payment.

Lines of code

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

Vulnerability details

Use of selfdestruct() is deprecated: https://eips.ethereum.org/EIPS/eip-6049. Vitalik's post: https://hackmd.io/@vbuterin/selfdestruct

A breaking change is expected where SELFDESTRUCT will be removed some time in the future. This will break the contracts functionality and cause the NFT creator to not be able to receive payment.

Impact

The protocol will be broken when the breaking change is implemented, it may result in creator being unable to receive payment since selfdestruct() is used.

Recommendation

Use call() instead to send ether.