Open code423n4 opened 2 years ago
@jack-the-pug Can you let me know why this report was ranked so low? Without any feedback I can't improve. Also, it feels like I'm being penalized for other people copying my work. I've spent months learning gas optimizations and figuring out rules, and this report https://github.com/code-423n4/2022-08-rigor-findings/issues/260 is ranked higher, even though it has fewer findings and copies my text word for word, including links to my gists (see Multiple accesses of a mapping/array should use a local variable cache
in both reports, along with many others).
So, first thing first, there is nothing wrong with copying. This is the ethos of the whole Ethereum and the greater OSS community, and probably also the entire human civilization.
I recommend you to watch this great documentary Everything is a Remix which explained why and how copying is great.
And also this article Copying is the way design works, it's about design, but I think it also applies to everything else that involves human creativity.
Thank you for your work and I appreciate your dedication to standardizing the gas optimization know-hows.
You have the best formatting, solid proof, short but concrete description of the findings, and many other merits in your QA and Gas reports. That's some great work being done!
If I remember correctly, I have scored your reports quite a few times as the top QA/gas report among the contests I judged.
However, as you can see from #260 and many other reports, once a gas optimization rule/know-how is known to the community, there will be people (or robots) that submit them many times in duplication.
So I wonder what kind of scoring rules can better incentive good, practical, down-to-earth, REAL helpful gas optimization reports to the sponsor, instead of the ones that only work theoretically, and should not even be considered in most cases as they can hurt the readability.
My rules can be explained like this:
This whole scoring process is done in a quick-decision mode, it can be wrong by a little bit sometimes, but generally, that's what I'm trying to do.
So, for this report being scored 60 and #260 being scored 65 (not a super big difference), I believe it is because of this: [G‑21] Don't use _msgSender() if not supporting EIP-2771
As we know, EIP-2771 is being used across many contracts in this project, so this may just be invalid.
All in all, I have no problem with copying. Creative and original work is welcomed, copying is okay. But I do prefer the findings that considered the context of the project much more than the pattern-matching findings, because they are more practical and helpful to the sponsor.
Have a good day and keep up with the good work!
The copying has been going on for a while and I didn't mention it previously, because I agree with you that copying is fair game, however in this case it looked like some sort of drastic penalty was being applied, and without an explanation it was confusing. Thank you for your explanation, it makes sense now, and I'll continue improving my system
@jack-the-pug Regarding your comment about [G-21], it's invalid to use _msgSender()
without also implementing isTrustedForwarder()
, which the contract did not do, and that's why I flagged it. I linked to the EIP, but I guess I could have been clearer about the reason in my text. The EIP says, The Recipient MUST check that it trusts the Forwarder to prevent it from extracting address data appended from an untrusted contract.
Does that make this finding first of its kind rather than invalid?
Yes, [G-12] is valid. Using _msgSender()
in DebtToken
is not necessary as it does not work. I've updated your score from 60 to 65.
But for the sake of consistency with other access control modifiers across the codebase, I think it's okay to keep it so.
modifier onlyAdmin() {
require(admin == _msgSender(), "HomeFi::!Admin");
_;
}
Summary
Gas Optimizations
calldata
instead ofmemory
for read-only arguments inexternal
functions saves gas<x> += <y>
costs more gas than<x> = <x> + <y>
for state variablesinternal
functions only called once can be inlined to save gasunchecked {}
for subtractions where the operands cannot underflow because of a previousrequire()
orif
-statement<array>.length
should not be looked up in every loop of afor
-loop++i
/i++
should beunchecked{++i}
/unchecked{i++}
when it is not possible for them to overflow, as is the case when used infor
- andwhile
-loopsbool
s for storage incurs overhead> 0
costs more gas than!= 0
when used on auint
in arequire()
statement++i
costs less gas thani++
, especially when it's used infor
-loops (--i
/i--
too)require()
statements that use&&
saves gasrequire()
orrevert()
statements that check input arguments should be at the top of the functionrevert()
/require()
strings to save gaspayable
_msgSender()
if not supporting EIP-2771Total: 217 instances over 21 issues
Gas Optimizations
[G‑01] State variables can be packed into fewer storage slots
If variables occupying the same slot are both written the same function or by the constructor, avoids a separate Gsset (20000 gas). Reads of the variables can also be cheaper
There is 1 instance of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L40
[G‑02] Using
calldata
instead ofmemory
for read-only arguments inexternal
functions saves gasWhen a function with a
memory
array is called externally, theabi.decode()
step has to use a for-loop to copy each index of thecalldata
to thememory
index. Each iteration of this for-loop costs at least 60 gas (i.e.60 * <mem_array>.length
). Usingcalldata
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 havingmemory
arguments, it's still valid for implementation contracs to usecalldata
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 thereforememory
is used in theexternal
call, it's still more gass-efficient to usecalldata
when theexternal
function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length oneNote that I've also flagged instances where the function is
public
but can be marked asexternal
since it's not called by the contract, and cases where a constructor is involvedThere are 4 instances of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L484-L489
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/DebtToken.sol#L43-L48
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L210-L213
[G‑03] Avoid contract existence checks by using solidity version 0.8.10 or later
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 valueThere are 4 instances of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L91
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/ProjectFactory.sol#L65
[G‑04] State variables should be cached in stack variables rather than re-reading them from storage
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 25 instances of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L143
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L121
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L229
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L603
[G‑05] Multiple accesses of a mapping/array should use a local variable cache
The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local
storage
orcalldata
variable when the value is accessed multiple times, saves ~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. Caching an array's struct avoids recalculating the array offsets into memory/calldataThere are 26 instances of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L402-L403
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L354
[G‑06] The result of external function calls should be cached rather than re-calling the function
The instances below point to the second+ call of the function within a single function
There is 1 instance of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L394
[G‑07]
<x> += <y>
costs more gas than<x> = <x> + <y>
for state variablesUsing the addition operator instead of plus-equals saves 113 gas
There are 7 instances of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L289
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L179
[G‑08]
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 7 instances of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L207
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L197-L200
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L284-L286
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L770
[G‑09] Add
unchecked {}
for subtractions where the operands cannot underflow because of a previousrequire()
orif
-statementrequire(a <= b); x = b - a
=>require(a <= b); unchecked { x = b - a }
There are 2 instances of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L794
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L427
[G‑10]
<array>.length
should not be looked up in every loop of afor
-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD
(3 gas)CALLDATALOAD
(3 gas)Caching the length changes each of these to a
DUP<N>
(3 gas), and gets rid of the extraDUP<N>
needed to store the stack offsetThere is 1 instance of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L603
[G‑11]
++i
/i++
should beunchecked{++i}
/unchecked{i++}
when it is not possible for them to overflow, as is the case when used infor
- andwhile
-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 loopThere are 11 instances of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L624
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L87
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L181
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L248
[G‑12] Optimize names to save gas
public
/external
function names andpublic
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 shiftedThere are 2 instances of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L14
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L41
[G‑13] Using
bool
s for storage incurs overheadhttps://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use
uint256(1)
anduint256(2)
for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing fromfalse
totrue
, after having beentrue
in the pastThere are 7 instances of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L55
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L30
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L50
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L68
[G‑14] Using
> 0
costs more gas than!= 0
when used on auint
in arequire()
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:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L764
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L195
[G‑15]
++i
costs less gas thani++
, especially when it's used infor
-loops (--i
/i--
too)Saves 5 gas per loop
There are 14 instances of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L140
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L87
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L181
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L248
[G‑16] Splitting
require()
statements that use&&
saves gasSee this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper by 3 gas
There are 3 instances of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L353-L357
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L61-L65
[G‑17] Stack variable used as a cheaper cache for a state variable is only used once
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:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L420
[G‑18]
require()
orrevert()
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 is 1 instance of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L195
[G‑19] Use custom errors rather than
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 74 instances of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L69
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/DebtToken.sol#L31-L34
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L39
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L41
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L73
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L44
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/ProjectFactory.sol#L36
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L123
[G‑20] 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 aspayable
will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided areCALLVALUE
(2),DUP1
(3),ISZERO
(3),PUSH2
(3),JUMPI
(10),PUSH1
(3),DUP1
(3),REVERT
(0),JUMPDEST
(1),POP
(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment costThere are 23 instances of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L285-L290
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/DebtToken.sol#L61-L64
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L84-L87
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L100-L102
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L123-L139
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L88-L91
[G‑21] Don't use
_msgSender()
if not supporting EIP-2771Use
msg.sender
if the code does not implement EIP-2771 trusted forwarder supportThere is 1 instance of this issue:
https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/DebtToken.sol#L32