FIAT DAO veFDT contest - __141345__'s results

EVENT IS MISSING INDEXED FIELDS

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

contracts/VotingEscrow.sol

    event Deposit(
        address indexed provider,
        uint256 value,
        uint256 locktime,
        LockAction indexed action,
        uint256 ts
    );
    event Withdraw(
        address indexed provider,
        uint256 value,
        LockAction indexed action,
        uint256 ts
    );

MAGICAL NUMBER CAN BE DOCUMENTED AND EXPLAINED

In several locations in the code, numbers like 1000000000000000000, 1000000000 are used. It's quite easy to make a mistake somewhere, also when comparing values.

which both obscure the purpose of the function and unnecessarily lead to potential error if the constants are changed during development. Since they are used to refer to a constant, when needed that constant can be called.

Suggestion: Use constants as this would make the code more maintainable and readable while costing nothing gas-wise.

contracts/VotingEscrow.sol

57-58:
    Point[1000000000000000000] public pointHistory; // 1e9 * userPointHistory-length, so sufficient for 1e9 users
    mapping(address => Point[1000000000]) public userPointHistory;

AVOID FLOATING PRAGMAS: THE VERSION SHOULD BE LOCKED (PREFERABLY AT >= 0.8.4)

The pragma declared across the solution is ^0.8.3. As the compiler introduces a several interesting upgrades in Solidity 0.8.4, consider locking at this version or a more recent one.

Locking the pragma (for e.g. by not using ^ in pragma solidity 0.8.3) ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs. (see here)

USE A MORE RECENT VERSION OF SOLIDITY

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.

USE CUSTOM ERRORS INSTEAD OF REVERT STRINGS

Custom errors from Solidity 0.8.4 are more gas efficient than revert strings (lower deployment cost and runtime cost when the revert condition is met) (reference)

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 are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

The demo of the gas comparison can be seen here.

contracts/features/Blocklist.sol
24-25:
            require(msg.sender == manager, "Only manager");
            require(_isContract(addr), "Only contracts");

contracts/VotingEscrow.sol
116:        require(decimals <= 18, "Exceeds max decimals");
125-128:
            require(
                !IBlocklist(blocklist).isBlocked(msg.sender),
                "Blocked contract"
            );
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-416:
            require(_value > 0, "Only non zero amount");
            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");
425-428:
            require(
                token.transferFrom(msg.sender, address(this), _value),
                "Transfer failed"
            );
448-450:
            require(_value > 0, "Only non zero amount");
            require(locked_.amount > 0, "No lock");
            require(locked_.end > block.timestamp, "Lock expired");
469-470:
            require(locked_.amount > 0, "Delegatee has no lock");
            require(locked_.end > block.timestamp, "Delegatee lock expired");
485-488:
            require(
                token.transferFrom(msg.sender, address(this), _value),
                "Transfer failed"
            );
502-504:
            require(locked_.amount > 0, "No lock");
            require(unlock_time > locked_.end, "Only increase lock end");
            require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
511:        require(oldUnlockTime > block.timestamp, "Lock expired");
529-531:
            require(locked_.amount > 0, "No lock");
            require(locked_.end <= block.timestamp, "Lock not expired");
            require(locked_.delegatee == msg.sender, "Lock delegated");
546:        require(token.transfer(msg.sender, value), "Transfer failed");
563-565:
            require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");
            require(locked_.amount > 0, "No lock");
            require(locked_.delegatee != _addr, "Already delegated");
587-589:
            require(toLocked.amount > 0, "Delegatee has no lock");
            require(toLocked.end > block.timestamp, "Delegatee lock expired");
            require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
635-637:
            require(locked_.amount > 0, "No lock");
            require(locked_.end > block.timestamp, "Lock expired");
            require(locked_.delegatee == msg.sender, "Lock delegated");
657:        require(token.transfer(msg.sender, remainingAmount), "Transfer failed");
676:        require(token.transfer(penaltyRecipient, amount), "Transfer failed");
877:        require(_blockNumber <= block.number, "Only past block number");

The modifier checkBlocklist() might be called frequently, for gas saving perspective, it could be beneficial.

Suggestion: Use solidity version at least 0.8.4, and use custom errors.

Variable re-arrangement by storage packing

In struct LockedBalance, int128 amount can be placed next to int128 delegated, as a result, 1 slot storage can be saved. According to the currently layout, they both occupy 1 slot, but after re-arrangement, they can be packed into 1 slot.

contracts/VotingEscrow.sol

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

Suggestion: change to:

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

It is the location of delegated that was moved upwards.

Reference: Layout of State Variables in Storage.

Variables declaration can be moved outside the loop

Variables declaration can be moved outside the loop, since each declaration has overhead. If needed, these variables can be set to 0 after each use.

contracts/VotingEscrow.sol int128 dslope

309-347:
        for (uint256 i = 0; i < 255; ++i) {
            int128 dslope = 0;
            // ...
            } else {
                dSlope = slopeChanges[iterativeTime];
            }
            // ...
            lastPoint.slope = lastPoint.slope + dSlope;
        }

uint256 mid

717-725:
        for (uint256 i = 0; i < 128; ++i) {
            // ...
            uint256 mid = (min + max + 1) / 2;
            if (pointHistory[mid].blk <= _block) {
                min = mid;
            } else {
                max = mid - 1;
            }
        }

uint256 mid

739-751:
        for (uint256 i = 0; i < 128; ++i) {
            uint256 mid = (min + max + 1) / 2;
            if (userPointHistory[_addr][mid].blk <= _block) {
                min = mid;
            } else {
                max = mid - 1;
            }
        }

int128 dslope

834-855:
        for (uint256 i = 0; i < 255; ++i) {
            int128 dslope = 0;
            // ...
                dslope = slope_changes[t_i];
            }
            // ...
            lastPoint.slope = lastPoint.slope + dSlope;
        }

The demo of the gas comparison can be seen here.

NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

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

contracts/VotingEscrow.sol

229:        int128 oldSlopeDelta = 0;
230:        int128 newSlopeDelta = 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;

FOR-LOOP unchecked{++i} COSTS LESS GAS COMPARED TO i++ OR i += 1

Use ++i instead of i++ to increment the value of an uint variable, and for guaranteed non-overflow/underflow, it can be unchecked. And the optimizer need to be turned on.

The demo of the loop gas comparison can be seen here.

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

Suggestion: For readability, uncheck the whole for loop is the same.

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

DUPLICATED REQUIRE() CHECKS COULD BE REFACTORED TO A MODIFIER OR FUNCTION

An error message can be passed as a parameter if needed.

The duplicates can be divided into 3 groups in the following: 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");

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");
635:        require(locked_.amount > 0, "No lock");

450:        require(locked_.end > block.timestamp, "Lock expired");
470:        require(locked_.end > block.timestamp, "Delegatee lock expired");
637:        require(locked_.end > block.timestamp, "Lock expired");

MULTIPLE ADDRESS MAPPINGS CAN BE COMBINED INTO A SINGLE MAPPING OF AN ADDRESS TO A STRUCT WHERE APPROPRIATE

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.

contracts/VotingEscrow.sol

58-59:
    mapping(address => Point[1000000000]) public userPointHistory;
    mapping(address => uint256) public userPointEpoch;

Save some variables as constant

These variables will not be changed, can be set to constant to save gas.

contracts/VotingEscrow.sol

64-66:
    string public name;
    string public symbol;
    uint256 public decimals = 18;

MAKING SOME VARIABLES AS NON-PUBLIC

The following can be set to internal or private if appropriate:

contracts/VotingEscrow.sol

60:    mapping(uint256 => int128) public slopeChanges;

X = X + Y / X = X - Y IS CHEAPER THAN X += Y / X -= Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y / X = X - Y to save gas

contracts/VotingEscrow.sol

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));
603:        newLocked.delegated += value;
654:        penaltyAccumulated += penaltyAmount;

537:        newLocked.delegated -= int128(int256(value));
612:        newLocked.delegated -= value;
642:        newLocked.delegated -= int128(int256(value));

Variables referred multiple times can be cached in local memory

Each storage read uses opcode sload which costs 100 gas (warm access), while memory read uses mload which only cost 3 gas. (reference) Even for a memory struct variable, the member variable access still has overhead. It can be saved further by caching the final result.

userPointEpoch[_addr] can be cached. contracts/VotingEscrow.sol

210-215:
        uint256 uepoch = userPointEpoch[_addr];
        Point memory point = userPointHistory[_addr][uepoch];

The following variables can all be saved into local memory for cheaper gas:

• _oldLocked.end
• block.timestamp
• _oldLocked.delegated
• _newLocked.delegated
• uEpoch + 1

contracts/VotingEscrow.sol

236-341:
        if (_oldLocked.end > block.timestamp && _oldLocked.delegated > 0) {
            userOldPoint.slope =
                _oldLocked.delegated /
                int128(int256(MAXTIME));
            userOldPoint.bias =
                userOldPoint.slope *
                int128(int256(_oldLocked.end - block.timestamp));
        }
        if (_newLocked.end > block.timestamp && _newLocked.delegated > 0) {
            userNewPoint.slope =
                _newLocked.delegated /
                int128(int256(MAXTIME));
            userNewPoint.bias =
                userNewPoint.slope *
                int128(int256(_newLocked.end - block.timestamp));
        }

            uint256 uEpoch = userPointEpoch[_addr];
            if (uEpoch == 0) {
                userPointHistory[_addr][uEpoch + 1] = userOldPoint;
            }

        userPointEpoch[_addr] = uEpoch + 1;
        userNewPoint.ts = block.timestamp;
        userNewPoint.blk = block.number;
        userPointHistory[_addr][uEpoch + 1] = userNewPoint;

        // ...

        oldSlopeDelta = slopeChanges[_oldLocked.end];
        if (_newLocked.end != 0) {
            if (_newLocked.end == _oldLocked.end) {
                newSlopeDelta = oldSlopeDelta;
            } else {
                newSlopeDelta = slopeChanges[_newLocked.end];
            }
        }

        if (block.timestamp > lastPoint.ts) {
            blockSlope =
                (MULTIPLIER * (block.number - lastPoint.blk)) /
                (block.timestamp - lastPoint.ts);
        }

        // ...
            if (iterativeTime > block.timestamp) {
                iterativeTime = block.timestamp;
            } else {
        // ...
            if (iterativeTime == block.timestamp) {

        }

(userPointHistory[_addr] can be cached. contracts/VotingEscrow.sol

739-749:
        for (uint256 i = 0; i < 128; i++) {
            if (min >= max) {
                break;
            }
            uint256 mid = (min + max + 1) / 2;
            if (userPointHistory[_addr][mid].blk <= _block) {
                min = mid;
            } else {
                max = mid - 1;
            }
        }