search

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

2 stars 1 forks source link

Gas Optimizations #29

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago
Summary

Gas savings are estimated using existing tests and may vary depending on the implementation of the fix. I keep my version of the fix for each finding and can provide them if you need. Since the optimizer config is set to 10000 runs, some optimizations were excluded. At a lower level of optimization, additional fixes make sense.

Gas Optimizations Issue Instances Estimated gas(deployments) Estimated gas(method call)
1 Use Custom Errors instead of Revert/Require Strings to save Gas 42 195612 -
2 The same check can be combined into a modifier or a function 3 180175 -
3 Avoid contract existence checks by using solidity version 0.8.10 or later 2 128057 7684
4 Pack structure members in one slot 1 102300 18294
5 State variables only set in the constructor should be declared immutable 2 29599 4203
6 Increment and assignment optimization 4/15 28389 3572
7 Using private rather than public for constants, saves gas 3 25197
8 Using bools for storage incurs overhead 1 3894 -
9 Division by two should use bit shifting 2 3692
10 Do not recalculate the same expression 1 3452 896

Total: 72 instances over 10 issues

  1. Use Custom Errors instead of Revert/Require Strings to save Gas (42 instances)

    Deployment gas saved: 195612

    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 save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Additional gas is saved due to the lack of defining string. https://blog.soliditylang.org/2021/04/21/custom-errors/#errors-in-depth

    116     require(decimals <= 18, "Exceeds max decimals");
...
125     require(
126         !IBlocklist(blocklist).isBlocked(msg.sender),
127         "Blocked contract"
128     );
...
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");
...
171     require(msg.sender == blocklist, "Only Blocklist");
...
412     require(_value > 0, "Only non zero amount");
413     require(locked_.amount == 0, "Lock exists");
414     require(unlock_time >= locked_.end, "Only increase lock end");
415     require(unlock_time > block.timestamp, "Only future lock end");
416     require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
...
425     require(
426         token.transferFrom(msg.sender, address(this), _value),
427         "Transfer failed"
428     );
...
448     require(_value > 0, "Only non zero amount");
449     require(locked_.amount > 0, "No lock");
450     require(locked_.end > block.timestamp, "Lock expired");
...
469     require(locked_.amount > 0, "Delegatee has no lock");
470     require(locked_.end > block.timestamp, "Delegatee lock expired");
...
485     require(
486         token.transferFrom(msg.sender, address(this), _value),
487         "Transfer failed"
488     );
...
502     require(locked_.amount > 0, "No lock");
503     require(unlock_time > locked_.end, "Only increase lock end");
504     require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
...
511     require(oldUnlockTime > block.timestamp, "Lock expired");
...
529     require(locked_.amount > 0, "No lock");
530     require(locked_.end <= block.timestamp, "Lock not expired");
531     require(locked_.delegatee == msg.sender, "Lock delegated");
...
546     require(token.transfer(msg.sender, value), "Transfer failed");
...
563     require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");
564     require(locked_.amount > 0, "No lock");
565     require(locked_.delegatee != _addr, "Already delegated");
...
587     require(toLocked.amount > 0, "Delegatee has no lock");
588     require(toLocked.end > block.timestamp, "Delegatee lock expired");
589     require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
...
635     require(locked_.amount > 0, "No lock");
636     require(locked_.end > block.timestamp, "Lock expired");
637     require(locked_.delegatee == msg.sender, "Lock delegated");
...
657     require(token.transfer(msg.sender, remainingAmount), "Transfer failed");
...
676     require(token.transfer(penaltyRecipient, amount), "Transfer failed");
...
776     require(_blockNumber <= block.number, "Only past block number");
...
877     require(_blockNumber <= block.number, "Only past block number");
    • contracts/features/Blocklist.sol:24-25
    24      require(msg.sender == manager, "Only manager");
25      require(_isContract(addr), "Only contracts");

  2. The same check can be combined into a modifier or a function (3 instances)

    Deployment Gas saved in total: 180175

    Deployment Gas Saved: 89936

    449     require(locked_.amount > 0, "No lock");
...
469     require(locked_.amount > 0, "Delegatee has no lock");
...
502     require(locked_.amount > 0, "No lock");
...
529     require(locked_.amount > 0, "No lock");
...
564     require(locked_.amount > 0, "No lock");
...
587     require(toLocked.amount > 0, "Delegatee has no lock");
...
635     require(locked_.amount > 0, "No lock");

    fix:

    function is_lock(int128 amount) internal pure {
    require(amount > 0, "No lock");
}

    Deployment Gas saved: 57894

    450     require(locked_.end > block.timestamp, "Lock expired");
...
470     require(locked_.end > block.timestamp, "Delegatee lock expired");
...
511     require(oldUnlockTime > block.timestamp, "Lock expired");
...
588     require(toLocked.end > block.timestamp, "Delegatee lock expired");
...
636     require(locked_.end > block.timestamp, "Lock expired");

    fix:

    function is_expired(uint256 exp_time) internal view {
    require(exp_time > block.timestamp, "Lock expired");
}

    Deployment Gas saved: 32345

    414     require(unlock_time >= locked_.end, "Only increase lock end");
...
416     require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
...
503     require(unlock_time > locked_.end, "Only increase lock end");
504     require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

    fix:

    function is_valid_unlock_time(uint256 unlock_time, uint256 exp_time)
    internal
    view
{
    require(unlock_time >= exp_time, "Only increase lock end");
    require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
}

  3. Avoid contract existence checks by using solidity version 0.8.10 or later

    Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath
Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
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

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

By changing version to 0.8.10 in contracts VotingEscrow.sol and Blocklist.sol

    Deployment gas saved: 128057 Method Call gas saved: 7684

  4. Pack structure members in one slot

    Deployment Gas Saved: 102300 Method Call Gas Saved: 18294

    • contracts/VotingEscrow.sol:75-80
    75     struct LockedBalance {
76          int128 amount;
77          uint256 end;
78          int128 delegated;
79          address delegatee;
80      }

    fix:

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

  5. State variables only set in the constructor should be declared immutable

    Deployment Gas Saved: 29599 Method Call Gas Saved: 4203

    • contracts/features/Blocklist.sol:11-12
    11    address public manager;
12    address public ve;

  6. Increment and assignment optimization (4/15 instances)

    All gas calculation for this 4 optimization are cummulitive

    1. Post increment and preincrement:

      Deployment gas Saved: 1724

    2. x=x+1 more efficient :

      Deployment Gas Saved: 12329

    3. increment in loop can be unchecked

      Deployment Gas Saved : 21181

      Method call Gas SAved: 3420

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

    1. It is also more efficient to use x = x + y instead of x +=

      Deployment Gas Saved : 28389

      Method call Gas SAved: 3572

    418     locked_.amount += int128(int256(_value));
...
420     locked_.delegated += int128(int256(_value));
...
460     newLocked.amount += int128(int256(_value));
461     newLocked.delegated += int128(int256(_value));
...
465     locked_.amount += int128(int256(_value));
...
472     newLocked.delegated += int128(int256(_value));
...
537     newLocked.delegated -= int128(int256(value));
...
603     newLocked.delegated += value;
...
643     newLocked.delegated -= int128(int256(value));
...
654     penaltyAccumulated += penaltyAmount;

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

    Deployment Gas Saved: 25197

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

  8. Using bools for storage incurs overhead (1 instances)

    // 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 to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from 'false' to 'true', after having been 'true' in the past

    Gas Saved: 3894

    • contracts/features/Blocklist.sol:10
    10      mapping(address => bool) private _blocklist;

    fix:

    10      mapping(address => uint256) private _blocklist;

    plus сhanging all occurrences in the code

  9. Division by two should use bit shifting

    Deploy Gas Saved: 3692

    / 2 is the same as >> 1. The DIV opcode costs 5 gas, whereas SHR only costs 3 gas. Especially in loops - contracts/VotingEscrow.sol:[719](https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L719),[743](https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L743) ```solidity 719 uint256 mid = (min + max + 1) / 2; ... 743 uint256 mid = (min + max + 1) / 2; ```

  10. Do not recalculate the same expression

    Deployment Gas Saved: 3452 Method Call Gas Saved: 896

    258      userPointHistory[_addr][uEpoch + 1] = userOldPoint;
...
261      userPointEpoch[_addr] = uEpoch + 1;
...
264      userPointHistory[_addr][uEpoch + 1] = userNewPoint;

    fix:

    uEpoch = uEpoch + 1;
if (uEpoch == 1) {
    userPointHistory[_addr][uEpoch] = userOldPoint;
}
userPointEpoch[_addr] = uEpoch;
userPointHistory[_addr][uEpoch] = userNewPoint;
gititGoro commented 2 years ago

Excellent report!