code-423n4 / 2022-08-fiatdao-findings

2 stars 1 forks source link

QA Report #35

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Table Of Content

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:

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:

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:

Some of the following function specification is missing

Code Instances:

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:

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:

lacoop6tu commented 2 years ago

Good one