FIAT DAO veFDT contest - Junnon's results

Missing address 0 check

check for address 0 is a best practice for smart contract development, recalling the <a href=https://twitter.com/samczsun/status/1554252024723546112>Nomad hack</a> in some case lack of checking 0 value can be fatal.

Instance: Blocklist.sol

1. Blocklist::constructor() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L15-L16

VotingEscrow.sol

1. VotingEscrow::constructor() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L107-L121
2. VotingEscrow::transferOwnership() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L139-L143
3. VotingEscrow::updateBlocklist() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L146-L150
4. VotingEscrow::updatePenaltyRecipient() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L153-L157

Solution: using this following code

require(addr != address(0))

It's Recommended to make a significant role change to two step of function

Some role can make this contract unoperateable, make it a one step change can be risky if we transfer the role to wrong address. it's a best practice to make several role changes function to two step instead.

Instance: VotingEscrow::transferOwnership()

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

Solution: make it two steps

    address pendingOwner;

    function changePendingOwner(address _addr) external {
        require(_addr != address(0), "zero address"); //according to report above
        require(msg.sender == owner, "Only owner"); 
        pendingOwner = _addr;
        emit PendingOwnerChange(_addr);
    }

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

Floating compiler

Both Blocklist.sol and VotingEscrow.sol using a floating pragma ^0.8.3 it's recommended to change it become a fixed pragma to mitigated the bug that can be dangerous caused by outdated compiler. i personally recommended pragma solidity '0.8.15`.

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

Mitigation: can use fixed pragma soldiity, like pragma solidity 0.8.15;

Function naming shadow the buildin symbol

Naming of a function in Blocklist.sol shadow the builtin symbol. that's function is block()(line 23), block is a builtin symbol of solidity that point to block onchain. In this case, Blocklist not calling a builtin block.

instance: Blocklist.sol Blocklist::block() https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/features/Blocklist.sol#L23

Mitigation: rename the function be blockUser or blockAddress,etc.

Missing event

event have a useage for frontend indexing and monitoring. In checkpoint theres no event emitted.

instance VotingEscrow.sol VotingEscrow::checkpoint() or VotingEscrow::_checkpoint() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L397 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L222

Mitigation: make an event for checkpoint then emit it in _checkpoint() or checkpoint()

Variable that only declared at constructor can be marked as immutable

Instance : VotingEscrow.sol

1. VotingEscrow::token https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L45
2. VotingEscrow::name https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L64
3. VotingEscrow::symbol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L65

Blocklist.sol

1. Blocklist::manager https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L11
2. Blocklist::ve https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L12

public function that never called inside the contract should marked as external instead

Instance: VotingEscrow.sol

1. VotingEscrow::balanceOf() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L754
2. VotingEscrow::balanceOfAt() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L770
3. VotingEscrow::totalSupply() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L864
4. VotingEscrow::totalSupplyAt() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L871

Since uint/int variable has default value 0, it is cheaper to leave it empty than passing it with 0 value.

Example

uint a 
//is cheaper than 
uint a = 0

Instance VotingEscrow.sol

1. VotingEscrow::_checkpoint()

229        int128 oldSlopeDelta = 0;
230        int128 newSlopeDelta = 0;

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

309        for (uint256 i = 0; i < 255; i++) {

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

2. VotingEscrow::_findBlockEpoch()

714        uint256 min = 0;

717        for (uint256 i = 0; i < 128; i++) {

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

3. VotingEscrow::_findUserBlockEpoch()

737        uint256 min = 0;

739        for (uint256 i = 0; i < 128; i++) {

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

4. VotingEscrow::balanceOfAt()

793        uint256 dBlock = 0;
794        uint256 dTime = 0;

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

5. VotingEscrow::_supplyAt()

834        for (uint256 i = 0; i < 255; i++) {

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

For Loop can be Cheaper

Loop is gas costly, but we can decrease the gas by this step

1. uint i = 0 can leave default be uint i instead
2. i++ can changed to ++i, since ++i is cheaper
3. can used unchecked for ++i operation

Example

// Existing loop by fiatdao
for (uint256 i = 0; i < 255; i++) {
    ... //implementation
}

// Cheaper version of Loop
for (uint256 i; i < 255 ){
    ...//implementation
    unchecked{ //at the end of the loop
        ++i
    }
}

Instance: VotingEscrow.sol

1. VotingEscrow::_checkpoint() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L309
2. VotingEscrow::_findBlockEpoch()(informational only since the function only called outside the contract) https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L717
3. VotingEscrow::_findUserBlockEpoch()(informational only since the function only called outside the contract) https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L739
4. VotingEscrow::_supplyAt()(informational only since the function only called outside the contract) https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L834