`LendingLedger.sol`:`setRewards` can have unpredictable behavior if reward of the current epoch or the past is changed

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-03-neobase/blob/d6e6127e6763b93c23ee95cdf7622fe950d9ed30/src/LendingLedger.sol#L140-L149

Vulnerability details

Impact

The function setRewards is used to set the canto rewards awarded per block by a gauge. This is controlled by the governance, which is a DAO. The function take a range of block numbers, and sets the canto reward rate between those epochs.

function setRewards(
  uint256 _fromEpoch,
  uint256 _toEpoch,
  uint256 _amountPerBlock
) external onlyGovernance {
  require(_fromEpoch % BLOCK_EPOCH == 0 && _toEpoch % BLOCK_EPOCH == 0, "Invalid block number");
  for (uint256 i = _fromEpoch; i <= _toEpoch; i += BLOCK_EPOCH) {
    cantoPerBlock[i] = _amountPerBlock;
  }
}

The issue is that there is no validation of the range of epochs. So users can even vote to change the rewards for epochs in the past. This can lead to unpredictable/undefined behavior of markets.

For example, lets say there are two markets each with 50% gauge weight, thus implying they will receive the same rewards. Say the governance changes the reward rate for the range of epochs 5e6 to 10e6 to 1000 canto/block from the older value of 500 canto/block. Lets assume the current block number corresponds to the epoch 6e6. Also lets assume market A has ben recently updated, but market B hasnt been updated since the last epoch.

Now, if update_market is called, Market A will calculate rewards for the epoch 6e6 as 1000 canto/block, but Market B will calculate rewards for both epochs 5e6 and 6e6 as 1000 canto/block. In this scenario, due to Market B having not been updated recently, it calculates the past rewards differently than Market A. This can lead to unpredictable reward calculations of the markets.

Proof of Concept

Lets assume the current epoch is 6e6, and the governance changes the reward rate for the epochs 5e6 to 10e6 to 1000 canto/block from the older value of 500 canto/block.

Also, assume there are two markets with equal gauge weights, and Market A has been updated recently, but Market B hasnt been updated since the last epoch.

1. Call update_market on both markets
2. Market A was updated after epoch 5e6, so it has calculated rewards for the 5e6 epoch at 500 canto/block.
3. Market B was not updated and calculates the rewards for the 5e6 epoch now, which is 1000 canto/block.
4. Reward calculations of Market A and Market B are different for the 5e6 epoch.

This function is not admin-controlled and is instead controlled by a DAO, so remediation can be slow, and delays can be possible.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check so that the reward rates for only future epochs can be changed.

Assessed type

Math

[c4-judge]

MarioPoneder marked the issue as duplicate of #16

[MarioPoneder]

So users can even vote to change the rewards for epochs in the past. This can lead to unpredictable/undefined behavior of markets.

This present issue relies on Admin mistake.
https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#verdict-severity-standardization-centralization-risks

[c4-judge]

MarioPoneder marked the issue as not a duplicate

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-a

[carrotsmuggler]

This is identical to #16

The basis of rejecting this but accepting #16 is that this deals with admins setting incorrect values, while #16 also deals with the fact that markets might just be out of sync.

The thing is this report ALSO deals with that scenario!

Now, if update_market is called, Market A will calculate rewards for the epoch 6e6 as 1000 canto/block, but Market B will calculate rewards for both epochs 5e6 and 6e6 as 1000 canto/block. In this scenario, due to Market B having not been updated recently, it calculates the past rewards differently than Market A. This can lead to unpredictable reward calculations of the markets.

This above statement is identical to the statement made in #16.

Any market that was not updated on the exact same block that either rewards or the block time parameters are adjusted will have these adjustments retroactively applied, leading to over-estimations or under-estimations of the rewards that should be attributed to the market.

Both issues have the same vector: changes made on an un-updated market. Both issues have the same POC, which highlights the fact that the changes have been made on an un updated market. Both issues describe the same impact.

I do not believe given the same attack vector, the same impact, and the same problem description, the two issues should be judged separately. The issue #16 mentions

Administrator mistakes usually fall under QA / Analysis reports, however, in this circumstance, the mistake is not based on input but rather on the state of the contract.

But the same is also described here when setting up the exploit scenario. The wording of this single one line should not change the outcome given everything else is the same. The scenario shown in this report in the POC section very clearly highlights a situation where the mistake is based on the state of the contract, this being the markets not being updated in that same block.

I believe this should be duped with #16 and not downgraded to Low. The current judgement would have been acceptable if the same vector is discussed but with different impacts, but that is clearly not the case here, since this report too addresses the incorrect state of contract vector.

[MarioPoneder]

Thank your for your comment!
Essentially I can agree with you. Although not worded as precisely as the primary issue (at least my impression), the present report still identifies the core issue and implies that the given impacts can arise without purposeful wrongdoing of the governance. Therefore, restoring duplication.

[c4-judge]

This previously downgraded issue has been upgraded by MarioPoneder

[c4-judge]

MarioPoneder marked the issue as duplicate of #16

[c4-judge]

MarioPoneder marked the issue as satisfactory