NextGen - Aymen0909's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L196-L255 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L189-L200 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L227-L232

Vulnerability details

Impact

When the MinterContract.mint function is called it mints new tokens to the user by calling _safeMint (in NextGenCore._mintProcessing) before updating the amount of tokens minted per address associated with the caller, thus a malicious actor can use the ERC721OnReceived callback to reenter into the mint function to mint multiple tokens and bypass the limit per address.

Proof of Concept

The mint function does check if the caller will remain below the minting limit per address after either an allowList or public mint by performing the following checks :

• For allowList mint:

require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit");

• And for public mint:

require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");

Then assuming we are below the max token index of the collection, the function calls gencore.mint (function from NextGenCore) multiple time in the for loop to mint all the tokens to the user:

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
    );
}

The NextGenCore.mint function shown below :

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
        );
        //@audit mints new token before updating the tokens minted amount
        if (phase == 1) {
            tokensMintedAllowlistAddress[_collectionID][_mintingAddress] =
                tokensMintedAllowlistAddress[_collectionID][
                    _mintingAddress
                ] +
                1;
        } else {
            tokensMintedPerAddress[_collectionID][_mintingAddress] =
                tokensMintedPerAddress[_collectionID][_mintingAddress] +
                1;
        }
    }
}

As it can be seen the function after incrementing the circulating supply proceed to mint the token by calling _mintProcessing which under the hood is invoking the _safeMint function, and only after that the amount of token minted per address is increased for the caller.

So this introduces a reentrancy risk where the caller could be a contract that uses the ERC721OnReceived callback to reenter the MinterContract.mint function multiple times to mint new tokens bypassing the minting limit checks mentioned above as its tokensMintedPerAddress amount (either allowList or Public) is only incremented at the end of each call and thus will not affect the overall transaction.

• Attack scenario:

To illustrate this attack let's take the following scenario:

We assume that Bob is using a contract that implements the ERC721OnReceived callback and that he wants to use it to mint tokens from the collection named colA.

We also assume that we are in the public minting period and we have the following :

gencore.viewMaxAllowance(colA) = 10 tokensMintedPerAddress[colA][Bob] = 4

Bob calls the MinterContract.mint function to mint 6 tokens, this will allow him to pass the minting limit check as :

tokensMintedPerAddress[colA][Bob] + 6 = 10 <= gencore.viewMaxAllowance(colA)

Then when the gencore.mint will eventually call _mintProcessing which under the hood invokes _safeMint, the ERC721OnReceived callback will be triggered and in its logic it will call MinterContract.mint function again to mint another token, because tokensMintedPerAddress[colA][Bob] was not yet updated it's still equal to 4 and thus the the minting limit check will not revert.

A new token will be minted to Bob, and let's suppose that he wants to reenter a single time (using a counter in its contract to only execute the reentrancy once) thus his tokensMintedPerAddress[colA][Bob] will now be incremented by 1 resulting in tokensMintedPerAddress[colA][Bob] = 5.

Now that the reentrancy call is over the original call can resume minting the first 6 tokens requested (we are still in the for loop so the minting limit check will not be verified again), and incrementing each time the value of tokensMintedPerAddress[colA][Bob] by 1 resulting in :

tokensMintedPerAddress[colA][Bob] + 6 = 5 + 6 = 11

As the minting limit limit was checked the first time and not updated, we see that Bob was able to mint more tokens and is now above the minting limit set for the collection.

It should also be noted that in this scenario it was supposed that Bob wanted only to reenter once (using a specific logic in the ERC721OnReceived callback) but this can be done multiple times even minting the entire collection, provided Bob can cover the total minting cost.

Even though the given scenario is illustrated in public period, the same attack can be conducted during the allowList period as both use the same logic.

Important: this attack is not possible for the collections with a periodic sale model salesOption == 3 as in that case only one token can be minted per period and thus mint call will revert in case of minting multiple tokens, but it is feasible for all other sale models

Tools Used

Manual review

Recommended Mitigation Steps

To avoid these attack i recommend to update the amount of token minted per address before calling _mintProcessing which ensure that the limit checks will not allow reentrancy.

The NextGenCore.mint function can be modified as follows :

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
    ) {
         //@audit update amount of tokens minted before calling _safeMint
        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