SignatureDrop: using DropSinglePhase

[nkrishang]

No description provided.

[jakeloo]

probably doesn't need to be indexed.
single phase = index always 0?

[jakeloo]

we should probably inform people that this is vulnerable to signature frontrunning attack

[nkrishang]

Will add this consideration to the design document.

[jakeloo]

https://github.com/chiru-labs/ERC721A/blob/main/contracts/ERC721A.sol#L742-L744 is not overriden

[jakeloo]

contracts/contracts/signature-drop/SignatureDrop.sol

Lines 96 to 110 in 7f575f5

	_setupRole(DEFAULT_ADMIN_ROLE, _msgSender());
	setContractURI(_contractURI);
	setOwner(_defaultAdmin);
	_setupRole(DEFAULT_ADMIN_ROLE, _defaultAdmin);
	_setupRole(MINTER_ROLE, _defaultAdmin);
	_setupRole(TRANSFER_ROLE, _defaultAdmin);
	_setupRole(TRANSFER_ROLE, address(0));
	setPlatformFeeInfo(_platformFeeRecipient, _platformFeeBps);
	setDefaultRoyaltyInfo(_royaltyRecipient, _royaltyBps);
	setPrimarySaleRecipient(_saleRecipient);
	_revokeRole(DEFAULT_ADMIN_ROLE, _msgSender());

_setupRole docstring:

This function should only be called from the constructor when setting up the initial roles for the system. Using this function in any other way is effectively circumventing the admin system imposed by AccessControl.

i think this setup is a little weird and wasteful. basically with _setupRole, you don't need to be an admin to do so.

Other thought, could we have _setupX internal functions for our setters to address the same problem?

[jakeloo]

make a comment here that _burn internally check for approvals from erc721a

[nkrishang]

Will add consideration in a code comment + design document.

[nkrishang]

https://github.com/chiru-labs/ERC721A/blob/main/contracts/ERC721A.sol#L742-L744 is not overriden

We were using erc721a-upgradeable v3.3 where this _msgSenderERC721A did not exist. Will upgrade to the latest erc721a-upgradeable v4.0 where _msgSenderERC721A does exist, and will override it.

[nkrishang]

https://github.com/chiru-labs/ERC721A/blob/main/contracts/ERC721A.sol#L742-L744 is not overriden

We were using erc721a-upgradeable v3.3 where this _msgSenderERC721A did not exist. Will upgrade to the latest erc721a-upgradeable v4.0 where _msgSenderERC721A does exist, and will override it.

Update: we'll not be upgrading to erc721a-upgradeable v4.0, and will continue using v3.3 (which we started with). The version upgrade has to do with changes in the storage layout of the ERC721A contract, and introduces breaking changes that make using ERC721AUpgradeable incompatible with OpenZeppelin's Initializable.