RateBasedLockRewardDistributionFacetStorage Manual Review Findings

RBR-01M: Incorrect Inclusive Limit

Type	Severity	Location
Logical Fault		RateBasedLockRewardDistributionFacetStorage.sol:L315, L358-L366

Description:

The RateBasedLockRewardDistributionFacetStorage::calculateRewardNoLock function appears to utilize an incorrect limit to detect whether a rate change occurred.

Specifically, the maximum value that the RateBasedLockRewardDistributionFacetStorage::snapshotsTimestampLookup function will yield is equal to rateChangeSnapshots.length - 1 which in turn will always equal currentIndex + 1 - 1 = currentIndex. The conditional checks whether checkingIndex + 1 <= length which would, at maximum, translate to currentIndex + 1 <= currentIndex + 1 thus always triggering.

Impact:

While the conditional and overall structure of the code is incorrect, we do not believe that it introduces a flaw in the calculations due to the special if-else block within the first conditional branch's code.

Example:

Copy
304CampaignInfo storage campaignInfoLocal = l.campaignInfoLocal[campaignId];
305NftInfo storage nftInfoLocal = l.campaignInfoLocal[campaignId].nftInfoLocal[nftId];
306
307uint256 virtualPacketBalance = GeneralStorage.layout().nftInfo[nftId].virtualPacketsStaked;
308uint256 length = campaignInfoLocal.currentIndex + 1;
309uint256 startingTimestamp = nftInfoLocal.lastRewardTimestamp;
310uint256 reward;
311
312// We find the index to start iteration from
313uint256 checkingIndex = snapshotsTimestampLookup(campaignInfoLocal.rateChangeSnapshots, startingTimestamp);
314
315if (checkingIndex + 1 <= length) {

Recommendation:

We advise the outer if-else block to be omitted as we believe that the campaignInfoLocal.campaignRates[checkingIndex] rate will equate the nftInfoLocal.currentNftRate when the last rate is being utilized.

Alleviation (b64b659786cf3c84bea52feb3a69f546ba3601f0):

The outer if-else clause has been removed as advised, simplifying the code's structure.

RBR-02M: Incompatible Initialization Implementation

Type	Severity	Location
Logical Fault		RateBasedLockRewardDistributionFacetStorage.sol:L121, L122

Description:

The mechanism via which campaign creations are permitted allows facet initialization functions to be invoked multiple times.

While most initialization mechanisms are compatible with this approach, the RateBasedRewardLockDistributionFacetStorage::setCampaignRewardsDistribution implementation is not as it will push to an array instead of overwriting it with a single data entry.

Impact:

Multiple invocations of the RateBasedRewardLockDistributionFacetStorage::setCampaignRewardsDistribution function will result in corrupted campaignRates and rateChangeSnapshots arrays that could cause the overall campaign to malfunction.

Example:

Copy
101function setCampaignRewardsDistribution(
102    Layout storage l,
103    uint256 campaignId,
104    bytes calldata campaignRewardsDistributionData
105) internal returns (uint256, bool) {
106    if (GeneralStorage.layout().campaignsInfo[campaignId].state != 1) {
107        revert CampaignNotOnCreationStateForSetting(campaignId);
108    }
109
110    (uint256 campaignRate, bool isVirtualLockMultipliersSupported) = abi.decode(
111        campaignRewardsDistributionData,
112        (uint256, bool)
113    );
114
115    if (campaignRate == 0) {
116        revert InvalidZeroCampaignRate(campaignId);
117    }
118
119    CampaignInfo storage campaignInfoLocal = l.campaignInfoLocal[campaignId];
120
121    campaignInfoLocal.campaignRates.push(campaignRate);
122    campaignInfoLocal.rateChangeSnapshots.push(block.timestamp);
123    campaignInfoLocal.isVirtualLockMultipliersSupported = isVirtualLockMultipliersSupported;
124
125    return (campaignRate, isVirtualLockMultipliersSupported);
126}

Recommendation:

We advise the code to either locally overwrite each array with a single entry, or the CampaignCreationSkeletonNID::createCampaign implementation and its non-NexeraID counterpart to ensure each function signature is invoked only once (i.e. by mandating that the function signatures are in a strictly ascending order).

Alleviation (b64b659786cf3c84bea52feb3a69f546ba3601f0):

The Evergon Labs team evaluated this exhibit and opted to prevent duplicate initializations of optional facets by ensuring that each optional facet's function selector is invoked at most once in both campaign creation implementations, alleviating this exhibit in full.

RBR-03M: Inexistent Gradual Catch-Up Mechanism

Type	Severity	Location
Logical Fault		RateBasedLockRewardDistributionFacetStorage.sol:L326-L356

Description:

The RateBasedRewardLockDistributionFacetStorage::calculateRewardNoLock implementation mandates that all rate changes are properly applied to a particular reward calculation which may result in stakes becoming impossible to unstake and claim rewards due to breaching the block gas limit.

Impact:

A malicious campaign owner can sabotage funds deposited to it by rapidly changing the campaign's rate multiple times, and inactive stakers in honest campaigns with multiple rate changes might become unable to unstake their funds.

Example:

Copy
325} else {
326    // More than one rate to account for the whole active period (since last rewards provision occurred).
327    // The first one (first part of the whole active period) can be calculated externally
328    // to save gas upon the rest of execution.
329    reward = _calculateRewardNoLock(
330        campaignInfoLocal.campaignRates[checkingIndex],
331        virtualPacketBalance,
332        campaignInfoLocal.rateChangeSnapshots[checkingIndex + 1] - startingTimestamp
333    );
334    checkingIndex++;
335
336    // Continue with the remaining parts of the whole active period
337    while (checkingIndex < length) {
338        // if they have difference of 1, then block.timestamp is used as endingTimestamp
339        // as it is the last iteration
340        if (length - checkingIndex == 1) {
341            reward += _calculateRewardNoLock(
342                campaignInfoLocal.campaignRates[checkingIndex],
343                virtualPacketBalance,
344                block.timestamp - campaignInfoLocal.rateChangeSnapshots[checkingIndex]
345            );
346            break;
347        } else {
348            reward += _calculateRewardNoLock(
349                campaignInfoLocal.campaignRates[checkingIndex],
350                virtualPacketBalance,
351                campaignInfoLocal.rateChangeSnapshots[checkingIndex + 1] -
352                    campaignInfoLocal.rateChangeSnapshots[checkingIndex]
353            );
354            checkingIndex++;
355        }
356    }
357}

Recommendation:

We advise a mechanism to be introduced permitting rewards to be gradually claimed, ensuring that inactive stakes can be properly extracted from the system even if multiple rate changes have occurred.

Alternative alleviations include being able to forfeit rewards, imposing an upper bound on the total number of rate changes permitted for a campaign, and / or calculating rewards for a fixed maximum number of rate changes forfeiting the rest.

Alleviation (b64b659786cf3c84bea52feb3a69f546ba3601f0):

A maximum rate change limitation was introduced to the codebase instead, ensuring that a campaign's rate changes are finite and thus always able to be caught up.