howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/Tokenomics.sol#L1238

Vulnerability details

Impact

In the Tokenomics smart contract, there is a stakingIncentive param inside of StakingPoint struct that represents the staking incentives for each epoch. Additionally, it's supposed to keep refund amounts after claimStakingIncentives() in Dispenser is called. So, as per specification, if there are some amounts remained after refunds, they'll be transferred in the next epoch. However, this logic is not implemented in the current functionality and the incentives stay in the same epoch.

Proof of Concept

Let's say there were some refunds happen during the current epoch:

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/Tokenomics.sol#L826-833

function refundFromStaking(uint256 amount) external {
        // Check for the dispenser access
        if (dispenser != msg.sender) {
            revert ManagerOnly(msg.sender, depository);
        }

        uint256 eCounter = epochCounter;
        uint256 stakingIncentive = mapEpochStakingPoints[eCounter].stakingIncentive + amount;

So now stakingIncentives accumulates some excessive amounts that were not distributed during the current epoch. So, as per spec, they should be transferred onto the next epoch when calling checkpoint() in Tokenomics smart contract:

Specifically, this logic ensures that OLAS emissions, which have been
decided to be retained by veOLAS holders through voting, are returned to the staking inflation pool for the next epoch

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/Tokenomics.sol#L1235-1238

incentives[7] = mapEpochStakingPoints[eCounter].stakingIncentive;
        // Adding service staking top-ups amount based on a current epoch inflation
        incentives[8] = incentives[7] + (inflationPerEpoch * mapEpochStakingPoints[eCounter].stakingFraction) / 100;
        mapEpochStakingPoints[eCounter].stakingIncentive = uint96(incentives[8]);

As you can see here, the function just takes parameter for this epoch and then updates it for the same epoch. The epochCounter is only increased at the end of the checkpoint:

  epochCounter = uint32(eCounter + 1);

So this creates a deviation from the spec as the rewards are left in the same epoch instead of being transferred onto the next one.

Tools Used

Manual review.

Recommended Mitigation Steps

Make sure the refunds from the current epoch are transferred onto the next one.

Assessed type

Other

kupermind commented 2 months ago

By design, we never claim incentives from the current epoch, as it's still incomplete and the inflation is unknown. We return all the unused staking incentives in the current epoch, such that the next epoch it becomes the last epoch, and that's where the incentives can be claimed from during the next epoch.

In other words, we dump all the unused incentives into the n-th epoch, such that in the epoch n+1-th we start distributing incentives allocated during the n-th epoch.

c4-sponsor commented 2 months ago

kupermind (sponsor) disputed

c4-judge commented 2 months ago

0xA5DF marked the issue as unsatisfactory: Invalid

0xA5DF commented 2 months ago

Closing as per sponsor's comment

rodiontr commented 2 months ago

I think this issue is valid as the staking incentives are assigned the following way:

 incentives[8] = incentives[7] + (inflationPerEpoch * mapEpochStakingPoints[eCounter].stakingFraction) / 100;
 mapEpochStakingPoints[eCounter].stakingIncentive = uint96(incentives[8]);

So if it's epoch 1, we update incentives for epoch 1 (same `eCounter) recalculating the inflation for it and adding the leftover amounts but then we just increase the counter and the leftover incentives are not claimed for the next epoch