code-423n4 / 2023-04-frankencoin-findings

5 stars 4 forks source link

Gas Optimizations #427

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

See the markdown file with the details of this report here.

0xA5DF commented 1 year ago

Contains some false findings: G2, G4, G5, G8, G10 borderline low quality imo

c4-judge commented 1 year ago

hansfriese marked the issue as grade-a

noamyakov commented 1 year ago

@hansfriese please review this report again. I think it should be graded as C because it has too many wrong optimizations.

[G-02] Use hardcoded address instead address(this)

This won't save any gas.

[G-04] Public Functions To External

The instances refer to functions that cannot be marked external because they're used internally. They must be public.

[G-05] Using calldata instead of memory for read-only arguments in external functions saves gas

The single instance of this finding is wrong because calldata cannot be used on local variables.

[G-06] >= costs less gas than >

>= and > are not equivalent. This kind of optimization can only be applied to something like this:

if (x > y) {
    doSomething1();
} else {
    doSomething2();
}

And the optimized code would look like this:

if (y >= x) {
    doSomething2();
} else {
    doSomething1();
}

Most of the instances are wrong. The only valid instance is:

File: /contracts/Position.sol
160    if (newPrice > price) {

[G-08] Using bools for storage incurs overhead

None of the instances describe a bool that is used for storage. They are all local variables and functions' return values.

[G-09]Avoid using state variable in emit

This is not an optimization unless that state variables is cached in a local variable, which isn't the case here. In addition, some of the instances refer to immutables.

[G-10]Change public state variable visibility to private

This is not an optimization because it changes functionality.

[G-12]use Mappings Instead of Arrays

Changes functionality and doesn't save any gas in this case.

[G‑13] Multiple accesses of a mapping/array should use a local variable cache

Most of the instances are wrong because the don't refer to accesses done in the same function. Some of them aren't even read operations (they are write operations) so caching won't help.

[G‑14] State variables should be cached in stack variables rather than re-reading them from storage

Most of the instances are wrong because they refer to immutables.

hansfriese commented 1 year ago

G9 has invalid finding. G13, G14 has many invalid instances.