Platform: Code4rena
Start Date: 18/10/2022
Pot Size: $75,000 USDC
Total HM: 27
Participants: 144
Period: 7 days
Judge: gzeon
Total Solo HM: 13
Id: 170
League: ETH
Rank: 41/144
Findings: 2
Award: $82.02
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: Rolezn
Also found by: 0x1f8b, 0x52, 0x5rings, 0xNazgul, 0xSmartContract, 0xZaharina, 0xhunter, 0xzh, 8olidity, Amithuddar, Aymen0909, B2, Bnke0x0, Chom, Deivitto, Diana, Diraco, Dravee, Franfran, JC, Jeiwan, Josiah, JrNet, Jujic, KingNFT, KoKo, Lambda, Margaret, Migue, Ocean_Sky, PaludoX0, Picodes, Rahoz, RaoulSchaffranek, RaymondFam, RedOneN, ReyAdmirado, Shinchan, Tagir2003, Trust, Waze, Yiko, __141345__, a12jmx, adriro, ajtra, arcoun, aysha, ballx, bin2chen, bobirichman, brgltd, bulej93, catchup, catwhiskeys, caventa, cccz, cdahlheimer, ch0bu, chaduke, chrisdior4, cloudjunky, cryptostellar5, cryptphi, csanuragjain, cylzxje, d3e4, delfin454000, djxploit, durianSausage, erictee, fatherOfBlocks, francoHacker, gianganhnguyen, gogo, hansfriese, i_got_hacked, ignacio, imare, karanctf, kv, leosathya, louhk, lukris02, lyncurion, m_Rassska, malinariy, martin, mcwildy, mics, minhtrng, nicobevi, oyc_109, pashov, peanuts, pedr02b2, peiw, rbserver, ret2basic, rotcivegaf, rvierdiiev, ryshaw, sakman, sakshamguruji, saneryee, securerodd, seyni, sikorico, svskaushik, teawaterwire, tnevler, w0Lfrum
55.6726 USDC - $55.67
receive() or fallback() functionAlthough leaving the receive() below without a revert might save gas (as @dev states), it could result in a loss of funds for a user who sends Ether to the contract (having no way to get anything back)
HolographOperator.sol: L1207-1209
* @dev Purposefully left empty to ensure ether transfers use least amount of gas possible */ receive() external payable {}
Again, leaving the receive() below without a revert could result in a loss of funds for a user who sends Ether to the contract.
* @dev Purposefully left empty, to prevent running out of gas errors when receiving native token payments. */ receive() external payable {}
Similarly for the following:
NatSpec statements (including @dev @param) do not relate directly to the function below them (a function which may be incomplete).
HolographOperator.sol: L656-669
* @dev Will provide exact costs on protocol and message side, combine the two to get total * @dev @param toChain holograph chain id of destination chain for payload * @dev @param gasLimit amount of gas to provide for executing payload on destination chain * @dev @param gasPrice maximum amount to pay for gas price, can be set to 0 and will be chose automatically * @dev @param crossChainPayload the entire packet being sent cross-chain * @return hlgFee the amount (in wei) of native gas token that will cost for finalizing job on destiantion chain * @return msgFee the amount (in wei) of native gas token that will cost for sending message to destiantion chain */ function getMessageFee( uint32, uint256, uint256, bytes calldata ) external view returns (uint256, uint256) {
@return statementsThe functions in this section have complete NatSpec, apart from missing @return statements. Below is one example:
* @notice Used internally to initialize the contract instead of through a constructor * @dev This function is called by the deployer/factory when creating a contract * @param initPayload abi encoded payload to use for contract initilaization */ function init(bytes memory initPayload) external override returns (bytes4) {
Similarly for the following functions:
Multiple instances of NatSpec missing @return statements only also occur in the remaining contracts
@param statementsThe functions in this section are missing @param statements (as well as @return statements in a few cases). Below is one example:
HolographOperator.sol: L442-445
* @dev Purposefully made to be external so that Operator can call it during executeJob function * Check the executeJob function to understand it's implementation */ function nonRevertingBridgeCall(address msgSender, bytes calldata payload) external payable {
Missing: @param msgSender and @param payload
Similarly for the following:
HolographOperator.sol: L481-484
Missing: @param bridgeInRequestPayload
HolographOperator.sol: L574-590
Missing: @param msgSender
HolographOperator.sol: L1120-1122
Missing: @param pod and @param operatorIndex
HolographOperator.sol: L1158-1160
Missing: @return and @param pod
HolographOperator.sol: L1196-1198
Missing: @return and @param contractAddress
HolographFactory.sol: L156-163
Missing: @return and @param payload
HolographFactory.sol: L173-182
Missing: @return and @param payload
HolographFactory.sol: L307-309
Missing: @return and @param contractAddress
HolographFactory.sol: L318-326
Missing: @return and @param r, @param s, @param v, @param hash and @param signer
Multiple instances also occur in LayerZeroModule.sol, PA1D.sol, HolographERC721.sol, HolographERC20.sol, ERC721H.sol, and ERC20H.sol
The same typo (initilaization) occurs in all 10 lines referenced below:
* @param initPayload abi encoded payload to use for contract initilaization
Change initilaization to initialization in each case
The same typo (destiantion) occurs in all four lines referenced below:
* @return hlgFee the amount (in wei) of native gas token that will cost for finalizing job on destiantion chain
Change destiantion to destination in each case
The same typo (accomodate) occurs in both referenced below:
* decrease pod size to accomodate popped operator
Change accomodate to accommodate in both casees
* @dev bridgeInRequest doNotRevert is purposefully set to false so a rever would happen
Change rever to revert
* @dev All cross-chain message requests will get forwarded to this adress
Change adress to address
* @param config contract deployement configurations
Change deployement to deployment
* @param recipients Address array of wallets that will receive tha royalties.
Change tha to the
* @dev Use this modifier to lock public functions that should not be accesible to non-owners.
Change accesible to accessible
// The slot hash has been precomputed for gas optimizaion
Change optimizaion to optimization
* @dev This function validates that the call is being made by an authorised wallet.
Change authorised to authorized. The American English spelling of this word is used elsewhere in Holograph/PA1D, so this version should be used throughout for consistency
* @dev Will revert entire tranaction if it fails.
Change tranaction to tranaction
require(addresses.length == bps.length, "PA1D: missmatched array lenghts");
Change missmatched to mismatched and lenghts to lengths
require(totalBp == 10000, "PA1D: bps down't equal 10000");
Change down't to don't
* @dev Usually utilised for supporting marketplace proxy wallets.
Change utilised to utilized. Again, this assumes the use of American Engligh throughout for consistency
require(success && selector == InitializableInterface.init.selector, "ERC721: coud not init PA1D");
Change coud to could
* @dev Defaults the the Arweave URI
Remove repeated word the
// require(wallets.length == tokenIds.length, "ERC721: array length missmatch");
Change missmatch to mismatch
* @dev Single operator set for a specific token. Usually used for one-time very specific authorisations.
Change authorisations to authorizations, again assuming American English
* @dev Mapping of all the addresse's balances.
Change addresse's to addressee's, assuming this was what was meant
Comments that contain TODOs or other language referring to open items should be addressed and better explained, or else modified or removed. Below are several instances:
* @notice Do not call this function, it will always revert
HolographOperator.sol: L275-276
* @dev temp function, used for quicker updates/resets during development * NOT PART OF FINAL CODE !!!
// TODO: move the bit-shifting around to have it be sequential
// To be quite honest, SuperRare is a closed marketplace. They're working on opening it up but looks like they want to use private smart contracts. // We'll just leave this here for just in case they open the flood gates.
// this information is outside of the scope of our [line appears to be truncated]
* @dev Since it's not being used, the _data variable is commented out to avoid compiler warnings. * are aware of the ERC721 protocol to prevent tokens from being forever locked.
Named return variable (here, totalPods) is never used
HolographOperator.sol: L714-719
* @notice Get number of pods available * @dev This returns number of pods that have been opened via bonding */ function getTotalPods() external view returns (uint256 totalPods) { return _operatorPods.length; }
In theory, comments over 80 characters should wrap using multi-line comment syntax. Even if somewhat longer comments (up to 120 characters) are acceptable and a scroll bar is provided, there are cases where very long comments interfere with readability. Below are five instances of extra-long comments whose readability could be improved by wrapping, as shown:
* @dev The contract abstracts all the complexities of making bridge requests and uses a universal interface to bridge any type of holographable assets
Suggestion:
* @dev The contract abstracts all the complexities of making bridge requests and * uses a universal interface to bridge any type of holographable assets.
* @dev The contract provides methods that allow for the creation of Holograph Protocol compliant smart contracts, that are capable of minting holographable assets
Suggestion:
* @dev The contract provides methods that allow for the creation of Holograph Protocol * compliant smart contracts, that are capable of minting holographable assets.
* @dev Returns the block height of when the smart contract was deployed. Useful for retrieving deployment config for re-deployment on other EVM-compatible chains.
Suggestion:
* @dev Returns the block height of when the smart contract was deployed. Useful for * retrieving deployment config for re-deployment on other EVM-compatible chains.
* @param bps Uint256 array of base points(percentages) that each wallet(specified in recipients) will receive from the royalty payouts. Make sure that all the base points add up to a total of 10000.
Suggestion:
* @param bps Uint256 array of base points(percentages) that each wallet(specified in recipients) will receive * from the royalty payouts. Make sure that all the base points add up to a total of 10000.
// To be quite honest, SuperRare is a closed marketplace. They're working on opening it up but looks like they want to use private smart contracts.
Suggestion:
// To be quite honest, SuperRare is a closed marketplace. They're working on // opening it up but looks like they want to use private smart contracts.
require revert stringA require message should be included to enable users to understand the reason for failure
Recommendation: Add a brief (<= 32 char) explanatory message to each of the 31 requires referenced below. Also, consider using custom error codes (which would be cheaper than revert strings).
require(SourceERC721().afterApprove(tokenOwner, to, tokenId));
require(SourceERC721().beforeBurn(wallet, tokenId));
require(SourceERC721().afterBurn(wallet, tokenId));
require(SourceERC721().beforeSafeTransfer(from, to, tokenId, data));
require(SourceERC721().afterSafeTransfer(from, to, tokenId, data));
require(SourceERC721().beforeApprovalAll(to, approved));
require(SourceERC721().afterApprovalAll(to, approved));
require(SourceERC721().beforeTransfer(from, to, tokenId, data));
require(SourceERC721().afterTransfer(from, to, tokenId, data));
require(SourceERC721().beforeOnERC721Received(_operator, _from, address(this), _tokenId, _data));
require(SourceERC721().afterOnERC721Received(_operator, _from, address(this), _tokenId, _data));
require(SourceERC20().beforeApprove(msg.sender, spender, amount));
require(SourceERC20().afterApprove(msg.sender, spender, amount));
require(SourceERC20().beforeBurn(msg.sender, amount));
require(SourceERC20().afterBurn(msg.sender, amount));
require(SourceERC20().beforeBurn(account, amount));
require(SourceERC20().afterBurn(account, amount));
require(SourceERC20().beforeApprove(msg.sender, spender, newAllowance));
require(SourceERC20().afterApprove(msg.sender, spender, newAllowance));
require(SourceERC20().beforeApprove(msg.sender, spender, newAllowance));
require(SourceERC20().afterApprove(msg.sender, spender, newAllowance));
require(SourceERC20().beforeOnERC20Received(account, sender, address(this), amount, data));
require(SourceERC20().afterOnERC20Received(account, sender, address(this), amount, data));
require(SourceERC20().beforeApprove(account, spender, amount));
require(SourceERC20().afterApprove(account, spender, amount));
require(SourceERC20().beforeSafeTransfer(msg.sender, recipient, amount, data));
require(SourceERC20().afterSafeTransfer(msg.sender, recipient, amount, data));
require(SourceERC20().beforeSafeTransfer(account, recipient, amount, data));
require(SourceERC20().afterSafeTransfer(account, recipient, amount, data));
require(SourceERC20().beforeTransfer(msg.sender, recipient, amount));
require(SourceERC20().afterTransfer(msg.sender, recipient, amount));
require(SourceERC20().beforeTransfer(account, recipient, amount));
require(SourceERC20().afterTransfer(account, recipient, amount));
indexed fieldsAn event should use three indexed fields if there are three or more fields
event SecondarySaleFees(uint256 tokenId, address[] recipients, uint256[] bps);
For readability, scientific notation for large multiples of ten is preferable to using exponentiation
_baseBondAmount = 100 * (10**18); // one single token unit * 100
Suggestion: Change 10**18 to 1e18
return (((gasPrice * (gasLimit + (gasLimit / 10))) * dstPriceRatio) / (10**10), nativeFee);
return ((gasPrice * (gasLimit + (gasLimit / 10))) * dstPriceRatio) / (10**10);
Suggestion: Change 10**10 to 1e10 in both cases
🌟 Selected for report: oyc_109
Also found by: 0x040, 0x1f8b, 0x5rings, 0xNazgul, 0xSmartContract, 0xZaharina, 0xsam, 0xzh, 2997ms, Amithuddar, Aymen0909, B2, Bnke0x0, Deivitto, Diana, Dinesh11G, Franfran, JC, JrNet, Jujic, KingNFT, KoKo, Mathieu, Metatron, Mukund, Olivierdem, PaludoX0, Pheonix, Picodes, RaymondFam, RedOneN, ReyAdmirado, Rolezn, Saintcode_, Satyam_Sharma, Shinchan, Tagir2003, Tomio, Waze, Yiko, __141345__, adriro, ajtra, aysha, ballx, beardofginger, bobirichman, brgltd, bulej93, catchup, catwhiskeys, cdahlheimer, ch0bu, chaduke, chrisdior4, cryptostellar5, cylzxje, d3e4, delfin454000, dharma09, djxploit, durianSausage, emrekocak, erictee, exolorkistis, fatherOfBlocks, gianganhnguyen, gogo, halden, hxzy, i_got_hacked, iepathos, karanctf, leosathya, lucacez, lukris02, lyncurion, m_Rassska, martin, mcwildy, mics, nicobevi, peanuts, peiw, rbserver, ret2basic, rotcivegaf, ryshaw, sakman, sakshamguruji, saneryee, sikorico, skyle, svskaushik, tnevler, vv7, w0Lfrum, zishansami
26.3525 USDC - $26.35
!= 0 instead of > 0 in a require statement if variable is an unsigned integer!= 0 should be used where possible since > 0 costs more gas
require(timeDifference > 0, "HOLOGRAPH: operator has time");
Change timeDifference > 0 to timeDifference != 0
require(tokenId > 0, "ERC721: token id cannot be zero");
Change tokenId > 0 to tokenId != 0
require functionSplitting into separate require() statements saves gas
require(_bondedOperators[operator] == 0 && _bondedAmounts[operator] == 0, "HOLOGRAPH: operator is bonded");
Recommendation:
require(_bondedOperators[operator] == 0, "HOLOGRAPH: operator is bonded"); require(_bondedAmounts[operator] == 0, "HOLOGRAPH: operator is bonded");
require(success && selector == InitializableInterface.init.selector, "initialization failed");
Recommendation:
require(success, "initialization failed"); require(selector == InitializableInterface.init.selector, "initialization failed");
require(success && selector == InitializableInterface.init.selector, "ERC721: coud not init PA1D");
Recommendation:
require(success, "ERC721: could not init PA1D"); require(selector == InitializableInterface.init.selector, "ERC721: could not init PA1D");
require( (ERC165(to).supportsInterface(ERC165.supportsInterface.selector) && ERC165(to).supportsInterface(ERC721TokenReceiver.onERC721Received.selector) && ERC721TokenReceiver(to).onERC721Received(address(this), from, tokenId, data) == ERC721TokenReceiver.onERC721Received.selector), "ERC721: onERC721Received fail" );
Recommendation:
require(ERC165(to).supportsInterface(ERC165.supportsInterface.selector), "ERC721: onERC721Received fail"); require(ERC165(to).supportsInterface(ERC721TokenReceiver.onERC721Received.selector), "ERC721: onERC721Received fail"); require(ERC721TokenReceiver(to).onERC721Received(address(this), from, tokenId, data) == ERC721TokenReceiver.onERC721Received.selector), "ERC721: onERC721Received fail");
Require revert string is too longThe revert strings below can be shortened to 32 characters or fewer (as shown) to save gas (or else consider using custom error codes throughout, which could save even more gas)
require(balance > 10000, "PA1D: Not enough tokens to transfer");
Change message to PA1D: Not enough tokens to xfer
Similarly for: PA1D.sol: L435
Initializing uint variables to their default value of 0 (as occurs below) is unnecessary and costs gas
uint256 fee = 0;
Change to: uint256 fee;
HolographOperator.sol: L310-311
uint256 gasLimit = 0; uint256 gasPrice = 0;
Change to: uint256 gasLimit; and uint256 gasPrice;
for loopSince calculating the array length costs gas, it's best to read the length of the array from memory before executing the loop, as demonstrated below:
for (uint256 t = 0; t < tokenAddresses.length; t++) { erc20 = ERC20(tokenAddresses[t]); balance = erc20.balanceOf(address(this)); require(balance > 10000, "PA1D: Not enough tokens to transfer"); // uint256 sent; for (uint256 i = 0; i < addresses.length; i++) { sending = ((bps[i] * balance) / 10000); require(erc20.transfer(addresses[i], sending), "PA1D: Couldn't transfer token"); // sent = sent + sending; } }
The above has an embedded for loop. The suggestion below addresses both of the loops:
uint256 totalTokenAddressesLength = tokenAddresses.length; uint256 totalAddressesLength = addresses.length; for (uint256 t = 0; t < totalTokenAddressesLength; t++) { erc20 = ERC20(tokenAddresses[t]); balance = erc20.balanceOf(address(this)); require(balance > 10000, "PA1D: Not enough tokens to transfer"); // uint256 sent; for (uint256 i = 0; i < totalAddressesLength; i++) { sending = ((bps[i] * balance) / 10000); require(erc20.transfer(addresses[i], sending), "PA1D: Couldn't transfer token"); // sent = sent + sending; } }
Similarly for the additional for loops referenced below:
++i instead of i++ to increase count in a for loopi++ (or similar counters) are used throughout Holograph. It would be better to use ++i, as demonstrated below, since use of i++ costs more gas :
HolographOperator.sol: L781-783
for (uint256 i = 0; i < length; i++) { operators[i] = _operatorPods[pod][index + i]; }
Suggestion:
for (uint256 i = 0; i < length; ++i) { operators[i] = _operatorPods[pod][index + i]; }
for loopUnderflow/overflow checks are made every time i++ (or equivalent counter) is called. Such checks are unnecessary since i is already limited.
I found only one instance of a for loop that used unchecked behavior: HolographOperator.sol: L871-876 Therefore, use unchecked behavior for the remaining loops, as shown below:
HolographOperator.sol: L781-783
for (uint256 i = 0; i < length; i++) { operators[i] = _operatorPods[pod][index + i]; }
Suggestion:
for (uint256 i = 0; i < length;) { operators[i] = _operatorPods[pod][index + i]; unchecked { ++i; } }
Note that i++ has been changed to ++i, as recommended earlier in this submission