code-423n4 / 2022-08-fiatdao-findings

2 stars 1 forks source link

Gas Optimizations #220

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Gas Optimizations

Summary

Issue Instances
1 Multiple address mappings can be combined into a single mapping of an address to a struct 3
2 State variables only set in the constructor should be declared immutable 5
3 Variables inside struct should be packed to save gas 1
4 Avoid contract existence checks by using solidity version 0.8.10 or later 2
5 Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead 4
6 Duplicated require() checks should be refactored to a modifier for saving deployment costs 4
7 It costs more gas to initialize variables to zero than to let the default of zero be applied 14
8 Use of ++i cost less gas than i++ in for loops 4
9 ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops 4
10 Using > 0 costs more gas than != 0 when used on a uint in a require() statement 1
11 X += Y costs more gas than X = X + Y for state variables 1
12 Using private rather than public for constants, saves gas 3
13 Public functions not called by the contract should be declared external instead 1
14 Functions guaranteed to revert when called by normal users can be marked payable 4

Findings

1- Multiple address mappings can be combined into a single mapping of an address to a struct :

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.

There are 3 instances of this issue:

File: contracts/VotingEscrow.sol

58      mapping(address => Point[1000000000]) public userPointHistory;
59      mapping(address => uint256) public userPointEpoch;
60      mapping(address => LockedBalance) public locked;

These mappings could be refactored into the following struct and mapping for example :

struct UserInfo {
        Point[1000000000] userPointHistory;
        uin256 userPointEpoch;
        LockedBalance locked;
    }
mapping(address => UserInfo) public usersInfo;

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

There are 5 instances of this issue:

File: contracts/VotingEscrow.sol

64      string public name;
65      string public symbol;
66      uint256 public decimals = 18;

File: contracts/features/Blocklist.sol

11      address public manager;
12      address public ve;

3- Variables inside struct should be packed to save gas :

As the solidity EVM works with 32 bytes, variables less than 32 bytes should be packed inside a struct so that they can be stored in the same slot, this saves gas when saving to storage

There is 1 instance of this issue:

File: contracts/VotingEscrow.sol

75      struct LockedBalance {
76          int128 amount;
77          uint256 end;
78          int128 delegated;
79          address delegatee;
80      }

It should be rearranged as follow :

  struct LockedBalance {
      uint256 end;
      int128 amount;
      int128 delegated;
      address delegatee;
  }

This saves approx 20000 gas for creating a Lock and 10000 gas in deployment cost as the Gas test performed using the "votingEscrowGasTest.ts" file shows :

function Min Max Avg
createLock 327904 3978208 485659 Before packing
createLock 305494 3956110 463259 After packing
Before packing After packing
VotingEscrow Deployments cost 4374338 4268594

4- Avoid contract existence checks by using solidity version 0.8.10 or later :

Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (700 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value.

There are 2 instances of this issue:

File: contracts/VotingEscrow.sol

125     require(!IBlocklist(blocklis).isBlocked(msg.sender),"Blocked contract");
563     require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");

5- 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 as you can check here.

So use uint256/int256 for state variables and then downcast to lower sizes where needed.

There are 4 instances of this issue:

File: contracts/VotingEscrow.sol

60      mapping(uint256 => int128) public slopeChanges;
229     int128 oldSlopeDelta = 0;
230     int128 newSlopeDelta = 0;
313     int128 dSlope = 0;

6- Duplicated require() checks should be refactored to a modifier for saving deployment costs :

require() checks repeated in multiple functions should be refactored into a modifier to save deployment gas.

There are 4 instances of this issue:

File: contracts/VotingEscrow.sol

140     require(msg.sender == owner, "Only owner");
147     require(msg.sender == owner, "Only owner");
154     require(msg.sender == owner, "Only owner");
162     require(msg.sender == owner, "Only owner");

Those checks should be replaced by a onlyOwner modifier.

7- It costs more gas to initialize variables to zero than to let the default of zero be applied :

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

There are 14 instances of this issue:

File: contracts/VotingEscrow.sol

229     int128 oldSlopeDelta = 0;
230     int128 newSlopeDelta = 0;
298     uint256 blockSlope = 0;
309     for (uint256 i = 0; i < 255; i++)
313     int128 dSlope = 0;
714     uint256 min = 0;
717     for (uint256 i = 0; i < 128; i++)
737     uint256 min = 0;
739     for (uint256 i = 0; i < 128; i++)
793     uint256 dBlock = 0;
794     uint256 dTime = 0;
834     for (uint256 i = 0; i < 255; i++)
836     int128 dSlope = 0
889     uint256 dTime = 0;

8- Use of ++i cost less gas than i++ in for loops :

There are 4 instances of this issue:

File: contracts/VotingEscrow.sol

309     for (uint256 i = 0; i < 255; i++)
717     for (uint256 i = 0; i < 128; i++)
739     for (uint256 i = 0; i < 128; i++)
834     for (uint256 i = 0; i < 255; i++)

9- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops :

There are 4 instances of this issue:

File: contracts/VotingEscrow.sol

309     for (uint256 i = 0; i < 255; i++)
717     for (uint256 i = 0; i < 128; i++)
739     for (uint256 i = 0; i < 128; i++)
834     for (uint256 i = 0; i < 255; i++)

10- Using > 0 costs more gas than != 0 when used on a uint in a require() statement :

There is 1 instance of this issue:

File: contracts/VotingEscrow.sol

412     require(_value > 0, "Only non zero amount");
448     require(_value > 0, "Only non zero amount");

11- X += Y costs more gas than X = X + Y for state variables :

There is 1 instance of this issue:

File: contracts/VotingEscrow.sol

654     penaltyAccumulated += penaltyAmount;

12- Using private rather than public for constants, saves gas :

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

There are 3 instances of this issue:

File: contracts/VotingEscrow.sol
46     uint256 public constant WEEK = 7 days;
47     uint256 public constant MAXTIME = 365 days;
48     uint256 public constant MULTIPLIER = 10**18;

13- Public functions not called by the contract should be declared external instead :

There is 1 instance of this issue:

File: contracts/features/Blocklist.sol

33     function isBlocked(address addr) public view returns (bool)

14- Functions guaranteed to revert when called by normal users can be marked payable :

If a function modifier such as onlyAdmin is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for the owner because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are :

CALLVALUE(gas=2), DUP1(gas=3), ISZERO(gas=3), PUSH2(gas=3), JUMPI(gas=10), PUSH1(gas=3), DUP1(gas=3), REVERT(gas=0), JUMPDEST(gas=1), POP(gas=2). Which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 4 instances of this issue:

File: contracts/VotingEscrow.sol

139     function transferOwnership(address _addr) external
146     function updateBlocklist(address _addr) external
153     function updatePenaltyRecipient(address _addr) external
161     function unlock() external
lacoop6tu commented 2 years ago

Good one