FIAT DAO veFDT contest - pfapostol's results

Summary

Low Risk Issues

Non-critical Issues

Total: 19 instances over 8 issues

Low Risk:

1. Missing checks for address(0x0) when assigning values to address state variable

   • contracts/VotingEscrow.sol:101-103,107,120-121,139,141,146,148,153,155,

   101     address _owner,
   102     address _penaltyRecipient,
   103     address _token,
   ...
   107     token = IERC20(_token);
   ...
   120     owner = _owner;
   121     penaltyRecipient = _penaltyRecipient;
   ...
   139     function transferOwnership(address _addr) external {
   ...
   141     owner = _addr;
   ...
   146     function updateBlocklist(address _addr) external {
   ...
   148     blocklist = _addr;
   ...
   153     function updatePenaltyRecipient(address _addr) external {
   ...
   155     penaltyRecipient = _addr;

   • contracts/features/Blocklist.sol:14-16

   14      constructor(address _manager, address _ve) {
   15      manager = _manager;
   16      ve = _ve;

Non Critical:

1. constants should be defined rather than using magic numbers

   using readable constants instead of hex/numeric literals

   • contracts/VotingEscrow.sol:57-58

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

2. Event is missing indexed fields

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

   • contracts/VotingEscrow.sol:25-41

   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      );
   38      event TransferOwnership(address owner);
   39      event UpdateBlocklist(address blocklist);
   40      event UpdatePenaltyRecipient(address recipient);
   41      event CollectPenalty(uint256 amount, address recipient);

3. Contracts implementing an interface should inherit from that interface

   Blocklist does not implement IBlocklist interface

   fix:

   import { IBlocklist } from "../interfaces/IBlocklist.sol";
   ...
   contract Blocklist is IBlocklist {
   ...
   function isBlocked(address addr) public view override returns (bool) {

4. Adding a return statement when the function defines a named return variable, is redundant

   • contracts/VotingEscrow.sol:204-208,215

   204     returns (
   205         int128 bias,
   206         int128 slope,
   207         uint256 ts
   208     )
   ...
   215     return (point.bias, point.slope, point.ts);

5. public functions not called by the contract should be declared external instead

   • contracts/VotingEscrow.sol:754,770,864,871

   754    function balanceOf(address _owner) public view override returns (uint256) {
   ...
   770    function balanceOfAt(address _owner, uint256 _blockNumber)
   ...
   864    function totalSupply() public view override returns (uint256) {
   ...
   871    function totalSupplyAt(uint256 _blockNumber)

   • contracts/features/Blocklist.sol:33

   33    function isBlocked(address addr) public view returns (bool) {

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

   • 2022-08-fiatdao/contracts/features/Blocklist.sol
   • 2022-08-fiatdao/contracts/VotingEscrow.sol

   2       pragma solidity ^0.8.3;

7. declaration shadows a builtin symbol.

   • contracts/features/Blocklist.sol:23-28

   23      function block(address addr) external {
   24          require(msg.sender == manager, "Only manager");
   25          require(_isContract(addr), "Only contracts");
   26          _blocklist[addr] = true;
   27          IVotingEscrow(ve).forceUndelegate(addr);
   28      }

8. Non-assembly method available

   If using solidity version >0.8.1, one can replace this by <address>.code.length. The same also works for other versions, but not as efficient.

   • contracts/features/Blocklist.sol:37-43

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

   fix:

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

Summary

Gas savings are estimated using existing tests and may vary depending on the implementation of the fix. I keep my version of the fix for each finding and can provide them if you need. Since the optimizer config is set to 10000 runs, some optimizations were excluded. At a lower level of optimization, additional fixes make sense.

Gas Optimizations

Total: 72 instances over 10 issues

1. Use Custom Errors instead of Revert/Require Strings to save Gas (42 instances)

   Deployment gas saved: 195612

   Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

   Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Additional gas is saved due to the lack of defining string. https://blog.soliditylang.org/2021/04/21/custom-errors/#errors-in-depth

   ---

   • contracts/VotingEscrow.sol:116,125-128,140,147,154,162,171,412-416,425-428,448-450,469-470,485-488,502-504,511,529-531,546,563-565,587-589,635-637,657,676,776,877

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

   24      require(msg.sender == manager, "Only manager");
   25      require(_isContract(addr), "Only contracts");

2. The same check can be combined into a modifier or a function (3 instances)

   Deployment Gas saved in total: 180175

   • contracts/VotingEscrow.sol:449,469,502,529,564,587,635,

   Deployment Gas Saved: 89936

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

   fix:

   function is_lock(int128 amount) internal pure {
       require(amount > 0, "No lock");
   }

   • contracts/VotingEscrow.sol:450,470,511,588,636,

   Deployment Gas saved: 57894

   450     require(locked_.end > block.timestamp, "Lock expired");
   ...
   470     require(locked_.end > block.timestamp, "Delegatee lock expired");
   ...
   511     require(oldUnlockTime > block.timestamp, "Lock expired");
   ...
   588     require(toLocked.end > block.timestamp, "Delegatee lock expired");
   ...
   636     require(locked_.end > block.timestamp, "Lock expired");

   fix:

   function is_expired(uint256 exp_time) internal view {
       require(exp_time > block.timestamp, "Lock expired");
   }

   • contracts/VotingEscrow.sol:414,416,503-504,

   Deployment Gas saved: 32345

   414     require(unlock_time >= locked_.end, "Only increase lock end");
   ...
   416     require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
   ...
   503     require(unlock_time > locked_.end, "Only increase lock end");
   504     require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

   fix:

   function is_valid_unlock_time(uint256 unlock_time, uint256 exp_time)
       internal
       view
   {
       require(unlock_time >= exp_time, "Only increase lock end");
       require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
   }

3. Avoid contract existence checks by using solidity version 0.8.10 or later

   Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath
   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
   
   Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (700 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value
   
   By changing version to 0.8.10 in contracts VotingEscrow.sol and Blocklist.sol
   

   Deployment gas saved: 128057 Method Call gas saved: 7684

4. Pack structure members in one slot

   Deployment Gas Saved: 102300 Method Call Gas Saved: 18294

   • contracts/VotingEscrow.sol:75-80

   75     struct LockedBalance {
   76          int128 amount;
   77          uint256 end;
   78          int128 delegated;
   79          address delegatee;
   80      }

   fix:

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

5. State variables only set in the constructor should be declared immutable

   Deployment Gas Saved: 29599 Method Call Gas Saved: 4203

   • contracts/features/Blocklist.sol:11-12

   11    address public manager;
   12    address public ve;

6. Increment and assignment optimization (4/15 instances)

   All gas calculation for this 4 optimization are cummulitive

   1. Post increment and preincrement:

      Deployment gas Saved: 1724

   2. x=x+1 more efficient :

      Deployment Gas Saved: 12329

   3. increment in loop can be unchecked

      Deployment Gas Saved : 21181

      Method call Gas SAved: 3420

   • contracts/VotingEscrow.sol:309,717,739,834

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

   4. It is also more efficient to use x = x + y instead of x +=

      Deployment Gas Saved : 28389

      Method call Gas SAved: 3572

   • contracts/VotingEscrow.sol:418,420,460-461,465,472,537,603,642,654,

   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));
   ...
   537     newLocked.delegated -= int128(int256(value));
   ...
   603     newLocked.delegated += value;
   ...
   643     newLocked.delegated -= int128(int256(value));
   ...
   654     penaltyAccumulated += penaltyAmount;

7. Using private rather than public for constants, saves gas

   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

   Deployment Gas Saved: 25197

   • contracts/VotingEscrow.sol:46-48

   46  uint256 public constant WEEK = 7 days;
   47  uint256 public constant MAXTIME = 365 days;
   48  uint256 public constant MULTIPLIER = 10**18;

8. Using bools for storage incurs overhead (1 instances)

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

   Gas Saved: 3894

   • contracts/features/Blocklist.sol:10

   10      mapping(address => bool) private _blocklist;

   fix:

   10      mapping(address => uint256) private _blocklist;

   plus сhanging all occurrences in the code

9. Division by two should use bit shifting

   Deploy Gas Saved: 3692

   <x> / 2 is the same as <x> >> 1. The DIV opcode costs 5 gas, whereas SHR only costs 3 gas. Especially in loops

   • contracts/VotingEscrow.sol:719,743

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

10. Do not recalculate the same expression

    Deployment Gas Saved: 3452 Method Call Gas Saved: 896

    • contracts/VotingEscrow.sol:258,261,264

    258      userPointHistory[_addr][uEpoch + 1] = userOldPoint;
    ...
    261      userPointEpoch[_addr] = uEpoch + 1;
    ...
    264      userPointHistory[_addr][uEpoch + 1] = userNewPoint;

    fix:

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