FIAT DAO veFDT contest - simon135's results

no address zero checks

    owner = _addr;

https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/features/Blocklist.sol#L15-L17 https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L107 https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L142 https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L146 https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L156

make solc version locked because if you use diffrent version for deployment and testing then you can run into unexecupted bugs.

blocklist.sol:1 VotingEscrow.sol:1

make emit of the old owner thats best practise

emit TransferOwnership(_addr);

https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L142 https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L146 https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L156

for a few seconds block.timestamp gets to year 2106 it will revert

VotingEscrow.sol:415:        require(unlock_time > block.timestamp, "Only future lock end");
VotingEscrow.sol:416:        require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L416

make transfer ownership model into a approve model and then the new owner can be come owner onced approved

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

make transferowner and importent set functions have a timelock and pause functionillity

voteingEscorw.sol:142 voteingEscorw.sol:146 voteingEscorw.sol:156

Using bools for storage incurs overhead

// 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.

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

make i++ into ++i to save 3 gas on a copy

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

save gas by making require (uint amount >0); into

require (!= 0); because a uint is ether 0 or greater so it saves gas . https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L448

use custom errors instead require to save gas

VotingEscrow.sol:116:        require(decimals <= 18, "Exceeds max decimals");
VotingEscrow.sol:125:        require(
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(
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(
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");

make address(0) into long form to save gas

ex: 0x00000000000000

VotingEscrow.sol:235:        if (_addr != address(0)) {
VotingEscrow.sol:354:        if (_addr != address(0)) {
VotingEscrow.sol:376:        if (_addr != address(0)) {
VotingEscrow.sol:401:        _checkpoint(address(0), empty, empty);
VotingEscrow.sol:425:        _checkpoint(msg.sender, LockedBalance(0, 0, 0, address(0)), locked_);
VotingEscrow.sol:540:        newLocked.delegatee = address(0);
VotingEscrow.sol:645:        newLocked.delegatee = address(0);

USE ASSEMBLY TO CHECK FOR ADDRESS(0)

ex: iszero

VotingEscrow.sol:235:        if (_addr != address(0)) {
VotingEscrow.sol:354:        if (_addr != address(0)) {
VotingEscrow.sol:376:        if (_addr != address(0)) {
VotingEscrow.sol:401:        _checkpoint(address(0), empty, empty);
VotingEscrow.sol:425:        _checkpoint(msg.sender, LockedBalance(0, 0, 0, address(0)), locked_);
VotingEscrow.sol:540:        newLocked.delegatee = address(0);
VotingEscrow.sol:645:        newLocked.delegatee = address(0);

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

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. https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L142 https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L146 https://github.com/code-423n4/2022-08-fiatdao/blob/5a254ab15a387bd65a7dc50ac8371cb77de1e5d5/contracts/VotingEscrow.sol#L156

make i unchecked when it wont overflow

you are saving gas on solidity not checking on an overflow

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