Foundation Drop contest - 0x1f8b's results

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L183

Vulnerability details

Impact

The NFTDropMarketFixedPriceSale contract contains a very easy to bypass constraint.

Proof of Concept

The mintFromFixedPriceSale method checks that the balance after the user's mint is not greater than that configured in saleConfig.limitPerAccount.

    // Confirm that the buyer will not exceed the limit specified after minting.
    if (IERC721(nftContract).balanceOf(msg.sender) + count > saleConfig.limitPerAccount) {
      revert ...;
    }

But this is easy to avoid if prior to the call of mintFromFixedPriceSale the balance is sent to another account, since control of how much said account carries is out of our control, since the counter is controlled by the sender, not by the contract.

Recommended Mitigation Steps

• Use a counter that keeps the sum of the total number of minted tokens per account.

• Low
  • 1. Outdated compiler
  • 2. Upgradable contracts without GAP can lead a upgrade disaster
  • 3. Wrong gap desing
  • 4. Admin can burn tokens
  • 5. tokenURI could return a wrong uri
• Non-Critical
  • 6. Improvable library design
  • 7. Integer overflow in BytesLibrary
  • 8. Open TODO

Low

1. Outdated compiler

The pragma version used is:

pragma solidity ^0.8.12;

The minimum required version must be 0.8.15; otherwise, contracts will be affected by the following important bug fixes:

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

Apart from these, there are several minor bug fixes and improvements.

2. Upgradable contracts without GAP can lead a upgrade disaster

Some contract does not implement a good upgradeable logic.

Proxied contracts MUST include an empty reserved space in storage, usually named __gap, in all the inherited contracts.

For example, if it is an interface, you have to use the interface keyword, otherwise it is a contract that requires your GAP to be correctly updated, so if a new variable is created it could lead in storage collisions that will produce a contract dissaster, including loss of funds.

Reference:

• https://docs.openzeppelin.com/contracts/3.x/upgradeable#storage_gaps
• https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/2cb8996b777060e658e2b8c9b1630313aedb04c0/contracts/token/ERC20/ERC20Upgradeable.sol#L394

MarketSharedCore has a gap because it will be upgradeable but the base class FETHNode don't.

Affected source code:

• MarketSharedCore.sol#L43
• FETHNode.sol#L72

The same problem occurred with AdminRole and MinterRole, both should have a gap, but only AdminRole has.

Affected source code:

• AdminRole.sol#L56
• MinterRole.sol#L57

Other upgradable contracts without a gap have been found:

• NFTDropCollection.sol#L28
• NFTCollection.sol#L28
• CollectionRoyalties.sol#L17
• SequentialMintCollection.sol#L13

3. Wrong gap desing

The inheritance of Gap10000 is a bad design to have gaps, the gaps have to be defined in the contract itself, otherwise if a variable is added, it has to be modified in the base contract, affecting all the contracts that inherit it.

Affected source code:

• Gap10000.sol#L8
• NFTCollectionFactory.sol#L62
• NFTDropMarket.sol#L72

The same behavior is observed with NFTDropMarketCore:

• NFTDropMarketCore.sol#L11
• NFTDropMarket.sol#L68

4. Admin can burn tokens

Using the burn method, the admin can burn an arbitrary amount of tokens from any address.

I believe this is unnecessary and poses a serious centralization risk. A malicious or compromised admin can take advantage of this.

Consider limiting the burn function to the token owner.

Affected source code:

• NFTDropCollection.sol#L159
• NFTCollection.sol#L119

5. tokenURI could return a wrong uri

The baseURI state variable allows it to be hidden by the admin and revealed via the reveal method.

If the baseURI has not been revealed, the tokenURI method will return an incorrect URL.

The following modifications are recommended:

  function tokenURI(uint256 tokenId) public view override returns (string memory uri) {
+   require(bytes(_baseURI).length != 0, "NFTDropCollection: require reveal");
    _requireMinted(tokenId);

    return string.concat(baseURI, tokenId.toString(), ".json");
  }

Affected source code:

• NFTDropCollection.sol#L303

---

Non-Critical

6. Improvable library design

The design of the replaceAtIf method forces the transaction to be reversed if the content to be replaced is not found. It would be more useful and versatile if a boolean was returned indicating whether the replacement was successful or not.

Affected source code:

• BytesLibrary.sol#L28

7. Integer overflow in BytesLibrary

Although it is not vulnerable, it is not convenient to make such large unchecked blocks, on this occasion the only thing that prevents the overflow in startLocation + i is that it will not be possible to send a data long enough to exploit it.

The following modifications are recommended:

  function replaceAtIf(
    bytes memory data,
    uint256 startLocation,
    address expectedAddress,
    address newAddress
  ) internal pure {
    bytes memory expectedData = abi.encodePacked(expectedAddress);
    bytes memory newData = abi.encodePacked(newAddress);
-   unchecked {
      // An address is 20 bytes long
-     for (uint256 i = 0; i < 20; ++i) {
+     for (uint256 i; i < 20;) {
        uint256 dataLocation = startLocation + i;
        if (data[dataLocation] != expectedData[i]) {
          revert BytesLibrary_Expected_Address_Not_Found();
        }
        data[dataLocation] = newData[i];
+       unchecked { ++i; }       
      }
-   }
  }

Affected source code:

• BytesLibrary.sol#L26

8. Open TODO

The code that contains "open todos" reflects that the development is not finished and that the code can change a posteriori, prior release, with or without audit.

Affected source code:

• MarketFees.sol#L193

• Gas
  • 1. Unused import
  • 2. constants expressions are expressions, not constants
  • 3. Reduce the size of error messages Long revert Strings
  • 4. Use Custom Errors instead of Revert Strings to save Gas
  • 5. There's no need to set default values for variables
  • 6. Use unckecked region
  • 7. Remove event index
  • 8. Remove unnecessary inheritance
  • 9. Reduce logic in NFTDropMarketFixedPriceSale
  • 10. use calldata instead of memory
  • 11. Use abi.encode instead of abi.encodePacked
  • 12. > 0 is less efficient than != 0 for unsigned integers
  • 13. Don't use the length of an array for loops condition
  • 14. ++i costs less gas compared to i++ or i += 1

Gas

1. Unused import

Importing pointless files costs gas during deployment and is a bad coding practice that is important to ignore.

Remove the following imports:

• ICollectionFactory.sol from ContractFactory.sol#L7

2. constants expressions are expressions, not constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

Reference:

• https://github.com/ethereum/solidity/issues/9232

Consequences: each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..). since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

Affected source code:

• MinterRole.sol#L19
• NFTDropMarketFixedPriceSale.sol#L70

3. 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.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
  function testShortRevert(bool path) public view { 
    require(path, "test error");
  }
}

contract TesterB {
  function testLongRevert(bool path) public view { 
    require(path, "test big error message, more than 32 bytes");
  }
}

Gas saving executing: 18 per entry

TesterA.testShortRevert: 21886    
TesterB.testLongRevert:  21904       

Affected source code:

• AdminRole.sol#L19
• MinterRole.sol#L22
• ContractFactory.sol#L22
• ContractFactory.sol#L31
• AddressLibrary.sol#L31
• SequentialMintCollection.sol#L58
• SequentialMintCollection.sol#L63
• SequentialMintCollection.sol#L74
• SequentialMintCollection.sol#L87
• SequentialMintCollection.sol#L88
• SequentialMintCollection.sol#L89
• NFTCollectionFactory.sol#L173
• NFTCollectionFactory.sol#L203
• NFTCollectionFactory.sol#L227
• NFTCollectionFactory.sol#L262
• NFTDropCollection.sol#L88
• NFTDropCollection.sol#L93
• NFTDropCollection.sol#L130
• NFTDropCollection.sol#L131
• NFTDropCollection.sol#L172
• NFTDropCollection.sol#L179
• NFTDropCollection.sol#L238
• NFTCollection.sol#L158
• NFTCollection.sol#L263
• NFTCollection.sol#L264
• NFTCollection.sol#L268
• NFTCollection.sol#L327

4. 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).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
  function testRevert(bool path) public view { 
    require(path, "test error");
  }
}

contract TesterB {
  error MyError(string msg);

  function testError(bool path) public view {
     if(path) revert MyError("test error");
  }
}

Gas saving executing: 9 per entry

TesterA.testRevert: 21611  
TesterB.testError:  21602     

Affected source code:

• AdminRole.sol#L19
• MinterRole.sol#L22
• ContractFactory.sol#L22
• ContractFactory.sol#L31
• AddressLibrary.sol#L31
• SequentialMintCollection.sol#L58
• SequentialMintCollection.sol#L63
• SequentialMintCollection.sol#L74
• SequentialMintCollection.sol#L87
• SequentialMintCollection.sol#L88
• SequentialMintCollection.sol#L89
• NFTCollectionFactory.sol#L173
• NFTCollectionFactory.sol#L203
• NFTCollectionFactory.sol#L227
• NFTCollectionFactory.sol#L262
• NFTDropCollection.sol#L88
• NFTDropCollection.sol#L93
• NFTDropCollection.sol#L130
• NFTDropCollection.sol#L131
• NFTDropCollection.sol#L172
• NFTDropCollection.sol#L179
• NFTDropCollection.sol#L238
• NFTCollection.sol#L158
• NFTCollection.sol#L263
• NFTCollection.sol#L264
• NFTCollection.sol#L268
• NFTCollection.sol#L327

5. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
  function testInit() public view returns (uint) { uint a = 0; return a; }
}

contract TesterB {
  function testNoInit() public view returns (uint) { uint a; return a; }
}

Gas saving executing: 8 per entry

TesterA.testInit:   21392 
TesterB.testNoInit: 21384    

Affected source code:

• BytesLibrary.sol#L25
• BytesLibrary.sol#L44
• MarketFees.sol#L126
• MarketFees.sol#L198
• MarketFees.sol#L484

6. Use unckecked region

It's possible to save gas using the unckecked keyword around a math operation. This will avoid the required checks to ensure that the variable won't overflow.

Reference:

• https://docs.soliditylang.org/en/v0.8.0/control-structures.html#checked-or-unchecked-arithmetic

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
  function testUnChecked(uint a, uint b) public returns (uint) { unchecked { return a - b; } }
}

contract TesterB {
  function testChecked(uint a, uint b) public returns (uint) { return a - b; }
}

Gas saving executing: 182 per entry

TesterA.testUnChecked:  22103
TesterB.testChecked:    22285   

Affected source code:

• BytesLibrary.sol#L25
• MarketFees.sol#L126
• MarketFees.sol#L198

7. Remove event index

The following values are considered unnecessary to index and may incur unnecessary overhead.

Affected source code:

• maxTokenId in SequentialMintCollection.sol#L49
• admin in SequentialMintCollection.sol#L55
• implementation and version in NFTCollectionFactory.sol#L111
• version in NFTCollectionFactory.sol#L120 and NFTCollectionFactory.sol#L136

8. Remove unnecessary inheritance

The NFTDropMarketCore contract does not provide any logic, only a gap in the style of the Gap10000 contract, these gaps are badly designed since if the class that inherits them modifies the number of slots used, modifying the base class will affect all the contracts that inherit them.

It's recommended to delete both base contracts:

• NFTDropMarketCore.sol#L11
• Gap10000.sol#L8

Affected source code:

• NFTDropMarket.sol#L68
• NFTCollectionFactory.sol#L62
• NFTDropMarket.sol#L72

9. Reduce logic in NFTDropMarketFixedPriceSale

The _getSellerOf method calls the getFixedPriceSale method and runs totally unnecessary logic like checking if you have the right to mint (marketCanMint).

The following modifications are recommended:

  function _getSellerOf(
    address nftContract,
    uint256 /* tokenId */
  ) internal view virtual override returns (address payable seller) {
-   (seller, , , , ) = getFixedPriceSale(nftContract);
+   try INFTDropCollectionMint(nftContract).numberOfTokensAvailableToMint() returns (uint256 count) {
+     if (count != 0) {
+       seller = nftContractToFixedPriceSaleConfig[nftContract].seller;
+     }
+   } catch // solhint-disable-next-line no-empty-blocks
+   {
+     // Contract not supported - return default values
+   }
  }

Affected source code:

• NFTDropMarketFixedPriceSale.sol#L305

10. use calldata instead of memory

The method createNFTDropCollectionWithPaymentFactory is external, but the argument paymentAddressFactoryCall was defined as memory instead of as calldata.

The following modifications are recommended:

  function createNFTDropCollectionWithPaymentFactory(
    string calldata name,
    string calldata symbol,
    string calldata baseURI,
    bytes32 postRevealBaseURIHash,
    uint32 maxTokenId,
    address approvedMinter,
    uint256 nonce,
-   CallWithoutValue memory paymentAddressFactoryCall
+   CallWithoutValue calldata paymentAddressFactoryCall
  ) external returns (address collection) {

Affected source code:

• NFTCollectionFactory.sol#L371

11. Use abi.encode instead of abi.encodePacked

abi.encodePacked has a cost difference with respect to abi.encode depending on the types it uses, in the case of the reference shown below you can see that the type address is affected, so I have decided to do a test on it.

Also there is a discussion about removing abi.encodePacked from future versions of Solidity (ethereum/solidity#11593), so using abi.encode now will ensure compatibility in the future.

Reference:

• https://github.com/ConnorBlockchain/Solidity-Encode-Gas-Comparison#address-comparison

Proof of concept (without optimizations):

pragma solidity ^0.8.12;

contract TesterA {
   function getSaltEncode(address creator, uint256 nonce) public pure returns (bytes32) {
    return keccak256(abi.encode(creator, nonce));
  }
}

contract TesterB {
   function getSaltEncodePacked(address creator, uint256 nonce) public pure returns (bytes32) {
    return keccak256(abi.encodePacked(creator, nonce));
  }
}

Gas saving: 188

TesterA.getSaltEncode:       22774
TesterB.getSaltEncodePacked: 22962 

It's recommended to apply the following changes:

  function _getSalt(address creator, uint256 nonce) private pure returns (bytes32) {
-   return keccak256(abi.encodePacked(creator, nonce));
+   return keccak256(abi.encode(creator, nonce));
  }

Affected source code:

• NFTCollectionFactory.sol#L449

12. > 0 is less efficient than != 0 for unsigned integers

Although it would appear that > 0 is less expensive than != 0, this is only true in the absence of the optimizer and outside of a need statement. Gas will be saved if the optimizer is enabled at 10k and you're in a require statement.

Reference:

• https://twitter.com/gzeon/status/1485428085885640706

Affected source code:

• NFTDropCollection.sol#L88
• NFTDropCollection.sol#L130
• NFTDropCollection.sol#L131

13. Don't use the length of an array for loops condition

It's cheaper to store the length of the array inside a local variable and iterate over it.

Affected source code:

• MarketFees.sol#L126
• MarketFees.sol#L198
• MarketFees.sol#L484
• MarketFees.sol#L503

14. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• NFTCollectionFactory.sol#L207
• NFTCollectionFactory.sol#L231

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