FIAT DAO veFDT contest - 0x52's results

QA Report

Low Risk Findings

[L-01] Use two-step ownership transfer

Description

VotingEscrow.sol doesn't have any checks in place and could accidentally transfer ownership to the wrong address.

Lines of Code

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

Mitigation

Consider adding a two-step transfer. The first transaction would set the new owner as pending and the second would be called by the new owner to confirm the transfer

[L-02] Use of floating pragma

Description

Contracts should be deployed with the same compiler version and flags that they have been designed and tested. Different compiler versions/flags may introduce bugs that affect the contract negatively. Locking the pragma helps avoid this.

Lines of Code

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

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

Mitigation

Change to:

pragma solidity 0.8.3;

Non-Critical Findings

[NC-01] Use 52 WEEKS for MAXTIME instead of 365 days

Description

Since time is floored to weeks, max time should be 52 weeks instead of 365 days so that MAXTIME is divisible by weeks

Lines of Code

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

Mitigation

Change to:

uint256 public constant MAXTIME = 52*WEEK;

[NC-02] Decimals is declared as 18 but is always overwritten in the constructor

Description

Always going to be overwritten so waste of gas to declare it then overwrite it.

Lines of Code

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

Mitigation

Change to:

uint256 public decimals;

[NC-03] Create and use OnlyOwner modifer for owner functions

Description

All owner functions check that msg.sender == owner. Since it is repeated so many times, it would benefit readability if this check was added as a modifer

Lines of Code

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

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

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

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

Mitigation

Implement OnlyOwner modifier and implement it on these functions

[NC-04] Comment indicates that that user should use increaseAmount after quitLock but that is impossible

Description

Comment indicates that user should use increaseAmount instead of createLock after using quitLock. This is impossible due to the require statement in L449 which would revert because lock.amount == 0.

Lines of Code

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

Mitigation

Remove L449