Shaolin Album Tokenization

The purchaseWithUSDC function lacks an onlyLive modifier, therefore mints can still occur with USDC as a payment token when the contract is disabled.

Add an onlyLive modifier to the purchaseWithUSDC function.

PleasrDAO Team: The issue was resolved in commit bb5c069.

The Token contract implements the ERC2981 standard to signal royalty fees to be taken when NFT sales are made on NFT marketplaces. However the Token contract is the ERC20 compatible contract, not the ERC721 compatible contract, therefore NFT exchanges which will interact with the DN404Mirror contract will not read the royaltyRecipient and royaltyFee that are configured in the Token contract.

As the team currently does not planning on utilizing the royalty feature, the severity of the omission is limited.

Create a contract which inherits DN404Mirror and implements the ERC2981 standard with the royalty configurations. Consider adding a function to update the bps for future-proofing.

PleasrDAO Team: The issue was resolved in commit 63e2585.

The Token contract implements a contractURI function which is intended to include collection level metadata about the ERC721 counterpart of the DN404 pair.

However, unlike the tokenURI, the contractURI cannot be queried from the DN404Mirror contract and therefore this metadata is not available for integrators who would query the ERC721 compatible contract for it.

Create a contract which inherits from the DN404Mirror contract and adds a contractURI function which uses _readString to query the DN404 base contract, similar to the tokenURI function. Then override the dn404Fallback function to add the corresponding selector functionality for a _contractURI function.

PleasrDAO Team: The issue was resolved in commit 7d1d3b7.

The DN404 contract has a public setSkipNFT function where the msg.sender can assign a true or false skip status to themselves.

A malicious actor may abuse this functionality to place bids on listed NFTs which would gas grief the lister upon acceptance with the following steps:

• Call setSkipNFT to set their skip status to true.
• Accumulate many ERC20 DN404 tokens by minting through the token contract, but no NFTs since

they have a skip status of true.

• Call setSkipNFT to set their skip status to false.
• Place bids on listed DN404 NFTs, such that if they are accepted the lister would have to expend a

significant amount of gas to mint the NFTs corresponding to the accounts pre-existing ERC20 balance.

This can result in an unexpected loss of funds for the lister through gas expenditure, or even allow for the creation of bids which cannot be accepted as their execution cost would exceed the block gas limit.

Be aware of this risk and document it for users. Consider overriding and disabling the setSkipNFT function to remove this potential griefing vector.

PleasrDAO Team: The issue was resolved in commit cd3cd28.

Users can buy Album NFTs with ETH or USDC based on their preference. These token prices are predetermined (0.004 ETH or 15 USDC) and immutable. However, since this is a long term project, there will definitely be significant price movements in terms of ETH/USDC. Users will always choose to buy with the lower price.

None of the users will buy with ETH when the ETH price increases in the long term and the protocol will still get $15 per token in that scenario. However, in the other scenario when ETH price goes down, users will buy with ETH at a much cheaper price. Even if ETH goes to $2500, NFT price per token will be $10 and it is 33% discount in expected sale price.

One option is giving the owner the right to arrange prices based on market movements. The other option is determining a minimum token price in terms of USD value, and charging users at least corresponding amount of ETH if it requires more than 0.004 ETH.

Consider choosing an option based on the protocol’s intentions since the former increases the owner power and the latter requires an oracle implementation and increases complexity.

PleasrDAO Team: Acknowledged.

In contractURI, the field for external_url should be named external_link instead to conform with ERC-7572 (https://eips.ethereum.org/EIPS/eip-7572). Additionally, ERC-7572 requires an event ContractURIUpdated, which is currently not implemented.

This is also the standard that Opensea uses, see metadata. Not adhering to ERC-7572 could result in improper display of information on secondary marketplaces like Opensea where NFTs are traded. It should be noted however that in tokenURI, external_url is the correct naming.

Rename the media field from external_url to external_link. Create a separate update function for contractURI and emit the event ContractURIUpdated.

PleasrDAO Team: The issue was resolved in commit 1cf921e.

ERC20 tokens are non transferable when the contract is deployed. This is enforced with the transferable flag, preventing _transfer and _transferFromNFT to be executed when the from address is not the zero address, to allow token minting.

The DN404 contract does not validate if the from address is the zero address in the transferFrom function. Therefore, users may call the function as follows: transferFrom(address(0), BOB, 0).

The call will not revert, the transferable condition is circumvented, and a Transfer event will be emitted. Although there is no impact on user's balance, this is an unexpected behavior which may trick off-chain services.

Consider overriding the DN404 transferFrom function to add the zero address check on the from address.

PleasrDAO Team: The issue was resolved in commit 5b49193.

Both purchase() and purchaseWithUSDC() do not validate if nftAmount_ parameters is greater than zero. Even though there is no change in the contract state, there will be unexpected events emitted like Transfer, IntervalsReduced and Minted.

Furthermore, purchases with zero amounts are possible before startTime and after endTime due to modifier checkAndUpdateReducedIntervals calculating currentIntervalsLeft as zero instead of reverting. Again, although there is no impact of contract state, there could be unexpected behavior with off-chain monitoring systems due to this issue.

Require that nftAmount_ parameter is non zero in both purchase functions. Also, in checkAndUpdateReducedIntervals revert instead of returning zero if before or after minting period.

uint256 currentIntervalsLeft = block.timestamp < startTime ||
            block.timestamp >= endTime
            ? revert BeforeAfterMintingPeriod()
            : _initialIntervals - ((block.timestamp - startTime) / interval);

PleasrDAO Team: The issue was resolved in commit 883d02c.

Users can purchase NFTs with ETH or USDC, where purchase() is a payable function in charge of receiving ETH for tokens. It validates if a user has sent enough funds with the checkPrice modifier. In case a user sends more ETH than the NFT value, these funds will not be refunded, and will be sent to the fundsRecipient instead.

Consider adding a refund function where the excess ETH is first sent to the user and the remaining sent to the funds recipient.

PleasrDAO Team: Acknowledged.

User purchasing tokens with USDC will call purchaseWithUSDC(). This function will collect the USDC from the user and transfer it to the fundsRecipient.

The USDC token in Base has a blacklist functionality. In case fundsRecipient address gets blacklisted, purchaseWithUSDC() will revert for all users. Due to the fact that there is no way to update the fundsRecipient, users will only be able to buy tokens using ETH.

Consider adding an admin function to update the fundsRecipient address.

PleasrDAO Team: Acknowledged.

The checkAndUpdateReducedIntervals function uses round down division to reduce the currentIntervalsLeft by the intervals that have passed. Therefore the final interval in the mint period can be less than 15 minutes, however the Mint event assumes that an entire interval of time was removed from the countdown.

Consider the following scenario:

• Intervals are 10 seconds
• The period is 100 seconds in total
• Time is currently at 95 seconds
• currentIntervalsLeft = 10 - 95 / 10 = 1
• Bob mints the last interval, technically this only removes 5 seconds from the countdown, but the

Mint event emits that it took off the entire 10 seconds

This will misinform consumers of the Mint event and potentially cause issues with integrating off-chain applications.

Consider using round up division to compute the currentIntervalsLeft, thus not allowing for partial interval mints.

PleasrDAO Team: Acknowledged.

The Token constructor validates the MintConfig params, and initializes the _initialIntervals immutable param with the following formula: _initialIntervals = (mintConfig_.endTime

• -mintConfig_.startTime) / mintConfig_.interval;

This param will be used to calculate the amount of intervals a user can purchase. When interval is not a multiple of the difference between endTime and startTime, the division will round down, and _initialIntervals value will be 1 interval short. Therefore, when a user purchases all intervals available to buy, the countdown will not reach zero.

Consider the following scenario:

• startTime: 100, endTime: 200, interval: 17
• _initialIntervals: 5 (200 - 100 / 17)
• user buys all available intervals at t=100
• final countdown = 15 seconds

endTime - intervalsReduced * interval - block.timestamp = 200 - 5*17 - 100 = 15

Ensure the difference in time between end and start time is an even multiple of interval. Otherwise, consider using a round up division to make sure countdown reaches zero when all available intervals are purchased. Keep in mind this will cause countdown to be negative in some cases.

PleasrDAO Team: Acknowledged.

reduceIntervals() is an important admin function used to reduce intervals without minting NFTs. The initial number of intervals is estimated at 2.1 mil (41 mil minutes / 20 min intervals). Given this large number, admin input error is possible when trying to reduce the number of intervals.

If an incorrect intervalAmount_ input is used, it could greatly reduce the amount of time left for minting in an irreversible way.

Consider some form of input validation, for example by adding a max interval amount check.

PleasrDAO Team: The issue was resolved in commit 7087a65.

The specification document states that update of tokenURI metadata is a mandatory feature. While there is a setMedia function implemented, it does not allow for all tokenConfig fields to be updated.

tokenConfig has a total of 14 fields while setMedia can only update 5 of those fields. Fields such as image and encrypted_media_url are among those excluded and cannot be updated. Allowing these fields to be updated is important for possible features to be added in the future.

Allow setMedia to configure all of tokenConfig fields.

PleasrDAO Team: Acknowledged.

The Minted event lacks an indexed parameter for to address, so off-chain services will not be able to filter them by user address.

Add the indexed to parameter:

event Minted(address indexed to, uint256 amount, uint256 secondsReduced);

PleasrDAO Team: Acknowledged.

While ERC-721 royalties cannot be enforced, royalties could be earned indirectly through fees from staking in Liquidity Pools (LPs), because of DN404's dual nature as an ERC20.

Therefore, the protocol should consider:

1. Setting aside an amount of tokens to create LP
2. Adding admin ability to setSkipNFT for the liquidity pool contract to prevent it from minting NFTs

each time a swap is done so as to improve gas efficiency.

PleasrDAO Team: Acknowledged.

In the Token constructor the royalty recipient parameter is misspelled as royaltRecipient_.

Correct the royaltRecipient_ to royaltyRecipient_.

PleasrDAO Team: The issue was resolved in commit 63e2585.

The InvalidMint custom error is defined but never used.

Remove unused custom error.

PleasrDAO Team: The issue was resolved in commit 5b49193.

The string memory parameters for the setMedia function are never mutated and therefore can be declared as calldata.

Convert the string memory parameters to string calldata parameters.

PleasrDAO Team: The issue was resolved in commit a8700e5.

In the Token contract the startTime, endTime, and interval storage variables are assigned to only once in the constructor and are never reassigned, therefore they can be declared as immutable.

Declare the startTime, endTime, and interval storage variables as immutable.

PleasrDAO Team: The issue was resolved in commit e97717d.

DN404 and Mirror contracts are synced with _initializeDN404, and the owner of the DN404 is updated with _initializeOwner in the constructor. However, this action do not update the owner of the Mirror contract.

After the initialization of these contracts, the owner of the Mirror contract is still address(0) even though the owner of the DN404 is the owner. Anyone can call the pullOwner function and sync them, but there will be a mismatch until this function is called.

Call the pullOwner function in the constructor as a last step after calling _initializeDN404.

PleasrDAO Team: The issue was resolved in commit 43c8bc5.

Throughout the Token contract snake case is used to represent the names of url media fields, however the animation url field is named animationURL, which does not follow the snake case standard for url fields.

Consider if the animationURL field should be renamed as animation_url. Additionally, ensure the other fields have the appropriate expected formatting.

PleasrDAO Team: The issue was resolved in commit c7c9871.

According to protocol specs, the purchase function “Should try to mint max to, or up to that much if there is less supply left”.

However, the function reverts when the requested amount is more than the remaining supply. This might cause a big purchase to revert when there are still tokens to be minted, and the user may lose their chance while trying again with a lower amount due to race condition if the demand is high.

Consider updating the function in a way to mint the remaining supply or document this behaviour.

PleasrDAO Team: Acknowledged.

_tokenURI function uses NFTMetadataRenderer library to create the metadata for NFTs. According to this library, last parameter of the tokenURIMetadata function should be the size of entire edition. However, the function is called with the value 0 and the metadata is not rendered with the true collection size as expected.

This may be the expected behavior, however if it is not use the current NFT supply as the size while rendering metadata, with the knowledge that this size can fluctuate.

PleasrDAO Team: Acknowledged.

Users can climb the leaderboard by minting Album tokens and will get incentives based on their leaderboard ranking. The leaderboard is determined solely based on the mintCount.

mintCount is only updated when a user purchases and mints directly but it is not updated after transfers or secondary sales. This is done to prevent gaming like buying a lot of tokens from secondary market just before the sale ends.

A user could potentially mint many tokens, sell them all, and use the proceeds to mint more tokens in order to move up the leaderboard while only expending a limited amount of initial capital. This risk is however not applicable if token transfers are disabled during the entire period of the sale.

Be sure to keep token transfers disabled during the entire period of the sale.

PleasrDAO Team: Resolved.