FIAT DAO veFDT contest - ElKu's results

Low Risk Issues

Summary Of Findings:

Details on Findings:

1. <ins>Zero-check is not performed for address</ins>

In the VotingEscrow.sol, address is not checked for a zero value before assigning it to the owner in the transferOwnership function.

Mitigation would be to add a require statement in the function as shown below:

    function transferOwnership(address _addr) external {
        require(msg.sender == owner, "Only owner");
        require(_addr != address(0), "Zero Address");
        owner = _addr;  // @audit check for non-zero
        emit TransferOwnership(_addr);
    }

2. <ins>Use extra checks in the unlock() function</ins>

In the VotingEscrow.sol, in the unlock() function, as an irreversible action, I would suggest to add some extra checks.

Mitigation would be to either make it a timelock or add a simple uintparameter to make sure that the owner really intents to do what he is doing.

    function unlock(uint256 password) external {
        require(msg.sender == owner, "Only owner");
        require(password == 666, "Accidental calling of unlock");
        maxPenalty = 0;
        emit Unlock();
    }

Non-Critical Issues

Summary Of Findings:

Details on Findings:

1. <ins>Non-library/interface files should use fixed compiler versions, not floating ones</ins>

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

https://swcregistry.io/docs/SWC-103

One instance found in VotingEscrow.sol

pragma solidity ^0.8.3;

2. <ins>Use a newer version of Solidity</ins>

The codebase uses Solidity version 0.8.3 which was released in March 2021. Though its not possible to keep up with the version changes of Solidity, its advisable to use a relatively newer version. The current Solidity version is 0.8.16 which was released in August 2022 (almost 1.5 years later than the current version used by the codebase).

By using an older version you might be missing the following features:

• Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
• Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
• Use a solidity version of at least 0.8.16 to have more efficient code for checked addition and subtraction.

See this for a more detailed list of features and bug fixes in each solidity version.

Gas Optimizations

Summary Of Gas Savings:

The gas savings are done on the VotingEscrow contract. The savings for each of the methods are tabulated below:

Detailed Report on Gas Optimization Findings:

1. <ins>Function increaseAmount</ins>

In the increaseAmount function, the following line causes unnecessary storage writes:

//Original Code:
        locked[msg.sender] = locked_;

//Can be optimized into:
        locked[msg.sender].amount = locked_.amount;
//because only 'amount' is updated in the struct. No need to rewrite the whole struct.

Comparison of Hardhat gas reports, before and after the above changes, showed an average gas saving of 163 gas.

2. <ins>Function increaseUnlockTime</ins>

In the increaseUnlockTime function, the following line causes unnecessary storage writes:

//Original Code:
        locked[msg.sender] = locked_;

//Can be optimized into:
        locked[msg.sender].end = unlock_time;
//because only 'end' is updated in the struct. No need to rewrite the whole struct.

Comparison of Hardhat gas reports, before and after the above changes, showed an average gas saving of 766 gas.

3. <ins>Function delegate</ins>

In the delegate function, the following line causes unnecessary storage writes:

//Original Code:
        locked[msg.sender] = locked_;

//Can be optimized into:
        locked[msg.sender].delegatee = _addr;
//because only 'delegatee' is updated in the struct. No need to rewrite the whole struct.

Comparison of Hardhat gas reports, before and after the above changes, showed an average gas saving of 67 gas.

4. <ins>Use custom errors instead of revert strings</ins>

Another major area in which we could save deployment cost would be in converting the revert strings into custom errors. If the function does revert, you can also save on dynamic gas cost. See this example implementation to understand the dynamics of custom errors. It shows that each require string converted into a custom error saves you around 11000 gas.

Also in the VotingEscrow contract, many of the revert strings are repetitive. They all could use the same custom error. Custom errors are available from Solidity version 0.8.4.

    error Only_Owner();

 	require(msg.sender == owner, "Only owner");
	//can be rewritten as:
	if(msg.sender != owner)
            revert Only_Owner();

If you don't want to change the pragma to 0.8.4, then you can use constant strings to avoid repetitive usage of strings. For example, the error, Only owner appears in 4 places. Deployment gas could be saved by doing the following mitigation:

    string constant only_owner= "Only owner";

 	require(msg.sender == owner, "Only owner");
	//can be rewritten as:
	require(msg.sender == owner, only_owner);

File : VotingEscrow.sol

Listing out all the require strings which could be converted into custom errors:

File:     contracts/VotingEscrow.sol
---------------------------------------

Line 116:       require(decimals <= 18, "Exceeds max decimals");
Line 125:       require(
                    !IBlocklist(blocklist).isBlocked(msg.sender),
                    "Blocked contract"
                ); 
Line 140:       require(msg.sender == owner, "Only owner");
Line 147:       require(msg.sender == owner, "Only owner");
Line 154:       require(msg.sender == owner, "Only owner");
Line 162:       require(msg.sender == owner, "Only owner");
Line 171:       require(msg.sender == blocklist, "Only Blocklist");
Line 412-416:   require(_value > 0, "Only non zero amount");
                require(locked_.amount == 0, "Lock exists");
                require(unlock_time >= locked_.end, "Only increase lock end"); 
                require(unlock_time > block.timestamp, "Only future lock end");
                require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");   
Line 425:       require(
                    token.transferFrom(msg.sender, address(this), _value),
                    "Transfer failed"
                );
Line 448-450:   require(_value > 0, "Only non zero amount");
                require(locked_.amount > 0, "No lock");
                require(locked_.end > block.timestamp, "Lock expired");   
Line 469-470:   require(locked_.amount > 0, "Delegatee has no lock");
                require(locked_.end > block.timestamp, "Delegatee lock expired");   
Line 485:       require(
                      token.transferFrom(msg.sender, address(this), _value),
                      "Transfer failed"
                  );
Line 502-504:   require(locked_.amount > 0, "No lock");
                require(unlock_time > locked_.end, "Only increase lock end");
                require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");   
Line 511:       require(oldUnlockTime > block.timestamp, "Lock expired");     
Line 529-531:   require(locked_.amount > 0, "No lock");
                require(locked_.end <= block.timestamp, "Lock not expired");
                require(locked_.delegatee == msg.sender, "Lock delegated");   
Line 546:       require(token.transfer(msg.sender, value), "Transfer failed");
Line 563-565:   require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");
                require(locked_.amount > 0, "No lock");
                require(locked_.delegatee != _addr, "Already delegated");   
Line 587-589:   require(toLocked.amount > 0, "Delegatee has no lock");
                require(toLocked.end > block.timestamp, "Delegatee lock expired");
                require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");   
Line 635-637:   require(locked_.amount > 0, "No lock");
                require(locked_.end > block.timestamp, "Lock expired");
                require(locked_.delegatee == msg.sender, "Lock delegated");   
Line 657:       require(token.transfer(msg.sender, remainingAmount), "Transfer failed");
Line 676:       require(token.transfer(penaltyRecipient, amount), "Transfer failed");
Line 776:       require(_blockNumber <= block.number, "Only past block number");
Line 877:       require(_blockNumber <= block.number, "Only past block number");     

There are a total of 40 require strings. Converting all of them into custom errors can save us around 40 * 11000 = 440,000 gas when deploying.

Or if you want to keep the current solidity version, and use constant strings for repetitive errors, the following strings are used more than once:

"Blocked contract"             used 2 times.
"Only owner"                   used 4 times. 
"Only non zero amount"         used 2 times.
"Only increase lock end"       used 2 times.
"Exceeds maxtime"              used 2 times.
"Transfer failed"              used 5 times.
"No lock"                      used 5 times.
"Lock expired"                 used 3 times.
"Lock delegated"               used 2 times.
"Only past block number"       used 2 times.
"Delegatee lock expired"       used 2 times.
"Delegatee has no lock"        used 2 times.

Conclusions:

The above changes reduced the deployment cost by 440,000. And Dynamic cost was reduced by 996.