FIAT DAO veFDT contest - oyc_109's results

Judge has assessed an item in Issue #9 as High risk. The relevant finding follows:

[L-01] Upgrade Open Zeppelin contract dependency

An outdated OZ version is used (which has known vulnerabilities, see https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories).

The solution uses:

"@openzeppelin/contracts": "^4.4.2",

[L-02] No Transfer Ownership Pattern

Recommend considering implementing a two step process where the owner or admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

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

[L-03] Unspecific Compiler Version Pragma

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

Blocklist.sol::2 => pragma solidity ^0.8.3;
IBlocklist.sol::2 => pragma solidity ^0.8.3;
IERC20.sol::2 => pragma solidity ^0.8.3;
IVotingEscrow.sol::2 => pragma solidity ^0.8.3;
VotingEscrow.sol::2 => pragma solidity ^0.8.3;

[L-04] Use of Block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

VotingEscrow.sol::236 => if (_oldLocked.end > block.timestamp && _oldLocked.delegated > 0) {
VotingEscrow.sol::242 => int128(int256(_oldLocked.end - block.timestamp));
VotingEscrow.sol::244 => if (_newLocked.end > block.timestamp && _newLocked.delegated > 0) {
VotingEscrow.sol::250 => int128(int256(_newLocked.end - block.timestamp));
VotingEscrow.sol::262 => userNewPoint.ts = block.timestamp;
VotingEscrow.sol::299 => if (block.timestamp > lastPoint.ts) {
VotingEscrow.sol::302 => (block.timestamp - lastPoint.ts);
VotingEscrow.sol::314 => if (iterativeTime > block.timestamp) {
VotingEscrow.sol::315 => iterativeTime = block.timestamp;
VotingEscrow.sol::341 => if (iterativeTime == block.timestamp) {
VotingEscrow.sol::378 => if (_oldLocked.end > block.timestamp) {
VotingEscrow.sol::386 => if (_newLocked.end > block.timestamp) {
VotingEscrow.sol::415 => require(unlock_time > block.timestamp, "Only future lock end");
VotingEscrow.sol::416 => require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
VotingEscrow.sol::450 => require(locked_.end > block.timestamp, "Lock expired");
VotingEscrow.sol::470 => require(locked_.end > block.timestamp, "Delegatee lock expired");
VotingEscrow.sol::489 => emit Deposit(msg.sender, _value, unlockTime, action, block.timestamp);
VotingEscrow.sol::504 => require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
VotingEscrow.sol::511 => require(oldUnlockTime > block.timestamp, "Lock expired");
VotingEscrow.sol::530 => require(locked_.end <= block.timestamp, "Lock not expired");
VotingEscrow.sol::547 => emit Withdraw(msg.sender, value, LockAction.WITHDRAW, block.timestamp);
VotingEscrow.sol::588 => require(toLocked.end > block.timestamp, "Delegatee lock expired");
VotingEscrow.sol::636 => require(locked_.end > block.timestamp, "Lock expired");
VotingEscrow.sol::658 => emit Withdraw(msg.sender, value, LockAction.QUIT, block.timestamp);
VotingEscrow.sol::668 => return ((end - block.timestamp) * maxPenalty) / MAXTIME;
VotingEscrow.sol::762 => (lastPoint.slope * int128(int256(block.timestamp - lastPoint.ts)));
VotingEscrow.sol::801 => dTime = block.timestamp - point0.ts;
VotingEscrow.sol::867 => return _supplyAt(lastPoint, block.timestamp);
VotingEscrow.sol::899 => ((_blockNumber - point.blk) * (block.timestamp - point.ts)) /

[L-05] decimals() not part of ERC20 standard

decimals() is not part of the official ERC20 standard and might fail for tokens that do not implement it. While in practice it is very unlikely, as usually most of the tokens implement it, this should still be considered as a potential issue.

VotingEscrow.sol::115 => decimals = IERC20(_token).decimals();

[L-06] Unsafe use of transfer()/transferFrom() with IERC20

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert. Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead

VotingEscrow.sol::426 => token.transferFrom(msg.sender, address(this), _value),
VotingEscrow.sol::486 => token.transferFrom(msg.sender, address(this), _value),

[L-07] Missing Zero address checks

Zero-address checks are a best practice for input validation of critical address parameters. While the codebase applies this to most cases, there are many places where this is missing in constructors and setters. Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens, or force redeployment of contracts.

Blocklist.sol::15 => manager = _manager;
Blocklist.sol::16 => ve = _ve;
VotingEscrow.sol::120 => owner = _owner;
VotingEscrow.sol::121 => penaltyRecipient = _penaltyRecipient;
VotingEscrow.sol::141 => owner = _addr;
VotingEscrow.sol::148 => blocklist = _addr;
VotingEscrow.sol::155 => penaltyRecipient = _addr;

[N-01] Use a more recent version of solidity

Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>) Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>) Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

Blocklist.sol::2 => pragma solidity ^0.8.3;
IBlocklist.sol::2 => pragma solidity ^0.8.3;
IERC20.sol::2 => pragma solidity ^0.8.3;
IVotingEscrow.sol::2 => pragma solidity ^0.8.3;
VotingEscrow.sol::2 => pragma solidity ^0.8.3;

[N-02] Large multiples of ten should use scientific notation

Use (e.g. 1e6) rather than decimal literals (e.g. 1000000), for better code readability

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

[N-03] Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

Scientific notation should be used for better code readability

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

[N-04] Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

VotingEscrow.sol::38 => event TransferOwnership(address owner);
VotingEscrow.sol::39 => event UpdateBlocklist(address blocklist);
VotingEscrow.sol::40 => event UpdatePenaltyRecipient(address recipient);
VotingEscrow.sol::41 => event CollectPenalty(uint256 amount, address recipient);

[N-05] Missing NatSpec

Code should include NatSpec

IERC20.sol::1 => // SPDX-License-Identifier: Apache-2.0

[N-06] Constants should be defined rather than using magic numbers

It is bad practice to use numbers directly in code without explanation

VotingEscrow.sol::309 => for (uint256 i = 0; i < 255; i++) {

[N-07] Public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

Blocklist.sol::33 => function isBlocked(address addr) public view returns (bool) {
VotingEscrow.sol::754 => function balanceOf(address _owner) public view override returns (uint256) {
VotingEscrow.sol::864 => function totalSupply() public view override returns (uint256) {

[G-01] Don't Initialize Variables with Default Value

Uninitialized variables are assigned with the types default value. Explicitly initializing a variable with it's default value costs unnecesary gas.

VotingEscrow.sol::298 => uint256 blockSlope = 0; // dblock/dt
VotingEscrow.sol::309 => for (uint256 i = 0; i < 255; i++) {
VotingEscrow.sol::714 => uint256 min = 0;
VotingEscrow.sol::717 => for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol::737 => uint256 min = 0;
VotingEscrow.sol::739 => for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol::793 => uint256 dBlock = 0;
VotingEscrow.sol::794 => uint256 dTime = 0;
VotingEscrow.sol::834 => for (uint256 i = 0; i < 255; i++) {
VotingEscrow.sol::889 => uint256 dTime = 0;

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

When dealing with unsigned integer types, comparisons with != 0 are cheaper then with > 0. This change saves 6 gas per instance

VotingEscrow.sol::412 => require(_value > 0, "Only non zero amount");
VotingEscrow.sol::448 => require(_value > 0, "Only non zero amount");
VotingEscrow.sol::449 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::469 => require(locked_.amount > 0, "Delegatee has no lock");
VotingEscrow.sol::502 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::529 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::564 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::587 => require(toLocked.amount > 0, "Delegatee has no lock");
VotingEscrow.sol::635 => require(locked_.amount > 0, "No lock");

[G-03] Use Shift Right/Left instead of Division/Multiplication if possible

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

VotingEscrow.sol::719 => uint256 mid = (min + max + 1) / 2;
VotingEscrow.sol::743 => uint256 mid = (min + max + 1) / 2;

[G-04] 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.

VotingEscrow.sol::70 => int128 bias;
VotingEscrow.sol::71 => int128 slope;
VotingEscrow.sol::76 => int128 amount;
VotingEscrow.sol::78 => int128 delegated;
VotingEscrow.sol::174 => int128 value = locked_.amount;
VotingEscrow.sol::229 => int128 oldSlopeDelta = 0;
VotingEscrow.sol::230 => int128 newSlopeDelta = 0;
VotingEscrow.sol::533 => uint256 value = uint256(uint128(locked_.amount));
VotingEscrow.sol::567 => int128 value = locked_.amount;

[G-05] 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. Use uint256(1) and uint256(2) for true/false instead

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

[G-06] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, for example 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

VotingEscrow.sol::309 => for (uint256 i = 0; i < 255; i++) {
VotingEscrow.sol::717 => for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol::739 => for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol::834 => for (uint256 i = 0; i < 255; i++) {

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

use <x> = <x> + <y> or <x> = <x> - <y> instead to save gas

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

[G-08] 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

Blocklist.sol::24 => require(msg.sender == manager, "Only manager");
Blocklist.sol::25 => require(_isContract(addr), "Only contracts");
VotingEscrow.sol::116 => require(decimals <= 18, "Exceeds max decimals");
VotingEscrow.sol::140 => require(msg.sender == owner, "Only owner");
VotingEscrow.sol::147 => require(msg.sender == owner, "Only owner");
VotingEscrow.sol::154 => require(msg.sender == owner, "Only owner");
VotingEscrow.sol::162 => require(msg.sender == owner, "Only owner");
VotingEscrow.sol::171 => require(msg.sender == blocklist, "Only Blocklist");
VotingEscrow.sol::412 => require(_value > 0, "Only non zero amount");
VotingEscrow.sol::413 => require(locked_.amount == 0, "Lock exists");
VotingEscrow.sol::414 => require(unlock_time >= locked_.end, "Only increase lock end"); // from using quitLock, user should increaseAmount instead
VotingEscrow.sol::415 => require(unlock_time > block.timestamp, "Only future lock end");
VotingEscrow.sol::416 => require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
VotingEscrow.sol::448 => require(_value > 0, "Only non zero amount");
VotingEscrow.sol::449 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::450 => require(locked_.end > block.timestamp, "Lock expired");
VotingEscrow.sol::469 => require(locked_.amount > 0, "Delegatee has no lock");
VotingEscrow.sol::470 => require(locked_.end > block.timestamp, "Delegatee lock expired");
VotingEscrow.sol::502 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::503 => require(unlock_time > locked_.end, "Only increase lock end");
VotingEscrow.sol::504 => require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
VotingEscrow.sol::511 => require(oldUnlockTime > block.timestamp, "Lock expired");
VotingEscrow.sol::529 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::530 => require(locked_.end <= block.timestamp, "Lock not expired");
VotingEscrow.sol::531 => require(locked_.delegatee == msg.sender, "Lock delegated");
VotingEscrow.sol::546 => require(token.transfer(msg.sender, value), "Transfer failed");
VotingEscrow.sol::563 => require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");
VotingEscrow.sol::564 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::565 => require(locked_.delegatee != _addr, "Already delegated");
VotingEscrow.sol::587 => require(toLocked.amount > 0, "Delegatee has no lock");
VotingEscrow.sol::588 => require(toLocked.end > block.timestamp, "Delegatee lock expired");
VotingEscrow.sol::589 => require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
VotingEscrow.sol::635 => require(locked_.amount > 0, "No lock");
VotingEscrow.sol::636 => require(locked_.end > block.timestamp, "Lock expired");
VotingEscrow.sol::637 => require(locked_.delegatee == msg.sender, "Lock delegated");
VotingEscrow.sol::657 => require(token.transfer(msg.sender, remainingAmount), "Transfer failed");
VotingEscrow.sol::676 => require(token.transfer(penaltyRecipient, amount), "Transfer failed");
VotingEscrow.sol::776 => require(_blockNumber <= block.number, "Only past block number");
VotingEscrow.sol::877 => require(_blockNumber <= block.number, "Only past block number");

[G-09] Do not calculate constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas. Consequences: each usage of a constant costs more gas on each access. Since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library)

VotingEscrow.sol::48 => uint256 public constant MULTIPLIER = 10**18;

[G-10] 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

Blocklist.sol::2 => pragma solidity ^0.8.3;
IBlocklist.sol::2 => pragma solidity ^0.8.3;
IERC20.sol::2 => pragma solidity ^0.8.3;
IVotingEscrow.sol::2 => pragma solidity ^0.8.3;
VotingEscrow.sol::2 => pragma solidity ^0.8.3;

[G-11] Prefix increments cheaper than Postfix increments

++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) Saves 5 gas PER LOOP

VotingEscrow.sol::309 => for (uint256 i = 0; i < 255; i++) {
VotingEscrow.sol::717 => for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol::739 => for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol::834 => for (uint256 i = 0; i < 255; i++) {

[G-12] Public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public and can save gas by doing so.

Blocklist.sol::33 => function isBlocked(address addr) public view returns (bool) {
VotingEscrow.sol::754 => function balanceOf(address _owner) public view override returns (uint256) {
VotingEscrow.sol::864 => function totalSupply() public view override returns (uint256) {

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

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

[G-14] Use assembly to check for address(0)

Saves 6 gas per instance if using assembly to check for address(0)

e.g.

assembly {
 if iszero(_addr) {
  mstore(0x00, "zero address")
  revert(0x00, 0x20)
 }
}

instances:

VotingEscrow.sol::233 => if (_addr != address(0)) {
VotingEscrow.sol::352 => if (_addr != address(0)) {
VotingEscrow.sol::374 => if (_addr != address(0)) {

[G-15] 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.

Blocklist.sol::37 => function _isContract(address addr) internal view returns (bool) {

[G-16] State variables only set in the constructor should be declared immutable

Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

Blocklist.sol::15 => manager = _manager;
Blocklist.sol::16 => ve = _ve;
VotingEscrow.sol::107 => token = IERC20(_token);
VotingEscrow.sol::115 => decimals = IERC20(_token).decimals();
VotingEscrow.sol::118 => name = _name;
VotingEscrow.sol::119 => symbol = _symbol;

[G-17] Unnecesary cast

no need to cast int256(MAXTIME)

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