Foundation Drop contest - simon135's results

dont use charot in solidtiy files because you can have issues with deployment and testing version causing weird bugs

pragma solidity ^0.8.0;

its in most of the main contracts https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L3

remove dead code

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L37-L40

you can remove that gap because they have alot of them in the drevitve contracts

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L42

accounting of burning can happen only when there are enough nfts

when burnCounter overflow to zero then we dont know that what is burned which then only has tokendIds and not minus the burnedTokens

      supply = latestTokenId - burnCounter;
    }

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L121

creator can decrease maxTokenId and make it more rare

creator says maxTokens is 100 but then one day the creators is like lets make it more rare so he makes Maxtokens to 50 causing users destrust of how many are going to be out there. There can even have a dos after like 1 users mints because then if max tokens is 2 and admin buy one and a user makes a tx to buy one then the admin can frontrun to make it the users tx revert and cause gas funds to be lost . make maxtokens a immutable to give users more trust in buying nfts from collectors. https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L86

if there is one nft left it wouldnt be fair to a user who makes tx then a frontruning bot makes the same tx bu with more gas and the frontrun bot gets the nft.

make it some how better then having a gas off. myabe tell users to use private tx,flashbots to avoid this. https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L170

there an be a underflow but since its not in a unchecked will revert

10-20-1= -11 which since uint and not in unchecked block it will revert so on certin times when users are buying nfts it will revert.

        // 10 -20 - 1 = -11 
        sellerRev = price - totalFees - creatorRev;
      }

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L473 make it into a unchecked block

---

this comment didnt happen,replace it or change the varaible

  /// @dev This value was replaced with an immutable version.
  address payable private __gap_was_treasury;

https://github.com/code-423n4/2022-08-foundation/blob/b4e1dc6a9a434d576cb8f74676e7dbb65dacc98d/contracts/mixins/shared/FoundationTreasuryNode.sol#L22

remove todos its just best pratcise

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L193

Make event with 3 fields or more make them indexed

https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L133 https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L158

make i++ into ++i to save 3 gas on copy

NFTCollectionFactory.sol:207:      versionNFTCollection++;
NFTCollectionFactory.sol:231:      versionNFTDropCollection++;

make array.length into a cahced variable to sav gas

mixins/shared/MarketFees.sol:126:      for (uint256 i = 0; i < creatorRecipients.length; ++i) {
mixins/shared/MarketFees.sol:198:    for (uint256 i = 0; i < creatorShares.length; ++i) {
mixins/shared/MarketFees.sol:484:          for (uint256 i = 0; i < creatorRecipients.length; ++i) {

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) Source Custom Errors in Solidity: Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them. Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

NFTCollection.sol:158:    require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
NFTCollection.sol:263:    require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");
NFTCollection.sol:264:    require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");
NFTCollection.sol:268:      require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");
NFTCollection.sol:327:    require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");
NFTCollectionFactory.sol:173:    require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");
NFTCollectionFactory.sol:178:   * @notice Defines requirements for the collection drop factory at deployment time.
NFTCollectionFactory.sol:182:    require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");
NFTCollectionFactory.sol:203:    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
NFTCollectionFactory.sol:227:    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
NFTCollectionFactory.sol:262:    require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
NFTDropCollection.sol:88:    require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
NFTDropCollection.sol:93:    require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");
NFTDropCollection.sol:130:    require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
NFTDropCollection.sol:131:    require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");
NFTDropCollection.sol:172:    require(count != 0, "NFTDropCollection: `count` must be greater than 0");
NFTDropCollection.sol:179:    require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");
NFTDropCollection.sol:238:    require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");
NFTDropCollection.sol:301:    _requireMinted(tokenId);

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc. 1 byte for character

NFTCollection.sol:158:    require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
NFTCollection.sol:268:      require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");
NFTCollection.sol:327:    require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");
NFTCollectionFactory.sol:173:    require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");
NFTCollectionFactory.sol:178:   * @notice Defines requirements for the collection drop factory at deployment time.
NFTCollectionFactory.sol:182:    require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");
NFTCollectionFactory.sol:203:    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
NFTCollectionFactory.sol:227:    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

Unchecking arithmetics operations that can’t underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: Checked or Unchecked Arithmetic. I suggest wrapping with an unchecked block: Same thing with second unchecked because total can't overflow amount cant overflow

Unchecked { uint256 total = amount + hasMinted[msg.sender]; }

mixins/shared/MarketFees.sol:126:      for (uint256 i = 0; i < creatorRecipients.length; ++i) {
mixins/shared/MarketFees.sol:198:    for (uint256 i = 0; i < creatorShares.length; ++i) {
mixins/shared/MarketFees.sol:484:          for (uint256 i = 0; i < creatorRecipients.length; ++i) {

Using bools for storage incurs overhead

// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.

mixins/shared/MarketFees.sol:40:   * mapping(address => mapping(uint256 => bool)) private _nftContractToTokenIdToFirstSaleCompleted;
mixins/shared/MarketFees.sol:61:  bool private immutable assumePrimarySale;

save gas by making require (uint amount >0); into

require (!= 0); because a uint is ether 0 or greater so it saves gas .

NFTDropCollection.sol:131:    require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");

make address(0) into long form to save gas

ex: 0x00000000000000

mixins/shared/MarketFees.sol:358:    if (creator != address(0)) {
mixins/shared/MarketFees.sol:369:      if (owner != address(0)) {
mixins/shared/MarketFees.sol:522:    if (buyReferrer != address(0) && buyReferrer != msg.sender && buyReferrer != seller && buyReferrer != creator) 
NFTDropMarket.sol:118:        return payable(address(0));
NFTDropMarket.sol:140:      if (owner != address(0)) {
NFTDropMarket.sol:116:      if (owner != address(0)) {
NFTDropCollection.sol:138:    if (_paymentAddress != address(0)) {
NFTDropCollection.sol:149:    if (_approvedMinter != address(0)) {
NFTDropCollection.sol:256:    if (creatorPaymentAddress == address(0)) {
NFTCollection.sol:305:    if (creatorPaymentAddress == address(0)) {
NFTCollection.sol:158:    require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");

USE ASSEMBLY TO CHECK FOR ADDRESS(0)

ex: iszero

mixins/shared/MarketFees.sol:358:    if (creator != address(0)) {
mixins/shared/MarketFees.sol:369:      if (owner != address(0)) {
mixins/shared/MarketFees.sol:522:    if (buyReferrer != address(0) && buyReferrer != msg.sender && buyReferrer != seller && buyReferrer != creator) 
NFTDropMarket.sol:118:        return payable(address(0));
NFTDropMarket.sol:140:      if (owner != address(0)) {
NFTDropMarket.sol:116:      if (owner != address(0)) {
NFTDropCollection.sol:138:    if (_paymentAddress != address(0)) {
NFTDropCollection.sol:149:    if (_approvedMinter != address(0)) {
NFTDropCollection.sol:256:    if (creatorPaymentAddress == address(0)) {
NFTCollection.sol:305:    if (creatorPaymentAddress == address(0)) {
NFTCollection.sol:158:    require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");

FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED PAYABLE

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

NFTCollection.sol:119:  function burn(uint256 tokenId) public override onlyCreator {
NFTCollection.sol:230:  function selfDestruct() external onlyCreator {
NFTCollection.sol:238:  function updateBaseURI(string calldata baseURIOverride) external onlyCreator {
NFTCollection.sol:251:  function updateMaxTokenId(uint32 _maxTokenId) external onlyCreator {
NFTCollection.sol:262:  function _mint(string calldata tokenCID) private onlyCreator returns (uint256 tokenId) {
NFTDropCollection.sol:159:  function burn(uint256 tokenId) public override onlyAdmin {
NFTDropCollection.sol:195:  function reveal(string calldata _baseURI) external onlyAdmin validBaseURI(_baseURI) onlyWhileUnrevealed {
NFTDropCollection.sol:209:  function selfDestruct() external onlyAdmin {
NFTDropCollection.sol:220:  function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {
NFTCollectionFactory.sol:202:  function adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin {
NFTCollectionFactory.sol:226:  function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {

you can save gas by just reading the immutable variable

function getFethAddress() external view returns (address fethAddress) {
  fethAddress = address(feth);
}

change it to return feth; saves 3 gas https://github.com/code-423n4/2022-08-foundation/blob/b4e1dc6a9a434d576cb8f74676e7dbb65dacc98d/contracts/mixins/shared/FETHNode.sol#L69 2. you can save gas by not using this memory variable remove delta and just do the math when doing the functoin to save 3 gas

        uint256 delta = totalAmount - msg.value;
        feth.marketWithdrawFrom(msg.sender, delta);
      }

change to

feth.marketWithdrawFrom(msg.sender,totalAmount-msg.value);

FETHNode.sol:51

createNFTDropCollection gas reporter results:
  using > 0 (current):
    - 319246  ·     319578  ·      319361
  using != 0 (recommendation):
    -  319252  ·     319584  ·      319367
  impact: +6 gas