Escher contest - slvDev'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: 11/119

Findings: 4

Award: $640.51

QA:
grade-b
Gas:
grade-a

🌟 Selected for report: 1

🚀 Solo Findings: 0

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L117-L125 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDAFactory.sol#L29-L42

Vulnerability details

Impact

LPDA Sales can start reverting the buy() and refund() functions at some point of time if initialized with incorrect parameters (revert happens at getPrice() function).

Users might not be able to withdraw their excess balance using refund() function if getPrice() reverts.

Users will not be able to Buy if getPrice() reverts.

getPrice() will revert if startPrice is less than dropPerSecond * (endTime-startTime)

Proof of Concept

file: src/minters/LPDA.sol

startPrice = 100
dropPersecond = 10
endTime = 100
startTime = 0

// getPrice() function uses in buy() and refund() 
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);
}

Add a require(sale.startPrice >= sale.dropPerSecond * (sale.endTime - sale.startTime)); after all require statements in createLPDASale()

file: src/minters/LPDAFactory.sol

function createLPDASale(LPDA.Sale calldata sale) external returns (address clone) {
    ...
		require(sale.dropPerSecond > 0, "INVALID DROP PER SECOND");
+		require(sale.startPrice >= sale.dropPerSecond * (sale.endTime - sale.startTime), "Incorrect LPDA parameters");

		...
}

#0 - c4-judge

2022-12-11T11:36:28Z

berndartmueller marked the issue as duplicate of #392

#1 - c4-judge

2023-01-02T19:53:17Z

berndartmueller marked the issue as satisfactory

#2 - c4-judge

2023-01-02T19:53:22Z

berndartmueller changed the severity to 3 (High Risk)

Findings Information

🌟 Selected for report: ladboy233

Also found by: 0x1f8b, Matin, UniversalCrypto, gzeon, karanctf, minhquanym, obront, rvierdiiev, seyni, slvDev, yixxas

Labels

bug
2 (Med Risk)
satisfactory
duplicate-369

Awards

57.6274 USDC - $57.63

External Links

Lines of code

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

Vulnerability details

Impact

If user calls buy(amount) with a value larger than uint48 it will overflow during casting (identical to amount = _amount % type(uint48).max) and cause unexpected behavior for the user. When a user pays a lot of money for a large amount of NFT, but instead they will get a small amount of NFTs.

User will still be able to withdraw their extra balance using refund() function later.

Proof of Concept

file: src/minters/LPDA.sol

buy(type(uint48).max + 2); // 281474976710655 + 2 = 281474976710657
										
function buy(uint256 _amount) external payable {
//                      ^ 281474976710657
    uint48 amount = uint48(_amount); // <<- uint48(281474976710657) = 1
	  ...
    require(msg.value >= amount * price, "WRONG PRICE");
//		  ^ msg.value will be bigger than 1 * price
		...
    uint48 newId = amount + temp.currentId;
//           ^ increment by 1
    require(newId <= temp.finalId, "TOO MANY");

    receipts[msg.sender].amount += amount;
    receipts[msg.sender].balance += uint80(msg.value);

    for (uint256 x = temp.currentId + 1; x <= newId; x++) {
        nft.mint(msg.sender, x);
//	    ^ mint only 1 NFT
    }
    ...
}

Tools Used

Manual audit

Add a require(_amount <= type(uint48).max) in the first line of the function.

It will revert the transaction when too big value is passed as _amount.

file: src/minters/LPDA.sol

function buy(uint256 _amount) external payable {
+		require(_amount <= type(uint48).max, "Amount too big");
		uint48 amount = uint48(_amount);
		...
}

#0 - c4-judge

2022-12-10T17:09:55Z

berndartmueller marked the issue as duplicate of #369

#1 - c4-judge

2023-01-03T13:50:41Z

berndartmueller marked the issue as satisfactory

Awards

31.1555 USDC - $31.16

Labels

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

External Links

IssueInstances
[L-01]Different emit event behavior in buy() functions2
[N-01]Non-library/interface files should use fixed compiler versions, not floating ones.4
[N-02]Function order is incorrect.3
[N-03]Internal function should use prefix _1
[N-04]_ in front of function parameters.9
[N-05]Missing NatSpec comments44
[N-06]Missing NatSpec comments for Function parameters3
[N-07]Wrong NatSpec @notice for buy() function1

Low-Risk

[L-01] Different emit event behavior in buy() functions

In LPDA.sol and OpenEdition.sol buy() function emit Buy event with the cached version of Sale object with the old currentId (before minting). While in FixedPrice.sol buy() emit Buy event with Sale object from storage with changed currentId to newId.

file: src/minters/FixedPrice.sol

function buy(uint256 _amount) external payable {
		...
    emit Buy(msg.sender, _amount, msg.value, sale); <<- read sale from storage
		...
}
file: src/minters/LPDA.sol
&
file: src/minters/OpenEdition.sol

function buy(uint256 _amount) external payable {
		...
    Sale memory temp = sale; <<- Cache sale to temp
		...
    sale.currentId = newId; <<- Store newId to storage
		...
    emit Buy(msg.sender, amount, msg.value, temp); <<- Emit event with cached sale
		...
}

Non-Critical Issues

[N-01] Non-library/interface files should use fixed compiler versions, not floating ones.

file: src/interfaces/IEscher721.sol

- pragma solidity ^0.8.17;
+ pragma solidity 0.8.17;
file: src/interfaces/ISale.sol

- pragma solidity ^0.8.17;
+ pragma solidity 0.8.17;
file: src/interfaces/ISaleFactory.sol

- pragma solidity ^0.8.17;
+ pragma solidity 0.8.17;
file: src/interfaces/ITokenUriDelegate.sol

- pragma solidity ^0.8.17;
+ pragma solidity 0.8.17;

[N-02] Function order is incorrect, struct definition can not go after state variable declaration

file: src/minters/FixedPrice.sol

- address public immutable factory;
	/// store nextId and remainingSupply, where nextId increases and remainingSupply decreases to 0
	/// avoids strict equality of current == final
	struct Sale {
	    ...
	}
+ address public immutable factory;
file: src/minters/LPDA.sol

- address public immutable factory;
	/// we use different uints and some weird ordering to pack variables into 32 bytes
	struct Sale {
		...
	}
	struct Receipt {
		...
  }
+ address public immutable factory;
file: src/minters/OpenEdition.sol

- address public immutable factory;
	/// we use different uints and some weird ordering to pack variables into 32 bytes
	struct Sale {
	    ...
	}
+ address public immutable factory;

[N-03] Internal function should use prefix _

Using an underscore in front of function parameters in Solidity is a convention that can help to make your code more readable and maintainable.

file: src/uris/Generative.sol

- function tokenToSeed(uint256 _tokenId) internal view returns (bytes32) {
+ function _tokenToSeed(uint256 _tokenId) internal view returns (bytes32) {

[N-04] _ in front of function parameters.

Using an underscore in front of function parameters in Solidity is a convention that can help to make your code more readable and maintainable.

file: src/Escher721.sol

- function mint(address to, uint256 tokenId) public virtual onlyRole(MINTER_ROLE) {
+ function mint(address _to, uint256 _tokenId) public virtual onlyRole(MINTER_ROLE) {

- function setDefaultRoyalty(address receiver, uint96 feeNumerator) public onlyRole(DEFAULT_ADMIN_ROLE) {
+ function setDefaultRoyalty(address _receiver, uint96 _feeNumerator) public onlyRole(DEFAULT_ADMIN_ROLE) {

- function supportsInterface(bytes4 interfaceId) ...
+ function supportsInterface(bytes4 _interfaceId) ...

- function _burn(uint256 tokenId) internal override(ERC721Upgradeable) {
+ function _burn(uint256 _tokenId) internal override(ERC721Upgradeable) {
file: src/minters/FixedPriceFactory.sol

- function setFeeReceiver(address payable fees) public onlyOwner {
+ function setFeeReceiver(address payable _fees) public onlyOwner {
file: src/minters/LPDAFactory.sol

- function createLPDASale(LPDA.Sale calldata sale) external returns (address clone) {
+ function createLPDASale(LPDA.Sale calldata _sale) external returns (address clone) {

- function setFeeReceiver(address payable fees) public onlyOwner {
+ function setFeeReceiver(address payable _fees) public onlyOwner {
file: src/minters/OpenEditionFactory.sol

- function createOpenEdition(OpenEdition.Sale calldata sale) external returns (address clone) {
+ function createOpenEdition(OpenEdition.Sale calldata _sale) external returns (address clone) {

- function setFeeReceiver(address payable fees) public onlyOwner {
+ function setFeeReceiver(address payable _fees) public onlyOwner {

[N-05] Fully missing NatSpec comments

NatSpec comments are used to provide detailed documentation for a Solidity smart contract. This includes information about the contract's functions, variables, and overall purpose. This allows other developers to understand the contract's intended functionality and use it correctly.

file: src/Escher.sol

function supportsInterface(bytes4 _interfaceId)
file: src/Escher721.sol

contract Escher721 is Initializable, ERC721Upgradeable, AccessControlUpgradeable, ERC2981Upgradeable {
function supportsInterface(bytes4 interfaceId)
function _burn(uint256 tokenId) internal override(ERC721Upgradeable) {
file: src/Escher721Factory.sol

contract Escher721Factory {
Escher public immutable escher;
address public immutable implementation;
event NewEscher721Contract(address indexed creator, address indexed clone, address indexed uri);
file: src/uris/Base.sol

contract Base is ITokenUriDelegate, OwnableUpgradeable {
function setBaseURI(string memory _baseURI) external onlyOwner {
function tokenURI(uint256) external view virtual returns (string memory) {
function initialize(address _owner) public initializer {
file: src/uris/Generative.sol

contract Generative is Unique {
bytes32 public seedBase;
function setSeedBase() external onlyOwner {
function tokenToSeed(uint256 _tokenId) internal view returns (bytes32) {
file: src/uris/Unique.sol

contract Unique is Base {
function tokenURI(uint256 _tokenId) external view virtual override returns (string memory) {
file: src/minters/FixedPrice.sol

contract FixedPrice is Initializable, OwnableUpgradeable, ISale {
address public immutable factory;
file: src/minters/FixedPriceFactory.sol

contract FixedPriceFactory is Ownable {
address payable public feeReceiver;
address public immutable implementation;
event NewFixedPriceContract(address indexed creator, address indexed edition, address indexed saleContract, FixedPrice.Sale saleInfo);
file: src/minters/LPDA.sol

contract LPDA is Initializable, OwnableUpgradeable, ISale {
address public immutable factory;
uint48 public amountSold = 0;
event Start(Sale _saleInfo);
event End(Sale _saleInfo);
event Buy(address indexed _buyer, uint256 _amount, uint256 _value, Sale _saleInfo);
function lowestPrice() public view returns (uint256) {
function _end() internal {
file: src/minters/LPDAFactory.sol

contract LPDAFactory is Ownable {
address payable public feeReceiver;
address public immutable implementation;
event NewLPDAContract(address indexed _creator, address indexed _edition, address indexed _saleContract, LPDA.Sale _saleInfo);
file: src/minters/OpenEdition.sol

contract OpenEdition is Initializable, OwnableUpgradeable, ISale {
address public immutable factory;
function available() public view returns (uint256) {
function _end(Sale memory _sale) internal {
file: src/minters/OpenEditionFactory.sol

contract OpenEditionFactory is Ownable {
address payable public feeReceiver;
address public immutable implementation;
event NewOpenEditionContract(address indexed creator, address indexed edition, address indexed saleContract, OpenEdition.Sale saleInfo);

[N-06] Missing NatSpec comments for Function parameters

NatSpec comments are used to provide detailed documentation for a Solidity smart contract. This includes information about the contract's functions, variables, and overall purpose. This allows other developers to understand the contract's intended functionality and use it correctly.

file: src/Escher.sol

function _grantRole(bytes32 _role, address _account) internal override {
function _revokeRole(bytes32 _role, address _account) internal override {
file: src/Escher721.sol

function _burn(uint256 tokenId) internal override(ERC721Upgradeable) {

[N-07] Wrong NatSpec @notice for buy() function

Wrong @notice for buy() function since in LPDA.sol msg.value not fixed.

file: src/minters/LPDA.sol

/// @notice buy from a fixed price sale after the sale starts
/// @param _amount the amount of editions to buy
function buy(uint256 _amount) external payable {

#0 - c4-judge

2023-01-04T10:37:55Z

berndartmueller marked the issue as grade-b

Findings Information

Awards

550.8787 USDC - $550.88

Labels

bug
G (Gas Optimization)
grade-a
selected for report
G-17

External Links

IssueInstancesSaved GasOverall gas change from forge snapshot --diff
[G-01]Refactor Sale struct to avoid using unnecessary storage slot373328972780
[G-02]Cache repeated parameters from Sale in buy() function240910066
[G-03]Use cached value from memory rather than read storage11151380
[G-04]Removing cache Sale to memory saves gas12546775
[G-05]Increment in the for loop’s postcondition can be made unchecked{++i}/unchecked{i++}32155855
[G-06]Simplify / Refactor _end function23255460
[G-07]Change for loop behavior by removing add (+1) and ++x is more gas efficient33044025
[G-08]<x> += <y> cost more gas than <x> = <x> + <y>142756

Gas Report

Total gas saved: 74992

Overall gas change from forge snapshot: 1002920

test_RevertsWhenEnded_Buy() (gas: -26155 (-3.807%)) 
test_SellsOut_Buy() (gas: -26157 (-3.824%)) 
test_LPDA() (gas: -26988 (-3.850%)) 
test_RevertsWhenSoldOut_Buy() (gas: -27100 (-3.887%)) 
test_RevertsWhenMintedOut_Buy() (gas: -26564 (-4.306%)) 
test_WhenMintsOut_Buy() (gas: -26256 (-4.318%)) 
test_Refund() (gas: -25377 (-6.436%)) 
test_WhenNotOver_Refund() (gas: -25377 (-6.436%)) 
test_RevertsWhenAlreadyRefunded_Refund() (gas: -25731 (-6.472%)) 
test_RevertsWhenTooSoon_Buy() (gas: -25463 (-6.477%)) 
test_RevertsWhenTooLate_Cancel() (gas: -25002 (-6.481%)) 
test_RevertsWhenNotOwner_Cancel() (gas: -25007 (-6.483%)) 
test_RevertsWhenNoRefund_Refund() (gas: -25377 (-6.507%)) 
test_Buy() (gas: -25007 (-6.560%)) 
test_RevertsWhenTooLittleValue_Buy() (gas: -25818 (-6.667%)) 
test_WhenEnded_Finalize() (gas: -24907 (-7.069%)) 
test_RevertsWhenNotAdmin_CreateSale() (gas: -2025 (-7.794%)) 
test_RevertsWhenTooSoon_Buy() (gas: -24797 (-7.880%)) 
test_RevertsWhenEnded_Buy() (gas: -24813 (-7.886%)) 
test_RevertsTooMuchValue_Buy() (gas: -24924 (-7.926%)) 
test_RevertsTooMuchValue_Buy() (gas: -25137 (-8.003%)) 
test_RevertsWhenTooMany_Buy() (gas: -25155 (-8.006%)) 
test_RevertsWhenNotOwner_TransferOwnership() (gas: -24708 (-8.010%)) 
test_RevertsWhenTooSoon_Buy() (gas: -25239 (-8.025%)) 
test_RevertsWhenNotOwner_TransferOwnership() (gas: -24758 (-8.031%)) 
test_RevertsWhenNotOwner_Cancel() (gas: -24708 (-8.035%)) 
test_RevertsWhenTooLate_Cancel() (gas: -24720 (-8.040%)) 
test_RevertsWhenTooLate_Cancel() (gas: -24864 (-8.091%)) 
test_RevertsWhenTooLittleValue_Buy() (gas: -24924 (-8.096%)) 
test_RevertsWhenNotOwner_Cancel() (gas: -24869 (-8.104%)) 
test_Buy() (gas: -24708 (-8.156%)) 
test_RevertsWhenNotEnded_Finalize() (gas: -25144 (-8.162%)) 
test_RevertsWhenTooLittleValue_Buy() (gas: -25137 (-8.176%)) 
test_Buy() (gas: -24847 (-8.207%)) 
test_RevertsWhenNotAdmin_CreateSale() (gas: -2078 (-8.743%)) 
test_RevertsWhenNotAdmin_CreateSale() (gas: -2093 (-8.814%)) 
test_Cancel() (gas: -24400 (-9.382%)) 
test_Cancel() (gas: -24208 (-9.592%)) 
test_Cancel() (gas: -24699 (-10.928%)) 
test_RevertsWhenInitialized_Initialize() (gas: -1971 (-10.952%)) 
test_CreateSale() (gas: -24216 (-11.069%)) 
test_RevertsWhenInitialized_Initialize() (gas: -2137 (-11.845%)) 
test_CreateSale() (gas: -24277 (-12.900%)) 
test_CreateSale() (gas: -24316 (-12.913%)) 
test_RevertsWhenInitialized_Buy() (gas: -2089 (-13.919%)) 
test_RevertsWhenInitialized_Buy() (gas: -4392 (-29.262%)) 
test_RevertsWhenInitialized_Finalize() (gas: -4414 (-29.460%)) 
Overall gas change: -1002920 (-413.087%)

[G-01] Refactor Sale struct to avoid using unnecessary storage slot

We can use uint32 for time variables instead of uint96 since it will be enough until the year 2106 but can save a decent amount of gas.

file: src/minters/LPDA.sol

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

test_Buy() (gas: -24451 (-6.415%))

Overall gas change: -370656 (-99.469%)

Same here, we can use uint32 for the time variables

file: src/minters/OpenEdition.sol

struct Sale {
	  // slot 1
	  uint72 price;
	  uint24 currentId;
	  address edition;
	  // slot 2
-	  uint96 startTime;
+	  uint32 startTime;
-   uint96 endTime;
+   uint32 endTime;
	  address payable saleReceiver;
-	  // slot 3
-	  uint96 endTime;
}

test_Buy() (gas: -24420 (-8.061%))

Overall gas change: -301789 (-149.642%)

In FixedPrice.sol we use uint32 for the startTime variable and uint32 for currentId and finalId . Since in OpenEdition.sol use uint24 for currentId

file: src/minters/FixedPrice.sol

struct Sale {
	  // slot 1
-	  uint48 currentId;
-	  uint48 finalId;
+   uint32 currentId;
+	  uint32 finalId;
+   uint32 startTime;
	  address edition;
	  // slot 2
	  uint96 price;
	  address payable saleReceiver;
-	  // slot 3
-	  uint96 startTime;
}

test_Buy() (gas: -24457 (-8.078%))

Overall gas change: -300344 (-126.975%)

[G-02] Cache repeated parameters from Sale in buy() function.

Instead of caching the entire Sale object, only cache the currentId and finalId parameters in the function. This will save on gas consumption and improve efficiency.

file: src/minters/FixedPrice.sol

function buy(uint256 _amount) external payable {
-        Sale memory sale_ = sale;
+        uint48 currentId = sale.currentId;
+        uint48 finalId = sale.finalId;
-        IEscher721 nft = IEscher721(sale.edition);
+        IEscher721 nft = IEscher721(sale.edition);
-        require(block.timestamp >= sale_.startTime, "TOO SOON");
+        require(block.timestamp >= sale.startTime, "TOO SOON");
-        require(_amount * sale_.price == msg.value, "WRONG PRICE");
+        require(_amount * sale.price == msg.value, "WRONG PRICE");
-        uint48 newId = uint48(_amount) + sale_.currentId;
+        uint48 newId = uint48(_amount) + currentId;
-        require(newId <= sale_.finalId, "TOO MANY");
+        require(newId <= finalId, "TOO MANY");

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

         sale.currentId = newId;

         emit Buy(msg.sender, _amount, msg.value, sale);

-        if (newId == sale_.finalId) _end(sale);
+        if (newId == finalId) _end(sale);
    }

test_Buy() (gas: -189 (-0.062%))

file: src/minters/LPDA.sol

function buy(uint256 _amount) external payable {
    uint48 amount = uint48(_amount);
-    Sale memory temp = sale;
+    uint48 currentId = sale.currentId;
+    uint48 finalId = sale.finalId;
-    IEscher721 nft = IEscher721(temp.edition);
+    IEscher721 nft = IEscher721(sale.edition);
-    require(block.timestamp >= temp.startTime, "TOO SOON");
+    require(block.timestamp >= sale.startTime, "TOO SOON");
     uint256 price = getPrice();
     require(msg.value >= amount * price, "WRONG PRICE");

     amountSold += amount;
-    uint48 newId = amount + temp.currentId;
+    uint48 newId = amount + currentId;
-    require(newId <= temp.finalId, "TOO MANY");
+    require(newId <= finalId, "TOO MANY");

     receipts[msg.sender].amount += amount;
     receipts[msg.sender].balance += uint80(msg.value);

-    for (uint256 x = temp.currentId + 1; x <= newId; x++) {
+    for (uint256 x = currentId + 1; x <= newId; x++) {
         nft.mint(msg.sender, x);
     }

     sale.currentId = newId;

-    emit Buy(msg.sender, amount, msg.value, temp);
+    emit Buy(msg.sender, amount, msg.value, sale);
-    if (newId == temp.finalId) {
+    if (newId == finalId) {
         sale.finalPrice = uint80(price);
         uint256 totalSale = price * amountSold;
         uint256 fee = totalSale / 20;
         ISaleFactory(factory).feeReceiver().transfer(fee);
-        temp.saleReceiver.transfer(totalSale - fee);
+        sale.saleReceiver.transfer(totalSale - fee);
         _end();
     }
}

test_Buy() (gas: -220 (-0.058%)) and test_LPDA() (gas: -319 (-0.046%))

[G-03] Use cached value from memory rather than read storage

file: src/minters/OpenEdition.sol

function buy(uint256 _amount) external payable {
		uint24 amount = uint24(_amount);
		Sale memory temp = sale;
		IEscher721 nft = IEscher721(temp.edition);
		require(block.timestamp >= temp.startTime, "TOO SOON");
		require(block.timestamp < temp.endTime, "TOO LATE");
-		require(amount * sale.price == msg.value, "WRONG PRICE");
+		require(amount * temp.price == msg.value, "WRONG PRICE");
		uint24 newId = amount + temp.currentId;
		
		for (uint24 x = temp.currentId + 1; x <= newId; x++) {
		    nft.mint(msg.sender, x);
		}
		sale.currentId = newId;
		
		emit Buy(msg.sender, amount, msg.value, temp);
}

test_Buy() (gas: -115 (-0.038%))

[G-04] Removing cache Sale to memory saves gas

it will save gas in buy() and refund() functions

file: src/minters/LPDA.sol

function getPrice() public view returns (uint256) {
-		Sale memory temp = sale;
-		(uint256 start, uint256 end) = (temp.startTime, temp.endTime);
+		(uint256 start, uint256 end) = (sale.startTime, sale.endTime);
		if (block.timestamp < start) return type(uint256).max;
-		if (temp.currentId == temp.finalId) return temp.finalPrice;
+		if (sale.currentId == sale.finalId) return temp.finalPrice;
		
-		uint256 timeElapsed = end > block.timestamp ? block.timestamp - start : end - start;
-		uint256 timeElapsed = end > block.timestamp ? block.timestamp - start : end - start;
-		return temp.startPrice - (temp.dropPerSecond * timeElapsed);
+   return sale.startPrice - (sale.dropPerSecond * timeElapsed);
}

test_Buy() (gas: -254 (-0.067%)) and test_Refund() (gas: -507 (-0.129%))

[G-05] Increment in the for loop’s post condition can be made unchecked

The newIdvariable is calculated using addition, which means it cannot overflow. (In Solidity version 0.8.0 and higher)

file: src/minters/FixedPrice.sol

function buy(uint256 _amount) external payable {
		...

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

test_Buy() (gas: -78 (-0.026%))

file: src/minters/LPDA.sol

function buy(uint256 _amount) external payable {
		...

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

test_Buy() (gas: -59 (-0.015%)) and test_LPDA() (gas: -590 (-0.084%))

file: src/minters/OpenEdition.sol

function buy(uint256 _amount) external payable {
		...

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

test_Buy() (gas: -78 (-0.026%))

[G-06] Simplify _end function

Also in OpenEdition.sol we can remove and unnecessary struct cache in finalize() function.

file: src/minters/OpenEdition.sol

function cancel() external onlyOwner {
    require(block.timestamp < sale.startTime, "TOO LATE");
-   _end(sale);
+   _end();
}

function finalize() public {
-   Sale memory temp = sale;
-   require(block.timestamp >= temp.endTime, "TOO SOON");
+   require(block.timestamp >= sale.endTime, "TOO SOON");
    ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);
-   _end(temp);
+   _end();
}

- function _end(Sale memory _sale) internal {
+ function _end() internal {
-	    emit End(_sale);
+	    emit End(sale);
-	    selfdestruct(_sale.saleReceiver);
+	    selfdestruct(sale.saleReceiver);
	}

test_Cancel() (gas: -304 (-0.135%))

In FixedPrice.sol refactor _end function save 21 gas.

file: src/minters/FixedPrice.sol

function cancel() external onlyOwner {
    require(block.timestamp < sale.startTime, "TOO LATE");
-   _end(sale);
+   _end();
}

function buy(uint256 _amount) external payable {
...
-		if (newId == sale_.finalId) _end(sale);
+		if (newId == sale_.finalId) _end();
}

- function _end(Sale memory _sale) internal {
+ function _end() internal {
+     Sale memory temp = sale;
-	    emit End(_sale);
+     emit End(temp);
	    ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);
-	    selfdestruct(_sale.saleReceiver);
+    selfdestruct(temp.saleReceiver);
	}

test_Cancel() (gas: -21 (-0.008%))

[G-07] Change for loop behavior by removing add (+1) and ++x is more gas efficient

file: src/minters/FixedPrice.sol

function buy(uint256 _amount) external payable {
				...

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

test_Buy() (gas: -105 (-0.035%))

file: src/minters/LPDA.sol

function buy(uint256 _amount) external payable {
				...

-       for (uint256 x = temp.currentId + 1; x <= newId; x++) {
+       for (uint256 x = temp.currentId; x < newId; ++x) {
            nft.mint(msg.sender, x);
        }

				...
    }

test_Buy() (gas: -97 (-0.025%)) and test_LPDA() (gas: -210 (-0.030%))

file: src/minters/OpenEdition.sol

function buy(uint256 _amount) external payable {
		...

-   for (uint24 x = temp.currentId + 1; x <= newId; x++) {
+   for (uint24 x = temp.currentId; x < newId; ++x) {
        nft.mint(msg.sender, x);
    }

		...
}

test_Buy() (gas: -102 (-0.034%))

[G-08] <x> += <y> cost more gas than <x> = <x> + <y>

file: src/minters/LPDA.sol

function buy(uint256 _amount) external payable {
		...
		require(msg.value >= amount * price, "WRONG PRICE");

-		amountSold += amount;
+		amountSold = amountSold + amount;
		uint48 newId = amount + temp.currentId;

		...
}

test_Buy() (gas: -42 (-0.011%))

#0 - c4-judge

2022-12-14T13:21:53Z

berndartmueller marked the issue as selected for report

#1 - liveactionllama

2023-01-11T20:39:57Z

Adding the grade-a label, since this issue is selected for report.

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