code-423n4 / 2022-08-fiatdao-findings

2 stars 1 forks source link

Gas Optimizations #223

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Summary

Gas Optimizations

Issue Instances
[G‑01] Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate 1
[G‑02] State variables only set in the constructor should be declared immutable 14
[G‑03] Structs can be packed into fewer storage slots 1
[G‑04] Using calldata instead of memory for read-only arguments in external functions saves gas 2
[G‑05] Using storage instead of memory for structs/arrays saves gas 9
[G‑06] Avoid contract existence checks by using solidity version 0.8.10 or later 3
[G‑07] State variables should be cached in stack variables rather than re-reading them from storage 3
[G‑08] <x> += <y> costs more gas than <x> = <x> + <y> for state variables 1
[G‑09] internal functions only called once can be inlined to save gas 3
[G‑10] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement 2
[G‑11] ++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 4
[G‑12] Optimize names to save gas 4
[G‑13] Using bools for storage incurs overhead 1
[G‑14] Use a more recent version of solidity 5
[G‑15] Using > 0 costs more gas than != 0 when used on a uint in a require() statement 2
[G‑16] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) 4
[G‑17] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead 3
[G‑18] Using private rather than public for constants, saves gas 3
[G‑19] Division by two should use bit shifting 2
[G‑20] Stack variable used as a cheaper cache for a state variable is only used once 1
[G‑21] require() or revert() statements that check input arguments should be at the top of the function 2
[G‑22] Superfluous event fields 2
[G‑23] Use custom errors rather than revert()/require() strings to save gas 42

Total: 114 instances over 23 issues

Gas Optimizations

[G‑01] Multiple address/ID mappings can be combined into a single mapping of an address/ID 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 operations.

There is 1 instance of this issue:

File: contracts/VotingEscrow.sol

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

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L58-L59

[G‑02] State variables only set in the constructor should be declared immutable

Avoids a Gsset (20000 gas) in the constructor, and replaces the first access in each transaction (Gcoldsload - 2100 gas) and each access thereafter (Gwarmacces - 100 gas) with a PUSH32 (3 gas).

There are 14 instances of this issue:

File: contracts/features/Blocklist.sol

/// @audit manager (constructor)
15:           manager = _manager;

/// @audit manager (access)
24:           require(msg.sender == manager, "Only manager");

/// @audit ve (constructor)
16:           ve = _ve;

/// @audit ve (access)
27:           IVotingEscrow(ve).forceUndelegate(addr);

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L15

File: contracts/VotingEscrow.sol

/// @audit token (constructor)
107:          token = IERC20(_token);

/// @audit token (access)
426:              token.transferFrom(msg.sender, address(this), _value),

/// @audit token (access)
486:              token.transferFrom(msg.sender, address(this), _value),

/// @audit token (access)
546:          require(token.transfer(msg.sender, value), "Transfer failed");

/// @audit token (access)
657:          require(token.transfer(msg.sender, remainingAmount), "Transfer failed");

/// @audit token (access)
676:          require(token.transfer(penaltyRecipient, amount), "Transfer failed");

/// @audit name (constructor)
118:          name = _name;

/// @audit symbol (constructor)
119:          symbol = _symbol;

/// @audit decimals (constructor)
115:          decimals = IERC20(_token).decimals();

/// @audit decimals (access)
116:          require(decimals <= 18, "Exceeds max decimals");

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L107

diff --git a/contracts/VotingEscrow.sol b/contracts/VotingEscrow.sol
index f15781a..d2c7666 100644
--- a/contracts/VotingEscrow.sol
+++ b/contracts/VotingEscrow.sol
@@ -42,7 +42,7 @@ contract VotingEscrow is IVotingEscrow, ReentrancyGuard {
     event Unlock();

     // Shared global state
-    IERC20 public token;
+    IERC20 public immutable token;
     uint256 public constant WEEK = 7 days;
     uint256 public constant MAXTIME = 365 days;
     uint256 public constant MULTIPLIER = 10**18;
@@ -61,9 +61,9 @@ contract VotingEscrow is IVotingEscrow, ReentrancyGuard {
     mapping(address => LockedBalance) public locked;

     // Voting token
-    string public name;
-    string public symbol;
-    uint256 public decimals = 18;
+    string public constant name = "veFDT";
+    string public constant symbol = "veFDT";
+    uint256 public immutable decimals;

     // Structs
     struct Point {
@@ -112,11 +112,10 @@ contract VotingEscrow is IVotingEscrow, ReentrancyGuard {
             blk: block.number
         });

-        decimals = IERC20(_token).decimals();
-        require(decimals <= 18, "Exceeds max decimals");
+        uint256 _dec = IERC20(_token).decimals();
+        require(_dec <= 18, "Exceeds max decimals");
+        decimals = _dec;

-        name = _name;
-        symbol = _symbol;
         owner = _owner;
         penaltyRecipient = _penaltyRecipient;
     }
diff --git a/contracts/features/Blocklist.sol b/contracts/features/Blocklist.sol
index 27db9b0..ca3a226 100644
--- a/contracts/features/Blocklist.sol
+++ b/contracts/features/Blocklist.sol
@@ -8,8 +8,8 @@ import { IVotingEscrow } from "../interfaces/IVotingEscrow.sol";
 /// @dev This is a basic implementation using a mapping for address => bool
 contract Blocklist {
     mapping(address => bool) private _blocklist;
-    address public manager;
-    address public ve;
+    address public immutable manager;
+    address public immutable ve;

     constructor(address _manager, address _ve) {
         manager = _manager;
diff --git a/tmp/gas_before b/tmp/gas_after
index 3deb415..cf71599 100644
--- a/tmp/gas_before
+++ b/tmp/gas_after
@@ -167,7 +167,7 @@ No need to generate any newer typings.
 ····················|······················|·············|·············|···············|···············|··············
 |  Contract         ·  Method              ·  Min        ·  Max        ·  Avg          ·  # calls      ·  eur (avg)  │
 ····················|······················|·············|·············|···············|···············|··············
-|  Blocklist        ·  block               ·      45000  ·     409628  ·       198815  ·            5  ·          -  │
+|  Blocklist        ·  block               ·      40797  ·     405425  ·       194612  ·            5  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
 |  MockERC20        ·  approve             ·      46176  ·      46200  ·        46196  ·           96  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
@@ -177,46 +177,46 @@ No need to generate any newer typings.
 ····················|······················|·············|·············|···············|···············|··············
 |  MockERC20        ·  transfer            ·      51588  ·      51612  ·        51606  ·           83  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  MockSmartWallet  ·  createLock          ·     334002  ·     378450  ·       355494  ·            7  ·          -  │
+|  MockSmartWallet  ·  createLock          ·     331896  ·     376344  ·       353388  ·            7  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
 |  MockSmartWallet  ·  delegate            ·          -  ·          -  ·       340388  ·            2  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
 |  MockSmartWallet  ·  increaseUnlockTime  ·          -  ·          -  ·       208859  ·            1  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  MockSmartWallet  ·  quitLock            ·     131158  ·     256210  ·       201886  ·            4  ·          -  │
+|  MockSmartWallet  ·  quitLock            ·     129058  ·     254110  ·       199786  ·            4  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  MockSmartWallet  ·  withdraw            ·          -  ·          -  ·       666280  ·            1  ·          -  │
+|  MockSmartWallet  ·  withdraw            ·          -  ·          -  ·       664174  ·            1  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
 |  VotingEscrow     ·  checkpoint          ·      82307  ·    3715511  ·      1272491  ·           10  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  VotingEscrow     ·  collectPenalty      ·          -  ·          -  ·        49948  ·            1  ·          -  │
+|  VotingEscrow     ·  collectPenalty      ·          -  ·          -  ·        47863  ·            1  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  VotingEscrow     ·  createLock          ·     293060  ·    3978208  ·       414348  ·           60  ·          -  │
+|  VotingEscrow     ·  createLock          ·     290954  ·    3976102  ·       412242  ·           60  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
 |  VotingEscrow     ·  delegate            ·     246709  ·    4048411  ·       650225  ·           33  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  VotingEscrow     ·  increaseAmount      ·     234777  ·    1077801  ·       448554  ·            4  ·          -  │
+|  VotingEscrow     ·  increaseAmount      ·     232671  ·    1075695  ·       446448  ·            4  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
 |  VotingEscrow     ·  increaseUnlockTime  ·      46794  ·     416024  ·       143186  ·           23  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  VotingEscrow     ·  quitLock            ·     127639  ·    1605731  ·       375486  ·           13  ·          -  │
+|  VotingEscrow     ·  quitLock            ·     125539  ·    1603631  ·       373386  ·           13  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
 |  VotingEscrow     ·  unlock              ·          -  ·          -  ·        14596  ·            3  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
 |  VotingEscrow     ·  updateBlocklist     ·          -  ·          -  ·        47186  ·           14  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  VotingEscrow     ·  withdraw            ·     106462  ·    3752666  ·       610141  ·           33  ·          -  │
+|  VotingEscrow     ·  withdraw            ·     104356  ·    3750560  ·       608035  ·           33  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
 |  Deployments                             ·                                           ·  % of limit   ·             │
 ···········································|·············|·············|···············|···············|··············
-|  Blocklist                               ·     278212  ·     278236  ·       278231  ·        2.2 %  ·          -  │
+|  Blocklist                               ·     248613  ·     248637  ·       248632  ·          2 %  ·          -  │
 ···········································|·············|·············|···············|···············|··············
 |  MockERC20                               ·          -  ·          -  ·      1278169  ·       10.3 %  ·          -  │
 ···········································|·············|·············|···············|···············|··············
 |  MockSmartWallet                         ·          -  ·          -  ·       416156  ·        3.3 %  ·          -  │
 ···········································|·············|·············|···············|···············|··············
-|  VotingEscrow                            ·    4374338  ·    4374350  ·      4374350  ·       35.1 %  ·          -  │
+|  VotingEscrow                            ·    4280168  ·    4280180  ·      4280180  ·       34.4 %  ·          -  │
 ·------------------------------------------|-------------|-------------|---------------|---------------|-------------·

-  117 passing (48s)
+  117 passing (46s)

[G‑03] Structs can be packed into fewer storage slots

Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings

There is 1 instance of this issue:

File: contracts/VotingEscrow.sol

/// @audit Variable ordering with 3 slots instead of the current 4:
///           uint256(32):end, address(20):delegatee, int128(16):amount, int128(16):delegated
75        struct LockedBalance {
76            int128 amount;
77            uint256 end;
78            int128 delegated;
79            address delegatee;
80:       }

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L75-L80

diff --git a/contracts/VotingEscrow.sol b/contracts/VotingEscrow.sol
index f15781a..0318bc3 100644
--- a/contracts/VotingEscrow.sol
+++ b/contracts/VotingEscrow.sol
@@ -73,10 +73,10 @@ contract VotingEscrow is IVotingEscrow, ReentrancyGuard {
         uint256 blk;
     }
     struct LockedBalance {
-        int128 amount;
         uint256 end;
-        int128 delegated;
         address delegatee;
+        int128 amount;
+        int128 delegated;
     }

     // Miscellaneous
@@ -420,7 +420,7 @@ contract VotingEscrow is IVotingEscrow, ReentrancyGuard {
         locked_.delegated += int128(int256(_value));
         locked_.delegatee = msg.sender;
         locked[msg.sender] = locked_;
-        _checkpoint(msg.sender, LockedBalance(0, 0, 0, address(0)), locked_);
+        _checkpoint(msg.sender, LockedBalance({amount:0, end:0, delegated:0, delegatee:address(0)}), locked_);
         // Deposit locked tokens
         require(
             token.transferFrom(msg.sender, address(this), _value),
diff --git a/test/votingEscrowDelegationMathTest.ts b/test/votingEscrowDelegationMathTest.ts
index 5e43096..088fb9c 100644
--- a/test/votingEscrowDelegationMathTest.ts
+++ b/test/votingEscrowDelegationMathTest.ts
@@ -138,10 +138,10 @@ describe("VotingEscrow Delegation Math test", () => {
   };

   interface LockedBalance {
-    amount: BN;
     end: BN;
-    delegated: BN;
     delegatee: string;
+    amount: BN;
+    delegated: BN;
   }

   interface Point {
@@ -173,10 +173,10 @@ describe("VotingEscrow Delegation Math test", () => {
       epoch,
       userEpoch,
       userLocked: {
-        amount: locked[0],
-        end: locked[1],
-        delegated: locked[2],
-        delegatee: locked[3],
+        end: locked[0],
+        delegatee: locked[1],
+        amount: locked[2],
+        delegated: locked[3],
       },
       userLastPoint: {
         bias: userLastPoint[0],
diff --git a/test/votingEscrowGasTest.ts b/test/votingEscrowGasTest.ts
index d6d03fd..af94f35 100644
--- a/test/votingEscrowGasTest.ts
+++ b/test/votingEscrowGasTest.ts
@@ -174,10 +174,10 @@ describe("Gas usage tests", () => {
       epoch,
       userEpoch,
       userLocked: {
-        amount: locked[0],
-        end: locked[1],
-        delegated: locked[2],
-        delegatee: locked[3],
+        end: locked[0],
+        delegatee: locked[1],
+        amount: locked[2],
+        delegated: locked[3],
       },
       userLastPoint: {
         bias: userLastPoint[0],
diff --git a/test/votingEscrowMathTest.ts b/test/votingEscrowMathTest.ts
index 9d0b9b6..e084b30 100644
--- a/test/votingEscrowMathTest.ts
+++ b/test/votingEscrowMathTest.ts
@@ -207,8 +207,10 @@ describe("VotingEscrow Math test", () => {
   });

   interface LockedBalance {
-    amount: BN;
     end: BN;
+    delegatee: string;
+    amount: BN;
+    delegated: BN;
   }

   interface Point {
@@ -252,8 +254,10 @@ describe("VotingEscrow Math test", () => {
       //totalStaticWeight: await votingLockup.totalStaticWeight(),
       // userStaticWeight: await votingLockup.staticBalanceOf(sender.address),
       userLocked: {
-        amount: locked[0],
-        end: locked[1],
+        end: locked[0],
+        delegatee: locked[1],
+        amount: locked[2],
+        delegated: locked[3],
       },
       userLastPoint: {
         bias: userLastPoint[0],
diff --git a/tmp/gas_before b/tmp/gas_after
index 3deb415..09dd64b 100644
--- a/tmp/gas_before
+++ b/tmp/gas_after
@@ -167,7 +167,7 @@ No need to generate any newer typings.
 ····················|······················|·············|·············|···············|···············|··············
 |  Contract         ·  Method              ·  Min        ·  Max        ·  Avg          ·  # calls      ·  eur (avg)  │
 ····················|······················|·············|·············|···············|···············|··············
-|  Blocklist        ·  block               ·      45000  ·     409628  ·       198815  ·            5  ·          -  │
+|  Blocklist        ·  block               ·      42887  ·     387472  ·       188685  ·            5  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
 |  MockERC20        ·  approve             ·      46176  ·      46200  ·        46196  ·           96  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
@@ -177,35 +177,35 @@ No need to generate any newer typings.
 ····················|······················|·············|·············|···············|···············|··············
 |  MockERC20        ·  transfer            ·      51588  ·      51612  ·        51606  ·           83  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  MockSmartWallet  ·  createLock          ·     334002  ·     378450  ·       355494  ·            7  ·          -  │
+|  MockSmartWallet  ·  createLock          ·     311601  ·     356043  ·       333090  ·            7  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  MockSmartWallet  ·  delegate            ·          -  ·          -  ·       340388  ·            2  ·          -  │
+|  MockSmartWallet  ·  delegate            ·          -  ·          -  ·       350371  ·            2  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  MockSmartWallet  ·  increaseUnlockTime  ·          -  ·          -  ·       208859  ·            1  ·          -  │
+|  MockSmartWallet  ·  increaseUnlockTime  ·          -  ·          -  ·       206298  ·            1  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  MockSmartWallet  ·  quitLock            ·     131158  ·     256210  ·       201886  ·            4  ·          -  │
+|  MockSmartWallet  ·  quitLock            ·     140849  ·     265907  ·       211580  ·            4  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  MockSmartWallet  ·  withdraw            ·          -  ·          -  ·       666280  ·            1  ·          -  │
+|  MockSmartWallet  ·  withdraw            ·          -  ·          -  ·       676067  ·            1  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  VotingEscrow     ·  checkpoint          ·      82307  ·    3715511  ·      1272491  ·           10  ·          -  │
+|  VotingEscrow     ·  checkpoint          ·      82319  ·    3715835  ·      1272603  ·           10  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
 |  VotingEscrow     ·  collectPenalty      ·          -  ·          -  ·        49948  ·            1  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  VotingEscrow     ·  createLock          ·     293060  ·    3978208  ·       414348  ·           60  ·          -  │
+|  VotingEscrow     ·  createLock          ·     270653  ·    3956119  ·       391951  ·           60  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  VotingEscrow     ·  delegate            ·     246709  ·    4048411  ·       650225  ·           33  ·          -  │
+|  VotingEscrow     ·  delegate            ·     224688  ·    4026678  ·       646994  ·           33  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  VotingEscrow     ·  increaseAmount      ·     234777  ·    1077801  ·       448554  ·            4  ·          -  │
+|  VotingEscrow     ·  increaseAmount      ·     229422  ·    1072518  ·       443303  ·            4  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  VotingEscrow     ·  increaseUnlockTime  ·      46794  ·     416024  ·       143186  ·           23  ·          -  │
+|  VotingEscrow     ·  increaseUnlockTime  ·      44338  ·     413481  ·       140686  ·           23  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  VotingEscrow     ·  quitLock            ·     127639  ·    1605731  ·       375486  ·           13  ·          -  │
+|  VotingEscrow     ·  quitLock            ·     137330  ·    1615548  ·       385196  ·           13  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
 |  VotingEscrow     ·  unlock              ·          -  ·          -  ·        14596  ·            3  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
 |  VotingEscrow     ·  updateBlocklist     ·          -  ·          -  ·        47186  ·           14  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
-|  VotingEscrow     ·  withdraw            ·     106462  ·    3752666  ·       610141  ·           33  ·          -  │
+|  VotingEscrow     ·  withdraw            ·     116189  ·    3762705  ·       619911  ·           33  ·          -  │
 ····················|······················|·············|·············|···············|···············|··············
 |  Deployments                             ·                                           ·  % of limit   ·             │
 ···········································|·············|·············|···············|···············|··············
@@ -215,8 +215,8 @@ No need to generate any newer typings.
 ···········································|·············|·············|···············|···············|··············
 |  MockSmartWallet                         ·          -  ·          -  ·       416156  ·        3.3 %  ·          -  │
 ···········································|·············|·············|···············|···············|··············
-|  VotingEscrow                            ·    4374338  ·    4374350  ·      4374350  ·       35.1 %  ·          -  │
+|  VotingEscrow                            ·    4245313  ·    4245325  ·      4245325  ·       34.1 %  ·          -  │
 ·------------------------------------------|-------------|-------------|---------------|---------------|-------------·

-  117 passing (48s)
+  117 passing (44s)

[G‑04] Using calldata instead of memory for read-only arguments in external functions saves gas

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one

Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved

There are 2 instances of this issue:

File: contracts/VotingEscrow.sol

/// @audit _name
/// @audit _symbol
100       constructor(
101           address _owner,
102           address _penaltyRecipient,
103           address _token,
104           string memory _name,
105:          string memory _symbol

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L100-L105

[G‑05] Using storage instead of memory for structs/arrays saves gas

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct

There are 9 instances of this issue:

File: contracts/VotingEscrow.sol

410:          LockedBalance memory locked_ = locked[msg.sender];

446:          LockedBalance memory locked_ = locked[msg.sender];

499:          LockedBalance memory locked_ = locked[msg.sender];

527:          LockedBalance memory locked_ = locked[msg.sender];

561:          LockedBalance memory locked_ = locked[msg.sender];

633:          LockedBalance memory locked_ = locked[msg.sender];

788:          Point memory point0 = pointHistory[epoch];

866:          Point memory lastPoint = pointHistory[epoch_];

882:          Point memory point = pointHistory[targetEpoch];

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L410

[G‑06] Avoid contract existence checks by using solidity version 0.8.10 or later

Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 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

There are 3 instances of this issue:

File: contracts/VotingEscrow.sol

/// @audit decimals()
115:          decimals = IERC20(_token).decimals();

/// @audit isBlocked()
126:              !IBlocklist(blocklist).isBlocked(msg.sender),

/// @audit isBlocked()
563:          require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L115

[G‑07] State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

There are 3 instances of this issue:

File: contracts/VotingEscrow.sol

/// @audit penaltyRecipient on line 676
677:          emit CollectPenalty(amount, penaltyRecipient);

/// @audit pointHistory on line 788
796:              Point memory point1 = pointHistory[epoch + 1];

/// @audit pointHistory on line 882
891:              Point memory pointNext = pointHistory[targetEpoch + 1];

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L677

[G‑08] <x> += <y> costs more gas than <x> = <x> + <y> for state variables

Using the addition operator instead of plus-equals saves 113 gas

There is 1 instance of this issue:

File: contracts/VotingEscrow.sol

654:          penaltyAccumulated += penaltyAmount;

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L654

[G‑09] internal functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

There are 3 instances of this issue:

File: contracts/features/Blocklist.sol

37:       function _isContract(address addr) internal view returns (bool) {

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L37

File: contracts/VotingEscrow.sol

662       function _calculatePenaltyRate(uint256 end)
663           internal
664           view
665:          returns (uint256)

732       function _findUserBlockEpoch(address _addr, uint256 _block)
733           internal
734           view
735:          returns (uint256)

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L662-L665

[G‑10] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement

require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }

There are 2 instances of this issue:

File: contracts/VotingEscrow.sol

/// @audit if-condition on line 299
301:                  (MULTIPLIER * (block.number - lastPoint.blk)) /

/// @audit if-condition on line 299
302:                  (block.timestamp - lastPoint.ts);

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L301

[G‑11] ++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

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

There are 4 instances of this issue:

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

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L309

[G‑12] Optimize names to save gas

public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted

There are 4 instances of this issue:

File: contracts/features/Blocklist.sol

/// @audit block(), isBlocked()
9:    contract Blocklist {

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L9

File: contracts/interfaces/IBlocklist.sol

/// @audit isBlocked()
6:    interface IBlocklist {

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/interfaces/IBlocklist.sol#L6

File: contracts/interfaces/IVotingEscrow.sol

/// @audit createLock(), increaseAmount(), increaseUnlockTime(), withdraw(), quitLock(), balanceOfAt(), totalSupplyAt(), forceUndelegate()
4:    interface IVotingEscrow {

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/interfaces/IVotingEscrow.sol#L4

File: contracts/VotingEscrow.sol

/// @audit updateBlocklist(), updatePenaltyRecipient(), unlock(), lockEnd(), getLastUserPoint(), checkpoint(), collectPenalty()
23:   contract VotingEscrow is IVotingEscrow, ReentrancyGuard {

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L23

[G‑13] Using bools for storage incurs overhead

    // 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) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past

There is 1 instance of this issue:

File: contracts/features/Blocklist.sol

10:       mapping(address => bool) private _blocklist;

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L10

[G‑14] 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

There are 5 instances of this issue:

File: contracts/features/Blocklist.sol

2:    pragma solidity ^0.8.3;

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L2

File: contracts/interfaces/IBlocklist.sol

2:    pragma solidity ^0.8.3;

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/interfaces/IBlocklist.sol#L2

File: contracts/interfaces/IERC20.sol

2:    pragma solidity ^0.8.3;

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/interfaces/IERC20.sol#L2

File: contracts/interfaces/IVotingEscrow.sol

2:    pragma solidity ^0.8.3;

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/interfaces/IVotingEscrow.sol#L2

File: contracts/VotingEscrow.sol

2:    pragma solidity ^0.8.3;

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L2

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

This change saves 6 gas per instance. The optimization works until solidity version 0.8.13 where there is a regression in gas costs.

There are 2 instances of this issue:

File: contracts/VotingEscrow.sol

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

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

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L412

[G‑16] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

There are 4 instances of this issue:

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

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L309

[G‑17] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

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 Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed

There are 3 instances of this issue:

File: contracts/VotingEscrow.sol

/// @audit int128 oldSlopeDelta
380:                  oldSlopeDelta = oldSlopeDelta + userOldPoint.slope;

/// @audit int128 oldSlopeDelta
382:                      oldSlopeDelta = oldSlopeDelta - userNewPoint.slope; // It was a new deposit, not extension

/// @audit int128 newSlopeDelta
388:                      newSlopeDelta = newSlopeDelta - userNewPoint.slope; // old slope disappeared at this point

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L380

[G‑18] Using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

There are 3 instances of this issue:

File: contracts/VotingEscrow.sol

46:       uint256 public constant WEEK = 7 days;

47:       uint256 public constant MAXTIME = 365 days;

48:       uint256 public constant MULTIPLIER = 10**18;

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L46

[G‑19] Division by two should use bit shifting

<x> / 2 is the same as <x> >> 1. While the compiler uses the SHR opcode to accomplish both, the version that uses division incurs an overhead of 20 gas due to JUMPs to and from a compiler utility function that introduces checks which can be avoided by using unchecked {} around the division by two

There are 2 instances of this issue:

File: contracts/VotingEscrow.sol

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

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

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L719

[G‑20] Stack variable used as a cheaper cache for a state variable is only used once

If the variable is only accessed once, it's cheaper to use the state variable directly that one time, and save the 3 gas the extra stack assignment would spend

There is 1 instance of this issue:

File: contracts/VotingEscrow.sol

865:          uint256 epoch_ = globalEpoch;

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L865

[G‑21] require() or revert() statements that check input arguments should be at the top of the function

Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (**2100 gas***) in a function that may ultimately revert in the unhappy case.

There are 2 instances of this issue:

File: contracts/VotingEscrow.sol

/// @audit expensive op on line 410
412:          require(_value > 0, "Only non zero amount");

/// @audit expensive op on line 446
448:          require(_value > 0, "Only non zero amount");

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L412

[G‑22] Superfluous event fields

block.timestamp and block.number are added to event information by default so adding them manually wastes gas

There are 2 instances of this issue:

File: contracts/VotingEscrow.sol

30:           uint256 ts

36:           uint256 ts

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L30

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

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

There are 42 instances of this issue:

File: contracts/features/Blocklist.sol

24:           require(msg.sender == manager, "Only manager");

25:           require(_isContract(addr), "Only contracts");

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L24

File: contracts/VotingEscrow.sol

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"); // from using quitLock, user should increaseAmount instead

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

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L116

lacoop6tu commented 2 years ago

Good one