Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gas Optimizations #48

Open
code423n4 opened this issue Jun 2, 2022 · 2 comments
Open

Gas Optimizations #48

code423n4 opened this issue Jun 2, 2022 · 2 comments
Labels
bug Something isn't working G (Gas Optimization) resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

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

@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Jun 2, 2022
code423n4 added a commit that referenced this issue Jun 2, 2022
@chase-manning chase-manning added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) labels Jun 6, 2022
@GalloDaSballo
Copy link
Collaborator

[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

@GalloDaSballo
Copy link
Collaborator

Total Gas Saved:
4200

To be honest had the warden written some POC for the other refactorings this may have been the shortest yet most effective report of the contest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working G (Gas Optimization) resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

3 participants