ANIME Claimer, Part 2

In the _checkNFTClaim function the owner of the tokenId is checked with the ownerOf function, however this function will often revert for many NFT collections and thus cause a DoS for the OAPP preventing all subsequent reads.

This occurs because the DVNs cannot process the read request, producing an error of UnresolvableCommand which prevents the DVN from verifying their response for the nonce.

When DVNs have not verified the response for a nonce, validation in the EndpointV2 contract prevents any subsequent messages for the OApp from being processed in lzReceive:

https://github.com/LayerZero-Labs/LayerZero-v2/blob/7da76840e41dc593d3c2007ce35b911b1d8

16b4b/packages/layerzero-v2/evm/protocol/contracts/MessagingChannel.sol#L138

This produces a block of read messages on the chain which the invalid read request was triggered from. The delegate of the OApp can call the skip function on the endpoint in order to skip the affected nonce which cannot be verified and resume lzReceive processing.

However a malicious actor can continue to trigger read requests which will revert and place the OApp in a locked state.

Wrap the call to ownerOf in a try/catch and return false if the ownerOf call reverts as a validation response to indicate that the claim is not valid.

Animecoin Team: Resolved.

Because lzRead receptions can revert initially and be retried at a later time it is possible for malicious actors to steal vested amounts after an NFT has already been transferred to a new user.

For example, consider the following chain of events:

• Alice has Azuki #7 with 10 ANIME tokens vested on day 10
• The day 10 withdrawal limit has already been met on Arbitrum
• Alice intentionally submits a claim request that will fail upon lzReceive due to the withdrawal limit,

but validates Alice as the owner of Azuki #7 up to block.timestamp of day 10, allowing Alice to claim 10 ANIME tokens

• On day 11 Alice lists their Azuki #7 for sale under the pretense of the buyer being able to claim the

10 vested ANIME tokens

• Azuki #7 is sold to Bob on day 11
• Alice retries their lzReceive on Arbitrum by invoking the lzReceive function through the endpoint

themselves. The read result is still in the message channel so this action is allowed.

• Alice claims the 10 vested ANIME tokens that had accrued up to day 10
• Bob purchased Azuki #7 under the pretense that he would also receive these 10 vested ANIME

tokens, however after his purchase Alice took those 10 ANIME tokens from him

Retries of lzRead results can be disabled by simply returning without claiming tokens in the lzReceive function. This would require users to re-submit a requestClaim transaction when their read request fails due to the withdrawal limit, contract pause, etc. However L2 transaction costs + lzRead costs are cheap so this may be acceptable.

Additionally, consider including validation that the withdrawal limit is not met for the current day in the requestClaim function to disallow users from potentially wasting gas + the lzRead fee when this scenario arises. This also allows for a quicker feedback loop on the withdrawal limit error.

Animecoin Team: Partially Resolved.

In the setReadChannel function the active parameter determines whether the channel is being activated or deactivated. However the $.lzReadChannel = channelId; assignment is always made no matter the active value.

Therefore when a read channel is deactivated it is assigned as the active lzReadChannel. This will put the app in an unintentional DoS’d state since the peer is no longer assigned for this channel.

Do not assign the lzReadChannel value if active is false.

Animecoin Team: Resolved.

In the _checkNFTClaim function the following boolean condition is used to determine if the claimer is authorized to initiate the ANIME claim for the nftOwner.

DelegateCheckerLib.checkDelegateForERC721(claimer, nftOwner, nftContractAddress, tokenId) || _checkDelegateV2Rights(claimer, nftOwner)

However the _checkDelegateV2Rights function does not check for ERC721 specific delegations, it checks for global delegations for the nftOwner address.

Therefore if the nftOwner issues an ERC721 token delegation to the claimer address with the specific SUBDELEGATION_RIGHTS_KEY the delegation will not be seen as a valid authorization of the claimer.

Create a new _checkDelegateV2RightsForNft function which uses checkDelegateForERC721.

Animecoin Team: Resolved.

The setNFTToUUID function allows the owner to update the nftToUUID value thus changing the UUID which an NFT corresponds to.

This implies that it may be possible that a signature is provided which shows that UUID X is associated with NFT A at one point in time and then subsequently after correction a signature is provided that shows that UUID X is associated with NFT B at another time.

If this is the case, then a malicious actor may retrieve the original signature which shows that UUID X is associated with NFT A and use this signature for their own NFT A to claim the vest of NFT B.

Also be aware that updating the NFT to UUID mapping allows for more than the original vesting allocation to be claimed, since the vesting state storage slot which stores the withdrawnAmount is unique to the nft and tokenId, not the UUID.

Be aware of these risks when making any updates with the signatures and nftToUUID mapping. To avoid the risk of stale signature replay in these scenarios consider assigning a new UUID signer after every update so that previous signatures cannot be used.

Otherwise every affected NFT can be force assigned the most recent correct UUID in the nftToUUID mapping. To resolve the withdrawnAmount discrepancy, consider including an admin function to update the withdrawnAmount of a user’s claim.

Otherwise be sure to update the merkle proof accordingly to reduce the user’s allocation by the already withdrawn amount.

Animecoin Team: Acknowledged.

In the ClaimChecker contract the _DELEGATE_REGISTRY_V2 is called directly in the _checkDelegateV2Rights function without using the DelegateCheckerLib.

However the DelegateCheckerLib performs a more efficient call, supporting the bytes32 rights parameter with the overloaded checkDelegateForAll function.

Consider using the DelegateCheckerLib for the SUBDELEGATION_RIGHTS_KEY check as well.

Animecoin Team: Resolved.

In the requestClaim function there is no validation that prevents users from requesting claims that will trivially fail when the daily withdrawal limit has been reached.

Consider adding validation which prevents users from requesting claims when the daily withdrawal limit has already been reached to save users from potentially wasting gas and LayerZero fees.

Animecoin Team: Acknowledged.

The LayerZero DVN is unnecessarily repeated as a required DVN and an optional DVN, however for the desired behavior of 2/3 DVNs with one of them always being LayerZero then the LayerZero DVN should be included only once in the requiredDvns list with the optional DVNs list being the other DVNs with an optionalDVNThreshold of 1.

Furthermore, currently there are 4 total unique DVNs included in the config file, meaning that one of the non-LayerZero DVNs should be removed from the optional DVNs list to make it a 2/3 total verification.

Remove the LayerZero DVN from the optionalDVNs list as well as another of the DVNs from the optionalDVNs list. Then reduce the optionalDVNThreshold to 1 to achieve a 2/3 total DVNs verification where one DVN must always be LayerZero.

Animecoin Team: Resolved.

In the AnimeClaimer contract the requestClaim function does not include any validation on the maximum length of the config list.

This may be a good validation to include to avoid any unexpected behaviors while providing DVNs with requests that include an unnecessarily large amount of tokenIds to claim for.

It’s not clear what the upper limit is for return data that the executor and/or DVNs can process and given the implications of a potentially stuck OApp like in C-01 it may be best to avoid this undefined behavior altogether by limiting the amount of configs provided to an individual requestClaim call to a reasonable amount.

Consider validating that the configs.length is within a reasonable limit.

Animecoin Team: Resolved.

The checkClaims function in the ClaimChecker contract uses a nonReentrant modifier to prevent any potentially malicious contracts from re-entering into the checkClaims function during the DVN simulation.

However if a malicious contract somehow does reenter into the checkClaims function and trigger the nonReentrant modifier to revert this will cause the OApp to enter a stuck state as described in C-01.

No pathway for this to occur has been identified, however out of an abundance of caution instead of using a nonReentrant modifier, consider using an ENTERED storage variable and setting it as true at the beginning of the checkClaims function.

Instead of reverting if a call reenters the checkClaims function, simply return false to indicate that the claim could not be verified. This allows a malicious actor to DoS a single claim, but they cannot freeze the OApp and DoS all subsequent read messages if this behavior is implemented.

Animecoin Team: Acknowledged.

The SUBDELEGATION_RIGHTS_KEY in the ClaimChecker contract is declared as the bytes of a string, however typically rights keys are hashed strings with keccak256. An example of this is the upcoming Shadow rights for APE chain: https://x.com/0xQuit/status/1869499764464906373

Consider using a hash of the "AN_CLAIMER_PERMS" string instead of the bytes of the string directly.

Animecoin Team: Resolved.

The explicitContractClaimers mapping is used both for NFT claims and collector claims. Therefore if a user explicitly designates a contract claimer then that claimer is able to claim all NFT claims and collector level claims for the user.

This may be unexpected for the user as they may expect to be able to designate a contract claimer as the claimer for an individual NFT or just NFT claims in general.

Be aware of this behavior, if intended then be sure to document it clearly to users.

Animecoin Team: Acknowledged.