When new drops are created from the factory, there is an optional argument approvedMinter to grant the MINTER_ROLE.
All minting functionality uses the onlyMinterOrAdmin modifier, which effectively implements the spec, since the MINTER_ROLE is optional, as long as the admin calls the function.
However, when calling NFTDropMarketFixedPriceSale.sol:createFixedPriceSale(), the MINTER_ROLE is required and the function reverts if one hasn't been supplied.
Proof of Concept
When NFTCollectionFactory.sol:createNFTDropCollection() is called, the approvedMinter argument is marked as an optional way to add grant additional access to the MINTER_ROLE. This argument is passed to _createNFTDropCollection(), which passes it to collection:initialize().
In initialize, the function checks if approvedMinter was included, and initializes the role if so. Otherwise, it doesn't leaves the role as-is:
AdminRole._initializeAdminRole(_creator);
if (_approvedMinter != address(0)) {
MinterRole._initializeMinterRole(_approvedMinter);
}
Later, in NFTDropMarketFixedPriceSale.sol:createFixedPriceSale(), there are a number of checks to confirm that the sale is allowed to be listed. One of those checks explicitly looks for the MINTER_ROLE:
if (!IAccessControl(nftContract).hasRole(MINTER_ROLE, address(this))) {
revert NFTDropMarketFixedPriceSale_Mint_Permission_Required();
}
This check will revert for any drop that hasn't explicitly set the MINTER_ROLE by including the approvedMinter argument in the original call.
Tools Used
VS Code
Recommended Mitigation Steps
The NFTDropMarketFixedPriceSale.sol:createFixedPriceSale() function does not need to check for the MINTER_ROLE, because it's already checking for DEFAULT_ADMIN_ROLE, which implies the ability to mint.
Alternatively, you could set the MINTER_ROLE to the contract creator in the initialize() function if an approvedMinter isn't explicitly set:
Lines of code
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L146 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L148-L151 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L136-L138
Vulnerability details
Impact
When new drops are created from the factory, there is an optional argument
approvedMinter
to grant theMINTER_ROLE
.All minting functionality uses the
onlyMinterOrAdmin
modifier, which effectively implements the spec, since theMINTER_ROLE
is optional, as long as the admin calls the function.However, when calling
NFTDropMarketFixedPriceSale.sol:createFixedPriceSale()
, theMINTER_ROLE
is required and the function reverts if one hasn't been supplied.Proof of Concept
When
NFTCollectionFactory.sol:createNFTDropCollection()
is called, theapprovedMinter
argument is marked as an optional way to add grant additional access to theMINTER_ROLE
. This argument is passed to_createNFTDropCollection()
, which passes it tocollection:initialize()
.In
initialize
, the function checks ifapprovedMinter
was included, and initializes the role if so. Otherwise, it doesn't leaves the role as-is:Later, in
NFTDropMarketFixedPriceSale.sol:createFixedPriceSale()
, there are a number of checks to confirm that the sale is allowed to be listed. One of those checks explicitly looks for theMINTER_ROLE
:This check will revert for any drop that hasn't explicitly set the
MINTER_ROLE
by including theapprovedMinter
argument in the original call.Tools Used
VS Code
Recommended Mitigation Steps
The
NFTDropMarketFixedPriceSale.sol:createFixedPriceSale()
function does not need to check for theMINTER_ROLE
, because it's already checking forDEFAULT_ADMIN_ROLE
, which implies the ability to mint.Alternatively, you could set the
MINTER_ROLE
to the contract creator in theinitialize()
function if anapprovedMinter
isn't explicitly set: