FIAT DAO veFDT contest - TomJ's results

Table of Contents

Low Risk Issues

• Missing Zero Address Check
• Admin Address Change should be a 2-Step Process
• Performing a Multiplication on the Result of a Division

Non-critical Issues

• Use fixed compiler versions instead of floating version
• Define Magic Numbers to Constant
• Event is Missing Indexed Fields

 

Low Risk Issues

Missing Zero Address Check

Issue

I recommend adding check of 0-address for input validation of critical address parameters. Not doing so might lead to non-functional contract and have to redeploy the contract, when it is updated to 0-address accidentally.

PoC

Total of 8 issues found.

There is no way to change to another addresses once it is set for following 5 instances.

1. "token" address: set by _token parameter of constructor() of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L107

2. "owner" address: set by _owner parameter of constructor() of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L120

3. "owner" address: set by _addr parameter of transferOwnership() of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L141

4. "manager" address: set by _manager parameter of constructor() of Blocklist.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L15

5. "ve" address: set by _ve parameter of constructor() of Blocklist.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L16

There is way to change to another addresses but still recommended to add 0-address check for following 3 instances.

6. "penaltyRecipient" address: set by _penaltyRecipient parameter of constructor() of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L121

7. "penaltyRecipient" address: set by _addr parameter of updatePenaltyRecipient() of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L155

8. "blocklist" address: set by _addr parameter of updateBlocklist() of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L148

Mitigation

Add 0-address check for above addresses.

 

Admin Address Change should be a 2-Step Process

Issue

High privilege account such as admin / owner is changed with only single process. This can be a concern since an admin / owner role has a high privilege in the contract and mistakenly setting a new admin to an incorrect address will end up losing that privilege.

PoC

Total of 1 issue found.

1. "owner" privilege of VotingEscrow.sol

139:    function transferOwnership(address _addr) external {
140:        require(msg.sender == owner, "Only owner");
141:        owner = _addr;
142:        emit TransferOwnership(_addr);
143:    }

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

Mitigation

This can be fixed by implementing 2-step process. We can do this by following. First make the setAdmin function approve a new address as a pending admin. Next that pending admin has to claim the ownership in a separate transaction to be a new admin.

 

Performing a Multiplication on the Result of a Division

Issue

Solidity integer division might truncate. As a result, performing multiplication before division can sometimes avoid loss of precision. Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#divide-before-multiply

PoC

1. Line 301-302 of _checkpoint() of VotingEscrow.sol

301:                (MULTIPLIER * (block.number - lastPoint.blk)) /
302:                (block.timestamp - lastPoint.ts);

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

2. Line 336-337 of _checkpoint() of VotingEscrow.sol

336:                (blockSlope * (iterativeTime - initialLastPoint.ts)) /
337:                MULTIPLIER;

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

Mitigation

Consider ordering multiplication before division.

 

Non-critical Issues

Use fixed compiler versions instead of floating version

Issue

it is best practice to lock your pragma instead of using floating pragma. the use of floating pragma has a risk of accidentally get deployed using latest complier which may have higher risk of undiscovered bugs. Reference: https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/locking-pragmas/

PoC

./IERC20.sol:2:pragma solidity ^0.8.3;
./IVotingEscrow.sol:2:pragma solidity ^0.8.3;
./VotingEscrow.sol:2:pragma solidity ^0.8.3;
./Blocklist.sol:2:pragma solidity ^0.8.3;
./IBlocklist.sol:2:pragma solidity ^0.8.3;

Mitigation

I suggest to lock your pragma and aviod using floating pragma.

// bad
pragma solidity ^0.8.10;

// good
pragma solidity 0.8.10;

 

Define Magic Numbers to Constant

Issue

It is best practice to define magic numbers to constant rather than just using as a magic number. This improves code readability and maintainability.

PoC

1. Magic Number: 255

./VotingEscrow.sol:309:        for (uint256 i = 0; i < 255; i++) {
./VotingEscrow.sol:834:        for (uint256 i = 0; i < 255; i++) {

2. Magic Number: 128

./VotingEscrow.sol:717:        for (uint256 i = 0; i < 128; i++) {
./VotingEscrow.sol:739:        for (uint256 i = 0; i < 128; i++) {

Mitigation

Define magic numbers to constant.

 

Event is Missing Indexed Fields

Issue

Each event should have 3 indexed fields if there are 3 or more fields.

PoC

Total of 8 issues found.

./IERC20.sol:29:    event Transfer(address indexed from, address indexed to, uint256 value);
./IERC20.sol:30:    event Approval(address indexed owner, address indexed spender, uint256 value);
./VotingEscrow.sol:25:    event Deposit(address indexed provider, uint256 value, uint256 locktime, LockAction indexed action, uint256 ts);
./VotingEscrow.sol:32:    event Withdraw(address indexed provider, uint256 value, LockAction indexed action, uint256 ts);
./VotingEscrow.sol:38:    event TransferOwnership(address owner);
./VotingEscrow.sol:39:    event UpdateBlocklist(address blocklist);
./VotingEscrow.sol:40:    event UpdatePenaltyRecipient(address recipient);
./VotingEscrow.sol:41:    event CollectPenalty(uint256 amount, address recipient);

Mitigation

Add up to 3 indexed fields when possible.

 

Table of Contents

• Should Use Unchecked Block where Over/Underflow is not Possible
• Minimize the Number of SLOADs by Caching State Variable
• Defined Variables Used Only Once
• Unchanging State Variable Should be Immutable
• Duplicate require() Checks Should be a Modifier or a Function
• Change Function Visibility Public to External
• Internal Function Called Only Once can be Inlined
• Use Bit Shifting Instead of Multiplication/Division of 2
• Unnecessary Default Value Initialization
• ++i Costs Less Gas than i++
• != 0 costs less gass than > 0
• Use Custom Errors to Save Gas

 

Should Use Unchecked Block where Over/Underflow is not Possible

Issue

Since Solidity 0.8.0, all arithmetic operations revert on overflow and underflow by default. However in places where overflow and underflow is not possible, it is better to use unchecked block to reduce the gas usage. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#checked-or-unchecked-arithmetic

PoC

Total of 3 issues found.

1. _checkpoint() of VotingEscrow.sol

Wrap line 242 with unchecked since underflow is not possible due to line 236 check
236:            if (_oldLocked.end > block.timestamp && _oldLocked.delegated > 0) {
242:                    int128(int256(_oldLocked.end - block.timestamp));

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

2. _checkpoint() of VotingEscrow.sol

Wrap line 250 with unchecked since underflow is not possible due to line 244 check
244:            if (_newLocked.end > block.timestamp && _newLocked.delegated > 0) {
250:                    int128(int256(_newLocked.end - block.timestamp));

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

3. _checkpoint() of VotingEscrow.sol

Wrap line 302 with unchecked since underflow is not possible due to line 299 check
299:        if (block.timestamp > lastPoint.ts) {
302:                (block.timestamp - lastPoint.ts);

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

Mitigation

Wrap the code with uncheck like described in above PoC.

 

Minimize the Number of SLOADs by Caching State Variable

Issue

SLOADs cost 100 gas where MLOADs/MSTOREs cost only 3 gas. Whenever function reads storage value more than once, it should be cached to save gas.

PoC

Total of 1 issue found.

1. Cache penaltyRecipient of collectPenalty() of VotingEscrow.sol 2 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOAD https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L676-L677

Mitigation

When certain state variable is read more than once, cache it to local variable to save gas.

 

Defined Variables Used Only Once

Issue

Certain variables is defined even though they are used only once. Remove these unnecessary variables to save gas. For cases where it will reduce the readability, one can use comments to help describe what the code is doing.

PoC

Total of 2 issues found.

1. Remove "epoch_" and "lastPoint" variables of totalSupply() of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L865-L867 Delete line 865, 866 and replace line 867 with below line

        return _supplyAt(pointHistory[globalEpoch], block.timestamp);

Mitigation

Don't define variable that is used only once. Details are listed on above PoC.

 

Unchanging State Variable Should be Immutable

Issue

State variable that is only set in the constructor and can't be changed afterwards, should be declared as immutable to save gas.

PoC

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

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

3. token variable of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L45

4. name variable of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L64

5. symbol variable of VotingEscrow.sol https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L65

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

Mitigation

Change above variables to immutable.

 

Duplicate require() Checks Should be a Modifier or a Function

Issue

Since below require checks are used more than once, I recommend making these to a modifier or a function.

PoC

Total of 8 issues found.

./VotingEscrow.sol:140:        require(msg.sender == owner, "Only owner");
./VotingEscrow.sol:147:        require(msg.sender == owner, "Only owner");
./VotingEscrow.sol:154:        require(msg.sender == owner, "Only owner");
./VotingEscrow.sol:162:        require(msg.sender == owner, "Only owner");

./VotingEscrow.sol:412:        require(_value > 0, "Only non zero amount");
./VotingEscrow.sol:448:        require(_value > 0, "Only non zero amount");

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

./VotingEscrow.sol:425:        require(token.transferFrom(msg.sender, address(this), _value), "Transfer failed");
./VotingEscrow.sol:485:        require(token.transferFrom(msg.sender, address(this), _value), "Transfer failed");

./VotingEscrow.sol:449:        require(locked_.amount > 0, "No lock");
./VotingEscrow.sol:502:        require(locked_.amount > 0, "No lock");
./VotingEscrow.sol:529:        require(locked_.amount > 0, "No lock");
./VotingEscrow.sol:564:        require(locked_.amount > 0, "No lock");
./VotingEscrow.sol:635:        require(locked_.amount > 0, "No lock");

./VotingEscrow.sol:450:        require(locked_.end > block.timestamp, "Lock expired");
./VotingEscrow.sol:636:        require(locked_.end > block.timestamp, "Lock expired");

./VotingEscrow.sol:531:        require(locked_.delegatee == msg.sender, "Lock delegated");
./VotingEscrow.sol:637:        require(locked_.delegatee == msg.sender, "Lock delegated");

./VotingEscrow.sol:776:        require(_blockNumber <= block.number, "Only past block number");
./VotingEscrow.sol:877:        require(_blockNumber <= block.number, "Only past block number");

Mitigation

I recommend making duplicate require statement into modifier or a function.

 

Change Function Visibility Public to External

Issue

If the function is not called internally, it is cheaper to set your function visibility to external instead of public.

PoC

Total of 4 issues found.

VotingEscrow.sol: balanceOf() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L754

VotingEscrow.sol: balanceOfAt() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L770

VotingEscrow.sol: totalSupply() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L864

VotingEscrow.sol: totalSupplyAt() https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L871

Mitigation

Change the function visibility to external.

 

Internal Function Called Only Once Can be Inlined

Issue

Certain function is defined even though it is called only once. Inline it instead to where it is called to avoid usage of extra gas.

PoC

Total of 1 issue found.

1. _isContract() of Blocklist.sol Function called only once at line 25 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L37 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L25

Mitigation

I recommend to not define above functions and instead inline it at place it is called.

 

Use Bit Shifting Instead of Multiplication/Division of 2

Issue

The MUL and DIV opcodes cost 5 gas but SHL and SHR only costs 3 gas. Since MUL/DIV and SHL/SHR result the same, use cheaper bit shifting.

PoC

Total of 2 issues found.

./VotingEscrow.sol:719:            uint256 mid = (min + max + 1) / 2;
./VotingEscrow.sol:743:            uint256 mid = (min + max + 1) / 2;

Mitigation

Use bit shifting instead of multiplication/division. Example:

Bad:
uint256 mid = (min + max + 1) / 2;
Good:
uint256 mid = (min + max + 1) >> 2;

 

Unnecessary Default Value Initialization

Issue

When variable is not initialized, it will have its default values. For example, 0 for uint, false for bool and address(0) for address. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#scoping-and-declarations

PoC

Total of 14 issues found.

./VotingEscrow.sol:229:        int128 oldSlopeDelta = 0;
./VotingEscrow.sol:230:        int128 newSlopeDelta = 0;
./VotingEscrow.sol:298:        uint256 blockSlope = 0; // dblock/dt
./VotingEscrow.sol:309:        for (uint256 i = 0; i < 255; i++) {
./VotingEscrow.sol:313:            int128 dSlope = 0;
./VotingEscrow.sol:714:        uint256 min = 0;
./VotingEscrow.sol:717:        for (uint256 i = 0; i < 128; i++) {
./VotingEscrow.sol:737:        uint256 min = 0;
./VotingEscrow.sol:739:        for (uint256 i = 0; i < 128; i++) {
./VotingEscrow.sol:793:        uint256 dBlock = 0;
./VotingEscrow.sol:794:        uint256 dTime = 0;
./VotingEscrow.sol:834:        for (uint256 i = 0; i < 255; i++) {
./VotingEscrow.sol:836:            int128 dSlope = 0;
./VotingEscrow.sol:889:        uint256 dTime = 0;

Mitigation

I suggest removing default value initialization. For example,

• int128 oldSlopeDelta;
• for (uint256 i; i < 255; i++) {

 

++i Costs Less Gas than i++

Issue

Prefix increments/decrements (++i or --i) costs cheaper gas than postfix increment/decrements (i++ or i--).

PoC

Total of 4 issues found.

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

Mitigation

Change it to postfix increments/decrements. It saves 6 gas per loop.

 

!= 0 costs less gass than > 0

Issue

!= 0 costs less gas when optimizer is enabled and is used for unsigned integers in require statement.

PoC

Total of 2 issues found.

./VotingEscrow.sol:412:        require(_value > 0, "Only non zero amount");
./VotingEscrow.sol:448:        require(_value > 0, "Only non zero amount");

Mitigation

I suggest changing > 0 to != 0

require(_value != 0, "Only non zero amount");

 

Use Custom Errors to Save Gas

Issue

Custom errors from Solidity 0.8.4 are cheaper than revert strings. Details are explained here: https://blog.soliditylang.org/2021/04/21/custom-errors/

PoC

Total of 42 issues found.

./VotingEscrow.sol:116:        require(decimals <= 18, "Exceeds max decimals");
./VotingEscrow.sol:125:        require(!IBlocklist(blocklist).isBlocked(msg.sender), "Blocked contract");
./VotingEscrow.sol:140:        require(msg.sender == owner, "Only owner");
./VotingEscrow.sol:147:        require(msg.sender == owner, "Only owner");
./VotingEscrow.sol:154:        require(msg.sender == owner, "Only owner");
./VotingEscrow.sol:162:        require(msg.sender == owner, "Only owner");
./VotingEscrow.sol:171:        require(msg.sender == blocklist, "Only Blocklist");
./VotingEscrow.sol:412:        require(_value > 0, "Only non zero amount");
./VotingEscrow.sol:413:        require(locked_.amount == 0, "Lock exists");
./VotingEscrow.sol:414:        require(unlock_time >= locked_.end, "Only increase lock end"); // from using quitLock, user should increaseAmount instead
./VotingEscrow.sol:415:        require(unlock_time > block.timestamp, "Only future lock end");
./VotingEscrow.sol:416:        require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
./VotingEscrow.sol:425:        require(token.transferFrom(msg.sender, address(this), _value), "Transfer failed");
./VotingEscrow.sol:448:        require(_value > 0, "Only non zero amount");
./VotingEscrow.sol:449:        require(locked_.amount > 0, "No lock");
./VotingEscrow.sol:450:        require(locked_.end > block.timestamp, "Lock expired");
./VotingEscrow.sol:469:            require(locked_.amount > 0, "Delegatee has no lock");
./VotingEscrow.sol:470:            require(locked_.end > block.timestamp, "Delegatee lock expired");
./VotingEscrow.sol:485:        require(token.transferFrom(msg.sender, address(this), _value), "Transfer failed");
./VotingEscrow.sol:502:        require(locked_.amount > 0, "No lock");
./VotingEscrow.sol:503:        require(unlock_time > locked_.end, "Only increase lock end");
./VotingEscrow.sol:504:        require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
./VotingEscrow.sol:511:            require(oldUnlockTime > block.timestamp, "Lock expired");
./VotingEscrow.sol:529:        require(locked_.amount > 0, "No lock");
./VotingEscrow.sol:530:        require(locked_.end <= block.timestamp, "Lock not expired");
./VotingEscrow.sol:531:        require(locked_.delegatee == msg.sender, "Lock delegated");
./VotingEscrow.sol:546:        require(token.transfer(msg.sender, value), "Transfer failed");
./VotingEscrow.sol:563:        require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");
./VotingEscrow.sol:564:        require(locked_.amount > 0, "No lock");
./VotingEscrow.sol:565:        require(locked_.delegatee != _addr, "Already delegated");
./VotingEscrow.sol:587:        require(toLocked.amount > 0, "Delegatee has no lock");
./VotingEscrow.sol:588:        require(toLocked.end > block.timestamp, "Delegatee lock expired");
./VotingEscrow.sol:589:        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
./VotingEscrow.sol:635:        require(locked_.amount > 0, "No lock");
./VotingEscrow.sol:636:        require(locked_.end > block.timestamp, "Lock expired");
./VotingEscrow.sol:637:        require(locked_.delegatee == msg.sender, "Lock delegated");
./VotingEscrow.sol:657:        require(token.transfer(msg.sender, remainingAmount), "Transfer failed");
./VotingEscrow.sol:676:        require(token.transfer(penaltyRecipient, amount), "Transfer failed");
./VotingEscrow.sol:776:        require(_blockNumber <= block.number, "Only past block number");
./VotingEscrow.sol:877:        require(_blockNumber <= block.number, "Only past block number");
./Blocklist.sol:24:        require(msg.sender == manager, "Only manager");
./Blocklist.sol:25:        require(_isContract(addr), "Only contracts");

Mitigation

I suggest implementing custom errors to save gas.