FIAT DAO veFDT contest - ellahi's results

Table of Contents

Low

• 1. Use Two-Step Transfer Pattern for Access Controls
• 2. Unsafe ERC20 Operation(s)
• 3. Missing checks for address(0) when assigning values to address state variables.
• 4. Unspecific Compiler Version Pragma

Non-Critical

• 1. constants should be defined rather than using magic numbers.
• 2. Typo

Low Findings

1. Use Two-Step Transfer Pattern for Access Controls

Impact

Contracts implementing access control's, e.g. owner, should consider implementing a Two-Step Transfer pattern. Otherwise it's possible that the role mistakenly transfers ownership to the wrong address, resulting in a loss of the role

Findings:

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

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

Recommendation

Consider adding a pendingOwner where the new owner will have to accept the ownership.

Example

address owner;
address pendingOwner;

// ...

function setPendingOwner(address newPendingOwner) external {
    require(msg.sender == owner, "!owner");
    emit NewPendingOwner(newPendingOwner);
    pendingOwner = newPendingOwner;
}

function acceptOwnership() external {
    require(msg.sender == pendingOwner, "!pendingOwner");
    emit NewOwner(pendingOwner);
    owner = pendingOwner;
    pendingOwner = address(0);
}

2. Unsafe ERC20 Operation(s)

Impact

The return value of an external transfer/transferFrom call is not checked

Findings:

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

Recommendation

Use SafeERC20, or ensure that the transferFrom return value is checked.

3. Missing checks for address(0) when assigning values to address state variables.

Impact

Setters of address type parameters should include a zero-address check otherwise contract functionality may become inaccessible or tokens burnt forever.

Findings:

VotingEscrow.sol::139 => function transferOwnership(address _addr) external {
VotingEscrow.sol::146 => function updateBlocklist(address _addr) external {
VotingEscrow.sol::152 => function updatePenaltyRecipient(address _addr) external {
Blocklist.sol::14 => constructor(address _manager, address _ve) {

VotingEscrow.sol#L100-L106

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

Recommendation

Add address(0) checks.

4. Unspecific Compiler Version Pragma

Impact

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.

Findings:

VotingEscrow.sol::2 => pragma solidity ^0.8.3;
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;

Recommendation

Avoid floating pragmas for non-library contracts. It is recommended to pin to a concrete compiler version.

Non-Critical Findings

1. constant's should be defined rather than using magic numbers.

VotingEscrow.sol::653 => uint256 penaltyAmount = (value * penaltyRate) / 10**18

2. Typo

- VotingEscrow.sol::705 preceeding
+ VotingEscrow.sol::705 => preceding
- VotingEscrow.sol::729 preceeding
+ VotingEscrow.sol::729 => preceding

Table of Contents

Gas

• 1. Use != 0 instead of > 0 for Unsigned Integer Comparison in require statements
• 2. Use Custom Errors instead of Revert Strings.
• 3. No need to initialize variables with default values
• 4. ++i costs less gas compared to i++ or i += 1
• 5. Use Shift Right/Left instead of Division/Multiplication if possible
• 6. Contracts using unlocked pragma.
• 7. Use storage instead of memory for structs/arrays.
• 8. x += y costs more gas than x = x + y for state variables.

Gas Findings

1. Use != 0 instead of > 0 for Unsigned Integer Comparison in require statements

Impact

!= 0 is cheapear than > 0 when comparing unsigned integers in require statements.

Findings:

VotingEscrow.sol::412 => require(_value > 0, "Only non zero amount");
VotingEscrow.sol::448 => require(_value > 0, "Only non zero amount");

Recommendation

Use != 0 instead of > 0.

2. Use Custom Errors instead of Revert Strings.

Impact

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Findings:

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");
Blocklist.sol::24 => require(msg.sender == manager, "Only manager");
Blocklist.sol::25 => require(_isContract(addr), "Only contracts");

Recommendation

Use custom errors instead of revert strings.

3. No need to initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Findings:

VotingEscrow.sol::229 => int128 oldSlopeDelta = 0;
VotingEscrow.sol::230 => int128 newSlopeDelta = 0;
VotingEscrow.sol::298 => uint256 blockSlope = 0; // dblock/dt
VotingEscrow.sol::309 => for (uint256 i = 0; i < 255; i++) {
VotingEscrow.sol::313 => int128 dSlope = 0;
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::836 => int128 dSlope = 0;
VotingEscrow.sol::889 => uint256 dTime = 0;

Recommendation

Remove explicit default initializations.

4. ++i costs less gas compared to i++ or i += 1

Impact

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

Findings:

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

Recommendation

Use ++i instead of i++ to increment the value of an uint variable.

5. Use Shift Right/Left instead of Division/Multiplication if possible

Impact

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.

Findings:

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

Recommendation

Use SHR/SHL. Bad

uint256 b = a / 2
uint256 c = a / 4;
uint256 d = a * 8;

Good

uint256 b = a >> 1;
uint256 c = a >> 2;
uint256 d = a << 3;

6. Contracts using unlocked pragma.

Impact

Contracts in scope use pragma solidity ^0.8.3 allowing a wide enough range of versions.

Findings:

VotingEscrow.sol::2 => pragma solidity ^0.8.3;
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;

Recommendation

Consider locking compiler version, for example pragma solidity 0.8.6. This can have additional benefits, for example using custom errors to save gas and so forth.

7. Use storage instead of memory for structs/arrays.

Impact

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct.

Findings:

VotingEscrow.sol::172 => LockedBalance memory locked_ = locked[_addr];
VotingEscrow.sol::214 => Point memory point = userPointHistory[_addr][uepoch];
VotingEscrow.sol::410 => LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol::446 => LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol::499 => LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol::527 => LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol::561 => LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol::633 => LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol::759 => Point memory lastPoint = userPointHistory[_owner][epoch];
VotingEscrow.sol::783 => Point memory upoint = userPointHistory[_owner][userEpoch];
VotingEscrow.sol::788 => Point memory point0 = pointHistory[epoch];
VotingEscrow.sol::796 => Point memory point1 = pointHistory[epoch + 1];
VotingEscrow.sol::866 => Point memory lastPoint = pointHistory[epoch_];
VotingEscrow.sol::882 => Point memory point = pointHistory[targetEpoch];
VotingEscrow.sol::891 => Point memory pointNext = pointHistory[targetEpoch + 1];

Recommendation

Use storage instead of memory for findings above

8. x += y costs more gas than x = x + y for state variables.

Same thing applies for subtraction

Findings:

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;

Recommendation

Use x = x + y instead of `x += y

Tools used

manual, c4udit, slither