checkpoint function is not called before staking which can cause loss of rewards for already staked services.

mediumCode4rena

Lines of code

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

Vulnerability details

Impact

Before allowing anyone to stake a service id checkpoint function should be called in order to distribute already present rewards in the contract and thus update the availableRewards because if this doesn't happen and a new service id is staked then after some time the rewards are then shared among the new service id as well thus causing the service ids which were staked before the new service id to receive less rewards. Further as there is no mention who calls checkpoint function and when is it called likelihood of this error is high and impact is loss of rewards thus impact is also high.

Proof of Concept

Following is stake function

solidity📋
function stake(uint256 serviceId) external {
        // Check if there available rewards
        if (availableRewards == 0) {
            revert NoRewardsAvailable();
        }

        // Check if the evicted service has not yet unstaked
        ServiceInfo storage sInfo = mapServiceInfo[serviceId];
        // tsStart being greater than zero means that the service was not yet unstaked: still staking or evicted
        if (sInfo.tsStart > 0) {
            revert ServiceNotUnstaked(serviceId);
        }

        // Check for the maximum number of staking services
        uint256 numStakingServices = setServiceIds.length;
        if (numStakingServices == maxNumServices) {
            revert MaxNumServicesReached(maxNumServices);
        }

        // Check the service conditions for staking
        IService.Service memory service = IService(serviceRegistry).getService(serviceId);

        // Check the number of agent instances
        if (numAgentInstances != service.maxNumAgentInstances) {
            revert WrongServiceConfiguration(serviceId);
        }

        // Check the configuration hash, if applicable
        if (configHash != 0 && configHash != service.configHash) {
            revert WrongServiceConfiguration(serviceId);
        }
        // Check the threshold, if applicable
        if (threshold > 0 && threshold != service.threshold) {
            revert WrongServiceConfiguration(serviceId);
        }
        // The service must be deployed
        if (service.state != IService.ServiceState.Deployed) {
            revert WrongServiceState(uint256(service.state), serviceId);
        }

        // Check that the multisig address corresponds to the authorized multisig proxy bytecode hash
        bytes32 multisigProxyHash = keccak256(service.multisig.code);
        if (proxyHash != multisigProxyHash) {
            revert UnauthorizedMultisig(service.multisig);
        }

        // Check the agent Ids requirement, if applicable
        uint256 size = agentIds.length;
        if (size > 0) {
            uint256 numAgents = service.agentIds.length;

            if (size != numAgents) {
                revert WrongServiceConfiguration(serviceId);
            }
            for (uint256 i = 0; i < numAgents; ++i) {
                // Check that the agent Ids
                if (agentIds[i] != service.agentIds[i]) {
                    revert WrongAgentId(agentIds[i]);
                }
            }
        }

        // Check service staking deposit and token, if applicable
        _checkTokenStakingDeposit(serviceId, service.securityDeposit, service.agentIds);

        // ServiceInfo struct will be an empty one since otherwise the safeTransferFrom above would fail
        sInfo.multisig = service.multisig;
        sInfo.owner = msg.sender;
        // This function might revert if it's incorrectly implemented, however this is not a protocol's responsibility
        // It is safe to revert in this place
        uint256[] memory nonces = IActivityChecker(activityChecker).getMultisigNonces(service.multisig);
        sInfo.nonces = nonces;
        sInfo.tsStart = block.timestamp;

        // Add the service Id to the set of staked services
        setServiceIds.push(serviceId);

        // Transfer the service for staking
        IService(serviceRegistry).safeTransferFrom(msg.sender, address(this), serviceId);

        emit ServiceStaked(epochCounter, serviceId, msg.sender, service.multisig, nonces);
    }

As we can see clearly checkpoint function is not called anywhere in the stake function. From above we can also see that the staking is not allowed if there are no availableRewards. So in order to get the latest and the correct value of the available rewards checkpoint function should be called at the start of stake function and then it should be checked whether availableRewards are zero or not so as to decide whether staking should be allowed or not.

Lets explain this issue with a simple example Here suppose liveness period has passed and checkpoint can be called but is not called . Suppose available rewards left are 300 and there are 3 current services. let liveness period = 10 and let rewards per sec = 10 So as of now all the 3 servies should receive 10 * 10[i have assumed that the staking time to be exact 10 sec can be any value] = 100 rewards each and thus the available rewards should become 0 but as checkpoint is not called before stake function and new sevice id is staked. Legitimtely new service should not be able to stake as all the available rewards should go to the already present three service ids but now as checkpoint is not called before calling stake function new service id is also staked. Now 5 more seconds have passed and now checkpoint is called, now already present three service ids would be eligible for rewards = 10 * 15 = 150 also now suppose the new service id added also passes the liveness ratio therefore it also becomes eligible or rewards = 10 * 5 = 50 thus total rewards would now be equal to 150 * 3 + 50 = 500 whereas total available rewards are 300, Therefore first three serive ids would recieve (150 * 300)/500 = 90 rewards as supposed to original 100 rewards they should be applicable to.Therefore there is loss of rewards for the service ids.

In all the staking protocols all the updation are done before allowing anyone to further stake so as there is no loss of rewards.

Tools Used

Manual Review

Recommended Mitigation Steps

Just call checkpoint function at the start of stake function as follows

solidity📋
function stake(uint256 serviceId) external {
==>         checkpoint();
        // Check if there available rewards
        if (availableRewards == 0) {
            revert NoRewardsAvailable();
        }

        // Check if the evicted service has not yet unstaked
        ServiceInfo storage sInfo = mapServiceInfo[serviceId];
        // tsStart being greater than zero means that the service was not yet unstaked: still staking or evicted
        if (sInfo.tsStart > 0) {
            revert ServiceNotUnstaked(serviceId);
        }

        // Check for the maximum number of staking services
        uint256 numStakingServices = setServiceIds.length;
        if (numStakingServices == maxNumServices) {
            revert MaxNumServicesReached(maxNumServices);
        }

        // Check the service conditions for staking
        IService.Service memory service = IService(serviceRegistry).getService(serviceId);

        // Check the number of agent instances
        if (numAgentInstances != service.maxNumAgentInstances) {
            revert WrongServiceConfiguration(serviceId);
        }

        // Check the configuration hash, if applicable
        if (configHash != 0 && configHash != service.configHash) {
            revert WrongServiceConfiguration(serviceId);
        }
        // Check the threshold, if applicable
        if (threshold > 0 && threshold != service.threshold) {
            revert WrongServiceConfiguration(serviceId);
        }
        // The service must be deployed
        if (service.state != IService.ServiceState.Deployed) {
            revert WrongServiceState(uint256(service.state), serviceId);
        }

        // Check that the multisig address corresponds to the authorized multisig proxy bytecode hash
        bytes32 multisigProxyHash = keccak256(service.multisig.code);
        if (proxyHash != multisigProxyHash) {
            revert UnauthorizedMultisig(service.multisig);
        }

        // Check the agent Ids requirement, if applicable
        uint256 size = agentIds.length;
        if (size > 0) {
            uint256 numAgents = service.agentIds.length;

            if (size != numAgents) {
                revert WrongServiceConfiguration(serviceId);
            }
            for (uint256 i = 0; i < numAgents; ++i) {
                // Check that the agent Ids
                if (agentIds[i] != service.agentIds[i]) {
                    revert WrongAgentId(agentIds[i]);
                }
            }
        }

        // Check service staking deposit and token, if applicable
        _checkTokenStakingDeposit(serviceId, service.securityDeposit, service.agentIds);

        // ServiceInfo struct will be an empty one since otherwise the safeTransferFrom above would fail
        sInfo.multisig = service.multisig;
        sInfo.owner = msg.sender;
        // This function might revert if it's incorrectly implemented, however this is not a protocol's responsibility
        // It is safe to revert in this place
        uint256[] memory nonces = IActivityChecker(activityChecker).getMultisigNonces(service.multisig);
        sInfo.nonces = nonces;
        sInfo.tsStart = block.timestamp;

        // Add the service Id to the set of staked services
        setServiceIds.push(serviceId);

        // Transfer the service for staking
        IService(serviceRegistry).safeTransferFrom(msg.sender, address(this), serviceId);

        emit ServiceStaked(epochCounter, serviceId, msg.sender, service.multisig, nonces);
    }

Assessed type

Context