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: 8/119
Findings: 7
Award: $1,010.70
🌟 Selected for report: 3
🚀 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
1.0936 USDC - $1.09
https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L117-L124
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.
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.
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(); } }
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
🌟 Selected for report: AkshaySrivastav
Also found by: 0x52, 0xA5DF, 0xdeadbeef0x, KingNFT, Madalad, Parth, Soosh, _Adam, adriro, csanuragjain, danyams, eyexploit, gasperpre, gz627, gzeon, hansfriese, hihen, immeas, jadezti, jonatascm, kiki_dev, kree-dotcom, ladboy233, lukris02, lumoswiz, mahdikarimi, minhtrng, nalus, nameruse, obront, reassor, rvierdiiev, seyni, tnevler, wait, yixxas
1.3417 USDC - $1.34
https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L81-L87
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.
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.
LPDAFactory.createLPDASale
with startTime = t1
and endTime = t2
.t1
to t2
not all NFTs from the collection are sold. Meaning that at time t2
, sale.currentId < sale.finalId
.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.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
🌟 Selected for report: adriro
Also found by: 0xA5DF, 0xRobocop, AkshaySrivastav, Ch_301, HollaDieWaldfee, KingNFT, bin2chen, carrotsmuggler, cccz, gasperpre, hansfriese, hihen, jonatascm, lumoswiz, neumo
36.8641 USDC - $36.86
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
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:
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"); ...
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.
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(); } }
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
🌟 Selected for report: adriro
Also found by: 0xA5DF, HollaDieWaldfee
773.4814 USDC - $773.48
https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEdition.sol#L75
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.
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.
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(); } }
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
🌟 Selected for report: ForkEth
Also found by: CRYP70, Ch_301, Chom, Lambda, adriro, csanuragjain, minhquanym
131.7499 USDC - $131.75
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
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.
A buyer is allowed to still call the buy
method after the sale has ended.
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(); } }
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
🌟 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
31.1555 USDC - $31.16
_disableInitializers()
in Escher721
contractThe Escher721
contract is used by cloning the implementation. Consider adding the _disableInitializers()
call in the constructor as precautionary measure.
_burn
override in Escher721
contractThe 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.
Start
event used in sales contractsConsider 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.
FixedPriceFactory
contractIn createFixedSale
function:
sale.price > 0
sale.saleReceiver != address(0)
amountSold
in LPDA
contractThe 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.
endTime
in LPDA
contractAdd 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
functionThe Buy
event is emitted using the temp
variable which has the old sale.currentId
(value is updated in line 69 directly to storage).
OpenEditionFactory
contractIn createOpenEdition
function:
sale.price > 0
sale.saleReceiver != address(0)
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
🌟 Selected for report: slvDev
Also found by: 0x4non, Bnke0x0, Diana, Dinesh11G, RaymondFam, ReyAdmirado, adriro, ahmedov, ajtra, c3phas, cryptostellar5, nicobevi, pfapostol, sakshamguruji, tnevler, zaskoh
35.0246 USDC - $35.02
Escher
contractThe 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.
sale
variable constructor initialization in Sales contracthttps://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
contracthttps://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.
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; }
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