FIAT DAO veFDT contest - Dravee's results

Overview

Table of Contents

• 1. Low Risk Issues
  • 1.1. Known vulnerabilities exist in currently used @openzeppelin/contracts version
  • 1.2. Prevent accidentally burning tokens
  • 1.3. Use a 2-step ownership transfer pattern
  • 1.4. ERC20 decimals size
• 2. Non-Critical Issues
  • 2.1. Related mappings should be grouped in a struct
  • 2.2. 1000000000000000000 should be changed to 1e18 for readability reasons (and 1000000000 to 1e9)
  • 2.3. Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)
  • 2.4. Non-library/interface files should use fixed compiler versions, not floating ones

1. Low Risk Issues

1.1. Known vulnerabilities exist in currently used @openzeppelin/contracts version

As some known vulnerabilities exist in the current @openzeppelin/contracts version, consider updating package.json with at least @openzeppelin/contracts@4.7.3 here:

• https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/package.json#L42

File: package.json
42:     "@openzeppelin/contracts": "^4.4.2",

While vulnerabilities are known, the current scope isn't affected (this might not hold true for the whole solution)

1.2. Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

As penaltyRecipient can be set to 0:

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

Consider adding a check to prevent accidentally burning tokens here, where it's using the state variable penaltyRecipient:

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

1.3. Use a 2-step ownership transfer pattern

Contracts inheriting from OpenZeppelin's libraries have the default transferOwnership() function (a one-step process). It's possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role. Consider overriding the default transferOwnership() function to first nominate an address as the pendingOwner and implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.

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

1.4. ERC20 decimals size

The EIP20 specification optionally defines a uint8 decimals field. However, the corresponding field here is a uint256. This is not compliant with the specification and may cause confusion when interacting with wallets and dApps. Consider setting the decimals type to uint8.

VotingEscrow.sol:66:    uint256 public decimals = 18;
VotingEscrow.sol:115:        decimals = IERC20(_token).decimals(); //@audit this returns a uint8

2. Non-Critical Issues

2.1. Related mappings should be grouped in a struct

The "userPoint" mappings should be grouped in a struct.

From:

VotingEscrow.sol:58:    mapping(address => Point[1000000000]) public userPointHistory;
VotingEscrow.sol:59:    mapping(address => uint256) public userPointEpoch;

To

    struct UserPointInfo {
        Point[1000000000] history;   
        uint256 epoch;   
    }

    mapping(address => UserPointInfo) public userPointInfo;

It would be less error-prone, more readable, and it would be possible to delete all related fields with a simple delete userPointInfo[address].

2.2. 1000000000000000000 should be changed to 1e18 for readability reasons (and 1000000000 to 1e9)

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

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

VotingEscrow.sol:48:    uint256 public constant MULTIPLIER = 10**18;
VotingEscrow.sol:51:    uint256 public maxPenalty = 10**18; // penalty for quitters with MAXTIME remaining lock
VotingEscrow.sol:653:        uint256 penaltyAmount = (value * penaltyRate) / 10**18; // quitlock_penalty is in 18 decimals precision

2.4. Non-library/interface files should use fixed compiler versions, not floating ones

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

Overview

Table of Contents:

• 1. State variables only set in the constructor should be declared immutable
• 2. Tightly packing struct LockedBalance for a cheaper copy in memory
• 3. decimals should be uint8 instead of uint256 and tightly packed
• 4. Caching storage values in memory
• 5. userOldPoint is overwritten
• 6. Reduce the size of error messages (Long revert Strings)
• 7. Duplicated conditions should be refactored to a modifier or function to save deployment costs
• 8. Multiple address mappings can be combined in a struct where relevant
• 9. Pre-Solidity 0.8.13: > 0 is less efficient than != 0 for unsigned integers
• 10. Using private rather than public for constants saves gas
• 11. Use shift right/left instead of division/multiplication if possible
• 12. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)
• 13. Increments/decrements can be unchecked in for-loops
• 14. It costs more gas to initialize variables with their default value than letting the default value be applied
• 15. Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)
• 16. Upgrade pragma
• 17. Use Custom Errors instead of Revert Strings to save Gas
• 18. (Not recommended, but true) Functions guaranteed to revert when called by normal users can be marked payable

1. State variables only set in the constructor should be declared immutable

Variables only set in the constructor and never edited afterwards should be marked as immutable, as it would avoid the expensive storage-writing operation in the constructor (around 20 000 gas per variable) and replace the expensive storage-reading operations (around 2100 gas per reading) to a less expensive value reading (3 gas)

• manager, ve

File: Blocklist.sol
09: contract Blocklist {
10:     mapping(address => bool) private _blocklist;
11:     address public manager;
12:     address public ve;
13: 
14:     constructor(address _manager, address _ve) {
15:         manager = _manager;
16:         ve = _ve;
17:     }

2. Tightly packing struct LockedBalance for a cheaper copy in memory

When copying a state struct in memory, there are as many SLOADs and MSTOREs as there are slots.

At multiple places, a LockedBalance state variable is copied in memory:

VotingEscrow.sol:410:        LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol:446:        LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol:499:        LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol:527:        LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol:561:        LockedBalance memory locked_ = locked[msg.sender];
VotingEscrow.sol:633:        LockedBalance memory locked_ = locked[msg.sender];

Here, the cost of a copy in memory is 4 SLOADs, as the struct takes 4 SLOTS:

File: VotingEscrow.sol
75:     struct LockedBalance {
76:         int128 amount; //@audit 16 bytes size (SLOT 1)
77:         uint256 end; //@audit 32 bytes size (SLOT 2)
78:         int128 delegated; //@audit 16 bytes size (SLOT 3)
79:         address delegatee; //@audit 20 bytes size (SLOT 4, as 16+20 == 36 > 32)
80:     }

To save 1 SLOAD on these operations, it would be a good idea to tightly pack the struct:

File: VotingEscrow.sol
75:     struct LockedBalance {
76:         int128 amount; //@audit 16 bytes size (SLOT 1)
78:         int128 delegated; //@audit 16 bytes size (SLOT 1 as 16+16 == 32)
77:         uint256 end; //@audit 32 bytes size (SLOT 2)
79:         address delegatee; //@audit 20 bytes size (SLOT 3)
80:     }

3. decimals should be uint8 instead of uint256 and tightly packed

The EIP20 specification optionally defines a uint8 decimals field. However, the corresponding field here is a uint256. This is not compliant with the specification and may cause confusion when interacting with wallets and dApps. Consider setting the decimals type to uint8.

contracts/VotingEscrow.sol:
   66:     uint256 public decimals = 18; //@audit should be uint8
  115:         decimals = IERC20(_token).decimals(); //@audit this function most likely returns a uint8 

With this change, it'll then be possible to save 1 storage SLOT:

File: VotingEscrow.sol
44:     // Shared global state
45:     IERC20 public token;
46:     uint256 public constant WEEK = 7 days;
47:     uint256 public constant MAXTIME = 365 days;
48:     uint256 public constant MULTIPLIER = 10**18;
49:     address public owner;
50:     address public penaltyRecipient; // receives collected penalty payments
51:     uint256 public maxPenalty = 10**18; // penalty for quitters with MAXTIME remaining lock
52:     uint256 public penaltyAccumulated; // accumulated and unwithdrawn penalty payments
53:     address public blocklist;
54: 
+ 55:     // Voting token
+ 56:     uint8 public decimals = 18; //@audit packed with address (20 bytes + 1 byte = 21 bytes < 32 bytes)
+ 57:     string public name;
+ 58:     string public symbol;
+ 59:     
55:     // Lock state
56:     uint256 public globalEpoch;
57:     Point[1000000000000000000] public pointHistory; // 1e9 * userPointHistory-length, so sufficient for 1e9 users
58:     mapping(address => Point[1000000000]) public userPointHistory;
59:     mapping(address => uint256) public userPointEpoch;
60:     mapping(uint256 => int128) public slopeChanges;
61:     mapping(address => LockedBalance) public locked;
62: 
- 63:     // Voting token
- 64:     string public name;
- 65:     string public symbol;
- 66:     uint256 public decimals = 18; //@audit should be uint8

4. Caching storage values in memory

The code can be optimized by minimizing the number of SLOADs.

SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.

• Favor the happy path here to save 1 SLOAD

File: VotingEscrow.sol
673:     function collectPenalty() external {
674:         uint256 amount = penaltyAccumulated;
675:         penaltyAccumulated = 0;
- 676:         require(token.transfer(penaltyRecipient, amount), "Transfer failed");
- 677:         emit CollectPenalty(amount, penaltyRecipient);
+ 676:         address _penaltyRecipient = penaltyRecipient;
+ 676:         require(token.transfer(_penaltyRecipient, amount), "Transfer failed");
+ 677:         emit CollectPenalty(amount, _penaltyRecipient);
678:     }

5. userOldPoint is overwritten

SSTOREs are expensive.

Here, under the uEpoch == 0 condition, 2 SSTOREs are made L258 and L264:

File: VotingEscrow.sol
253:             // Moved from bottom final if statement to resolve stack too deep err
254:             // start {
255:             // Now handle user history
256:             uint256 uEpoch = userPointEpoch[_addr];
257:             if (uEpoch == 0) {
258:                 userPointHistory[_addr][uEpoch + 1] = userOldPoint; //@audit what's even the point of the SSTORE here? It'll get overridden anyway L264. The impact might be big 
259:             }
260: 
261:             userPointEpoch[_addr] = uEpoch + 1;
262:             userNewPoint.ts = block.timestamp;
263:             userNewPoint.blk = block.number;
264:             userPointHistory[_addr][uEpoch + 1] = userNewPoint; 

While this does indeed seem like an error in logic that might have a deeper impact (userOldPoint is never persisted and never used, this doesn't feel intended), it still consumes 2 SSTOREs Consider using a ternary operator L264:

userPointHistory[_addr][uEpoch + 1] = uEpoch == 0 ? userOldPoint : userNewPoint; 

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.

Revert string > 32 bytes:

File: ERC20Permit.sol
225:         require(
226:             uint256(s) <=
227:                 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0,
228:             "ERC20: invalid signature 's' value" //@audit 34 bytes
229:         );

Consider shortening the revert string to fit in 32 bytes.

7. Duplicated conditions should be refactored to a modifier or function to save deployment costs

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:412:        require(_value > 0, "Only non zero amount");
VotingEscrow.sol:448:        require(_value > 0, "Only non zero amount");

VotingEscrow.sol:414:        require(unlock_time >= locked_.end, "Only increase lock end"); // from using quitLock, user should increaseAmount instead
VotingEscrow.sol:503:        require(unlock_time > locked_.end, "Only increase lock end");

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

VotingEscrow.sol:449:        require(locked_.amount > 0, "No lock");
VotingEscrow.sol:502:        require(locked_.amount > 0, "No lock");
VotingEscrow.sol:529:        require(locked_.amount > 0, "No lock");
VotingEscrow.sol:564:        require(locked_.amount > 0, "No lock");
VotingEscrow.sol:635:        require(locked_.amount > 0, "No lock");

VotingEscrow.sol:469:            require(locked_.amount > 0, "Delegatee has no lock");
VotingEscrow.sol:587:        require(toLocked.amount > 0, "Delegatee has no lock");

VotingEscrow.sol:450:        require(locked_.end > block.timestamp, "Lock expired");
VotingEscrow.sol:470:            require(locked_.end > block.timestamp, "Delegatee lock expired");
VotingEscrow.sol:636:        require(locked_.end > block.timestamp, "Lock expired");

VotingEscrow.sol:531:        require(locked_.delegatee == msg.sender, "Lock delegated");
VotingEscrow.sol:637:        require(locked_.delegatee == msg.sender, "Lock delegated");

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

8. Multiple address mappings can be combined in a struct where relevant

Computing storage costs ~42 gas, and this could be saved per access due to not having to recalculate the key's keccak256 hash:

VotingEscrow.sol:58:    mapping(address => Point[1000000000]) public userPointHistory;
VotingEscrow.sol:59:    mapping(address => uint256) public userPointEpoch;

9. Pre-Solidity 0.8.13: > 0 is less efficient than != 0 for unsigned integers

Up until Solidity 0.8.13: != 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 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

Consider changing > 0 with != 0 here:

VotingEscrow.sol:412:        require(_value > 0, "Only non zero amount");
VotingEscrow.sol:448:        require(_value > 0, "Only non zero amount");
VotingEscrow.sol:449:        require(locked_.amount > 0, "No lock");
VotingEscrow.sol:469:            require(locked_.amount > 0, "Delegatee has no lock");
VotingEscrow.sol:502:        require(locked_.amount > 0, "No lock");
VotingEscrow.sol:529:        require(locked_.amount > 0, "No lock");
VotingEscrow.sol:564:        require(locked_.amount > 0, "No lock");
VotingEscrow.sol:587:        require(toLocked.amount > 0, "Delegatee has no lock");
VotingEscrow.sol:635:        require(locked_.amount > 0, "No lock");

Also, please enable the Optimizer.

10. 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, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table.

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

11. Use shift right/left instead of division/multiplication if possible

While the DIV / MUL opcode uses 5 gas, the SHR / SHL opcode only uses 3 gas. Furthermore, beware that Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting. Eventually, overflow checks are never performed for shift operations as they are done for arithmetic operations. Instead, the result is always truncated, so the calculation can be unchecked in Solidity version 0.8+

• Use >> 1 instead of / 2

Affected code (saves around 2 gas + 20 for unchecked per instance):

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

12. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

In the pre-increment case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2.

Affected code:

libraries/ERC20Permit.sol:231:        nonces[owner]++;
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++) {

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

13. Increments/decrements can be unchecked in for-loops

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Consider wrapping with an unchecked block here (around 25 gas saved per instance):

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

The change would be:

- for (uint256 i; i < numIterations; i++) {
+ for (uint256 i; i < numIterations;) {
 // ...  
+   unchecked { ++i; }
}  

The same can be applied with decrements (which should use break when i == 0).

The risk of overflow is non-existent for uint256 here.

14. It costs more gas to initialize variables with their default value than letting the default value be applied

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 (around 3 gas per instance).

Affected code:

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

Consider removing explicit initializations for default values.

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

VotingEscrow.sol:48:    uint256 public constant MULTIPLIER = 10**18;
VotingEscrow.sol:51:    uint256 public maxPenalty = 10**18; // penalty for quitters with MAXTIME remaining lock
VotingEscrow.sol:653:        uint256 penaltyAmount = (value * penaltyRate) / 10**18; // quitlock_penalty is in 18 decimals precision

16. Upgrade pragma

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

The advantages here are:

• Custom errors (>= 0.8.4): cheaper deployment cost and runtime cost. Note: the runtime cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.
• Contract existence checks (>= 0.8.10): external calls skip contract existence checks if the external call has a return value

Consider upgrading here :

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

17. Use Custom Errors instead of Revert Strings to save Gas

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

Additionally, custom errors can be used inside and outside of contracts (including interfaces and libraries).

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

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.

Consider replacing all revert strings with custom errors in the solution, and particularly those that have multiple occurrences:

features/Blocklist.sol:24:        require(msg.sender == manager, "Only manager");
features/Blocklist.sol:25:        require(_isContract(addr), "Only contracts");
libraries/Authorizable.sol:19:        require(msg.sender == owner, "Sender not owner");
libraries/Authorizable.sol:25:        require(isAuthorized(msg.sender), "Sender not Authorized");
libraries/ERC20Permit.sol:104:        require(balance >= amount, "ERC20: insufficient-balance");
libraries/ERC20Permit.sol:114:                require(allowed >= amount, "ERC20: insufficient-allowance");
libraries/ERC20Permit.sol:216:        require(owner != address(0), "ERC20: invalid-address-0");
libraries/ERC20Permit.sol:218:        require(owner == ecrecover(digest, v, r, s), "ERC20: invalid-permit");
libraries/ERC20Permit.sol:220:        require(
libraries/ERC20Permit.sol:225:        require(
libraries/ReentrancyBlock.sol:10:        require(!_entered, "Reentrancy");
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");

18. (Not recommended, but true) 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.

libraries/Authorizable.sol:38:    function authorize(address who) external onlyOwner() {
libraries/Authorizable.sol:44:    function deauthorize(address who) external onlyOwner() {
libraries/Authorizable.sol:50:    function setOwner(address who) public onlyOwner() {
libraries/ERC20PermitWithMint.sol:29:    function mint(address account, uint256 amount) external onlyOwner {
libraries/ERC20PermitWithMint.sol:49:    function burn(address account, uint256 amount) external onlyOwner {