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
Rank: 7/119
Findings: 7
Award: $1,530.63
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: adriro
Also found by: 0x446576, 0xA5DF, 0xDave, 0xDecorativePineapple, 0xRobocop, 0xbepresent, 8olidity, Aymen0909, Ch_301, Chom, Franfran, HollaDieWaldfee, Madalad, Parth, Ruhum, Tricko, bin2chen, carrotsmuggler, chaduke, danyams, evan, gz627, hansfriese, hihen, imare, immeas, jadezti, jayphbee, jonatascm, kaliberpoziomka8552, kiki_dev, kree-dotcom, ladboy233, lukris02, lumoswiz, mahdikarimi, minhquanym, minhtrng, nameruse, neumo, obront, pauliax, poirots, reassor, rvierdiiev, slvDev, sorrynotsorry, yixxas, zapaz
0.8413 USDC - $0.84
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
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.
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.
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
🌟 Selected for report: immeas
Also found by: 0x52, 0xDecorativePineapple, AkshaySrivastav, HollaDieWaldfee, aviggiano, bin2chen, cryptonue, evan, ey88, fs0c, gzeon, hansfriese, hihen, jayphbee, minhquanym, pauliax, reassor, saian, wait
49.6134 USDC - $49.61
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:
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
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.
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:
LPDA.buy
himself (with sufficient msg.value) and withdraws the refundsfunction 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); }
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
🌟 Selected for report: HollaDieWaldfee
Also found by: 0xNazgul, cccz, hansfriese, obront
375.912 USDC - $375.91
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.
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.
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
🌟 Selected for report: RaymondFam
Also found by: 0xdeadbeef0x, 0xhacksmithh, AkshaySrivastav, Awesome, Bnke0x0, CRYP70, HollaDieWaldfee, JC, Parth, Rahoz, Tutturu, __141345__, ahmedov, ajtra, asgeir, aviggiano, bin2chen, btk, carrotsmuggler, cccz, chaduke, cryptonue, dic0de, fatherOfBlocks, fs0c, hansfriese, jonatascm, karanctf, ladboy233, lumoswiz, martin, obront, pashov, pauliax, rvierdiiev, shark, simon135, supernova, tourist, yellowBirdy, zapaz, zaskoh
0.6136 USDC - $0.61
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
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:
transfer
may break code that runs successfully nowFor 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
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.
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
🌟 Selected for report: adriro
Also found by: 0xA5DF, 0xRobocop, AkshaySrivastav, Ch_301, HollaDieWaldfee, KingNFT, bin2chen, carrotsmuggler, cccz, gasperpre, hansfriese, hihen, jonatascm, lumoswiz, neumo
28.357 USDC - $28.36
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
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.
FixedPrice
sale with 10 tokens for salestartTime
has passed which means that the artist cannot call cancel
anymore.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
🌟 Selected for report: adriro
Also found by: 0xA5DF, HollaDieWaldfee
594.9857 USDC - $594.99
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
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
).
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.
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
🌟 Selected for report: 0xSmartContract
Also found by: 0x1f8b, 0x4non, 0xA5DF, 0xNazgul, 0xRobocop, 0xfuje, Bnke0x0, HollaDieWaldfee, Lambda, RaymondFam, TomJ, _Adam, adriro, ajtra, carrotsmuggler, cccz, danyams, gasperpre, hansfriese, helios, immeas, joestakey, ladboy233, nameruse, obront, oyc_109, rvierdiiev, saian, sakshamguruji, seyni, slvDev, yixxas, zaskoh
480.3105 USDC - $480.31
Risk | Title | File | Instances |
---|---|---|---|
L-00 | Spamming Buy events by buying 0 NFTs | - | 3 |
L-01 | Do not use selfdestruct to transfer ETH | - | 2 |
L-02 | Use safeMint instead of mint | - | 3 |
L-03 | Missing zero address checks | - | 6 |
L-04 | openzeppelin Ownable does not use 2-Step-Process to transfer ownership | - | 7 |
L-05 | Missing sanity checks when creating Sales | - | 2 |
L-06 | NFTs with tokenId 0 cannot be sold | - | 3 |
N-00 | Unlocked pragma | - | 12 |
Buy
events by buying 0
NFTsThe 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
selfdestruct
to transfer ETHOpenEdition._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:
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
And the Escher721
contract must also make safeMint
accessible to addresses with the MINTER_ROLE
.
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
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.
Ownable
does not use 2-Step-Process to transfer ownershipMany 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:
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).
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
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
#0 - c4-judge
2023-01-04T10:53:20Z
berndartmueller marked the issue as grade-a