ANIME Claimer, Part 1

In the _getClaimChecksFromVestingConfigs function the checks.collectors array is assigned a length based on the numCollectors which is purely the number of configs which satisfy c.collector = address(0).

However the criteria for an address to be added to the checks.collectors array is isForCollector & co.collector = msg.sender.Therefore the checks.collectors array will have empty entries for the configs that are claimed by the collector address themselves.

The checks.collectors array with empty entries is then used to verify the claim with the ClaimChecker contract on L1. The _checkCollectorClaim(claimer, collectors[i]) check will trivially fail because no claimer can be approved in the delegate registry for the zero address.

Therefore these claims cannot be verified and cannot be completed.

Shrink the length of the checks.collectors array after writing to it or adjust the way the numCollectors is calculated so that it is only incremented in the case where isForCollector && c.collector = msg.sender.

Animecoin Team: The issue was resolved in PR#10.

In the _claimBatch function after the claim has been validated by the l1 state at the timestamp of the requestClaim call, the claimer is awarded with the vested amount computed by the _vested function.

However the _vested function determines the user’s vested amount based upon the current block.timestamp of the lzReceive action. This is however not the same timestamp that the claimer was verified to be authorized to claim at.

The claimer could have sold their NFT to another user after the block.timestamp of the requestClaim call and thus received vested amounts which should have gone to the new owner of the NFT.

There is a 8 block confirmation threshold currently configured, meaning that roughly 96 seconds will have to occur at minimum between the requestClaim call and the lzReceive which fulfills the claim.

This delay period could be made even more severe during a DVN outage or if a claim has passed all DVN checks but the lzReceive function reverts for some time until the revert is resolved (e.g. claim contract is paused and unpaused, or the daily withdrawal threshold has been met for the day) and then the lzReceive function is invoked again successfully passing a significant amount of time later.

Include the timestamp of the claim request in the ClaimParameters for the corresponding claimNonce if the claimer is shown to be validated for all of the claims then the claims should be carried out up to the stored timestamp of the claim request.

Animecoin Team: The issue was resolved in PR#22.

In the AnimeClaimer contract when the claimer is the collector for a collector vest then there is no further validation performed on the vest and the vested amount is granted to the collector address on the L2.

However the owner of the collector address on the L2 may be different then the owner of the collector address on the L1.

For example, a smart contract wallet/multisig could have been transferred from Alice to Bob on the L1, but Alice may have kept ownership of a smart contract wallet/multisig deployed at the same address on the L2.

Ensure that all collector addresses which are awarded are EOAs on both chains. If this is not the case then a more adept verification process is necessary.

Animecoin Team: The issue was resolved in PR#14.

In the requestClaim function the checks.numNFTs and checks.numCollectors values are used to compute the gas that should be provided to the lzReceive function on the L2.

However the checks.numCollectors value only includes the number of collector claims that need to be verified on the L1, which does not include claims where the collector is the claimer.

These claims will still have to be iterated over in the lzReceive function though, and thus should be accounted for in the gas estimation.

Consider basing the gas estimation in the _lzOptions function on the configs.length instead as this more closely represents the iterations that must be made in the lzReceive function.

Animecoin Team: The issue was resolved in PR#11.

A VestingConfig object contains uint256 streamId but the VestingConfigStorage contains uint8 streamId. Consequently, some user configurations will be unclaimable when the uint256 type is cast to uint8 within _saveClaimParameters, as it will revert with error Overflow().

Consider keeping types consistent or clearly document this behavior.

Animecoin Team: The issue was resolved in PR#25.

Currently checkClaims only validates sanctions against the claimer: if (isSanctioned(claimer)) return (claimNonce, false); However, there's is no validation that the actual owner of the NFT is not sanctioned in the case that the claimer is a delegate.

Consider adding sanctions validation on the NFT owner, otherwise clearly document this behavior.

Animecoin Team: The issue was resolved in PR#13.

In the setReadChannel and setReadConfirmations onlyOwner functions there is no call to the _logAdminAccess function to emit an event for these admin updates.

Additionally, in the OAppCore contract there is a setDelegate onlyOwner function which is not overridden and thus will not emit a AdminAccessed event.

Consider adding a _logAdminAccess invocation to these functions.

Animecoin Team: The issue was resolved in PR#15.

The AnimeClaimer includes many getters to query the state of the system but is missing a getter for lastWithdrawnDay.

Consider adding a getter for lastWithdrawnDay.

Animecoin Team: Acknowledged.

In the comment for the requestClaim function the _lzReceive function is referred to as _lsReceive.

Correct this to _lzReceive.

Animecoin Team: The issue was resolved in PR#16.

Function dailyTotalWithdrawn() aims to return the current dailyTotalWithdrawn, however the value returned may be stale since the current day be different from the lastWithdrawnDay but _resetDailyTotalWithdrawnIfNewDay hasn't been triggered yet.

This can be problematic for integrators reading this value and assuming the current day has already had withdraws when it hasn't.

Consider adding logic within dailyTotalWithdrawn() to reflect the start of a new day.

Animecoin Team: Acknowledged.

In the comment for the uuidSigner value in the AnimeClaimerStorage struct it is mentioned that this value SHOULD be configured, however this value must be configured along with the other values that are documented as such.

This is because the uuidSigner must be configured to a nonzero address for the readyForClaim function to return true.

Correct the comment to indicate that the uuidSigner qualifies as a variable which MUST be configured.

Animecoin Team: The issue was resolved in PR#17.

The requestClaim function assigns the msg.sender as the refund receiver for a native refund. As a result, Smart Contracts which receive a collector allocation, do not have the functionality to delegate, and do not have a receive function cannot claim their vest.

This scenario is unlikely to occur, especially for a contract which is able to claim from the claimer. However if this issue is desired to be solved, or additional utility should be added, a refundReceiver parameter can be added to and used within the requestClaim function.

Animecoin Team: Acknowledged.

Currently delegation is validated solely through the DelegateRegistry contract deployed on Ethereum within function checkClaims. Consequently, delegations through the DelegateRegistry contract deployed on Arbitrum will not allow for the receipt of allocations.

Clearly document this behavior to users.

Animecoin Team: Acknowledged.

In the previous version of the AnimeClaimer contract there was a to address which allowed users to claimBatch to their desired address on Arbitrum.

However in the new AnimeClaimer contract the allocations are always sent to the claimer who calls the requestClaim function.

This may decrease the simplicity of integrations/user interactions with the AnimeClaimer, especially for claims that are made on behalf of another collector using the delegate registry.

Consider adding a to parameter to the requestClaim function so that a claim can be sent to the configured address rather than always to the claimer.

Animecoin Team: Acknowledged.

The ChainAnalysis oracle for sanctioned addresses is also deployed on Arbitrum. Consequently, sanctions can be verified at the start of a requestClaim to prevent the message from even being sent, as well as _lzReceive to account for sanctions list changes in the case of delayed receipt (e.g. if there is a DVN outage).

Consider querying the Arbitrum oracle as well and accounting for any necessary gas usage increases, otherwise document this behavior.

Animecoin Team: Acknowledged.

In the AnimeClaimer constructor the expectedCalldataSize which is denominated in calladata bytes is assigned as 256.

This is ultimately used as the calldata size option in the lz options blob passed to the send function. This is meant to estimate the cost of the additional calldata which is provided from the result of the read function on the L1 chain.

However the checkClaims function result will not return a payload of 256 bytes of calldata to the lzReceive function, it returns a payload of 64 bytes of calldata to the lzReceive function. One 32 byte word for the claimNonce and one padded 32 byte word for the authorized boolean.

This results in overpaying the executor for the additional calldata that is estimated to be required for the lzRead response payload on every claim.

Update the expectedCalldataSize to 64 bytes.

Animecoin Team: The issue was resolved in PR#24.

The provided runbook contains a typo in the instructions for deploying the ClaimChecker contract on Ethereum. The first step mentions Only select “ethereum”, unselect “ethereum” and others. However it should read Only select “ethereum”, unselect “arbitrum” and others.

Correct the runbook step to Only select “ethereum”, unselect “arbitrum” and others.

Animecoin Team: Resolved.

In the comment for both implementations of the quoteForClaim function it is mentioned that the purpose of the function is to:

Returns the amount of ETH in wei that needs to be passed into claimBatch. However the quoteForClaim function is intended to return the amount of ETH in wei that needs to be passed into the requestClaim function.

Correct the comment above both implementations of the quoteForClaim functions to:

Returns the amount of ETH in wei that needs to be passed into requestClaim.

Animecoin Team: The issue was resolved in PR#23.

In the _validateRequestClaim function the ECDSA library recover function is used. However the config object with the signature to be verified is declared as calldata, therefore the recoverCalldata function should be used to save a couple hundred gas.

Use recoverCalldata instead of recover.

Animecoin Team: The issue was resolved in PR#26.

In the ClaimChecker contract the _checkNFTClaim function implements specific logic for Azuki NFTs which asserts that ownership of the NFT being verified has not begun at this block.timestamp.

This is to prevent NFT flash loans from being used to flash loan an NFT and claim it’s outstanding vest amount. However it is impossible to use an intra-transaction flash loan to prove ownership of an NFT because the DVNs will not check the state of the L1 at an intermediate transaction state.

Therefore this check for Azuki NFT ownership is no longer necessary and is instead preventing users from claiming their vest at the timestamp of when they actually began their ownership. This case is unlikely to affect UX, however it can be removed for simplicity.

Consider removing the specific logic for Azuki NFTs in the _checkNFTClaim function.

Animecoin Team: The issue was resolved in PR#29.

There exist platforms such as Blur which support fixed term loans of NFTs which will be awarded allocations, such as Azukis.

For Azukis which have accrued an unclaimed vest allocation a fixed term loan can be made to allow the loaner to claim the outstanding vested amount even though they did not hold ownership over the NFT for the previous vesting period.

Be aware of this risk and document it for users who have their NFTs available for loan.

Animecoin Team: Acknowledged.