Seedify NFT Launchpad

CertiK23 Apr 2024Files named in the report

Audit

CertiK reviewed Seedify (Seedify NFT Launchpad) on 23 Apr 2024: 30 findings, 0 critical; 20 marked fixed, 10 open.

No open critical findings

Open the report Open Seedify’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings30

Statuses are the security firm’s own, from its report.

Severity	Open	Fixed
Critical	0	0
High	2	2
Medium	0	7
Low	8	11

HighPotential Replay AttackFixed

The proof and allocation data defined in the parameters of functions `freeMint()` can be replayed multiple times. The malicious users can call the function `freeMint()` multiple times with the same payload and proof.

HighPotential Wrong `NFT` ReceiverFixed

The function `freeMint()` can be used to mint `NFT` for free. There are several checks for the passed allocation data. Once all validations are passed, the caller can receive a certain amount of `NFT`. One of them is validate whether the proof provided matches the passed free allocation data. In other words, validate if the allocation data is valid. ```solidity function __checkFreeMintParams( FreeAllocation calldata allocation, bytes32[] calldata proof ) private { _checkMaxMintInPhase(allocation.phaseId, allocation.toMint); _checkMaxMintWholeINO(allocation.toMint); _requireOpenedSaleAndPhase(allocation.phaseId); __requireValidFreeAllocation(allocation, proof); } ``` However, the actual account validated within the allocation data may not the caller `msg.sender`. There is a possibility that the caller get proof accidentally which was generated for other valid users, then the caller use it to mint `NFT` for free. ```solidity function freeMint( FreeAllocation calldata allocation, bytes32[] calldata proof ) external override { ... INFT(INOStorage.layout().collection).mint( msg.sender, allocation.toMint ); } ```

Medium`summedMaxPhaseCap` Was Not UpdateFixed

The `updatePhaseMaxCapAndMerkleRoot()` function is utilized for updating the maximum cap value and the `Merkle` root for a particular phase. It verifies whether the new `summedMaxPhaseCap` surpasses the `grandTotal` after updating a new maximum cap for the phase. Upon successful verification, the new maximum cap is then updated for the phase. However, the new `summedMaxPhaseCap` remains unaltered. ```solidity function updatePhaseMaxCapAndMerkleRoot( string calldata phaseId, uint256 maxPhaseCap, bytes32 merkleRoot ) external override onlyRole(DEFAULT_ADMIN_ROLE) { /// @custom:audit verifies underneath the phase is not completed updatePhaseMerkleRoot(phaseId, merkleRoot); uint256 summedMaxPhaseCap = SaleStorage .layout() .ledger .summedMaxPhaseCap; summedMaxPhaseCap -= SaleStorage .layout() .phases .data[phaseId] .maxPhaseCap; summedMaxPhaseCap += maxPhaseCap; _isSummedMaxPhaseCapLteGrandTotal( summedMaxPhaseCap, SaleStorage.layout().setUp.grandTotal ); emit PhaseMaxCapUpdated( phaseId, SaleStorage.layout().phases.data[phaseId].maxPhaseCap, maxPhaseCap ); SaleStorage.layout().phases.data[phaseId].maxPhaseCap = maxPhaseCap; } ```

MediumConcerns On the `_updateStorageOnBuy()` and PaymeFixed

It is noted that the contract supports two types of assets to buy NFTs and stores the sold amount in the `SaleStorage.layout().ledger`. However, the contract does not distinguish the amount in the `ledger`, which may cause the result of selling NFT not to align with the expected. ```solidity // update raised amount ledger.totalRaised += amount; ledger.raisedInPhase[phaseId] += amount; ledger.allocationReservedByIn[buyer][phaseId] += amount; ``` For example, assuming the payment asset is ETH(native) and DAI(erc20), and the raised value will mix both assets with the same value during the buy events, at this point, 1 DAI == 1 ETH.

MediumInaccurate Event InputFixed

According to the design of `ERC721` token, each token id start from a specific value `startTokenId` which may not be 1. So the first input value in `BatchMetadataUpdate` event is inaccurate. ```solidity startTokenId = data.startTokenId; ``` ```solidity emit BatchMetadataUpdate(1, _nextTokenId() - 1); ```

MediumIncompatiable storage hashFixed

The Sale component is considered as an extension for INO/IGO contract which follows the diamond design pattern. However, the sale storage is reusing the position for the IGO contract. ```solidity=72 /// @notice Storage position of {SaleStruct} in {Sale} contract. bytes32 public constant Sale_STORAGE = keccak256("igo.storage"); ``` By doing this, storage conflict or overwrite issues will arise based on the diamond design pattern.

MediumIncorrect Imported `ERC721ACloneable`Fixed

It is noted that the `ERC721ContractMetadataCloneable` contract inherits ERC721 functionality from the `seadrop/src/clones/ERC721ACloneable.sol`, not the customized `ERC721ACloneable` contract under the `imported` directory. ```solidity=8 import {ERC721ACloneable} from "seadrop/src/clones/ERC721ACloneable.sol"; ```

MediumPotential Error Start Token IdFixed

The `initialize()` function in `ERC721Base` contract is used to initiate configurations for new `ERC721` token. i.e the initial owner of the token contract, max supply etc. ```solidity function initialize( INOStorage.NFTCollectionData calldata data, address initialOwner, address ino_ ) public virtual override initializer { __ERC721ACloneable__init(data.name, data.symbol); _maxSupply = data.maxCap; _tokenBaseURI = data.uri; startTokenId = data.startTokenId; __ReentrancyGuard_init(); _transferOwnership(initialOwner); ino = ino_; } ``` However, there is an execution order issue inside the function. The internal function `__ERC721ACloneable__init()` will be called first and it will set token name, symbol and start token id. But the intended start token id is set after this function. As a result, the new token id cannot start from the correct value. ```solidity function __ERC721ACloneable__init( string memory name_, string memory symbol_ ) internal onlyInitializing { _name = name_; _symbol = symbol_; _currentIndex = _startTokenId(); } ```

MediumThe `proof` Can be Used Cross Buy FunctionsFixed

The functions `buyAndMintWithERC20()` and `buyAndMintWithNative()` use the same allocation type for storing purchase-related data. It is noted that both functions employ identical signature validation. Consequently, the `proof` and `allocation` used in buyAndMintWithNative() can be utilized in buyAndMintWithERC20(), leading to the unintended consequence of purchasing the same NFT tokens with **different assets(which may reflect different value)** but the same quantity. ```solidity function _requireValidAllocation( UserAllocationFee calldata allocation, bytes32[] calldata proof ) internal view { if ( !MerkleProof.verify( proof, SaleStorage .layout() .phases .data[allocation.usrData.base.phaseId] .merkleRoot, keccak256(abi.encode(allocation)) ) ) revert SaleWritableInternal_AllocationNotFound(); } ```

Low`saleStatus` Not CheckedFixed

In the `_requireOpenedSaleAndPhase()` function, the contract does not check if `SaleStorage.layout().ledger.status` equals the `Status.OPENED`, which may cause a closed sale is still active for buying.

Show 10 more

LowConcerns on Contract UpgradeabilityFixed

The INO contract follows the diamond pattern for the contract design and uses ERC1167(Minimal Proxy Standard - Clones) for the contract creation. And based on the current codebase, the deployed INO contract is not upgradeable or adding new facets. And in `ERC721Base`, the contract inherited from both upgradeable and non-upgradable Openzeppelin contracts: ```solidity import {IERC2981, ERC2981} from "openzeppelin-contracts/token/common/ERC2981.sol"; import {ReentrancyGuardUpgradeable} from "openzeppelin-contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol"; ```

LowConcerns on Phase's `startAt` and `endAt`Fixed

There are three concerns with the Phase's `startAt` and `endAt` that we would like to confirm with the team: 1. When Creating a phase based on the comments, the `startAt` or `endAt` is smaller than the current block.timestamp is allowed ```solidity=34 /** * @dev Phase can start in the past as we can have a phase that is already started BUT contract has * been deployed later due to unexpected reasons. */ if (phase_.startAt >= phase_.endAt) { revert RestrictedWritable_PhaseStartGteEnd(); } ``` However, such values can be updated with `updatePhaseEndDate` and `updatePhaseStartDate`. We would like to double check with the team if ignore validation on `block.timestamp` with `startAt` or `endAt` is intended, since if the `startAt` or `endAt` already passed it may cause unexpected functionality with the sale event. 2. For `updatePhaseEndDate` and `updatePhaseStartDate` there is no validation between `startAt` and `endAt`, as a result, the variant `startAt <= endAt` may not hold, and the unexpected behavior may raised for the sale event. Since both functions are privileged functions, we would like to check with the team if such scenario is considered and it is recommended to update both `startAt` and `endAt` in the same function. 3. Also when updating the phase's start and end timestamp, the phase status may also be changed, for example, from the "opened" to "not started", however, there is no corresponding logic/validation for that, we would like to check with the team if such scenario is considered.

LowInaccessible VariablesFixed

The following variables are not implemented with a getter function which may related to the off-chain logic: 1. SaleStorage.ledger.summedMaxPhaseCap 2. INOStorage.phaseMaxMint

LowInaccurate Supported InterfaceFixed

The function `supportsInterface()` returns all interfaces supported by current contract, including interfaces supported by `ERC721ContractMetadataCloneable` and `ERC2981` contracts. However, it only judge whether `IERC2981` are supported instead of all interfaces supported by `ERC2981` contract. ```solidity function supportsInterface( bytes4 interfaceId ) public view virtual override(ERC721ContractMetadataCloneable, ERC2981) returns (bool) { return interfaceId == type(IERC2981).interfaceId || ERC721ContractMetadataCloneable.supportsInterface(interfaceId); } ```

LowLack of Range Check for Max SupplyFixed

The function `_setMaxSupply()` has designed a check that the new max supply can not exceed the maximum value of `uint64`. ```solidity // Ensure the max supply does not exceed the maximum value of uint64. if (newMaxSupply > 2 ** 64 - 1) { revert CannotExceedMaxSupplyOfUint64(newMaxSupply); } ``` However, such kind of check isn't added in the `initialize()` function for the initial max supply. ```solidity function initialize( INOStorage.NFTCollectionData calldata data, address initialOwner, address ino_ ) public virtual override initializer { __ERC721ACloneable__init(data.name, data.symbol); _maxSupply = data.maxCap; ... } ```

LowMint may happened after the phase endFixed

There are two conditions to close the phase: 1. when the newly raised amount + previous raise amount reached the maxPhaseCap for the phase 2. when the current timestamp passes the `endAt` for the phase. The concern is, that the mint operation happened after the update, and the validation `_requireOpenedSaleAndPhase` will only check if the phase is open and will not check if the current timestamp is dropped inside the valid period. Based on the current implementation, the phase status may have higher priority than the phase period(start and end timestamp), and the end/start can be updated by the privileged role. We would like to check with the team if the description is allowed in the design.

LowMissing Validation on new phaseFixed

The validations for phases are missing when setting a new phase which may cause unexpected functionality, for example, - the start time for the new phase should be smaller than the phase end time - the merkle root should not be bytes(0) - `maxPhaseCap` should be large than 0 - `phase.status` should be `NOT_STARTED` for a complete the new phase. Note: since the `_setPhase` function will be used in both set and update phase functionality, the validation should be carefully set, for example, if the phase already exists, the aforementioned validation such as phase status may not be compatible.

LowPotential Index Out of RangeFixed

The function `getInosDetails()` is designed to query `inos` details with a specific range. The first return value will read data from `_inoDetails` array. However, the index may exceeds the length of `_inoDetails` when loop data from it.

LowPotential Replay Attack When Merkle Root ReuseFixed

The `INO` contract employs a Merkle root to validate user allocations for NFT purchase. If the same Merkle root is utilized across identical contracts deployed on multiple blockchain networks, there is a potential security risk known as a cross-chain replay attack. In this attack scenario, a malicious user could exploit the situation by reusing the same Merkle proof to claim tokens on each blockchain network where the contract is deployed. This could result in the unauthorized acquisition of tokens or the double-spending of the same tokens, which is a violation of the intended token distribution mechanism.

LowThe Sale May Never EndFixed

The value `grandTotal` should always be larger than or equal to the sum of the maxPhaseCap among all phases, however, it will not necessarily `grandTotal == summedMaxPhaseCap`. As a result, if `grandTotal > summedMaxPhaseCap`, the sale will not be able to end even if all phases are sold out. ```solidity=80 if (ledger.totalRaised == grandTotal) _closeSale(); ``` We would like to check with the team if such scenario is considered when setting the values.