FIAT DAO veFDT contest - rbserver's results

[L-01] THERE IS NO FUNCTION FOR UNBLOCKING A SMARTWALLET

Currently, there is no function for unblocking a SmartWallet in the Blocklist contract. If a SmartWallet is accidentally blocked through using the block function by the Blocklist contract's manager, it can be inconvenient and gas-consuming to unblock it. To unblock such SmartWallet, a new Blocklist contract needs to be deployed, the previously blocked contracts except for the SmartWallet to be unblocked need to be added to the new Blocklist contract's _blocklist, and the updateBlocklist function needs to be called to point the VotingEscrow contract's blocklist to the new Blocklist contract. Please consider adding a function that can only be called by the manager in the Blocklist contract for unblocking a SmartWallet.

[L-02] MISSING ZERO-ADDRESS CHECK FOR CRITICAL ADDRESSES

To prevent unintended behaviors, the critical address inputs should be checked against address(0).

Please consider checking _manager and _ve in the following constructor.

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

constructor(address _manager, address _ve) {

Please consider checking _owner, _penaltyRecipient, and _token in the following constructor.

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

constructor(
	address _owner,
	address _penaltyRecipient,
	address _token,
	string memory _name,
	string memory _symbol
) {

Please consider checking _addr in the following functions.

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

function transferOwnership(address _addr) external {

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

function updateBlocklist(address _addr) external {

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

function updatePenaltyRecipient(address _addr) external {

[N-01] THERE IS NO FUNCTION FOR SETTING maxPenalty TO OTHER VALUES

The following unlock function can be used to set maxPenalty to 0 in the VotingEscrow contract. It appears to be irreversible by design. Yet, if the protocol possibly wants to continue enforce a penalty for quitting a lock after unlock has been called, a function needs to be added for setting maxPenalty to a specified value, which could be capped with the initial maxPenalty value.

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#L161-L165

function unlock() external {
	require(msg.sender == owner, "Only owner");
	maxPenalty = 0;
	emit Unlock();
}

[N-02] CONSTANT CAN BE USED INSTEAD OF MAGIC NUMBER

To improve readability and maintainability, a constant can be used instead of the magic number. Please consider replacing 10**18 in the following code with a constant.

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

uint256 penaltyAmount = (value * penaltyRate) / 10**18; // quitlock_penalty is in 18 decimals precision

[N-03] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. @param or @return comments are missing for the following functions. Please consider completing NatSpec comments for these functions.

contracts\VotingEscrow.sol
  146: function updateBlocklist(address _addr) external {
  153: function updatePenaltyRecipient(address _addr) external {
  170: function forceUndelegate(address _addr) external override {
  662: function _calculatePenaltyRate(uint256 end)
  701: function _floorToWeek(uint256 _t) internal pure returns (uint256) {
  708: function _findBlockEpoch(uint256 _block, uint256 _maxEpoch)
  732: function _findUserBlockEpoch(address _addr, uint256 _block)

contracts\features\Blocklist.sol
  33: function isBlocked(address addr) public view returns (bool) {

[N-04] MISSING NATSPEC COMMENTS

NatSpec comments provide rich code documentation. NatSpec comments are missing for the following functions. Please consider adding them.

contracts\VotingEscrow.sol
  595: function _delegate(
  685: function _copyLock(LockedBalance memory _locked)   

contracts\features\Blocklist.sol
  37: function _isContract(address addr) internal view returns (bool) {

[N-05] FLOATING PRAGMAS

It is a best practice to lock pragmas instead of using floating pragmas to ensure that contracts are tested and deployed with the intended compiler version. Accidentally deploying contracts with different compiler versions can lead to unexpected risks and undiscovered bugs. Please consider locking pragmas for the following files.

contracts\VotingEscrow.sol
  2: pragma solidity ^0.8.3;

contracts\features\Blocklist.sol
  2: pragma solidity ^0.8.3;

[G-01] PENALTY CALCULATION CAN BE CONDITIONALLY SKIPPED IN quitLock FUNCTION IF maxPenalty IS 0

After the following unlock function is called, maxPenalty is set to 0.

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#L161-L165

function unlock() external {
	require(msg.sender == owner, "Only owner");
	maxPenalty = 0;
	emit Unlock();
}

When maxPenalty is 0, penalty would also be 0 for quitting a lock. Hence, the following quitLock function can be refactored so the penalty calculation can be conditionally skipped if maxPenalty == 0. Run-time gas is saved by skipping these unnecessary operations in this situation.

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#L632-L659

function quitLock() external override nonReentrant {
	...
	// apply penalty
	uint256 penaltyRate = _calculatePenaltyRate(locked_.end);
	uint256 penaltyAmount = (value * penaltyRate) / 10**18; // quitlock_penalty is in 18 decimals precision
	penaltyAccumulated += penaltyAmount;
	uint256 remainingAmount = value - penaltyAmount;
	// Send back remaining tokens
	require(token.transfer(msg.sender, remainingAmount), "Transfer failed");
	emit Withdraw(msg.sender, value, LockAction.QUIT, block.timestamp);
}

[G-02] SAME REQUIRE STATEMENTS CAN BE REFACTORED TO A MODIFIER FOR RELEVANT FUNCTIONS

The same require statements can be refactored to a modifier, which can then be used in the relevant functions. This can make deployment cheaper.

Please consider refactoring require(msg.sender == owner, "Only owner"); to a modifier for the following functions.

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#L136-L165

/// @notice Transfers ownership to a new owner
/// @param _addr The new owner
/// @dev Owner should always be a timelock contract
function transferOwnership(address _addr) external {
	require(msg.sender == owner, "Only owner");
	owner = _addr;
	emit TransferOwnership(_addr);
}

/// @notice Updates the blocklist contract
function updateBlocklist(address _addr) external {
	require(msg.sender == owner, "Only owner");
	blocklist = _addr;
	emit UpdateBlocklist(_addr);
}

/// @notice Updates the recipient of the accumulated penalty paid by quitters
function updatePenaltyRecipient(address _addr) external {
	require(msg.sender == owner, "Only owner");
	penaltyRecipient = _addr;
	emit UpdatePenaltyRecipient(_addr);
}

/// @notice Removes quitlock penalty by setting it to zero
/// @dev This is an irreversible action
function unlock() external {
	require(msg.sender == owner, "Only owner");
	maxPenalty = 0;
	emit Unlock();
}

[G-03] VARIABLE DOES NOT NEED TO BE INITIALIZED TO ITS DEFAULT VALUE

Explicitly initializing a variable with its default value costs more gas than uninitializing it. For example, uint256 i can be used instead of uint256 i = 0 in the following code.

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

[G-04] ++VARIABLE CAN BE USED INSTEAD OF VARIABLE++

++variable costs less gas than variable++. For example, i++ can be changed to ++i in the following code.

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

[G-05] X = X + Y OR X = X - Y CAN BE USED INSTEAD OF X += Y OR X -= Y

x = x + y or x = x - y costs less gas than x += y or x -= y. For example, newLocked.delegated += value can be changed to newLocked.delegated = newLocked.delegated + value in the following code.

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

[G-06] ARITHMETIC OPERATIONS THAT DO NOT OVERFLOW CAN BE UNCHECKED

Explicitly unchecking arithmetic operations that do not overflow by wrapping these in unchecked {} costs less gas than implicitly checking these.

For the following loops, unchecked {++i} can be used at the end of the loop instead of i++.

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

[G-07] REVERT WITH CUSTOM ERROR CAN BE USED INSTEAD OF REQUIRE() WITH REASON STRING

revert with custom error can cost less gas than require() with reason string. To use custom errors, Solidity v0.8.4 or newer is needed. More details about custom errors can be found in https://blog.soliditylang.org/2021/04/21/custom-errors/.

Please consider using Solidity v0.8.4 or newer and revert with custom error to replace the following require().

contracts\VotingEscrow.sol
  116: require(decimals <= 18, "Exceeds max decimals");
  125: require(
  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(
  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(
  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");

contracts\features\Blocklist.sol
  24: require(msg.sender == manager, "Only manager");
  25: require(_isContract(addr), "Only contracts");

[G-08] NEWER VERSION OF SOLIDITY CAN BE USED

The protocol can benefit from more gas-efficient features and fixes by using a newer version of Solidity. Changes for newer Solidity versions can be found in https://blog.soliditylang.org/category/releases/.

contracts\VotingEscrow.sol
  2: pragma solidity ^0.8.3;

contracts\features\Blocklist.sol
  2: pragma solidity ^0.8.3;