FIAT DAO veFDT contest - JohnSmith's results

Lines of code

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

Vulnerability details

[M-00] Attacker contract can avoid being blocked by BlockList.sol

Problem

To block an address it must pass the isContract(address) check: https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/features/Blocklist.sol#L25

contracts/features/Blocklist.sol
25:         require(_isContract(addr), "Only contracts");

Which just checks code length at the address provided.

contracts/features/Blocklist.sol
37:     function _isContract(address addr) internal view returns (bool) {
38:         uint256 size;
39:         assembly {
40:             size := extcodesize(addr)
41:         }
42:         return size > 0;
43:     }

Attacker can interact with the system and selfdestruct his contract, and with help of CREATE2 recreate it at same address when he needs to interact with the system again.

Proof of concept

Below is a simple example of salted contract creation, which you can test against _isContract(address) function.

pragma solidity 0.8.15;

contract BlockList {
    function _isContract(address addr) external view returns (bool) {
        uint256 size;
        assembly {
            size := extcodesize(addr)
        }
        return size > 0;
    }
}

contract AttackerContract {
  function destroy() external {
    selfdestruct(payable(0));
  }
}

contract AttackerFactory {
    function deploy() external returns (address) {
        return address(new AttackerContract{salt: bytes32("123")}());
    }
}

Mitigation

One of the goals of Ethereum is for humans and smart contracts to both be treated equally. This leads into a future where smart contracts interact seamlessly with humans and other contracts. It might change in the future , but for now an arbitrary address is ambiguous. We should consider blacklisting addresses without checking if they are contracts.

Lines of code

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

Vulnerability details

[M-00] increaseUnlockTime(uint256) assigns wrong expiration date to old lock

Problem

When a user increases lock time of undelegated lock via function increaseUnlockTime(uint256), old LockedBalance is updated with wrong expiration date and because of this _checkpoint() is fed with incorrect values, which may result in incorrect calculations inside _checkpoint().

Findings

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

contracts/VotingEscrow.sol
505:         // Update lock
506:         uint256 oldUnlockTime = locked_.end;
507:         locked_.end = unlock_time;
508:         locked[msg.sender] = locked_;
509:         if (locked_.delegatee == msg.sender) {
510:             // Undelegated lock
511:             require(oldUnlockTime > block.timestamp, "Lock expired");
512:             LockedBalance memory oldLocked = _copyLock(locked_);
513:             oldLocked.end = unlock_time;//@audit needs to be oldUnlockTime
514:             _checkpoint(msg.sender, oldLocked, locked_);
515:         }

Because of the bug both LockedBalances sent to _checkpoint() are same.

Mitigation

Old LockedBalance should be updated with old expiration date, which is stored in oldUnlockTime 513: oldLocked.end = oldUnlockTime;

[L-01] Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious Manager making a frontrunning/sandwich attack on the fees). Consider adding a timelock to:

contracts/VotingEscrow.sol
161:     function unlock() external {
162:         require(msg.sender == owner, "Only owner");
163:         maxPenalty = 0;
164:         emit Unlock();
165:     }

[L-02] Zero-address checks are missing

Zero-address checks are a best practice for input validation of critical address parameters. Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens, or force redeployment of contracts.

Findings

contracts/VotingEscrow.sol

107:         token = IERC20(_token); //@audit should be immutable

118:         name = _name;
119:         symbol = _symbol;
120:         owner = _owner;
121:         penaltyRecipient = _penaltyRecipient;

139:     function transferOwnership(address _addr) external {
140:         require(msg.sender == owner, "Only owner");
141:         owner = _addr;
142:         emit TransferOwnership(_addr);
143:     }

146:     function updateBlocklist(address _addr) external {
147:         require(msg.sender == owner, "Only owner");
148:         blocklist = _addr;
149:         emit UpdateBlocklist(_addr);
150:     }

153:     function updatePenaltyRecipient(address _addr) external {
154:         require(msg.sender == owner, "Only owner");
155:         penaltyRecipient = _addr;
156:         emit UpdatePenaltyRecipient(_addr);
157:     }

contracts/features/Blocklist.sol
14:     constructor(address _manager, address _ve) {
15:         manager = _manager;
16:         ve = _ve;
17:     }

[L-03] Use of floating pragma

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

Findings

contracts/VotingEscrow.sol
2: pragma solidity ^0.8.3;

contracts/features/Blocklist.sol
2: pragma solidity ^0.8.3;

Recommended Mitigation Steps

Lock the pragma version to the same version as used in the other contracts and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.

[L-04] Behavior described by comment is incomplete

138: /// @dev Owner should always be a timelock contract There are no checks that owner is a timelock contract, it is can be anything even address(0) by mistake or by malicious intent

Finidngs

contracts/VotingEscrow.sol
138:     /// @dev Owner should always be a timelock contract
139:     function transferOwnership(address _addr) external {
140:         require(msg.sender == owner, "Only owner");
141:         owner = _addr;
142:         emit TransferOwnership(_addr);
143:     }

Recommended Mitigation Steps

Add some checks or remove/modify the comment

[L-05] Shadowing builtin symbol.

https://swcregistry.io/docs/SWC-119 In future it could go unnoticed and subsequently lead to security issues.

Findings

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/vote-escrow/VoteEscrowDelegation.sol#L171

contracts/features/Blocklist.sol
23:     function block(address addr) external {

This declaration shadows a builtin symbol.

Recommended mitigation steps

Rename the local variables that shadow another component.

[N-01] Function is missing NatSpec

Findigs

contracts/features/Blocklist.sol
14:     constructor(address _manager, address _ve) {
15:         manager = _manager;
16:         ve = _ve;
17:     }

37:     function _isContract(address addr) internal view returns (bool) {
38:         uint256 size;
39:         assembly {
40:             size := extcodesize(addr)
41:         }
42:         return size > 0;// @audit no need to use assembly should be addr.code.length > 0
43:     }

contracts/VotingEscrow.sol
594:     // Delegates from/to lock and voting power
595:     function _delegate(
596:         address addr,
597:         LockedBalance memory _locked,
598:         int128 value,
599:         LockAction action
600:     ) internal {

662:     function _calculatePenaltyRate(uint256 end)
663:         internal
664:         view
665:         returns (uint256)
666:     {

684:     // Creates a copy of a lock
685:     function _copyLock(LockedBalance memory _locked)
686:         internal
687:         pure
688:         returns (LockedBalance memory)
689:     {

[N-02] 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. As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Findings

contracts/VotingEscrow.sol
229:         int128 oldSlopeDelta = 0;
230:         int128 newSlopeDelta = 0;

298:         uint256 blockSlope = 0; // dblock/dt

309:         for (uint256 i = 0; i < 255; i++) {

313:             int128 dSlope = 0;

714:         uint256 min = 0;

717:         for (uint256 i = 0; i < 128; i++) {

737:         uint256 min = 0;

739:         for (uint256 i = 0; i < 128; i++) {

793:         uint256 dBlock = 0;
794:         uint256 dTime = 0;

834:         for (uint256 i = 0; i < 255; i++) {

836:             int128 dSlope = 0;

889:         uint256 dTime = 0;

[N-03] Large multiples of ten should use scientific notation (e.g. 1e6) rather than decimal literals (e.g. 1000000 or 10**6), for readability

Findings

contracts/VotingEscrow.sol
48:     uint256 public constant MULTIPLIER = 10**18;

51:     uint256 public maxPenalty = 10**18; // penalty for quitters with MAXTIME remaining lock 

57:     Point[1000000000000000000] public pointHistory; // 1e9 * userPointHistory-length, so sufficient for 1e9 users
58:     mapping(address => Point[1000000000]) public userPointHistory;

653:         uint256 penaltyAmount = (value * penaltyRate) / 10**18; // quitlock_penalty is in 18 decimals precision

[N-04] Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

Findings

contracts/VotingEscrow.sol
25:     event Deposit(
26:         address indexed provider,
27:         uint256 value,
28:         uint256 locktime,
29:         LockAction indexed action,
30:         uint256 ts
31:     );
32:     event Withdraw(
33:         address indexed provider,
34:         uint256 value,
35:         LockAction indexed action,
36:         uint256 ts
37:     );

[N-05] Non-assembly method available

Findings

contracts/features/Blocklist.sol
38:         uint256 size;
39:         assembly {
40:             size := extcodesize(addr)
41:         }
42:         return size > 0;

can be: return addr.code.length > 0

[N-06] constants should be defined rather than using magic numbers

Findings

contracts/VotingEscrow.sol
57:     Point[1000000000000000000] public pointHistory; // 1e9 * userPointHistory-length, so sufficient for 1e9 users
58:     mapping(address => Point[1000000000]) public userPointHistory;

309:         for (uint256 i = 0; i < 255; i++) {

717:         for (uint256 i = 0; i < 128; i++) {

739:         for (uint256 i = 0; i < 128; i++) {

834:         for (uint256 i = 0; i < 255; i++) {

[N-07] NatSpec is incomplete

Check @audit comments for details

Findings

contracts/VotingEscrow.sol
			//@audit missing tag @param _addr
145:     /// @notice Updates the blocklist contract
146:     function updateBlocklist(address _addr) external {

				//@audit missing tag @param _addr
152:     /// @notice Updates the recipient of the accumulated penalty paid by quitters
153:     function updatePenaltyRecipient(address _addr) external {

					//@audit missing tag @return
699:     // @dev Floors a timestamp to the nearest weekly increment
700:     // @param _t Timestamp to floor
701:     function _floorToWeek(uint256 _t) internal pure returns (uint256) {

					//@audit missing tag @return
705:     // @dev Uses binarysearch to find the most recent point history preceeding block
706:     // @param _block Find the most recent point history before this block
707:     // @param _maxEpoch Do not search pointHistories past this index
708:     function _findBlockEpoch(uint256 _block, uint256 _maxEpoch)
709:         internal
710:         view
711:         returns (uint256)
712:     {

					//@audit missing tag @return
729:     // @dev Uses binarysearch to find the most recent user point history preceeding block
730:     // @param _addr User for which to search
731:     // @param _block Find the most recent point history before this block
732:     function _findUserBlockEpoch(address _addr, uint256 _block)
733:         internal
734:         view
735:         returns (uint256)
736:     {

[N-08] Consider two-phase ownership transfer

Admin calls Ownable.transferOwnership function to transfers the ownership to the new address directly. As such, there is a risk that the ownership is transferred to an invalid address, thus causing the contract to be without a owner.

contracts/VotingEscrow.sol
139:     function transferOwnership(address _addr) external {
140:         require(msg.sender == owner, "Only owner");
141:         owner = _addr;
142:         emit TransferOwnership(_addr);
143:     }

Recommendation

Consider implementing a two step process where the admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of admin to fully succeed. This ensures the nominated account is a valid and active account.

[N-09] Obsolete variable

contracts/VotingEscrow.sol
46:     uint256 public constant WEEK = 7 days;

Solidity already has time unit representing a week, we can use 1 weeks intead of WEEK

contracts/VotingEscrow.sol
291:         uint256 lastCheckpoint = lastPoint.ts;

lastCheckpoint always has lastPoint.ts value and updated together

contracts/VotingEscrow.sol
332:             lastCheckpoint = iterativeTime;
333:             lastPoint.ts = iterativeTime;

During function execution sometimes used lastCheckpoint other times lastPoint.ts. Choose one and stick with it.

[N-10] Consider unblocking functionality

Contract can be permanently blocked by mistake or malicious intent. Consider adding unblocking functionality.

[N-11] Duplicated require()/revert() checks should be refactored to a modifier or function

Findings

contracts/VotingEscrow.sol
140:         require(msg.sender == owner, "Only owner");

147:         require(msg.sender == owner, "Only owner");

154:         require(msg.sender == owner, "Only owner");

162:         require(msg.sender == owner, "Only owner");

[G-01] Cheaper input valdiations should come before expensive operations

check @audit for more details

Findings

contracts/VotingEscrow.sol
403:     function createLock(uint256 _value, uint256 _unlockTime)
404:         external
405:         override
406:         nonReentrant
407:         checkBlocklist
408:     {
409:         uint256 unlock_time = _floorToWeek(_unlockTime); // Locktime is rounded down to weeks
410:         LockedBalance memory locked_ = locked[msg.sender];//@audit SLOAD
411:         // Validate inputs
412:         require(_value > 0, "Only non zero amount"); // @audit should check input before SLOAD
413:         require(locked_.amount == 0, "Lock exists");
414:         require(unlock_time >= locked_.end, "Only increase lock end"); // from using quitLock, user should increaseAmount instead
415:         require(unlock_time > block.timestamp, "Only future lock end");// @audit should check input before SLOAD
416:         require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");// @audit should check input before SLOAD

555:     function delegate(address _addr)
556:         external
557:         override
558:         nonReentrant
559:         checkBlocklist
560:     {
561:         LockedBalance memory locked_ = locked[msg.sender];
562:         // Validate inputs
563:         require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");//@audit check before L561 to save gas

[G-02] Amounts should be checked for 0 before calling a transfer

Checking non-zero transfer values can avoid an expensive external call and save gas.

Findings

contracts/VotingEscrow.sol
673:     function collectPenalty() external {
674:         uint256 amount = penaltyAccumulated;
675:         penaltyAccumulated = 0;
676:         require(token.transfer(penaltyRecipient, amount), "Transfer failed");

[G-03] Public functions to external

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

Findings

contracts/VotingEscrow.sol
754:     function balanceOf(address _owner) public view override returns (uint256) {

770:     function balanceOfAt(address _owner, uint256 _blockNumber)
771:         public
772:         view
773:         override
774:         returns (uint256)
775:     {

864:     function totalSupply() public view override returns (uint256) {

871:     function totalSupplyAt(uint256 _blockNumber)
872:         public
873:         view
874:         override
875:         returns (uint256)
876:     {

contracts/features/Blocklist.sol
33:     function isBlocked(address addr) public view returns (bool) {

[G-04] Use a more recent version of solidity

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

Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 - 2600 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value

Findings

contracts/VotingEscrow.sol
2: pragma solidity ^0.8.3;

contracts/features/Blocklist.sol
2: pragma solidity ^0.8.3;

[G-05] x += y costs more gas than x = x + y for state variables

x += y costs more than x = x + y same as x -= y

Findings

contracts/VotingEscrow.sol
654:         penaltyAccumulated += penaltyAmount;

[G-06] 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

Findings

contracts/VotingEscrow.sol
139:     function transferOwnership(address _addr) external {
140:         require(msg.sender == owner, "Only owner");

146:     function updateBlocklist(address _addr) external {
147:         require(msg.sender == owner, "Only owner");

153:     function updatePenaltyRecipient(address _addr) external {
154:         require(msg.sender == owner, "Only owner");

161:     function unlock() external {
162:         require(msg.sender == owner, "Only owner");

170:     function forceUndelegate(address _addr) external override {
171:         require(msg.sender == blocklist, "Only Blocklist");

contracts/features/Blocklist.sol
23:     function block(address addr) external {
24:         require(msg.sender == manager, "Only manager");

[G-07] Do not calculate constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

Findings

contracts/VotingEscrow.sol
48:     uint256 public constant MULTIPLIER = 10**18;

[G-08] Copying to memory can be more expensive than using the storage keyword

In this particular case here, I suggest using the storage keyword instead of the memory one, as the copy in memory is wasting some MSTOREs and MLOADs.

contracts/VotingEscrow.sol
214:         Point memory point = userPointHistory[_addr][uepoch];
215:         return (point.bias, point.slope, point.ts);

[G-09] Structs can be packed into fewer storage slots

Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings From 4 slots:

contracts/VotingEscrow.sol
75:     struct LockedBalance {
76:         int128 amount;
77:         uint256 end;//@audit can we make it uint64, or even uint96
78:         int128 delegated;
79:         address delegatee;
80:     }

To 2 slots:

struct LockedBalance {
				//slot 1
        int128 amount;
        int128 delegated;
				//slot 2
        address delegatee;// 160 bits
        uint64 end;//uint64, or up to uint96
    }

[G-10] Variable reinitialization

Reinitializing n constructor already initialized decimals variable is waste of gas

contracts/VotingEscrow.sol
66:     uint256 public decimals = 18;//@audit reinitialized in constructor

contracts/
VotingEscrow.sol
100:     constructor(
101:         address _owner,
102:         address _penaltyRecipient,
103:         address _token,
104:         string memory _name,
105:         string memory _symbol
106:     ) {
107:         token = IERC20(_token);
108:         pointHistory[0] = Point({
109:             bias: int128(0),
110:             slope: int128(0),
111:             ts: block.timestamp,
112:             blk: block.number
113:         });
114: 
115:         decimals = IERC20(_token).decimals();

[G-11] internal functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

contracts/features/Blocklist.sol
25:         require(_isContract(addr), "Only contracts");// @audit save gss by inlining iscontract to addr.code.length

37:     function _isContract(address addr) internal view returns (bool) {
38:         uint256 size;
39:         assembly {
40:             size := extcodesize(addr)
41:         }
42:         return size > 0;
43:     }

Can be: 25: require(addr.code.length > 0, "Only contracts");

[G-12] Comparisons with zero for unsigned integers in require statements.

> 0 is less efficient than != 0 for unsigned integers.

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

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.

Findings

contracts/VotingEscrow.sol
412:         require(_value > 0, "Only non zero amount");

448:         require(_value > 0, "Only non zero amount");

[G-13] Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

Findings

contracts/VotingEscrow.sol
46:     uint256 public constant WEEK = 7 days;
47:     uint256 public constant MAXTIME = 365 days;
48:     uint256 public constant MULTIPLIER = 10**18;

[G-14] Unchecked arithmetic

The default “checked” behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected. if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas.

Findings

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

416:         require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

504:         require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

655:         uint256 remainingAmount = value - penaltyAmount;

717:         for (uint256 i = 0; i < 128; i++) {

739:         for (uint256 i = 0; i < 128; i++) {

834:         for (uint256 i = 0; i < 255; i++) {

Place the arithmetic operations in an unchecked block

	for (uint i; i < length;) {
		...
		unchecked{ ++i; }
	}

[G-15] 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.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from 'false' to 'true', after having been 'true' in the past

Findings

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

[G-16] Multiplication/division by 2 should use bit shifting

<x> * 2 is equivalent to <x> << 1 and <x> / 2 is the same as <x> >> 1. The MUL and DIV opcodes cost 5 gas, whereas SHL and SHR only cost 3 gas

Findings

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

743:             uint256 mid = (min + max + 1) / 2;

[G-17] Use custom errors rather than revert()/require() 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) while providing the same amount of information, as explained here https://blog.soliditylang.org/2021/04/21/custom-errors/

Custom errors are defined using the error statement

Findings

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

contracts/VotingEscrow.sol
116:         require(decimals <= 18, "Exceeds max decimals");

125:         require(
126:             !IBlocklist(blocklist).isBlocked(msg.sender),
127:             "Blocked contract"
128:         );

140:         require(msg.sender == owner, "Only owner");

146:         require(msg.sender == owner, "Only owner");

154:         require(msg.sender == owner, "Only owner");

162:         require(msg.sender == owner, "Only owner");

171:         require(msg.sender == blocklist, "Only Blocklist");

412:         require(_value > 0, "Only non zero amount");
413:         require(locked_.amount == 0, "Lock exists");
414:         require(unlock_time >= locked_.end, "Only increase lock end"); // from using quitLock, user should increaseAmount instead
415:         require(unlock_time > block.timestamp, "Only future lock end");
416:         require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

425:         require(
426:             token.transferFrom(msg.sender, address(this), _value),
427:             "Transfer failed"
428:         );

448:         require(_value > 0, "Only non zero amount");
449:         require(locked_.amount > 0, "No lock");
450:         require(locked_.end > block.timestamp, "Lock expired");

469:             require(locked_.amount > 0, "Delegatee has no lock");
470:             require(locked_.end > block.timestamp, "Delegatee lock expired");

485:         require(
486:             token.transferFrom(msg.sender, address(this), _value),
487:             "Transfer failed"
488:         );

502:         require(locked_.amount > 0, "No lock");
503:         require(unlock_time > locked_.end, "Only increase lock end");
504:         require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

511:             require(oldUnlockTime > block.timestamp, "Lock expired");

529:         require(locked_.amount > 0, "No lock");
530:         require(locked_.end <= block.timestamp, "Lock not expired");
531:         require(locked_.delegatee == msg.sender, "Lock delegated");

546:         require(token.transfer(msg.sender, value), "Transfer failed");

563:         require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");
564:         require(locked_.amount > 0, "No lock");
565:         require(locked_.delegatee != _addr, "Already delegated");

587:         require(toLocked.amount > 0, "Delegatee has no lock");
588:         require(toLocked.end > block.timestamp, "Delegatee lock expired");
589:         require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");

635:         require(locked_.amount > 0, "No lock");
636:         require(locked_.end > block.timestamp, "Lock expired");
637:         require(locked_.delegatee == msg.sender, "Lock delegated");

657:         require(token.transfer(msg.sender, remainingAmount), "Transfer failed");

676:         require(token.transfer(penaltyRecipient, amount), "Transfer failed");

776:         require(_blockNumber <= block.number, "Only past block number");

877:         require(_blockNumber <= block.number, "Only past block number");

[G-18] ++variable costs less gas than variable++, especially when it’s used in for-loops (--variable/variable-- too)

Prefix increments are cheaper than postfix increments. Saves 6 gas PER LOOP

Findings

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

717:         for (uint256 i = 0; i < 128; i++) {

739:         for (uint256 i = 0; i < 128; i++) {

834:         for (uint256 i = 0; i < 255; i++) {

[G-19] State variables only set in the constructor should be declared immutable

Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas). If getters are still desired, '_' can be added to the variable name and the getter can be added manually

Findings

contracts/VotingEscrow.sol
107:         token = IERC20(_token);

115:         decimals = IERC20(_token).decimals();

118:         name = _name;
119:         symbol = _symbol;

[G-20] lastPoint value rewritten right after initialization

If _epoch > 0, then last_point is rewritten, and initialization on L726 becomes waste of gas;

contracts/VotingEscrow.sol
726:        Point memory last_point = Point({bias: 0, slope: 0, ts: block.timestamp, blk: block.number});
727:        if (_epoch > 0) {
728:            last_point = point_history[_epoch];
729:        }

Mitigation

Point memory last_point = _epoch > 0 ? point_history[_epoch] : Point({bias: 0, slope: 0, ts: block.timestamp, blk: block.number});

[G-21] Obsolete calculations

When we create lock locked_.amount and locked_.delegated are zero, which is ensured by

contracts/VotingEscrow.sol
413:         require(locked_.amount == 0, "Lock exists");

							//can't delegate if lock does not exist
587:         require(toLocked.amount > 0, "Delegatee has no lock");

so instead of

contracts/VotingEscrow.sol
418:         locked_.amount += int128(int256(_value));

should be locked_.amount = int128(int256(_value));

and instead of

contracts/VotingEscrow.sol
420:         locked_.delegated += int128(int256(_value));

should be locked_.delegated = int128(int256(_value));