Amount distributed can be inaccurate when updating weights

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L220
https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/tokenomics/InflationManager.sol#L559
https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/tokenomics/InflationManager.sol#L572
https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/tokenomics/InflationManager.sol#L586

Vulnerability details

Impact

When updating pool inflation rates, other pools see their currentRate being modified without having poolCheckpoint called, which leads to false computations.

This will lead to either users losing a part of their claims, but can also lead to too many tokens could be distributed, preventing some users from claiming due to the totalAvailableToNow requirement in Minter.

Proof of concept

Imagine you have 2 AMM pools A and B, both with an ammPoolWeight of 100, where poolCheckpoint has not been called for a moment. Then, imagine calling executeAmmTokenWeight to reduce the weight of A to 0.

Only A is checkpointed here, so when B will be checkpointed it will call getAmmRateForToken, which will see a pool weight of 100 and a total weight of 100 over the whole period since the last checkpoint of B, which is false, therefore it will distribute too many tokens. This is critical has the minter expects an exact or lower than expected distribution due to the requirement of totalAvailableToNow.

In the opposite direction, when increasing weights, it will lead to less tokens being distributed in some pools than planned, leading to a loss for users.

Mitigation steps

Checkpoint every LpStakerVault, KeeperGauge or AmmGauge when updating the weights of one of them.

[chase-manning]

We think this should be Medium as impact is quite minor

[GalloDaSballo]

Because getAmmRateForToken uses the totalAmmTokenWeight, then all Gauges will need to be poolCheckpointed at the time the weight is changed-

This is because for systems that accrue points over time, any time the multiplier changes, the growth of points has changed, and as such a new accrual needs to be performed.

I believe there's 3 ways to judge this finding:

• It's Medium as the Admin is using their privilege to change the points, potentially to their favour or to someones detriment
• It's High because the math is wrong and the code fails (by a way of lack of approximation) to properly allocate the resources
• It's Medium because while the finding is correct in a vacuum, anyone can call poolCheckpoint, as such if a true concern for fairness is made, anyone can front-run and back-run the update of weights with the goal of offering the most accurate math possible

Given those considerations, I want to emphasize that not calling poolCheckpoint can cause potentially drastic leaks of value, in a way similar to the original Sushi MasterChef's lack of massUpdatePools could.

That said, I believe the finding is of medium severity as any individual could setup a bot to monitor and prevent this and it would require the weights to be changed by governance and impact can be quite minor as long as the pools are used