code-423n4 / 2022-08-fiatdao-findings

2 stars 1 forks source link

Gas Optimizations #213

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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