Platform: Code4rena
Start Date: 12/08/2022
Pot Size: $35,000 USDC
Total HM: 10
Participants: 126
Period: 3 days
Judge: Justin Goro
Total Solo HM: 3
Id: 154
League: ETH
Rank: 24/126
Findings: 2
Award: $211.14
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: oyc_109
Also found by: 0x1f8b, 0x52, 0xDjango, 0xLovesleep, 0xNazgul, 0xNineDec, 0xbepresent, 0xmatt, 0xsolstars, Aymen0909, Bahurum, Bnke0x0, CertoraInc, Chom, CodingNameKiki, DecorativePineapple, Deivitto, Dravee, ElKu, Funen, GalloDaSballo, IllIllI, JC, JohnSmith, Junnon, KIntern_NA, Lambda, LeoS, MiloTruck, Noah3o6, PaludoX0, RedOneN, Respx, ReyAdmirado, Rohan16, RoiEvenHaim, Rolezn, Ruhum, Sm4rty, TomJ, Vexjon, Waze, Yiko, __141345__, a12jmx, ajtra, ak1, apostle0x01, asutorufos, auditor0517, bin2chen, bobirichman, brgltd, bulej93, byndooa, c3phas, cRat1st0s, cryptphi, csanuragjain, d3e4, defsec, delfin454000, djxploit, durianSausage, ellahi, erictee, exd0tpy, fatherOfBlocks, gogo, jonatascm, ladboy233, medikko, mics, natzuu, neumo, p_crypt0, paribus, pfapostol, rbserver, reassor, ret2basic, robee, rokinot, rvierdiiev, sach1r0, saneryee, seyni, sikorico, simon135, sseefried, wagmi, wastewa
64.8414 USDC - $64.84
| Issue | Instances | |
|---|---|---|
| [L‑01] | Blocklist's ve address may not match the VotingEscrow with which it's being used | 1 |
| [L‑02] | Only a billion checkpoints available | 1 |
| [L‑03] | Missing checks for address(0x0) when assigning values to address state variables | 7 |
Total: 9 instances over 3 issues
| Issue | Instances | |
|---|---|---|
| [N‑01] | Invalid/extraneous/optional function definitions in interface | 2 |
| [N‑02] | public functions not called by the contract should be declared external instead | 1 |
| [N‑03] | Non-assembly method available | 1 |
| [N‑04] | constants should be defined rather than using magic numbers | 6 |
| [N‑05] | Large multiples of ten should use scientific notation (e.g. 1e6) rather than decimal literals (e.g. 1000000), for readability | 2 |
| [N‑06] | Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18) | 3 |
| [N‑07] | Non-library/interface files should use fixed compiler versions, not floating ones | 2 |
| [N‑08] | Typos | 4 |
| [N‑09] | File is missing NatSpec | 1 |
| [N‑10] | NatSpec is incomplete | 4 |
| [N‑11] | Event is missing indexed fields | 8 |
| [N‑12] | Not using the named return variables anywhere in the function is confusing | 3 |
| [N‑13] | Duplicated require()/revert() checks should be refactored to a modifier or function | 8 |
Total: 45 instances over 13 issues
Blocklist's ve address may not match the VotingEscrow with which it's being usedThe Blocklist is constructed with an address for the VotingEscrow with which it's meant to be used. The VotingEscrow contract itself only allows its stored Blocklist address to call forceUndelegate(), and if the wrong VotingEscrow is passed to the Blocklist, or the wrong Blocklist is passed to updateBlocklist(), block() will revert when called, and another timelock duration will have to be waited for in order to change to the correct Blocklist
There is 1 instance of this issue:
File: /contracts/features/Blocklist.sol 23 function block(address addr) external { 24 require(msg.sender == manager, "Only manager"); 25 require(_isContract(addr), "Only contracts"); 26 _blocklist[addr] = true; 27 IVotingEscrow(ve).forceUndelegate(addr); 28: }
A user can only have a billion checkpoints which, if the user is a DAO, may cause issues down the line, especially if the last checkpoint involved delegating and can thereafter not be undone
There is 1 instance of this issue:
File: /contracts/VotingEscrow.sol 58: mapping(address => Point[1000000000]) public userPointHistory;
address(0x0) when assigning values to address state variablesThere are 7 instances of this issue:
File: contracts/features/Blocklist.sol 15: manager = _manager; 16: ve = _ve;
File: contracts/VotingEscrow.sol 120: owner = _owner; 121: penaltyRecipient = _penaltyRecipient; 141: owner = _addr; 148: blocklist = _addr; 155: penaltyRecipient = _addr;
There are 2 instances of this issue:
File: contracts/interfaces/IERC20.sol /// @audit symbol() isn't defined with those arguments in the standard IERC20 definition 5: function symbol() external view returns (string memory); /// @audit decimals() isn't defined with those arguments in the standard IERC20 definition 10: function decimals() external view returns (uint8);
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
There is 1 instance of this issue:
File: contracts/features/Blocklist.sol 33: function isBlocked(address addr) public view returns (bool) {
assembly{ id := chainid() } => uint256 id = block.chainid, assembly { size := extcodesize() } => uint256 size = address().code.length
There are some automated tools that will flag a project as having higher complexity if there is inline-assembly, so it's best to avoid using it where it's not necessary
There is 1 instance of this issue:
File: contracts/features/Blocklist.sol 40: size := extcodesize(addr)
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 6 instances of this issue:
File: contracts/VotingEscrow.sol /// @audit 18 116: require(decimals <= 18, "Exceeds max decimals"); /// @audit 255 309: for (uint256 i = 0; i < 255; i++) { /// @audit 18 653: uint256 penaltyAmount = (value * penaltyRate) / 10**18; // quitlock_penalty is in 18 decimals precision /// @audit 128 717: for (uint256 i = 0; i < 128; i++) { /// @audit 128 739: for (uint256 i = 0; i < 128; i++) { /// @audit 255 834: for (uint256 i = 0; i < 255; i++) {
1e6) rather than decimal literals (e.g. 1000000), for readabilityThere are 2 instances of this issue:
File: contracts/VotingEscrow.sol 57: Point[1000000000000000000] public pointHistory; // 1e9 * userPointHistory-length, so sufficient for 1e9 users 58: mapping(address => Point[1000000000]) public userPointHistory;
1e18) rather than exponentiation (e.g. 10**18)While the compiler knows to optimize away the exponentiation, it's still better coding practice to use idioms that do not require compiler optimization, if they exist
There are 3 instances of this issue:
File: contracts/VotingEscrow.sol 48: uint256 public constant MULTIPLIER = 10**18; 51: uint256 public maxPenalty = 10**18; // penalty for quitters with MAXTIME remaining lock 653: uint256 penaltyAmount = (value * penaltyRate) / 10**18; // quitlock_penalty is in 18 decimals precision
There are 2 instances of this issue:
File: contracts/features/Blocklist.sol 2: pragma solidity ^0.8.3;
File: contracts/VotingEscrow.sol 2: pragma solidity ^0.8.3;
There are 4 instances of this issue:
File: contracts/VotingEscrow.sol /// @audit binarysearch /// @audit preceeding 705: // @dev Uses binarysearch to find the most recent point history preceeding block /// @audit binarysearch /// @audit preceeding 729: // @dev Uses binarysearch to find the most recent user point history preceeding block
There is 1 instance of this issue:
File: contracts/interfaces/IERC20.sol
There are 4 instances of this issue:
File: contracts/features/Blocklist.sol /// @audit Missing: '@return' 31 /// @dev This method will be called by the VotingEscrow contract 32 /// @param addr The contract address to check 33: function isBlocked(address addr) public view returns (bool) {
File: contracts/VotingEscrow.sol /// @audit Missing: '@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: '@return' 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) /// @audit Missing: '@return' 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)
indexed fieldsIndex event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
There are 8 instances of this issue:
File: contracts/interfaces/IERC20.sol 29: event Transfer(address indexed from, address indexed to, uint256 value); 30 event Approval( 31 address indexed owner, 32 address indexed spender, 33 uint256 value 34: );
File: 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: ); 38: event TransferOwnership(address owner); 39: event UpdateBlocklist(address blocklist); 40: event UpdatePenaltyRecipient(address recipient); 41: event CollectPenalty(uint256 amount, address recipient);
Consider changing the variable to be an unnamed one
There are 3 instances of this issue:
File: contracts/VotingEscrow.sol /// @audit bias /// @audit slope /// @audit ts 201 function getLastUserPoint(address _addr) 202 external 203 view 204 returns ( 205 int128 bias, 206 int128 slope, 207: uint256 ts
require()/revert() checks should be refactored to a modifier or functionThe compiler will inline the function, which will avoid JUMP instructions usually associated with functions
There are 8 instances of this issue:
File: contracts/VotingEscrow.sol 147: require(msg.sender == owner, "Only owner"); 448: require(_value > 0, "Only non zero amount"); 504: require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime"); 485 require( 486 token.transferFrom(msg.sender, address(this), _value), 487 "Transfer failed" 488: ); 502: require(locked_.amount > 0, "No lock"); 636: require(locked_.end > block.timestamp, "Lock expired"); 637: require(locked_.delegatee == msg.sender, "Lock delegated"); 877: require(_blockNumber <= block.number, "Only past block number");
#0 - lacoop6tu
2022-08-26T15:32:54Z
Good one
🌟 Selected for report: IllIllI
Also found by: 0x040, 0x1f8b, 0xDjango, 0xHarry, 0xLovesleep, 0xNazgul, 0xNineDec, 0xSmartContract, 0xackermann, 0xbepresent, 2997ms, Amithuddar, Aymen0909, Bnke0x0, CRYP70, CertoraInc, Chom, CodingNameKiki, Deivitto, Dravee, ElKu, Fitraldys, Funen, GalloDaSballo, JC, JohnSmith, Junnon, LeoS, Metatron, MiloTruck, Noah3o6, NoamYakov, PaludoX0, RedOneN, Respx, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, SooYa, SpaceCake, TomJ, Tomio, Waze, Yiko, __141345__, a12jmx, ajtra, ak1, apostle0x01, asutorufos, bobirichman, brgltd, bulej93, c3phas, cRat1st0s, carlitox477, chrisdior4, csanuragjain, d3e4, defsec, delfin454000, djxploit, durianSausage, ellahi, erictee, fatherOfBlocks, gerdusx, gogo, ignacio, jag, ladboy233, m_Rassska, medikko, mics, natzuu, newfork01, oyc_109, paribus, pfapostol, rbserver, reassor, ret2basic, robee, rokinot, rvierdiiev, sach1r0, saian, sashik_eth, sikorico, simon135
146.296 USDC - $146.30
| Issue | Instances | |
|---|---|---|
| [G‑01] | Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate | 1 |
| [G‑02] | State variables only set in the constructor should be declared immutable | 14 |
| [G‑03] | Structs can be packed into fewer storage slots | 1 |
| [G‑04] | Using calldata instead of memory for read-only arguments in external functions saves gas | 2 |
| [G‑05] | Using storage instead of memory for structs/arrays saves gas | 9 |
| [G‑06] | Avoid contract existence checks by using solidity version 0.8.10 or later | 3 |
| [G‑07] | State variables should be cached in stack variables rather than re-reading them from storage | 3 |
| [G‑08] | <x> += <y> costs more gas than <x> = <x> + <y> for state variables | 1 |
| [G‑09] | internal functions only called once can be inlined to save gas | 3 |
| [G‑10] | Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement | 2 |
| [G‑11] | ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops | 4 |
| [G‑12] | Optimize names to save gas | 4 |
| [G‑13] | Using bools for storage incurs overhead | 1 |
| [G‑14] | Use a more recent version of solidity | 5 |
| [G‑15] | Using > 0 costs more gas than != 0 when used on a uint in a require() statement | 2 |
| [G‑16] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 4 |
| [G‑17] | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 3 |
| [G‑18] | Using private rather than public for constants, saves gas | 3 |
| [G‑19] | Division by two should use bit shifting | 2 |
| [G‑20] | Stack variable used as a cheaper cache for a state variable is only used once | 1 |
| [G‑21] | require() or revert() statements that check input arguments should be at the top of the function | 2 |
| [G‑22] | Superfluous event fields | 2 |
| [G‑23] | Use custom errors rather than revert()/require() strings to save gas | 42 |
Total: 114 instances over 23 issues
address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriateSaves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.
There is 1 instance of this issue:
File: contracts/VotingEscrow.sol 58 mapping(address => Point[1000000000]) public userPointHistory; 59: mapping(address => uint256) public userPointEpoch;
immutableAvoids a Gsset (20000 gas) in the constructor, and replaces the first access in each transaction (Gcoldsload - 2100 gas) and each access thereafter (Gwarmacces - 100 gas) with a PUSH32 (3 gas).
There are 14 instances of this issue:
File: contracts/features/Blocklist.sol /// @audit manager (constructor) 15: manager = _manager; /// @audit manager (access) 24: require(msg.sender == manager, "Only manager"); /// @audit ve (constructor) 16: ve = _ve; /// @audit ve (access) 27: IVotingEscrow(ve).forceUndelegate(addr);
File: contracts/VotingEscrow.sol /// @audit token (constructor) 107: token = IERC20(_token); /// @audit token (access) 426: token.transferFrom(msg.sender, address(this), _value), /// @audit token (access) 486: token.transferFrom(msg.sender, address(this), _value), /// @audit token (access) 546: require(token.transfer(msg.sender, value), "Transfer failed"); /// @audit token (access) 657: require(token.transfer(msg.sender, remainingAmount), "Transfer failed"); /// @audit token (access) 676: require(token.transfer(penaltyRecipient, amount), "Transfer failed"); /// @audit name (constructor) 118: name = _name; /// @audit symbol (constructor) 119: symbol = _symbol; /// @audit decimals (constructor) 115: decimals = IERC20(_token).decimals(); /// @audit decimals (access) 116: require(decimals <= 18, "Exceeds max decimals");
diff --git a/contracts/VotingEscrow.sol b/contracts/VotingEscrow.sol index f15781a..d2c7666 100644 --- a/contracts/VotingEscrow.sol +++ b/contracts/VotingEscrow.sol @@ -42,7 +42,7 @@ contract VotingEscrow is IVotingEscrow, ReentrancyGuard { event Unlock(); // Shared global state - IERC20 public token; + IERC20 public immutable token; uint256 public constant WEEK = 7 days; uint256 public constant MAXTIME = 365 days; uint256 public constant MULTIPLIER = 10**18; @@ -61,9 +61,9 @@ contract VotingEscrow is IVotingEscrow, ReentrancyGuard { mapping(address => LockedBalance) public locked; // Voting token - string public name; - string public symbol; - uint256 public decimals = 18; + string public constant name = "veFDT"; + string public constant symbol = "veFDT"; + uint256 public immutable decimals; // Structs struct Point { @@ -112,11 +112,10 @@ contract VotingEscrow is IVotingEscrow, ReentrancyGuard { blk: block.number }); - decimals = IERC20(_token).decimals(); - require(decimals <= 18, "Exceeds max decimals"); + uint256 _dec = IERC20(_token).decimals(); + require(_dec <= 18, "Exceeds max decimals"); + decimals = _dec; - name = _name; - symbol = _symbol; owner = _owner; penaltyRecipient = _penaltyRecipient; } diff --git a/contracts/features/Blocklist.sol b/contracts/features/Blocklist.sol index 27db9b0..ca3a226 100644 --- a/contracts/features/Blocklist.sol +++ b/contracts/features/Blocklist.sol @@ -8,8 +8,8 @@ import { IVotingEscrow } from "../interfaces/IVotingEscrow.sol"; /// @dev This is a basic implementation using a mapping for address => bool contract Blocklist { mapping(address => bool) private _blocklist; - address public manager; - address public ve; + address public immutable manager; + address public immutable ve; constructor(address _manager, address _ve) { manager = _manager;
diff --git a/tmp/gas_before b/tmp/gas_after index 3deb415..cf71599 100644 --- a/tmp/gas_before +++ b/tmp/gas_after @@ -167,7 +167,7 @@ No need to generate any newer typings. ····················|······················|·············|·············|···············|···············|·············· | Contract · Method · Min · Max · Avg · # calls · eur (avg) │ ····················|······················|·············|·············|···············|···············|·············· -| Blocklist · block · 45000 · 409628 · 198815 · 5 · - │ +| Blocklist · block · 40797 · 405425 · 194612 · 5 · - │ ····················|······················|·············|·············|···············|···············|·············· | MockERC20 · approve · 46176 · 46200 · 46196 · 96 · - │ ····················|······················|·············|·············|···············|···············|·············· @@ -177,46 +177,46 @@ No need to generate any newer typings. ····················|······················|·············|·············|···············|···············|·············· | MockERC20 · transfer · 51588 · 51612 · 51606 · 83 · - │ ····················|······················|·············|·············|···············|···············|·············· -| MockSmartWallet · createLock · 334002 · 378450 · 355494 · 7 · - │ +| MockSmartWallet · createLock · 331896 · 376344 · 353388 · 7 · - │ ····················|······················|·············|·············|···············|···············|·············· | MockSmartWallet · delegate · - · - · 340388 · 2 · - │ ····················|······················|·············|·············|···············|···············|·············· | MockSmartWallet · increaseUnlockTime · - · - · 208859 · 1 · - │ ····················|······················|·············|·············|···············|···············|·············· -| MockSmartWallet · quitLock · 131158 · 256210 · 201886 · 4 · - │ +| MockSmartWallet · quitLock · 129058 · 254110 · 199786 · 4 · - │ ····················|······················|·············|·············|···············|···············|·············· -| MockSmartWallet · withdraw · - · - · 666280 · 1 · - │ +| MockSmartWallet · withdraw · - · - · 664174 · 1 · - │ ····················|······················|·············|·············|···············|···············|·············· | VotingEscrow · checkpoint · 82307 · 3715511 · 1272491 · 10 · - │ ····················|······················|·············|·············|···············|···············|·············· -| VotingEscrow · collectPenalty · - · - · 49948 · 1 · - │ +| VotingEscrow · collectPenalty · - · - · 47863 · 1 · - │ ····················|······················|·············|·············|···············|···············|·············· -| VotingEscrow · createLock · 293060 · 3978208 · 414348 · 60 · - │ +| VotingEscrow · createLock · 290954 · 3976102 · 412242 · 60 · - │ ····················|······················|·············|·············|···············|···············|·············· | VotingEscrow · delegate · 246709 · 4048411 · 650225 · 33 · - │ ····················|······················|·············|·············|···············|···············|·············· -| VotingEscrow · increaseAmount · 234777 · 1077801 · 448554 · 4 · - │ +| VotingEscrow · increaseAmount · 232671 · 1075695 · 446448 · 4 · - │ ····················|······················|·············|·············|···············|···············|·············· | VotingEscrow · increaseUnlockTime · 46794 · 416024 · 143186 · 23 · - │ ····················|······················|·············|·············|···············|···············|·············· -| VotingEscrow · quitLock · 127639 · 1605731 · 375486 · 13 · - │ +| VotingEscrow · quitLock · 125539 · 1603631 · 373386 · 13 · - │ ····················|······················|·············|·············|···············|···············|·············· | VotingEscrow · unlock · - · - · 14596 · 3 · - │ ····················|······················|·············|·············|···············|···············|·············· | VotingEscrow · updateBlocklist · - · - · 47186 · 14 · - │ ····················|······················|·············|·············|···············|···············|·············· -| VotingEscrow · withdraw · 106462 · 3752666 · 610141 · 33 · - │ +| VotingEscrow · withdraw · 104356 · 3750560 · 608035 · 33 · - │ ····················|······················|·············|·············|···············|···············|·············· | Deployments · · % of limit · │ ···········································|·············|·············|···············|···············|·············· -| Blocklist · 278212 · 278236 · 278231 · 2.2 % · - │ +| Blocklist · 248613 · 248637 · 248632 · 2 % · - │ ···········································|·············|·············|···············|···············|·············· | MockERC20 · - · - · 1278169 · 10.3 % · - │ ···········································|·············|·············|···············|···············|·············· | MockSmartWallet · - · - · 416156 · 3.3 % · - │ ···········································|·············|·············|···············|···············|·············· -| VotingEscrow · 4374338 · 4374350 · 4374350 · 35.1 % · - │ +| VotingEscrow · 4280168 · 4280180 · 4280180 · 34.4 % · - │ ·------------------------------------------|-------------|-------------|---------------|---------------|-------------· - 117 passing (48s) + 117 passing (46s)
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
There is 1 instance of this issue:
File: contracts/VotingEscrow.sol /// @audit Variable ordering with 3 slots instead of the current 4: /// uint256(32):end, address(20):delegatee, int128(16):amount, int128(16):delegated 75 struct LockedBalance { 76 int128 amount; 77 uint256 end; 78 int128 delegated; 79 address delegatee; 80: }
diff --git a/contracts/VotingEscrow.sol b/contracts/VotingEscrow.sol index f15781a..0318bc3 100644 --- a/contracts/VotingEscrow.sol +++ b/contracts/VotingEscrow.sol @@ -73,10 +73,10 @@ contract VotingEscrow is IVotingEscrow, ReentrancyGuard { uint256 blk; } struct LockedBalance { - int128 amount; uint256 end; - int128 delegated; address delegatee; + int128 amount; + int128 delegated; } // Miscellaneous @@ -420,7 +420,7 @@ contract VotingEscrow is IVotingEscrow, ReentrancyGuard { locked_.delegated += int128(int256(_value)); locked_.delegatee = msg.sender; locked[msg.sender] = locked_; - _checkpoint(msg.sender, LockedBalance(0, 0, 0, address(0)), locked_); + _checkpoint(msg.sender, LockedBalance({amount:0, end:0, delegated:0, delegatee:address(0)}), locked_); // Deposit locked tokens require( token.transferFrom(msg.sender, address(this), _value), diff --git a/test/votingEscrowDelegationMathTest.ts b/test/votingEscrowDelegationMathTest.ts index 5e43096..088fb9c 100644 --- a/test/votingEscrowDelegationMathTest.ts +++ b/test/votingEscrowDelegationMathTest.ts @@ -138,10 +138,10 @@ describe("VotingEscrow Delegation Math test", () => { }; interface LockedBalance { - amount: BN; end: BN; - delegated: BN; delegatee: string; + amount: BN; + delegated: BN; } interface Point { @@ -173,10 +173,10 @@ describe("VotingEscrow Delegation Math test", () => { epoch, userEpoch, userLocked: { - amount: locked[0], - end: locked[1], - delegated: locked[2], - delegatee: locked[3], + end: locked[0], + delegatee: locked[1], + amount: locked[2], + delegated: locked[3], }, userLastPoint: { bias: userLastPoint[0], diff --git a/test/votingEscrowGasTest.ts b/test/votingEscrowGasTest.ts index d6d03fd..af94f35 100644 --- a/test/votingEscrowGasTest.ts +++ b/test/votingEscrowGasTest.ts @@ -174,10 +174,10 @@ describe("Gas usage tests", () => { epoch, userEpoch, userLocked: { - amount: locked[0], - end: locked[1], - delegated: locked[2], - delegatee: locked[3], + end: locked[0], + delegatee: locked[1], + amount: locked[2], + delegated: locked[3], }, userLastPoint: { bias: userLastPoint[0], diff --git a/test/votingEscrowMathTest.ts b/test/votingEscrowMathTest.ts index 9d0b9b6..e084b30 100644 --- a/test/votingEscrowMathTest.ts +++ b/test/votingEscrowMathTest.ts @@ -207,8 +207,10 @@ describe("VotingEscrow Math test", () => { }); interface LockedBalance { - amount: BN; end: BN; + delegatee: string; + amount: BN; + delegated: BN; } interface Point { @@ -252,8 +254,10 @@ describe("VotingEscrow Math test", () => { //totalStaticWeight: await votingLockup.totalStaticWeight(), // userStaticWeight: await votingLockup.staticBalanceOf(sender.address), userLocked: { - amount: locked[0], - end: locked[1], + end: locked[0], + delegatee: locked[1], + amount: locked[2], + delegated: locked[3], }, userLastPoint: { bias: userLastPoint[0],
diff --git a/tmp/gas_before b/tmp/gas_after index 3deb415..09dd64b 100644 --- a/tmp/gas_before +++ b/tmp/gas_after @@ -167,7 +167,7 @@ No need to generate any newer typings. ····················|······················|·············|·············|···············|···············|·············· | Contract · Method · Min · Max · Avg · # calls · eur (avg) │ ····················|······················|·············|·············|···············|···············|·············· -| Blocklist · block · 45000 · 409628 · 198815 · 5 · - │ +| Blocklist · block · 42887 · 387472 · 188685 · 5 · - │ ····················|······················|·············|·············|···············|···············|·············· | MockERC20 · approve · 46176 · 46200 · 46196 · 96 · - │ ····················|······················|·············|·············|···············|···············|·············· @@ -177,35 +177,35 @@ No need to generate any newer typings. ····················|······················|·············|·············|···············|···············|·············· | MockERC20 · transfer · 51588 · 51612 · 51606 · 83 · - │ ····················|······················|·············|·············|···············|···············|·············· -| MockSmartWallet · createLock · 334002 · 378450 · 355494 · 7 · - │ +| MockSmartWallet · createLock · 311601 · 356043 · 333090 · 7 · - │ ····················|······················|·············|·············|···············|···············|·············· -| MockSmartWallet · delegate · - · - · 340388 · 2 · - │ +| MockSmartWallet · delegate · - · - · 350371 · 2 · - │ ····················|······················|·············|·············|···············|···············|·············· -| MockSmartWallet · increaseUnlockTime · - · - · 208859 · 1 · - │ +| MockSmartWallet · increaseUnlockTime · - · - · 206298 · 1 · - │ ····················|······················|·············|·············|···············|···············|·············· -| MockSmartWallet · quitLock · 131158 · 256210 · 201886 · 4 · - │ +| MockSmartWallet · quitLock · 140849 · 265907 · 211580 · 4 · - │ ····················|······················|·············|·············|···············|···············|·············· -| MockSmartWallet · withdraw · - · - · 666280 · 1 · - │ +| MockSmartWallet · withdraw · - · - · 676067 · 1 · - │ ····················|······················|·············|·············|···············|···············|·············· -| VotingEscrow · checkpoint · 82307 · 3715511 · 1272491 · 10 · - │ +| VotingEscrow · checkpoint · 82319 · 3715835 · 1272603 · 10 · - │ ····················|······················|·············|·············|···············|···············|·············· | VotingEscrow · collectPenalty · - · - · 49948 · 1 · - │ ····················|······················|·············|·············|···············|···············|·············· -| VotingEscrow · createLock · 293060 · 3978208 · 414348 · 60 · - │ +| VotingEscrow · createLock · 270653 · 3956119 · 391951 · 60 · - │ ····················|······················|·············|·············|···············|···············|·············· -| VotingEscrow · delegate · 246709 · 4048411 · 650225 · 33 · - │ +| VotingEscrow · delegate · 224688 · 4026678 · 646994 · 33 · - │ ····················|······················|·············|·············|···············|···············|·············· -| VotingEscrow · increaseAmount · 234777 · 1077801 · 448554 · 4 · - │ +| VotingEscrow · increaseAmount · 229422 · 1072518 · 443303 · 4 · - │ ····················|······················|·············|·············|···············|···············|·············· -| VotingEscrow · increaseUnlockTime · 46794 · 416024 · 143186 · 23 · - │ +| VotingEscrow · increaseUnlockTime · 44338 · 413481 · 140686 · 23 · - │ ····················|······················|·············|·············|···············|···············|·············· -| VotingEscrow · quitLock · 127639 · 1605731 · 375486 · 13 · - │ +| VotingEscrow · quitLock · 137330 · 1615548 · 385196 · 13 · - │ ····················|······················|·············|·············|···············|···············|·············· | VotingEscrow · unlock · - · - · 14596 · 3 · - │ ····················|······················|·············|·············|···············|···············|·············· | VotingEscrow · updateBlocklist · - · - · 47186 · 14 · - │ ····················|······················|·············|·············|···············|···············|·············· -| VotingEscrow · withdraw · 106462 · 3752666 · 610141 · 33 · - │ +| VotingEscrow · withdraw · 116189 · 3762705 · 619911 · 33 · - │ ····················|······················|·············|·············|···············|···············|·············· | Deployments · · % of limit · │ ···········································|·············|·············|···············|···············|·············· @@ -215,8 +215,8 @@ No need to generate any newer typings. ···········································|·············|·············|···············|···············|·············· | MockSmartWallet · - · - · 416156 · 3.3 % · - │ ···········································|·············|·············|···············|···············|·············· -| VotingEscrow · 4374338 · 4374350 · 4374350 · 35.1 % · - │ +| VotingEscrow · 4245313 · 4245325 · 4245325 · 34.1 % · - │ ·------------------------------------------|-------------|-------------|---------------|---------------|-------------· - 117 passing (48s) + 117 passing (44s)
calldata instead of memory for read-only arguments in external functions saves gasWhen a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead.
If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one
Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved
There are 2 instances of this issue:
File: contracts/VotingEscrow.sol /// @audit _name /// @audit _symbol 100 constructor( 101 address _owner, 102 address _penaltyRecipient, 103 address _token, 104 string memory _name, 105: string memory _symbol
storage instead of memory for structs/arrays saves gasWhen fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct
There are 9 instances of this issue:
File: contracts/VotingEscrow.sol 410: LockedBalance memory locked_ = locked[msg.sender]; 446: LockedBalance memory locked_ = locked[msg.sender]; 499: LockedBalance memory locked_ = locked[msg.sender]; 527: LockedBalance memory locked_ = locked[msg.sender]; 561: LockedBalance memory locked_ = locked[msg.sender]; 633: LockedBalance memory locked_ = locked[msg.sender]; 788: Point memory point0 = pointHistory[epoch]; 866: Point memory lastPoint = pointHistory[epoch_]; 882: Point memory point = pointHistory[targetEpoch];
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 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
There are 3 instances of this issue:
File: contracts/VotingEscrow.sol /// @audit decimals() 115: decimals = IERC20(_token).decimals(); /// @audit isBlocked() 126: !IBlocklist(blocklist).isBlocked(msg.sender), /// @audit isBlocked() 563: require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 3 instances of this issue:
File: contracts/VotingEscrow.sol /// @audit penaltyRecipient on line 676 677: emit CollectPenalty(amount, penaltyRecipient); /// @audit pointHistory on line 788 796: Point memory point1 = pointHistory[epoch + 1]; /// @audit pointHistory on line 882 891: Point memory pointNext = pointHistory[targetEpoch + 1];
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesUsing the addition operator instead of plus-equals saves 113 gas
There is 1 instance of this issue:
File: contracts/VotingEscrow.sol 654: penaltyAccumulated += penaltyAmount;
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 3 instances of this issue:
File: contracts/features/Blocklist.sol 37: function _isContract(address addr) internal view returns (bool) {
File: contracts/VotingEscrow.sol 662 function _calculatePenaltyRate(uint256 end) 663 internal 664 view 665: returns (uint256) 732 function _findUserBlockEpoch(address _addr, uint256 _block) 733 internal 734 view 735: returns (uint256)
unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statementrequire(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }
There are 2 instances of this issue:
File: contracts/VotingEscrow.sol /// @audit if-condition on line 299 301: (MULTIPLIER * (block.number - lastPoint.blk)) / /// @audit if-condition on line 299 302: (block.timestamp - lastPoint.ts);
++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loopsThe unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop
There are 4 instances of this issue:
File: 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++) {
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
There are 4 instances of this issue:
File: contracts/features/Blocklist.sol /// @audit block(), isBlocked() 9: contract Blocklist {
File: contracts/interfaces/IBlocklist.sol /// @audit isBlocked() 6: interface IBlocklist {
File: contracts/interfaces/IVotingEscrow.sol /// @audit createLock(), increaseAmount(), increaseUnlockTime(), withdraw(), quitLock(), balanceOfAt(), totalSupplyAt(), forceUndelegate() 4: interface IVotingEscrow {
File: contracts/VotingEscrow.sol /// @audit updateBlocklist(), updatePenaltyRecipient(), unlock(), lockEnd(), getLastUserPoint(), checkpoint(), collectPenalty() 23: contract VotingEscrow is IVotingEscrow, ReentrancyGuard {
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
There is 1 instance of this issue:
File: contracts/features/Blocklist.sol 10: mapping(address => bool) private _blocklist;
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
There are 5 instances of this issue:
File: contracts/features/Blocklist.sol 2: pragma solidity ^0.8.3;
File: contracts/interfaces/IBlocklist.sol 2: pragma solidity ^0.8.3;
File: contracts/interfaces/IERC20.sol 2: pragma solidity ^0.8.3;
File: contracts/interfaces/IVotingEscrow.sol 2: pragma solidity ^0.8.3;
File: contracts/VotingEscrow.sol 2: pragma solidity ^0.8.3;
> 0 costs more gas than != 0 when used on a uint in a require() statementThis change saves 6 gas per instance. The optimization works until solidity version 0.8.13 where there is a regression in gas costs.
There are 2 instances of this issue:
File: contracts/VotingEscrow.sol 412: require(_value > 0, "Only non zero amount"); 448: require(_value > 0, "Only non zero amount");
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 5 gas per loop
There are 4 instances of this issue:
File: 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++) {
uints/ints smaller than 32 bytes (256 bits) incurs overheadWhen using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html
Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed
There are 3 instances of this issue:
File: contracts/VotingEscrow.sol /// @audit int128 oldSlopeDelta 380: oldSlopeDelta = oldSlopeDelta + userOldPoint.slope; /// @audit int128 oldSlopeDelta 382: oldSlopeDelta = oldSlopeDelta - userNewPoint.slope; // It was a new deposit, not extension /// @audit int128 newSlopeDelta 388: newSlopeDelta = newSlopeDelta - userNewPoint.slope; // old slope disappeared at this point
private rather than public for constants, saves gasIf needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas 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
There are 3 instances of this issue:
File: contracts/VotingEscrow.sol 46: uint256 public constant WEEK = 7 days; 47: uint256 public constant MAXTIME = 365 days; 48: uint256 public constant MULTIPLIER = 10**18;
<x> / 2 is the same as <x> >> 1. While the compiler uses the SHR opcode to accomplish both, the version that uses division incurs an overhead of 20 gas due to JUMPs to and from a compiler utility function that introduces checks which can be avoided by using unchecked {} around the division by two
There are 2 instances of this issue:
File: contracts/VotingEscrow.sol 719: uint256 mid = (min + max + 1) / 2; 743: uint256 mid = (min + max + 1) / 2;
If the variable is only accessed once, it's cheaper to use the state variable directly that one time, and save the 3 gas the extra stack assignment would spend
There is 1 instance of this issue:
File: contracts/VotingEscrow.sol 865: uint256 epoch_ = globalEpoch;
require() or revert() statements that check input arguments should be at the top of the functionChecks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas*) in a function that may ultimately revert in the unhappy case.
There are 2 instances of this issue:
File: contracts/VotingEscrow.sol /// @audit expensive op on line 410 412: require(_value > 0, "Only non zero amount"); /// @audit expensive op on line 446 448: require(_value > 0, "Only non zero amount");
block.timestamp and block.number are added to event information by default so adding them manually wastes gas
There are 2 instances of this issue:
File: contracts/VotingEscrow.sol 30: uint256 ts 36: uint256 ts
revert()/require() strings to save gasCustom 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
There are 42 instances of this issue:
File: contracts/features/Blocklist.sol 24: require(msg.sender == manager, "Only manager"); 25: require(_isContract(addr), "Only contracts");
File: 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"); 147: 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");
#0 - lacoop6tu
2022-08-26T15:34:28Z
Good one