NextGen - xuwinnie's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L189-L200

Vulnerability details

Impact

Checks-Effects-Interactions Pattern is not followed. Reentrancy is possible with safeMint and (allowlist or public) mint limit can be bypassed.

Proof of Concept

function mint(uint256 mintIndex, address _mintingAddress , address _mintTo, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint256 phase) external {
    require(msg.sender == minterContract, "Caller is not the Minter Contract");
    collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
    if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) {
        _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
        if (phase == 1) {
            tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
        } else {
            tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
        }
    }
}

As above, tokensMintedAllowlistAddress and tokensMintedPerAddress is incremented after _mintProcessing.

function _mintProcessing(uint256 _mintIndex, address _recipient, string memory _tokenData, uint256 _collectionID, uint256 _saltfun_o) internal {
    tokenData[_mintIndex] = _tokenData;
    collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
    tokenIdsToCollectionIds[_mintIndex] = _collectionID;
    _safeMint(_recipient, _mintIndex);
}

Inside _mintProcessing, _safeMint will call onERC721Received of _recipient, which makes reentrancy possible.

function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable {
    ......
    if (block.timestamp >= collectionPhases[col].allowlistStartTime && block.timestamp <= collectionPhases[col].allowlistEndTime) {
        phase = 1;
        bytes32 node;
        if (_delegator != 0x0000000000000000000000000000000000000000) {
            bool isAllowedToMint;
            isAllowedToMint = dmc.retrieveGlobalStatusOfDelegation(_delegator, 0x8888888888888888888888888888888888888888, msg.sender, 1) || dmc.retrieveGlobalStatusOfDelegation(_delegator, 0x8888888888888888888888888888888888888888, msg.sender, 2);
            if (isAllowedToMint == false) {
            isAllowedToMint = dmc.retrieveGlobalStatusOfDelegation(_delegator, collectionPhases[col].delAddress, msg.sender, 1) || dmc.retrieveGlobalStatusOfDelegation(_delegator, collectionPhases[col].delAddress, msg.sender, 2);    
            }
            require(isAllowedToMint == true, "No delegation");
            node = keccak256(abi.encodePacked(_delegator, _maxAllowance, tokData));
            require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, _delegator) + _numberOfTokens, "AL limit");
            mintingAddress = _delegator;
        } else {
            node = keccak256(abi.encodePacked(msg.sender, _maxAllowance, tokData));
            require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit");
            mintingAddress = msg.sender;
        }
        require(MerkleProof.verifyCalldata(merkleProof, collectionPhases[col].merkleRoot, node), 'invalid proof');
    } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {
        phase = 2;
        require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
        require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");
        mintingAddress = msg.sender;
        tokData = '"public"';
    } else {
        revert("No minting");
    }
......
    for(uint256 i = 0; i < _numberOfTokens; i++) {
        uint256 mintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col);
        gencore.mint(mintIndex, mintingAddress, _mintTo, tokData, _saltfun_o, col, phase);
    }
}

Inside mint, _maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, _delegator) + _numberOfTokens and gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col) is checked before calling gencore.mint, so attacker can reenter minter.mint inside onERC721Received to bypass the allowlist or public mint limit.

Tools Used

Manual

Recommended Mitigation Steps

    function mint(uint256 mintIndex, address _mintingAddress , address _mintTo, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint256 phase) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
        if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) {
            if (phase == 1) {
                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
            } else {
                tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
            }
            _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
        }
    }

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L105 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L125

Vulnerability details

Impact

At AuctionEndTime, both claimAuction and cancelBid are possible. Attacker can reenter to get the NFT while drain the contract's balance.

Proof of Concept

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
    auctionClaim[_tokenid] = true;
    uint256 highestBid = returnHighestBid(_tokenid);
    address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
    address highestBidder = returnHighestBidder(_tokenid);
    for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
        if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
            IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
            (bool success, ) = payable(owner()).call{value: highestBid}("");
            emit ClaimAuction(owner(), _tokenid, success, highestBid);
        } else if (auctionInfoData[_tokenid][i].status == true) {
            (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
            emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
        } else {}
    }
}

function cancelBid(uint256 _tokenid, uint256 index) public {
    require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
    require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
    auctionInfoData[_tokenid][index].status = false;
    (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
    emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
}

As above, claimAuction requires block.timestamp >= minter.getAuctionEndTime(_tokenid) and cancelBid requires block.timestamp <= minter.getAuctionEndTime(_tokenid). Both actions are possible when block.timestamp == minter.getAuctionEndTime(_tokenid). As a result, the highest bidder can call claimAuction at this timestamp, and safeTransferFrom will call onERC721Received of the highest bidder. Then, the highest bidder calls cancelBid inside onERC721Received, since status will not be set to false during claimAuction, the call will succeed, which means the highest bidder gets both the NFT and the refund.

Although claimAuction cannot be called by other bidders, the attacker can make multiple bids by multiple contracts. The highest bidder calls cancelBid inside onERC721Received and the rest call cancelBid inside fallback. As a result, the rest of bidders will get double refunds and drain the balance of auction contract.

Tools Used

Manual

Recommended Mitigation Steps

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    require(block.timestamp > minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
......

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L540

Vulnerability details

Impact

Exponential Descending Sale: At each time period (which can vary), the minting cost decreases exponentially until it reaches its resting price (ending minting cost) and stays there until the end of the minting phase. Linear Descending Sale: At each time period (which can vary), the minting cost decreases linearly until it reaches its resting price (ending minting cost) and stays there until the end of the minting phase.

During a Descending Sale, if an unlucky user mints at the last second, the price will not be the resting price but the initial price. The user will overpay.

Proof of Concept

else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {
    phase = 2;
    require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
    require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");
        mintingAddress = msg.sender;
        tokData = '"public"';

As above, the last available timestamp to mint is publicEndTime, and there could be some speculators deliberately minting at the last moment.

function getPrice(uint256 _collectionId) public view returns (uint256) {
    uint tDiff;
    if (collectionPhases[_collectionId].salesOption == 3) {
        // increase minting price by mintcost / collectionPhases[_collectionId].rate every mint (1mint/period)
        // to get the price rate needs to be set
        if (collectionPhases[_collectionId].rate > 0) {
            return collectionPhases[_collectionId].collectionMintCost + ((collectionPhases[_collectionId].collectionMintCost / collectionPhases[_collectionId].rate) * gencore.viewCirSupply(_collectionId));
        } else {
            return collectionPhases[_collectionId].collectionMintCost;
        }
    } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp > collectionPhases[_collectionId].allowlistStartTime && block.timestamp < collectionPhases[_collectionId].publicEndTime) {
        // decreases exponentially every time period
        // collectionPhases[_collectionId].timePeriod sets the time period for decreasing the mintcost
        // if just public mint set the publicStartTime = allowlistStartTime
        // if rate = 0 exponetialy decrease
        // if rate is set the linear decrase each period per rate
        tDiff = (block.timestamp - collectionPhases[_collectionId].allowlistStartTime) / collectionPhases[_collectionId].timePeriod;
        uint256 price;
        uint256 decreaserate;
        if (collectionPhases[_collectionId].rate == 0) {
            price = collectionPhases[_collectionId].collectionMintCost / (tDiff + 1);
            decreaserate = ((price - (collectionPhases[_collectionId].collectionMintCost / (tDiff + 2))) / collectionPhases[_collectionId].timePeriod) * ((block.timestamp - (tDiff * collectionPhases[_collectionId].timePeriod) - collectionPhases[_collectionId].allowlistStartTime));
        } else {
            if (((collectionPhases[_collectionId].collectionMintCost - collectionPhases[_collectionId].collectionEndMintCost) / (collectionPhases[_collectionId].rate)) > tDiff) {
                price = collectionPhases[_collectionId].collectionMintCost - (tDiff * collectionPhases[_collectionId].rate);
            } else {
                price = collectionPhases[_collectionId].collectionEndMintCost;
            }
        }
        if (price - decreaserate > collectionPhases[_collectionId].collectionEndMintCost) {
            return price - decreaserate; 
        } else {
            return collectionPhases[_collectionId].collectionEndMintCost;
        }
    } else {
        // fixed price
        return collectionPhases[_collectionId].collectionMintCost;
    }
}

When block.timestamp is publicEndTime, else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp > collectionPhases[_collectionId].allowlistStartTime && block.timestamp < collectionPhases[_collectionId].publicEndTime) will not be satisfied, so the price will be the initial collectionMintCost, which is higher than the collectionEndMintCost. The user overpays.

Tools Used

Manual

Recommended Mitigation Steps

    } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp >= collectionPhases[_collectionId].allowlistStartTime && block.timestamp <= collectionPhases[_collectionId].publicEndTime){

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L149

Vulnerability details

Impact

In NextGen, artists can sign their collections with a string. However, two critical invariants do not always hold in current implementation:

1. Once a signature is added, it must remain immutable.
2. The artistsSignatures should only be signed by the collectionArtistAddress.

    function artistSignature(uint256 _collectionID, string memory _signature) public {
        require(msg.sender == collectionAdditionalData[_collectionID].collectionArtistAddress, "Only artist");
        require(artistSigned[_collectionID] == false, "Already Signed");
        artistsSignatures[_collectionID] = _signature;
        artistSigned[_collectionID] = true;
    }

Proof of Concept

function setCollectionData(uint256 _collectionID, address _collectionArtistAddress, uint256 _maxCollectionPurchases, uint256 _collectionTotalSupply, uint _setFinalSupplyTimeAfterMint) public CollectionAdminRequired(_collectionID, this.setCollectionData.selector) {
    require((isCollectionCreated[_collectionID] == true) && (collectionFreeze[_collectionID] == false) && (_collectionTotalSupply <= 10000000000), "err/freezed");
    if (collectionAdditionalData[_collectionID].collectionTotalSupply == 0) {
        collectionAdditionalData[_collectionID].collectionArtistAddress = _collectionArtistAddress;
        collectionAdditionalData[_collectionID].maxCollectionPurchases = _maxCollectionPurchases;
        collectionAdditionalData[_collectionID].collectionCirculationSupply = 0;
        collectionAdditionalData[_collectionID].collectionTotalSupply = _collectionTotalSupply;
        collectionAdditionalData[_collectionID].setFinalSupplyTimeAfterMint = _setFinalSupplyTimeAfterMint;
        collectionAdditionalData[_collectionID].reservedMinTokensIndex = (_collectionID * 10000000000);
        collectionAdditionalData[_collectionID].reservedMaxTokensIndex = (_collectionID * 10000000000) + _collectionTotalSupply - 1;
        wereDataAdded[_collectionID] = true;
    } 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;
    }
}

As the second if else branch suggests, collectionArtistAddress should only be modifiable when artistSigned is false. However, a bad collection admin can do the following to bypass the check:

1. Call setCollectionData with {_collectionArtistAddress: badAdmin, _collectionTotalSupply: 0}
2. Call artistSignature with {_signature: fakeSignature}
3. Call setCollectionData with {_collectionArtistAddress: realArtist, _collectionTotalSupply: realSupply}

In the third step, since collectionTotalSupply is zero, the artistSigned[_collectionID] == false will not be checked, which means:

1. _collectionArtistAddress can be changed to the real artist's address even after the bad collection admin already signed it.
2. The collection is not actually signed by the artist, but by the bad collection admin.

Tools Used

Manual

Recommended Mitigation Steps

function setCollectionData(uint256 _collectionID, address _collectionArtistAddress, uint256 _maxCollectionPurchases, uint256 _collectionTotalSupply, uint _setFinalSupplyTimeAfterMint) public CollectionAdminRequired(_collectionID, this.setCollectionData.selector) {
    require(_collectionTotalSupply > 0);
    ......
}

Assessed type

Context