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

Open
code423n4 opened this issue Aug 13, 2022 · 0 comments
Open

Gas Optimizations #49

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

Comments

@code423n4
Copy link
Contributor

Table of Contents

Gas

Gas Findings

1. Use != 0 instead of > 0 for Unsigned Integer Comparison in require statements

Impact

!= 0 is cheapear than > 0 when comparing unsigned integers in require statements.

Findings:

VotingEscrow.sol::412 => require(_value > 0, "Only non zero amount");
VotingEscrow.sol::448 => require(_value > 0, "Only non zero amount");

Recommendation

Use != 0 instead of > 0.

2. Use Custom Errors instead of Revert Strings.

Impact

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

Findings:

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");
Blocklist.sol::24 => require(msg.sender == manager, "Only manager");
Blocklist.sol::25 => require(_isContract(addr), "Only contracts");

Recommendation

Use custom errors instead of revert strings.

3. No need to initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Findings:

VotingEscrow.sol::229 => int128 oldSlopeDelta = 0;
VotingEscrow.sol::230 => int128 newSlopeDelta = 0;
VotingEscrow.sol::298 => uint256 blockSlope = 0; // dblock/dt
VotingEscrow.sol::309 => for (uint256 i = 0; i < 255; i++) {
VotingEscrow.sol::313 => int128 dSlope = 0;
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::836 => int128 dSlope = 0;
VotingEscrow.sol::889 => uint256 dTime = 0;

Recommendation

Remove explicit default initializations.

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

Impact

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

Findings:

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

Recommendation

Use ++i instead of i++ to increment the value of an uint variable.

5. Use Shift Right/Left instead of Division/Multiplication if possible

Impact

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.

Findings:

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

Recommendation

Use SHR/SHL.
Bad

uint256 b = a / 2
uint256 c = a / 4;
uint256 d = a * 8;

Good

uint256 b = a >> 1;
uint256 c = a >> 2;
uint256 d = a << 3;

6. Contracts using unlocked pragma.

Impact

Contracts in scope use pragma solidity ^0.8.3 allowing a wide enough range of versions.

Findings:

VotingEscrow.sol::2 => pragma solidity ^0.8.3;
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;

Recommendation

Consider locking compiler version, for example pragma solidity 0.8.6. This can have additional benefits, for example using custom errors to save gas and so forth.

7. Use storage instead of memory for structs/arrays.

Impact

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct.

Findings:

VotingEscrow.sol::172 => LockedBalance memory locked_ = locked[_addr];
VotingEscrow.sol::214 => Point memory point = userPointHistory[_addr][uepoch];
VotingEscrow.sol::410 => LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol::446 => LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol::499 => LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol::527 => LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol::561 => LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol::633 => LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol::759 => Point memory lastPoint = userPointHistory[_owner][epoch];
VotingEscrow.sol::783 => Point memory upoint = userPointHistory[_owner][userEpoch];
VotingEscrow.sol::788 => Point memory point0 = pointHistory[epoch];
VotingEscrow.sol::796 => Point memory point1 = pointHistory[epoch + 1];
VotingEscrow.sol::866 => Point memory lastPoint = pointHistory[epoch_];
VotingEscrow.sol::882 => Point memory point = pointHistory[targetEpoch];
VotingEscrow.sol::891 => Point memory pointNext = pointHistory[targetEpoch + 1];

Recommendation

Use storage instead of memory for findings above

8. x += y costs more gas than x = x + y for state variables.

Same thing applies for subtraction

Findings:

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;

Recommendation

Use x = x + y instead of `x += y

Tools used

manual, c4udit, slither

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

1 participant