Open code423n4 opened 2 years ago
constant
variables must have an initialiser -- but your suggestion is right. I changed
address private constant NATIVE_ASSET = address(0)
to
address private constant NATIVE_ASSET = 0x0000000000000000000000000000000000000000;
Go ahead and implement the suggested changes. We can change the optimizer rounds in the config later.
Also don't make any changes to LibDiamond.sol
for now.
Nope I'm totally on board with that. Thanks.
We internally decided not to have prefix increments for now.
Duplicate of #197
Duplicate of #39
Duplicate of #196
Duplicate of #100
We internally decided to avoid unchecked operations for now.
We internally decided to avoid previx increments for now.
Gas Report
Table of Contents:
> 0
is less efficient than!= 0
for unsigned integers (with proof)++i
costs less gas compared toi++
unchecked
blocks for arithmetics operations that can't underflow/overflowFindings
Storage
Help the optimizer by declaring a storage variable instead of repeatedly fetching a value in storage (for reading and writing)
To help the optimizer, declare a
storage
type variable and use it instead of repeatedly fetching the value in a map or an array.The effect can be quite significant.
Instances include (check the
@audit
tags):Emitting storage values
Here, the values emitted shouldn't be read from storage. The existing memory values should be used instead:
Variables
No need to explicitly initialize variables with default values
If a variable is not set/initialized, it is assumed to have the default value (
0
foruint
,false
forbool
,address(0)
for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.Instances include:
I suggest removing explicit initializations for default values.
"constants" expressions are expressions, not constants
Due to how
constant
variables are implemented (replacements at compile-time), an expression assigned to aconstant
variable is recomputed each time that the variable is used, which wastes some gas.If the variable was
immutable
instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.See: ethereum/solidity#9232
Change these expressions from
constant
toimmutable
and implement the calculation in the constructor or hardcode these values in the constants and add a comment to say how the value was calculated.Comparisons
Boolean comparisons
Comparing to a constant (
true
orfalse
) is a bit more expensive than directly checking the returned boolean value. I suggest usingif(directValue)
instead ofif(directValue == true)
andif(!directValue)
instead ofif(directValue == false)
here:> 0
is less efficient than!= 0
for unsigned integers (with proof)!= 0
costs less gas compared to> 0
for unsigned integers inrequire
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 at 10k AND you're in arequire
statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706I suggest changing
> 0
with!= 0
here:Also, please enable the Optimizer.
For-Loops
An array's length should be cached to save gas in for-loops
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
Caching the array length in the stack saves around 3 gas per iteration.
Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:
This is already done here:
++i
costs less gas compared toi++
++i
costs less gas compared toi++
for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)i++
incrementsi
and returns the initial value ofi
. Which means:But
++i
returns the actual incremented value:In the first case, the compiler has to create a temporary variable (when used) for returning
1
instead of2
Instances include:
I suggest using
++i
instead ofi++
to increment the value of an uint variable.Increments can be unchecked
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
Instances include:
The code would go from:
to:
I suggest not unchecking the increments at these places as the risk of overflow is existent for
uint8
there:Arithmetics
unchecked
blocks for arithmetics operations that can't underflow/overflowSolidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation, or the operation doesn't depend on user input), some gas can be saved by using an
unchecked
block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmeticI suggest wrapping with an
unchecked
block here (see@audit
tags for more details):Visibility
Public functions to external
The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.
Errors
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 strings > 32 bytes:
I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next.
Use Custom Errors instead of Revert Strings to save Gas
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)
Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:
Custom errors are defined using the
error
statement, which can be used inside and outside of contracts (including interfaces and libraries).Instances include:
I suggest replacing revert strings with custom errors.