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

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

Gas Optimizations #107

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

Title: Caching .length for loop can save gas

Proof of Concept:
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/access/RoleManager.sol#L82
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/zaps/PoolMigrationZap.sol#L39

Recommended Mitigation Steps:
Change to:

	uint256 Length = roles.length;
	for (uint256 i; i < Length; i = i.uncheckedInc()) {

========================================================================

Title: Using != is more gas efficient

Proof of Concept:
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/BkdLocker.sol#L91
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/BkdLocker.sol#L254

Recommended Mitigation Steps:
Change to !=

	require(amount != 0, Error.INVALID_AMOUNT);

========================================================================

Title: Using delete statement to empty curRewardTokenData.userShares can save gas

Proof of Concept:
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/BkdLocker.sol#L215
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/utils/Preparable.sol#L87-L88

Recommended Mitigation Steps:

	delete curRewardTokenData.userShares[msg.sender];

========================================================================

Title: Gas improvement on returning totalEthRequired value

Proof of Concept:
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/Controller.sol#L121-L130

Recommended Mitigation Steps:
by set totalEthRequired in returns and delete L#123 can save gas

function getTotalEthRequiredForGas(address payer) external view override returns (uint256 totalEthRequired) { //@audit-info: set here
        // solhint-disable-previous-line ordering
	//@audit-info: remove this line
        address[] memory actions = addressProvider.allActions();
        uint256 numActions = actions.length;
        for (uint256 i = 0; i < numActions; i++) {
            totalEthRequired += IAction(actions[i]).getEthRequiredForGas(payer);
        }
	return totalEthRequired;
    }

========================================================================

Title: Using unchecked to calculate

Proof of Concept:
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/StakerVault.sol#L124

Recommended Mitigation Steps:
balances[msg.sender] value was checked that it >= than amount so using unchecked can save gas:

unchecked{
	balances[msg.sender] -= amount;
}

========================================================================

Title: Unnecessary MSTORE timeElapsed

Proof of Concept:
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/AmmGauge.sol#L144-L149

Recommended Mitigation Steps:
By passing block.timestamp - uint256(ammLastUpdated) directly to L#148 without storing it to timeElapsed can save gas without damaging readability of the code

	ammStakedIntegral += (currentRate * (block.timestamp - uint256(ammLastUpdated)).scaledDiv(totalStaked);

========================================================================

Title: Unnecessary MSTORE

Proof of Concept:
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/BkdLocker.sol#L138-L144

Recommended Mitigation Steps:
instead of caching length to i. use it directly can save gas
delete L#138 and replace i with `length

========================================================================

Title: Use allowance_ directly to substract

Proof of Concept:
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/StakerVault.sol#L387

Recommended Mitigation Steps:

	_allowances[src][msg.sender] = allowance_ - unstaked;

========================================================================

Title: Unnecessary bool var set

Proof of Concept:
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/InflationManager.sol#L412

Recommended Mitigation Steps:
the default value of bool is false

========================================================================

Title: Using > instead >= can save gas

Proof of Concept:
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/VestedEscrow.sol#L57
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/utils/Preparable.sol#L110

Recommended Mitigation Steps:
1 second difference can be ignored to validate. using > operator can save gas

	require(starttime_ > block.timestamp, "start must be future");

========================================================================

Title: Use custom errors rather than revert()/require() strings to save deployment gas

Proof of Concept:
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/VestedEscrowRevocable.sol#L53-L54

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

========================================================================

Title: Using += to increase value

Proof of Concept:
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/VestedEscrow.sol#L145

Recommended Mitigation Steps:
Change to:

	totalClaimed[msg.sender] += claimable;

========================================================================

14
Title: Unnecessary MSTORE

Proof of Concept:
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/VestedEscrow.sol#L155-L156

Recommended Mitigation Steps:
instead of caching to elapsed. calculate directly can save gas
delete L#155

	return Math.min((locked * (_time - startTime)) / totalTime, locked);

========================================================================

@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

GalloDaSballo commented Jun 17, 2022

Title: Caching .length for loop can save gas

3 per instance
6

Title: Using != is more gas efficient

Saves 3 gas, only for require, solidity < 0.8.13, when using optimizer
3

Title: Using delete statement to empty curRewardTokenData.userShares can save gas

Disagree, delete is the same as setting to default value

## Title: Gas improvement on returning totalEthRequired value
I don't believe it will save gas, it's just syntactic sugar, in lack of math I can only give it 0 gas saved

Title: Using unchecked to calculate

Agree, this will save 20 gas
20

Title: Unnecessary MSTORE timeElapsed

Saves 6 gas but does make the code more complex to scan

Title: Unnecessary MSTORE

I guess, saves 6 gas, notice that the sponsor forgot to use the cached length inside the loop, that would be a more rational refactoring

## Title: Use allowance_ directly to substract
Avoiding the =- saves a SLOAD which saves 100 gas

Title: Unnecessary bool var set

Saves 3 gas

Title: Using > instead >= can save gas

I assume this will save the extra eq, hence saves 3 gas

Use custom errors rather than revert()/require() strings to save deployment gas

In lack of POC i can't give it points

Title: Using += to increase value

Doesn't save gas

Title: Unnecessary MSTORE

Saves 6 gas and makes the code pretty ugly

Total Gas Saved
153

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