FIAT DAO veFDT contest - gogo's results

2022-08-FIATDAO

Low Risk and Non-Critical Issues

Use an onlyOwner modifier instead of repeated checks

The following statement occurs in a total of 4 places in the VotingEscrow contract
require(msg.sender == owner, "Only owner");

File: contracts/features/Blocklist.sol

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

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/features/Blocklist.sol

Missing check for address(0)

File: contracts/VotingEscrow.sol

function transferOwnership(address _addr) external {
    require(msg.sender == owner, "Only owner");
    owner = _addr;
    emit TransferOwnership(_addr);
}

function updateBlocklist(address _addr) external {
    require(msg.sender == owner, "Only owner");
    blocklist = _addr;
    emit UpdateBlocklist(_addr);
}

function updatePenaltyRecipient(address _addr) external {
    require(msg.sender == owner, "Only owner");
    penaltyRecipient = _addr;
    emit UpdatePenaltyRecipient(_addr);
}

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol

Events not emmited on important state changes

Emmiting events is recommended each time when a state variable's value is being changed or just some critical event for the contract has occurred. It also helps off-chain monitoring of the contract's state.

There are 1 instances of this issue:

File: contracts/VotingEscrow.sol

222:  function _checkpoint(

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol

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

There are 7 instances of this issue:

File: contracts/features/Blocklist.sol

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

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/features/Blocklist.sol

File: contracts/VotingEscrow.sol

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)

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol

File: contracts/libraries/ERC20Permit.sol

81:   function transfer(address recipient, uint256 amount)

163:  function approve(address account, uint256 amount)

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/libraries/ERC20Permit.sol

Use 1e18 instead of 10**18 or 1000000000000000000

There are 5 instances of this issue:

File: contracts/VotingEscrow.sol

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

51:   uint256 public maxPenalty = 10**18;

57:   Point[1000000000000000000] public pointHistory;

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

653:  uint256 penaltyAmount = (value * penaltyRate) / 10**18;

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol

Blocklist should inherit from IBlocklist

https://github.com/crytic/slither/wiki/Detector-Documentation#missing-inheritance

File: contracts/features/Blocklist.sol

9:    contract Blocklist {

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/features/Blocklist.sol

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

There are 2 instances of this issue:

File: contracts/features/Blocklist.sol

2:    pragma solidity ^0.8.3;

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/features/Blocklist.sol

File: contracts/VotingEscrow.sol

2:    pragma solidity ^0.8.3;

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol

Dead code

Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#dead-code

File: contracts/libraries/ERC20Permit.sol

137:  function _mint(address account, uint256 amount) internal virtual {

151:  function _burn(address account, uint256 amount) internal virtual {

240:  function _setupDecimals(uint8 decimals_) internal {

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/libraries/ERC20Permit.sol

Event is missing indexed fields

There are 2 instances of this issue:

File: contracts/interfaces/IERC20.sol

29:   event Transfer(address indexed from, address indexed to, uint256 value);

30:   event Approval(
31:       address indexed owner,
32:       address indexed spender,
33:       uint256 value
34:   );

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

2022-08-FIATDAO

Gas Optimizations Report

The usage of ++i will cost less gas than i++. The same change can be applied to i-- as well.

This change would save up to 6 gas per instance/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/tree/main/contracts/VotingEscrow.sol

Using != 0 on uints costs less gas than > 0.

This change saves 3 gas per instance/loop

There are 4 instances of this issue:

File: contracts/features/Blocklist.sol

42:   return size > 0;

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/features/Blocklist.sol

File: contracts/VotingEscrow.sol

288:  if (epoch > 0) {

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

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

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol

It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

Not overwriting the default for stack variables saves 8 gas. Storage and memory variables have larger savings

There are 14 instances of this issue:

File: contracts/VotingEscrow.sol

229:  int128 oldSlopeDelta = 0;

230:  int128 newSlopeDelta = 0;

298:  uint256 blockSlope = 0;

309:  for (uint256 i = 0; i < 255; i++) {

313:  int128 dSlope = 0;

714:  uint256 min = 0;

717:  for (uint256 i = 0; i < 128; i++) {

737:  uint256 min = 0;

739:  for (uint256 i = 0; i < 128; i++) {

793:  uint256 dBlock = 0;

794:  uint256 dTime = 0;

834:  for (uint256 i = 0; i < 255; i++) {

836:  int128 dSlope = 0;

889:  uint256 dTime = 0;

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol

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/tree/main/contracts/VotingEscrow.sol

Functions that are access-restricted from most users may be marked as payable

Marking a function as payable reduces gas cost since the compiler does not have to check whether a payment was provided or not. This change will save around 21 gas per function call.

There are 6 instances of this issue:

File: contracts/features/Blocklist.sol

23:   function block(address addr) external {

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/features/Blocklist.sol

File: contracts/VotingEscrow.sol

139:  function transferOwnership(address _addr) external {

146:  function updateBlocklist(address _addr) external {

153:  function updatePenaltyRecipient(address _addr) external {

161:  function unlock() external {

170:  function forceUndelegate(address _addr) external override {

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol

++i/i++ should be unchecked{++I}/unchecked{I++} in for-loops

When an increment or any arithmetic operation is not possible to overflow it should be placed in unchecked{} block. \This is because of the default compiler overflow and underflow safety checks since Solidity version 0.8.0. \In for-loops it saves around 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/tree/main/contracts/VotingEscrow.sol

<x> += <y> costs more gas than <x> = <x> + <y> for state variables

There are 1 instances of this issue:

File: contracts/VotingEscrow.sol

654:  penaltyAccumulated += penaltyAmount;

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol

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.15/internals/layout_in_storage.html \ Use a larger size then downcast where needed

There are 14 instances of this issue:

File: contracts/VotingEscrow.sol

70:   int128 bias;

71:   int128 slope;

76:   int128 amount;

78:   int128 delegated;

174:  int128 value = locked_.amount;

205:  int128 bias,

206:  int128 slope,

229:  int128 oldSlopeDelta = 0;

230:  int128 newSlopeDelta = 0;

313:  int128 dSlope = 0;

319:  int128 biasDelta =

567:  int128 value = locked_.amount;

598:  int128 value,

836:  int128 dSlope = 0;

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol

Use calldata instead of memory for function parameters

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

There are 5 instances of this issue:

File: contracts/VotingEscrow.sol

224:  LockedBalance memory _oldLocked,

225:  LockedBalance memory _newLocked

597:  LockedBalance memory _locked,

685:  function _copyLock(LockedBalance memory _locked)

825:  function _supplyAt(Point memory _point, uint256 _t)

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol

Replace x <= y with x < y + 1, and x >= y with x > y - 1

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =. Using strict comparison operators hence saves gas

There are 13 instances of this issue:

File: contracts/VotingEscrow.sol

116:  require(decimals <= 18, "Exceeds max decimals");

414:  require(unlock_time >= locked_.end, "Only increase lock end");

416:  require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

504:  require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

530:  require(locked_.end <= block.timestamp, "Lock not expired");

589:  require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");

718:  if (min >= max) break;

720:  if (pointHistory[mid].blk <= _block) {

740:  if (min >= max) {

744:  if (userPointHistory[_addr][mid].blk <= _block) {

776:  require(_blockNumber <= block.number, "Only past block number");

814:  if (upoint.bias >= 0) {

877:  require(_blockNumber <= block.number, "Only past block number");

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol

Use immutable & constant for state variables that do not change their value

There are 5 instances of this issue:

File: contracts/features/Blocklist.sol

11:   address public manager;

12:   address public ve;

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/features/Blocklist.sol

File: contracts/VotingEscrow.sol

45:   IERC20 public token;

64:   string public name;

65:   string public symbol;

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol

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 hitby 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/tree/main/contracts/features/Blocklist.sol

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

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/tree/main/contracts/VotingEscrow.sol

Use a more recent version of solidity

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

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/tree/main/contracts/features/Blocklist.sol

File: contracts/interfaces/IBlocklist.sol

2:    pragma solidity ^0.8.3;

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/interfaces/IBlocklist.sol

File: contracts/interfaces/IERC20.sol

2:    pragma solidity ^0.8.3;

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/interfaces/IERC20.sol

File: contracts/interfaces/IVotingEscrow.sol

2:    pragma solidity ^0.8.3;

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/interfaces/IVotingEscrow.sol

File: contracts/VotingEscrow.sol

2:    pragma solidity ^0.8.3;

https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol