[N-1] public functions not called by the contract should be declared external instead

Vulnerability Details

Contracts are allowed to override their parents' functions and change the visibility from external to public.

Lines of code

Total -> 4

8, 13, 19, 27

[N-2] Event is not properly indexed.

Vulnerability Details

Index event fields make the field more quickly accessible to off-chain tools that parse events.This is especially useful when it comes to filtering based on an address. 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).

Lines of code

Total -> 1

16

Recommended Mitigation Steps

Where applicable, 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 applicable fields, all of the applicable fields should be indexed.

[N-3] Use a modifier instead of require to check for msg.sender

Vulnerability Details

If some functions are only allowed to be called by some specific users, consider using a modifier instead of checking with a require statement, especially if this check is done in multiple functions.

Lines of code

Total -> 3

19, 36, 16

[G-1] Using storage instead of memory for structs/arrays saves gas

Vulnerability Details

When fetching data from a storage location, assigning the data to a memory variable causes all fields of thestruct/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.

Lines of code

Total -> 4

26, 35, 21, 23

Impact

Instead of declaring 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 incurring 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.

[G-2] Functions guaranteed to revert when called by normal users can be marked payable

Vulnerability Details

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay thefunction. 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.

Lines of code

Total -> 2

23, 32

Recommended Mitigation Steps

The extra opcodes avoided are CALLVALUE(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 cost.

[G-3] Use assembly to emit events, in order to save gas

Vulnerability Details

Using the scratch space for event arguments (two words or fewer) will save gas over needing Solidity's full abi memory expansion used for emitting normally.

Lines of code

Total -> 1

28

Recommended Mitigation Steps

See the scratch space example.

[G-4] Use at least Solidity version 0.8.19 to gain some gas boost

Vulnerability Details

Upgrade to at least solidity version 0.8.19 or later to get additional gas savings. Check the documentation for reference.

Lines of code

Total -> 3

2, 2, 2

Impact

Some additional details:
In earlier releases and in the default legacy code generation, when an internal library function or a free function accessed via a module was called only during contract creation, e.g. only in the constructor, a copy of the function still also occurred in the contract’s runtime bytecode.
So a function pointer in creation code also refers to the offset of the function in runtime code, which requires thefunction to actually be present in runtime code.
For direct calls to internal contract functions the full encoding of the function expression is bypassed by the compiler. However, this bypassing did not happen for internallibrary functions and for free functions called via modules, causing the undesirable behaviour that is now fixed in this release.