FIAT DAO veFDT contest - c3phas's results

QA

Missing checks for address(0x0) when assigning values to address state variables

File: Blocklist.sol Line 15

        manager = _manager;

File: Blocklist.sol Line 16

        ve = _ve;

File: VotingEscrow.sol Line 120

        owner = _owner;

File: VotingEscrow.sol Line 121

        penaltyRecipient = _penaltyRecipient;

constants should be defined rather than using magic numbers

There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.

Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name.

File: VotingEscrow.sol Line 653 10**18

        uint256 penaltyAmount = (value * penaltyRate) / 10**18; // quitlock_penalty is in 18 decimals precision

File: VotingEscrow.sol Line 309 255

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

File: VotingEscrow.sol Line 834 255

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

Lock pragmas to specific compiler version

Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs. Contracts may also be deployed by others and the pragma indicates the compiler version intended by the original authors.

File: Blocklist.sol Line 2

pragma solidity ^0.8.3;

File: VotingEscrow.sol Line 2

pragma solidity ^0.8.3;

Natspec is incomplete

File: VotingEscrow.sol Line 152-157 Missing @param _addr

    /// @notice Updates the recipient of the accumulated penalty paid by quitters
    function updatePenaltyRecipient(address _addr) external {
        require(msg.sender == owner, "Only owner");
        penaltyRecipient = _addr;
        emit UpdatePenaltyRecipient(_addr);
    }

File: VotingEscrow.sol Line 167-170 Missing @param _addr

    /// @notice Forces an undelegation of virtual balance for a blocked locker
    /// @dev Can only be called by the Blocklist contract (as part of a block)
    /// @dev This is an irreversible action
    function forceUndelegate(address _addr) external override {

File: VotingEscrow.sol Line 145-146 Missing @param _addr

    /// @notice Updates the blocklist contract
    function updateBlocklist(address _addr) external {

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.

File:Blocklist.sol Line 33-35

    function isBlocked(address addr) public view returns (bool) {
        return _blocklist[addr];
    }

File:VotingEscrow.sol Line 754-767

    function balanceOf(address _owner) public view override returns (uint256) {
        uint256 epoch = userPointEpoch[_owner];
        if (epoch == 0) {
            return 0;
        }
        Point memory lastPoint = userPointHistory[_owner][epoch];
        lastPoint.bias =
            lastPoint.bias -
            (lastPoint.slope * int128(int256(block.timestamp - lastPoint.ts)));
        if (lastPoint.bias < 0) {
            lastPoint.bias = 0;
        }
        return uint256(uint128(lastPoint.bias));
    }

File:VotingEscrow.sol Line 770

    function balanceOfAt(address _owner, uint256 _blockNumber)

FINDINGS

Using immutable on variables that are only set in the constructor and never after

Use immutable if you want to assign a permanent value at construction. Use constants if you already know the permanent value. Both get directly embedded in bytecode, saving SLOAD.Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

File: Blocklist.sol Line 11

    address public manager;

File: Blocklist.sol Line 12

    address public ve;

File: VotingEscrow.sol Line 45

    IERC20 public token;

Cache storage values in memory to minimize SLOADs

The code can be optimized by minimizing the number of SLOADs.

SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.

File: VotingEscrow.sol Line 673-678

VotingEscrow.sol.collectPenalty(): penaltyRecipient should be cached(Saves 1 sload : ~94 gas)

    function collectPenalty() external {
        uint256 amount = penaltyAccumulated;
        penaltyAccumulated = 0;
        require(token.transfer(penaltyRecipient, amount), "Transfer failed");
        emit CollectPenalty(amount, penaltyRecipient);
    }

SLOAD 1: Line 676 SLOAD 2: Line 677

Pack structs by putting data types in ascending size

When defining a struct, pack the values so that the data types are in ascending order. This will make sure that data types that can be put into the same slot are packed together instead of each variable having a separate storage slot.

Affected code File: VotingEscrow.sol Line 75-80

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

The above should be modified to:

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

Using unchecked blocks to save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block see resource

File: VotingEscrow.sol Line 302

                (block.timestamp - lastPoint.ts);

The above operation cannot underflow due to the check on Line 299 which ensures that block.timestamp is greater than lastPoint.ts

Using unchecked blocks to save gas - Increments in for loop can be unchecked ( save 30-40 gas per loop iteration)

The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.

e.g Let's work with a sample loop below.

for(uint256 i; i < 10; i++){
//doSomething
}

can be written as shown below.

for(uint256 i; i < 10;) {
  // loop logic
  unchecked { i++; }
}

We can also write it as an inlined function like below.

function inc(i) internal pure returns (uint256) {
  unchecked { return i + 1; }
}
for(uint256 i; i < 10; i = inc(i)) {
  // doSomething
}

Affected code

File:VotingEscrow.sol Line 309

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

Other Instances to modify File:VotingEscrow.sol Line 717

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

File:VotingEscrow.sol Line 739

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

File: VotingEscrow.sol Line 834

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

see resource

Internal/Private 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.

Affected code: File: VotingEscrow.sol Line 662-669

    function _calculatePenaltyRate(uint256 end)
        internal
        view
        returns (uint256)
    {
        // We know that end > block.timestamp because expired locks cannot be quitted
        return ((end - block.timestamp) * maxPenalty) / MAXTIME;
    }

The above function is only used once on File: VotingEscrow.sol at Line 652

File: Blocklist.sol Line 37-43

    function _isContract(address addr) internal view returns (bool) {
        uint256 size;
        assembly {
            size := extcodesize(addr)
        }
        return size > 0;
    }

++i costs less gas compared to i++ or i += 1 (~5 gas per iteration)

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

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

Instances include: File:VotingEscrow.sol Line 309

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

File:VotingEscrow.sol Line 717

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

File:VotingEscrow.sol Line 739

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

File: VotingEscrow.sol Line 834

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

Comparisons: != is more efficient than > in require (6 gas less)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

For uints the minimum value would be 0 and never a negative value. Since it cannot be a negative value, then the check > 0 is essentially checking that the value is not equal to 0 therefore >0 can be replaced with !=0 which saves gas.

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

File: VotingEscrow.sol Line 412

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

File:VotingEscrow.sol Line 448

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

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

Instances affected include File: Blocklist.sol Line 10

    mapping(address => bool) private _blocklist;

Use Shift Right/Left instead of Division/Multiplication

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.

relevant source

File: VotingEscrow.sol Line 719

            uint256 mid = (min + max + 1) / 2;

File:VotingEscrow.sol Line 743

            uint256 mid = (min + max + 1) / 2;

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

File:VotingEscrow.sol Line 654

        penaltyAccumulated += penaltyAmount;

penaltyAccumulated is a state variable as such we should modify the above as

        penaltyAccumulated = penaltyAccumulated + penaltyAmount;

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

This saves deployement gas

File: VotingEscrow.sol Line 140

        require(msg.sender == owner, "Only owner");

The above require statement is repeated 3 more times in the following lines File: VotingEscrow.sol Line 147 File: VotingEscrow.sol Line 154 File: VotingEscrow.sol Line 162

Use Custom Errors instead of Revert Strings to save Gas(~50 gas)

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) Custom errors save ~50 gas each time they’re hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries). see Source

File: Blocklist.sol Line 24

        require(msg.sender == manager, "Only manager");

File: Blocklist.sol Line 25

        require(_isContract(addr), "Only contracts");

File: VotingEscrow.sol Line 116

        require(decimals <= 18, "Exceeds max decimals");

File: VotingEscrow.sol Line 125-128

        require(
            !IBlocklist(blocklist).isBlocked(msg.sender),
            "Blocked contract"
        );

File: VotingEscrow.sol Line 140

        require(msg.sender == owner, "Only owner");

File: VotingEscrow.sol Line 171

        require(msg.sender == blocklist, "Only Blocklist");

File: VotingEscrow.sol Line 412

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

File:VotingEscrow.sol Line 413-416

        require(locked_.amount == 0, "Lock exists");
        require(unlock_time >= locked_.end, "Only increase lock end"); // from using quitLock, user should increaseAmount instead
        require(unlock_time > block.timestamp, "Only future lock end");
        require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

File:VotingEscrow.sol Line 425-428

        require(
            token.transferFrom(msg.sender, address(this), _value),
            "Transfer failed"
        );

File:VotingEscrow.sol Line 448

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

File:VotingEscrow.sol Line 449-450

        require(locked_.amount > 0, "No lock");
        require(locked_.end > block.timestamp, "Lock expired");

File: VotingEscrow.sol Line 469-470

            require(locked_.amount > 0, "Delegatee has no lock");
            require(locked_.end > block.timestamp, "Delegatee lock expired");

File:VotingEscrow.sol Line 485

        require(
            token.transferFrom(msg.sender, address(this), _value),
            "Transfer failed"
        );

File: VotingEscrow.sol Lines 502-504 File: VotingEscrow.sol Line 511 File: VotingEscrow.sol Line 529,530,531 File: VotingEscrow.sol Line 546 File: VotingEscrow.sol Line 563,564,565 File: VotingEscrow.sol Line 587,588,589 File: VotingEscrow.sol Line 635,636,637 File: VotingEscrow.sol Line 657 File: VotingEscrow.sol Line 676 File: VotingEscrow.sol Line 776

Proof of custom errors optimizations

contract GasTest is DSTest {
    Contract0 c0;
    Contract1 c1;

    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();
    }

    function testFailGas() public {
        c0.stringErrorMessage();
        c1.customErrorMessage();
    }
}

contract Contract0 {
    function stringErrorMessage() public {
        bool check = false;
        require(check, "error message");
    }
}

contract Contract1 {
    error CustomError();

    function customErrorMessage() public {
        bool check = false;
        if (!check) {
            revert CustomError();
        }
    }
}

Gas comparisons

╭────────────────────┬─────────────────┬─────┬────────┬─────┬─────────╮
│ Contract0 contract ┆                 ┆     ┆        ┆     ┆         │
╞════════════════════╪═════════════════╪═════╪════════╪═════╪═════════╡
│ Deployment Cost    ┆ Deployment Size ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 34087              ┆ 200             ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name      ┆ min             ┆ avg ┆ median ┆ max ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ stringErrorMessage ┆ 218             ┆ 218 ┆ 218    ┆ 218 ┆ 1       │
╰────────────────────┴─────────────────┴─────┴────────┴─────┴─────────╯
╭────────────────────┬─────────────────┬─────┬────────┬─────┬─────────╮
│ Contract1 contract ┆                 ┆     ┆        ┆     ┆         │
╞════════════════════╪═════════════════╪═════╪════════╪═════╪═════════╡
│ Deployment Cost    ┆ Deployment Size ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 26881              ┆ 164             ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name      ┆ min             ┆ avg ┆ median ┆ max ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ customErrorMessage ┆ 161             ┆ 161 ┆ 161    ┆ 161 ┆ 1       │
╰────────────────────┴─────────────────┴─────┴────────┴─────┴─────────╯