FIAT DAO veFDT contest - durianSausage's results

Low Risk

L01: address variable should check if it is zero MISSING CHECKS FOR ADDRESS(0X0) WHEN ASSIGNING VALUES TO ADDRESS STATE VARIABLES

prof

Blocklist.sol,15,16

        manager = _manager;
        ve = _ve;

VotingEscrow.sol, 107,120,121,141,148,155

Non-Critical Issues

N01: NON-ASSEMBLY METHOD AVAILABLE

prof

Blocklist.sol,39-41

        assembly {
            size := extcodesize(addr)
        }

we have the following form codes.

size = address(addr).code.length

gas optimization

G01: custom error save more gas

problem

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained https://blog.soliditylang.org/2021/04/21/custom-errors/. Custom errors are defined using the error statement.

prof

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

G02: COMPARISONS WITH ZERO FOR UNSIGNED INTEGERS

problem

0 is less gas efficient than !0 if you enable the optimizer at 10k AND you’re in a require statement. Detailed explanation with the opcodes https://twitter.com/gzeon/status/1485428085885640706

prof

features/Blocklist.sol, 42, b' return size > 0;' VotingEscrow.sol, 288, b' if (epoch > 0) {' VotingEscrow.sol, 412, b' require(_value > 0, "Only non zero amount");' VotingEscrow.sol, 448, b' require(_value > 0, "Only non zero amount");'

G03: PREFIX INCREMENT SAVE MORE GAS

problem

prefix increment ++i is more cheaper than postfix i++

prof

VotingEscrow.sol, 309, b' for (uint256 i = 0; i < 255; i++) {' VotingEscrow.sol, 717, b' for (uint256 i = 0; i < 128; i++) {' VotingEscrow.sol, 739, b' for (uint256 i = 0; i < 128; i++) {' VotingEscrow.sol, 834, b' for (uint256 i = 0; i < 255; i++) {'

G04: X += Y COSTS MORE GAS THAN X = X + Y FOR STATE VARIABLES

prof

VotingEscrow.sol, 654, b' penaltyAccumulated += penaltyAmount;'

G05: USING BOOLS FOR STORAGE INCURS OVERHEAD

problem

// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.

prof

features/Blocklist.sol, 10, b' mapping(address => bool) private _blocklist;'

G06: resign the default value to the variables.

problem

resign the default value to the variables will cost more gas.

prof

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

G07: USING PRIVATE RATHER THAN PUBLIC FOR CONSTANTS, SAVES GAS

problem:

We can save getter function of public constants.

prof:

VotingEscrow.sol, 46-48

G08: DUPLICATED REQUIRE()/REVERT() CHECKS SHOULD BE REFACTORED TO A MODIFIER OR FUNCTION

problem

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they’re hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

prof

VotingEscrow.sol, 140,147,154,162

G09: FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED PAYABLE

problem

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

prof

VotingEscrow.sol, 140,147,154,162 Blocklist.sol, 23

G10: USE A MORE RECENT VERSION OF SOLIDITY

problem

Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads 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.

G11: ++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

problem

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

prof

VotingEscrow.sol, 309, b' for (uint256 i = 0; i < 255; i++) {' VotingEscrow.sol, 717, b' for (uint256 i = 0; i < 128; i++) {' VotingEscrow.sol, 739, b' for (uint256 i = 0; i < 128; i++) {' VotingEscrow.sol, 834, b' for (uint256 i = 0; i < 255; i++) {'