FIAT DAO veFDT contest - 0xDjango's results

Low Risk Findings Overview

Non-critical Findings Overview

QA overview per contract

Low Risk Findings

[L-01] Floating Pragma

A floating pragma might result in contract being tested/deployed with different or inconsistent compiler versions possibly leading to unexpected behaviour. 1 instance of this issue has been found:

[L-01] Blocklist.sol#L2-L3

pragma solidity ^0.8.3;

[L-02] Unsafe call to optional decimals ERC20 method

This function was optional in the initial ERC-20 and might fail for old tokens that have not implemented it. 1 instance of this issue has been found:

[L-02] VotingEscrow.sol#L113-L114

        decimals = IERC20(_token).decimals();

[L-03] Ownership transfer should be a two-step process

If ownership is accidentally transferred to an inactive account all functionalities depending on it will be lost. 1 instance of this issue has been found:

[L-03] VotingEscrow.sol#L137-L141

   function transferOwnership(address _addr) external {
        require(msg.sender == owner, "Only owner");
        owner = _addr;
        emit TransferOwnership(_addr);
    }

[L-04] Irreversible action to erase penalty mechanism might require redeploy

If unlock() function is accidentally called the contract might have to be redeployed. Perhaps making it reversible could be a sensible choice. 1 instance of this issue has been found:

[L-04] VotingEscrow.sol#L159-L163

    function unlock() external {
        require(msg.sender == owner, "Only owner");
        maxPenalty = 0;
        emit Unlock();
    }

Non-critical Findings

[N-01] It is recommend to use scientific notation (1e18) instead of exponential (10**18)

Improves readbility. 3 instances of this issue have been found:

[N-01] VotingEscrow.sol#L51-L52

    uint256 public maxPenalty = 10**18; // penalty for quitters with MAXTIME remaining lock

[N-01b] VotingEscrow.sol#L48-L49

    uint256 public constant MULTIPLIER = 10**18;

[N-01c] VotingEscrow.sol#L653-L654

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

[N-02] 2**<n> - 1 should be factored as type(uint<n>).max

2**<n> - 1 should be re-written as type(uint<n>).max 1 instance of this issue has been found:

[N-02] VotingEscrow.sol#L310-L311

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

Gas Optimisations

Gas overview per contract

Gas Optimisations

[G-01] Setting variable to default value is redundant

Setting variable to default value is redundant. 3 instances of this issue have been found:

[G-01] VotingEscrow.sol#L717-L718

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

[G-01b] VotingEscrow.sol#L739-L740

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

[G-01c] VotingEscrow.sol#L310-L311

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

[G-02] Using prefix(++i) instead of postfix (i++) saves gas

It saves 6 gas per iteration. 3 instances of this issue have been found:

[G-02] VotingEscrow.sol#L739-L740

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

[G-02b] VotingEscrow.sol#L717-L718

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

[G-02c] VotingEscrow.sol#L310-L311

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

[G-03] for loop increments should be unchecked{} if overflow is not possible

From Solidity 0.8.0 onwards using the unchecked keyword saves 30 to 40 gas per loop. Example:

uint length = array.length;
for (uint i; i < length;){
		uncheck { ++i }
}

3 instances of this issue have been found:

[G-03] VotingEscrow.sol#L717-L718

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

[G-03b] VotingEscrow.sol#L739-L740

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

[G-03c] VotingEscrow.sol#L310-L311

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

[G-04] 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. 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 operatio 1 instance of this issue has been found:

[G-04] VotingEscrow.sol#L56-L59

    mapping(address => Point[1000000000]) public userPointHistory;
    mapping(address => uint256) public userPointEpoch;
    mapping(uint256 => int128) public slopeChanges;
    mapping(address => LockedBalance) public locked;

[G-05] Variable should be immutable

Variables set in the constructor that are not able to be changed by other functions should be set as immutable in order to save gas. 1 instance of this issue has been found:

[G-05] Blocklist.sol#L11-L12

    address public manager;
    address public ve;

[G-06] immutable variable missing zero address check

If variable is accidentally set to 0 the contract will have to be redeployed, spending unnecessary gas. 1 instance of this issue has been found:

[G-06] Blocklist.sol#L14-L17

    constructor(address _manager, address _ve) {
        manager = _manager;
        ve = _ve;
    }

[G-07] require()/revert() checks used multiple times should be turned into a function or modifier

Doing so increases code readability decreases number of instructions for the compiler. 20 instances of this issue have been found:

[G-07] VotingEscrow.sol#L676-L677

        require(token.transfer(penaltyRecipient, amount), "Transfer failed");

[G-07b] VotingEscrow.sol#L657-L658

        require(token.transfer(msg.sender, remainingAmount), "Transfer failed");

[G-07c] VotingEscrow.sol#L546-L547

        require(token.transfer(msg.sender, value), "Transfer failed");

[G-07d] VotingEscrow.sol#L485-L488

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

[G-07e] VotingEscrow.sol#L425-L428

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

[G-07f] VotingEscrow.sol#L504-L505

        require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

[G-07g] VotingEscrow.sol#L416-L417

        require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

[G-07h] VotingEscrow.sol#L503-L504

        require(unlock_time > locked_.end, "Only increase lock end");

[G-07i] VotingEscrow.sol#L414-L415

        require(unlock_time >= locked_.end, "Only increase lock end"); // from using quitLock, user should increaseAmount instead

[G-07j] VotingEscrow.sol#L412-L413

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

[G-07k] VotingEscrow.sol#L563-L564

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

[G-07l] VotingEscrow.sol#L123-L128

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

[G-07m] VotingEscrow.sol#L448-L449

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

[G-07n] VotingEscrow.sol#L412-L413

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

[G-07o] VotingEscrow.sol#L138-L139

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

[G-07p] VotingEscrow.sol#L145-L146

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

[G-07q] VotingEscrow.sol#L152-L153

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

[G-07r] VotingEscrow.sol#L160-L161

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

[G-07s] VotingEscrow.sol#L877-L878

        require(_blockNumber <= block.number, "Only past block number");

[G-07t] VotingEscrow.sol#L776-L777

        require(_blockNumber <= block.number, "Only past block number");

[G-08] Use custom errors rather than revert()/require() strings to save gas

Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. 27 instances of this issue have been found:

[G-08] VotingEscrow.sol#L114-L115

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

[G-08b] VotingEscrow.sol#L123-L127

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

[G-08c] VotingEscrow.sol#L138-L139

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

[G-08d] VotingEscrow.sol#L152-L153

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

[G-08e] VotingEscrow.sol#L160-L161

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

[G-08f] VotingEscrow.sol#L169-L170

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

[G-08g] VotingEscrow.sol#L412-L416

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

[G-08h] VotingEscrow.sol#L425-L427

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

[G-08i] VotingEscrow.sol#L448-L450

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

[G-08j] VotingEscrow.sol#L469-L470

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

[G-08k] VotingEscrow.sol#L485-L488

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

[G-08l] VotingEscrow.sol#L502-L504

        require(locked_.amount > 0, "No lock");
        require(unlock_time > locked_.end, "Only increase lock end");
        require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

[G-08m] VotingEscrow.sol#L511-L512

            require(oldUnlockTime > block.timestamp, "Lock expired");

[G-08n] VotingEscrow.sol#L529-L531

        require(locked_.amount > 0, "No lock");
        require(locked_.end <= block.timestamp, "Lock not expired");
        require(locked_.delegatee == msg.sender, "Lock delegated");

[G-08o] VotingEscrow.sol#L563-L564

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

[G-08p] VotingEscrow.sol#L564-L565

        require(locked_.amount > 0, "No lock");

[G-08q] VotingEscrow.sol#L565-L566

        require(locked_.delegatee != _addr, "Already delegated");

[G-08r] VotingEscrow.sol#L587-L588

        require(toLocked.amount > 0, "Delegatee has no lock");

[G-08s] VotingEscrow.sol#L588-L589

        require(toLocked.end > block.timestamp, "Delegatee lock expired");

[G-08t] VotingEscrow.sol#L589-L590

        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");

[G-08u] VotingEscrow.sol#L635-L636

        require(locked_.amount > 0, "No lock");

[G-08v] VotingEscrow.sol#L636-L637

        require(locked_.end > block.timestamp, "Lock expired");

[G-08w] VotingEscrow.sol#L637-L638

        require(locked_.delegatee == msg.sender, "Lock delegated");

[G-08x] VotingEscrow.sol#L657-L658

        require(token.transfer(msg.sender, remainingAmount), "Transfer failed");

[G-08y] VotingEscrow.sol#L676-L677

        require(token.transfer(penaltyRecipient, amount), "Transfer failed");

[G-08z] VotingEscrow.sol#L776-L777

        require(_blockNumber <= block.number, "Only past block number");

[G-08{] VotingEscrow.sol#L877-L878

        require(_blockNumber <= block.number, "Only past block number");

[G-09] MULTIPLIER constant not used

Use already existent MULTIPLIER constant to avoid waisting gas on calculation. 1 instance of this issue has been found:

[G-09] VotingEscrow.sol#L653-L654

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