Holograph contest - sakshamguruji's results

Use modifiers instead of using similar require statements

Description:

Using the same require statements again and again may make the code less readable , instead put those conditions inside a modifier and use those modifier on the function.

Affected code:

https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L203 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L255 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L352

consider putting them inside a modifier.

Functions lack natspec comments/Dev comments

Description:

There were many instances where the functions lacked natspec comments/dev comments telling about the functionality of the function.

Affected Functions without comments: https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L298 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L298 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L332 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L349 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L365 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L372 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L558 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L634 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L413 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L643

Wrong comment Describing a mapping

Description:

The mapping on https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographBridge.sol#L352 and https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L223 have the same description , whereas the comments describing operatorPodIndex should be something like the mapping for an address to the index of the operator pod instead of Internal mapping of bonded operators, to prevent double bonding

Consider Changing the name of the function to something else

Description:

The function on https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/PA1D.sol#L205 is being declared as onlyOwner whereas it permits the access to admins too(not owners) , there fore consider changing the name of the function to something like onlyAuth or something else.

Wrong grammar used

Description:

https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L319 Use single the instead of two the

Considering changing the name of the function to avoid confusion

Description:

The functions on https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L336 and https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L347 have the same name i.e tokensOfOwner , instead consider changing the latter to tokensOfOwnerLength to avoid confusion.

No zero address check On token approval

Description:

The function on https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L368 does not check if the to address is 0 address or not , if it is a zero address the token would be assigned to the zero address and be lost forever as the mapping _tokenOwner[tokenId] would now point to zero address and no further approvals can be made.

Add a zero address check for this .

Similar thing happens on https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L483 where there are no zero address checks on the address to , a user can mistakenly give the zero address in the to field and approve all his/her nft's to the zero address making them lost/locked forever

MULTIPLE ADDRESS/ID MAPPINGS CAN BE COMBINED INTO A SINGLE MAPPING OF AN ADDRESS/ID TO A STRUCT, WHERE APPROPRIATE

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations.

Affected Code(mappings):

https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L170 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L175 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L180 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L185 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L190 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L201 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L206

X += Y costs 22 more gas than X = X + Y (Same with -)

This can mean a lot of gas wasted in a function call when the computation is repeated n times (loops) Recommended Mitigation use X = X + Y instead of X += Y (same with -).

Affected Code:

https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L378

USING PRIVATE RATHER THAN PUBLIC/INTERNAL FOR CONSTANTS, SAVES GAS

If needed, the value can be read from the verified contract source code. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment call data, 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. As the constants declared in the holograph contracts have no visibility set , they are internal by default , change them to private.

Affected code:

https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L129 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L133 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L137 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L141 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L145 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L149 https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L153

Similarly consider changing the constants for all the contracts