FIAT DAO veFDT contest - MiloTruck's results

Non-Critical Report

Total: 10 instances over 4 issues

<address>.code.length can be used instead of extcodesize()

Since Solidity v0.8.0 onwards, calling <address>.code returns the bytecode stored at the address in bytes, as seen from the documentation.

Thus, rather than using extcodesize(), check if <address>.code.length != 0 to determine if an address represents a contract that contains code.

The function _isContract() in Blocklist.sol uses extcodesize():

contracts/features/Blocklist.sol:
  37:        function _isContract(address addr) internal view returns (bool) {
  38:            uint256 size;
  39:            assembly {
  40:                size := extcodesize(addr)
  41:            }
  42:            return size > 0;
  43:        }

Consider changing it to the following:

    function _isContract(address addr) internal view returns (bool) {
        return addr.code.length != 0;
    }

This would help to improve code readeability and avoid the use of assembly unnecessarily.

Use scientific notation instead of exponentiation

Scientific notation should be used for clarity, such as 10e18 instead of 10**18.

There are 3 instances of this issue:

contracts/VotingEscrow.sol:
  48:        uint256 public constant MULTIPLIER = 10**18;
  51:        uint256 public maxPenalty = 10**18; // penalty for quitters with MAXTIME remaining lock
 653:        uint256 penaltyAmount = (value * penaltyRate) / 10**18; // quitlock_penalty is in 18 decimals precision

Floating compiler versions

Non-library/interface files should use fixed compiler versions, not floating ones.

There are 2 instances of this issue:

contracts/VotingEscrow.sol:
   2:        pragma solidity ^0.8.3;

contracts/features/Blocklist.sol:
   2:        pragma solidity ^0.8.3;

event is missing indexed fields

Each event should use three indexed fields if there are three or more fields.

There are 4 instances of this issue:

contracts/VotingEscrow.sol:
  25:        event Deposit(
  26:            address indexed provider,
  27:            uint256 value,
  28:            uint256 locktime,
  29:            LockAction indexed action,
  30:            uint256 ts
  31:        );


  32:        event Withdraw(
  33:            address indexed provider,
  34:            uint256 value,
  35:            LockAction indexed action,
  36:            uint256 ts
  37:        );

contracts/interfaces/IERC20.sol:
  29:        event Transfer(address indexed from, address indexed to, uint256 value);

  30:        event Approval(
  31:            address indexed owner,
  32:            address indexed spender,
  33:            uint256 value
  34:        );

Gas Report

Total: 112 instances over 19 issues

For-loops: Index initialized with default value

Uninitialized uint variables are assigned with a default value of 0.

Thus, in for-loops, explicitly initializing an index with 0 costs unnecesary gas. For example, the following code:

for (uint256 i = 0; i < length; ++i) {

can be written without explicitly setting the index to 0:

for (uint256 i; i < length; ++i) {

There are 4 instances of this issue:

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

For-Loops: Index increments can be left unchecked

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks.

In for-loops, as it is impossible for the index to overflow, index increments can be left unchecked to save 30-40 gas per loop iteration.

For example, the code below:

for (uint256 i; i < numIterations; ++i) {  
    // ...  
}  

can be changed to:

for (uint256 i; i < numIterations;) {  
    // ...  
    unchecked { ++i; }  
}  

There are 4 instances of this issue:

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

Arithmetics: ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2, thus it costs more gas.

The same logic applies for --i and i--.

There are 4 instances of this issue:

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

Arithmetics: Use != 0 instead of > 0 for unsigned integers in require statements

A variable of type uint will never go below 0. Thus, checking != 0 rather than > 0 is sufficient, which would save 6 gas per instance.

There are 9 instances of this issue:

contracts/VotingEscrow.sol:
 412:        require(_value > 0, "Only non zero amount");
 448:        require(_value > 0, "Only non zero amount");
 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");

Arithmetics: 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 MUL and DIV opcodes use 5 gas, the SHL and SHR opcodes only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

For example, the following code:

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

can be changed to:

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

There are 2 instances of this issue:

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

Visibility: Consider declaring constants as non-public to save gas

If a constant is not used outside of its contract, declaring its visibility as private or internal instead of public can save gas. If needed, the value can be read from the verified contract source code.

Gas savings occur as the compiler does not have to create non-payable getter functions for deployment calldata and add another entry to the method ID table.

There are 3 instances of this issue:

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

Visibility: public functions can be set to external

Calls to external functions are cheaper than public functions. Thus, if a function is not used internally in any contract, it should be set to external to save gas and improve code readability.

There is 1 instance of this issue:

contracts/features/Blocklist.sol:
  33:        function isBlocked(address addr) public view returns (bool) {

Duplicated require() checks should be refactored to a modifier or function

If a require() statement that is used to validate function parameters or global variables is present across multiple functions in a contract, it should be rewritten into modifier if possible. This would help to reduce code deployment size, which saves gas, and improve code readability.

There are 8 instances of this issue:
Instance #1:

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");

Instance #2:

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

Instance #3:

contracts/VotingEscrow.sol:
 416:        require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
 504:        require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

Instance #4:

contracts/VotingEscrow.sol:
 425:        require(
 426:            token.transferFrom(msg.sender, address(this), _value),
 427:            "Transfer failed"
 428:        );

 485:        require(
 486:            token.transferFrom(msg.sender, address(this), _value),
 487:            "Transfer failed"
 488:        );

Instance #5:

contracts/VotingEscrow.sol:
 449:        require(locked_.amount > 0, "No lock");
 502:        require(locked_.amount > 0, "No lock");
 529:        require(locked_.amount > 0, "No lock");
 564:        require(locked_.amount > 0, "No lock");
 635:        require(locked_.amount > 0, "No lock");

Instance #6:

contracts/VotingEscrow.sol:
 450:        require(locked_.end > block.timestamp, "Lock expired");
 636:        require(locked_.end > block.timestamp, "Lock expired");

Instance #7:

contracts/VotingEscrow.sol:
 531:        require(locked_.delegatee == msg.sender, "Lock delegated");
 637:        require(locked_.delegatee == msg.sender, "Lock delegated");

Instance #8:

contracts/VotingEscrow.sol:
 776:        require(_blockNumber <= block.number, "Only past block number");
 877:        require(_blockNumber <= block.number, "Only past block number");

Errors: Use custom errors instead of revert strings

Since Solidity v0.8.4, custom errors can be used rather than require statements.

Taken from Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors reduce runtime gas costs as they save about 50 gas each time they are hit by avoiding having to allocate and store the revert string. Furthermore, not definiting error strings also help to reduce deployment gas costs.

There are 42 instances of this issue:

contracts/VotingEscrow.sol:
 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"); // from using quitLock, user should increaseAmount instead
 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:        require(msg.sender == manager, "Only manager");
  25:        require(_isContract(addr), "Only contracts");

Unnecessary initialization of variables with default values

Uninitialized variables are assigned with a default value depending on its type:

• uint: 0
• bool: false
• address: address(0)

Thus, explicitly initializing a variable with its default value costs unnecesary gas. For example, the following code:

bool b = false;
address c = address(0);
uint256 a = 0;

can be changed to:

uint256 a;
bool b;
address c;

There are 6 instances of this issue:

contracts/VotingEscrow.sol:
 298:        uint256 blockSlope = 0; // dblock/dt
 714:        uint256 min = 0;
 737:        uint256 min = 0;
 793:        uint256 dBlock = 0;
 794:        uint256 dTime = 0;
 889:        uint256 dTime = 0;

Storage variables should be declared immutable when possible

If a storage variable is assigned only in the constructor, it should be declared as immutable. This would help to reduce gas costs as calls to immutable variables are much cheaper than regular state variables, as seen from the Solidity Docs:

Compared to regular state variables, the gas costs of constant and immutable variables are much lower. Immutable variables are evaluated once at construction time and their value is copied to all the places in the code where they are accessed.

This helps to save gas as it 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:

contracts/VotingEscrow.sol:
  45:        IERC20 public token;
  64:        string public name;
  65:        string public symbol;

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

State variables should be cached in stack variables rather than re-reading them from storage

If a state variable is read from storage multiple times in a function, it should be cached in a stack variable.

Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

There is 1 instance of this issue:
penaltyRecipient in collectPenalty():

contracts/VotingEscrow.sol:
 676:        require(token.transfer(penaltyRecipient, amount), "Transfer failed");
 677:        emit CollectPenalty(amount, penaltyRecipient);

<x> += <y> costs more gas than <x> = <x> + <y> for state variables

The above also applies to state variables that use -=.

There is 1 instance of this issue:

contracts/VotingEscrow.sol:
 654:        penaltyAccumulated += penaltyAmount;

Using bool for storage incurs overhead

Declaring storage variables as bool is more expensive compared to uint256, as explained here:

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

There is 1 instance of this issue:

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

Stack variable used to cache a state variable is only accessed once

If a state variable is only accessed once, avoid caching it in a local variable and use the state variable directly to save gas.

There is 1 instance of this issue:

contracts/VotingEscrow.sol:
 865:        uint256 epoch_ = globalEpoch;

Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

As seen from here:

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.

However, this does not apply to storage values as using reduced-size types might be beneficial to pack multiple elements into a single storage slot. Thus, where appropriate, use uint256/int256 and downcast when needed.

There are 15 instances of this issue:

contracts/VotingEscrow.sol:
  70:        int128 bias;
  71:        int128 slope;
  76:        int128 amount;
  78:        int128 delegated;
 174:        int128 value = locked_.amount;
 205:        int128 bias,
 206:        int128 slope,
 229:        int128 oldSlopeDelta = 0;
 230:        int128 newSlopeDelta = 0;
 313:        int128 dSlope = 0;
 319:        int128 biasDelta =
 567:        int128 value = locked_.amount;
 598:        int128 value,
 836:        int128 dSlope = 0;

contracts/interfaces/IERC20.sol:
  10:        function decimals() external view returns (uint8);

Not using the named return variables when a function returns, wastes deployment gas

There are 2 instances of this issue:

contracts/VotingEscrow.sol:
 212:        return (0, 0, 0);
 215:        return (point.bias, point.slope, point.ts);

internal functions only called once can be inlined to save gas

As compared to internal functions, a non-inlined function costs 20 to 40 gas extra because of two extra JUMP instructions and additional stack operations needed for function calls. Thus, consider inlining internal functions that are only called once in the function that calls them.

There is 1 instance of this issue:

contracts/features/Blocklist.sol:
  37:        function _isContract(address addr) internal view returns (bool) {

Multiple accesses of a mapping/array should use a local variable cache

If a mapping/array is read with the same key/index multiple times in a function, it should be cached in a stack variable.

Caching a mapping's value in a local storage variable when the value is accessed multiple times, saves ~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. Caching an array's struct avoids recalculating the array offsets into memory.

There are 2 instances of this issue:
locked[_addr] in delegate():

contracts/VotingEscrow.sol:
 575:        toLocked = locked[_addr];
 583:        toLocked = locked[_addr];

locked[delegatee] in delegate():

contracts/VotingEscrow.sol:
 578:        fromLocked = locked[delegatee];
 582:        fromLocked = locked[delegatee];