Backd Tokenomics contest - Picodes's results

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/tokenomics/Minter.sol#L181

Vulnerability details

Impact

The actual total supply of the token is random and depends on when _executeInflationRateUpdate is executed.

Proof of concept

The README and tokenomic documentation clearly states that “The token supply is limited to a total of 268435456 tokens.”. However when executing _executeInflationRateUpdate, it first uses the current inflation rate to update the total available before checking if it needs to be reduced.

Therefore if no one mints or calls executeInflationRateUpdate for some time around the decay point, the inflation will be updated using the previous rate so the totalAvailableToNow will grow too much.

Mitigation steps

You should do

totalAvailableToNow += (currentTotalInflation * (block.timestamp - lastEvent));

Only if the condition block.timestamp >= lastInflationDecay + _INFLATION_DECAY_PERIOD is false.

Otherwise you should do

totalAvailableToNow += (currentTotalInflation * (lastInflationDecay + _INFLATION_DECAY_PERIOD - lastEvent));


Then update the rates, then complete with

totalAvailableToNow += (currentTotalInflation * (block.timestamp - lastInflationDecay + _INFLATION_DECAY_PERIOD));


Note that as all these variables are either constants either already loaded in memory this is super cheap to do.

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.

[Non-critical - 01] - In Minter, currentInflationAmountLp, currentInflationAmountKeeper, currentInflationAmountAmm could be made internal.

There are getters like getLpInflationRate for these variables, so there is no need to make them public as it just creates duplicates view functions.

[Gas - 01] - Gauges could mint directly to Minter

Currently all mint action pass through the InflationManager which checks the access control using the mapping gauges and then forward the call to the Minter: see this code. It would be way more gas efficient considering the frequency of mints versus gauges mapping updates to maintain identical mappings in InflationManager and Minter and to mint directly to Minter.

This would just imply using an internal function in InflationManager to make sure each time gauges is updated it is also updated in Minter.

[Gas - 02] - In Minter, token could be made immutable

In Minter, token is not immutable but can only be set once. I assume this is for simplicity as you want to deploy Minter, BkdToken and then set the address of BkdToken in Minter. But this overhead could easily be avoided by pre-computing the deployment address of BkdToken so it could be set in the constructor of Minter and be immutable. This would save a lot of gas during the whole contract lifecycle.

To precompute deployment addresses, you can use the CREATE2 opcode: check https://docs.openzeppelin.com/cli/2.8/deploying-with-create2 or https://medium.com/coinmonks/pre-compute-contract-deployment-address-using-create2-8c01e80ab7da.

This also applies to other places in the code like https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/tokenomics/InflationManager.sol#L56

[Gas - 03] - In Minter, currentInflationAmountLp, currentInflationAmountKeeper, currentInflationAmountAmm could be made internal.

There are getters like getLpInflationRate for these variables, so there is no need to make them public as it just creates duplicates view functions.

[Gas - 04] - Minter and BkdToken could be merged

To save expensive external calls, why not merging Minter and BkdToken into a single contract ? I think it doable from a contract size point of view, and references to one another are immutable so it would totally make sense to merge them.

[Gas - 05] In VestedEscrow, totalTime could be made immutable.

totalTime is not changed during the contract lifetime so could be made immutable !

[Gas - 06] In Minter, there is no need to inherit ReentrancyGuard

In Minter the keyword nonReentrant is used only once here, but it is useless as there is only an external call to a trusted contract: the token, and no crucial states updates after this call.