QA Report

[code423n4]

Overview

Risk Rating	Number of issues
Low Risk	4
Non-Critical Risk	4

Table of Contents

• 1. Low Risk Issues
  • 1.1. Known vulnerabilities exist in currently used @openzeppelin/contracts version
  • 1.2. Prevent accidentally burning tokens
  • 1.3. Use a 2-step ownership transfer pattern
  • 1.4. ERC20 decimals size
• 2. Non-Critical Issues
  • 2.1. Related mappings should be grouped in a struct
  • 2.2. 1000000000000000000 should be changed to 1e18 for readability reasons (and 1000000000 to 1e9)
  • 2.3. Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)
  • 2.4. Non-library/interface files should use fixed compiler versions, not floating ones

1. Low Risk Issues

1.1. Known vulnerabilities exist in currently used @openzeppelin/contracts version

As some known vulnerabilities exist in the current @openzeppelin/contracts version, consider updating package.json with at least @openzeppelin/contracts@4.7.3 here:

• https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/package.json#L42

File: package.json
42:     "@openzeppelin/contracts": "^4.4.2",

While vulnerabilities are known, the current scope isn't affected (this might not hold true for the whole solution)

1.2. Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

As penaltyRecipient can be set to 0:

File: VotingEscrow.sol
153:     function updatePenaltyRecipient(address _addr) external {
154:         require(msg.sender == owner, "Only owner");
155:         penaltyRecipient = _addr;
156:         emit UpdatePenaltyRecipient(_addr);
157:     }

Consider adding a check to prevent accidentally burning tokens here, where it's using the state variable penaltyRecipient:

VotingEscrow.sol:676:        require(token.transfer(penaltyRecipient, amount), "Transfer failed");

1.3. Use a 2-step ownership transfer pattern

Contracts inheriting from OpenZeppelin's libraries have the default transferOwnership() function (a one-step process). It's possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role. Consider overriding the default transferOwnership() function to first nominate an address as the pendingOwner and implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.

File: VotingEscrow.sol
139:     function transferOwnership(address _addr) external {
140:         require(msg.sender == owner, "Only owner");
141:         owner = _addr;
142:         emit TransferOwnership(_addr);
143:     }

1.4. ERC20 decimals size

The EIP20 specification optionally defines a uint8 decimals field. However, the corresponding field here is a uint256. This is not compliant with the specification and may cause confusion when interacting with wallets and dApps. Consider setting the decimals type to uint8.

VotingEscrow.sol:66:    uint256 public decimals = 18;
VotingEscrow.sol:115:        decimals = IERC20(_token).decimals(); //@audit this returns a uint8

2. Non-Critical Issues

2.1. Related mappings should be grouped in a struct

The "userPoint" mappings should be grouped in a struct.

From:

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

To

    struct UserPointInfo {
        Point[1000000000] history;   
        uint256 epoch;   
    }

    mapping(address => UserPointInfo) public userPointInfo;

It would be less error-prone, more readable, and it would be possible to delete all related fields with a simple delete userPointInfo[address].

2.2. 1000000000000000000 should be changed to 1e18 for readability reasons (and 1000000000 to 1e9)

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

2.3. Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

VotingEscrow.sol:48:    uint256 public constant MULTIPLIER = 10**18;
VotingEscrow.sol:51:    uint256 public maxPenalty = 10**18; // penalty for quitters with MAXTIME remaining lock
VotingEscrow.sol:653:        uint256 penaltyAmount = (value * penaltyRate) / 10**18; // quitlock_penalty is in 18 decimals precision

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

features/Blocklist.sol:2:pragma solidity ^0.8.3;
VotingEscrow.sol:2:pragma solidity ^0.8.3;