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 #139

Open
code423n4 opened this issue May 2, 2022 · 1 comment
Open

Gas Optimizations #139

code423n4 opened this issue May 2, 2022 · 1 comment
Labels
bug Something isn't working G (Gas Optimization)

Comments

@code423n4
Copy link
Contributor

Table of Contents:

Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it

To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.

The effect can be quite significant.

As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.

Instances include (check the @audit tags):

core/contracts/dex/DexAddressProvider.sol:
  53:     return (_dexMapping[index].proxy, _dexMapping[index].router);  //@audit gas: should declare a storage variable "Dex storage _dex = _dexMapping[index]"

Caching external values in memory

See the @audit tags for further details:

core/contracts/dex/DexAddressProvider.sol:
  22:     require(_a.controller().hasRole(_a.controller().MANAGER_ROLE(), msg.sender), "LM010"); //@audit gas: should cache _a.controller()

supervaults/contracts/SuperVault.sol:
  369:     if (ga.mimo().balanceOf(address(this)) > 0) {  //@audit gas: should cache ga.mimo()

Using an existing memory variable instead of reading storage

See the @audit tags for further details:

core/contracts/oracles/BalancerV2LPOracle.sol:
  41:     (address _pool, IBalancerVault.PoolSpecialization tokensNum) = vault.getPool(poolId); //@audit gas: should use memory variable _poolId: "vault.getPool(_poolId)"

BalancerV2LPOracle.sol: Tighly pack storage variables

I suggest going from (see @audit tags):

File: BalancerV2LPOracle.sol
14: contract BalancerV2LPOracle is AggregatorV3Interface, BNum {
15:   using SafeMath for uint256;
16: 
17:   string public override description; //@audit gas: 32 bytes
18:   uint256 public override version = 3; //@audit gas: 32 bytes
19:   uint8 public override decimals; //@audit gas: 1 byte, can be tightly packed by being moved further down or by moving an address closer
20: 
21:   bytes32 public poolId; //@audit gas: 32 bytes
22:   IBalancerVault public vault; //@audit gas: 20 bytes
23:   IBalancerPool public pool; //@audit gas: 20 bytes
24:   AggregatorV3Interface public oracleA; //@audit gas: 20 bytes
25:   AggregatorV3Interface public oracleB; //@audit gas: 20 bytes

to

contract BalancerV2LPOracle is AggregatorV3Interface, BNum {
  using SafeMath for uint256;

  string public override description; //@audit gas: 32 bytes (slot 1)
  uint256 public override version = 3; //@audit gas: 32 bytes (slot 2)
  uint8 public override decimals; //@audit gas: 1 byte (slot 3)

  IBalancerVault public vault; //@audit gas: 20 bytes (slot 3)
  IBalancerPool public pool; //@audit gas: 20 bytes  (slot 4)
  bytes32 public poolId; //@audit gas: 32 bytes <= this is the one we moved
  AggregatorV3Interface public oracleA; //@audit gas: 20 bytes
  AggregatorV3Interface public oracleB; //@audit gas: 20 bytes

Which would save 1 storage slot.

Variables that should be constant

According to slither:

BalancerV2LPOracle.version (contracts/oracles/BalancerV2LPOracle.sol#18) should be constant
GUniLPOracle.version (contracts/oracles/GUniLPOracle.sol#16) should be constant

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

core/contracts/inception/InceptionVaultsCore.sol:122:    require(_amount > 0, "IV100");
core/contracts/liquidityMining/v2/GenericMinerV2.sol:58:    require(boostConfig.a >= 1 && boostConfig.d > 0 && boostConfig.maxBoost >= 1, "LM004");
core/contracts/liquidityMining/v2/GenericMinerV2.sol:70:    require(newBoostConfig.a >= 1 && newBoostConfig.d > 0 && newBoostConfig.maxBoost >= 1, "LM004");
core/contracts/liquidityMining/v2/GenericMinerV2.sol:175:    require(value > 0, "LM101");
core/contracts/liquidityMining/v2/GenericMinerV2.sol:195:    require(value > 0, "LM101");
core/contracts/liquidityMining/v2/PARMinerV2.sol:52:    require(boostConfig.a >= 1 && boostConfig.d > 0 && boostConfig.maxBoost >= 1, "LM004");
core/contracts/liquidityMining/v2/PARMinerV2.sol:71:    require(newBoostConfig.a >= 1 && newBoostConfig.d > 0 && newBoostConfig.maxBoost >= 1, "LM004");
core/contracts/liquidityMining/v2/PARMinerV2.sol:254:    require(_value > 0, "LM101");
core/contracts/liquidityMining/v2/PARMinerV2.sol:284:    require(_value > 0, "LM101");
core/contracts/oracles/GUniLPOracle.sol:112:    require(rA > 0 || rB > 0, "C100");

Also, please enable the Optimizer.

<= is cheaper than <

Strict inequalities (<) are more expensive than non-strict ones (<=). This is due to some supplementary checks (ISZERO, 3 gas)

I suggest using <= instead of < here:

core/contracts/libraries/ABDKMath64x64.sol:697:      return uint128(r < r1 ? r : r1);

Splitting require() statements that use && saves gas

Instead of using the && operator in a single require statement to check multiple conditions, I suggest using multiple require statements with 1 condition per require statement (saving 3 gas per &):

core/contracts/libraries/ABDKMath64x64.sol:
   35:     require(x >= -0x8000000000000000 && x <= 0x7FFFFFFFFFFFFFFF);
   83:     require(result >= MIN_64x64 && result <= MAX_64x64);
  107:     require(result >= MIN_64x64 && result <= MAX_64x64);
  120:     require(result >= MIN_64x64 && result <= MAX_64x64);
  133:     require(result >= MIN_64x64 && result <= MAX_64x64);
  207:     require(result >= MIN_64x64 && result <= MAX_64x64);
  288:     require(result >= MIN_64x64 && result <= MAX_64x64);
  413:     require(result >= MIN_64x64 && result <= MAX_64x64);

core/contracts/liquidityMining/v2/GenericMinerV2.sol:
   58:     require(boostConfig.a >= 1 && boostConfig.d > 0 && boostConfig.maxBoost >= 1, "LM004");
   70:     require(newBoostConfig.a >= 1 && newBoostConfig.d > 0 && newBoostConfig.maxBoost >= 1, "LM004");
  331:     require(multiplier >= 1e18 && multiplier <= _boostConfig.maxBoost, "LM103");

core/contracts/liquidityMining/v2/PARMinerV2.sol:
   52:     require(boostConfig.a >= 1 && boostConfig.d > 0 && boostConfig.maxBoost >= 1, "LM004");
   71:     require(newBoostConfig.a >= 1 && newBoostConfig.d > 0 && newBoostConfig.maxBoost >= 1, "LM004");
  426:     require(multiplier >= 1e18 && multiplier <= _boostConfig.maxBoost, "LM103");

supervaults/contracts/SuperVault.sol:
  344:     require(proxy != address(0) && router != address(0), "SV201"); 

require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking

Properly functioning code should never reach a failing assert statement, unless there is a bug in your contract you should fix. Here, I believe the assert should be a require or a revert:

core/contracts/libraries/ABDKMath64x64.sol:641:      assert(xh == hi >> 128);

As the Solidity version is 0.6.12 < 0.8.0, the remaining gas would not be refunded in case of failure.

Amounts should be checked for 0 before calling a transfer

Checking non-zero transfer values can avoid an expensive external call and save gas.

While this is done at some places, it's not consistently done in the solution.

I suggest adding a non-zero-value check here:

core/contracts/inception/AdminInceptionVault.sol:81:    asset.safeTransferFrom(msg.sender, address(this), _depositAmount);
core/contracts/inception/AdminInceptionVault.sol:101:    asset.safeTransfer(msg.sender, _amount);
core/contracts/inception/AdminInceptionVault.sol:124:    stablex.safeTransfer(_to, _amount);
core/contracts/inception/AdminInceptionVault.sol:131:    _mimo.safeTransfer(_to, _amount);
core/contracts/inception/AdminInceptionVault.sol:139:    par.safeTransfer(_to, _amount);
core/contracts/inception/AdminInceptionVault.sol:145:    this function uses `transferFrom()` and requires pre-approval via `approve()` on the ERC20.
core/contracts/inception/AdminInceptionVault.sol:151:    asset.safeTransferFrom(msg.sender, address(this), _amount);
core/contracts/inception/InceptionVaultsCore.sol:67:    _inceptionCollateral.safeTransferFrom(msg.sender, address(this), _amount);
core/contracts/inception/InceptionVaultsCore.sol:93:    _inceptionCollateral.safeTransfer(msg.sender, _amount);
core/contracts/inception/InceptionVaultsCore.sol:186:    stablex.safeTransferFrom(msg.sender, address(_adminInceptionVault), _amount);
core/contracts/inception/InceptionVaultsCore.sol:234:    stablex.safeTransferFrom(msg.sender, address(this), _amount);
core/contracts/inception/InceptionVaultsCore.sol:235:    stablex.safeTransfer(address(_adminInceptionVault), _amount);
core/contracts/inception/InceptionVaultsCore.sol:239:    _inceptionCollateral.safeTransfer(msg.sender, collateralToReceive);
core/contracts/liquidityMining/v2/DemandMinerV2.sol:67:    _token.safeTransferFrom(msg.sender, address(this), amount);
core/contracts/liquidityMining/v2/DemandMinerV2.sol:72:      _token.safeTransfer(_feeCollector, fee);
core/contracts/liquidityMining/v2/DemandMinerV2.sol:87:      _token.safeTransfer(_feeCollector, fee);
core/contracts/liquidityMining/v2/DemandMinerV2.sol:90:    _token.safeTransfer(msg.sender, withdrawAmount);
core/contracts/liquidityMining/v2/PARMinerV2.sol:92:    _par.safeTransferFrom(msg.sender, address(this), amount);
core/contracts/liquidityMining/v2/PARMinerV2.sol:101:    _par.safeTransfer(msg.sender, amount);
core/contracts/liquidityMining/v2/PARMinerV2.sol:127:    _par.safeTransfer(msg.sender, _liquidateCallerReward);
supervaults/contracts/SuperVault.sol:129:    IERC20(asset).transferFrom(msg.sender, address(this), depositAmount);
supervaults/contracts/SuperVault.sol:247:    require(asset.transfer(msg.sender, amount));
supervaults/contracts/SuperVault.sol:274:    token.transferFrom(msg.sender, address(this), amount);
supervaults/contracts/SuperVault.sol:290:    token.transferFrom(msg.sender, address(this), depositAmount);

An array's length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:

core/contracts/dex/DexAddressProvider.sol:16:    for (uint256 i; i < dexes.length; i++) {

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

core/contracts/dex/DexAddressProvider.sol:16:    for (uint256 i; i < dexes.length; i++) {
core/contracts/inception/AdminInceptionVault.sol:108:    for (uint8 i = 1; i < _collateralCount + 1; i++) {
core/contracts/libraries/ABDKMath64x64.sol:396:            resultShift += 1;
core/contracts/libraries/ABDKMath64x64.sol:403:          absXShift += 1;
core/contracts/libraries/ABDKMath64x64.sol:463:    if (xc >= 0x2) msb += 1; // No need to shift xc anymore
core/contracts/libraries/ABDKMath64x64.sol:624:      if (xc >= 0x2) msb += 1; // No need to shift xc anymore

I suggest using ++i instead of i++ to increment the value of an uint variable.

Usage of a non-native 256 bits uint as a counter in for-loops increases gas cost

Due to how the EVM natively works on 256 bit numbers, using a 8 bit number in for-loops introduces additional costs as the EVM has to properly enforce the limits of this smaller type.

See the warning at this link: https://docs.soliditylang.org/en/v0.8.0/internals/layout_in_storage.html#layout-of-state-variables-in-storage :

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
It is only beneficial to use reduced-size arguments if you are dealing with storage values because the compiler will pack multiple elements into one storage slot, and thus, combine multiple reads or writes into a single operation. When dealing with function arguments or memory values, there is no inherent benefit because the compiler does not pack these values.

Affected code:

core/contracts/inception/AdminInceptionVault.sol:108:    for (uint8 i = 1; i < _collateralCount + 1; i++) {

Consider manually checking for the upper bound before the for-loop and using the uint256 type as a counter in the mentioned for-loops.

Public functions to external

The following functions could be set external to save gas and improve code quality.
External call cost is less expensive than of public functions.

clone(bytes) should be declared external:
 - SuperVaultFactory.clone(bytes) (contracts/SuperVaultFactory.sol#23-28)
parallel() should be declared external:
 - DexAddressProvider.parallel() (contracts/dex/DexAddressProvider.sol#43-45)
dexMapping(uint256) should be declared external:
 - DexAddressProvider.dexMapping(uint256) (contracts/dex/DexAddressProvider.sol#52-54)
deposit(address,uint256) should be declared external:
 - AdminInceptionVault.deposit(address,uint256) (contracts/inception/AdminInceptionVault.sol#149-154)
borrow(uint256,uint256) should be declared external:
 - AdminInceptionVault.borrow(uint256,uint256) (contracts/inception/AdminInceptionVault.sol#164-173)
a() should be declared external:
 - AdminInceptionVault.a() (contracts/inception/AdminInceptionVault.sol#175-177)
debtNotifier() should be declared external:
 - AdminInceptionVault.debtNotifier() (contracts/inception/AdminInceptionVault.sol#179-181)
weth() should be declared external:
 - AdminInceptionVault.weth() (contracts/inception/AdminInceptionVault.sol#183-185)
mimo() should be declared external:
 - AdminInceptionVault.mimo() (contracts/inception/AdminInceptionVault.sol#187-189)
inceptionCore() should be declared external:
 - AdminInceptionVault.inceptionCore() (contracts/inception/AdminInceptionVault.sol#191-193)
collateralCount() should be declared external:
 - AdminInceptionVault.collateralCount() (contracts/inception/AdminInceptionVault.sol#195-197)
collaterals(uint8) should be declared external:
 - AdminInceptionVault.collaterals(uint8) (contracts/inception/AdminInceptionVault.sol#199-201)
collateralId(address) should be declared external:
 - AdminInceptionVault.collateralId(address) (contracts/inception/AdminInceptionVault.sol#203-205)
a() should be declared external:
 - InceptionVaultFactory.a() (contracts/inception/InceptionVaultFactory.sol#138-140)
debtNotifier() should be declared external:
 - InceptionVaultFactory.debtNotifier() (contracts/inception/InceptionVaultFactory.sol#142-144)
weth() should be declared external:
 - InceptionVaultFactory.weth() (contracts/inception/InceptionVaultFactory.sol#146-148)
mimo() should be declared external:
 - InceptionVaultFactory.mimo() (contracts/inception/InceptionVaultFactory.sol#150-152)
adminInceptionVaultBase() should be declared external:
 - InceptionVaultFactory.adminInceptionVaultBase() (contracts/inception/InceptionVaultFactory.sol#154-156)
inceptionVaultsCoreBase() should be declared external:
 - InceptionVaultFactory.inceptionVaultsCoreBase() (contracts/inception/InceptionVaultFactory.sol#158-160)
inceptionVaultsDataProviderBase() should be declared external:
 - InceptionVaultFactory.inceptionVaultsDataProviderBase() (contracts/inception/InceptionVaultFactory.sol#162-164)
inceptionVaultCount() should be declared external:
 - InceptionVaultFactory.inceptionVaultCount() (contracts/inception/InceptionVaultFactory.sol#166-168)
priceFeedCount() should be declared external:
 - InceptionVaultFactory.priceFeedCount() (contracts/inception/InceptionVaultFactory.sol#170-172)
inceptionVaults(uint256) should be declared external:
 - InceptionVaultFactory.inceptionVaults(uint256) (contracts/inception/InceptionVaultFactory.sol#174-176)
priceFeeds(uint8) should be declared external:
 - InceptionVaultFactory.priceFeeds(uint8) (contracts/inception/InceptionVaultFactory.sol#178-180)
priceFeedIds(address) should be declared external:
 - InceptionVaultFactory.priceFeedIds(address) (contracts/inception/InceptionVaultFactory.sol#182-184)
cumulativeRate() should be declared external:
 - InceptionVaultsCore.cumulativeRate() (contracts/inception/InceptionVaultsCore.sol#243-245)
lastRefresh() should be declared external:
 - InceptionVaultsCore.lastRefresh() (contracts/inception/InceptionVaultsCore.sol#247-249)
vaultConfig() should be declared external:
 - InceptionVaultsCore.vaultConfig() (contracts/inception/InceptionVaultsCore.sol#251-253)
a() should be declared external:
 - InceptionVaultsCore.a() (contracts/inception/InceptionVaultsCore.sol#255-257)
inceptionCollateral() should be declared external:
 - InceptionVaultsCore.inceptionCollateral() (contracts/inception/InceptionVaultsCore.sol#259-261)
adminInceptionVault() should be declared external:
 - InceptionVaultsCore.adminInceptionVault() (contracts/inception/InceptionVaultsCore.sol#263-265)
inceptionVaultsData() should be declared external:
 - InceptionVaultsCore.inceptionVaultsData() (contracts/inception/InceptionVaultsCore.sol#267-269)
inceptionPriceFeed() should be declared external:
 - InceptionVaultsCore.inceptionPriceFeed() (contracts/inception/InceptionVaultsCore.sol#271-273)
a() should be declared external:
 - InceptionVaultsDataProvider.a() (contracts/inception/InceptionVaultsDataProvider.sol#161-163)
inceptionVaultsCore() should be declared external:
 - InceptionVaultsDataProvider.inceptionVaultsCore() (contracts/inception/InceptionVaultsDataProvider.sol#165-167)
inceptionVaultCount() should be declared external:
 - InceptionVaultsDataProvider.inceptionVaultCount() (contracts/inception/InceptionVaultsDataProvider.sol#169-171)
baseDebt() should be declared external:
 - InceptionVaultsDataProvider.baseDebt() (contracts/inception/InceptionVaultsDataProvider.sol#173-175)
a() should be declared external:
 - ChainlinkInceptionPriceFeed.a() (contracts/inception/priceFeed/ChainlinkInceptionPriceFeed.sol#87-89)
inceptionCollateral() should be declared external:
 - ChainlinkInceptionPriceFeed.inceptionCollateral() (contracts/inception/priceFeed/ChainlinkInceptionPriceFeed.sol#91-93)
assetOracle() should be declared external:
 - ChainlinkInceptionPriceFeed.assetOracle() (contracts/inception/priceFeed/ChainlinkInceptionPriceFeed.sol#95-97)
eurOracle() should be declared external:
 - ChainlinkInceptionPriceFeed.eurOracle() (contracts/inception/priceFeed/ChainlinkInceptionPriceFeed.sol#99-101)
deposit(uint256) should be declared external:
 - DemandMinerV2.deposit(uint256) (contracts/liquidityMining/v2/DemandMinerV2.sol#66-76)
withdraw(uint256) should be declared external:
 - DemandMinerV2.withdraw(uint256) (contracts/liquidityMining/v2/DemandMinerV2.sol#82-92)
token() should be declared external:
 - DemandMinerV2.token() (contracts/liquidityMining/v2/DemandMinerV2.sol#94-96)
feeCollector() should be declared external:
 - DemandMinerV2.feeCollector() (contracts/liquidityMining/v2/DemandMinerV2.sol#98-100)
feeConfig() should be declared external:
 - DemandMinerV2.feeConfig() (contracts/liquidityMining/v2/DemandMinerV2.sol#102-104)
releaseRewards(address) should be declared external:
 - GenericMinerV2.releaseRewards(address) (contracts/liquidityMining/v2/GenericMinerV2.sol#80-86)
 - PARMinerV2.releaseRewards(address) (contracts/liquidityMining/v2/PARMinerV2.sol#136-142)
updateBoost(address) should be declared external:
 - GenericMinerV2.updateBoost(address) (contracts/liquidityMining/v2/GenericMinerV2.sol#91-94)
stake(address) should be declared external:
 - GenericMinerV2.stake(address) (contracts/liquidityMining/v2/GenericMinerV2.sol#101-103)
 - PARMinerV2.stake(address) (contracts/liquidityMining/v2/PARMinerV2.sol#172-174)
stakeWithBoost(address) should be declared external:
 - GenericMinerV2.stakeWithBoost(address) (contracts/liquidityMining/v2/GenericMinerV2.sol#110-112)
 - PARMinerV2.stakeWithBoost(address) (contracts/liquidityMining/v2/PARMinerV2.sol#181-183)
pendingMIMO(address) should be declared external:
 - GenericMinerV2.pendingMIMO(address) (contracts/liquidityMining/v2/GenericMinerV2.sol#119-122)
 - PARMinerV2.pendingMIMO(address) (contracts/liquidityMining/v2/PARMinerV2.sol#190-193)
pendingPAR(address) should be declared external:
 - GenericMinerV2.pendingPAR(address) (contracts/liquidityMining/v2/GenericMinerV2.sol#129-132)
 - PARMinerV2.pendingPAR(address) (contracts/liquidityMining/v2/PARMinerV2.sol#200-207)
par() should be declared external:
 - GenericMinerV2.par() (contracts/liquidityMining/v2/GenericMinerV2.sol#134-136)
 - PARMinerV2.par() (contracts/liquidityMining/v2/PARMinerV2.sol#209-211)
a() should be declared external:
 - GenericMinerV2.a() (contracts/liquidityMining/v2/GenericMinerV2.sol#138-140)
 - PARMinerV2.a() (contracts/liquidityMining/v2/PARMinerV2.sol#213-215)
boostConfig() should be declared external:
 - GenericMinerV2.boostConfig() (contracts/liquidityMining/v2/GenericMinerV2.sol#142-144)
 - PARMinerV2.boostConfig() (contracts/liquidityMining/v2/PARMinerV2.sol#217-219)
totalStake() should be declared external:
 - GenericMinerV2.totalStake() (contracts/liquidityMining/v2/GenericMinerV2.sol#146-148)
 - PARMinerV2.totalStake() (contracts/liquidityMining/v2/PARMinerV2.sol#221-223)
totalStakeWithBoost() should be declared external:
 - GenericMinerV2.totalStakeWithBoost() (contracts/liquidityMining/v2/GenericMinerV2.sol#150-152)
 - PARMinerV2.totalStakeWithBoost() (contracts/liquidityMining/v2/PARMinerV2.sol#225-227)
userInfo(address) should be declared external:
 - GenericMinerV2.userInfo(address) (contracts/liquidityMining/v2/GenericMinerV2.sol#164-166)
 - PARMinerV2.userInfo(address) (contracts/liquidityMining/v2/PARMinerV2.sol#243-245)
deposit(uint256) should be declared external:
 - PARMinerV2.deposit(uint256) (contracts/liquidityMining/v2/PARMinerV2.sol#91-94)
withdraw(uint256) should be declared external:
 - PARMinerV2.withdraw(uint256) (contracts/liquidityMining/v2/PARMinerV2.sol#100-103)
liquidate(uint256,uint256,uint256,bytes) should be declared external:
 - PARMinerV2.liquidate(uint256,uint256,uint256,bytes) (contracts/liquidityMining/v2/PARMinerV2.sol#112-130)
restakePAR(address) should be declared external:
 - PARMinerV2.restakePAR(address) (contracts/liquidityMining/v2/PARMinerV2.sol#148-157)
updateBoost(address) should be declared external:
 - PARMinerV2.updateBoost(address) (contracts/liquidityMining/v2/PARMinerV2.sol#162-165)
liquidateCallerReward() should be declared external:
 - PARMinerV2.liquidateCallerReward() (contracts/liquidityMining/v2/PARMinerV2.sol#229-231)
baseDebtChanged(address,uint256) should be declared external:
 - SupplyMinerV2.baseDebtChanged(address,uint256) (contracts/liquidityMining/v2/SupplyMinerV2.sol#45-47)
collateral() should be declared external:
 - SupplyMinerV2.collateral() (contracts/liquidityMining/v2/SupplyMinerV2.sol#49-51)
syncStake(address) should be declared external:
 - VotingMinerV2.syncStake(address) (contracts/liquidityMining/v2/VotingMinerV2.sol#56-60)

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances include:

core/contracts/inception/InceptionVaultsCore.sol:218:    uint256 insuranceAmount = 0;
core/contracts/libraries/ABDKMath64x64.sol:153:      bool negativeResult = false;
core/contracts/libraries/ABDKMath64x64.sol:222:    bool negativeResult = false;
core/contracts/libraries/ABDKMath64x64.sol:387:      uint256 resultShift = 0;
core/contracts/libraries/ABDKMath64x64.sol:437:    int256 msb = 0;

I suggest removing explicit initializations for default values.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

supervaults/contracts/SuperVault.sol:39:    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "SV001");
supervaults/contracts/SuperVault.sol:56:    require(address(_a) != address(0));
supervaults/contracts/SuperVault.sol:57:    require(address(_ga) != address(0));
supervaults/contracts/SuperVault.sol:58:    require(address(_lendingPool) != address(0));
supervaults/contracts/SuperVault.sol:59:    require(address(dexAP) != address(0));
supervaults/contracts/SuperVault.sol:83:    require(msg.sender == address(lendingPool), "SV002");
supervaults/contracts/SuperVault.sol:109:    require(token.balanceOf(address(this)) >= flashloanRepayAmount, "SV101");
supervaults/contracts/SuperVault.sol:156:    require(fromCollateral.balanceOf(address(this)) >= flashloanRepayAmount, "SV101");
supervaults/contracts/SuperVault.sol:207:    require(vaultCollateral.balanceOf(address(this)) >= flashloanRepayAmount, "SV101");
supervaults/contracts/SuperVault.sol:233:    require(IERC20(a.stablex()).transfer(msg.sender, IERC20(a.stablex()).balanceOf(address(this))));
supervaults/contracts/SuperVault.sol:247:    require(asset.transfer(msg.sender, amount));
supervaults/contracts/SuperVault.sol:255:    require(IERC20(a.stablex()).transfer(msg.sender, IERC20(a.stablex()).balanceOf(address(this))));
supervaults/contracts/SuperVault.sol:264:    require(token.transfer(msg.sender, token.balanceOf(address(this))));
supervaults/contracts/SuperVault.sol:292:    require(IERC20(a.stablex()).transfer(msg.sender, IERC20(a.stablex()).balanceOf(address(this)))); //par
supervaults/contracts/SuperVault.sol:313:    require(IERC20(a.stablex()).transfer(msg.sender, IERC20(a.stablex()).balanceOf(address(this)))); //par
supervaults/contracts/SuperVault.sol:344:    require(proxy != address(0) && router != address(0), "SV201"); 
supervaults/contracts/SuperVault.sol:370:      require(ga.mimo().transfer(msg.sender, ga.mimo().balanceOf(address(this))));
supervaults/contracts/SuperVaultFactory.sol:18:    require(address(_base) != address(0));

I suggest replacing revert strings with custom errors.

@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels May 2, 2022
code423n4 added a commit that referenced this issue May 2, 2022
@m19
Copy link
Collaborator

m19 commented May 8, 2022

This was a very thorough gas optimization report and is very helpful for us.

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)
Projects
None yet
Development

No branches or pull requests

2 participants