FIAT DAO veFDT contest - Metatron's results

[G-01] ++i costs less gas compared to i++

++i costs about 5 gas less per iteration compared to i++ for unsigned integer. This statement is true even with the optimizer enabled. Summarized my results where i is used in a loop, is unsigned integer, and you safely can be changed to ++i without changing any behavior,

I've found 4 locations:

contracts/VotingEscrow.sol:
  309          uint256 iterativeTime = _floorToWeek(lastCheckpoint);
  310:         for (uint256 i = 0; i < 255; i++) {
  311              // Hopefully it won't happen that this won't get used in 5 years!

  727          // Will be always enough for 128-bit numbers
  728:         for (uint256 i = 0; i < 128; i++) {
  729              if (min >= max) break;

  749          uint256 max = userPointEpoch[_addr];
  750:         for (uint256 i = 0; i < 128; i++) {
  751              if (min >= max) {

  844          // Iterate through all weeks between _point & _t to account for slope changes
  845:         for (uint256 i = 0; i < 255; i++) {
  846              iterativeTime = iterativeTime + WEEK;

---

[G-02] Use immutable for state variables that are only set in the constructor - to save gas

Save around 20,000 gas per uint256 variable (use 100+3 gas for immutable variable instead of 20000 for regular public)

I've found 3 locations:

contracts/VotingEscrow.sol:
   64      // Voting token
   65:     string public name;  //@audit G can be immutable with a small trick
   66:     string public symbol;  //@audit G can be immutable with a small trick
   67:     uint256 public decimals = 18;  //@audit G can be simply immutable
   68  

For short strings (<32 chars) we can do a small trick, like in the example below: https://gist.github.com/frangio/61497715c43b79e3e2d7bfab907b01c2#file-testshortstring-sol

---

[G-03] Using default values is cheaper than assignment

If a variable is not set/initialized, it is assumed to have the default value 0 for uint and int, and false for boolean. Explicitly initializing it with its default value is an anti-pattern and wastes gas. For example: uint8 i = 0; should be replaced with uint8 i;

I've found 14 locations:

contracts/VotingEscrow.sol:
  229          Point memory userNewPoint;
  230:         int128 oldSlopeDelta = 0;
  231:         int128 newSlopeDelta = 0;
  232          uint256 epoch = globalEpoch;
  
  298              Point({ bias: 0, slope: 0, ts: lastPoint.ts, blk: lastPoint.blk });
  299:         uint256 blockSlope = 0; // dblock/dt
  300          if (block.timestamp > lastPoint.ts) {

  309          uint256 iterativeTime = _floorToWeek(lastCheckpoint);
  310:         for (uint256 i = 0; i < 255; i++) {
  311              // Hopefully it won't happen that this won't get used in 5 years!

  313              iterativeTime = iterativeTime + WEEK;
  314:             int128 dSlope = 0;
  315              if (iterativeTime > block.timestamp) {

  724          // Binary search
  725:         uint256 min = 0;
  726          uint256 max = _maxEpoch;
  727          // Will be always enough for 128-bit numbers
  728:         for (uint256 i = 0; i < 128; i++) {
  729              if (min >= max) break;

  747      {
  748:         uint256 min = 0;
  749          uint256 max = userPointEpoch[_addr];
  750:         for (uint256 i = 0; i < 128; i++) {
  751              if (min >= max) {

  803          // the two points
  804:         uint256 dBlock = 0;
  805:         uint256 dTime = 0;
  806          if (epoch < maxEpoch) {

  844          // Iterate through all weeks between _point & _t to account for slope changes
  845:         for (uint256 i = 0; i < 255; i++) {
  846              iterativeTime = iterativeTime + WEEK;
  847:             int128 dSlope = 0;
  848              // If week end is after timestamp, then truncate & leave dSlope to 0

  899  
  900:         uint256 dTime = 0;
  901          if (targetEpoch < epoch) {

---

[G-04] != 0 is cheaper than > 0

!= 0 costs less gas compared to > 0 for unsigned integers even when optimizer enabled For non-negative >0 and != have exactly the same effect. ** saves 6 gas ** each

** NOTE: I've included only occurrences of uint, as there are also int (amount of LockedBalance, and some uses of value) **

I've found 4 locations in 2 files:

contracts/VotingEscrow.sol:
  288              });
  289:         if (epoch > 0) {
  290              lastPoint = pointHistory[epoch];

  412          // Validate inputs
  413:         require(_value > 0, "Only non zero amount");
  414          require(locked_.amount == 0, "Lock exists");

  448          // Validate inputs
  449:         require(_value > 0, "Only non zero amount");
  450         require(locked_.amount > 0, "No lock");

contracts/features/Blocklist.sol:
  41          }
  42:         return size > 0;
  43      }

---

[G-05] Custom errors save gas

NOTE: Solidity version declared is 0.8.3, but if considering upgrading to 0.8.4 and above - this finding is relevant and saves gas.

From solidity 0.8.4 and above, Custom errors from are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) Source: https://blog.soliditylang.org/2021/04/21/custom-errors/: Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

I've found 39 locations in 2 files:

contracts/VotingEscrow.sol:
  116          decimals = IERC20(_token).decimals();
  117:         require(decimals <= 18, "Exceeds max decimals");
  118  

  140      function transferOwnership(address _addr) external {  // @audit transfer ownership? search on reports
  141:         require(msg.sender == owner, "Only owner");
  142          owner = _addr;

  147      function updateBlocklist(address _addr) external {  // @audit can this be a problem on some level? where some blocked addresses are unblocked now
  148:         require(msg.sender == owner, "Only owner");
  149          blocklist = _addr;

  154      function updatePenaltyRecipient(address _addr) external {
  155:         require(msg.sender == owner, "Only owner");
  156          penaltyRecipient = _addr;

  162      function unlock() external {
  163:         require(msg.sender == owner, "Only owner");
  164          maxPenalty = 0;

  171      function forceUndelegate(address _addr) external override {  //@audit N this is actully not Owner function..
  172:         require(msg.sender == blocklist, "Only Blocklist");
  173          LockedBalance memory locked_ = locked[_addr];

  412          // Validate inputs
  413:         require(_value > 0, "Only non zero amount");
  414:         require(locked_.amount == 0, "Lock exists");
  415:         require(unlock_time >= locked_.end, "Only increase lock end"); // from using quitLock, user should increaseAmount instead
  416:         require(unlock_time > block.timestamp, "Only future lock end");
  417:         require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
  418          // Update lock and voting power (checkpoint)

  448          // Validate inputs
  449:         require(_value > 0, "Only non zero amount");
  450:         require(locked_.amount > 0, "No lock");
  451:         require(locked_.end > block.timestamp, "Lock expired");
  452          // Update lock

  469              locked_ = locked[delegatee];
  470:             require(locked_.amount > 0, "Delegatee has no lock");
  471:             require(locked_.end > block.timestamp, "Delegatee lock expired");
  472              newLocked = _copyLock(locked_);

  502          // Validate inputs
  503:         require(locked_.amount > 0, "No lock");
  504:         require(unlock_time > locked_.end, "Only increase lock end");
  505:         require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
  506          // Update lock

  511              // Undelegated lock
  512:             require(oldUnlockTime > block.timestamp, "Lock expired");
  513              LockedBalance memory oldLocked = _copyLock(locked_);

  529          // Validate inputs
  530:         require(locked_.amount > 0, "No lock");
  531:         require(locked_.end <= block.timestamp, "Lock not expired");
  532:         require(locked_.delegatee == msg.sender, "Lock delegated");
  533          // Update lock

  546          // Send back deposited tokens
  547:         require(token.transfer(msg.sender, value), "Transfer failed");
  548          emit Withdraw(msg.sender, value, LockAction.WITHDRAW, block.timestamp);

  563          // Validate inputs
  564:         require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");
  565:         require(locked_.amount > 0, "No lock");
  566:         require(locked_.delegatee != _addr, "Already delegated");
  567          // Update locks

  587          }
  588:         require(toLocked.amount > 0, "Delegatee has no lock");
  589:         require(toLocked.end > block.timestamp, "Delegatee lock expired");
  590:         require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
  591          _delegate(delegatee, fromLocked, value, LockAction.UNDELEGATE);

  635          // Validate inputs
  636:         require(locked_.amount > 0, "No lock");
  637:         require(locked_.end > block.timestamp, "Lock expired");
  638:         require(locked_.delegatee == msg.sender, "Lock delegated");
  639          // Update lock

  662          // Send back remaining tokens
  663:         require(token.transfer(msg.sender, remainingAmount), "Transfer failed");
  664          emit Withdraw(msg.sender, value, LockAction.QUIT, block.timestamp);

  686          penaltyAccumulated = 0;
  687:         require(token.transfer(penaltyRecipient, amount), "Transfer failed");
  688          emit CollectPenalty(amount, penaltyRecipient);

  786      {
  787:         require(_blockNumber <= block.number, "Only past block number");
  788  

  887      {
  888:         require(_blockNumber <= block.number, "Only past block number");
  889  

contracts/features/Blocklist.sol:
  23      function block(address addr) external {
  24:         require(msg.sender == manager, "Only manager");
  25:         require(_isContract(addr), "Only contracts");
  26          _blocklist[addr] = true;

---

[G-06] Upgrade pragma to 0.8.15 to save gas

Across the whole solution, the declared pragma is 0.8.3 Upgrading to 0.8.15 has many benefits, cheaper on gas is one of them. The following information is regarding 0.8.15 over 0.8.14. Assume that over 0.8.3 the save is higher!

According to the release note of 0.8.15: https://blog.soliditylang.org/2022/06/15/solidity-0.8.15-release-announcement/
The benchmark shows saving of 2.5-10% Bytecode size,
Saving 2-8% Deployment gas,
And saving up to 6.2% Runtime gas.