Automated Findings
Summary
Medium Risk Issues
| Issue | Instances | |
|---|---|---|
| [M‑01] | The owner is a single point of failure and a centralization risk |
3 |
| [M‑02] | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() |
1 |
Total: 4 instances over 2 issues
Low Risk Issues
| Issue | Instances | |
|---|---|---|
| [L‑01] | Use Ownable2Step rather than Ownable |
1 |
| [L‑02] | Missing checks for address(0x0) in the constructor |
1 |
| [L‑03] | Execution at deadlines should be allowed | 5 |
| [L‑04] | External calls in an un-bounded loop may result in a DOS | 1 |
| [L‑05] | Loss of precision | 1 |
| [L‑06] | Unsafe downcast | 1 |
| [L‑07] | Avoid double casting | 3 |
Total: 13 instances over 7 issues
Non-critical Issues
| Issue | Instances | |
|---|---|---|
| [N‑01] | Variable names that consist of all capital letters should be reserved for constant/immutable variables |
4 |
| [N‑02] | Constants in comparisons should appear on the left side | 7 |
| [N‑03] | Consider disabling renounceOwnership() |
1 |
| [N‑04] | public functions not called by the contract should be declared external instead |
6 |
| [N‑05] | Function ordering does not follow the Solidity style guide | 1 |
| [N‑06] | Large multiples of ten should use scientific notation (e.g. 1e6) rather than decimal literals (e.g. 1000000), for readability |
2 |
| [N‑07] | Contract does not follow the Solidity style guide's suggested layout ordering | 1 |
| [N‑08] | NatSpec @param is missing |
4 |
| [N‑09] | NatSpec @return argument is missing |
2 |
| [N‑10] | Lines are too long | 2 |
| [N‑11] | Typos | 3 |
| [N‑12] | Non-external/public variable names should begin with an underscore |
5 |
| [N‑13] | Constants should be defined rather than using magic numbers | 2 |
| [N‑14] | Functions not used internally could be marked external | 6 |
| [N‑15] | Consider using named mappings | 2 |
Total: 48 instances over 15 issues
Gas Optimizations
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [G‑01] | Reduce gas usage by moving to Solidity 0.8.19 or later | 3 | - |
| [G‑02] | Use assembly to check for address(0) |
7 | - |
| [G‑03] | Use assembly to emit events, in order to save gass | 3 | 114 |
| [G‑04] | Using bools for storage incurs overhead |
1 | 1100 |
| [G‑05] | Cache array length outside of loop | 1 | - |
| [G‑06] | The result of function calls should be cached rather than re-calling the function | 1 | - |
| [G‑07] | internal functions only called once can be inlined to save gas |
3 | 60 |
| [G‑08] | <array>.length should not be looked up in every loop of a for-loop |
1 | 3 |
| [G‑09] | Not using the named return variables anywhere in the function is confusing | 5 | - |
| [G‑10] | Constructors can be marked payable |
3 | 63 |
| [G‑11] | Functions guaranteed to revert when called by normal users can be marked payable |
3 | 63 |
| [G‑12] | <x> += <y> costs more gas than <x> = <x> + <y> for state variables |
1 | 113 |
| [G‑13] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) |
1 | 5 |
| [G‑14] | Using private rather than public for constants, saves gas |
2 | - |
| [G‑15] | Use a more recent version of solidity | 3 | - |
Total: 38 instances over 15 issues with 1521 gas saved
Medium Risk Issues
[M‑01] The owner is a single point of failure and a centralization risk
Having a single EOA as the only owner of contracts is a large centralization risk and a single point of failure. A single private key may be taken in a hack, or the sole holder of the key may become unable to retrieve the key when necessary. Consider changing to a multi-signature setup, or having a role-based authorization model.
There are 3 instances of this issue:
[M‑02] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). “Unless there is a compelling reason, abi.encode should be preferred”. If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
There is one instance of this issue:
GitHub: [227]
Low Risk Issues
[L‑01] Use Ownable2Step rather than Ownable
Ownable2Step and Ownable2StepUpgradeable prevent the contract ownership from mistakenly being transferred to an address that cannot handle it (e.g. due to a typo in the address), by requiring that the recipient of the owner permissions actively accept via a contract call of its own.
There is one instance of this issue:
GitHub: [37]
[L‑02] Missing checks for address(0x0) in the constructor
There is one instance of this issue:
[L‑03] Execution at deadlines should be allowed
The condition may be wrong in these cases, as when block.timestamp is equal to the compared > or < variable these blocks will not be executed.
There are 5 instances of this issue:
GitHub: [110, 134, 163, 187, 216]
[L‑04] External calls in an un-bounded loop may result in a DOS
Consider limiting the number of iterations in for-loops that make external calls
There is one instance of this issue:
GitHub: [147]
[L‑05] Loss of precision
Division by large numbers may result in the result being zero, due to solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator
There is one instance of this issue:
see instances
GitHub: [146]
[L‑06] Unsafe downcast
When a type is downcast to a smaller type, the higher order bits are truncated, effectively applying a modulo to the original value. Without any other checks, this wrapping will lead to unexpected behavior and bugs
There is one instance of this issue:
GitHub: [228]
[L‑07] Avoid double casting
Consider refactoring the following code, as double casting may introduce unexpected truncations and/or rounding issues.
Furthermore, double type casting can make the code less readable and harder to maintain, increasing the likelihood of errors and misunderstandings during development and debugging.
There are 3 instances of this issue:
see instances
Non-critical Issues
[N‑01] Variable names that consist of all capital letters should be reserved for constant/immutable variables
If the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).
There are 4 instances of this issue:
[N‑02] Constants in comparisons should appear on the left side
Doing so will prevent typo bugs
There are 7 instances of this issue:
see instances
[N‑03] Consider disabling renounceOwnership()
If the plan for your project does not include eventually giving up all ownership control, consider overwriting OpenZeppelin's Ownable's renounceOwnership() function in order to disable it.
There is one instance of this issue:
GitHub: [37]
[N‑04] public functions not called by the contract should be declared external instead
Contracts are allowed to override their parents' functions and change the visibility from external to public.
There are 6 instances of this issue:
GitHub: [105, 127, 152, 179, 205, 225]
[N‑05] Function ordering does not follow the Solidity style guide
According to the Solidity style guide, functions should be laid out in the following order :constructor(), receive(), fallback(), external, public, internal, private, but the cases below do not follow this pattern.
There is one instance of this issue:
see instances
GitHub: [38]
[N‑06] Large multiples of ten should use scientific notation (e.g. 1e6) rather than decimal literals (e.g. 1000000), for readability
There are 2 instances of this issue:
see instances
[N‑07] Contract does not follow the Solidity style guide's suggested layout ordering
The style guide says that, within a contract, the ordering should be 1) Type declarations, 2) State variables, 3) Events, 4) Modifiers, and 5) Functions, but the contract(s) below do not follow this ordering
There is one instance of this issue:
see instances
GitHub: [37]
[N‑08] NatSpec @param is missing
There are 4 instances of this issue:
see instances
GitHub: [42]
[N‑09] NatSpec @return argument is missing
There are 2 instances of this issue:
see instances
[N‑10] Lines are too long
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. The solidity style guide recommends a maximum line length of 120 characters, so the lines below should be split when they reach that length.
There are 2 instances of this issue:
see instances
[N‑11] Typos
There are 3 instances of this issue:
GitHub: [140]
[N‑12] Non-external/public variable names should begin with an underscore
According to the Solidity Style Guide, Non-external/public variable names should begin with an underscore
There are 5 instances of this issue:
see instances
[N‑13] Constants should be defined rather than using magic numbers
Even assembly can benefit from using readable constants instead of hex/numeric literals
There are 2 instances of this issue:
see instances
GitHub: [135]
GitHub: [227]
[N‑14] Functions not used internally could be marked external
There are 6 instances of this issue:
GitHub: [105, 127, 152, 179, 205, 225]
[N‑15] Consider using named mappings
Consider moving to solidity version 0.8.18 or later, and using named mappings to make it easier to understand the purpose of each mapping
There are 2 instances of this issue:
see instances
Gas Optimizations
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
[G‑01] Reduce gas usage by moving to Solidity 0.8.19 or later
See this link for the full details
There are 3 instances of this issue:
see instances
GitHub: [24]
GitHub: [24]
GitHub: [24]
[G‑02] Use assembly to check for address(0)
Saves 6 gas per instance
There are 7 instances of this issue:
[G‑03] Use assembly to emit events, in order to save gass
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.
There are 3 instances of this issue:
GitHub: [155]
[G‑04] Using bools for storage incurs overhead
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past
There is one instance of this issue:
GitHub: [71]
[G‑05] Cache array length outside of loop
If not cached, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first).
There is one instance of this issue:
GitHub: [83]
[G‑06] The result of 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 one instance of this issue:
GitHub: [227]
[G‑07] internal functions only called once can be inlined to save gas
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 3 instances of this issue:
[G‑08] <array>.length should not be looked up in every loop of a for-loop
The overheads outlined below are PER LOOP, excluding the first loop
storage arrays incur a Gwarmaccess (100 gas)
memory arrays use
MLOAD(3 gas)calldata arrays use
CALLDATALOAD(3 gas)
There is one instance of this issue:
GitHub: [83]
[G‑09] Not using the named return variables anywhere in the function is confusing
Consider changing the variable to be an unnamed one, since the variable is never assigned, nor is it returned by name. If the optimizer is not turned on, leaving the code as it is will also waste gas for the stack variable.
There are 5 instances of this issue:
GitHub: [225]
[G‑10] Constructors can be marked payable
Payable functions cost less gas to execute, since the compiler does not have to add extra checks to ensure that a payment wasn't provided. A constructor can safely be marked as payable, since only the deployer would be able to pass funds, and the project itself would not pass any funds.
There are 3 instances of this issue:
see instances
GitHub: [70]
GitHub: [44]
GitHub: [81]
[G‑11] 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. 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
There are 3 instances of this issue:
see instances
[G‑12] <x> += <y> costs more gas than <x> = <x> + <y> for state variables
Using the addition operator instead of plus-equals saves 113 gas
There is one instance of this issue:
GitHub: [129]
[G‑13] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)
Saves 5 gas per loop
There is one instance of this issue:
GitHub: [87]
[G‑14] Using private rather than public for constants, saves gas
If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas 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
There are 2 instances of this issue:
[G‑15] Use a more recent version of solidity
Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
There are 3 instances of this issue:
Prize pool breakdown
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.