Skip to content

Instantly share code, notes, and snippets.

@Picodes

Picodes/4naly3er-2022-12-escher.md Secret

Created December 6, 2022 22:14
Show Gist options
  • Save Picodes/cd4ff52d400a1d060dcbd3d85b08b10f to your computer and use it in GitHub Desktop.
Save Picodes/cd4ff52d400a1d060dcbd3d85b08b10f to your computer and use it in GitHub Desktop.
Download ZIP
Raw
4naly3er-2022-12-escher.md

Report

Gas Optimizations

Issue Instances
GAS-1 Use selfbalance() instead of address(this).balance 2
GAS-2 Use assembly to check for address(0) 3
GAS-3 State variables should be cached in stack variables rather than re-reading them from storage 1
GAS-4 Use calldata instead of memory for function arguments that do not get mutated 13
GAS-5 Use Custom Errors 34
GAS-6 Don't initialize variables with default value 1
GAS-7 ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) 3
GAS-8 Using private rather than public for constants, saves gas 4
GAS-9 Use shift Right/Left instead of division/multiplication if possible 3
GAS-10 Use != 0 instead of > 0 for unsigned integer comparison 3
GAS-11 internal functions not called by the contract should be removed 1

[GAS-1] Use selfbalance() instead of address(this).balance

Use assembly when getting a contract's balance of ETH.

You can use selfbalance() instead of address(this).balance when getting your contract's balance of ETH to save gas. Additionally, you can use balance(address) instead of address.balance() when getting an external contract's balance of ETH.

Saves 15 gas when checking internal balance, 6 for external

Instances (2):

File: src/minters/FixedPrice.sol

109:         ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);

File: src/minters/OpenEdition.sol

92:         ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);

[GAS-2] Use assembly to check for address(0)

Saves 6 gas per instance

Instances (3):

File: src/minters/LPDAFactory.sol

31:         require(sale.saleReceiver != address(0), "INVALID SALE RECEIVER");

File: src/uris/Generative.sol

20:         require(seedBase == bytes32(0), "SEED BASE SET");

28:         require(seedBase != bytes32(0), "SEED BASE NOT SET");

[GAS-3] State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

Saves 100 gas per instance

Instances (1):

File: src/minters/FixedPrice.sol

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

[GAS-4] Use calldata instead of memory for function arguments that do not get mutated

Mark data types as calldata instead of memory where possible. This makes it so that the data is not automatically loaded into memory. If the data passed into the function does not need to be changed (like updating values in an array), it can be passed in as calldata. The one exception to this is if the argument must later be passed into another function that takes an argument that specifies memory storage.

Instances (13):

File: src/Escher.sol

21:     function setURI(string memory _newuri) public onlyRole(DEFAULT_ADMIN_ROLE) {

43:         bytes memory

52:         uint256[] memory,

53:         uint256[] memory,

54:         bytes memory

File: src/Escher721.sol

35:         string memory _name,

36:         string memory _symbol

File: src/Escher721Factory.sol

28:         string memory _name,

29:         string memory _symbol,

File: src/minters/FixedPrice.sol

78:     function initialize(Sale memory _sale) public initializer {

File: src/minters/OpenEdition.sol

82:     function initialize(Sale memory _sale) public initializer {

File: src/uris/Base.sol

10:     function setBaseURI(string memory _baseURI) external onlyOwner {

File: src/uris/Generative.sol

14:     function setScriptPiece(uint256 _id, string memory _data) external onlyOwner {

[GAS-5] Use Custom Errors

Source Instead of using error strings, to reduce deployment and runtime cost, you should use Custom Errors. This would save both deployment and runtime cost.

Instances (34):

File: src/Escher.sol

45:         revert("SoulBound");

56:         revert("SoulBound");

61:         require(balanceOf(_account, uint256(_role)) == 0, "Already Creator");

File: src/Escher721Factory.sol

32:         require(escher.hasRole(escher.CREATOR_ROLE(), msg.sender), "NOT AUTHORIZED");

File: src/minters/FixedPrice.sol

51:         require(block.timestamp < sale.startTime, "TOO LATE");

60:         require(block.timestamp >= sale_.startTime, "TOO SOON");

61:         require(_amount * sale_.price == msg.value, "WRONG PRICE");

63:         require(newId <= sale_.finalId, "TOO MANY");

File: src/minters/FixedPriceFactory.sol

30:         require(IEscher721(_sale.edition).hasRole(bytes32(0x00), msg.sender), "NOT AUTHORIZED");

31:         require(_sale.startTime >= block.timestamp, "START TIME IN PAST");

32:         require(_sale.finalId > _sale.currentId, "FINAL ID BEFORE CURRENT");

File: src/minters/LPDA.sol

62:         require(block.timestamp >= temp.startTime, "TOO SOON");

64:         require(msg.value >= amount * price, "WRONG PRICE");

68:         require(newId <= temp.finalId, "TOO MANY");

93:         require(block.timestamp < sale.startTime, "TOO LATE");

103:         require(owed > 0, "NOTHING TO REFUND");

File: src/minters/LPDAFactory.sol

30:         require(IEscher721(sale.edition).hasRole(bytes32(0x00), msg.sender), "NOT AUTHORIZED");

31:         require(sale.saleReceiver != address(0), "INVALID SALE RECEIVER");

32:         require(sale.startTime >= block.timestamp, "INVALID START TIME");

33:         require(sale.endTime > sale.startTime, "INVALID END TIME");

34:         require(sale.finalId > sale.currentId, "INVALID FINAL ID");

35:         require(sale.startPrice > 0, "INVALID START PRICE");

36:         require(sale.dropPerSecond > 0, "INVALID DROP PER SECOND");

File: src/minters/OpenEdition.sol

61:         require(block.timestamp >= temp.startTime, "TOO SOON");

62:         require(block.timestamp < temp.endTime, "TOO LATE");

63:         require(amount * sale.price == msg.value, "WRONG PRICE");

76:         require(block.timestamp < sale.startTime, "TOO LATE");

91:         require(block.timestamp >= temp.endTime, "TOO SOON");

File: src/minters/OpenEditionFactory.sol

30:         require(IEscher721(sale.edition).hasRole(bytes32(0x00), msg.sender), "NOT AUTHORIZED");

31:         require(sale.startTime >= block.timestamp, "START TIME IN PAST");

32:         require(sale.endTime > sale.startTime, "END TIME BEFORE START");

File: src/uris/Generative.sol

15:         require(bytes(scriptPieces[_id]).length == 0, "ALREADY SET");

20:         require(seedBase == bytes32(0), "SEED BASE SET");

28:         require(seedBase != bytes32(0), "SEED BASE NOT SET");

[GAS-6] Don't initialize variables with default value

Instances (1):

File: src/minters/LPDA.sol

36:     uint48 public amountSold = 0;

[GAS-7] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

Instances (3):

File: src/minters/FixedPrice.sol

65:         for (uint48 x = sale_.currentId + 1; x <= newId; x++) {

File: src/minters/LPDA.sol

73:         for (uint256 x = temp.currentId + 1; x <= newId; x++) {

File: src/minters/OpenEdition.sol

66:         for (uint24 x = temp.currentId + 1; x <= newId; x++) {

[GAS-8] Using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

Instances (4):

File: src/Escher.sol

11:     bytes32 public constant CREATOR_ROLE = keccak256("CREATOR_ROLE");

13:     bytes32 public constant CURATOR_ROLE = keccak256("CURATOR_ROLE");

File: src/Escher721.sol

17:     bytes32 public constant URI_SETTER_ROLE = keccak256("URI_SETTER_ROLE");

19:     bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

[GAS-9] Use shift Right/Left instead of division/multiplication if possible

Instances (3):

File: src/minters/FixedPrice.sol

109:         ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);

File: src/minters/LPDA.sol

84:             uint256 fee = totalSale / 20;

File: src/minters/OpenEdition.sol

92:         ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);

[GAS-10] Use != 0 instead of > 0 for unsigned integer comparison

Instances (3):

File: src/minters/LPDA.sol

103:         require(owed > 0, "NOTHING TO REFUND");

File: src/minters/LPDAFactory.sol

35:         require(sale.startPrice > 0, "INVALID START PRICE");

36:         require(sale.dropPerSecond > 0, "INVALID DROP PER SECOND");

[GAS-11] internal functions not called by the contract should be removed

If the functions are required by an interface, the contract should inherit from that interface and use the override keyword

Instances (1):

File: src/uris/Generative.sol

27:     function tokenToSeed(uint256 _tokenId) internal view returns (bytes32) {

Non Critical Issues

Issue Instances
NC-1 Missing checks for address(0) when assigning values to address state variables 2
NC-2 Event is missing indexed fields 9
NC-3 Functions not used internally could be marked external 33

[NC-1] Missing checks for address(0) when assigning values to address state variables

Instances (2):

File: src/Escher721.sol

41:         tokenUriDelegate = _uri;

58:         tokenUriDelegate = _uriDelegate;

[NC-2] Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

Instances (9):

File: src/minters/FixedPrice.sol

31:     event Start(Sale _saleInfo);

34:     event End(Sale _saleInfo);

40:     event Buy(address indexed _buyer, uint256 _amount, uint256 _value, Sale _saleInfo);

File: src/minters/LPDA.sol

41:     event Start(Sale _saleInfo);

42:     event End(Sale _saleInfo);

43:     event Buy(address indexed _buyer, uint256 _amount, uint256 _value, Sale _saleInfo);

File: src/minters/OpenEdition.sol

30:     event Start(Sale _saleInfo);

33:     event End(Sale _saleInfo);

39:     event Buy(address indexed _buyer, uint256 _amount, uint256 _value, Sale _saleInfo);

[NC-3] Functions not used internally could be marked external

Instances (33):

File: src/Escher.sol

21:     function setURI(string memory _newuri) public onlyRole(DEFAULT_ADMIN_ROLE) {

27:     function addCreator(address _account) public onlyRole(CURATOR_ROLE) {

31:     function supportsInterface(

38:     function safeTransferFrom(

49:     function safeBatchTransferFrom(

File: src/Escher721.sol

32:     function initialize(

57:     function updateURIDelegate(address _uriDelegate) public onlyRole(URI_SETTER_ROLE) {

64:     function setDefaultRoyalty(

72:     function resetDefaultRoyalty() public onlyRole(DEFAULT_ADMIN_ROLE) {

78:     function tokenURI(

84:     function supportsInterface(

File: src/minters/FixedPrice.sol

78:     function initialize(Sale memory _sale) public initializer {

86:     function getPrice() public view returns (uint256) {

91:     function startTime() public view returns (uint256) {

96:     function editionContract() public view returns (address) {

101:     function available() public view returns (uint256) {

File: src/minters/FixedPriceFactory.sol

42:     function setFeeReceiver(address payable fees) public onlyOwner {

File: src/minters/LPDA.sol

99:     function refund() public {

110:     function initialize(Sale calldata _sale) public initializer {

128:     function startTime() public view returns (uint256) {

133:     function editionContract() public view returns (address) {

138:     function available() public view returns (uint256) {

142:     function lowestPrice() public view returns (uint256) {

File: src/minters/LPDAFactory.sol

46:     function setFeeReceiver(address payable fees) public onlyOwner {

File: src/minters/OpenEdition.sol

82:     function initialize(Sale memory _sale) public initializer {

89:     function finalize() public {

97:     function getPrice() public view returns (uint256) {

102:     function startTime() public view returns (uint256) {

107:     function timeLeft() public view returns (uint256) {

112:     function editionContract() public view returns (address) {

116:     function available() public view returns (uint256) {

File: src/minters/OpenEditionFactory.sol

42:     function setFeeReceiver(address payable fees) public onlyOwner {

File: src/uris/Base.sol

18:     function initialize(address _owner) public initializer {

Low Issues

Issue Instances
L-1 abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() 2
L-2 Unsafe ERC20 operation(s) 5

[L-1] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead. If all arguments are strings and or bytes, bytes.concat() should be used instead

Instances (2):

File: src/uris/Generative.sol

24:         seedBase = keccak256(abi.encodePacked(numb, blockhash(numb - 1), time, (time % 200) + 1));

29:         return keccak256(abi.encodePacked(_tokenId, seedBase));

[L-2] Unsafe ERC20 operation(s)

Instances (5):

File: src/minters/FixedPrice.sol

109:         ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);

File: src/minters/LPDA.sol

85:             ISaleFactory(factory).feeReceiver().transfer(fee);

86:             temp.saleReceiver.transfer(totalSale - fee);

105:         payable(msg.sender).transfer(owed);

File: src/minters/OpenEdition.sol

92:         ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment