Node Sale

An attacker can DOS the import of user data by front-running withupdateVaultsInfoForUser.

This increases the nextAccountID and causes a mismatch in accountId and indexGiven when importUserData is called by admin.

The attack can be performed repeatedly at low cost (only gas cost incurred) and delays the commencement of vesting.

Consider allowing updateVaultsInfoForUser to be called only after import is complete.

calculateTotalBonusWeightingForChonk is an important function used to calculate the daily total bonus weight of users.

The function requires that nextAccountId in ZapAccount and VaultVesting are equivalent: require(vaultCountPlusOne == nextAccountID + 2)

This assumes that master accounts do not have a VaultVesting id. However, anyone can call updateVaultsInfoForUser on either of the two master accounts which would increase VaultVesting's nextAccountId and cause this check to always fail.

Prevent updateVaultsInfoForUser from being called on master accounts: require(accountId != 1 && accountId != 2, "master accounts);

When a user unstakes their vaults early, they will have to pay an unstake fee. However, that unstake fee is simply paid by having the VaultVesting contract transfer the fee from its balance of ZAP tokens to the treasury with vaultVesting.payEarlyUnstakingFee(fee);

Consequently, a user can continuously stake and unstake to drain the VaultVesting contract out of its ZAP balance. This is especially detrimental since these ZAP tokens are used for the claiming vested tokens. Even if the protocol replenished the VaultVesting contract, a malicious user can continuously do it at the expense of cheap gas.

Consider requiring vault holders to pay a fee in ZAP tokens.

When _calculateTotalTokenStakerTokens() is called, it will call getUserTokenStakes() to determine how many tiers the user is staked in. getUserTokenStakes() will return an array with the exact amount of tiers and then iterate through the array’s length.

During the iteration, it will call getTokenStake() based off what iteration it is on and not what tier it represents. If a user stakes in tier 1 and tier 3, it will grab the stake data for tier 1 and tier 2. This will exclude the stake data from the tier they are actually staked in, which in this case is tier 3. This will lead to a user losing out on tokens that they earned.

Loop through each tier and if the user does not have any tokens staked in that tier then continue.

In claimVaults, users will claim less tokens over multiple claims.

This is due to _calculateUnlockedTokensForClaimedVaults using vaultsHeld as the denominator. Consider this example:

• Alice has 100 locked tokens and 4 vaults
• Alice claims 2 vaults: (2/4)*100 = 50, new locked is 50
• Alice claims 1 vault: (1/4)*50=12.5 tokens, new locked is 37.5
• Alice claims her last vault: (1/4) * 37.5 = 9.4 tokens, new locked is 28.1

The remaining amount of Alice’s locked tokens cannot be claimed after.

Adjust the state.standardPoolTokensLocked by (vaultsToClaim / (vaultsHeld - vaultsClaimed).

Functions stakeTokens and stakeVaults do not validate that the tierIndex is a valid tier. A user could stake for an invalid tier, but still be considered an active staker. When performing saveCalculatedVaultHolderRewardsToState function _calculateEligibleStandardPoolTokensForUser will be entered since the vaultStaker is considered active and a non-zero address is returned.

Since the user does not have stakes in any of the valid tiers, the totalStandardPoolTokens returned will just be zero and the following would trigger a revert: require(standardPoolTokensEligibleForRewards > 0, "ZapRewards: User has no eligable rewards");

Ultimately, by staking in an inappropriate tier, a user is able to DoS the accrual of standard pool and bonus pool rewards for vault holders.

Validate that the tierIndex is 0-2 when staking.

Functions stakeTokens and stakeVaults do not validate that amountToStake is valid. A user could stake zero amounts but still be considered an active staker.

When performing saveCalculatedVaultHolderRewardsToState function _calculateEligibleStandardPoolTokensForUser will be entered since the vaultStaker is considered active and a non-zero address is returned.

Since the user does not have staked amount, the totalStandardPoolTokens returned will just be zero and the following would trigger a revert: require(standardPoolTokensEligibleForRewards > 0, "ZapRewards: User has no eligable rewards");

Ultimately, by staking zero amounts, a user is able to DoS the accrual of standard pool and bonus pool rewards for vault holders.

Validate that amountToStake is greater than zero for vaults and greater than a minimum amount for tokens.

Users are supposed to be charged a penalty if they unstake early prior to expiration. However, a user can simply stake with autoRelock and always avoid the penalty logic, causing no disincentive to unstake early and a loss of fees for the protocol.

Remove the direct check for autoRelock from the if-statement that determines if a user has to pay a fee for unstaking early. A potential solution may be: timeNow < stake.expiresAt || (stake.autoRelock && !isWithinGracePeriod)

The endIndex is equal to amountOfLoopIterations when it is final chonk. However, this is incorrect because amountOfLoopIterations is not the end index; it represents the difference between the start index and the end index.

For example: startingIndexOfChonk: 100, sizeOfChonk: 20, totalUsersWithStake: 110

As a result of this, isFinal will be true, amountOfLoopIterations will be 10, and endIndex will also be 10. The final chonk will not be iterated at all since the loop for (uint32 i = startingIndexOfChonk; i < endIndex; i++) will not be executed.

The endIndex should be amountOfLoopIterations + startingIndexOfChonk regardless of whether isFinal is true or not, since amountOfLoopIterations is already updated in the _prepareChonk function based on the total number of stakers.

The two most important parameters in the TokenStake struct are tokensStaked and previousStake. Ideally, these should be updated whenever a new stake or unstake action is performed. However, the previousStake variable is not updated in the unstakeTokens function.

Since this state variable is used when calculating rewards in the ZapRewards contract, incorrect state updates will lead to both user-specific rewards and the total reward weight being calculated incorrectly, leading to loss of rewards for users.

Update the stake.previousStake state variable within the unstakeTokens function.

ZAP offers an autoRelock feature which allows a user to keep earning rewards post-expiration of their stake. Within _calculateEligibleStandardPoolTokensForUser, the _checkIfShouldProcessStakeForReward internal function will determine that because the stake is autoRelock, it is a valid stake for reward processing even if the expiry is passed.

Although the stake is valid and processing is attempted, an underflow will occur in uint256 daysRemainingInStake = _secondsToDays(vaultStake.expiresAt - timeNow); because the expiration has been passed, so expiresAt < timeNow. Ultimately, this user (and chunk) will be unable to have vault rewards calculated, and the relock feature does not work as intended.

Add separate handling for auto-relocking stakes such as an automatic extension of the expiry so underflow does not occur.

In calculateTotalBonusWeightingForChonk, the total bonus pool weight for all users is calculated and stored. This assumes that when calculateDailyVestingForChonk is called, that the users' info has not been altered.

A malicious user could increase his share by purchasing more vaults or getting more referrals in between this two-step process. He would then earn more rewards at the expense of other users.

Consider locking updates to user info in between this two-step process.

It is intended that users still receive rewards based on their previous stake when they unstake after the reward distribution has started, and the _checkIfShouldProcessStakeForReward function is used for this purpose.

The _checkIfShouldProcessStakeForReward function is called within the _calculateEligibleStandardPoolTokensForUser method, using data retrieved from zapStaking.getUserVaultStakes.

However, when a vault is unstaked, its vaultsStaked value is set to 0, which causes getUserVaultStakes to skip those vaults. As a result, only vaults that are still staked will be considered. This means that users who unstake their vaults after the rewards distribution has begun will lose out on rewards, as unstaked vaults are excluded from the reward calculation.

Instead of calling getUserVaultStakes use getVaultStake

In calculateDailyVaultHolderWeightingsForChonk, the total standard pool weight for all users is calculated and stored. This assumes that when saveCalculatedVaultHolderRewardsToState is called, that the users' info has not been altered.

A malicious user could increase his share by purchasing more vaults in between this two-step process and would then earn more rewards at the expense of other users.

Consider locking updates to user info in between this two-step process.

The protocols offers users the option to stake their vaults, so they are awarded rewards not just for their current emitted tokens but also on all of their forecasted emissions for the duration of their stake.

The postHalvingEventEmissions are calculated by forecasting the rewards over the stake duration past the next halving, using the total daily rewards at next halving event as the daily emission rate. The issue is that some stakes can cross more than a single halving period.

Consider the following example:

• The current day is 179, HALVING_EVENT_1_OCCURANCE_DAY is at 180 days, and HALVING_EVENT_2_OCCURANCE_DAY is at 360 days.
• User stakes for 360 days, which will pass HALVING_EVENT_1_OCCURANCE_DAY and HALVING_EVENT_2_OCCURANCE_DAY.
• When calculating the user's postHalvingEventEmissions prior to the first halving, tokensVestedAtNextHalvingPeriod will be HALVING_EVENT_1_DISTRO_PER_DAY
• postHalvingEventEmissions are inflated for the user since the HALVING_EVENT_2_DISTRO_PER_DAY is ignored for a portion of their stake duration.
• 1 day accounted for the current emission until the first halving event, 180 days accounted for the emissions after the first halving, and the 179 days of vesting after the second halving event ended up using the daily emissions from the first halving event.

Ultimately, the totalTokensToVestOverStake is inflated and certain users will earn more rewards at the expense of others. Also note that the dailyTokenEmissionPerVault in the calculation is assumed to be consistent across the days staked, but the daily token emission can change from day-to-day as the proportion of vaults a user holds relative to total vaults sold changes.

Consider adding logic to account for multiple halvings being passed. Furthermore, document the assumption about the daily emission rate staying constant for the forecast.

In ZapRewards, the _calculateEligibleStandardPoolTokensForUser function calculates locked rewards that have already vested out as:

uint256 existingLockedTokensForStake = (totalUserLockedTokens * vaultsStaked) / totalVaultsHeldByUser

However, totalVaultsHeldByUser includes claimed vaults when it should only be using unclaimed vaults.

Consider this example: Alice has claimed 2 out of 5 vaults and vested 100 tokens for the day tokensUnlocked: 2/5 * 100 = 40 tokensLocked: 3/5 * 100 = 60

If she has staked all remaining 3 vaults: existingLockedTokensForStake: 3/5 * 60 = 36

This implies that she never gets the full rewards for her locked tokens despite staking all remaining vaults. Instead, she should benefit from the full locked amount.

Change the calculation to: (totalUserLockedTokens * vaultsStaked) / (totalVaultsHeldByUser - userState.vaultsClaimed)

After vesting ends, stakers should still receive rewards in ZapRewards. This should be done by first calling calculateDailyVaultHolderWeightingsForChonk in order to _resetState. However, that function will invoke VaultVesting.getDaysLeftOfVesting() which will revert because daysCalculated > TOTAL_DAYS_VESTING.

Return 0 from getDaysLeftOfVesting if daysCalculated > TOTAL_DAYS_VESTING.

When rewards are saved for vaults, the transaction will revert if standardPoolTokensEligibleForRewards is 0 because of the following require statement. A user may (maliciously or not) cause a DOS with this revert if all of their stakes have expired. The getVaultStakerById will return the address of the staker even if the stake is expired. Then _checkIfShouldProcessReward() will return false and standardPoolTokensEligibleForRewards will end up being 0 causing the whole transaction to revert.

Another possible case when this may happen is if after the vesting period ended a user with no locked tokens stakes a new vault.

Consider doing a continue instead of require revert.

A large amount of ZapTokens are expected to be transferred to the VaultVesting and ZapRewards contracts for users to claim.

After the entire vesting duration, it is likely that there will be leftover tokens unclaimed. Furthermore, there may be some leftover tokens due to rounding over time. These tokens should be recoverable by admin.

Consider allowing ZAP tokens to be covered by privileged address post-vesting period.

If a user unstakes after therewardsDistroStartTimestamp, they should still get staking rewards. However, if the stake has expired after that timestamp, the user will lose their rewards.

This is because _checkIfShouldProcessStakeForReward will mark the stake as invalid during the calculations. This is asymmetrical behavior that may lead to unexpected loss of rewards for some users.

Consider checking if the stake has expired after the start distribution timestamp and if so, reward them accordingly.

- if (!autoRelock && expiresAt < block.timestamp) return (false, amountStaked);
+ if (!autoRelock && expiresAt < rewardsDistroStartTimestamp) return (false, amountStaked);

vestingPeriodHasNotElapsed modifier will pass when the daysCalculated is 900. However, it should revert when calculated days reaches to 900.

Change the modifier to require(TOTAL_VESTING_DAYS > daysCalculated)

While enabling auto-relock may seem like the optimal strategy for maximizing staking rewards, savvy users can achieve better results by manually restaking daily.

By doing this, they ensure their remaining staking days are consistently maximized, allowing them to earn more rewards than users who rely solely on auto-relock.

This behavior is unexpected and may create an unfair advantage for users who manually restake, potentially disadvantaging regular users who choose the auto-relock option.

Consider if this gaming is acceptable to the protocol. If not, a potential solution is to allow restaking only after some time has passed. Alternatively, a redesign of the forecasted rewards mechanism may be required.

Both VaultVesting and ZapRewards use a variable startingIndexOfChonk when calculating daily weights in calculateTotalBonusWeightingForChonk and calculateDailyVaultHolderWeightingsForChonk respectively.

However, VaultVesting expects startingIndexOfChonk to be 1 -- since the first user index starts from 1 -- while ZapRewards expects it to be 0 since it does address vaultStaker = zapStaking.getVaultStakerByIndex(i + 1); to account for user index also starting from 1.

This discrepancy can lead to admin errors and possibly result in missing the first user when calculating daily weights in ZapRewards.

Be aware of this difference and account for it in offchan processes.

The loop in calculateDailyVaultHolderWeightingsForChonk calls _getCurrentVestingInformation for each user. However, this information does not change across users and could only be called once.

Consider retrieving the vesting information outside of the for-loop.

Function calculateTotalBonusWeightingForChonk tries sets a limit of how much iterations can be done. If the starting index of the chonk and the chonk size surpass the available accounts, the result is capped. However, the following calculation is wrong:

amountOfLoopIterations = (nextAccountID - startingIndexOfChonk) + 2

Let's say there are 2 master accounts and 2 normal accounts. If admins start from 0 and pass a very large chonk size, we will be calculating this amount of loop iterations as:

amountOfLoopIterations = (5 - 0) + 2 = 7

This will result in looping 7 times and calculating bonus weights for accounts that don't exist, i.e address(0).

Don't add 2 to amountOfLoopIterations

The treasury account is not set in initialize but within setTreasuryAddress by an admin.

While the treasury address is the zero address, unstake functions would revert when trying to send tokens to the zero address.

Be aware of this and set the treasury address early, potentially in the initialize function.

nextTokenStakerId variable and allTokenStakers mapping in the ZapStaking contract are never used.

Consider removing unused state variables.

The Unstaked event includes an earlyUnstakeFee parameter which is hardcoded to 0 when emitting events, regardless of whether the fee was actually 0 or not.

It is recommended to emit the actual early unstake fee during events.

The total tokens over the lifetime of the vault stake is calculated with uint256 totalTokensOverLifetimeOfStake = totalTokensToVestOverStake + existingLockedTokensForStake;

If the calculation was done at the exact time such that vaultStake.expiresAt - timeNow is a perfect multiple of days, it is possible for totalTokensToVestOverStake to include the current day as remaining.

This can be problematic since existingLockedTokensForStake already included the rewards for the current day, which can lead to double counting and inflated standard pool rewards.

This is an unlikely scenario, so just be aware of when calculations are performed.

In calculateTotalTokenStakerRewardsForChonk, the loop updates the state variable totalStandardPoolWeighting instead of the local variable totalStandardPoolTokens.

This results in gas wastage and defeats the purpose of the local variable.

Increment the totalStandardPoolTokens variable inside the loop.

If a user got 0 locked tokens and acquires a new vault that they stake it after the vesting has ended they will receive no rewards.

Warn users they have no benefit of staking in these conditions.

In _calculateTokensWithNegativeMultiplier, tokens are divided by 100 before multiplying by negativeMultiplier.

This leads to loss of precision due to rounding and users therefore lose tokens. For example, even if negativeMultiplier was 100, the user will still experience some loss due to rounding.

Perform multiplication before the division.

There are numerous daily functions which have to be called every day in order to calculate state variables used for rewarding both in VaultVesting and ZapRewards. Currently, these function can be called more than once per day. If that happens, the state variables used for calculating rewards will be incorrect and users will receive wrong rewards.

Be sure to call these function exactly once per day.

In calculateDailyVestingForChonk, daysCalculated is incremented before calculating tokensEmitted. This inflates tokensEmitted if daysCalculated crosses into a new halving event.

Calculate tokensEmitted first before incrementing daysCalculated.

Everyday stakers receive rewards from the ZapRewards standard pool based on their (userWeight / totalWeight) proportion. There are also bonus pool rewards given to vault stakers and these rewards should be calculated using the value obtained by the user's preTGE tokens multiplied by a similar ratio.

//In saveCalculatedVaultHolderRewardsToState
standardPoolRewards = (standardPoolTokensEligibleForRewards * DAILY_STANDARD_POOL_TOKENS) / totalStandardPoolWeighting

// In _calculateBonusPoolTokensForVaultHolder
bonusTokens = standardPoolRewards * preTGE / standardPoolTokensEligibleForRewards=>
bonusTokens = (standardPoolTokensEligibleForRewards * DAILY_STANDARD_POOL_TOKENS / totalStandardPoolWeighting) * preTGE / standardPoolTokensEligibleForRewards=>
bonusTokens = (standardPoolTokensEligibleForRewards * DAILY_STANDARD_POOL_TOKENS * preTGE) / (totalStandardPoolWeighting * standardTokensEligibleForRewards) =>
bonusTokens = (DAILY_STANDARD_POOL_TOKENS * preTGE) / totalStandardPoolWeighting

After unraveling the bonus pool tokens earned, the final proportion used to adjust the staked pre-TGE tokens is the (DAILY_STANDARD_POOL_TOKENS / totalStandardPoolWeighting). The amount of tokens in the standard pool is hardcoded to 12950e18 and the totalStandardPoolWeighting is shared across all users in the calculation. Consequently, the only unique portion of the bonus pool rewards for each user is the amount of pre-TGE tokens they hold per staked vault. This may be unexpected as the pre-TGE tokens are not directly correlated with the standard pool weighting, which is contrast to the standard pool reward calculation.

Ensure this is the intended protocol tokenomics for bonus pool rewards.

In functions isReturningVaultStaker and isReturningTokenStaker the userStakeCount is incremented regardless of the status of the user’s stake. Because it will always be incremented to three since there are three tiers, a vault staker will only be considered returning if a user had previously unstaked from all three tiers which may be unexpected functionality.

Clarify the logic behind returning stakers or just set userStakeCount = 3 if the function is meant to check whether a user unstaked from all three vaults previously.

The default admin is able to call function finishImport multiple times, which can be used to change daysCalculated to various values and negatively influence what halving the protocol is currently in and how many rewards are distributed.

Consider restricting how many times finishImport can be called or clearly documenting this behavior.

When users unstake early calculateEarlyUnstakeFee applies a fee % they have to pay based on how much of their stake duration has passed. This function doesn't consider stakes with autoRelock = true.

Because of that, after a stake relocks, percentageProgressThroughStake will exceed 100%. In result:

• For as long as this percentage is less than 100% of the 75% of the stake duration, autoRelock users will be able to unlock earlier by paying much smaller fee.
• Once the percentage goes above 100% of these 75% of the stake duration, the following calculation will revert and autoRelocks will not be able to unstake:

(100 * PCT_BASE) - percentageProgressThroughStake

Add a modulus operator in calculateEarlyUnstakeFee to handle autoRelocks

_createNewExpiryTimeForAutoRelock() checks if expiresAt is less than timeNow, and returns early if that is the case. Otherwise it will attempt to store timeNow minus expiresAt into a uint256. For this line of code to be hit, expiresAt must be greater than or equal to timeNow, which will cause an underflow. This will prevent users with autoRelock enabled from restaking early.

Change the if statement to check if expiresAt is larger than timeNow.

Similarly to C-04 from the main review, _calculateTotalTokenStakerTokens uses the wrong tier for calculations. However, this time it is using the wrong standardPoolNegativeMultiplier when making the calculation for userTokenWeighting.

This occurs because _calculateTotalTokenStakerTokens() loops through tokenStakes and uses i to get the multiplier from stakingTiers. If a user only stakes in the third tier, i will be 0 and it will use the negative multiplier from the first tier.

Loop through each tier and if the user does not have any tokens staked in that tier then continue.

ZapStaking.restakeVaults() should allow restaking of vaults only if half of the stake's duration has passed. However, the expiresAt variable it passes to the _isLessThanHalfWayThroughStake() function is the time the stake would expire after the restake. In result, the duration of the stake will be greater than it should be and users won't be able to restake their vaults if they haven't passed halfway that duration.

Instead of expiresAt, pass vaultStakes[msg.sender][tierIndex].expiresAt to _isLessThanHalfWayThroughStake()

The _isLessThanHalfWayThroughStake() function will return false if timeNow > expiresAt. This is fine for normal stakes, but ones with autoRelock = true will not yield correct result. Once after the first relock happens, the above condition will always be met and users will be able to restake vaults whenever they want to.

Consider adding autoRelock logic to _isLessThanHalfWayThroughStake().

In order to calculate the fee that needs to be paid when early unstaking an autorelock stake, the expiresAt of that stake is adjusted to match the end time of the current (relocked) stake.

Then that new expiresAt is used in calculateEarlyUnstakeFee. However, when such a state relocks, it stakedAt should also be updated. Because that's not the case, the calculations will be wrong and users will pay less fee because their percentage through the stake will be calculated higher.

Let's look at an example:

• Stake on day 0 for 180 days with autoRelock = true
• We are on day 200, which is 20/180 of the stake
• The new expiresAt will be 360
• The elapsed time is 200 - stakedAt = 200
• Duration = 360
• Calculations will return 200 / 360 which is a lot bigger than 20/18

Adjust the stakedAt for autoRelock as well.

Vesting is completed when daysCalculated is equal to or greater than TOTAL_VESTING_DAYS but the current recovery mechanism will prevent token recovery when TOTAL_VESTING_DAYS == daysCalculated:

require(TOTAL_VESTING_DAYS < daysCalculated, "Vesting period has not completed");

Modify < to <= in recoverTokensAfterVesting

The newly added VaultVesting.recoverTokensAfterVesting() function allows the admin of the contract to recover any tokens sent to the contract. The intention of that function is to retrieve the difference between the originally sent tokens and the actually distributed amount. However, this function can be used by the admin to transfer locked/unlocked tokens of eligible users out of the contract.

Be aware of the risk.

When users unstake their vaults, the fees paid are taken from their locked tokens. This will affect the rewards earned in ZapRewards for the past because locked tokens will have decreased, but the available vaults will not change. Because of that users will receive overall less rewards.

Be sure you are aware of this behavior.