Staking contract emissions limit can be bypassed by multiple deposits

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/tokenomics/contracts/staking/DefaultTargetDispenserL2.sol#L160-L186 https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingBase.sol#L356 https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingBase.sol#L266 https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingVerifier.sol#L247 https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingVerifier.sol#L256

Vulnerability details

The DefaultTargetDispenserL2 abstract contract is the base class for all contracts responsible for distributing OLAS tokens to staking contracts on L2. When processing a batch of deposits, it calls verifyInstanceAndGetEmissionsAmount() on the StakingFactory to check if each staking contract is valid and to get the maximum emissions amount allowed for that contract.

The verifyInstanceAndGetEmissionsAmount() function returns the lower of:

1. The emissionsAmount() returned by the staking contract instance, which is calculated on initialization based on the _stakingParams
2. The amount returned by StakingVerifier.getEmissionsAmountLimit(instance), which returns a fixed limit defined when the staking limits are changed

The issue is that both of these amounts only depend on the current configuration, and do not take into account any tokens already deposited into the staking contract. This means that the intended emissions limit can be trivially bypassed by simply spreading deposits across multiple batches.

Impact

Some staking contracts could receive a larger share of OLAS emissions than others, depending on when they claim their staking incentives. This could lead to an uneven distribution of rewards among stakers.

While total emissions are still bounded by other protocol limits, the ability to bypass the per-instance limit could undermine the intended incentive structure. Stakers who are aware of this issue could gain an advantage over those who are not.

Proof of Concept

1. Assume the StakingVerifier emissions limit is set to 1000 tokens for a certain staking contract
2. User calls Dispenser.claimStakingIncentives() with that staking contract as the target when the incentives reach 1000 tokens
3. verifyInstanceAndGetEmissionsAmount() passes and the 1000 tokens are deposited
4. User calls Dispenser.claimStakingIncentives() again with the same target and another 1000 tokens
5. verifyInstanceAndGetEmissionsAmount() passes again since it only looks at the current config, allowing the second 1000 token deposit
6. The staking contract has now received 2000 tokens, bypassing the 1000 token limit

Tools Used

Manual review

Recommended Mitigation Steps

The emissions limit check should take into account tokens already deposited into the staking contract.

One way to do this would be to add a depositedAmount state variable to the staking contracts that gets incremented in the deposit() function. The verifyInstanceAndGetEmissionsAmount() function could then check depositedAmount in addition to the config-based limits.

Alternatively, DefaultTargetDispenserL2 could track deposited amounts itself in a mapping and consult that as part of the emissions limit check.

The exact mitigation depends on the intended scope of the limit - whether it's meant to be a lifetime limit or a per-period limit. But in either case, already deposited amounts need to be factored in.

Assessed type

Other

[kupermind]

By design the inflation staking incentives distribution only cares about not giving an excess of staking inflation per epoch, and does not consider any other deposits to the staking contract. This is just the way to limit overly greedy staking setups.

[c4-sponsor]

kupermind (sponsor) disputed

[0xA5DF]

Hey @kupermind Can you elaborate on why you don't consider this an issue? From what I can tell, there's a limit on how much can be sent and that limit can be bypassed (e.g. instead of making one call to calculateStakingIncentives() with numClaimedEpochs = 10 we do 2 calls with numClaimedEpochs = 5) Are you saying this limit isn't significant?

[kupermind]

@0xA5DF the check is super simple by design. One can't claim for several epochs and expect they are going to dump all that funds on the contract. Knowing the limits and that they have missed specific epochs, staking incentives parties them should claim one by one, or two by two at the maximum, etc., depending on their contract limits. There are so many scenarios when things get cheated on and be so overly complicated in gas if we start accounting for each and every deposit coming from the dispenser contract and other sources.

Note that the check is for the final target dispenser to limit the amounts specifically coming from the tokenomics dispenser accounting for the staking inflation. So by design we consider that the incoming inslation-based staking amount is limited by a specified verifier limit. Otherwise it becomes more and more complex whether we check for one epoch, or several, and how many are several epochs. Plus that easily conflicts with rewardsPerSecond amount.

All in all, the check is per single token transfer per epoch / epochs that are going to be deposited to the staking contract coming from the staking inflation.

[kupermind]

We have run extended statistical tests, and the best choice would be to actually get incentives separately for each epoch. The default deployed contract will have a limit of claiming incentives for a single epoch.

[kupermind]

From the warden's comment:

This means that the intended emissions limit can be trivially bypassed by simply spreading deposits across multiple batches.

This is exactly the case for the protocol. You can't get lower than emissions per a single epoch per a single staking contract. There is one epoch, per one contract, and no duplicated contracts can be sent over in a single batch. Once the epoch is accounted for, there is no way to add to that contract from the same epoch.

If one wants to create 100 different staking contracts and split the inflation per 100 contracts, note that one would nee 100x more veOLAS amount to vote for those contracts to have proportionally same amounts, and create 100x more services to feed on those incentives. Moreover, one staking contract can't transfer funds to another one, and thus they are all independent. This type of scaling is counter incentivizing the idea to cheat the system.

[0xA5DF]

Got it, given that the limit is designed per a single epoch, I'm downgrading to low

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[c4-judge]

0xA5DF marked the issue as grade-c

[0xA5DF]

Moving to #34

[0xEVom]

@0xA5DF this is an interesting situation.

While I understand that the finding isn't accurate as described, it is clear that the mechanism to limit emissions was broken at a core level.

I understood the issue as "emissions limit can be bypassed by multiple deposits", while it seems it would have been more accurately described as "claiming incentives for multiple epochs will result in being unfairly limited by the current emissions limit". These seem like two sides of the same coin to me and only really differ in the intention behind the implementation.

As per the heuristic applied here of "in a world where this issue wasn't reported, could this issue occur" and seeing as this finding allowed the sponsor to identify and correct the issue, I wonder if it wouldn't be reasonable to accept the finding despite the inaccuracies in its stated impact.

[kupermind]

@0xEVom @0xA5DF the issue has not allowed us to identify anything an fix it. It was already in the docs prior to this audit, and those docs pointed out that one epoch per claim is going to be used, as even with one epoch gas-wise we are able to handle only a maximum of 10 distinctive staking targets per batch.

[0xEVom]

Okay, I understood your previous comments as if you were working out a solution to this issue.

@0xA5DF I still think the rest of my argument holds and the the severity of the underlying issue is higher than QA (limit is designed per a single epoch, but more than one epoch can be claimed), but this is entirely your decision.

At least however, could you not mark this report as unsatisfactory, which would negatively affect my accuracy?

[0xA5DF]

Leaving as low, esp. since this is in the docs

At least however, could you not mark this report as unsatisfactory, which would negatively affect my accuracy?

Oh, I'm doing this only in order to move the discussion to the main report I'll check if this still affects the score negatively