FIAT DAO veFDT contest - m_Rassska's results

Table of contents

• [0x0] Disclaimer ✅
• **[G-01] Try ++i instead of i++**✅
• [G-02] Try unchecked{++i} instead of i++ in loops ✅
• [G-03] Consider a = a + b instead of a += b ✅
• [G-04] Consider marking onlyOwner functions as payable ✅
• [G-05] Use binary shifting instead of a / 2^x, x > 0 ✅
• [G-06] Cache state variables, MLOAD << SLOAD ✅
• [G-07] Internal functions can be inlined ✅
• [G-08] Use private/internal for constants/immutable variables instead of public ✅
• [G-10] Define modifier/function to cover the same require statements ✅
• [G-11] Use precomputed values instead of compile-time computations ✅
• [G-12] Use uint instead of uint8, uint64, if it's possible ✅
• [G-13] Mark functions as external instead of public, if there are no internal calls ✅
• [G-14] Use calldataload instead of mload ✅
• [G-15] Define state variables as immutable/const ✅
• [G-16] Consider custom errors instead of strings ✅
• [G-17] Do not initialize state variables by zero ✅
• [G-18] Try increment/decrement instead of +1/-1 ✅

Disclaimer<a name="0x0"></a>

• Please, consider everything described below as a general recommendation. I'll not include every single occurance, but instead will point out the files to look for other findings of the same type.
• These notes will represent potential possibilities to optimize gas consumption. It's okay, if something is not suitable in your case. Just let me know the reason in the comments section. Enjoy!

[G-01] Try ++i instead of i++<a name="G-01"></a>

Description:

• In case of i++, the compiler has to to create a temp variable to return i (if there's a need) and then i gets incremented.
• In case of ++i, the compiler just simply returns already incremented value.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
    
      // Lines: [309-309]
       for (uint256 i = 0; i < 255; i++) {}
  
      // Lines: [717-717]
        for (uint256 i = 0; i < 128; i++) {}
  
      // Lines: [739-739]
        for (uint256 i = 0; i < 128; i++) {}
  
      // Lines: [834-834]
        for (uint256 i = 0; i < 255; i++) {}
  

• Also occured in the following files:

[G-02] Try unchecked{++i}; instead of i++;/++i; in loops<a name="G-02"></a>

Description:

• Since Solidity 0.8.0, all arithmetic operations revert on over- and underflow by default Therefore, unchecked box can be used to prevent all unnecessary checks, if it's no a way to get a reversion.

• There are ~1e80 atoms in the universe, so 2^256 is closed to that number, therefore it's no a way to be overflowed, because of the gas limit as well.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
    
      // Lines: [309-309]
       for (uint256 i = 0; i < 255; i++) {}
  
      // Lines: [717-717]
        for (uint256 i = 0; i < 128; i++) {}
  
      // Lines: [739-739]
        for (uint256 i = 0; i < 128; i++) {}
  
      // Lines: [834-834]
        for (uint256 i = 0; i < 255; i++) {}
  

• Also occured in the following files:

[G-03] Consider a = a + b instead of a += b<a name="G-03"></a>

Description:

• It has an impact on the deployment cost and the cost for distinct transaction as well.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
    
      // Lines: [418-418]
        locked_.amount += int128(int256(_value));
  
      // Lines: [420-420]
        locked_.delegated += int128(int256(_value));
  
      // Lines: [460-461]
          newLocked.amount += int128(int256(_value));
          newLocked.delegated += int128(int256(_value));
  
      // Lines: [465-465]
          locked_.amount += int128(int256(_value));
  
      // Lines: [472-472]
          newLocked.delegated += int128(int256(_value));
  
      // Lines: [603-603]
          newLocked.delegated += value;
  
      // Lines: [654-654]
          penaltyAccumulated += penaltyAmount;
  
      // Lines: [537-537]
          newLocked.delegated -= int128(int256(value));
  
      // Lines: [612-612]
          newLocked.delegated -= value;
  
      // Lines: [642-642]
          newLocked.delegated -= int128(int256(value));

• Also occured in the following files:

[G-04] Consider marking onlyOwner functions as payable<a name="G-04"></a>

Description:

• This one is a bit questionable, but you can try that out. So, the compiler adds some extra conditions in case of non-payable, but we know that onlyOwner modifier will be reverted, if the user who is not an owner invokes following methods.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
    
      // Lines: [139-139]
        function transferOwnership(address _addr) external {}
  
      // Lines: [146-146]
        function updateBlocklist(address _addr) external {}
  
      // Lines: [153-153]
        function updatePenaltyRecipient(address _addr) external {}
  
      // Lines: [161-161]
        function unlock() external {}
  
      ```

• Also occured in the following files:

[G-05] Use binary shifting instead of a / 2^x, x > 0 or a * 2^x, x > 0<a name="G-05"></a>

Description:

• It's also pretty impactful one, especially in loops.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
    
      // Lines: [719-719]
        uint256 mid = (min + max + 1) / 2; 
  
      // Lines: [743-743]
        uint256 mid = (min + max + 1) / 2;
  
    ```
    

[G-06] Cache state variables, MLOAD << SLOAD<a name="G-06"></a>

Description:

• MLOAD costs only 3 units of gas, SLOAD(warm access) is about 100 units. Therefore, cache, when it's possible.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
  
    // penaltyRecipient could be cached into memory to avoid warm access on state var. 
      // Lines: [673-678]
          function collectPenalty() external {
            uint256 amount = penaltyAccumulated;
            penaltyAccumulated = 0;
            require(token.transfer(penaltyRecipient, amount), "Transfer failed");
            emit CollectPenalty(amount, penaltyRecipient);
        } 
  
      ```
  

[G-07] Internal functions can be inlined<a name="G-07"></a>

Description:

• It takes some extra JUMPs to find a function and also the gas consumption for pushing arguments into memory and etc.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
    
      // Lines: [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;
        }
  
      // Lines: [685-697]
          function _copyLock(LockedBalance memory _locked)
            internal
            pure
            returns (LockedBalance memory)
        {
            return
                LockedBalance({
                    amount: _locked.amount,
                    end: _locked.end,
                    delegatee: _locked.delegatee,
                    delegated: _locked.delegated
                });
        }
  
      // Lines: [701-703]
          function _floorToWeek(uint256 _t) internal pure returns (uint256) {
            return (_t / WEEK) * WEEK;
          }
  
      // Lines: [662-669]
  

[G-08] Use private/internal for constants/immutable variables instead of public<a name="G-08"></a>

Description:

• If there is no need in state/constants/immutable variables outside the contract, it's better to mark them as internal/private. Optimization comes from not creating a getter function for each public instance.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
    
      // Lines: [45-66]
  
        IERC20 public token;
        uint256 public constant WEEK = 7 days;
        uint256 public constant MAXTIME = 365 days;
        uint256 public constant MULTIPLIER = 10**18;
        address public owner;
        address public penaltyRecipient; // receives collected penalty payments
        uint256 public maxPenalty = 10**18; // penalty for quitters with MAXTIME remaining lock
        uint256 public penaltyAccumulated; // accumulated and unwithdrawn penalty payments
        address public blocklist;
  
        // Lock state
        uint256 public globalEpoch;
        Point[1000000000000000000] public pointHistory; // 1e9 * userPointHistory-length, so sufficient for 1e9 users
        mapping(address => Point[1000000000]) public userPointHistory;
        mapping(address => uint256) public userPointEpoch;
        mapping(uint256 => int128) public slopeChanges;
        mapping(address => LockedBalance) public locked;
  
        // Voting token
        string public name;
        string public symbol;
        uint256 public decimals = 18;

• Also occured in the following files:

[G-10] Define modifier/function to cover the same require statements<a name="G-10"></a>

Description:

• Optimization relies on the deployemnt side.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
  
      // Lines: [140; 147; 154; 162]
        require(msg.sender == owner, "Only owner");
  
      // Lines: [412; 448]
        require(_value > 0, "Only non zero amount");
  

[G-11] Use precomputed values instead of run-time computations<a name="G-11"></a>

Description:

• Not sure about readability, but it saves some gas.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
    
      // Lines: [653-653]
        uint256 penaltyAmount = (value * penaltyRate) / 10**18; // quitlock_penalty is in 18 decimals precision
  
    // Update to:
      // Lines: [653-653]
        uint256 penaltyAmount = (value * penaltyRate) / 0xDE0B6B3A7640000; // quitlock_penalty is in 18 decimals precision
  
      // Lines: [48-48]
        uint256 public constant MULTIPLIER = 10**18;
  
      // Lines: [51-51]
        uint256 public maxPenalty = 10**18; // penalty for quitters with MAXTIME remaining lock

• Also occured in the following files:

[G-12] Use uint instead of uint8, uint64, if it's possible<a name="G-12"></a>

Description:

• I deployed a contract with only a single error and function and compared between:
  • error VaultAlreadyUnderAuction(bytes12 vaultId, address witch);.
  • error VaultAlreadyUnderAuction(bytes32 vaultId, address witch);.
• The second one is saving about ~10k units of gas for deployment and 10k per each transaction.
• However, defaining further errors with !bytes32 doesn't give a huge optimization.
• The stack slots are 32bytes, just use 32bytes, if you are not trying to tight up the storage.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
    
      // Lines: [69-74]
        struct Point {
            int128 bias;
            int128 slope;
            uint256 ts;
            uint256 blk;
        }
    
    // Update to:
  
      // Lines: [69-74]
        struct Point {
            int bias;
            int slope;
            uint256 ts;
            uint256 blk;
        }
      // Lines: [75-80]
        struct LockedBalance {
          int128 amount;
          uint256 end;
          int128 delegated;
          address delegatee;
        }
  
      // Lines: [60-60]
          mapping(uint256 => int128) public slopeChanges;
  
      // Lines: [174-174]
          int128 value = locked_.amount;
  
     // And etc, you've got the point... 
  
    ```
  

• Also occured in the following files:

[G-14] Use calldataload instead of mload<a name="G-14"></a>

Description:

• After Berlin hard fork, to load a non-zero byte from calldata dropped from 64 units of gas to 16 units, therefore if you do not modify args, use a calldata instead of memory. Here you need to explicitly specify calldataload, or replace memory with calldata. If the args are pretty huge, allocating args in memory will cost a lot.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
  
      // Lines: [100-106]
        constructor(
            address _owner,
            address _penaltyRecipient,
            address _token,
            string memory _name,
            string memory _symbol
        ) {} 
  
    // Update to:
      // Lines: [100-106]
        constructor(
            address _owner,
            address _penaltyRecipient,
            address _token,
            string calldata _name,
            string calldata  _symbol
        ) {} 
  
      // Lines: [222-226]
          function _checkpoint(
            address _addr,
            LockedBalance memory _oldLocked,
            LockedBalance memory _newLocked
        ) internal {}
  
      // Lines: [595-600]
          function _delegate(
            address addr,
            LockedBalance memory _locked,
            int128 value,
            LockAction action
        ) internal {}
  
      // Lines: [685-689]
          function _copyLock(LockedBalance memory _locked)
            internal
            pure
            returns (LockedBalance memory)
        {}
  
      // Lines: [825-829]
          function _supplyAt(Point memory _point, uint256 _t)
            internal
            view
            returns (uint256)
        {}
  

• Also occured in the following files:

[G-15] Define state variables as immutable/const<a name="G-15"></a>

Description:

• The opcode SSTORE_SET_GAS after Berlin hard fork costs around 22100 units of gas, therefore to get a huge optimization it's possible to define the following state variables as immutable/const.
• Also, in the future, there is no need in cold/warm(2200/100 gas for accessing) SLOADs, since they are consts/immutable variables, which also saves sagnificant amount of gas.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
    
      // Lines: [45-45]
        IERC20 public token; // could be defined as immutable. 
  
      // Lines: [64-66]
        string public name;
        string public symbol;
        uint256 public decimals = 18;
  
    ```
  

• Also occured in the following files:

[G-16] Consider custom errors instead of strings<a name="G-16"></a>

Description:

• After the solidity 0.8.4, the custom errors are available. Consider adding them to avoid long reversion messages in order to optimize the gas consumtion.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
    
      // Lines: [140-140]
        require(msg.sender == owner, "Only owner");
  
      // Lines: [171-171]
        require(msg.sender == blocklist, "Only Blocklist");
  
      // And etc, you've got the point...
  
    ```
  

• Also occured in the following files:

[G-18] Try increment/decrement instead of +1/-1<a name="G-18"></a>

Description:

• It's sagnificant gas optimization, if you look for every single occurance. Be careful with post/pre increment/decrements.

All occurances:

• Contracts:

    file: contracts/VotingEscrow.sol
    .................................
  
      // Lines: [723-723]
        // Original 
              max = mid - 1;
  
        // Updated 
              max = mid--;
  
        // Updated 
              --mid;
              max = mid;
  
      // Lines: [747-747]
        // Original 
              max = mid - 1;
  
        // Updated 
              max = mid--;
  
        // Updated 
              --mid;
              max = mid;
  

Kudos for the quality of the code! It's pretty easy to explore