Gas Optimizations - Githubissues

code423n4 commented 2 years ago

[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.

GalloDaSballo commented 2 years ago

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

Great idea, however in lack of POC and before / after I can't do anything but give it 0 gas saved

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

Agree with caching token in Minter, will save at least one cold SLOAD = 2100 Disagree with second comment

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

Will save deployment cost while removing certain functions / interface

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

I think the ideas are really good, but you're gonna have to code a POC for these to be considered

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

Similar to 3. Saves 2.1k

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

I'm going to disagree in lack of detailed POC, especially given the un-CEIness of https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L217