You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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)
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: uint256public decimals =18; //@audit should be uint8115: 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.
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 err254: // start {255: // Now handle user history256: 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:
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: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.
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.
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: uint256public constant MULTIPLIER =10**18;
VotingEscrow.sol:51: uint256public 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
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:
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(addresswho) externalonlyOwner() {
libraries/Authorizable.sol:44: function deauthorize(addresswho) externalonlyOwner() {
libraries/Authorizable.sol:50: function setOwner(addresswho) publiconlyOwner() {
libraries/ERC20PermitWithMint.sol:29: function mint(addressaccount, uint256amount) external onlyOwner {
libraries/ERC20PermitWithMint.sol:49: function burn(addressaccount, uint256amount) external onlyOwner {
The text was updated successfully, but these errors were encountered:
Overview
Table of Contents:
immutableLockedBalancefor a cheaper copy inmemorydecimalsshould beuint8instead ofuint256and tightly packeduserOldPointis overwritten0.8.13:> 0is less efficient than!= 0for unsigned integers++icosts less gas compared toi++ori += 1(same for--ivsi--ori -= 1)1e18) rather than exponentiation (e.g.10**18)payable1. State variables only set in the constructor should be declared
immutableVariables 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,ve2. Tightly packing struct
LockedBalancefor a cheaper copy inmemoryWhen copying a state struct in memory, there are as many SLOADs and MSTOREs as there are slots.
At multiple places, a
LockedBalancestate variable is copied inmemory:Here, the cost of a copy in
memoryis 4 SLOADs, as the struct takes 4 SLOTS:To save 1 SLOAD on these operations, it would be a good idea to tightly pack the struct:
3.
decimalsshould beuint8instead ofuint256and tightly packedThe 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 thedecimalstype touint8.With this change, it'll then be possible to save 1 storage SLOT:
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.
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.
userOldPointis overwrittenSSTOREs are expensive.
Here, under the
uEpoch == 0condition, 2 SSTOREs are made L258 and L264:While this does indeed seem like an error in logic that might have a deeper impact (
userOldPointis never persisted and never used, this doesn't feel intended), it still consumes 2 SSTOREsConsider using a ternary operator L264:
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:
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
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:
9. Pre-Solidity
0.8.13:> 0is less efficient than!= 0for unsigned integersUp until Solidity
0.8.13:!= 0costs less gas compared to> 0for unsigned integers inrequirestatements with the optimizer enabled (6 gas)Proof: While it may seem that
> 0is cheaper than!=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer AND you're in arequirestatement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706Consider changing
> 0with!= 0here: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.
11. Use shift right/left instead of division/multiplication if possible
While the
DIV/MULopcode uses 5 gas, theSHR/SHLopcode 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 version0.8+>> 1instead of/ 2Affected code (saves around 2 gas + 20 for unchecked per instance):
12.
++icosts less gas compared toi++ori += 1(same for--ivsi--ori -= 1)Pre-increments and pre-decrements are cheaper.
For a
uint256 ivariable, the following is true with the Optimizer enabled at 10k:Increment:
i += 1is the most expensive formi++costs 6 gas less thani += 1++icosts 5 gas less thani++(11 gas less thani += 1)Decrement:
i -= 1is the most expensive formi--costs 11 gas less thani -= 1--icosts 5 gas less thani--(16 gas less thani -= 1)Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:
However, pre-increments (or pre-decrements) return the new value:
In the pre-increment case, the compiler has to create a temporary variable (when used) for returning
1instead of2.Affected code:
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
uncheckedblock here (around 25 gas saved per instance):The change would be:
The same can be applied with decrements (which should use
breakwheni == 0).The risk of overflow is non-existent for
uint256here.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 (
0foruint,falseforbool,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:
Consider removing explicit initializations for default values.
15. Use scientific notation (e.g.
1e18) rather than exponentiation (e.g.10**18)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:
Consider upgrading here :
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/:
Consider replacing all revert strings with custom errors in the solution, and particularly those that have multiple occurrences:
18. (Not recommended, but true) Functions guaranteed to revert when called by normal users can be marked
payableIf a function modifier such as
onlyOwneris used, the function will revert if a normal user tries to pay the function. Marking the function aspayablewill lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.The text was updated successfully, but these errors were encountered: