code-423n4 / 2022-08-foundation-findings

0 stars 0 forks source link

Gas Optimizations #221

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Foundation Drop Contest Gas Optimization Report

Summary

The following gas optimization issues were found during the code audit:

  1. Use calldata instead of memory (21 instances)
  2. Cache <array>.length (9 instances)
  3. Use unchecked{} to suppress overflow/underflow check (13 instances)
  4. Long require()/revert() string (44 instances)
  5. Using bools for storage incurs overhead (5 instances)
  6. Use != 0 instead of > 0 when comparing uint (5 instances)
  7. Empty blocks should be removed (5 instances)
  8. Don't initialize variables with default value (8 instances)
  9. Use uint256/int256 instead of other variations (43 instances)
  10. Use abi.encodePacked() instead of abi.encode() (2 instances)
  11. Use private instead of public for constants (6 instances)
  12. Do not use SafeMath for Solidity >= 0.8.0 (1 instances)
  13. Use custom errors instead of revert()/require() strings (52 instances)
  14. Use shift right/left instead of division/multiplication if possible (1 instance)
  15. Not using the named return variables when a function returns, wastes deployment gas (16 instances)

Total 231 instances of 15 issues.

Use calldata instead of memory (21 instances)

When a function with a memory array is called externally, the abi.decode() step has to use a for loop to copy each index of the calldata to the memory index. This overhead can be optimized by using calldata directly.

2022-08-foundation/contracts/FETH.sol::753 => function getLockups(address account) external view returns (uint256[] memory expiries, uint256[] memory amounts) {

2022-08-foundation/contracts/NFTCollection.sol::282 => function baseURI() external view returns (string memory uri) {

2022-08-foundation/contracts/NFTCollection.sol::326 => function tokenURI(uint256 tokenId) public view override returns (string memory uri) {

2022-08-foundation/contracts/NFTCollection.sol::332 => function _baseURI() internal view override returns (string memory) {

2022-08-foundation/contracts/NFTDropCollection.sol::300 => function tokenURI(uint256 tokenId) public view override returns (string memory uri) {

2022-08-foundation/contracts/PercentSplitETH.sol::129 => function initialize(Share[] memory shares) external initializer {

2022-08-foundation/contracts/PercentSplitETH.sol::167 => function createSplit(Share[] memory shares) external returns (PercentSplitETH splitInstance) {

2022-08-foundation/contracts/PercentSplitETH.sol::190 => function proxyCall(address payable target, bytes memory callData) external onlyRecipient {

2022-08-foundation/contracts/PercentSplitETH.sol::292 => function getPredictedSplitAddress(Share[] memory shares) external view returns (address splitInstance) {

2022-08-foundation/contracts/PercentSplitETH.sol::318 => function getShares() external view returns (Share[] memory shares) {

2022-08-foundation/contracts/interfaces/IGetFees.sol::16 => function getFeeRecipients(uint256 tokenId) external view returns (address payable[] memory recipients);

2022-08-foundation/contracts/interfaces/IGetFees.sol::24 => function getFeeBps(uint256 tokenId) external view returns (uint256[] memory royaltiesInBasisPoints);

2022-08-foundation/contracts/interfaces/IProxyCall.sol::6 => function proxyCallAndReturnAddress(address externalContract, bytes memory callData)

2022-08-foundation/contracts/libraries/AddressLibrary.sol::25 => function callAndReturnContractAddress(address externalContract, bytes memory callData)

2022-08-foundation/contracts/libraries/AddressLibrary.sol::34 => function callAndReturnContractAddress(CallWithoutValue memory call)

2022-08-foundation/contracts/libraries/ArrayLibrary.sol::13 => function capLength(address payable[] memory data, uint256 maxLength) internal pure {

2022-08-foundation/contracts/libraries/ArrayLibrary.sol::25 => function capLength(uint256[] memory data, uint256 maxLength) internal pure {

2022-08-foundation/contracts/libraries/BytesLibrary.sol::38 => function startsWith(bytes memory callData, bytes4 functionSig) internal pure returns (bool) {

2022-08-foundation/contracts/libraries/LockedBalance.sol::98 => function get(Lockups storage lockups, uint256 index) internal view returns (Lockup memory balance) {

2022-08-foundation/contracts/mixins/collections/CollectionRoyalties.sol::21 => function getFeeRecipients(uint256 tokenId) external view returns (address payable[] memory recipients) {

2022-08-foundation/contracts/mocks/NonReceivableMock.sol::10 => function callContract(address _contract, bytes memory _callData) public payable {

Cache <array>.length (9 instances)

If <array>.length is used as for loop termination condition, then the .length method will be called in each iteration. Caching it in a local variable can save gas.

2022-08-foundation/contracts/PercentSplitETH.sol::115 => for (uint256 i = 0; i < _shares.length; ++i) {

2022-08-foundation/contracts/PercentSplitETH.sol::135 => for (uint256 i = 0; i < shares.length; ++i) {

2022-08-foundation/contracts/PercentSplitETH.sol::227 => for (uint256 i = _shares.length - 1; i != 0; i--) {

2022-08-foundation/contracts/PercentSplitETH.sol::261 => for (uint256 i = _shares.length - 1; i != 0; i--) {

2022-08-foundation/contracts/PercentSplitETH.sol::320 => for (uint256 i = 0; i < shares.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::126 => for (uint256 i = 0; i < creatorRecipients.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::198 => for (uint256 i = 0; i < creatorShares.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::484 => for (uint256 i = 0; i < creatorRecipients.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::503 => for (uint256 i = 1; i < creatorRecipients.length; ) {

Use unchecked{} to suppress overflow/underflow check (13 instances)

Starting from version 0.8.0, Solidity does overflow/underflow checks by default. It is a good feature to prevent vulnerabilities but it has a significant overhead, especially when used in for loop. When using uint256/int256, it is extremely hard to trigger overflow, so it makes sense to skip these checks. To suppress the overflow/underflow checks, use unchecked {}. For increment situations, just use unchecked {} directly; for decrement situations, add a require() statement before decrementing to prevent underflow.

2022-08-foundation/contracts/FETH.sol::591 => for (uint256 escrowIndex = accountInfo.lockupStartIndex; ; ++escrowIndex) {

2022-08-foundation/contracts/FETH.sol::718 => for (uint256 escrowIndex = accountInfo.lockupStartIndex; ; ++escrowIndex) {

2022-08-foundation/contracts/FETH.sol::760 => for (uint256 escrowIndex = accountInfo.lockupStartIndex; ; ++escrowIndex) {

2022-08-foundation/contracts/FETH.sol::780 => for (uint256 escrowIndex = accountInfo.lockupStartIndex; ; ++escrowIndex) {

2022-08-foundation/contracts/FETH.sol::807 => for (uint256 escrowIndex = accountInfo.lockupStartIndex; ; ++escrowIndex) {

2022-08-foundation/contracts/PercentSplitETH.sol::115 => for (uint256 i = 0; i < _shares.length; ++i) {

2022-08-foundation/contracts/PercentSplitETH.sol::135 => for (uint256 i = 0; i < shares.length; ++i) {

2022-08-foundation/contracts/PercentSplitETH.sol::320 => for (uint256 i = 0; i < shares.length; ++i) {

2022-08-foundation/contracts/libraries/BytesLibrary.sol::25 => for (uint256 i = 0; i < 20; ++i) {

2022-08-foundation/contracts/libraries/BytesLibrary.sol::44 => for (uint256 i = 0; i < 4; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::126 => for (uint256 i = 0; i < creatorRecipients.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::198 => for (uint256 i = 0; i < creatorShares.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::484 => for (uint256 i = 0; i < creatorRecipients.length; ++i) {

Long require()/revert() string (44 instances)

Shortening revert strings to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

2022-08-foundation/contracts/NFTCollection.sol::158 => require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");

2022-08-foundation/contracts/NFTCollection.sol::263 => require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");

2022-08-foundation/contracts/NFTCollection.sol::264 => require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");

2022-08-foundation/contracts/NFTCollection.sol::268 => require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");

2022-08-foundation/contracts/NFTCollection.sol::327 => require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");

2022-08-foundation/contracts/NFTCollectionFactory.sol::173 => require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");

2022-08-foundation/contracts/NFTCollectionFactory.sol::182 => require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");

2022-08-foundation/contracts/NFTCollectionFactory.sol::203 => require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

2022-08-foundation/contracts/NFTCollectionFactory.sol::227 => require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

2022-08-foundation/contracts/NFTCollectionFactory.sol::262 => require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");

2022-08-foundation/contracts/NFTDropCollection.sol::88 => require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol::93 => require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");

2022-08-foundation/contracts/NFTDropCollection.sol::130 => require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol::131 => require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol::172 => require(count != 0, "NFTDropCollection: `count` must be greater than 0");

2022-08-foundation/contracts/NFTDropCollection.sol::179 => require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");

2022-08-foundation/contracts/NFTDropCollection.sol::238 => require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");

2022-08-foundation/contracts/PercentSplitETH.sol::121 => revert("Split: Can only be called by one of the recipients");

2022-08-foundation/contracts/PercentSplitETH.sol::136 => require(shares[i].percentInBasisPoints < BASIS_POINTS, "Split: Share must be less than 100%");

2022-08-foundation/contracts/PercentSplitETH.sol::148 => require(total == BASIS_POINTS, "Split: Total amount must equal 100%");

2022-08-foundation/contracts/libraries/AddressLibrary.sol::31 => require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::58 => require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::63 => require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::74 => require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::87 => require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::88 => require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::89 => require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");

2022-08-foundation/contracts/mixins/roles/AdminRole.sol::19 => require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

2022-08-foundation/contracts/mixins/roles/MinterRole.sol::22 => require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");

2022-08-foundation/contracts/mixins/shared/ContractFactory.sol::22 => require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");

2022-08-foundation/contracts/mixins/shared/ContractFactory.sol::31 => require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");

2022-08-foundation/contracts/mixins/treasury/AdminRole.sol::20 => require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol::137 => require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to grant");

2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol::152 => require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to revoke");

2022-08-foundation/contracts/mocks/BasicERC721.sol::41 => require(owner != address(0), "ERC721: balance query for the zero address");

2022-08-foundation/contracts/mocks/BasicERC721.sol::59 => require(to != owner, "ERC721: approval to current owner");

2022-08-foundation/contracts/mocks/BasicERC721.sol::73 => require(_exists(tokenId), "ERC721: approved query for nonexistent token");

2022-08-foundation/contracts/mocks/BasicERC721.sol::104 => require(_isApprovedOrOwner(msg.sender, tokenId), "ERC721: caller is not token owner nor approved");

2022-08-foundation/contracts/mocks/BasicERC721.sol::129 => require(_isApprovedOrOwner(msg.sender, tokenId), "ERC721: caller is not token owner nor approved");

2022-08-foundation/contracts/mocks/BasicERC721.sol::166 => require(_checkOnERC721Received(from, to, tokenId, _data), "ERC721: transfer to non ERC721Receiver implementer");

2022-08-foundation/contracts/mocks/BasicERC721.sol::189 => require(_exists(tokenId), "ERC721: operator query for nonexistent token");

2022-08-foundation/contracts/mocks/BasicERC721.sol::263 => require(ownerOf(tokenId) == from, "ERC721: transfer from incorrect owner");

2022-08-foundation/contracts/mocks/BasicERC721.sol::264 => require(to != address(0), "ERC721: transfer to the zero address");

2022-08-foundation/contracts/mocks/BasicERC721.sol::307 => revert("ERC721: transfer to non ERC721Receiver implementer");

Using bools for storage incurs overhead (5 instances)

Use uint256(1) and uint256(2) for true/false. 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.

2022-08-foundation/contracts/PercentSplitETH.sol::229 => bool success;

2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol::272 => bool marketCanMint

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::61 => bool private immutable assumePrimarySale;

2022-08-foundation/contracts/mocks/NFTDropCollectionUnknownCreatorMock.sol::12 => bool private sold;

2022-08-foundation/contracts/mocks/NFTDropCollectionWithoutAccessControl.sol::10 => bool private sold;

Use != 0 instead of > 0 when comparing uint (5 instances)

When dealing with unsigned integer types, comparisons with != 0 are cheaper then with > 0.

2022-08-foundation/contracts/NFTDropCollection.sol::88 => require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol::130 => require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol::131 => require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::244 => if (royaltyAmount > 0) {

2022-08-foundation/contracts/mixins/shared/OZERC165Checker.sol::36 => return success && abi.decode(result, (uint256)) > 0;

Empty blocks should be removed (5 instances)

Empty blocks exist in the code. The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.

2022-08-foundation/contracts/NFTCollection.sol::97 => {}

2022-08-foundation/contracts/NFTDropCollection.sol::103 => {}

2022-08-foundation/contracts/NFTDropMarket.sol::94 => {}

2022-08-foundation/contracts/mixins/treasury/CollateralManagement.sol::29 => receive() external payable {}

2022-08-foundation/contracts/mocks/MockNFT.sol::13 => {}

Don't initialize variables with default value (8 instances)

Uninitialized variables are assigned with the types default value. Explicitly initializing a variable with it's default value costs unnecesary gas.

2022-08-foundation/contracts/PercentSplitETH.sol::115 => for (uint256 i = 0; i < _shares.length; ++i) {

2022-08-foundation/contracts/PercentSplitETH.sol::135 => for (uint256 i = 0; i < shares.length; ++i) {

2022-08-foundation/contracts/PercentSplitETH.sol::320 => for (uint256 i = 0; i < shares.length; ++i) {

2022-08-foundation/contracts/libraries/BytesLibrary.sol::25 => for (uint256 i = 0; i < 20; ++i) {

2022-08-foundation/contracts/libraries/BytesLibrary.sol::44 => for (uint256 i = 0; i < 4; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::126 => for (uint256 i = 0; i < creatorRecipients.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::198 => for (uint256 i = 0; i < creatorShares.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::484 => for (uint256 i = 0; i < creatorRecipients.length; ++i) {

Use uint256/int256 instead of other variations (43 instances)

Using smaller data types such as uint8/int8 is more expensive than using uint256/int256. The EVM works with 256bit/32byte words. Every operation is based on these base units. If the data is smaller, further operations are needed to downscale from 256 bits to 8 bits, and this is more expensive than using uint256/int256.

2022-08-foundation/contracts/FETH.sol::96 => uint32 lockupStartIndex;

2022-08-foundation/contracts/FETH.sol::125 => uint8 public constant decimals = 18;

2022-08-foundation/contracts/FETH.sol::552 => accountInfo.lockupStartIndex = uint32(escrowIndex);

2022-08-foundation/contracts/FETH.sol::594 => if (expiration > type(uint32).max) {

2022-08-foundation/contracts/NFTCollection.sol::251 => function updateMaxTokenId(uint32 _maxTokenId) external onlyCreator {

2022-08-foundation/contracts/NFTCollectionFactory.sol::65 => using Strings for uint32;

2022-08-foundation/contracts/NFTCollectionFactory.sol::80 => uint32 public versionNFTCollection;

2022-08-foundation/contracts/NFTCollectionFactory.sol::95 => uint32 public versionNFTDropCollection;

2022-08-foundation/contracts/NFTCollectionFactory.sol::192 => function initialize(uint32 _versionNFTCollection) external initializer {

2022-08-foundation/contracts/NFTCollectionFactory.sol::291 => uint32 maxTokenId,

2022-08-foundation/contracts/NFTCollectionFactory.sol::329 => uint32 maxTokenId,

2022-08-foundation/contracts/NFTCollectionFactory.sol::368 => uint32 maxTokenId,

2022-08-foundation/contracts/NFTCollectionFactory.sol::391 => uint32 maxTokenId,

2022-08-foundation/contracts/NFTDropCollection.sol::126 => uint32 _maxTokenId,

2022-08-foundation/contracts/NFTDropCollection.sol::171 => function mintCountTo(uint16 count, address to) external onlyMinterOrAdmin returns (uint256 firstTokenId) {

2022-08-foundation/contracts/NFTDropCollection.sol::220 => function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {

2022-08-foundation/contracts/PercentSplitETH.sol::73 => uint16 percentInBasisPoints;

2022-08-foundation/contracts/PercentSplitETH.sol::142 => percentInBasisPoints: uint16(shares[i].percentInBasisPoints)

2022-08-foundation/contracts/interfaces/INFTDropCollectionInitializer.sol::12 => uint32 _maxTokenId,

2022-08-foundation/contracts/interfaces/INFTDropCollectionMint.sol::10 => function mintCountTo(uint16 count, address to) external returns (uint256 firstTokenId);

2022-08-foundation/contracts/libraries/LockedBalance.sol::11 => uint32 expiration;

2022-08-foundation/contracts/libraries/LockedBalance.sol::104 => uint128 lockedBalanceBits;

2022-08-foundation/contracts/libraries/LockedBalance.sol::107 => lockedBalanceBits = uint128(lockupMetadata >> 128);

2022-08-foundation/contracts/libraries/LockedBalance.sol::110 => lockedBalanceBits = uint128(lockupMetadata % (2**128));

2022-08-foundation/contracts/libraries/LockedBalance.sol::113 => balance.expiration = uint32(lockedBalanceBits >> 96);

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::27 => uint32 public latestTokenId;

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::34 => uint32 public maxTokenId;

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::40 => uint32 private burnCounter;

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::62 => function _initializeSequentialMintCollection(address payable _creator, uint32 _maxTokenId) internal onlyInitializing {

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::86 => function _updateMaxTokenId(uint32 _maxTokenId) internal {

2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol::54 => uint80 price;

2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol::58 => uint16 limitPerAccount;

2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol::120 => uint80 price,

2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol::121 => uint16 limitPerAccount

2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol::172 => uint16 count,

2022-08-foundation/contracts/mocks/BasicERC721WithoutOwnerOfRevert.sol::36 => function mintCountTo(uint16 count, address to) external returns (uint256 firstTokenId) {

2022-08-foundation/contracts/mocks/WETH9.sol::8 => uint8 public decimals = 18;

2022-08-foundation/contracts/mocks/collections/SequentialMintCollectionMock.sol::10 => function initializeWithModifier(address payable _creator, uint32 _maxTokenId) external initializer {

2022-08-foundation/contracts/mocks/collections/SequentialMintCollectionMock.sol::14 => function initializeWithoutModifier(address payable _creator, uint32 _maxTokenId) external {

2022-08-foundation/test/foundry/FixedPriceDrop.sol::57 => uint32 maxTokenId = 100;

2022-08-foundation/test/foundry/FixedPriceDrop.sol::73 => uint80 price = 0.5 ether;

2022-08-foundation/test/foundry/FixedPriceDrop.sol::74 => uint16 limitPerAccount = 10;

2022-08-foundation/test/foundry/FixedPriceDrop.sol::79 => uint16 count = 3;

Use abi.encodePacked() instead of abi.encode() (2 instances)

abi.encodePacked() is more efficient than abi.encode().

2022-08-foundation/contracts/PercentSplitETH.sol::168 => bytes32 salt = keccak256(abi.encode(shares));

2022-08-foundation/contracts/PercentSplitETH.sol::293 => bytes32 salt = keccak256(abi.encode(shares));

Use private instead of public for constants (6 instances)

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table.

2022-08-foundation/contracts/FETH.sol::125 => uint8 public constant decimals = 18;

2022-08-foundation/contracts/FETH.sol::130 => string public constant name = "Foundation ETH";

2022-08-foundation/contracts/FETH.sol::135 => string public constant symbol = "FETH";

2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol::70 => bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

2022-08-foundation/contracts/mixins/roles/MinterRole.sol::19 => bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

2022-08-foundation/contracts/mocks/NFTDropCollectionUnknownCreatorMock.sol::11 => bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

Do not use SafeMath for Solidity >= 0.8.0 (1 instances)

Solidity v0.8.0 introduces built-in overflow checks, so using SafeMath is a waste of gas.

2022-08-foundation/contracts/PercentSplitETH.sol::50 => import "@openzeppelin/contracts/utils/math/SafeMath.sol";

Use custom errors instead of revert()/require() strings (52 instances)

Using require()/revert() strings is expensive. Starting from Soldity 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.

Custom errors decrease both deploy and runtime gas costs. Note that runtime gas cost is only relevant when the revert condition is met.

2022-08-foundation/contracts/NFTCollection.sol::158 => require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");

2022-08-foundation/contracts/NFTCollection.sol::263 => require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");

2022-08-foundation/contracts/NFTCollection.sol::264 => require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");

2022-08-foundation/contracts/NFTCollection.sol::268 => require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");

2022-08-foundation/contracts/NFTCollection.sol::327 => require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");

2022-08-foundation/contracts/NFTCollectionFactory.sol::173 => require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");

2022-08-foundation/contracts/NFTCollectionFactory.sol::182 => require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");

2022-08-foundation/contracts/NFTCollectionFactory.sol::203 => require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

2022-08-foundation/contracts/NFTCollectionFactory.sol::227 => require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

2022-08-foundation/contracts/NFTCollectionFactory.sol::262 => require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");

2022-08-foundation/contracts/NFTDropCollection.sol::88 => require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol::93 => require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");

2022-08-foundation/contracts/NFTDropCollection.sol::130 => require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol::131 => require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol::172 => require(count != 0, "NFTDropCollection: `count` must be greater than 0");

2022-08-foundation/contracts/NFTDropCollection.sol::179 => require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");

2022-08-foundation/contracts/NFTDropCollection.sol::238 => require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");

2022-08-foundation/contracts/PercentSplitETH.sol::121 => revert("Split: Can only be called by one of the recipients");

2022-08-foundation/contracts/PercentSplitETH.sol::130 => require(shares.length >= 2, "Split: Too few recipients");

2022-08-foundation/contracts/PercentSplitETH.sol::131 => require(shares.length <= 5, "Split: Too many recipients");

2022-08-foundation/contracts/PercentSplitETH.sol::136 => require(shares[i].percentInBasisPoints < BASIS_POINTS, "Split: Share must be less than 100%");

2022-08-foundation/contracts/PercentSplitETH.sol::148 => require(total == BASIS_POINTS, "Split: Total amount must equal 100%");

2022-08-foundation/contracts/PercentSplitETH.sol::216 => require(_splitERC20Tokens(erc20Contract), "Split: ERC20 split failed");

2022-08-foundation/contracts/libraries/AddressLibrary.sol::31 => require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::58 => require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::63 => require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::74 => require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::87 => require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::88 => require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::89 => require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");

2022-08-foundation/contracts/mixins/roles/AdminRole.sol::19 => require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

2022-08-foundation/contracts/mixins/roles/MinterRole.sol::22 => require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");

2022-08-foundation/contracts/mixins/shared/ContractFactory.sol::22 => require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");

2022-08-foundation/contracts/mixins/shared/ContractFactory.sol::31 => require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");

2022-08-foundation/contracts/mixins/treasury/AdminRole.sol::20 => require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol::137 => require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to grant");

2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol::152 => require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to revoke");

2022-08-foundation/contracts/mocks/BasicERC721.sol::41 => require(owner != address(0), "ERC721: balance query for the zero address");

2022-08-foundation/contracts/mocks/BasicERC721.sol::50 => require(owner != address(0), "ERC721: invalid token ID");

2022-08-foundation/contracts/mocks/BasicERC721.sol::59 => require(to != owner, "ERC721: approval to current owner");

2022-08-foundation/contracts/mocks/BasicERC721.sol::73 => require(_exists(tokenId), "ERC721: approved query for nonexistent token");

2022-08-foundation/contracts/mocks/BasicERC721.sol::82 => require(operator != msg.sender, "ERC721: approve to caller");

2022-08-foundation/contracts/mocks/BasicERC721.sol::104 => require(_isApprovedOrOwner(msg.sender, tokenId), "ERC721: caller is not token owner nor approved");

2022-08-foundation/contracts/mocks/BasicERC721.sol::129 => require(_isApprovedOrOwner(msg.sender, tokenId), "ERC721: caller is not token owner nor approved");

2022-08-foundation/contracts/mocks/BasicERC721.sol::166 => require(_checkOnERC721Received(from, to, tokenId, _data), "ERC721: transfer to non ERC721Receiver implementer");

2022-08-foundation/contracts/mocks/BasicERC721.sol::189 => require(_exists(tokenId), "ERC721: operator query for nonexistent token");

2022-08-foundation/contracts/mocks/BasicERC721.sol::238 => require(to != address(0), "ERC721: mint to the zero address");

2022-08-foundation/contracts/mocks/BasicERC721.sol::239 => require(!_exists(tokenId), "ERC721: token already minted");

2022-08-foundation/contracts/mocks/BasicERC721.sol::263 => require(ownerOf(tokenId) == from, "ERC721: transfer from incorrect owner");

2022-08-foundation/contracts/mocks/BasicERC721.sol::264 => require(to != address(0), "ERC721: transfer to the zero address");

2022-08-foundation/contracts/mocks/BasicERC721.sol::307 => revert("ERC721: transfer to non ERC721Receiver implementer");

2022-08-foundation/contracts/mocks/FETHMarketMock.sol::11 => require(msg.sender == address(feth), "Only receive from FETH");

Use shift right/left instead of division/multiplication if possible (1 instance)

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left. While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

2022-08-foundation/contracts/libraries/LockedBalance.sol::100 => uint256 lockupMetadata = lockups.lockups[index / 2];

Not using the named return variables when a function returns, wastes deployment gas (16 instances)

If a named return variable is declared when declaring the function, don't add return statement.

2022-08-foundation/contracts/FETH.sol::237 => return true;

2022-08-foundation/contracts/FETH.sol::294 => return _marketLockupFor(lockupFor, lockupAmount);

2022-08-foundation/contracts/FETH.sol::312 => return _marketLockupFor(account, amount);

2022-08-foundation/contracts/FETH.sol::383 => return transferFrom(msg.sender, to, amount);

2022-08-foundation/contracts/FETH.sol::417 => return true;

2022-08-foundation/contracts/FETH.sol::521 => return accountInfo;

2022-08-foundation/contracts/FETH.sol::554 => return accountInfo;

2022-08-foundation/contracts/FETH.sol::691 => return accountInfo;

2022-08-foundation/contracts/FETH.sol::824 => return address(this).balance;

2022-08-foundation/contracts/NFTCollection.sol::334 => return baseURI_;

2022-08-foundation/contracts/NFTCollection.sol::336 => return "ipfs://";

2022-08-foundation/contracts/NFTCollectionFactory.sol::449 => return keccak256(abi.encodePacked(creator, nonce));

2022-08-foundation/contracts/NFTDropCollection.sol::303 => return string.concat(baseURI, tokenId.toString(), ".json");

2022-08-foundation/contracts/NFTDropMarket.sol::118 => return payable(address(0));

2022-08-foundation/contracts/NFTDropMarket.sol::125 => return super._getSellerOf(nftContract, tokenId);

2022-08-foundation/contracts/NFTDropMarket.sol::149 => return super._getSellerOf(nftContract, tokenId);
HardlyDifficult commented 2 years ago

This report includes a lot of invalid examples - some outright invalid recommendations, others include out of scope files including test mocks. Please spot check your report when generating these. Spammy lists like this are not worth the time to sort through just in case a valid finding is buried in there somewhere.

HickupHH3 commented 2 years ago

siding with sponsor here. it's a waste of both our times.