BoostManager, PickCallbackV2 and NftDistributorV2

Users earn different kinds of rewards based on boost types, and the time boost is one of them. According to the documentation and governance proposal, the time boost is based on the minting date of active NFTs and should be linear. Users who hold NFTs longer should receive higher time boost rewards.

“The older your IFT is the larger the reward compared to others. For example somebody who minted/won an IFT 2 months ago will have twice their time boost reward compared to someone who minted 1 month ago.”

The protocol uses a stake balance of 1 for each epoch to calculate the time boost. A user’s time boost balance will be 1 for holding the NFT for one epoch. The boost balance will be 2 for holding it for 2 epochs, and so on. These balances are updated in the _updateTokenId function.

However, the time boost balance is not accurately reflected in the earned rewards. Currently, a user who stakes an NFT and waits 4 epochs without taking any action will receive a time boost based on a balance of 1 for all 4 epochs, after which their balance will be updated to 4. The earned rewards would be calculated as 1 * 4 (balance * rpt). In contrast, another user who calls the process function in every epoch would receive a reward of (1 * 1) + (2 * 1) + (3 * 1) + (4 * 1), since this user’s balance would correctly reflect the time-based boost.

Utilizing both the time difference and the increased staked balance of the user causes rewards to become exponential rather than linear, creating an opportunity for malicious users to game the system.

Consider a laddering approach to apply the appropriate multiplier for each epoch the user was staked in.

Unstaking is not performed when a user burns their NFT while the boost is disabled. The NFT gets burnt, but the rewards for that token ID remain unchanged until the sync function is called. The sync function resolves these state inconsistencies and forfeits rewards from that token ID.

However, the sync is not automatically invoked, and there is a time gap between burning an NFT and calling sync. Additionally, any user can mint a new NFT with the exact same token ID after it has been burnt.

When a user burns their NFT while the boost is disabled, another user can mint a new NFT with the same ID before the sync is called, leading them to gain the previous user's rewards that should have been forfeited.

Consider allowing a user to claim their tokenId rewards even when boost is disabled and/or invoke the sync function directly from the _processBurn when a user burns their NFT while the boost is disabled.

The BoostManager contains function sync so that mint and burn actions performed when the boost was disabled can be reflected within the BoostManager.

For example, when a user has an existing stake and burns their NFT when boost is disabled, the stake balance will remain until sync is called. However, the current handling logic for isDisabled leaves a token's stake balance open to manipulation when boostDisabled() is toggled.

Consider the following scenario:

1. Boost is disabled and Bob has an existing stake.
2. Bob burns NFT; BASE balance is still greater than 0.
3. Boost is enabled; Because else branch is now entered and the NFT does not exist, function sync continues and does not actually sync.
4. Alice mints that same tokenId and it is staked. The stake balance is now double the TOKENS_PER_NFT.

Ultimately, Alice was able to double her BASE balance since Bob's was never removed from the system.

Either ensure all tokenIds are synced prior to re-enabling boost, or have sync be called automatically.

The protocol has an auto processing for token Ids, and this is done in a linear order with a tracking index. Processing starts from the latest autoProcessedIndex and continues.

However, _tokenIds is an EnumerableSet and it doesn’t have a certain order. Anytime a token is burnt or unstaked, the last element of the set is moved to the burnt token’s index and the last element is popped.

As a result of this, the unprocessed last token in the set might be moved to an already processed index, causing it to remain unprocessed for a longer time and decrease the compounding effect of the rewards for that token.

Consider specifically calling the process for the token that is being moved from the last index to the new index during burns/unstakes if the new index is smaller than autoProcessedIndex. Otherwise, consider using an ordered data structure.

_process() does not validate if the protocol has set boostDisabled to true. This will allow users to compound their staked assets even when the boost is meant to be disabled. This allows already staked users to gain an edge over newly staked users when the boost is reenabled.

_process() should verify that boostDisabled is false and otherwise return early.

The reward calculation is correct when using tokens that have 18 decimal places. However if a token that uses six decimals is used, such as USDC, a rounding error can occur.

Consider the following scenario: Distribution Tokens : 500e6 Epoch Length : 604800 Remaining Rewards : 0 Reward Rate : 826.72

TokensPerNft : 100,00e18 NFTs Minted : 100 Total Supply : 10,000,000e18 Last Interaction : 2 RPT : 1.65e-4

With Solidity, rpt will be rounded down to 0, and no tokens will be distributed.

To properly calculate rpt, you must consider the token decimals of the reward token being used. Consider normalizing the decimals for the rpt calculation and then converting back to the proper decimals for transfers.

process() will get all the rewards for all boost types, then stake them to BASE. This will increase a user’s balance, and, in turn, allow them to receive more reward tokens throughout the boost. Then, process() will call _claim(), which will send the same tokens that were just staked to the user (or their vault).

A user can extract more rewards by calling process() repeatedly throughout their staking duration. This will allow them to out-earn users who deposit at the same time as them by actively engaging with process() and compounding their rewards.

The change in distribution is at the expense of other staked users, as there is a set amount of tokens to be distributed and the final rewards are skewed towards the user due to their continuous compounding.

Consider processing existing tokenIds in a scheduled manner to avoid gaming. Otherwise, clearly document this to users. Restricting users to process at only certain times will add too much complexity.

boostDisabled is initialized as false, which allows users to stake in epoch zero. If a user stakes in epoch zero, it will set the cachedLastUpdateTime to a value before startTimestamp.

Then when epoch one starts, the rpt calculation will assign a higher value than expected based off the abnormal cachedLastUpdateTime value. This will lead to a user (or users) to earn more rewards than are allocated for the epoch.

Disable the boost in the constructor and do not enable it until the first epoch has started.

The function process uses a push pattern to send user rewards. Because the pool config does not restrict which tokens can be used as reward tokens, if even one reward token has blacklist or freeze functionality, it can potentially brick auto-processing and reward token transfers.

Consider utilizing a pull-over-push pattern.

According to the protocol docs, a minimum of 10 NFTs should be distributed every week. Since the protocol has been live for a while, both distributionCountPerSet and minDistributionCountPerSet are currently set to 10.

The NFT distribution is based on the total ticket amount for the week and distributionSpacing. However, the weekly distribution count might fall below 10, even if more than 10 tickets are sold.

For example: Ticket sold: 11 _distributionCountPerSet: 10 distributionSpacing will be 2. In this case, winner ticket ids based on the vrf result will either be:

• 0, 2, 4, 6, 8, 10 or
• 1, 3, 5, 7, 9

From the perspective of a user who invests in the protocol based on the documentation, having 1 of 11 tickets should provide a greater than 90% chance of earning an NFT (10/11). However, in both scenarios mentioned above, the number of distributed NFTs is significantly below the minimum weekly amount, reducing the user's chance of getting an NFT to around 50%.

Document this behavior on the website protocol documentation and let users know that there might be some cases where the minimum number of NFTs cannot be distributed.

The NftDistributor contract holds a portion of the reward tokens and transfers 100,000 tokens to the corresponding NftVault whenever a lottery winner receives an NFT.

However, the currently active NftDistributor contract lacks a transfer function to move all of its reward token balances to the new distributor contract.

To resolve this, the reward token admin can burn the entire balance of the old distributor and mint an equivalent amount to the new distributor. However, the StakingAirdropDeployment contract, which is responsible for setting up the new distributor, does not perform this action during deployment. This means there will be a gap between deploying the new contract and minting reward tokens to it, which could cause some distributions to fail depending on the timing of the deployment.

Consider implementing the burning and minting of reward tokens within the deploy function to ensure a seamless migration. Alternatively, ensure that the deployment timing is precise, so there won’t be a lottery distribution between deploying the contract and minting the balance to it.

addPoolRewardToken function does not have a check to prevent adding same token address twice. This function adds a new entry to poolConfigs but there isn’t a way to remove a config from it. Adding the same config twice would cause incorrect state updates during regular action flows.

There's an assumption that a pushed pool config with a non-INT_POOL_ID also does not have INT as a reward token. If such a pool config was indeed added, then the token callbacks won't be properly disabled when adding the reward to the BoostManager with function addRewards:

if (poolId == INT_POOL_ID) _rewardToken.disableCallbacks();

Consider adding a duplicate entry check for this function.

Users have the ability to burn their NFTs through the distributor contract. If the boost feature is enabled, users' stakes will be force unstaked and any earned rewards will be processed before the INT tokens are transferred back to the user.

In the event that a user chooses to burn their NFT while the boost feature is disabled, they will lose any unprocessed rewards that were accumulated while the boost was active.

Document this behavior for users and inform them that there is a possibility of losing rewards if they burn their tokens while the boost feature is disabled. Also consider suggesting to users to manually process their rewards before burning their tokens.

In the constructor of StakingAirdropDeployment.sol, _startTimestamp is only validated that it must be larger than the current timestamp. However, in BoostManager.sol the start timestamp must be at least one day in the future. If the _startTimestamp is set to less than one day it will prevent deployment of BoostManager.sol and require you to redeploy StakingAirdropDeployment.sol.

Additionally, a 30 day window is allowed in BoostManager.sol, but only a 1 week window is allowed in StakingAirdropDeployment.sol.

Validate that the _startTimestamp is greater than one day from time of deployment.

If a user calls getEpochStartTimestamp(0), the result is 0.

If a user calls getEpochEndTimestamp(0), the result is startTimestamp - 1.

This delta between start and end timestamp is not the length of an epoch PERIOD and may confuse external systems.

Clearly document this behavior.

In the BoostManager contract the receiverConfig value is written and stored on the poolConfigs, however is never used after assignment.

Implement the use-case for the receiverConfig or consider removing it.

The current _processUnstake logic resets the autoProcessedIndex to 0 if an unstake causes the index to be out-of-bounds with the new tokenIds set.

A user could burn their NFT just before a user stakes, so that the autoProcessedIndex is reset, and there may have to be multiple process() calls before the newly staked tokenId is reached.

Consider documenting this potential behavior.

NFT holders earn both INT and WETH rewards. INT rewards are re-staked and held in the NftVault contract, while WETH rewards are directly transferred to the NFT owner. The owner of the NftVault is the same person who holds the NFT. This means that the ownership of the NftVault will change whenever the NFT is transferred to someone else.

When users transfer their NFTs, they will lose all of their unprocessed reward. They would also lose all INT rewards even if they process it.

An inactive holder might transfer or sell their NFT at a lower price without knowing that the NFT also carries additional internal value in the form of rewards. Since this behavior is not documented, the holder might mistakenly think they receive their rewards passively each week.

Consider not only re-staking INT rewards but also allowing users to claim their INT rewards to their own wallet in a manner similar to WETH rewards. Additionally, document this behavior and advise users to process their rewards before selling their NFTs on the secondary market.

Another option to consider is calling the process function in a _beforeTokenTransfer callback anytime an NFT transfer happens.

The disablePool function is introduced to address issue M-06. When a pool is paused using this function, it is excluded from reward processing. However, there is currently no way to unpause a pool afterward.

If even a single user is blacklisted, the auto-processing will be DoS’ed. To resolve this, admins will disable the pool. However, this will cause all other users who are not blacklisted to miss out on their rewards, as the pool will no longer distribute rewards.

Clearly document this behavior to users, or introduce some exception logic to allow non-blacklisted users to claim their rewards. This can mean excluding specific addresses rather than pausing the entire pool. Additionally, consider adding a function to enable pools afterward.

BoostManager.sol can be deployed so that the first epoch can happen up to 30 days after the deployment. A user can stake in epoch 0 and grow their balance of time based rewards since the calculation in _processTime() is based off the amount of time a user has been staked divided by an epoch's length. This gives users who deposit in epoch 0 an unfair advantage over other users

Consider disabling staking in epoch 0 or updating the joinedTimestamp to the start time of the first epoch.

Users are now able to process their TIME boost balance prior to updating the tokenId. This will increase the total supply of this boost, which would ultimately lower the reward per token.

Consider the following scenario:

1. Bob sends a transaction to process and claim to receive X earnings returned by function totalEarned
2. Alice who is in cooldown processes right before Bob and updates their time balance, increasing TIME total supply and decreasing the reward per token.
3. Bob's transaction goes through afterwards and receives <X tokens, which is less earnings than initially reported.

Note that frontrunning is not necessary, and will happen naturally as multiple tokenId's are iterated through with process(). Ultimately, Bob's total earnings decreased although staking earnings should typically monotonically increase. This is especially unexpected since totalEarned should accurately reflect how much is claimable to users.

Consider processing the TIME boost after updating token metadata. Ensure users do not lose unclaimed rewards.

The tests test_gas_multiBurn , test_gas_multiForceBurn, and test_gas_multiMint now fail and show usage of more than 120,000,000 gas.

Adjust the failing tests.