FIAT DAO veFDT contest - robee's results

safeApprove of openZeppelin is deprecated

You use safeApprove of openZeppelin although it's deprecated. (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38) You should change it to increase/decrease Allowance as OpenZeppilin says.

Code instances:

    Deprecated safeApprove in MockSmartWallet.sol line 19: fdt.approve(ve, amount);
    Deprecated safeApprove in MockSmartWallet.sol line 24: fdt.approve(ve, amount);

Not verified input

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

Code instances:

    VotingEscrow.sol.forceUndelegate _addr
    Blocklist.sol.block addr
    ERC20Permit.sol.transferFrom recipient

Solidity compiler versions mismatch

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

Code instance:

Use safe math for solidity version <8

You should use safe math for solidity version <8 since there is no default over/under flow check it suchversions of solidity.

Code instance:

    The contract Authorizable.sol doesn't use safe math and is of solidity version < 8

Named return issue

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

Code instance:

    VotingEscrow.sol, getLastUserPoint

Two Steps Verification before Transferring Ownership

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

Code instances:

    VotingEscrow.sol
    Authorizable.sol

Missing non reentrancy modifier

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

Code instance:

    VotingEscrow.sol, collectPenalty is missing a reentrancy modifier

In the following public update functions no value is returned

In the following functions no value is returned, due to which by default value of return will be 0. We assumed that after the update you return the latest new value. (similar issue here: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/85).

Code instances:

    VotingEscrow.sol, updatePenaltyRecipient
    VotingEscrow.sol, updateBlocklist

Check transfer receiver is not 0 to avoid burned money

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Code instances:

    https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol#L486
    https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol#L426
    https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/libraries/ERC20Permit.sol#L141
    https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/libraries/ERC20Permit.sol#L154
    https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/libraries/ERC20PermitWithMint.sol#L68

Add a timelock

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Code instances:

    https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/mocks/MockERC20.sol#L20
    https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/libraries/Authorizable.sol#L50
    https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/mocks/MockERC20.sol#L16

Must approve 0 first

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

Code instances:

approve without approving 0 first MockSmartWallet.sol, 19, fdt.approve(ve, amount);

approve without approving 0 first MockSmartWallet.sol, 24, fdt.approve(ve, amount);

Div by 0

Division by 0 can lead to accidentally revert, (An example of a similar issue - https://github.com/code-423n4/2021-10-defiprotocol-findings/issues/84)

Code instances:

    https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol#L300 lastPoint might be 0
    https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol#L893 point might be 0
    https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol#L898 point might be 0

Tokens with fee on transfer are not supported

There are ERC20 tokens that charge fee for every transfer() / transferFrom().

Vault.sol#addValue() assumes that the received amount is the same as the transfer amount, and uses it to calculate attributions, balance amounts, etc. But, the actual transferred amount can be lower for those tokens. Therefore it's recommended to use the balance change before and after the transfer instead of the amount. This way you also support the tokens with transfer fee - that are popular.

Code instance:

    https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/libraries/ERC20Permit.sol#L107

approve return value is ignored

Some tokens don't correctly implement the EIP20 standard and their approve function returns void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert. Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the mentioned contracts as they revert the transaction because of the missing return value. We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handle the return value check as well as non-standard-compliant tokens. The list of occurrences in format (solidity file, line number, actual line)

Code instances:

MockSmartWallet.sol, 24, fdt.approve(ve, amount);

MockSmartWallet.sol, 19, fdt.approve(ve, amount);

transfer return value of a general ERC20 is ignored

Need to use safeTransfer instead of transfer. As there are popular tokens, such as USDT that transfer/trasnferFrom method doesn’t return anything. The transfer return value has to be checked (as there are some other tokens that returns false instead revert), that means you must

1. Check the transfer return value Another popular possibility is to add a whiteList. Those are the appearances (solidity file, line number, actual line):

Code instances:

    VotingEscrow.sol, 426 (createLock), token.transferFrom(msg.sender, address(this), _value),
    VotingEscrow.sol, 486 (increaseAmount), token.transferFrom(msg.sender, address(this), _value),

State variables that could be set immutable

In the following files there are state variables that could be set immutable to save gas.

Code instances:

    ve in Blocklist.sol
    symbol in ERC20Permit.sol
    fdt in MockSmartWallet.sol
    name in ERC20Permit.sol
    manager in Blocklist.sol
    DOMAIN_SEPARATOR in ERC20Permit.sol

Prefix increments are cheaper than postfix increments

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

Code instances:

    change to prefix increment and unchecked: VotingEscrow.sol, i, 717
    change to prefix increment and unchecked: VotingEscrow.sol, i, 834
    change to prefix increment and unchecked: VotingEscrow.sol, i, 739
    change to prefix increment and unchecked: VotingEscrow.sol, i, 309

Unnecessary index init

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

Code instances:

    VotingEscrow.sol, 717
    VotingEscrow.sol, 309
    VotingEscrow.sol, 739
    VotingEscrow.sol, 834

Short the following require messages

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

Code instance:

    Solidity file: ERC20Permit.sol, In line 225, Require message length to shorten: 34, The message: ERC20: invalid signature 's' value

Use != 0 instead of > 0

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

Code instances:

    ERC20Permit.sol, 104: change 'balance > 0' to 'balance != 0'
    VotingEscrow.sol, 448: change '_value > 0' to '_value != 0'
    VotingEscrow.sol, 412: change '_value > 0' to '_value != 0'

Use unchecked to save gas for certain additive calculations that cannot overflow

You can use unchecked in the following calculations since there is no risk to overflow:

Code instances:

    VotingEscrow.sol (L#416) - require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
    VotingEscrow.sol (L#504) - require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

Consider inline the following functions to save gas

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)


    VotingEscrow.sol, _floorToWeek, { return (_t / WEEK) * WEEK; }

Inline one time use functions

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

Code instances:

    ERC20PermitWithMint.sol, _burn
    VotingEscrow.sol, _calculatePenaltyRate
    Blocklist.sol, _isContract
    ERC20Permit.sol, _extraConstruction
    ERC20PermitWithMint.sol, _mint
    Authorizable.sol, _authorize
    VotingEscrow.sol, _findUserBlockEpoch

Cache powers of 10 used several times

You calculate the power of 10 every time you use it instead of caching it once as a constant variable and using it instead. Fix the following code lines:

Code instances:

    VotingEscrow.sol, 50 : You should cache the used power of 10 as constant state variable since it's used several times (3): uint256 public maxPenalty = 10**18;
    VotingEscrow.sol, 652 : You should cache the used power of 10 as constant state variable since it's used several times (3): uint256 penaltyAmount = (value * penaltyRate) / 10**18;

Upgrade pragma to at least 0.8.4

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.

The advantages of versions 0.8.* over <0.8.0 are:

    1. Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.)
    2. Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
    3. Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
    4. Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Code instance:

    Authorizable.sol

Do not cache msg.sender

We recommend not to cache msg.sender since calling it is 2 gas while reading a variable is more.

Code instances:

    https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/libraries/Authorizable.sol#L15
    https://github.com/code-423n4/2022-08-fiatdao/tree/main/contracts/VotingEscrow.sol#L418