Removed nominee doesn't receive staking incentives for the epoch in which they were removed which is against the intended behaviour

mediumCode4rena

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/tokenomics/contracts/Dispenser.sol#L393 https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/tokenomics/contracts/Dispenser.sol#L849 https://github.com/code-423n4/2024-05-olas/blame/3ce502ec8b475885b90668e617f3983cea3ae29f/tokenomics/contracts/Dispenser.sol#L774

Vulnerability details

Impact

If a nominee is removed in the ith epoch then they are eligible to receive staking incentives for the ith epoch if they had enough staking weight but due to current implementation the removed nominee would never receive staking incentives for the ith epoch thus causing loss of staking incentives for the nominee. It is evident from the code that it is intended that the nominee is eligible to receive staking incentives for the epoch when it was removed.Plus I have also asked from the sponsers and they have agreed too that nominee is eligible to receive incentives for that epoch.

Proof of Concept

Following is calculateStaking incentives function

solidity📋
function calculateStakingIncentives(
        uint256 numClaimedEpochs,
        uint256 chainId,
        bytes32 stakingTarget,
        uint256 bridgingDecimals
    ) public returns (
        uint256 totalStakingIncentive,
        uint256 totalReturnAmount,
        uint256 lastClaimedEpoch,
        bytes32 nomineeHash
    ) {
        // Check for the correct chain Id
        if (chainId == 0) {
            revert ZeroValue();
        }

        // Check for the zero address
        if (stakingTarget == 0) {
            revert ZeroAddress();
        }

        // Get the staking target nominee hash
        nomineeHash = keccak256(abi.encode(IVoteWeighting.Nominee(stakingTarget, chainId)));

        uint256 firstClaimedEpoch;
        (firstClaimedEpoch, lastClaimedEpoch) =
            _checkpointNomineeAndGetClaimedEpochCounters(nomineeHash, numClaimedEpochs);

        // Checkpoint staking target nominee in the Vote Weighting contract
        IVoteWeighting(voteWeighting).checkpointNominee(stakingTarget, chainId);

        // Traverse all the claimed epochs
        for (uint256 j = firstClaimedEpoch; j < lastClaimedEpoch; ++j) {
            // Get service staking info
            ITokenomics.StakingPoint memory stakingPoint =
                ITokenomics(tokenomics).mapEpochStakingPoints(j);

            uint256 endTime = ITokenomics(tokenomics).getEpochEndTime(j);

            // Get the staking weight for each epoch and the total weight
            // Epoch endTime is used to get the weights info, since otherwise there is a risk of having votes
            // accounted for from the next epoch
            // totalWeightSum is the overall veOLAS power (bias) across all the voting nominees
            (uint256 stakingWeight, uint256 totalWeightSum) =
                IVoteWeighting(voteWeighting).nomineeRelativeWeight(stakingTarget, chainId, endTime);

            uint256 stakingIncentive;
            uint256 returnAmount;

            // Adjust the inflation by the maximum amount of veOLAS voted for service staking contracts
            // If veOLAS power is lower, it reflects the maximum amount of OLAS allocated for staking
            // such that all the inflation is not distributed for a minimal veOLAS power
            uint256 availableStakingAmount = stakingPoint.stakingIncentive;

            uint256 stakingDiff;
            if (availableStakingAmount > totalWeightSum) {
                stakingDiff = availableStakingAmount - totalWeightSum;
                availableStakingAmount = totalWeightSum;
            }

            // Compare the staking weight
            // 100% = 1e18, in order to compare with minStakingWeight we need to bring it from the range of 0 .. 10_000
            if (stakingWeight < uint256(stakingPoint.minStakingWeight) * 1e14) {
                // If vote weighting staking weight is lower than the defined threshold - return the staking incentive
                returnAmount = ((stakingDiff + availableStakingAmount) * stakingWeight) / 1e18;
            } else {
                // Otherwise, allocate staking incentive to corresponding contracts
                stakingIncentive = (availableStakingAmount * stakingWeight) / 1e18;
                // Calculate initial return amount, if stakingDiff > 0
                returnAmount = (stakingDiff * stakingWeight) / 1e18;

                // availableStakingAmount is not used anymore and can serve as a local maxStakingAmount
                availableStakingAmount = stakingPoint.maxStakingAmount;
                if (stakingIncentive > availableStakingAmount) {
                    // Adjust the return amount
                    returnAmount += stakingIncentive - availableStakingAmount;
                    // Adjust the staking incentive
                    stakingIncentive = availableStakingAmount;
                }

                // Normalize staking incentive if there is a bridge decimals limiting condition
                // Note: only OLAS decimals must be considered
                if (bridgingDecimals < 18) {
                    uint256 normalizedStakingAmount = stakingIncentive / (10 ** (18 - bridgingDecimals));
                    normalizedStakingAmount *= 10 ** (18 - bridgingDecimals);
                    // Update return amounts
                    // stakingIncentive is always bigger or equal than normalizedStakingAmount
                    returnAmount += stakingIncentive - normalizedStakingAmount;
                    // Downsize staking incentive to a specified number of bridging decimals
                    stakingIncentive = normalizedStakingAmount;
                }
                // Adjust total staking incentive amount
                totalStakingIncentive += stakingIncentive;
            }
            // Adjust total return amount
            totalReturnAmount += returnAmount;
        }
    }

From the above for loop it is visible that the staking incentives are calculated till lastClaimedEpoch-1.

solidity📋
for (uint256 j = firstClaimedEpoch; j < lastClaimedEpoch; ++j) {

Following function is used to check for which epoches is the nominee eligible to receive the staking incentives.

solidity📋
 (firstClaimedEpoch, lastClaimedEpoch) =
            _checkpointNomineeAndGetClaimedEpochCounters(nomineeHash, numClaimedEpochs);

solidity📋
function _checkpointNomineeAndGetClaimedEpochCounters(
        bytes32 nomineeHash,
        uint256 numClaimedEpochs
    ) internal view returns (uint256 firstClaimedEpoch, uint256 lastClaimedEpoch) {
        // Get the current epoch number
        uint256 eCounter = ITokenomics(tokenomics).epochCounter();

        // Get the first claimed epoch, which is equal to the last claiming one
        firstClaimedEpoch = mapLastClaimedStakingEpochs[nomineeHash];

        // This must never happen as the nominee gets enabled when added to Vote Weighting
        // This is only possible if the dispenser has been unset in Vote Weighting for some time
        // In that case the check is correct and those nominees must not be considered
        if (firstClaimedEpoch == 0) {
            revert ZeroValue();
        }

        // Must not claim in the ongoing epoch
        if (firstClaimedEpoch == eCounter) {
            // Epoch counter is never equal to zero
            revert Overflow(firstClaimedEpoch, eCounter - 1);
        }

        // We still need to claim for the epoch number following the one when the nominee was removed
        uint256 epochAfterRemoved = mapRemovedNomineeEpochs[nomineeHash] + 1;
        // If the nominee is not removed, its value in the map is always zero, unless removed
        // The staking contract nominee cannot be removed in the zero-th epoch by default
        if (epochAfterRemoved > 1 && firstClaimedEpoch >= epochAfterRemoved) {
            revert Overflow(firstClaimedEpoch, epochAfterRemoved - 1);
        }

        // Get a number of epochs to claim for based on the maximum number of epochs claimed
        lastClaimedEpoch = firstClaimedEpoch + numClaimedEpochs;

        // Limit last claimed epoch by the number following the nominee removal epoch
        // The condition for is lastClaimedEpoch strictly > because the lastClaimedEpoch is not included in claiming
        if (epochAfterRemoved > 1 && lastClaimedEpoch > epochAfterRemoved) {
            lastClaimedEpoch = epochAfterRemoved;
        }

        // Also limit by the current counter, if the nominee was removed in the current epoch
        if (lastClaimedEpoch > eCounter) {
            lastClaimedEpoch = eCounter;
        }
    }

From the following if condition it is clear that nominee is also eligible to receive staking incentives for the epoch in which it was removed

solidity📋
 if (epochAfterRemoved > 1 && lastClaimedEpoch > epochAfterRemoved) {
            lastClaimedEpoch = epochAfterRemoved;
        }

So as initially for loop ran till lastClaimedEpoch -1 thus it is evident that removed nominnee is eligible for the staking incentives in which it was removed too.

Now lets see what happens when a nominee is removed Firstly it is removed in VoteWeighting.sol in removeNominee function which is as follows

solidity📋
function removeNominee(bytes32 account, uint256 chainId) external {
        // Check for the contract ownership
        if (msg.sender != owner) {
            revert OwnerOnly(owner, msg.sender);
        }

        // Get the nominee struct and hash
        Nominee memory nominee = Nominee(account, chainId);
        bytes32 nomineeHash = keccak256(abi.encode(nominee));

        // Get the nominee id in the nominee set
        uint256 id = mapNomineeIds[nomineeHash];
        if (id == 0) {
            revert NomineeDoesNotExist(account, chainId);
        }

        // Set nominee weight to zero
        uint256 oldWeight = _getWeight(account, chainId);
        uint256 oldSum = _getSum();
        uint256 nextTime = (block.timestamp + WEEK) / WEEK * WEEK;
        pointsWeight[nomineeHash][nextTime].bias = 0;
        timeWeight[nomineeHash] = nextTime;

        // Account for the the sum weight change
        uint256 newSum = oldSum - oldWeight;
        pointsSum[nextTime].bias = newSum;
        timeSum = nextTime;

        // Add to the removed nominee map and set
        mapRemovedNominees[nomineeHash] = setRemovedNominees.length;
        setRemovedNominees.push(nominee);

        // Remove nominee from the map
        mapNomineeIds[nomineeHash] = 0;

        // Shuffle the current last nominee id in the set to be placed to the removed one
        nominee = setNominees[setNominees.length - 1];
        bytes32 replacedNomineeHash = keccak256(abi.encode(nominee));
        mapNomineeIds[replacedNomineeHash] = id;
        setNominees[id] = nominee;
        // Pop the last element from the set
        setNominees.pop();

        // Remove nominee in dispenser, if applicable
        address localDispenser = dispenser;
        if (localDispenser != address(0)) {
            IDispenser(localDispenser).removeNominee(nomineeHash);
        }

        emit RemoveNominee(account, chainId, newSum);
    }

Key thing to note is that if a nominee is removed in ith week then its weight/bias in the upcoming weeks is set to zero i.e from i+1 weeks it will have no weight

uint256 nextTime = (block.timestamp + WEEK) / WEEK * WEEK;
        pointsWeight[nomineeHash][nextTime].bias = 0;

Also remove nominee in dispensor is also called which is as follows

solidity📋
function removeNominee(bytes32 nomineeHash) external {
        // Check for the contract ownership
        if (msg.sender != voteWeighting) {
            revert ManagerOnly(msg.sender, voteWeighting);
        }

        // Check for the retainer hash
        if (retainerHash == nomineeHash) {
            revert WrongAccount(retainer);
        }

        // Get the epoch counter
        uint256 eCounter = ITokenomics(tokenomics).epochCounter();

        // Get the previous epoch end time
        // Epoch counter is never equal to zero
        uint256 endTime = ITokenomics(tokenomics).getEpochEndTime(eCounter - 1);

        // Get the epoch length
        uint256 epochLen = ITokenomics(tokenomics).epochLen();

        // Check that there is more than one week before the end of the ongoing epoch
        // Note that epochLen cannot be smaller than one week as per specified limits
        uint256 maxAllowedTime = endTime + epochLen - 1 weeks;
        if (block.timestamp >= maxAllowedTime) {
            revert Overflow(block.timestamp, maxAllowedTime);
        }

        // Set the removed nominee epoch number
        mapRemovedNomineeEpochs[nomineeHash] = eCounter;
    }

Key thing to note here is the following condition

solidity📋
uint256 maxAllowedTime = endTime + epochLen - 1 weeks;
        if (block.timestamp >= maxAllowedTime) {
            revert Overflow(block.timestamp, maxAllowedTime);
        }

From the above condition it is clear that it is only allowed to remove a nominee if the time left to epoch end is greater than a week. In simpler terms if the epoch end time is scheduled to be at ith week then the nominee can only be removed in the weeks before the ith week(not inlcuding the ith week).

As we have seen from above that if a nominee is removed in ith week then its weight in the weeks starting from the i+1th week will be zero.

So now when the staking incentives are calculated for the removed nominee for the epoch in which they were removed it would always return zero as the weight of the nominee will always be zero.

 ITokenomics.StakingPoint memory stakingPoint =
                ITokenomics(tokenomics).mapEpochStakingPoints(j);

            uint256 endTime = ITokenomics(tokenomics).getEpochEndTime(j);

            // Get the staking weight for each epoch and the total weight
            // Epoch endTime is used to get the weights info, since otherwise there is a risk of having votes
            // accounted for from the next epoch
            // totalWeightSum is the overall veOLAS power (bias) across all the voting nominees
     (uint256 stakingWeight, uint256 totalWeightSum) =
         IVoteWeighting(voteWeighting).nomineeRelativeWeight(stakingTarget, chainId, endTime);

(@==>   /// here staking weight will be zero for the removed epoch.  )     

In the current code base as it is intended that the removed nominee is eligible for the epoch in which they were removed and currently they are not receiving it thus it is a issue/bug.

Tools Used

Manual review

Recommended Mitigation Steps

The most easy way would be to not allow staking incentives for the epoch in which they were removed or you can do the following Allow removing a nominee only when there is less than a week time left before the epoch ends for that only change the following if condition https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/tokenomics/contracts/Dispenser.sol#L774 to if (block.timestamp < maxAllowedTime) { revert ; } But in this case too staking incentives can be lost if the epoch didn't end for few more weeks.

Assessed type

Context