FIAT DAO veFDT contest - 0xbepresent's results

1 - Not initializing the blocklist contract in VotingEscrow.sol constructor

The blocklist contract address is not initialized in the VotingEscrow.sol constructor.

The modifier checkBlocklist uses the blocklist contract address therefore the functions that use this modifier could lead to unexpected behaivors.

Recommendation

Initialize the blocklist contract address in the constructor so the creator/owner do not forget to setting up.

2 - Consider two-phase ownershop transfer

The owner calls transferOwnership in order to tranfers the ownership to the new address directly. There is a risk that the ownership is transferred to an invalid address, thus causing the contract to be without owner.

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

Recommendation

Consider a two step process where the owner nominates an account and the nominated account need to call an acceptOwnership() function for the transfer of owner to fully succeed. This ensures the nominated EOA account is a valid and active account.

1 - Internal functions only called once can be inlined to save gas

Not inlining costs more gas because of extra JUMP instructions and additional stack operations needed for function calls.

There are 2 instances of this issue:

./VotingEscrow.sol:662:    function _calculatePenaltyRate(uint256 end)
./VotingEscrow.sol:732:    function _findUserBlockEpoch(address _addr, uint256 _block)

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

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

2 - Unnecessary variable convertion and assigment

There is a value assigment to a delegated property https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#L537 but in the line https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#L540 the property is assigned to zero. The line VotingEscrow.sol#L537 could be omitted to save gas

There are 2 instances of this issue

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#L537
https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#L642

3 - x =+ y costs more gas than x = x + y for state variables

There is 1 instance of this issue:

VotingEscrow.sol
L654: penaltyAccumulated += penaltyAmount;

4 - Unnecessary checked arithmetic in for loop

There is no risk that the loop counter can overflow, using solidity's unchecked block saves gas.

There are 4 instances of this issue:

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

Unchecked implementation example:

for (uint256 i; i < 10;) {
    j++;
    unchecked {
        ++i;
    }
}

Gas Report example using this gist:

╭─────────────────────────────────────────────┬─────────────────┬──────┬────────┬──────┬─────────╮
│ src/test/Unchecked.t.sol:Contract0 contract ┆                 ┆      ┆        ┆      ┆         │
╞═════════════════════════════════════════════╪═════════════════╪══════╪════════╪══════╪═════════╡
│ Deployment Cost                             ┆ Deployment Size ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 55105                                       ┆ 307             ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                               ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ withOutUnChecked                            ┆ 2068            ┆ 2068 ┆ 2068   ┆ 2068 ┆ 1       │
╰─────────────────────────────────────────────┴─────────────────┴──────┴────────┴──────┴─────────╯
╭─────────────────────────────────────────────┬─────────────────┬──────┬────────┬──────┬─────────╮
│ src/test/Unchecked.t.sol:Contract1 contract ┆                 ┆      ┆        ┆      ┆         │
╞═════════════════════════════════════════════╪═════════════════╪══════╪════════╪══════╪═════════╡
│ Deployment Cost                             ┆ Deployment Size ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 53705                                       ┆ 300             ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                               ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ withUnchecked                               ┆ 1408            ┆ 1408 ┆ 1408   ┆ 1408 ┆ 1       │
╰─────────────────────────────────────────────┴─────────────────┴──────┴────────┴──────┴─────────╯