Holograph contest - Josiah's results

COMMENT AND CODE LINE LENGTH

Try limit the length of comments and/or code lines to 80 - 100 character long for readability sake. Here is one instance found.

Line 289

CAPITALIZATION OF CONSTANTS

As documented in the link below:

https://docs.soliditylang.org/en/v0.8.17/style-guide.html?highlight=capitalized

Constants should be named with all capital letters with underscores separating words. There are numerous instances found.

Lines 120 - 152 Lines 126 - 142 Lines 127 -131 Lines 129 - 153 Lines 126 - 146 Lines 124 - 144 Lines 136 - 140 Lines 142 - 146 Lines 110 - 114 Lines 110 - 114

UNUSED RECEIVE() FUNCTION WILL LOCK ETHER IN CONTRACT

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert. There are five instances found.

Line 212 Line 212 Line 223 Line 962 Line 209 Line 251

USE OF BLOCK.TIMESTAMP

Some contract code uses the block.timestamp as part of the calculations and time checks. Nevertheless, timestamps can be slightly altered by miners/validators to favor them in contracts that have logics that depend strongly on them.

Consider taking into account this issue and warning the users that such a scenario could happen. If the alteration of timestamps cannot affect the protocol in any way, consider documenting the reasoning and writing tests enforcing that these guarantees will be preserved even if the code changes in the future. Here are four instances found.

Line 469 Line 345 Line 531 Line 499

Note that the last instance that involves a random number generation would need to be used with much care although it might have been mitigated by jobHash and _jobNonce()`. Elsewhere, it could have easily been manipulated by the miners or validators.

NOT USING OPENZEPPELIN CONTRACTS

OpenZeppelin maintains a library of standard, audited, community-reviewed, and battle-tested smart contracts. Instead of always importing these contracts, the Holograph project reimplements them in some cases, while in other cases it just copies them. This increases the amount of code that the Holograph team will have to maintain and misses all the improvements and bug fixes that the OpenZeppelin team is constantly implementing with the help of the community.

Consider importing the OpenZeppelin contracts instead of reimplementing or copying them. These contracts can be extended to add the extra functionalities required by Holograph. Here are some instances found.

Line 105 Line 115 Line 107 Line 130

PAYABLE(<ADDRESS>).TRANSFER COULD CAUSE ETHER UNABLE TO SEND

When sending ETH, HolographOperator.sol uses Solidity’s transfer function. This has some notable shortcomings when the address is a smart contract, which can render ETH impossible to withdraw. Specifically, the withdrawal will inevitably fail when: 1) The withdrawer smart contract does not implement a payable fallback function. 2) The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units. 3) The withdrawer smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300. Here is an instance found.

Line 497

It is recommended that sendValue function available in OpenZeppelin Contract’s Address library can be used to transfer the withdrawn Ether without being limited to 2300 gas units. Alternatively, a low level call() may also be adopted. Risks of reentrancy stemming from the use of this function can be mitigated by tightly following the “Check-effects-interactions” pattern and using OpenZeppelin Contract’s ReentrancyGuard contract.

MISSING EVENTS ON CRITICAL OPERATIONS

Several critical operations do not trigger events, which will make it difficult to review the correct behavior of the contracts once deployed. Users and blockchain monitoring systems will not be able to easily detect suspicious behaviors without events. There are numerous instances found.

Lines 340 - 344 Lines 380 - 384 Lines 470 - 474

Add a Timelock to Critical Parameter Change

It is a good practice to give time for users to react and adjust to critical changes with a mandatory time window between them. The first step merely broadcasts to users that a particular change is coming, and the second step commits that change after a suitable waiting period. This allows users that do not accept the change to withdraw within the grace period. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious Owner making any malicious or ulterior intention). Specifically, privileged roles could use front running to make malicious changes just ahead of incoming transactions, or purely accidental negative effects could occur due to the unfortunate timing of changes. Here are some of the instances entailed:

Lines 340 - 344 Lines 360 - 364 Lines 441 -445

SETTING TO DEFAULT VALUES

As documented in the link:

https://docs.soliditylang.org/en/v0.8.17/types.html?highlight=delete#delete

It is recommended using delete whenever there is a need to set state variable to its default value, which has a good side effect of saving gas. Here are the instances found.

Lines 667 - 668 Line 399 Line 924 Lines 1132 - 1136

DOS ON UNBOUNDED LOOP

Unbounded loop could lead to OOG (Out of Gas) denying the users' of needed services. Here are some instances found.

Line 781 Line 564 Line 357 Line 716

UNCOMMENTED ASSEMBLY BLOCK

Assembly is a low-level language that is harder to parse by readers, consider including extensive documentation regarding the rationale behind its use, clearly explaining what every single assembly instruction does. This will make it easier for users to trust the code, for reviewers to verify it, and for developers to build on top of it or update it. Note that the use of assembly discards several important safety features of Solidity, which may render the code unsafer and more error-prone. Here are some instances found.

Lines 937 - 997 Lines 218 -227

ECRECOVER SUBJECT TO REPLAY ATTACK

Whenever possible, use OpenZeppelin’s ECDSA library which has been time tested in preventing replay attack of signature malleability associated with `ecrecover'. The issue could arise from the non-unique value of v and an s value that could end up in the upper half of the modulo set. Here are the instances found.

Lines 333 - 334

COMMENTED CODE LINES

Throughout the codebase there are lines of code that have been commented out with //. This can lead to confusion and is detrimental to overall code readability. Consider removing commented out lines of code that are no longer needed. Here are the instances found.

Lines 527 - 537 Lines 542 - 552 Lines 557 - 570 Lines 580 - 587

COMMENT AND CODE MISMATCH

The comment of the following instance mentioned 2300 gas stipend for each 1x. However, the 23300 was used in the code instead.

Line3 386 - 388

MISSING NATSPEC

According to the following documentation:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

Some instances are missing @param.

Line 484 Line 1160 Line 1185