FIAT DAO veFDT contest - erictee's results

[L-01] decimals() not part of ERC20 standard.

Impact

decimals() is not part of the official ERC20 standard and might fall for tokens that do not implement it. While in practice it is very unlikely, as usually most of the tokens implement it, this should still be considered as a potential issue.

Findings:

2022-08-fiatdao/contracts/VotingEscrow.sol:L115        decimals = IERC20(_token).decimals();

[L-02] Avoid floating pragmas for non-library contracts.

Impact

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

Findings:

2022-08-fiatdao/contracts/VotingEscrow.sol:L2     pragma solidity ^0.8.3;

2022-08-fiatdao/contracts/interfaces/IBlocklist.sol:L2     pragma solidity ^0.8.3;

2022-08-fiatdao/contracts/interfaces/IERC20.sol:L2     pragma solidity ^0.8.3;

2022-08-fiatdao/contracts/interfaces/IVotingEscrow.sol:L2     pragma solidity ^0.8.3;

2022-08-fiatdao/contracts/features/Blocklist.sol:L2     pragma solidity ^0.8.3;

[N-01] Public functions not called by the contract should be declared external instead

Impact

Contracts are allowed to override their parents' functions and change the visibility from public to external to save gas.

Findings:

2022-08-fiatdao/contracts/VotingEscrow.sol:L754    function balanceOf(address _owner) public view override returns (uint256) {

2022-08-fiatdao/contracts/VotingEscrow.sol:L770    function balanceOfAt(address _owner, uint256 _blockNumber)

2022-08-fiatdao/contracts/VotingEscrow.sol:L1769    function totalSupply() public view override returns (uint256) {

2022-08-fiatdao/contracts/VotingEscrow.sol:L1776    function totalSupplyAt(uint256 _blockNumber)

2022-08-fiatdao/contracts/features/Blocklist.sol:L33    function isBlocked(address addr) public view returns (bool) {

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

Impact

Custom errors are available from solidity version 0.8.4, it saves around 50 gas each time they are hit by avoiding having to allocate and store the revert string.

Findings:

2022-08-fiatdao/contracts/VotingEscrow.sol:L116        require(decimals <= 18, "Exceeds max decimals");

2022-08-fiatdao/contracts/VotingEscrow.sol:L140        require(msg.sender == owner, "Only owner");

2022-08-fiatdao/contracts/VotingEscrow.sol:L147        require(msg.sender == owner, "Only owner");

2022-08-fiatdao/contracts/VotingEscrow.sol:L154        require(msg.sender == owner, "Only owner");

2022-08-fiatdao/contracts/VotingEscrow.sol:L162        require(msg.sender == owner, "Only owner");

2022-08-fiatdao/contracts/VotingEscrow.sol:L171        require(msg.sender == blocklist, "Only Blocklist");

2022-08-fiatdao/contracts/VotingEscrow.sol:L412        require(_value > 0, "Only non zero amount");

2022-08-fiatdao/contracts/VotingEscrow.sol:L413        require(locked_.amount == 0, "Lock exists");

2022-08-fiatdao/contracts/VotingEscrow.sol:L414        require(unlock_time >= locked_.end, "Only increase lock end"); // from using quitLock, user should increaseAmount instead

2022-08-fiatdao/contracts/VotingEscrow.sol:L415        require(unlock_time > block.timestamp, "Only future lock end");

2022-08-fiatdao/contracts/VotingEscrow.sol:L416        require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

2022-08-fiatdao/contracts/VotingEscrow.sol:L448        require(_value > 0, "Only non zero amount");

2022-08-fiatdao/contracts/VotingEscrow.sol:L449        require(locked_.amount > 0, "No lock");

2022-08-fiatdao/contracts/VotingEscrow.sol:L450        require(locked_.end > block.timestamp, "Lock expired");

2022-08-fiatdao/contracts/VotingEscrow.sol:L469            require(locked_.amount > 0, "Delegatee has no lock");

2022-08-fiatdao/contracts/VotingEscrow.sol:L470            require(locked_.end > block.timestamp, "Delegatee lock expired");

2022-08-fiatdao/contracts/VotingEscrow.sol:L502        require(locked_.amount > 0, "No lock");

2022-08-fiatdao/contracts/VotingEscrow.sol:L503        require(unlock_time > locked_.end, "Only increase lock end");

2022-08-fiatdao/contracts/VotingEscrow.sol:L504        require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

2022-08-fiatdao/contracts/VotingEscrow.sol:L511            require(oldUnlockTime > block.timestamp, "Lock expired");

2022-08-fiatdao/contracts/VotingEscrow.sol:L529        require(locked_.amount > 0, "No lock");

2022-08-fiatdao/contracts/VotingEscrow.sol:L530        require(locked_.end <= block.timestamp, "Lock not expired");

2022-08-fiatdao/contracts/VotingEscrow.sol:L531        require(locked_.delegatee == msg.sender, "Lock delegated");

2022-08-fiatdao/contracts/VotingEscrow.sol:L546        require(token.transfer(msg.sender, value), "Transfer failed");

2022-08-fiatdao/contracts/VotingEscrow.sol:L563        require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");

2022-08-fiatdao/contracts/VotingEscrow.sol:L564        require(locked_.amount > 0, "No lock");

2022-08-fiatdao/contracts/VotingEscrow.sol:L565        require(locked_.delegatee != _addr, "Already delegated");

2022-08-fiatdao/contracts/VotingEscrow.sol:L587        require(toLocked.amount > 0, "Delegatee has no lock");

2022-08-fiatdao/contracts/VotingEscrow.sol:L588        require(toLocked.end > block.timestamp, "Delegatee lock expired");

2022-08-fiatdao/contracts/VotingEscrow.sol:L589        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");

2022-08-fiatdao/contracts/VotingEscrow.sol:L635        require(locked_.amount > 0, "No lock");

2022-08-fiatdao/contracts/VotingEscrow.sol:L636        require(locked_.end > block.timestamp, "Lock expired");

2022-08-fiatdao/contracts/VotingEscrow.sol:L637        require(locked_.delegatee == msg.sender, "Lock delegated");

2022-08-fiatdao/contracts/VotingEscrow.sol:L657        require(token.transfer(msg.sender, remainingAmount), "Transfer failed");

2022-08-fiatdao/contracts/VotingEscrow.sol:L676        require(token.transfer(penaltyRecipient, amount), "Transfer failed");

2022-08-fiatdao/contracts/VotingEscrow.sol:L776        require(_blockNumber <= block.number, "Only past block number");

2022-08-fiatdao/contracts/VotingEscrow.sol:L877        require(_blockNumber <= block.number, "Only past block number");

2022-08-fiatdao/contracts/features/Blocklist.sol:L24        require(msg.sender == manager, "Only manager");

2022-08-fiatdao/contracts/features/Blocklist.sol:L25        require(_isContract(addr), "Only contracts");

[G-02] Using > 0 costs more gas than != 0 when used on a uint in a require() statement.

Impact

When working with unsigned integer types, comparisons with != 0 are cheaper than with > 0 . This changes saves 6 gas per instance.

Findings:

2022-08-fiatdao/contracts/VotingEscrow.sol:L412        require(_value > 0, "Only non zero amount");

2022-08-fiatdao/contracts/VotingEscrow.sol:L448        require(_value > 0, "Only non zero amount");

2022-08-fiatdao/contracts/VotingEscrow.sol:L449        require(locked_.amount > 0, "No lock");

2022-08-fiatdao/contracts/VotingEscrow.sol:L469            require(locked_.amount > 0, "Delegatee has no lock");

2022-08-fiatdao/contracts/VotingEscrow.sol:L502        require(locked_.amount > 0, "No lock");

2022-08-fiatdao/contracts/VotingEscrow.sol:L529        require(locked_.amount > 0, "No lock");

2022-08-fiatdao/contracts/VotingEscrow.sol:L564        require(locked_.amount > 0, "No lock");

2022-08-fiatdao/contracts/VotingEscrow.sol:L587        require(toLocked.amount > 0, "Delegatee has no lock");

2022-08-fiatdao/contracts/VotingEscrow.sol:L635        require(locked_.amount > 0, "No lock");

[G-03] Use a more recent version of solidity.

Impact

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.

Findings:

2022-08-fiatdao/contracts/VotingEscrow.sol:L2      pragma solidity ^0.8.3;

2022-08-fiatdao/contracts/interfaces/IBlocklist.sol:L2      pragma solidity ^0.8.3;

2022-08-fiatdao/contracts/interfaces/IERC20.sol:L2      pragma solidity ^0.8.3;

2022-08-fiatdao/contracts/interfaces/IVotingEscrow.sol:L2      pragma solidity ^0.8.3;

2022-08-fiatdao/contracts/features/Blocklist.sol:L2      pragma solidity ^0.8.3;

[G-04] ++i costs less gas than i++, especially when it's used in for loops.

Impact

Saves 6 gas per loop.

Findings:

2022-08-fiatdao/contracts/VotingEscrow.sol:L309        for (uint256 i = 0; i < 255; i++) {

2022-08-fiatdao/contracts/VotingEscrow.sol:L717        for (uint256 i = 0; i < 128; i++) {

2022-08-fiatdao/contracts/VotingEscrow.sol:L739        for (uint256 i = 0; i < 128; i++) {

2022-08-fiatdao/contracts/VotingEscrow.sol:L834        for (uint256 i = 0; i < 255; i++) {

[G-05] Using private rather than public for constants to saves gas.

Impact

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.

Findings:

2022-08-fiatdao/contracts/VotingEscrow.sol:L46    uint256 public constant WEEK = 7 days;

2022-08-fiatdao/contracts/VotingEscrow.sol:L47    uint256 public constant MAXTIME = 365 days;

2022-08-fiatdao/contracts/VotingEscrow.sol:L48    uint256 public constant MULTIPLIER = 10**18;

[G-06] Explicit initialization with zero not required

Impact

Explicit initialization with zero is not required for variable declaration because uints are 0 by default. Removing this will reduce contract size and save a bit of gas.

Findings:

2022-08-fiatdao/contracts/VotingEscrow.sol:L298        uint256 blockSlope = 0; // dblock/dt

2022-08-fiatdao/contracts/VotingEscrow.sol:L309        for (uint256 i = 0; i < 255; i++) {

2022-08-fiatdao/contracts/VotingEscrow.sol:L714        uint256 min = 0;

2022-08-fiatdao/contracts/VotingEscrow.sol:L717        for (uint256 i = 0; i < 128; i++) {

2022-08-fiatdao/contracts/VotingEscrow.sol:L737        uint256 min = 0;

2022-08-fiatdao/contracts/VotingEscrow.sol:L739        for (uint256 i = 0; i < 128; i++) {

2022-08-fiatdao/contracts/VotingEscrow.sol:L793        uint256 dBlock = 0;

2022-08-fiatdao/contracts/VotingEscrow.sol:L794        uint256 dTime = 0;

2022-08-fiatdao/contracts/VotingEscrow.sol:L834        for (uint256 i = 0; i < 255; i++) {

2022-08-fiatdao/contracts/VotingEscrow.sol:L889        uint256 dTime = 0;

[G-07] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops.

Impact

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop.

Findings:

2022-08-fiatdao/contracts/VotingEscrow.sol:L309        for (uint256 i = 0; i < 255; i++) {

2022-08-fiatdao/contracts/VotingEscrow.sol:L717        for (uint256 i = 0; i < 128; i++) {

2022-08-fiatdao/contracts/VotingEscrow.sol:L739        for (uint256 i = 0; i < 128; i++) {

2022-08-fiatdao/contracts/VotingEscrow.sol:L834        for (uint256 i = 0; i < 255; i++) {

[G-08] X += Y costs more gas than X = X + Y for state variables.

Impact

Consider changing X += Y to X = X + Y to save gas.

Findings:

2022-08-fiatdao/contracts/VotingEscrow.sol:L418        locked_.amount += int128(int256(_value));

2022-08-fiatdao/contracts/VotingEscrow.sol:L420        locked_.delegated += int128(int256(_value));

2022-08-fiatdao/contracts/VotingEscrow.sol:L460            newLocked.amount += int128(int256(_value));

2022-08-fiatdao/contracts/VotingEscrow.sol:L461            newLocked.delegated += int128(int256(_value));

2022-08-fiatdao/contracts/VotingEscrow.sol:L465            locked_.amount += int128(int256(_value));

2022-08-fiatdao/contracts/VotingEscrow.sol:L472            newLocked.delegated += int128(int256(_value));

2022-08-fiatdao/contracts/VotingEscrow.sol:L603            newLocked.delegated += value;

2022-08-fiatdao/contracts/VotingEscrow.sol:L654        penaltyAccumulated += penaltyAmount;

[G-09] Using bools for storage incurs overhead.

Impact

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

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 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

Findings:

2022-08-fiatdao/contracts/features/Blocklist.sol:L10    mapping(address => bool) private _blocklist;

[G-10] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead.

Impact

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. https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed.

Findings:

2022-08-fiatdao/contracts/interfaces/IERC20.sol:L10    function decimals() external view returns (uint8);