DN404

Proof of concept: PoC

The _mintNext function ignores the burn pool tokenIds, and mints directly starting from the existing totalSupply / unit() + 1 id.

However this id and the ids that follow can be within the burn pool. As a result burn pool NFTs will be minted without adjusting the burn pool head.

Therefore when attempting to mint regularly, these newly minted NFT ids will be duplicated in the toOwned mapping and overwritten in the oo entry, effectively stealing this NFT from the user it was originally minted to with the _mintNext function.

Do not allow the burn pool feature to be used in tandem with the _mintNext function, otherwise refactor the _mintNext function to account for the burn pool.

DN404 Team: The issue was resolved in PR#136.

Proof of concept: PoC

In the _mintNext function it is possible for the minted ERC721 IDs to wrap around unexpectedly, causing several potential issues for systems inheriting the DN404 contract. Consider the following scenario:

• User A has a balance of 3.6 erc20s
• Existing totalSupply is 42.55
• We _mintNext 74.43 tokens

The _mintNext function will wrap the final minted ERC721 id with the _wrapNFTId function, this is because we are attempting to mint 75 nfts to the receiver's address, however the maxId has only increased by 74. This is because:

For User A: 3.6 + 74.43 = 78.03 => +75 ERC721s for User A For totalSupply & maxId: 42.55 + 74.43 = 116.98 => + 74 ERC721s allowed by maxId.

As a result, the _mintNext function can unexpectedly mint ERC721 IDs that are a part of the burn pool, and burn pool NFTs will be minted without adjusting the burn pool head. Therefore when attempting to mint regularly, these newly minted NFT ids will be duplicated in the toOwned mapping and overwritten in the oo entry, effectively stealing this NFT from the user it was originally minted to with the _mintNext function.

In the _mintNext function revert if the toAddress would receive more ERC721s than the maxId increase would allow, e.g. _zeroFloorSub(t.toEnd, toIndex) > (totalSupply_ / _unit()) - preTotalSupply.

This behavior is also present in the _mint and _transfer functions, however there are no assumptions broken by this edge case for these functions. Therefore no code changes are necessary in these functions. Consider documenting this edge case behavior for these functions for users.

DN404 Team: The issue was resolved in PR#136. 14

Proof of concept: PoC

Whenever a mirror ERC721 token is transferred, minted, or burned through ERC20 transfers, the order in which the mirror tokens are taken from the holder is LIFO (last in first out). This behavior can be abused in certain circumstance by a malicious actor to steal user funds when interacting with NFT marketplaces.

The exact situation happens when a user that has a bid on a mirror NFT also sells a unit or more of base tokens from the same wallet. In that case, a malicious holder of the mirror NFT can make a profit and leave the user without the rare NFT.

Attack scenario using the live Asterix collection as an example:

• Bob has one of the rarest NFTs, 1960 and waits for people to bid on it
• At this point, Alice bids on it for 1.2938 WETH
• The floor for the collection is 0.674 ETH and buying an NFT by buying the base tokens from the liquidity pool is 0.5671 ETH.
• Alice has another NFT, lowest rarity, and sells by selling a unit of base tokens
• Since the collection is on Ethereum, the base swap transaction can be seen in the mempool
• Bob sees the base unit sell and front-runs it with accepting Alice’s bid
• Alice gets the rare 1960 ID and pays 1.2938 WETH, but immediately loses it as it was the last in and the base unit sell burns it, marking her a loss of 1.2938 - 0.5671 ETH
• Bob quickly initiates several cycled mints/burns to reclaim the rare NFT

Even if Bob fails to reclaim it, Alice still suffered a loss of the rare NFT. The above case can also happen unintentionally, when a user bid is accepted exactly in the same block as him selling base tokens equivalent to a NFT, having the same financial loss.

Consider modifying the synchronization logic to that of a FIFO (first in first out) instead of LIFO. Meaning that the first NFT to be minted to the wallet is also the first to leave it. Otherwise consider clearly documenting this risk for integrators.

DN404 Team: Acknowledged.

Proof of concept: PoC

Contracts that extend DN404 can implement the _afterNFTTransfer hook to be executed after any NFT token transfers, including minting and burning. An attacker can abuse any implementations that pass access to holders from within these hooks as there are situations when the _afterNFTTransfer function is called before the internal storage is committed, breaking CEI.

Consider the following attack scenario in a system implementing the _afterNFTTransfe hook:

• An approver initiates a transfer to a user where direct transfers will be made
• In the _afterNFTTransfer function the malicious attacker has an existing helper contract that directly transfers several ERC721 tokens to himself
• Since this was done before the balanceOf equivalent was updated, after the initial execution is finalized, the malicious strategy will have a smaller ERC721 balance then the number of ERC721 it owns
• As the attacker has less NFTs then they should by the ownedLength variable and because this ownedLength is used in determining how many NFT a user will have for their base token amount, the attacker can transfer any amount, even 0, to himself and the contract will mint him extra ERC721 tokens, the exact number that was sent by the helper contract

At this point, an attacker owns double the ERC721 tokens that he received, for half the amount of ERC20 base required to own that many.

Completely move the _afterNFTTransfer into its own separate loop in all cases except the call from _transferFromNFT which is already singular. In the particular case of direct transfer, also move it after setting the ownedLength.

DN404 Team: The issue was resolved in PR#136.

Upon the first interaction with an address the address data flags are initialized, checking to see if the address holds any bytecode. If the account does not hold any bytecode at the time of account initialization then it receives only the _ADDRESS_DATA_INITIALIZED_FLAG flag.

However if the address is later deployed to (e.g. a new pool is created at this address), the contract will not be excluded from ERC721 minting, as it will not automatically receive the _ADDRESS_DATA_SKIP_NFT_FLAG flag.

As a result malicious actors may transfer tokens to an address where a pool for the token is about to be deployed to in order to avoid getting the address marked with the _ADDRESS_DATA_SKIP_NFT_FLAG. NFTs will then errantly be minted to and burned from the pool address upon liquidity modification and swaps.

This creates a pool of NFTs which are not owned by end users, but instead locked up in a swap pool where a significant amount of them may not be able to move due to locked liquidity. In the case of a swap pool it would be possible for a flashloan to rescue potentially rare ERC721 tokens, however in the case of a locking contract or some other arbitrary integration these ERC721 tokens could be unintentionally locked for a significant amount of time.

Consider checking whether an account houses bytecode when reading the getSkipNFT function, regardless of if the account has been initialized or not. Otherwise be sure to document this risk to users of DN404, advising them to implement their own _getSkipNFT functions or adding their own functionality for trusted addresses to mark addresses with the _ADDRESS_DATA_SKIP_NFT_FLAG as needed.

DN404 Team: The issue was resolved in PR#136.

When a tokenURI function is called in the fallback of the DN404 contract, it is never checked whether the given tokenId exists. According to the recommendation in EIP712, the tokenURI function should throw an error if the tokenId is not a valid NFT. This recommendation is followed in the OpenZeppelin ERC721 implementation contract as well as the ERC721A implementation.

Check if the given tokenId exists; if it is invalid, revert with a custom error.

DN404 Team: The issue was resolved in PR#136.

The documentation for the CannotLink error states that this error is thrown when "linking to the DN404 base contract and the DN404 supportsInterface check fails or the call reverts". However while linking the Mirror contract with the linkMirrorContract(address) function selector there is no validation that the msg.sender is indeed a valid DN404 implementation.

Implement the appropriate validation when executing the linkMirrorContract logic such that implementsDN404() function selector is invoked on the purported DN404 contract to ensure that it is a valid implementation.

DN404 Team: The issue was resolved by removing the CannotLink error in PR#136.

Currently, during the transfer of tokens, there is a safeTransfer mechanism where the to address is checked if it implements the onERC721Received function. However, this safe mechanism does not exist during minting. Therefore, it is possible to mint a token to a smart contract that does not support ERC721, resulting in the token becoming permanently stuck.

Implement _safeMint function with the same safe mechanism as in the safeTransferFrom functions. If the to address is a smart contract, check if it implements the onERC721Received function.

DN404 Team: The issue was documented in PR#136.

When the DN404 contract is deployed, an initial supply amount and holder address can be passed. If they are provided, the internal logic within the contract will automatically set the corresponding address to skip NFT minting, regardless if it is an EOA or smart contract.

This behavior is not stated and cannot be intuitively considered since the skip NFT logic, as suggested by the EIP and implemented, is implemented to by default skip only smart contracts, where as here any provided address is set as so.

As a result external integrators expecting that by sending the initial ERC20 base tokens to an EOA to have the ERC721 minted as well to it are mislead.

Either clearly document this behavior or change the implementation of DN404._initializeDN404 such that it accepts a skipNFT parameter and mints the mirror contract tokens accordingly.

DN404 Team: The issue was documented in PR#136.

In EIP-7631, if the ERC20 base contract implements the IERC7631BaseNFTSkippable interface there are certain considerations that must be upheld. As specified in the interface, the SkipNFTSet event must be “Emitted when the skip NFT status of owner is changed by any mechanism” with the addition that the “initial skip NFT status for owner can be dynamically chosen to be true or false, but any changes to it MUST emit this event”.

In the DN404 implementation, the SkipNFTSet event is incorrectly emitted both when no change is done, by calling setSkipNFT with an already set status, and when setting the status for the initial supply owner, when deploying the DN404 token.

Emitting the SkipNFTSet event in situations where no change is done is not compliant with the EIP.

In the DN404._setSkipNFT function, move the event emission assembly block within the if branch. By doing so, the _setSkipNFT call from the DN404._initializeDN404 will also not emit the event.

If the _setSkipNFT alteration is not implemented, in DN404._initializeDN404 instead of calling _setSkipNFT, directly set the _ADDRESS_DATA_SKIP_NFT_FLAG flag to true, to avoid event emission.

DN404 Team: Acknowledged.

There are 2 functions that provide minting functionality for the ERC20 and ERC721 balances:

• _mint: mints IDs from the burn pool if using the feature; If none available, uses nextTokenId
• _mintNext: will always mint the next ID outside of the existing supply

An issue appears when a contract uses both mint functions to mint tokens and NFTs as the _mintNext function does not update the nextTokenId which indicates the next free token ID.

Consider the following scenario:

• For an implementing contract, transfers are paused until all the base tokens are sent, meaning the collection is minted
• Users mint using the _mintNext function, which does not update the nextTokenId
• After a large number of mints, the contract changes minting, for whatever reason, to using the _mint function
• _mint starts validating ownership from the nextTokenId token ID, and since it was never set, if a large enough amount of NFTs were minted, this operation will consume a large amount of gas or even revert with OOG in extreme cases before finding the next free ID.

Consider if this behavior should be allowed, if so clearly document this risk so integrating protocols can avoid this scenario.

DN404 Team: Acknowledged.

The DN404 implementation co-joins ERC20 and ERC721 standards representing dual nature token pair. Both tokens are deployed separately and then linked. The main idea of solution is to adjust both tokens balances, in such a way that the _unit represents an amount of ERC20 token balance that is equal to one NFT.

Thus, the balances are being updated within every burn, mint or transfer operation. Whenever significant amount of tokens are processed, that result in multiple mints or burns, a significant amount of gas is consumed. Eventually, such transactions are prone to revert due to Out Of Gas error, preventing successful execution of aforementioned operations.

Although this is mentioned as a possibility in the EIP7631 itself, it should be mentioned that the current implementation fails at around 2,600 NFTs to be minted. If the unit is low enough, complex integrators, swaps, lenders, will also fail.

Consider documenting these limitations, especially when it comes to the choice of a unit value.

DN404 Team: The issue was documented in PR#136.

DN404 integrators have the possibility to define whether Permit2 has infinite allowances by default for all owners, by means of the _givePermit2DefaultInfiniteAllowance internal function.

Additionally, when a user decides to overwrite the default allowance for Permit2 the _ADDRESS_DATA_OVERRIDE_PERMIT2_FLAG is set to remember the users choice of a custom allowance set.

However, the flag is only set when the _givePermit2DefaultInfiniteAllowance function returns true. Integrators may have implementations that change the value returned in the aforementioned function over time.

In such a case, the user may firstly set custom allowance for Permit2. Such action will not set on the _ADDRESS_DATA_OVERRIDE_PERMIT2_FLAG flag, as the _givePermit2DefaultInfiniteAllowance function by default returns false.

Subsequently, changing the behavior of the _givePermit2DefaultInfiniteAllowance function will overwrite user's custom allowance into maximum allowance. This behavior can be considered unexpected and may be leveraged by the attacker in further attacks.

Clearly document this risk that arises from a non-static _givePermit2DefaultInfiniteAllowance value.

DN404 Team: The issue was documented in PR#136.

When minting using the _mintNext function, IDs are chosen outside of the current NFT supply as distinctly new NFTs are created. However it is possible to mint NFTs with an ID that is above the computed totalNFTSupply retrieved by the totalSupply() / _unit() calculation.

This corner case appears because the index where the _mintNext function starts looking for available IDs is chosen as if all the possible NFTs would of been minted up to this point startId = preTotalSupply / _unit() + 1 and the wrapping function is not triggered due to the ID being free of an owner.

This creates odd situations, for example if the unit is 100 ERC20 tokens and 90 ERC20 tokens are minted to 2 addresses each as well as 20 ERC20 tokens to a 3rd address. Now, when initiating a mint of 10 base tokens to any of the first addresses, the ID minted will be 3, however the totalSupply() / _unit() = 210 / 100 = 2 indicates that only the IDs 1 and 2 ought to exist.

When implementing a fix for H-01, be sure to not allow the ID of minted NFTs with the _mintNext function to surpass the totalSupply / _unit().

DN404 Team: The issue was resolved in PR#136.

Whenever a mirror ERC721 token is transferred/minted/burned as a result of transferring enough base ERC20 tokens, the order in which the mirror tokens are taken from the holder is LIFO (last in first out). This behavior has a high impact on users and is virtually unmentioned.

Additionally, as the current draft of EIP-7631 does not cover the token synchronization logic it needs to be explicitly stated.

Consider a situation where, out of on the Nth base token transfer, a user mints a rare ERC721. Since they are only interested in the rare token, they send their entire base balance, minus enough for a single unit, to another address and by doing so actually lose the rare NFT.

Although this is a design decision, since it has severe economical implications to market participants it must be explicitly stated. Another solution is to add a mechanism to specify which NFTs you wish to have locked-in when transferring base tokens.

Users can currently workaround this by transferring NFT directly to a different (clean) wallet. Any subsequent base ERC20 transfer to it will leave the rare NFT within the wallet of the user provided that a minimum unit value is always kept.

DN404 Team: The issue was documented in PR#136.

The DN404 implementation co-joins ERC20 and ERC721 representing dual nature token pair. Both tokens are deployed separately and then linked. The processing of both tokens is being done simultaneously whenever a transfer, mint or burn is triggered for one of them.

However, the same does not occur for ERC20 and ERC721 token allowances. Whenever one of the token allowance is granted or revoked, the other remain unchanged. This behavior appear to be secure whenever allowance increases or grant is done.

When considering the opposite situation, whenever allowance is decreased or revoked, it can be prone to human errors. In the event of emergency, when one of the contract allowance is removed, the second will remain active, and might be leveraged by an attacker in a vulnerable scenario.

Consider implementing additional functionality, that sets both the ERC20 allowance to 0 and and the ERC721 operator allowance is revoked.

DN404 Team: The issue was documented in PR#136.

The setSkipNFT function allows the token holder to decide whenever the NFT should be minted upon increasing their base ERC20 tokens balance. While this functionality may have impact on Gas consumption and gives some flexibility, it has significant drawback.

A user can leverage this to burn owned NFTs and decrease the totalSupply by transferring base ERC20 tokens to secondary account which has this flag set to true. As a result the NFT tokens will be burned, but not minted for destination account.

This implementation may have impact on third party integration, e.g. if marketplace will base the NFT token value on total supply.

Clearly highlight the indicated behavior so that any integrating 3rd party does not unknowingly integrate expecting the totalSupply invariant to hold.

DN404 Team: The issue was documented in PR#136.

A user can allow an operator to manage the tokens of their mirror ERC721 tokens by calling the standard setApprovalForAll function.

This function is usually gated to not allow approval to be given to the zero address. In the current DN404Mirror implementation, zero address operator is allowed.

Validate that the operator passed to the _setApprovalForAll function is not the zero address.

DN404 Team: The issue was documented in PR#136.

In the documentation for the _findFirstUnset function it is mentioned that “If no set bit is found, returns type(uint256).max”.

However the behavior of the _findFirstUnset function is such that if no unset bit is found, the type(uint256).max is returned.

Correct the comment to reflect the behavior: “If no unset bit is found, returns type(uint256).max”.

DN404 Team: The issue was resolved in PR#136.

In the documentation for the DN404NFTStorage struct, the comment about the deployer “...link can only be done be the deployer via the ERC20 base contract”.

Should read “...link can only be done by the deployer via the ERC20 base contract”.

Replace the second instance of "be" with "by".

DN404 Team: The issue was resolved in PR#136.

In the DN404 contract the public approve function can be called before the DN404 contract has been initialized and connected to a corresponding mirrorERC721 contract.

While this poses no immediate risk it may be unexpected for users of the DN404 contract that approvals can be made and approval events can be emitted before the DN404 contract is initialized.

Consider adding validation that the mirrorERC721 contract is nonzero in the _approve function and revert with the DNNotInitialized error if it is.

DN404 Team: Acknowledged.

Address aliases are used to track ownership in bytes32 size entries in the oo mapping. In the _registerAndResolveAlias function, the resulting addressAlias is a monotonically increasing number. When type(uint32).max = 4,294,967,295 address aliases have been assigned, attempting to assign another alias will revert.

There are roughly 8,000,000,000 humans on earth, notice that 8,000,000,000 > 4,294,967,295. As a result DN404 tokens cannot reach 100% saturation of the human race, limiting their TAM.

Consider documenting this limitation for users of DN404 expecting to convert humanity into a hive-mind of DN404 token holders.

DN404 Team: Acknowledged.

In the DN404 contract the documentation for the _initiateTransferFromNFT function indicates that the “Call must originate from the mirror contract”. However there is no in-code validation that specifically requires this to hold.

Additionally, there is no dash preceding the “msgSender must be the owner of the token, or be approved to manage the token” requirement, therefore this requirement does not match the format of the others.

Either remove the documented requirement that the “Call must originate from the mirror contract” or implement this validation at the Smart Contract level.

Additionally add a dash preceding the “msgSender must be the owner of the token, or be approved to manage the token” requirement so that it matches the formatting of other requirements in the docstring.

DN404 Team: The issue was resolved in PR#136.

The deployer address in the Mirror contract can be immutable as it is never changed and is only used during the linking of the DN404 contract with the Mirror contract.

Make the deployer address immutable.

DN404 Team: Acknowledged.

In the DN404 contract the documentation for the _totalSupplyOverflows function indicates that it: “Returns whether amount is a valid totalSupply”.

However, the function returns whether the amount is not a valid total supply, specifically it returns true if it does not satisfy the required conditions.

Correct the comment to reflect the behavior: Returns whether amount is an invalid totalSupply.

DN404 Team: The issue was resolved in PR#136.

In the DN404 contract users may override the unit function in order to specify the denomination of ERC20 tokens that constitutes the ownership of 1 ERC721 token. There is no requirement nor documentation which specifies that users ought to keep the unit value constant, however significant risks arise when the unit value is changed.

Most notably, if the unit value is increased there is a risk that all ERC721 IDs from 1 up to the newly computed maxId are occupied when minting a new NFT. This scenario will result in an infinite loop while the _mint or _transfer function attempts to find an unoccupied ERC721 ID within the valid range, and as a result minting and transfers which would result in a mint are DoS’d.

Clearly document that the unit value should remain constant, similar to how is done for the _useExistsLookup function.

DN404 Team: Acknowledged.