FIAT DAO veFDT contest - bobirichman's results

Table Of Content

• QA REPORT * SPDX license not provided in source file * Loss of precision: multiplications should be before divisions * Missing 0 address check at transfer * Conditions that are based on block number * Array access is out of bounds * Different solidity versions are in use * Avoid floating pragma * Contract should have pause/unpause functionality * Not indexed events * Some of the following function specification is missing * Events not emitted for important state changes * Several functions are declaring named returns but then are using return statements. I suggest choosing only one for readability reasons. * Add event to the following functions

QA REPORT

SPDX license not provided in source file

Before publishing, consider adding a comment containing 'SPDX-License-Identifier: MIT' at the beginning of each source file.

For instance, Blocklist.sol

Loss of precision: multiplications should be before divisions

Consider changing the order of the following instances math operators such that multiplications comes before divisions to improve calculation precision with no cost.

For instance, VotingEscrow.sol#L701

Missing 0 address check at transfer

Some contracts does not support 0 transfer, then the transaction will revert with no explanation. We recommend to add a require statement that the amount is not 0.

For instance, VotingEscrow.sol#L675

Conditions that are based on block number

The following condition statements are based on block number that can be manipulated by a malicious miner.

For instance, VotingEscrow.sol#L896

Array access is out of bounds

There is no check for the access to be in the array bounds.

Code Instances:

• VotingEscrow.sol#L864
• VotingEscrow.sol#L712
• VotingEscrow.sol#L876
• VotingEscrow.sol#L775
• VotingEscrow.sol#L226

Different solidity versions are in use

The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.

Avoid floating pragma

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. (SWC-103)

Code Instances:

• Blocklist.sol
• VotingEscrow.sol

Contract should have pause/unpause functionality

In case a hack is occuring or an exploit is discovered, the team (or validators in this case) should be able to pause functionality until the necessary changes are made to the system. Additionally, the gravity.sol contract should be manged by proxy so that upgrades can be made by the validators.

Because an attack would probably span a number of blocks, a method for pausing the contract would be able to interrupt any such attack if discovered.)

For instance, VotingEscrow.sol

Not indexed events

The emitted event is not indexed, making off-chain scripts such as front-ends of dApps difficult to filter the events efficiently.

Code Instances:

• VotingEscrow.sol#L39
• VotingEscrow.sol#L41
• VotingEscrow.sol#L40
• VotingEscrow.sol#L38

Some of the following function specification is missing

Code Instances:

• VotingEscrow.sol#L498
• VotingEscrow.sol#L106
• VotingEscrow.sol#L560
• VotingEscrow.sol#L397
• VotingEscrow.sol#L526

Events not emitted for important state changes

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Code Instances:

• VotingEscrow.sol#L226
• Blocklist.sol#L14
• VotingEscrow.sol#L106

Several functions are declaring named returns but then are using return statements. I suggest choosing only one for readability reasons.

Using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.

For instance, VotingEscrow.sol#L209

Add event to the following functions

Code Instances:

• Blocklist.sol#L14
• VotingEscrow.sol#L106
• VotingEscrow.sol#L226

GAS REPORT

[GAS] Use assembly opcodes iszero in the following locations

Proof of concept:

• VotingEscrow.sol#L412
• VotingEscrow.sol#L256
• VotingEscrow.sol#L779

[GAS] Use > instead != to compare uint with 0

Proof of concept:

• VotingEscrow.sol#L447
• VotingEscrow.sol#L411

[GAS] Do not cache msg.sender since loading msg.sender is more efficient than a local variable

Proof of concept:

• VotingEscrow.sol#L513
• VotingEscrow.sol#L796
• VotingEscrow.sol#L834

[GAS] In the following revert statements consider using custom error instead a message

Proof of concept:

• VotingEscrow.sol#L469
• VotingEscrow.sol#L153
• Blocklist.sol#L23