search

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

0 stars 0 forks source link

QA Report #222

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Foundation Drop Contest QA Report

Summary

The following QA issues were found during the code audit:

  1. Avoid naming collision (1 instance)
  2. Unsafe ERC20 Operation(s) (3 instances)
  3. Unspecific Compiler Version Pragma (62 instances)
  4. Do not use Deprecated Library Functions (3 instances)
  5. File is missing NatSpec (7 instances)

Total 76 instances of 5 issues.

Avoid naming collision (1 instance)

Don't use Solidity keyword such as storage as variable name:

2022-08-foundation/contracts/FETH.sol::514 => function _freeFromEscrow(address account) private returns (AccountInfo storage) {

Unsafe ERC20 Operation(s) (3 instances)

ERC20 operations can be unsafe due to different implementations and vulnerabilities in the standard. It is therefore recommended to always either use OpenZeppelin's SafeERC20 library or at least to wrap each operation in a require statement.

2022-08-foundation/contracts/PercentSplitETH.sol::236 => try erc20Contract.transfer(share.recipient, amountToSend) {

2022-08-foundation/contracts/PercentSplitETH.sol::245 => try erc20Contract.transfer(_shares[0].recipient, amountToSend) {

2022-08-foundation/contracts/mocks/WETH9.sol::31 => payable(msg.sender).transfer(wad);

Unspecific Compiler Version Pragma (62 instances)

Avoid floating pragmas for non-library contracts. While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations. A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain. It is recommended to pin to a concrete compiler version.

2022-08-foundation/contracts/FETH.sol::42 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/FoundationTreasury.sol::48 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/NFTCollection.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/NFTCollectionFactory.sol::42 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/NFTDropCollection.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/NFTDropMarket.sol::42 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/PercentSplitETH.sol::42 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IAdminRole.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/ICollectionFactory.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IERC20Approve.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IERC20IncreaseAllowance.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IFethMarket.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IGetFees.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IGetRoyalties.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/INFTCollectionInitializer.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/INFTDropCollectionInitializer.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/INFTDropCollectionMint.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IOperatorRole.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IOwnable.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IProxyCall.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IRoles.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IRoyaltyInfo.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/ITokenCreator.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/libraries/AddressLibrary.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/libraries/ArrayLibrary.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/libraries/BytesLibrary.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/libraries/LockedBalance.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/collections/CollectionRoyalties.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketCore.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/roles/AdminRole.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/roles/MinterRole.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/Constants.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/ContractFactory.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/FETHNode.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/FoundationTreasuryNode.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/Gap10000.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/MarketSharedCore.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/OZERC165Checker.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/SendValueWithFallbackWithdraw.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/treasury/AdminRole.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/treasury/CollateralManagement.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol::10 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/treasury/OperatorRole.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mocks/BasicERC721.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mocks/BasicERC721WithAccessControlMock.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mocks/BasicERC721WithoutOwnerOfRevert.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mocks/EmptyMockContract.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mocks/FETHMarketMock.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mocks/MockNFT.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mocks/MockTreasury.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mocks/NFTDropCollectionUnknownCreatorMock.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mocks/NFTDropCollectionWithoutAccessControl.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mocks/NonReceivableMock.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mocks/ReceivableMock.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mocks/RoyaltyRegistry/MockRoyaltyRegistry.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mocks/WETH9.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/contracts/mocks/collections/SequentialMintCollectionMock.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/test/foundry/BytesLibrary.t.sol::3 => pragma solidity ^0.8.12;

2022-08-foundation/test/foundry/FixedPriceDrop.sol::3 => pragma solidity ^0.8.12;

Do not use Deprecated Library Functions (3 instances)

The usage of deprecated library functions should be discouraged. This issue is mostly related to OpenZeppelin libraries.

2022-08-foundation/contracts/mixins/treasury/AdminRole.sol::16 => _setupRole(DEFAULT_ADMIN_ROLE, admin);

2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol::173 => function _setupRole(bytes32 role, address account) internal {

2022-08-foundation/contracts/mocks/BasicERC721WithAccessControlMock.sol::10 => _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);

File is missing NatSpec (7 instances)

Providing NatSpec is a good practice for further development / debugging etc.

File: 2022-08-foundation/contracts/interfaces/ICollectionFactory.sol

File: 2022-08-foundation/contracts/interfaces/IERC20Approve.sol

File: 2022-08-foundation/contracts/interfaces/IERC20ApproveAllowance.sol

File: 2022-08-foundation/contracts/interfaces/INFTCollectionInitializer.sol

File: 2022-08-foundation/contracts/interfaces/INFTDropCollectionInitializer.sol

File: 2022-08-foundation/contracts/interfaces/IOwnable.sol

File: 2022-08-foundation/contracts/interfaces/IProxyCall.sol
HardlyDifficult commented 2 years ago

Avoid naming collision (1 instance)

Invalid. That is not a variable name, but part of the type being returned.

Unsafe ERC20 Operation(s) (3 instances)

Invalid. Both contracts listed here are out of scope (one is a mock contract!)

Unspecific Compiler Version Pragma (62 instances)

Disagree. We intentionally use a floating pragma in order to make integrating with contracts easier. Other contract developers are looking to interact with our contracts and they may be on a different version than we use. The pragma selected for our contracts is the minimum required in order to correctly compile and function. This way integration is easier if they lag a few versions behind, or if they use the latest but we don't bump our packages frequently enough, and when we do upgrade versions unless there was a breaking solidity change -- it should just swap in by incrementing our npm package version.

Do not use Deprecated Library Functions (3 instances)

Agree, will switch to _grantRole here. It was also inconsistent with our other role contracts which had already made this change.

File is missing NatSpec (7 instances)

Yes fair feedback, our interface documentation needs improvement.