function addRandomizer(uint256 _collectionID, address _randomizerContract) public FunctionAdminRequired(this.addRandomizer.selector) {
require(IRandomizer(_randomizerContract).isRandomizerContract() == true, "Contract is not Randomizer");
collectionAdditionalData[_collectionID].randomizerContract = _randomizerContract;
collectionAdditionalData[_collectionID].randomizer = IRandomizer(_randomizerContract);
}
As demonstrated above, addRandomizer() does not only add new randomizer - but when randomizer is already added - it will be updated.
Our recommendation is to change the name from addRandomizer to addOrUpdateRandomizer().
According to comment:
// only _collectionArtistAddress , _maxCollectionPurchases can change after total supply is set
However, the current implementation allows to change not only _collectionArtistAddress and _maxCollectionPurchases, but also _setFinalSupplyTimeAfterMint
} else if (artistSigned[_collectionID] == false) {
collectionAdditionalData[_collectionID].collectionArtistAddress = _collectionArtistAddress;
collectionAdditionalData[_collectionID].maxCollectionPurchases = _maxCollectionPurchases;
collectionAdditionalData[_collectionID].setFinalSupplyTimeAfterMint = _setFinalSupplyTimeAfterMint;
} else {
collectionAdditionalData[_collectionID].maxCollectionPurchases = _maxCollectionPurchases;
collectionAdditionalData[_collectionID].setFinalSupplyTimeAfterMint = _setFinalSupplyTimeAfterMint;
NextGenCore.sol implements only one function which allows to freeze a collection. However, there's no additional function which would allow to unfreeze it.
This may lead to some issues when collection is frozen by mistake.
function freezeCollection(uint256 _collectionID) public FunctionAdminRequired(this.freezeCollection.selector) {
require(isCollectionCreated[_collectionID] == true, "No Col");
collectionFreeze[_collectionID] = true;
}
function emergencyWithdraw() public FunctionAdminRequired(this.emergencyWithdraw.selector) {
uint balance = address(this).balance;
address admin = adminsContract.owner();
(bool success, ) = payable(admin).call{value: balance}("");
emit Withdraw(msg.sender, success, balance);
}
emergencyWithdraw() allows to withdraw any balance from the smart contract. The address where funds will go are defined as adminsContract.owner().
Since it's not possible to change the owner of adminsContract, this address is non-changeble. If private key will be stolen or lost for this address - it won't be possible to access funds withdrawn to this address.
Re-implement emergencyWithdraw() so that it will be possible to set any address where funds might be withdrawn.
The same issue occurs in MinterContract.sol:
function emergencyWithdraw() public FunctionAdminRequired(this.emergencyWithdraw.selector) {
uint balance = address(this).balance;
address admin = adminsContract.owner();
(bool success, ) = payable(admin).call{value: balance}("");
emit Withdraw(msg.sender, success, balance);
}
if (id==0) {
return wordsList[id];
} else {
return wordsList[id - 1];
}
getWord(0) and getWord(1) will return the same data. That implies, that the distribution is not the same for every id.
There's no need to perform wordsList[id - 1]. Returning wordsList[id] every time will be sufficient. Especially, that function getWord() is used in randomWord() function which performs % 100 on id.
This means that 0 <= id < 100, so we do not need to care about getWord(100), since it's not possible to reach it.
function setFinalSupply(uint256 _collectionID) public FunctionAdminRequired(this.setFinalSupply.selector) {
require (block.timestamp > IMinterContract(minterContract).getEndTime(_collectionID) + collectionAdditionalData[_collectionID].setFinalSupplyTimeAfterMint, "Time has not passed");
collectionAdditionalData[_collectionID].collectionTotalSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply;
collectionAdditionalData[_collectionID].reservedMaxTokensIndex = (_collectionID * 10000000000) + collectionAdditionalData[_collectionID].collectionTotalSupply - 1;
}
setFinalSupply() will execute on non-existing collections.
This does not pose any security threat - thus it's evaluated as Low. Nonetheless, our recommendation is to add additional check: require((isCollectionCreated[_collectionID] == true), "Not allowed");
collectionPhases[_collectionID].allowlistStartTime = _allowlistStartTime;
collectionPhases[_collectionID].allowlistEndTime = _allowlistEndTime;
collectionPhases[_collectionID].merkleRoot = _merkleRoot;
collectionPhases[_collectionID].publicStartTime = _publicStartTime;
collectionPhases[_collectionID].publicEndTime = _publicEndTime;
Function setCollectionPhases() is used to set the starting and ending times of both the allowlist and public phases.
However, there's no verificaiton of provided data:
- User may set
allowlistStartTime > allowlistEndTime - User may set
publicStartTime > publicEndTime - According to documentations, allowlist phase should be sooner than public phase, but user may set
allowlistStartTime > publicStartTime
This is user-related error (user calls function with incorrect parameters) - thus it was evaluated as Low.
emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
Function does not refund highestBid, but auctionInfoData[_tokenid][i].bid.
Above line of code should be changed to:
emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, auctionInfoData[_tokenid][i].bid);
A good coding-style practice is to divide parameters separated by comma by extra white-space, e.g.: A,B, should be changed to A, B.
bytes32 externalCol = keccak256(abi.encodePacked(_erc721Collection,_burnCollectionID));
bytes32 externalCol = keccak256(abi.encodePacked(_erc721Collection,_burnCollectionID));
Change _erc721Collection,_burnCollectionID to: _erc721Collection, _burnCollectionID.
AuctionDemo.sol:
69: for (uint256 i=0; i< auctionInfoData[_tokenid].length; i++) {
90: for (uint256 i=0; i< auctionInfoData[_tokenid].length; i++) {
110: for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
136: for (uint256 i=0; i<auctionInfoData[_tokenid].length; i++) {
Lines 69, 90, 110, 136: change i=0; to i = 0;
Lines 69, 90, 110: change i< auctionInfoData[_tokenid].length; to i < auctionInfoData[_tokenid].length;
Line 136: change i<auctionInfoData[_tokenid].length; to i <auctionInfoData[_tokenid].length;
Line 110: change i ++ to i++
MinterContract.sol
184: for (uint256 y=0; y< _recipients.length; y++) {
187: for(uint256 i = 0; i < _numberOfTokens[y]; i++) {
234: for(uint256 i = 0; i < _numberOfTokens; i++) {
Lines 184: change y=0; y< _recipients.length; to y = 0; y < _recipients.length;
Lines 187, 234: change for(uint256 i = 0; to for (uint256 i = 0;