Lines of code

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingBase.sol#L823 https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingBase.sol#L840

Vulnerability details

Impact

When unstake function is called for a specific service id checkpoint function is called and if the checkpoint is successful then the serviceIds array retrieved from the checkpoint function might be outdated/stale because of eviction of some service ids. Due to the use of outdated serviceIds array unstake function reverts.

Proof of Concept

Following is the unstake function

function unstake(uint256 serviceId) external returns (uint256 reward) {
        ServiceInfo storage sInfo = mapServiceInfo[serviceId];
        // Check for the service ownership
        if (msg.sender != sInfo.owner) {
            revert OwnerOnly(msg.sender, sInfo.owner);
        }

        // Get the staking start time
        // Note that if the service info exists, the service is staked or evicted, and thus start time is always valid
        uint256 tsStart = sInfo.tsStart;

        // Check that the service has staked long enough, or if there are no rewards left
        uint256 ts = block.timestamp - tsStart;
        if (ts <= minStakingDuration && availableRewards > 0) {
            revert NotEnoughTimeStaked(serviceId, ts, minStakingDuration);
        }

        // Call the checkpoint
        (uint256[] memory serviceIds, , , , uint256[] memory evictServiceIds) = checkpoint();

        // If the checkpoint was not successful, the serviceIds set is not returned and needs to be allocated
        if (serviceIds.length == 0) {
            serviceIds = getServiceIds();
            evictServiceIds = new uint256[](serviceIds.length);
        }

        // Get the service index in the set of services
        // The index must always exist as the service is currently staked, otherwise it has no record in the map
        uint256 idx;
        bool inSet;
        for (; idx < serviceIds.length; ++idx) {
            // Service is still in a global staking set if it is found in the services set,
            // and is not present in the evicted set
            if (evictServiceIds[idx] == serviceId) {
                break;
            } else if (serviceIds[idx] == serviceId) {
                inSet = true;
                break;
            }
        }

        // Get the unstaked service data
        reward = sInfo.reward;
        uint256[] memory nonces = sInfo.nonces;
        address multisig = sInfo.multisig;

        // Clear all the data about the unstaked service
        // Delete the service info struct
        delete mapServiceInfo[serviceId];

        // Update the set of staked service Ids
        // If the index was not found, the service was evicted and is not part of staked services set
        if (inSet) {
            setServiceIds[idx] = setServiceIds[setServiceIds.length - 1];
            setServiceIds.pop();
        }

        // Transfer the service back to the owner
        // Note that the reentrancy is not possible due to the ServiceInfo struct being deleted
        IService(serviceRegistry).safeTransferFrom(address(this), msg.sender, serviceId);

        // Transfer accumulated rewards to the service multisig
        if (reward > 0) {
            _withdraw(multisig, reward);
        }

        emit ServiceUnstaked(epochCounter, serviceId, msg.sender, multisig, nonces, reward);
    }

In order to show the error lets select the serviceId to be removed is the last in setServiceIds array. Lets assume that the current setServiceIds (global array) is of length 10. So the service id need to be unstake is setServiceIds[9].The owner of setServiceIds[9] calls the unstake function.

As can be seen from the following lines checkpoint function is called in order to retrieve serviceIds and evictServiceIds(i.e the service ids that have been evicted)

// Call the checkpoint
        (uint256[] memory serviceIds, , , , uint256[] memory evictServiceIds) = checkpoint();

Vulnerability lies in the serviceIds retrieved so lets look at how it is retrieved. Now lets look at the checkpoint function

 function checkpoint() public returns (
        uint256[] memory,
        uint256[][] memory,
        uint256[] memory,
        uint256[] memory,
        uint256[] memory evictServiceIds
    )
    {
        // Calculate staking rewards
        (uint256 lastAvailableRewards, uint256 numServices, uint256 totalRewards,
            uint256[] memory eligibleServiceIds, uint256[] memory eligibleServiceRewards,
            uint256[] memory serviceIds, uint256[][] memory serviceNonces,
            uint256[] memory serviceInactivity) = _calculateStakingRewards();

        // Get arrays for eligible service Ids and rewards of exact size
        uint256[] memory finalEligibleServiceIds;
        uint256[] memory finalEligibleServiceRewards;
        evictServiceIds = new uint256[](serviceIds.length);
        uint256 curServiceId;

        // If there are eligible services, proceed with staking calculation and update rewards
        if (numServices > 0) {
            finalEligibleServiceIds = new uint256[](numServices);
            finalEligibleServiceRewards = new uint256[](numServices);
            // If total allocated rewards are not enough, adjust the reward value
            if (totalRewards > lastAvailableRewards) {
                // Traverse all the eligible services and adjust their rewards proportional to leftovers
                uint256 updatedReward;
                uint256 updatedTotalRewards;
                for (uint256 i = 1; i < numServices; ++i) {
                    // Calculate the updated reward
                    updatedReward = (eligibleServiceRewards[i] * lastAvailableRewards) / totalRewards;
                    // Add to the total updated reward
                    updatedTotalRewards += updatedReward;
                    // Add reward to the overall service reward
                    curServiceId = eligibleServiceIds[i];
                    finalEligibleServiceIds[i] = eligibleServiceIds[i];
                    finalEligibleServiceRewards[i] = updatedReward;
                    mapServiceInfo[curServiceId].reward += updatedReward;
                }

                // Process the first service in the set
                updatedReward = (eligibleServiceRewards[0] * lastAvailableRewards) / totalRewards;
                updatedTotalRewards += updatedReward;
                curServiceId = eligibleServiceIds[0];
                finalEligibleServiceIds[0] = eligibleServiceIds[0];
                // If the reward adjustment happened to have small leftovers, add it to the first service
                if (lastAvailableRewards > updatedTotalRewards) {
                    updatedReward += lastAvailableRewards - updatedTotalRewards;
                }
                finalEligibleServiceRewards[0] = updatedReward;
                // Add reward to the overall service reward
                mapServiceInfo[curServiceId].reward += updatedReward;
                // Set available rewards to zero
                lastAvailableRewards = 0;
            } else {
                // Traverse all the eligible services and add to their rewards
                for (uint256 i = 0; i < numServices; ++i) {
                    // Add reward to the service overall reward
                    curServiceId = eligibleServiceIds[i];
                    finalEligibleServiceIds[i] = eligibleServiceIds[i];
                    finalEligibleServiceRewards[i] = eligibleServiceRewards[i];
                    mapServiceInfo[curServiceId].reward += eligibleServiceRewards[i];
                }

                // Adjust available rewards
                lastAvailableRewards -= totalRewards;
            }

            // Update the storage value of available rewards
            availableRewards = lastAvailableRewards;
        }

        // If service Ids are returned, then the checkpoint takes place
        if (serviceIds.length > 0) {
            uint256 eCounter = epochCounter;
            numServices = 0;
            // Record service inactivities and updated current service nonces
            for (uint256 i = 0; i < serviceIds.length; ++i) {
                // Get the current service Id
                curServiceId = serviceIds[i];
                // Record service nonces
                mapServiceInfo[curServiceId].nonces = serviceNonces[i];

                // Increase service inactivity if it is greater than zero
                if (serviceInactivity[i] > 0) {
                    // Get the overall continuous service inactivity
                    serviceInactivity[i] = mapServiceInfo[curServiceId].inactivity + serviceInactivity[i];
                    mapServiceInfo[curServiceId].inactivity = serviceInactivity[i];
                    // Check for the maximum allowed inactivity time
                    if (serviceInactivity[i] > maxInactivityDuration) {
                        // Evict a service if it has been inactive for more than a maximum allowed inactivity time
                        evictServiceIds[i] = curServiceId;
                        // Increase number of evicted services
                        numServices++;
                    } else {
                        emit ServiceInactivityWarning(eCounter, curServiceId, serviceInactivity[i]);
                    }
                } else {
                    // Otherwise, set it back to zero
                    mapServiceInfo[curServiceId].inactivity = 0;
                }
            }

            // Evict inactive services
            if (numServices > 0) {
                _evict(evictServiceIds, serviceInactivity, numServices);
            }

            // Record the actual epoch length
            uint256 epochLength = block.timestamp - tsCheckpoint;
            // Record the current timestamp such that next calculations start from this point of time
            tsCheckpoint = block.timestamp;

            // Increase the epoch counter
            epochCounter = eCounter + 1;

            emit Checkpoint(eCounter, lastAvailableRewards, finalEligibleServiceIds, finalEligibleServiceRewards,
                epochLength);
        }

        return (serviceIds, serviceNonces, finalEligibleServiceIds, finalEligibleServiceRewards, evictServiceIds);
    }

From the following lines we can see that serviceIds is retrieved from _calculateStakingRewards

// Calculate staking rewards
        (uint256 lastAvailableRewards, uint256 numServices, uint256 totalRewards,
            uint256[] memory eligibleServiceIds, uint256[] memory eligibleServiceRewards,
   ===>      uint256[] memory serviceIds, uint256[][] memory serviceNonces,
            uint256[] memory serviceInactivity) = _calculateStakingRewards();

_calculateStakingRewards function is as follows

function _calculateStakingRewards() internal view returns (
        uint256 lastAvailableRewards,
        uint256 numServices,
        uint256 totalRewards,
        uint256[] memory eligibleServiceIds,
        uint256[] memory eligibleServiceRewards,
        uint256[] memory serviceIds,
        uint256[][] memory serviceNonces,
        uint256[] memory serviceInactivity
    )
    {
        // Check the last checkpoint timestamp and the liveness period, also check for available rewards to be not zero
        uint256 tsCheckpointLast = tsCheckpoint;
        lastAvailableRewards = availableRewards;
        if (block.timestamp - tsCheckpointLast >= livenessPeriod && lastAvailableRewards > 0) {
            // Get the service Ids set length
            uint256 size = setServiceIds.length;

            // Get necessary arrays
            serviceIds = new uint256[](size);
            eligibleServiceIds = new uint256[](size);
            eligibleServiceRewards = new uint256[](size);
            serviceNonces = new uint256[][](size);
            serviceInactivity = new uint256[](size);

            // Calculate each staked service reward eligibility
            for (uint256 i = 0; i < size; ++i) {
                // Get current service Id
                serviceIds[i] = setServiceIds[i];

                // Get the service info
                ServiceInfo storage sInfo = mapServiceInfo[serviceIds[i]];

                // Calculate the liveness nonce ratio
                // Get the last service checkpoint: staking start time or the global checkpoint timestamp
                uint256 serviceCheckpoint = tsCheckpointLast;
                uint256 ts = sInfo.tsStart;
                // Adjust the service checkpoint time if the service was staking less than the current staking period
                if (ts > serviceCheckpoint) {
                    serviceCheckpoint = ts;
                }

                // Calculate the liveness ratio in 1e18 value
                // This subtraction is always positive or zero, as the last checkpoint is at most block.timestamp
                ts = block.timestamp - serviceCheckpoint;

                bool ratioPass;
                (ratioPass, serviceNonces[i]) = _checkRatioPass(sInfo.multisig, sInfo.nonces, ts);

                // Record the reward for the service if it has provided enough transactions
                if (ratioPass) {
                    // Calculate the reward up until now and record its value for the corresponding service
                    eligibleServiceRewards[numServices] = rewardsPerSecond * ts;
                    totalRewards += eligibleServiceRewards[numServices];
                    eligibleServiceIds[numServices] = serviceIds[i];
                    ++numServices;
                } else {
                    serviceInactivity[i] = ts;
                }
            }
        }
    }

Now if the following check passes i.e if enough time has passed for new epoch to occur if (block.timestamp - tsCheckpointLast >= livenessPeriod && lastAvailableRewards > 0) then as initially as we have assumed that the length of setServiceIds.length = 10 therefore serviceIds length is equal to 10 as can be seen from the following line

// Get necessary arrays
            serviceIds = new uint256[](size);

As we can see from the above function serviceIds array is just a copy of global setServiceIds array

serviceIds[i] = setServiceIds[i];

Note if the liveness ratio doesn't passes then the serviceInactivity of ith service id is increased as can be seen from the above function

               if (ratioPass) {
                    // Calculate the reward up until now and record its value for the corresponding service
                    eligibleServiceRewards[numServices] = rewardsPerSecond * ts;
                    totalRewards += eligibleServiceRewards[numServices];
                    eligibleServiceIds[numServices] = serviceIds[i];
                    ++numServices;
                } else {
                    serviceInactivity[i] = ts;
                }

Now lets go back to the checkpoint function till now serviceIds is just the copy of global setServiceIds array so till now setServiceIds and serviceIds contain same elements and thus are exact copies.

lets focus on the following lines of code

if (serviceIds.length > 0) {
            uint256 eCounter = epochCounter;
            numServices = 0;
            // Record service inactivities and updated current service nonces
            for (uint256 i = 0; i < serviceIds.length; ++i) {
                // Get the current service Id
                curServiceId = serviceIds[i];
                // Record service nonces
                mapServiceInfo[curServiceId].nonces = serviceNonces[i];

                // Increase service inactivity if it is greater than zero
                if (serviceInactivity[i] > 0) {
                    // Get the overall continuous service inactivity
                    serviceInactivity[i] = mapServiceInfo[curServiceId].inactivity + serviceInactivity[i];
                    mapServiceInfo[curServiceId].inactivity = serviceInactivity[i];
                    // Check for the maximum allowed inactivity time
                    if (serviceInactivity[i] > maxInactivityDuration) {
                        // Evict a service if it has been inactive for more than a maximum allowed inactivity time
                        evictServiceIds[i] = curServiceId;
                        // Increase number of evicted services
                        numServices++;
                    } else {
                        emit ServiceInactivityWarning(eCounter, curServiceId, serviceInactivity[i]);
                    }
                } else {
                    // Otherwise, set it back to zero
                    mapServiceInfo[curServiceId].inactivity = 0;
                }
            }

Now from above we can clearly see that if for some service ids if they have exceeded the max inactivity period then that service id is added to the evictServiceIds array and accordingly numServices which are needed to be evicted are increased.

Now suppose some ids are needed to be evicted i.e numServices >0 thus _evict function is called as follows

// Evict inactive services
            if (numServices > 0) {
                _evict(evictServiceIds, serviceInactivity, numServices);
            }

lets see the _evict function

function _evict(
        uint256[] memory evictServiceIds,
        uint256[] memory serviceInactivity,
        uint256 numEvictServices
    ) internal {
        // Get the total number of staked services
        // All the passed arrays have the length of the number of staked services
        uint256 totalNumServices = evictServiceIds.length;

        // Get arrays of exact sizes
        uint256[] memory serviceIds = new uint256[](numEvictServices);
        address[] memory owners = new address[](numEvictServices);
        address[] memory multisigs = new address[](numEvictServices);
        uint256[] memory inactivity = new uint256[](numEvictServices);
        uint256[] memory serviceIndexes = new uint256[](numEvictServices);

        // Fill in arrays
        uint256 sCounter;
        uint256 serviceId;
        for (uint256 i = 0; i < totalNumServices; ++i) {
            if (evictServiceIds[i] > 0) {
                serviceId = evictServiceIds[i];
                serviceIds[sCounter] = serviceId;

                ServiceInfo storage sInfo = mapServiceInfo[serviceId];
                owners[sCounter] = sInfo.owner;
                multisigs[sCounter] = sInfo.multisig;
                inactivity[sCounter] = serviceInactivity[i];
                serviceIndexes[sCounter] = i;
                sCounter++;
            }
        }

        // Evict services from the global set of staked services
        for (uint256 i = numEvictServices; i > 0; --i) {
            // Decrease the number of services
            totalNumServices--;
            // Get the evicted service index
            uint256 idx = serviceIndexes[i - 1];
            // Assign last service Id to the index that points to the evicted service Id
            setServiceIds[idx] = setServiceIds[totalNumServices];
            // Pop the last element
            setServiceIds.pop();
        }

        emit ServicesEvicted(epochCounter, serviceIds, owners, multisigs, inactivity);
    }

Key thing note in the evict function is that it modifies the setServiceIds global array essentially it shrinks the array as it removes the evicted services ids as can be seen from the following line of code

for (uint256 i = numEvictServices; i > 0; --i) {
            // Decrease the number of services
            totalNumServices--;
            // Get the evicted service index
            uint256 idx = serviceIndexes[i - 1];
            // Assign last service Id to the index that points to the evicted service Id
            setServiceIds[idx] = setServiceIds[totalNumServices];
            // Pop the last element
            setServiceIds.pop(); <===== shrinkage of setServiceIds array
        }

So now serviceIds array and the global setServiceIds array are not same. Neither the index of service ids is same nor the size of both the arrays because there is removal of atleast 1 service id because _evict function was called in the checkpoint function.

Now the following variables are returned by the checkpoint function

return (serviceIds, serviceNonces, finalEligibleServiceIds, finalEligibleServiceRewards, evictServiceIds);

Note that the it returns the serviceIds array which is not updated to the global setServiceIds array and thus contain more elements than setServiceIds array.

Now lets go back to the unstake function

Following loop is run in order to check if the service id to be removed is already evicted or is still present in the global setServiceIds array.

uint256 idx;
        bool inSet;
        for (; idx < serviceIds.length; ++idx) {
            // Service is still in a global staking set if it is found in the services set,
            // and is not present in the evicted set
            if (evictServiceIds[idx] == serviceId) {
                break;
            } else if (serviceIds[idx] == serviceId) {
                inSet = true;
                break;
            }
        }

Now if the service id to be removed is not evicted then it will be present in the global service ids array but as can be seen from the above the old outdated/stale serviceIds array is used to identify the index of the service id to be evicted. Here is where the issue arises because the index of service id can be different in old/outdated serviceIds array and global setServicesIds array because of eviction of some service ids.

For example initially setServiceIds and serviceIds were exact copy of each other and had 10 service ids i.e size of both the arrays was same. Now suppose service id at index 3 was removed. so now setServiceIds has length 9 and serviceIds has length 10. Also the service id which was at earlier at index 9 in setServicesIds global array is now at index 3 because of how eviction is done using pop functionality. Whereas service id which was earlier at index 9 remains at the same index in service ids array as it is outdated.

Now suppose the owner of service id which is at index 9 in the outdated service and at index 3 in the global array after eviction wants to unstake the service id then it would revert because the above loop will return the index 9 so then the following code will be executed

if (inSet) {
            setServiceIds[idx] = setServiceIds[setServiceIds.length - 1];
            setServiceIds.pop();
        }

but the above code will revert because of out of bound error because the above code essentially does the following setServiceIds[9] = setServiceIds[9-1] = setServiceIds[8]

Note that there could be more than one ids evicted so then also unstaking of serivce ids at the second last index would revert.

Tools Used

Manual review

Recommended Mitigation Steps

Make the following change in the checkpoint function where the eviction condition is checked

 // Evict inactive services
            if (numServices > 0) {
                _evict(evictServiceIds, serviceInactivity, numServices);
                serviceIds = getServiceIds();
            }

Assessed type

Context

code-423n4 / 2024-05-olas-findings

Unstake function reverts because of use of outdated/stale serviceIds array #57