Function _mintProcessing() has been used in mint() and airDropTokens() and
both doesn’t follow check-effect-interaction pattern and code updates the
values of tokensAirdropPerAddress, tokensMintedAllowlistAddress and
tokensMintedPerAddress variables after making external call by using
safeMint(). This would give attacker opportunity to reenter the Minter
contract logics and perform malicious action while contract storage state
is wrong.
Attacker can perform this action:
This is mint() code in NextGenCore contract:
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-issue
if (phase == 1) {
tokensMintedAllowlistAddress[_collectionID][_mintingAddress] =
tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
} else {
tokensMintedPerAddress[_collectionID][_mintingAddress] =
tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
}
}
}
_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); // @note callback hook
}
As you can see code would make external call to onERC721Received() function
of the account address by calling _safeMint() and the code only sets the
values for tokensMintedAllowlistAddress and tokensMintedPerAddress after
this call. so code don’t follow check-effect-interaction pattern and it’s
possible to perform reentrancy attack. there could be multiple scenarios
that attacker can perform the attack and do damage. e.g:
scenario #1 where attacker bypasses limit and mints possibly ALL
collection’s totalSupply (as shown above)
scenario #2 where attacker could execute a read-only reentrancy on the
retrieveTokensAirdroppedPerAddress() function if any integrated art company
rely on it’s returned value (as shown [here](<https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/NextGenCore.sol#L183))
Visual Studio Code
follow the check-effect-interaction pattern or add a reentrancy guard.
Reentrancy
The text was updated successfully, but these errors were encountered:
All reactions