[H-02] Forging and minting new NFTs would become impossible in the future, because of access control.

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/EntropyGenerator/EntropyGenerator.sol#L206

Vulnerability details

Vulnerability Details:

The function EntropyGenerator::initializeAlphaIndices is used for selecting index points for the 999999 Entropy, which is the "Golder God" entity. This function can only be called by the owner, because it has the onlyOwner modifier. The problem arises when the function forgeWithListed is called from the EntityForging contract:

    function forgeWithListed(uint256 forgerTokenId, uint256 mergerTokenId)
        external
        payable
        whenNotPaused
        nonReentrant
        returns (uint256)
    {
        .
        .
        .
        .
        .
        uint256 devFee = forgingFee / taxCut;
        uint256 forgerShare = forgingFee - devFee;
        address payable forgerOwner = payable(nftContract.ownerOf(forgerTokenId));

@>      uint256 newTokenId = nftContract.forge(msg.sender, forgerTokenId, mergerTokenId, "");

        (bool success,) = nukeFundAddress.call{value: devFee}("");
        require(success, "Failed to send to NukeFund");
        (bool success_forge,) = forgerOwner.call{value: forgerShare}("");
        require(success_forge, "Failed to send to Forge Owner");

        // Cancel listed forger nft
        _cancelListingForForging(forgerTokenId);

        uint256 newEntropy = nftContract.getTokenEntropy(newTokenId);

        emit EntityForged(newTokenId, forgerTokenId, mergerTokenId, newEntropy, forgingFee);

        return newTokenId;
    }

As we can see, the value of the newTokenId is derived by calling the forge function in the TraitForgeNft contract. The forge function on the other side calls the internal function _mintNewEntity, so we can get the new token id. In the _mintNewEntity there is a check if we have to transition to a new generation. If that's the case, we are going to call the _incrementGeneration function, because we are going to enter the first if-statement: In the _incrementGeneration function we call the function to transition to a new generation:

function _incrementGeneration() private {
    require(generationMintCounts[currentGeneration] >= maxTokensPerGen, "Generation limit not yet reached");
    currentGeneration++;
    generationMintCounts[currentGeneration] = 0;
    priceIncrement = priceIncrement + priceIncrementByGen;
@>  entropyGenerator.initializeAlphaIndices();
    emit GenerationIncremented(currentGeneration);
}

The problem arises, when we have to call the entropyGenerator.initializeAlphaIndices() function. Since the caller of the function is going to be the TraitForgeNft contract and not the owner, the function will revert. leading to not being able to transition to a new generation. This function is also called in the mintToken and mintWithBudget functions.

Impact

This finding impacts the 'Forging' aspect of the protocol. When it is time for a new generation, the forging of new NFT-s would be blocked, since the transactions are always going to revert.

Recommended Mitigation

Change the access control of EntitiyForging::forgeWithListed() from

modifier onlyAllowedCaller() {
-   require(msg.sender == allowedCaller, "Caller is not allowed");
+   require(msg.sender == allowedCaller || msg.sender == owner(), "Caller is not allowed");
    _;
}

Assessed type

Access Control

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)

[c4-judge]

koolexcrypto changed the severity to 3 (High Risk)