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

Open
code423n4 opened this issue Jun 3, 2022 · 1 comment
Open

Gas Optimizations #70

code423n4 opened this issue Jun 3, 2022 · 1 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")

Comments

@code423n4
Copy link
Contributor

Table of Contents:

Cheap Contract Deployment Through Clones

See @audit tag:

tokenomics/VestedEscrow.sol:99:                holdingAddress = address(new EscrowTokenHolder(address(rewardToken)));
tokenomics/VestedEscrowRevocable.sol:44:        holdingContract[treasury_] = address(new EscrowTokenHolder(rewardToken_));

There's a way to save a significant amount of gas on deployment using Clones: https://www.youtube.com/watch?v=3Mw-pMmJ7TA .

This is a solution that was adopted, as an example, by Porter Finance. They realized that deploying using clones was 10x cheaper:

I suggest applying a similar pattern.

> 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:

protocol/contracts/BkdLocker.sol:
   91:         require(amount > 0, Error.INVALID_AMOUNT);
   92:         require(totalLockedBoosted > 0, Error.NOT_ENOUGH_FUNDS);
  137:         require(length > 0, "No entries");

protocol/contracts/tokenomics/AmmConvexGauge.sol:
  158:         require(amount > 0, Error.INVALID_AMOUNT);
  171:         require(amount > 0, Error.INVALID_AMOUNT);

protocol/contracts/tokenomics/AmmGauge.sol:
  104:         require(amount > 0, Error.INVALID_AMOUNT);
  125:         require(amount > 0, Error.INVALID_AMOUNT);

protocol/contracts/tokenomics/KeeperGauge.sol:
  140:         require(totalClaimable > 0, Error.ZERO_TRANSFER_NOT_ALLOWED);

protocol/contracts/tokenomics/VestedEscrow.sol:
  84:         require(unallocatedSupply > 0, "No reward tokens in contract");

Also, please enable the Optimizer.

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

Reading array length at each iteration of the loop consumes more gas than necessary.

In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration.
In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.

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

access/RoleManager.sol:82:        for (uint256 i; i < roles.length; i = i.uncheckedInc()) {
tokenomics/FeeBurner.sol:56:        for (uint256 i; i < tokens_.length; i = i.uncheckedInc()) {
tokenomics/InflationManager.sol:116:        for (uint256 i; i < stakerVaults.length; i = i.uncheckedInc()) {
tokenomics/VestedEscrow.sol:94:        for (uint256 i; i < amounts.length; i = i.uncheckedInc()) {
zaps/PoolMigrationZap.sol:22:        for (uint256 i; i < newPools_.length; ++i) {
zaps/PoolMigrationZap.sol:39:        for (uint256 i; i < oldPoolAddresses_.length; ) {
RewardHandler.sol:42:        for (uint256 i; i < pools.length; i = i.uncheckedInc()) {
StakerVault.sol:259:        for (uint256 i; i < actions.length; i = i.uncheckedInc()) {

++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

  • i += 1 is the most expensive form
  • i++ costs 6 gas less than i += 1
  • ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

  • i -= 1 is the most expensive form
  • i-- costs 11 gas less than i -= 1
  • --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

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

Affected code:

tokenomics/KeeperGauge.sol:59:        epoch++;
tokenomics/KeeperGauge.sol:98:        epoch++;

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

Increments can be unchecked in for-loops

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Affected code:

zaps/PoolMigrationZap.sol:22:        for (uint256 i; i < newPools_.length; ++i) {

The change would be:

- for (uint256 i; i < numIterations; ++i) {
+ for (uint256 i; i < numIterations;) {
 // ...  
+   unchecked { ++i; }
}  

The risk of overflow is non-existant for uint256 here.

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) {

Affected code:

tokenomics/InflationManager.sol:412:        bool keeperGaugeExists = false;

I suggest removing explicit initializations for default values.

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

tokenomics/Minter.sol:152:            "Maximum non-inflation amount exceeded." //@audit size 38

I suggest shortening the revert strings to fit in 32 bytes.

Use Custom Errors instead of Revert Strings to save Gas

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

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

I suggest replacing all revert strings with custom errors in the solution.

access/AuthorizationBase.sol:17:        require(_roleManager().hasRole(role, msg.sender), Error.UNAUTHORIZED_ACCESS);
access/AuthorizationBase.sol:25:        require(_roleManager().hasRole(Roles.GOVERNANCE, msg.sender), Error.UNAUTHORIZED_ACCESS);
access/AuthorizationBase.sol:33:        require(_roleManager().hasAnyRole(role1, role2, msg.sender), Error.UNAUTHORIZED_ACCESS);
access/AuthorizationBase.sol:45:        require(
access/RoleManager.sol:28:        require(hasRole(Roles.GOVERNANCE, msg.sender), Error.UNAUTHORIZED_ACCESS);
access/RoleManager.sol:46:        require(getRoleMemberCount(Roles.GOVERNANCE) > 1, Error.CANNOT_REVOKE_ROLE);
access/RoleManager.sol:112:        require(role != Roles.GOVERNANCE, Error.CANNOT_REVOKE_ROLE);
access/RoleManager.sol:113:        require(hasRole(role, account), Error.INVALID_ARGUMENT);
tokenomics/AmmConvexGauge.sol:65:        require(
tokenomics/AmmConvexGauge.sol:87:        require(inflationRecipient == address(0), Error.ADDRESS_ALREADY_SET);
tokenomics/AmmConvexGauge.sol:93:        require(inflationRecipient != address(0), Error.ADDRESS_NOT_FOUND);
tokenomics/AmmConvexGauge.sol:158:        require(amount > 0, Error.INVALID_AMOUNT);
tokenomics/AmmConvexGauge.sol:171:        require(amount > 0, Error.INVALID_AMOUNT);
tokenomics/AmmConvexGauge.sol:172:        require(balances[msg.sender] >= amount, Error.INSUFFICIENT_BALANCE);
tokenomics/AmmGauge.sol:50:        require(msg.sender == address(controller.inflationManager()), Error.UNAUTHORIZED_ACCESS);
tokenomics/AmmGauge.sol:57:        require(
tokenomics/AmmGauge.sol:104:        require(amount > 0, Error.INVALID_AMOUNT);
tokenomics/AmmGauge.sol:125:        require(amount > 0, Error.INVALID_AMOUNT);
tokenomics/AmmGauge.sol:126:        require(balances[msg.sender] >= amount, Error.INSUFFICIENT_BALANCE);
tokenomics/BkdToken.sol:31:        require(msg.sender == minter, Error.UNAUTHORIZED_ACCESS);
tokenomics/FeeBurner.sol:49:        require(tokens_.length != 0, "No tokens to burn");
tokenomics/FeeBurner.sol:75:        require(burningEth_ || msg.value == 0, Error.INVALID_VALUE);
tokenomics/InflationManager.sol:48:        require(gauges[msg.sender], Error.UNAUTHORIZED_ACCESS);
tokenomics/InflationManager.sol:59:        require(minter == address(0), Error.ADDRESS_ALREADY_SET);
tokenomics/InflationManager.sol:60:        require(_minter != address(0), Error.INVALID_MINTER);
tokenomics/InflationManager.sol:95:        require(!weightBasedKeeperDistributionDeactivated, "Weight-based dist. deactivated.");
tokenomics/InflationManager.sol:139:        require(_keeperGauges.contains(pool), Error.INVALID_ARGUMENT);
tokenomics/InflationManager.sol:171:        require(length == weights.length, Error.INVALID_ARGUMENT);
tokenomics/InflationManager.sol:174:            require(_keeperGauges.contains(pools[i]), Error.INVALID_ARGUMENT);
tokenomics/InflationManager.sol:229:        require(gauges[IStakerVault(stakerVault).getLpGauge()], Error.GAUGE_DOES_NOT_EXIST);
tokenomics/InflationManager.sol:244:        require(IStakerVault(stakerVault).getLpGauge() != address(0), Error.ADDRESS_NOT_FOUND);
tokenomics/InflationManager.sol:265:        require(length == weights.length, "Invalid length of arguments");
tokenomics/InflationManager.sol:270:            require(IStakerVault(stakerVault).getLpGauge() != address(0), Error.ADDRESS_NOT_FOUND);
tokenomics/InflationManager.sol:295:            require(IStakerVault(stakerVault).getLpGauge() != address(0), Error.ADDRESS_NOT_FOUND);
tokenomics/InflationManager.sol:315:        require(_ammGauges.contains(token), "amm gauge not found");
tokenomics/InflationManager.sol:365:        require(length == weights.length, "Invalid length of arguments");
tokenomics/InflationManager.sol:367:            require(_ammGauges.contains(tokens[i]), "amm gauge not found");
tokenomics/InflationManager.sol:424:        require(!exists || keeperGauge != _keeperGauge, Error.INVALID_ARGUMENT);
tokenomics/InflationManager.sol:452:        require(IAmmGauge(_ammGauge).isAmmToken(token), Error.ADDRESS_NOT_WHITELISTED);
tokenomics/InflationManager.sol:484:        require(addressProvider.isStakerVault(msg.sender, lpToken), Error.UNAUTHORIZED_ACCESS);
tokenomics/InflationManager.sol:486:        require(lpGauge != address(0), Error.GAUGE_DOES_NOT_EXIST);
tokenomics/InflationManager.sol:621:        require(
tokenomics/KeeperGauge.sol:40:        require(msg.sender == address(controller.inflationManager()), Error.UNAUTHORIZED_ACCESS);
tokenomics/KeeperGauge.sol:78:        require(
tokenomics/KeeperGauge.sol:82:        require(!killed, Error.CONTRACT_PAUSED);
tokenomics/KeeperGauge.sol:126:        require(
tokenomics/KeeperGauge.sol:140:        require(totalClaimable > 0, Error.ZERO_TRANSFER_NOT_ALLOWED);
tokenomics/LpGauge.sol:31:        require(_stakerVault != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
tokenomics/LpGauge.sol:35:        require(address(_inflationManager) != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
tokenomics/LpGauge.sol:53:        require(
tokenomics/Minter.sol:72:        require(_annualInflationDecayLp < ScaledMath.ONE, Error.INVALID_PARAMETER_VALUE);
tokenomics/Minter.sol:73:        require(_annualInflationDecayKeeper < ScaledMath.ONE, Error.INVALID_PARAMETER_VALUE);
tokenomics/Minter.sol:74:        require(_annualInflationDecayAmm < ScaledMath.ONE, Error.INVALID_PARAMETER_VALUE);
tokenomics/Minter.sol:100:        require(address(token) == address(0), "Token already set!");
tokenomics/Minter.sol:105:        require(lastEvent == 0, "Inflation has already started.");
tokenomics/Minter.sol:132:        require(msg.sender == address(controller.inflationManager()), Error.UNAUTHORIZED_ACCESS);
tokenomics/Minter.sol:150:        require(
tokenomics/Minter.sol:220:        require(newTotalMintedToNow <= totalAvailableToNow, "Mintable amount exceeded");
tokenomics/VestedEscrow.sol:57:        require(starttime_ >= block.timestamp, "start must be future");
tokenomics/VestedEscrow.sol:58:        require(endtime_ > starttime_, "end must be greater");
tokenomics/VestedEscrow.sol:69:        require(_admin != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
tokenomics/VestedEscrow.sol:70:        require(msg.sender == admin, Error.UNAUTHORIZED_ACCESS);
tokenomics/VestedEscrow.sol:75:        require(_fundadmin != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
tokenomics/VestedEscrow.sol:76:        require(msg.sender == admin, Error.UNAUTHORIZED_ACCESS);
tokenomics/VestedEscrow.sol:81:        require(msg.sender == admin, Error.UNAUTHORIZED_ACCESS);
tokenomics/VestedEscrow.sol:82:        require(!initializedSupply, "Supply already initialized once");
tokenomics/VestedEscrow.sol:84:        require(unallocatedSupply > 0, "No reward tokens in contract");
tokenomics/VestedEscrow.sol:90:        require(msg.sender == fundAdmin || msg.sender == admin, Error.UNAUTHORIZED_ACCESS);
tokenomics/VestedEscrow.sol:91:        require(initializedSupply, "Supply must be initialized");
tokenomics/VestedEscrowRevocable.sol:52:        require(msg.sender == admin, Error.UNAUTHORIZED_ACCESS);
tokenomics/VestedEscrowRevocable.sol:53:        require(revokedTime[_recipient] == 0, "Recipient already revoked");
tokenomics/VestedEscrowRevocable.sol:54:        require(_recipient != treasury, "Treasury cannot be revoked!");
utils/Preparable.sol:28:        require(deadlines[key] == 0, Error.DEADLINE_NOT_ZERO);
utils/Preparable.sol:29:        require(delay >= _MIN_DELAY, Error.DELAY_TOO_SHORT);
utils/Preparable.sol:86:        require(deadlines[key] != 0, Error.NOTHING_PENDING);
utils/Preparable.sol:98:        require(deadlines[key] != 0, Error.NOTHING_PENDING);
utils/Preparable.sol:110:        require(block.timestamp >= deadline, Error.DEADLINE_NOT_REACHED);
utils/Preparable.sol:111:        require(deadline != 0, Error.DEADLINE_NOT_SET);
zaps/PoolMigrationZap.sol:56:        require(lpTokenAmount_ != 0, "No LP Tokens");
zaps/PoolMigrationZap.sol:57:        require(oldPool_.getWithdrawalFee(msg.sender, lpTokenAmount_) == 0, "withdrawal fee not 0");
AddressProvider.sol:64:        require(!_whiteListedFeeHandlers.contains(feeHandler), Error.ADDRESS_WHITELISTED);
AddressProvider.sol:71:        require(_whiteListedFeeHandlers.contains(feeHandler), Error.ADDRESS_NOT_WHITELISTED);
AddressProvider.sol:98:        require(pool != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
AddressProvider.sol:102:        require(poolToken != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
AddressProvider.sol:176:        require(_addressKeyMetas.contains(key), Error.ADDRESS_DOES_NOT_EXIST);
AddressProvider.sol:185:        require(!checkExists || _addressKeyMetas.contains(key), Error.ADDRESS_DOES_NOT_EXIST);
AddressProvider.sol:199:        require(exists, Error.ADDRESS_DOES_NOT_EXIST);
AddressProvider.sol:241:        require(!meta.frozen, Error.ADDRESS_FROZEN);
AddressProvider.sol:242:        require(meta.freezable, Error.INVALID_ARGUMENT);
AddressProvider.sol:260:        require(!meta.frozen, Error.ADDRESS_FROZEN);
AddressProvider.sol:270:        require(!meta.frozen, Error.ADDRESS_FROZEN);
AddressProvider.sol:295:        require(token != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
AddressProvider.sol:296:        require(!_stakerVaults.contains(token), Error.STAKER_VAULT_EXISTS);
AddressProvider.sol:325:        require(exists, Error.ADDRESS_NOT_FOUND);
AddressProvider.sol:428:        require(!_addressKeyMetas.contains(key), Error.INVALID_ARGUMENT);
AddressProvider.sol:434:        require(_addressKeyMetas.set(key, meta.toUInt()), Error.INVALID_ARGUMENT);
BkdLocker.sol:59:        require(currentUInts256[_START_BOOST] == 0, Error.CONTRACT_INITIALIZED);
BkdLocker.sol:91:        require(amount > 0, Error.INVALID_AMOUNT);
BkdLocker.sol:92:        require(totalLockedBoosted > 0, Error.NOT_ENOUGH_FUNDS);
BkdLocker.sol:119:        require(
BkdLocker.sol:137:        require(length > 0, "No entries");
BkdLocker.sol:208:        require(
Controller.sol:34:        require(address(inflationManager) == address(0), Error.ADDRESS_ALREADY_SET);
Controller.sol:35:        require(_inflationManager != address(0), Error.INVALID_ARGUMENT);
Controller.sol:82:        require(addressProvider.getBKDLocker() != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
CvxCrvRewardsLocker.sol:87:        require(
CvxCrvRewardsLocker.sol:152:        require(_treasury != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
CvxCrvRewardsLocker.sol:195:        require(delegate != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
GasBank.sol:29:        require(account != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
GasBank.sol:46:        require(currentBalance != 0, Error.INSUFFICIENT_BALANCE);
GasBank.sol:47:        require(
GasBank.sol:73:        require(currentBalance >= amount, Error.NOT_ENOUGH_FUNDS);
GasBank.sol:74:        require(
GasBank.sol:81:            require(currentBalance - amount >= ethRequired, Error.NOT_ENOUGH_FUNDS);
GasBank.sol:96:        require(success, Error.FAILED_TRANSFER);
LpToken.sol:22:        require(msg.sender == minter, Error.UNAUTHORIZED_ACCESS);
LpToken.sol:34:        require(_minter != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
StakerVault.sol:66:        require(address(inflationManager_) != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
StakerVault.sol:76:        require(currentAddresses[_LP_GAUGE] == address(0), Error.ROLE_EXISTS);
StakerVault.sol:99:        require(msg.sender == address(inflationManager), Error.UNAUTHORIZED_ACCESS);
StakerVault.sol:112:        require(msg.sender != account, Error.SELF_TRANSFER_NOT_ALLOWED);
StakerVault.sol:113:        require(balances[msg.sender] >= amount, Error.INSUFFICIENT_BALANCE);
StakerVault.sol:145:        require(src != dst, Error.SAME_ADDRESS_NOT_ALLOWED);
StakerVault.sol:154:        require(startingAllowance >= amount, Error.INSUFFICIENT_ALLOWANCE);
StakerVault.sol:157:        require(srcTokens >= amount, Error.INSUFFICIENT_BALANCE);
StakerVault.sol:202:        require(addressProvider.isAction(msg.sender), Error.UNAUTHORIZED_ACCESS);
StakerVault.sol:223:        require(addressProvider.isAction(msg.sender), Error.UNAUTHORIZED_ACCESS);
StakerVault.sol:323:        require(IERC20(token).balanceOf(msg.sender) >= amount, Error.INSUFFICIENT_BALANCE);
StakerVault.sol:339:        require(staked == amount, Error.INVALID_AMOUNT);
StakerVault.sol:366:        require(
StakerVault.sol:370:        require(balances[src] >= amount, Error.INSUFFICIENT_BALANCE);
@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Jun 3, 2022
code423n4 added a commit that referenced this issue Jun 3, 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

Cheap Contract Deployment Through Clones

Agree but in lack of gas comparison I can't quantify

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

Valid because of Solidity < 0.8.13 on require with optimizer
3 * 9 = 27

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

Valid as it saves reading the offset
3 * 8 = 24

++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

10 gas saved

Increments can be unchecked in for-loops

Agree, would save 20 gas

No need to explicitly initialize variables with default values

Saves 3 gas

Reduce the size of error messages (Long revert Strings)

6 gas

Use Custom Errors instead of Revert Strings to save Gas

In lack of POC I can't quantify the savings

Pretty well-written report, missing just a couple of extra tests to make it shine

Total Gas Saved
90

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