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

Open
code423n4 opened this issue Aug 13, 2022 · 1 comment
Open

Gas Optimizations #8

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

Comments

@code423n4
Copy link
Contributor

[G-01] Don't Initialize Variables with Default Value

Uninitialized variables are assigned with the types default value. Explicitly initializing a variable with it's default value costs unnecesary gas.

VotingEscrow.sol::298 => uint256 blockSlope = 0; // dblock/dt
VotingEscrow.sol::309 => for (uint256 i = 0; i < 255; i++) {
VotingEscrow.sol::714 => uint256 min = 0;
VotingEscrow.sol::717 => for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol::737 => uint256 min = 0;
VotingEscrow.sol::739 => for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol::793 => uint256 dBlock = 0;
VotingEscrow.sol::794 => uint256 dTime = 0;
VotingEscrow.sol::834 => for (uint256 i = 0; i < 255; i++) {
VotingEscrow.sol::889 => uint256 dTime = 0;

[G-02] Using > 0 costs more gas than != 0 when used on a uint in a require() statement

When dealing with unsigned integer types, comparisons with != 0 are cheaper then with > 0. This change saves 6 gas per instance

VotingEscrow.sol::412 => require(_value > 0, "Only non zero amount");
VotingEscrow.sol::448 => require(_value > 0, "Only non zero amount");
VotingEscrow.sol::449 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::469 => require(locked_.amount > 0, "Delegatee has no lock");
VotingEscrow.sol::502 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::529 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::564 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::587 => require(toLocked.amount > 0, "Delegatee has no lock");
VotingEscrow.sol::635 => require(locked_.amount > 0, "No lock");

[G-03] Use Shift Right/Left instead of Division/Multiplication if possible

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

VotingEscrow.sol::719 => uint256 mid = (min + max + 1) / 2;
VotingEscrow.sol::743 => uint256 mid = (min + max + 1) / 2;

[G-04] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

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.

VotingEscrow.sol::70 => int128 bias;
VotingEscrow.sol::71 => int128 slope;
VotingEscrow.sol::76 => int128 amount;
VotingEscrow.sol::78 => int128 delegated;
VotingEscrow.sol::174 => int128 value = locked_.amount;
VotingEscrow.sol::229 => int128 oldSlopeDelta = 0;
VotingEscrow.sol::230 => int128 newSlopeDelta = 0;
VotingEscrow.sol::533 => uint256 value = uint256(uint128(locked_.amount));
VotingEscrow.sol::567 => int128 value = locked_.amount;

[G-05] Using bools for storage incurs overhead

Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled.
Use uint256(1) and uint256(2) for true/false instead

Blocklist.sol::10 => mapping(address => bool) private _blocklist;

[G-06] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, for example when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

VotingEscrow.sol::309 => for (uint256 i = 0; i < 255; i++) {
VotingEscrow.sol::717 => for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol::739 => for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol::834 => for (uint256 i = 0; i < 255; i++) {

[G-07] += costs more gas than = + for state variables

use = + or = - instead to save gas

VotingEscrow.sol::418 => locked_.amount += int128(int256(_value));
VotingEscrow.sol::420 => locked_.delegated += int128(int256(_value));
VotingEscrow.sol::460 => newLocked.amount += int128(int256(_value));
VotingEscrow.sol::461 => newLocked.delegated += int128(int256(_value));
VotingEscrow.sol::465 => locked_.amount += int128(int256(_value));
VotingEscrow.sol::472 => newLocked.delegated += int128(int256(_value));
VotingEscrow.sol::537 => newLocked.delegated -= int128(int256(value));
VotingEscrow.sol::603 => newLocked.delegated += value;
VotingEscrow.sol::612 => newLocked.delegated -= value;
VotingEscrow.sol::642 => newLocked.delegated -= int128(int256(value));
VotingEscrow.sol::654 => penaltyAccumulated += penaltyAmount;

[G-08] Use custom errors rather than revert()/require() strings to save gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

Blocklist.sol::24 => require(msg.sender == manager, "Only manager");
Blocklist.sol::25 => require(_isContract(addr), "Only contracts");
VotingEscrow.sol::116 => require(decimals <= 18, "Exceeds max decimals");
VotingEscrow.sol::140 => require(msg.sender == owner, "Only owner");
VotingEscrow.sol::147 => require(msg.sender == owner, "Only owner");
VotingEscrow.sol::154 => require(msg.sender == owner, "Only owner");
VotingEscrow.sol::162 => require(msg.sender == owner, "Only owner");
VotingEscrow.sol::171 => require(msg.sender == blocklist, "Only Blocklist");
VotingEscrow.sol::412 => require(_value > 0, "Only non zero amount");
VotingEscrow.sol::413 => require(locked_.amount == 0, "Lock exists");
VotingEscrow.sol::414 => require(unlock_time >= locked_.end, "Only increase lock end"); // from using quitLock, user should increaseAmount instead
VotingEscrow.sol::415 => require(unlock_time > block.timestamp, "Only future lock end");
VotingEscrow.sol::416 => require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
VotingEscrow.sol::448 => require(_value > 0, "Only non zero amount");
VotingEscrow.sol::449 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::450 => require(locked_.end > block.timestamp, "Lock expired");
VotingEscrow.sol::469 => require(locked_.amount > 0, "Delegatee has no lock");
VotingEscrow.sol::470 => require(locked_.end > block.timestamp, "Delegatee lock expired");
VotingEscrow.sol::502 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::503 => require(unlock_time > locked_.end, "Only increase lock end");
VotingEscrow.sol::504 => require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
VotingEscrow.sol::511 => require(oldUnlockTime > block.timestamp, "Lock expired");
VotingEscrow.sol::529 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::530 => require(locked_.end <= block.timestamp, "Lock not expired");
VotingEscrow.sol::531 => require(locked_.delegatee == msg.sender, "Lock delegated");
VotingEscrow.sol::546 => require(token.transfer(msg.sender, value), "Transfer failed");
VotingEscrow.sol::563 => require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");
VotingEscrow.sol::564 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::565 => require(locked_.delegatee != _addr, "Already delegated");
VotingEscrow.sol::587 => require(toLocked.amount > 0, "Delegatee has no lock");
VotingEscrow.sol::588 => require(toLocked.end > block.timestamp, "Delegatee lock expired");
VotingEscrow.sol::589 => require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
VotingEscrow.sol::635 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::636 => require(locked_.end > block.timestamp, "Lock expired");
VotingEscrow.sol::637 => require(locked_.delegatee == msg.sender, "Lock delegated");
VotingEscrow.sol::657 => require(token.transfer(msg.sender, remainingAmount), "Transfer failed");
VotingEscrow.sol::676 => require(token.transfer(penaltyRecipient, amount), "Transfer failed");
VotingEscrow.sol::776 => require(_blockNumber <= block.number, "Only past block number");
VotingEscrow.sol::877 => require(_blockNumber <= block.number, "Only past block number");

[G-09] Do not calculate constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.
Consequences: each usage of a constant costs more gas on each access. Since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library)

VotingEscrow.sol::48 => uint256 public constant MULTIPLIER = 10**18;

[G-10] Use a more recent version of solidity

Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

Blocklist.sol::2 => pragma solidity ^0.8.3;
IBlocklist.sol::2 => pragma solidity ^0.8.3;
IERC20.sol::2 => pragma solidity ^0.8.3;
IVotingEscrow.sol::2 => pragma solidity ^0.8.3;
VotingEscrow.sol::2 => pragma solidity ^0.8.3;

[G-11] Prefix increments cheaper than Postfix increments

++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)
Saves 5 gas PER LOOP

VotingEscrow.sol::309 => for (uint256 i = 0; i < 255; i++) {
VotingEscrow.sol::717 => for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol::739 => for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol::834 => for (uint256 i = 0; i < 255; i++) {

[G-12] Public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public and can save gas by doing so.

Blocklist.sol::33 => function isBlocked(address addr) public view returns (bool) {
VotingEscrow.sol::754 => function balanceOf(address _owner) public view override returns (uint256) {
VotingEscrow.sol::864 => function totalSupply() public view override returns (uint256) {

[G-13] Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

VotingEscrow.sol::58 => mapping(address => Point[1000000000]) public userPointHistory;
VotingEscrow.sol::59 => mapping(address => uint256) public userPointEpoch;
VotingEscrow.sol::61 => mapping(address => LockedBalance) public locked;

[G-14] Use assembly to check for address(0)

Saves 6 gas per instance if using assembly to check for address(0)

e.g.

assembly {
 if iszero(_addr) {
  mstore(0x00, "zero address")
  revert(0x00, 0x20)
 }
}

instances:

VotingEscrow.sol::233 => if (_addr != address(0)) {
VotingEscrow.sol::352 => if (_addr != address(0)) {
VotingEscrow.sol::374 => if (_addr != address(0)) {

[G-15] internal functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

Blocklist.sol::37 => function _isContract(address addr) internal view returns (bool) {

[G-16] State variables only set in the constructor should be declared immutable

Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

Blocklist.sol::15 => manager = _manager;
Blocklist.sol::16 => ve = _ve;
VotingEscrow.sol::107 => token = IERC20(_token);
VotingEscrow.sol::115 => decimals = IERC20(_token).decimals();
VotingEscrow.sol::118 => name = _name;
VotingEscrow.sol::119 => symbol = _symbol;

[G-17] Unnecesary cast

no need to cast int256(MAXTIME)

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L239
https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L247

@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Aug 13, 2022
code423n4 added a commit that referenced this issue Aug 13, 2022
This was referenced Aug 14, 2022
@lacoop6tu
Copy link
Collaborator

Good one

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