Holograph contest - leosathya's results

[L-01] CONSIDER ADDINGS CHECKS FOR SIGNATURE MALLEABILITY

The ecrecover() function returns an address of zero when the signature does not match. This can cause problems if address zero is ever the owner of assets, and someone uses the permit function on address zero. If that happens, any invalid signature will pass the checks, and the assets will be stealable. In this case, the asset of concern is the vault’s ERC20 token, and fortunately OpenZeppelin’s implementation does a good job of making sure that address zero is never able to have a positive balance. If this contract ever changes to another ERC20 implementation that is laxer in its checks in favor of saving gas, this code may become a problem.

There are 1 instances of this issue:

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographFactory.sol#L333-L334

Recommended Mitigation

Use OpenZeppelin’s ECDSA contract rather than calling ecrecover() directly

[L-02] ABSENCE OF ZERO ADDRESS CHECK

There are 5 instances of this issue:

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L452-455 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L472-476 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L502-505 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L522-524 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L226-L229

Recommended Mitigation

Should be a require() condition that check for inputed addresses are zero address or not

[L-03] FOR SENDING ETH transfer() IS USED AND RETURN VALUE NOT CHECKED

The Istanbul hardfork increases the gas cost of the SLOAD operation and therefore breaks some existing smart contracts.

In file withdrawable.sol, contract uses transfer() to send eth from contract to EOA due which eth can get stuck.

The reason behind this is that, after the Istanbul hardfork, any smart contract that uses transfer() or send() is taking a hard dependency on gas costs by forwarding a fixed amount of gas (2300). This forwards 2300 gas, which may not be enough if the recipient is a contract and the cost of gas changes.

There are 2 instances of this issue:

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L596 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L396

Recommended Mitigation

Recommend using call() to send eth.

[L-04] RETURN VALUE OF IERC20.transfer() NOT CHECKED

Transfering ERC20 with transfer() Not all IERC20 implementations revert() when there’s a failure in transfer()/transferFrom(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment

There are 1 instances of this issue:

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L400

Recommended Mitigation

Use safeTransfer()/safeTransferFrom() or at least implement a return value check for ERC20 transfer() function

[L-05] MISSING EVENTS FOR ONLY FUNCTIONS THAT CHANGE CRITICAL PARAMETERS

The functions that change critical parameters should emit events. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.

There are multiple instances of this issue:

Recommended Mitigation

Recommended to emit events on critical variable change

[L-06] FUNCTION _payoutTokens() CAN LEAD TO DOS(Denial Of Service) CONDITION

This function using nested for loops and within them some other internal function call. More over these all operate on a dynamic array. If the array length increases significantly then may whole gas cost of calling this function may exceed from block gas limit and whole function reverted, so this will lead to a situation named Denial Of service

There are 1 instances of this issue:

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L426-L443

Recommended Mitigation

Properly check gas consumption for this function for different size of array. Then capped the array size, or specify some sorts of upper bound and handle large array in multiple time(multiple part).

[L-07] FUNCTION sourceMintBatch() DOES NOT CHECK WHETHER BOTH PARAMETER(DYNAMIC ARRAYS) ARE OF SAME LENGTH OR NOT

Function does not check both dynamic array have same length or not, if those have different lengths then further function call will give a unintensional result or result error,

As these inputs are provided by user, so human error is possible

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L563-L566

Recommended Mitigation

There should be a check that ensure that both inputed array have same length

[N-01] SOME COMMENTED CODE PRESENT IN CONTRACT FILE

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L525-L576

Recommended Mitigation

Remove those code before deployment

[G-01] Loop can be more optimizable

There are 15 instances of this issue:

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L781 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L871 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L307 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L323 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L340 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L356 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L394 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L414 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L432 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L437 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L454 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L474 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L357 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L716 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L564

Recommended Mitigation

. Should not initialize uint with default value i.e uint i=0 TO uint i; . Should use ++i instead i++ . Should uncheck i++

[G-02] USE CUSTOM ERRORS RATHER THAN REVERT()/REQUIRE() STRINGS TO SAVE GAS

Custom 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 1 instances of this issue:

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L214

Recommended Mitigation

require() with long error message length more than 32bytes can replace with custom error message to save gas.

[G-03] 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.

There are 26 instances of this issue:

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L285 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L969 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L989 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L1009 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L1029 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L1049 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L199 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L452 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L472 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L502 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L522 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographFactory.sol#L280 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographFactory.sol#L300 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/module/LayerZeroModule.sol#L320 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/module/LayerZeroModule.sol#L340 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/module/LayerZeroModule.sol#L360 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/module/LayerZeroModule.sol#L380 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/module/LayerZeroModule.sol#L441 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/module/LayerZeroModule.sol#L470 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L471 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L399 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L417 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L500 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L508 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L520 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L577

[G-04] Assigning default value to uint

There are 3 instances of this issue:

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L380 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L310 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L311

[G-05] Using Bools for storage incurs overhead

Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled.

There are 2 instances of this issue:

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L198 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L196

Recommended Mitigation

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

[G-06] <x> += <y> costs more gas than <x> = <x> + <y> for state variables

There are 11 instances of this issue:

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L378 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L382 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L834 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L1177 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L378 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L382 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L834 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L1175 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographFactory.sol#L328 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L685 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L686

[G-07] Divide by 2 should be bit-shift. If possible try to use bit-shift in replace of multiplication and division

There are multiple instances of this issue::

[G-08] Unchecking those arithmetic operation which can't overflow or underflow can save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

There are 3 instances of this issue ::

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L433 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L520 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L779

[G-09] Some public function could be external for saving gas

There is 14 instance of this issue ::

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L638 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L643 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L656 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L273 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L297 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L301 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L306 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L310 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L314 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L318 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L322 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L326 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L347 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L420 ** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L492

[G-10] Split require() statement that use && operator can help in save gas

There is 1 instances of this issue:

** File : ** => https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/Holographer.sol#L166