FIAT DAO veFDT contest - Sm4rty's results

1. Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Instances:

contracts/VotingEscrow.sol:426:            token.transferFrom(msg.sender, address(this), _value),
contracts/VotingEscrow.sol:486:            token.transferFrom(msg.sender, address(this), _value),
contracts/VotingEscrow.sol:546:        require(token.transfer(msg.sender, value), "Transfer failed");
contracts/VotingEscrow.sol:657:        require(token.transfer(msg.sender, remainingAmount), "Transfer failed");
contracts/VotingEscrow.sol:676:        require(token.transfer(penaltyRecipient, amount), "Transfer failed");

Reference:

This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol.

Recommended Mitigation Steps:

Consider using safeTransfer/safeTransferFrom or require() consistently.

---

2. USE OF FLOATING PRAGMA

Recommend using fixed solidity version

Instances

All contracts in scope contains floating pragma: https://github.com/code-423n4/2022-08-fiatdao#files-in-scope

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

---

1. Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Starting from Solidity v0.8.4,there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");),but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances:

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

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

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

contracts/features/Blocklist.sol:24:        require(msg.sender == manager, "Only manager");
contracts/features/Blocklist.sol:25:        require(_isContract(addr), "Only contracts");

References:

https://blog.soliditylang.org/2021/04/21/custom-errors/

Remediation:

I suggest replacing revert strings with custom errors.

---

2. !=0 instead of >0 for UINT

0 is less efficient than != 0 for unsigned integers (with proof) != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas) Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you’re in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

Instances

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

contracts/VotingEscrow.sol:412:        require(_value > 0, "Only non zero amount");
contracts/VotingEscrow.sol:448:        require(_value > 0, "Only non zero amount");
contracts/VotingEscrow.sol:449:        require(locked_.amount > 0, "No lock");
contracts/VotingEscrow.sol:469:            require(locked_.amount > 0, "Delegatee has no lock");
contracts/VotingEscrow.sol:502:        require(locked_.amount > 0, "No lock");
contracts/VotingEscrow.sol:529:        require(locked_.amount > 0, "No lock");
contracts/VotingEscrow.sol:564:        require(locked_.amount > 0, "No lock");
contracts/VotingEscrow.sol:587:        require(toLocked.amount > 0, "Delegatee has no lock");
contracts/VotingEscrow.sol:635:        require(locked_.amount > 0, "No lock");
                                                                                

Reference:

https://twitter.com/gzeon/status/1485428085885640706

Remediation:

I suggest changing > 0 with != 0. Also, please enable the Optimizer.

---

3. Variables: No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

We can use uint number; instead of uint number = 0; or bool abc; instead of bool abc = false

Instances:

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

// uint
contracts/VotingEscrow.sol:298:        uint256 blockSlope = 0; // dblock/dt
contracts/VotingEscrow.sol:714:        uint256 min = 0;
contracts/VotingEscrow.sol:737:        uint256 min = 0;
contracts/VotingEscrow.sol:793:        uint256 dBlock = 0;
contracts/VotingEscrow.sol:794:        uint256 dTime = 0;
contracts/VotingEscrow.sol:889:        uint256 dTime = 0;

---

4. ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas PER LOOP

Instances:

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

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

Reference:

https://code4rena.com/reports/2022-05-cally#g-11-ii-should-be-uncheckediuncheckedi-when-it-is-not-possible-for-them-to-overflow-as-is-the-case-when-used-in-for--and-while-loops

---

5. 10 ** 18 can be changed to 1e18 and save some gas Submitted by WatchPug, also found by robee

Instances:

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

contracts/VotingEscrow.sol:48:    uint256 public constant MULTIPLIER = 10**18;
contracts/VotingEscrow.sol:51:    uint256 public maxPenalty = 10**18; // penalty for quitters with MAXTIME remaining lock
contracts/VotingEscrow.sol:653:        uint256 penaltyAmount = (value * penaltyRate) / 10**18; // quitlock_penalty is in 18 decimals precision

Recommendations:

10 ** 18 can be changed to 1e18 to avoid unnecessary arithmetic operation and save gas.s

References:

https://github.com/code-423n4/2021-12-yetifinance-findings/issues/274

---

6. Shift Right instead of Dividing by 2

A division by 2 can be calculated by shifting one to the right.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity’s division operation also includes a division-by-0 prevention which is bypassed using shifting.

I suggest replacing / 2 with >> 1 here:

Instances:

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

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

References:

https://code4rena.com/reports/2022-04-badger-citadel/#g-32-shift-right-instead-of-dividing-by-2

---