FIAT DAO veFDT contest - 0x1f8b's results

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L426 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L486 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L546 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L657 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L676

Vulnerability details

Impact

The VotingEscrow contract is incompatible with major tokens such as USDT, BNB or OMG.

Proof of Concept

Some tokens do not return a bool (e.g. USDT, BNB, OMG) on ERC20 methods, (e.g. BNB) may return a bool for some methods, but fail to do so for others. This resulted in stuck BNB tokens in Uniswap v1 (details).

This is the code of USDT:

Reference:

• https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#code

contract ERC20Basic {
    uint public _totalSupply;
    function totalSupply() public constant returns (uint);
    function balanceOf(address who) public constant returns (uint);
+   function transfer(address to, uint value) public;
    event Transfer(address indexed from, address indexed to, uint value);
}

contract ERC20 is ERC20Basic {
    function allowance(address owner, address spender) public constant returns (uint);
+   function transferFrom(address from, address to, uint value) public;
    function approve(address spender, uint value) public;
    event Approval(address indexed owner, address indexed spender, uint value);
}

As you can see, it lacks the return defined in the ERC20 standard. So calling a transfer or transferFrom methods from a require will check for a return that will cause the transaction to fail.

Here you hace a list with tokens (outdated) that don't return a boolean in transfer:

• https://gist.githubusercontent.com/lukas-berlin/f587086f139df93d22987049f3d8ebd2/raw/1f937dc8eb1d6018da59881cbc633e01c0286fb0/Tokens%20missing%20return%20values%20in%20transfer

Recommended Mitigation Steps

• Use safeTransfer and safeTransferFrom

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L418-L420 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L460-L465

Vulnerability details

Impact

Due to an insecure conversion between uint256 and int128, it is possible to get an integer overflow which could be exploited depending on which token, as a result, the user could lose tokens.

Proof of Concept

increaseAmount and createLock have a cast code similar to the following proof of concept:

pragma solidity 0.8.15;

contract TesterA {
    uint256 public MAX= uint256(int256(type(int128).max)); // 170141183460469231731687303715884105727
    function testMe(uint256 _value) external view returns (int128)
    {
        return int128(int256(_value));
    }
}

As you can see, despite using version 8 of solidity, the overflow happens.

Input: "170141183460469231731687303715884105728"
Output: -170141183460469231731687303715884105728

For that reason, if a user calls these methods to create or increase a lock, if he uses a value that produces the overflow, he could end up losing tokens, since the transferFrom of the token would be done, but an incorrect amout will be registered.

Recommended Mitigation Steps

Apply the following changes in both methods, createLock and increaseAmount.

    function createLock(uint256 _value, uint256 _unlockTime)
        external
        override
        nonReentrant
        checkBlocklist
    {
        uint256 unlock_time = _floorToWeek(_unlockTime); // Locktime is rounded down to weeks
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(_value > 0, "Only non zero amount");
+       require(_value <= uint256(int256(type(int128).max)), "Max value reached");

• Low
  • 1. Outdated compiler
  • 2. Lack of checks
  • 3. Prevent multiple block
  • 4. Lack of ACK during owner change
  • 5. Different result of withdraw and quitLock
• Non-Critical
  • 6. Use solidity literals instead of maths
  • 7. Wrong Vyper translation
  • 8. Imprecise require's message

Low

1. Outdated compiler

The pragma version used are:

pragma solidity ^0.8.3;

The minimum required version must be 0.8.16; otherwise, contracts will be affected by the following important bug fixes:

0.8.4:

• ABI Decoder V2: For two-dimensional arrays and specially crafted data in memory, the result of abi.decode can depend on data elsewhere in memory. Calldata decoding is not affected.

0.8.9:

• Immutables: Properly perform sign extension on signed immutables.
• User Defined Value Type: Fix storage layout of user defined value types for underlying types shorter than 32 bytes.

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

Apart from these, there are several minor bug fixes and improvements.

2. Lack of checks

The following methods have a lack checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code for address(0):

• _manager: Blocklist.sol#L15
• _owner, _penaltyRecipient and _token: VotingEscrow.sol#L101-L103
• _addr in updatePenaltyRecipient from VotingEscrow.sol#L153-L157

Also, in the following case it's better to ensure that the argument it's a valid contract using _isContract(_ve):

• _ve: Blocklist.sol#L16
• _addr in transferOwnership, updateBlocklist from VotingEscrow.sol#L139-L150

3. Prevent multiple block

It is possible to call block multiple times by the manager, which would trigger multiple delegations on the line highlighted in green below:

    function forceUndelegate(address _addr) external override {
        require(msg.sender == blocklist, "Only Blocklist");
        LockedBalance memory locked_ = locked[_addr];
        address delegatee = locked_.delegatee;
        int128 value = locked_.amount;

        if (delegatee != _addr && value > 0) {
            LockedBalance memory fromLocked;
            locked_.delegatee = _addr;
            fromLocked = locked[delegatee];
-           _delegate(delegatee, fromLocked, value, LockAction.UNDELEGATE);
+           _delegate(_addr, locked_, value, LockAction.DELEGATE);
        }
    }

Due to the line marked in red in the previous code, an integer overflow would occur that would reverse the transaction, so the issue has been reduced to low, however it is advisable to make the appropriate modifications so that this does not happen and not depend on added protections by the compiler to make our code safe.

Affected source code:

• Blocklist.sol#L27

Recommended changes:

    function block(address addr) external {
        require(msg.sender == manager, "Only manager");
-       require(_isContract(addr), "Only contracts");
+       require(!_blocklist[addr], "Already blocked");
        _blocklist[addr] = true;
        IVotingEscrow(ve).forceUndelegate(addr);
    }

4. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

The problem increases because according to the comment "Owner should always be a timelock contract", It must be verified that it is a contract, and this does not happen, so this could lead to rogue pool attacks.

Affected source code:

• VotingEscrow.sol#L141

5. Different result of withdraw and quitLock

When the withdraw method finished, the stored lock has an empty end, but when the quitLock ends, it remains with the value of the lock end.

Affected source code:

• VotingEscrow.sol#L536
• VotingEscrow.sol#L645

Recommended changes:

    function quitLock() external override nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(locked_.amount > 0, "No lock");
        require(locked_.end > block.timestamp, "Lock expired");
        require(locked_.delegatee == msg.sender, "Lock delegated");
        // Update lock
        uint256 value = uint256(uint128(locked_.amount));
        LockedBalance memory newLocked = _copyLock(locked_);
        newLocked.amount = 0;
-       newLocked.delegated -= int128(int256(value));
        newLocked.delegatee = address(0);
+       newLocked.delegated = 0;
+       newLocked.end = 0;
        locked[msg.sender] = newLocked;
-       newLocked.end = 0;
-       newLocked.delegated = 0;
        ...

---

Non-Critical

6. Use solidity literals instead of maths

Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

Change 10**18 to 1e18:

• VotingEscrow.sol#L48
• VotingEscrow.sol#L51
• VotingEscrow.sol#L653

7. Wrong Vyper translation

As the comment "original concept and implementation in Vyper" says, in vyper it is necessary to specify the length of the arrays, but not solidly, it is better to use a dynamic length instead of the large ones that were chosen.

Affected source code:

• VotingEscrow.sol#L57-L58

Recommended changes:

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

8. Imprecise require's message

The following request appears in the increaseAmount method:

require(locked_.end > block.timestamp, "Lock expired");

But if the lock does not exist, this condition will be false, so the error displayed will be "Lock Expired" for non-existent locks.

Affected source code:

• VotingEscrow.sol#L450

• Gas
  • 1. Outdated compiler
  • 2. > 0 is less efficient than != 0 for unsigned integers
  • 3. Use Custom Errors instead of Revert Strings to save Gas
  • 4. There's no need to set default values for variables
  • 5. ++i costs less gas compared to i++ or i += 1
  • 6. Reduce the size of error messages Long revert Strings
  • 7. Gas saving using immutable
  • 8. Avoid unnecessary casting
  • 9. Change bool to uint256 can save gas
  • 10. Reduce math operations
  • 11. Reorder logic
  • 12. Optimize withdraw
  • 13. Optimize quitLock
  • 14. Create a casted constant
  • 15. Use unckecked region
  • 16. Shift right instead of dividing by 2

Gas

1. Outdated compiler

pragma solidity ^0.8.3;

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.

Solidity 0.8.10 has a useful change which reduced gas costs of external calls which expect a return value:

• https://blog.soliditylang.org/2021/11/09/solidity-0.8.10-release-announcement/

According to the release note of 0.8.15:

• https://blog.soliditylang.org/2022/06/15/solidity-0.8.15-release-announcement/

The benchmark shows saving of 2.5-10% Bytecode size, saving 2-8% Deployment gas and saving up to 6.2% Runtime gas.

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

Although it would appear that > 0 is less expensive than != 0, this is only true in the absence of the optimizer and outside of a need statement. Gas will be saved if the optimizer is enabled at 10k and you're in a require statement.

Reference:

• https://twitter.com/gzeon/status/1485428085885640706

Affected source code:

• VotingEscrow.sol#L412
• VotingEscrow.sol#L448
• VotingEscrow.sol#L449
• VotingEscrow.sol#L469
• VotingEscrow.sol#L502
• VotingEscrow.sol#L529
• VotingEscrow.sol#L564
• VotingEscrow.sol#L587
• VotingEscrow.sol#L635

3. Use 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)

Source Custom Errors in Solidity:

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

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testRevert(bool path) public view {
  require(path, "test error");
}
}

contract TesterB {
error MyError(string msg);

function testError(bool path) public view {
   if(path) revert MyError("test error");
}
}

Gas saving executing: 9 per entry

TesterA.testRevert: 21611 
TesterB.testError:  21602     

Affected source code:

• Blocklist.sol#L24
• Blocklist.sol#L25
• Authorizable.sol#L19
• Authorizable.sol#L25
• ERC20Permit.sol#L104
• ERC20Permit.sol#L114
• ERC20Permit.sol#L216
• ERC20Permit.sol#L218
• ERC20Permit.sol#L220-223
• ERC20Permit.sol#L225-229
• ReentrancyBlock.sol#L10
• VotingEscrow.sol#L116
• VotingEscrow.sol#L125-128
• VotingEscrow.sol#L140
• VotingEscrow.sol#L147
• VotingEscrow.sol#L154
• VotingEscrow.sol#L162
• VotingEscrow.sol#L171
• VotingEscrow.sol#L412
• VotingEscrow.sol#L413
• VotingEscrow.sol#L414
• VotingEscrow.sol#L415
• VotingEscrow.sol#L416
• VotingEscrow.sol#L425-428
• VotingEscrow.sol#L448
• VotingEscrow.sol#L449
• VotingEscrow.sol#L450
• VotingEscrow.sol#L469
• VotingEscrow.sol#L470
• VotingEscrow.sol#L485-488
• VotingEscrow.sol#L502
• VotingEscrow.sol#L503
• VotingEscrow.sol#L504
• VotingEscrow.sol#L511
• VotingEscrow.sol#L529
• VotingEscrow.sol#L530
• VotingEscrow.sol#L531
• VotingEscrow.sol#L546
• VotingEscrow.sol#L563
• VotingEscrow.sol#L564
• VotingEscrow.sol#L565
• VotingEscrow.sol#L587
• VotingEscrow.sol#L588
• VotingEscrow.sol#L589
• VotingEscrow.sol#L635
• VotingEscrow.sol#L636
• VotingEscrow.sol#L637
• VotingEscrow.sol#L657
• VotingEscrow.sol#L676
• VotingEscrow.sol#L776
• VotingEscrow.sol#L877

4. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testInit() public view returns (uint) { uint a = 0; return a; }
}

contract TesterB {
function testNoInit() public view returns (uint) { uint a; return a; }
}

Gas saving executing: 8 per entry

TesterA.testInit:   21392
TesterB.testNoInit: 21384   

Affected source code:

• VotingEscrow.sol#L229
• VotingEscrow.sol#L230
• VotingEscrow.sol#L298
• VotingEscrow.sol#L313
• VotingEscrow.sol#L714
• VotingEscrow.sol#L737
• VotingEscrow.sol#L793
• VotingEscrow.sol#L794
• VotingEscrow.sol#L836
• VotingEscrow.sol#L889

5. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• ERC20Permit.sol#L231
• VotingEscrow.sol#L309
• VotingEscrow.sol#L717
• VotingEscrow.sol#L739
• VotingEscrow.sol#L834

6. Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testShortRevert(bool path) public view {
  require(path, "test error");
}
}

contract TesterB {
function testLongRevert(bool path) public view {
  require(path, "test big error message, more than 32 bytes");
}
}

Gas saving executing: 18 per entry

TesterA.testShortRevert: 21886   
TesterB.testLongRevert:  21904       

Affected source code:

• ERC20Permit.sol#L225-229

7. Gas saving using immutable

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

It's also better to remove the initial values, because they will be set during the constructor.

Affected source code:

• Blocklist.sol#L11
• Blocklist.sol#L12
• VotingEscrow.sol#L45
• VotingEscrow.sol#L64-L66

8. Avoid unnecessary casting

The stored variable ve is always casted to IVotingEscrow when it's used, so it's cheaper to use IVotingEscrow as type and check that this address is a contract during the constructor's logic.

Affected source code:

• Blocklist.sol#L12-L14

Recommended changes:

+   IVotingEscrow public ve;
-   address public ve;

+   constructor(address _manager, IVotingEscrow _ve) {
-   constructor(address _manager, address _ve) {
        manager = _manager;
        ve = _ve;
    }

    /// @notice Add address to blocklist
    /// @dev only callable by owner.
    /// @dev Allows blocklisting only of smart contracts
    /// @param addr The contract address to blocklist
    function block(address addr) external {
        require(msg.sender == manager, "Only manager");
        require(_isContract(addr), "Only contracts");
        _blocklist[addr] = true;
+       ve.forceUndelegate(addr);
-       IVotingEscrow(ve).forceUndelegate(addr);
    }

We have a similar situation in VotingEscrow with the argument token, we can use the type IERC20 instead of cast the value.

Affected source code:

• VotingEscrow.sol#L100-L115

Recommended changes:

    constructor(
        address _owner,
        address _penaltyRecipient,
+       IERC20 _token,
-       address _token,
        string memory _name,
        string memory _symbol
    ) {
+       token = _token;
-       token = IERC20(_token);
        pointHistory[0] = Point({
            bias: int128(0),
            slope: int128(0),
            ts: block.timestamp,
            blk: block.number
        });

+       decimals = _token.decimals();
-       decimals = IERC20(_token).decimals();

We have a similar situation in VotingEscrow with the value blocklist stored, we can use the type IBlocklist instead of address and avoid multiple conversions.

Affected source code:

• VotingEscrow.sol#L53

Recommended changes:

-   event UpdateBlocklist(address blocklist);
+   event UpdateBlocklist(IBlocklist blocklist);

    ...

-   address public blocklist;
+   IBlocklist public blocklist;

    ...

    modifier checkBlocklist() {
        require(
+           !blocklist.isBlocked(msg.sender),
-           !IBlocklist(blocklist).isBlocked(msg.sender),
            "Blocked contract"
        );
        _;
    }

    ...

    /// @notice Updates the blocklist contract
+   function updateBlocklist(IBlocklist _addr) external {
-   function updateBlocklist(address _addr) external {
        require(msg.sender == owner, "Only owner");
        blocklist = _addr;
        emit UpdateBlocklist(_addr);
    }

In VotingEscrow.withdraw method the locked.amount is casted to uint256, but also this cast is reversed again to obtain the original value.

Affected source code:

• VotingEscrow.sol#L537

Recommended changes:

    function withdraw() external override nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(locked_.amount > 0, "No lock");
        require(locked_.end <= block.timestamp, "Lock not expired");
        require(locked_.delegatee == msg.sender, "Lock delegated");
        // Update lock
-       uint256 value = uint256(uint128(locked_.amount));
        LockedBalance memory newLocked = _copyLock(locked_);
        newLocked.amount = 0;
        newLocked.end = 0;
+       newLocked.delegated -= locked_.amount;
-       newLocked.delegated -= int128(int256(value));
        newLocked.delegatee = address(0);
        locked[msg.sender] = newLocked;
        newLocked.delegated = 0;
        // oldLocked can have either expired <= timestamp or zero end
        // currentLock has only 0 end
        // Both can have >= 0 amount
        _checkpoint(msg.sender, locked_, newLocked);
        // Send back deposited tokens
+       uint256 value = uint256(uint128(locked_.amount));
        require(token.transfer(msg.sender, value), "Transfer failed");
        emit Withdraw(msg.sender, value, LockAction.WITHDRAW, block.timestamp);
    }

9. Change bool to uint256 can save gas

Because each write operation requires an additional SLOAD to read the slot's contents, replace the bits occupied by the boolean, and then write back, booleans are more expensive than uint256 or any other type that uses a complete word. This cannot be turned off because it is the compiler's defense against pointer aliasing and contract upgrades.

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol#L23-L27

Affected source code:

• Blocklist.sol#L10

Recommended changes:

+   mapping(address => uint) private _blocklist;
-   mapping(address => bool) private _blocklist;
    address public manager;
    address public ve;

    constructor(address _manager, address _ve) {
        manager = _manager;
        ve = _ve;
    }

    /// @notice Add address to blocklist
    /// @dev only callable by owner.
    /// @dev Allows blocklisting only of smart contracts
    /// @param addr The contract address to blocklist
    function block(address addr) external {
        require(msg.sender == manager, "Only manager");
        require(_isContract(addr), "Only contracts");
+       _blocklist[addr] = 1;
-       _blocklist[addr] = true;
        IVotingEscrow(ve).forceUndelegate(addr);
    }

    /// @notice Check an address
    /// @dev This method will be called by the VotingEscrow contract
    /// @param addr The contract address to check
    function isBlocked(address addr) public view returns (bool) {
+       return _blocklist[addr] == 1;
-       return _blocklist[addr];
    }

10. Reduce math operations

Because the result of the following operation always will be one, it's cheaper to avoid the sum.

            if (uEpoch == 0) {
-               userPointHistory[_addr][uEpoch + 1] = userOldPoint;
+               userPointHistory[_addr][1] = userOldPoint;
            }

• VotingEscrow.sol#L257-L259

Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

Change 10**18 to 1e18:

• VotingEscrow.sol#L48
• VotingEscrow.sol#L51
• VotingEscrow.sol#L653

11. Reorder logic

It is convenient to do the require based on the variables that we have, and if we need to check against any state variable, do it at the end.

Recommended changes:

• VotingEscrow.sol#L402-L416

    // See IVotingEscrow for documentation
    function createLock(uint256 _value, uint256 _unlockTime)
        external
        override
        nonReentrant
        checkBlocklist
    {
+       require(_value > 0, "Only non zero amount");
        uint256 unlock_time = _floorToWeek(_unlockTime); // Locktime is rounded down to weeks
+       require(unlock_time > block.timestamp, "Only future lock end");
+       require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
-       require(_value > 0, "Only non zero amount");
        require(locked_.amount == 0, "Lock exists");
        require(unlock_time >= locked_.end, "Only increase lock end"); // from using quitLock, user should increaseAmount instead
-       require(unlock_time > block.timestamp, "Only future lock end");
-       require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");

• VotingEscrow.sol#L440-L449

    function increaseAmount(uint256 _value)
        external
        override
        nonReentrant
        checkBlocklist
    {
+       require(_value > 0, "Only non zero amount");
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
-       require(_value > 0, "Only non zero amount");
        require(locked_.amount > 0, "No lock");

• VotingEscrow.sol#L499-L504

+       // Validate inputs
-       LockedBalance memory locked_ = locked[msg.sender];
        uint256 unlock_time = _floorToWeek(_unlockTime); // Locktime is rounded down to weeks
-       // Validate inputs
-       require(locked_.amount > 0, "No lock");
        require(unlock_time > locked_.end, "Only increase lock end");
        require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
+       LockedBalance memory locked_ = locked[msg.sender];
+       require(locked_.amount > 0, "No lock");

12. Optimize withdraw

The withdraw method is for all the amount, so it's cheaper and not required to copy the previous lock and subtract the amount for newLocked.

Affected source code:

• VotingEscrow.sol#L534-L540

Recommended changes:

    function withdraw() external override nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(locked_.amount > 0, "No lock");
        require(locked_.end <= block.timestamp, "Lock not expired");
        require(locked_.delegatee == msg.sender, "Lock delegated");
        // Update lock
        uint256 value = uint256(uint128(locked_.amount));
+       LockedBalance memory newLocked; // empty
-       LockedBalance memory newLocked = _copyLock(locked_);
-       newLocked.amount = 0;
-       newLocked.end = 0;
-       newLocked.delegated -= int128(int256(value));
-       newLocked.delegatee = address(0);
        locked[msg.sender] = newLocked;
-       newLocked.delegated = 0;
        // oldLocked can have either expired <= timestamp or zero end
        // currentLock has only 0 end
        // Both can have >= 0 amount
        _checkpoint(msg.sender, locked_, newLocked);
        // Send back deposited tokens
        require(token.transfer(msg.sender, value), "Transfer failed");
        emit Withdraw(msg.sender, value, LockAction.WITHDRAW, block.timestamp);
    }

13. Optimize quitLock

The quitLock method is for all the amount, so it's cheaper and not required to copy the previous lock and subtract the amount for newLocked. Also it will fix the issue with the end that was stored with the copied value.

Affected source code:

• VotingEscrow.sol#L534-L540

Recommended changes:

    function quitLock() external override nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(locked_.amount > 0, "No lock");
        require(locked_.end > block.timestamp, "Lock expired");
        require(locked_.delegatee == msg.sender, "Lock delegated");
        // Update lock
        uint256 value = uint256(uint128(locked_.amount));
+       LockedBalance memory newLocked; // empty
-       LockedBalance memory newLocked = _copyLock(locked_);
-       newLocked.amount = 0;
-       newLocked.delegated -= int128(int256(value));
-       newLocked.delegatee = address(0);
        locked[msg.sender] = newLocked;
-       newLocked.end = 0;
-       newLocked.delegated = 0;
        // oldLocked can have either expired <= timestamp or zero end
        // currentLock has only 0 end
        // Both can have >= 0 amount
        _checkpoint(msg.sender, locked_, newLocked);
        // apply penalty
        uint256 penaltyRate = _calculatePenaltyRate(locked_.end);
        uint256 penaltyAmount = (value * penaltyRate) / 10**18; // quitlock_penalty is in 18 decimals precision
        penaltyAccumulated += penaltyAmount;
        uint256 remainingAmount = value - penaltyAmount;
        // Send back remaining tokens
        require(token.transfer(msg.sender, remainingAmount), "Transfer failed");
        emit Withdraw(msg.sender, value, LockAction.QUIT, block.timestamp);
    }

14. Create a casted constant

VotingEscrow.MAXTIME is casted to int128 in two times during the logic of VotingEscrow._checkpoint. It is cheaper to store this value in a constant to reduce the opcodes needed to output the value..

int128 public constant MAXTIME_i128 = int128(int256(MAXTIME));

Affected source code:

• VotingEscrow.sol#L239
• VotingEscrow.sol#L247

15. Use unckecked region

It's possible to save gas using the unckecked keyword around a math operation. This will avoid the required checks to ensure that the variable won't overflow.

Reference:

• https://docs.soliditylang.org/en/v0.8.0/control-structures.html#checked-or-unchecked-arithmetic

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testUnChecked(uint a, uint b) public returns (uint) { unchecked { return a - b; } }
}

contract TesterB {
function testChecked(uint a, uint b) public returns (uint) { return a - b; }
}

Gas saving executing: 182 per entry

TesterA.testUnChecked:  22103
TesterB.testChecked:    22285   

Affected source code:

end is always greater than block.timestamp:

• VotingEscrow.sol#L242
• VotingEscrow.sol#L250

block.timestamp is always greater than lastPoint.ts:

• VotingEscrow.sol#L302

Because it was divided first, it can never overflow:

• VotingEscrow.sol#L702

Because targetEpoch < epoch, targetEpoch + 1 can never overflow:

• VotingEscrow.sol#L891

16. Shift right instead of dividing by 2

Shifting one to the right will calculate a division by two.

he SHR opcode only requires 3 gas, compared to the DIV opcode's consumption of 5. Additionally, shifting is used to get around Solidity's division operation's division-by-0 prohibition.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
    function testDiv(uint a) public returns (uint) { return a / 2; }
}

contract TesterB {
    function testShift(uint a) public returns (uint) { return a >> 1; }
}

Gas saving executing: 172 per entry

TesterA.testDiv:    21965 
TesterB.testShift:  21793    

Affected source code:

• VotingEscrow.sol#L719
• VotingEscrow.sol#L743