Holograph contest - rotcivegaf's results

QA report

Low Risk

Non-critical

Low Risk

[L-01] Open TODO

Open TODO can point to architecture or programming issues that still need to be resolved.

File: /contracts/HolographOperator.sol

701        // TODO: move the bit-shifting around to have it be sequential

[L-02] Send ether with call instead of transfer

Use call instead of transfer to send ether. And return value must be checked if sending ether is successful or not. Sending ether with the transfer is no longer recommended.

This transaction will fail inevitably when:

1. The _to smart contract does not implement a payable function.
2. _to smart contract does implement a payable fallback which uses more than 2300 gas unit.
3. The _to smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call’s gas usage above 2300.

File: /contracts/HolographOperator.sol

596    payable(hToken).transfer(hlgFee);

File: /contracts/enforcer/PA1D.sol

396      addresses[i].transfer(sending);

[L-03] The sourceGetChainPrepend function don't return only the prepend

This function return an uint256 with _chainId()(uint32) plus 0(uint224), always return the token of tokenId 0

File: /contracts/enforcer/HolographERC721.sol

From:
520  function sourceGetChainPrepend() external view onlySource returns (uint256) {
521    return uint256(bytes32(abi.encodePacked(_chain(), uint224(0))));
522  }
To:
520  function sourceGetChainPrepend() external view onlySource returns (uint32) {
521    return _chain();
522  }
Other option:
520  function sourceGetChainPrepend(uint224 tokenId) external view onlySource returns (uint256) {
521    return uint256(bytes32(abi.encodePacked(_chain(), tokenId)));
522  }

[L-04] Wrong selector return

The initPA1D function should return: PA1D.initPA1D.selector

File: /contracts/enforcer/PA1D.sol

197    return InitializableInterface.init.selector;

[L-05] Unused Admin abstract contract

The Admin abstract contract in contracts/enforcer/HolographERC721.sol and contracts/enforcer/HolographERC20.sol it's unused, remove this inherited contract

[L-06] Not random

In the contract HolographOperator, the random generated by uint256 random = uint256(keccak256(abi.encodePacked(jobHash, _jobNonce(), block.number, block.timestamp))); could be calculate If the _messagingModule don't like this random could revert the transaction and send another to get the beneficied random that benefits it

[L-07] Not initialize slots

The HolographERC721 don't initialize the _adminSlot slot in the init function, although the abstract contract Admin is not used, the expect admin could want use the adminCall Also the _holographSlot and the _sourceContractSlot Same for the HolographERC20 contract

Recommended Mitigation Steps: initialize this slots in the init function

[L-08] Array length not checked in sourceMintBatch

Check if the wallets and amounts arrays have the same length before using them

File: contracts/enforcer/HolographERC20.sol

197  function sourceMintBatch(address[] calldata wallets, uint256[] calldata amounts) external onlySource {

Non-critical

[N-01] Unused constructor

Remove constructor if is empty, to clarify the code

File: /contracts/HolographBridge.sol

155  constructor() {}

File: /contracts/HolographFactory.sol

136  constructor() {}

File: /contracts/HolographOperator.sol

233  constructor() {}

File: /contracts/abstract/ERC20H.sol

133  constructor() {}

File: /contracts/abstract/ERC721H.sol

133  constructor() {}

File: /contracts/enforcer/Holographer.sol

140  constructor() {}

File: /contracts/enforcer/HolographERC20.sol

211  constructor() {}

File: /contracts/enforcer/HolographERC721.sol

231  constructor() {}

File: /contracts/enforcer/PA1D.sol

166  constructor() {}

File: /contracts/module/LayerZeroModule.sol

151  constructor() {}

[N-02] Unused commented code lines

Remove the commented code blocks, to clarify the code

File: /contracts/enforcer/HolographERC721.sol

524  /**
525   * @dev Allows for source smart contract to mint a batch of tokens.
526   */
527  //   function sourceMintBatch(address to, uint224[] calldata tokenIds) external onlySource {
528  //     require(tokenIds.length < 1000, "ERC721: max batch size is 1000");
529  //     uint32 chain = _chain();
530  //     uint256 token;
531  //     for (uint256 i = 0; i < tokenIds.length; i++) {
532  //       require(!_burnedTokens[token], "ERC721: can't mint burned token");
533  //       token = uint256(bytes32(abi.encodePacked(chain, tokenIds[i])));
534  //       require(!_burnedTokens[token], "ERC721: can't mint burned token");
535  //       _mint(to, token);
536  //     }
537  //   }

539  /**
540   * @dev Allows for source smart contract to mint a batch of tokens.
541   */
542  //   function sourceMintBatch(address[] calldata wallets, uint224[] calldata tokenIds) external onlySource {
543  //     require(wallets.length == tokenIds.length, "ERC721: array length missmatch");
544  //     require(tokenIds.length < 1000, "ERC721: max batch size is 1000");
545  //     uint32 chain = _chain();
546  //     uint256 token;
547  //     for (uint256 i = 0; i < tokenIds.length; i++) {
548  //       token = uint256(bytes32(abi.encodePacked(chain, tokenIds[i])));
549  //       require(!_burnedTokens[token], "ERC721: can't mint burned token");
550  //       _mint(wallets[i], token);
551  //     }
552  //   }

554  /**
555   * @dev Allows for source smart contract to mint a batch of tokens.
556   */
557  //   function sourceMintBatchIncremental(
558  //     address to,
559  //     uint224 startingTokenId,
550  //     uint256 length
561  //   ) external onlySource {
562  //     uint32 chain = _chain();
563  //     uint256 token;
564  //     for (uint256 i = 0; i < length; i++) {
565  //       token = uint256(bytes32(abi.encodePacked(chain, startingTokenId)));
566  //       require(!_burnedTokens[token], "ERC721: can't mint burned token");
567  //       _mint(to, token);
568  //       startingTokenId++;
569  //     }
570  //   }

File: /contracts/enforcer/PA1D.sol

393     // uint256 sent;

397      // sent = sent + sending;

413    //uint256 sent;

417      // sent = sent + sending;

436      // uint256 sent;

440        // sent = sent + sending;

579  // Rarible V2(not being used since it creates a conflict with Manifold royalties)
580  // struct Part {
581  //     address payable account;
582  //     uint96 value;
583  // }

585  // function getRoyalties(uint256 tokenId) public view returns (Part[] memory) {
586  //     return royalties[id];
587  // }

File: /contracts/HolographOperator.sol

 434    /**
 435     * @dev reward operator (with HLG) for executing the job
 436     * @dev this is out of scope and is purposefully omitted from code
 437     */
 438    ////  _bondedOperators[msg.sender] += reward;

1176      //       current += (current / _operatorThresholdDivisor) * position;

[N-03] Require/revert without custom errors

Use custom errors to give the idea what was the cause of failure

File: /contracts/enforcer/HolographERC20.sol

328      require(SourceERC20().beforeApprove(msg.sender, spender, amount));

332      require(SourceERC20().afterApprove(msg.sender, spender, amount));

339      require(SourceERC20().beforeBurn(msg.sender, amount));

343      require(SourceERC20().afterBurn(msg.sender, amount));

354      require(SourceERC20().beforeBurn(account, amount));

358      require(SourceERC20().afterBurn(account, amount));

371      require(SourceERC20().beforeApprove(msg.sender, spender, newAllowance));

375      require(SourceERC20().afterApprove(msg.sender, spender, newAllowance));

430      require(SourceERC20().beforeApprove(msg.sender, spender, newAllowance));

434      require(SourceERC20().afterApprove(msg.sender, spender, newAllowance));

447      require(SourceERC20().beforeOnERC20Received(account, sender, address(this), amount, data));

455      require(SourceERC20().afterOnERC20Received(account, sender, address(this), amount, data));

484      require(SourceERC20().beforeApprove(account, spender, amount));

488      require(SourceERC20().afterApprove(account, spender, amount));

502      require(SourceERC20().beforeSafeTransfer(msg.sender, recipient, amount, data));

507      require(SourceERC20().afterSafeTransfer(msg.sender, recipient, amount, data));

536      require(SourceERC20().beforeSafeTransfer(account, recipient, amount, data));

541      require(SourceERC20().afterSafeTransfer(account, recipient, amount, data));

582      require(SourceERC20().beforeTransfer(msg.sender, recipient, amount));

586      require(SourceERC20().afterTransfer(msg.sender, recipient, amount));

606      require(SourceERC20().beforeTransfer(account, recipient, amount));

610      require(SourceERC20().afterTransfer(account, recipient, amount));

File: /contracts/enforcer/HolographERC721.sol

373      require(SourceERC721().beforeApprove(tokenOwner, to, tokenId));

378      require(SourceERC721().afterApprove(tokenOwner, to, tokenId));

391      require(SourceERC721().beforeBurn(wallet, tokenId));

395      require(SourceERC721().afterBurn(wallet, tokenId));

460      require(SourceERC721().beforeSafeTransfer(from, to, tokenId, data));

473      require(SourceERC721().afterSafeTransfer(from, to, tokenId, data));

486      require(SourceERC721().beforeApprovalAll(to, approved));

491      require(SourceERC721().afterApprovalAll(to, approved));

624      require(SourceERC721().beforeTransfer(from, to, tokenId, data));

628      require(SourceERC721().afterTransfer(from, to, tokenId, data));

759      require(SourceERC721().beforeOnERC721Received(_operator, _from, address(this), _tokenId, _data));

767      require(SourceERC721().afterOnERC721Received(_operator, _from, address(this), _tokenId, _data));

File: /contracts/HolographBridge.sol

578    revert();

585    revert();

File: /contracts/HolographOperator.sol

1215    revert();

File: /contracts/module/LayerZeroModule.sol

417    revert();

424    revert();

[N-04] Typo

File: /contracts/enforcer/HolographERC721.sol

/// @audit: from "missmatched" to "mismatched"
472    require(addresses.length == bps.length, "PA1D: missmatched array lenghts");

/// @audit: from "lenghts" to "lengths"
472    require(addresses.length == bps.length, "PA1D: missmatched array lenghts");

/// @audit: from "down't" to "don't"
477    require(totalBp == 10000, "PA1D: bps down't equal 10000");

[N-05] Use address payable only when it's necessary(transfer)

Only in the L396 use something relate to transfer/send ether: addresses[i].transfer(sending); Change all address payable/address payable[] to address/address[] and cast to address payable the addresses[i] in L396: payable(addresses[i]).transfer(sending);

Address Docs

File: /contracts/enforcer/PA1D.sol

180    setRoyalties(0, payable(receiver), bp);

192    setRoyalties(0, payable(receiver), bp);

216  function _getDefaultReceiver() private view returns (address payable receiver) {

256  function _getReceiver(uint256 tokenId) private view returns (address payable receiver) {

298  function _getPayoutAddresses() private view returns (address payable[] memory addresses) {

305    addresses = new address payable[](length);

306    address payable value;

316  function _setPayoutAddresses(address payable[] memory addresses) private {

322    address payable value;

383    address payable[] memory addresses = _getPayoutAddresses();

406    address payable[] memory addresses = _getPayoutAddresses();

427    address payable[] memory addresses = _getPayoutAddresses();

452      address payable[] memory addresses = _getPayoutAddresses();

453      address payable sender = payable(msg.sender);

471  function configurePayouts(address payable[] memory addresses, uint256[] memory bps) public onlyOwner {

488  function getPayoutInfo() public view returns (address payable[] memory addresses, uint256[] memory bps) {

531    address payable receiver,

569  function getFeeRecipients(uint256 tokenId) public view returns (address payable[] memory) {

570    address payable[] memory receivers = new address payable[](1);

590  function getRoyalties(uint256 tokenId) public view returns (address payable[] memory, uint256[] memory) {

591    address payable[] memory receivers = new address payable[](1);

604  function getFees(uint256 tokenId) public view returns (address payable[] memory, uint256[] memory) {

605    address payable[] memory receivers = new address payable[](1);

[N-06] Wrong comment

File: /contracts/enforcer/HolographERC721.sol

446   * @dev Since it's not being used, the _data variable is commented out to avoid compiler warnings.
447   * are aware of the ERC721 protocol to prevent tokens from being forever locked.

[N-07] Constants should be defined and documented rather than using magic numbers

File: /contracts/module/LayerZeroModule.sol

270      uint16(1),

/// @audit: magic numbers "10"
274    return (((gasPrice * (gasLimit + (gasLimit / 10))) * dstPriceRatio) / (10**10), nativeFee);
293    return ((gasPrice * (gasLimit + (gasLimit / 10))) * dstPriceRatio) / (10**10);

/// @audit: magic numbers "10**10"
274    return (((gasPrice * (gasLimit + (gasLimit / 10))) * dstPriceRatio) / (10**10), nativeFee);
293    return ((gasPrice * (gasLimit + (gasLimit / 10))) * dstPriceRatio) / (10**10);

File: /contracts/enforcer/PA1D.sol

390    require(balance - gasCost > 10000, "PA1D: Not enough ETH to transfer");

395      sending = ((bps[i] * balance) / 10000);

411    require(balance > 10000, "PA1D: Not enough tokens to transfer");

415      sending = ((bps[i] * balance) / 10000);

435      require(balance > 10000, "PA1D: Not enough tokens to transfer");

438        sending = ((bps[i] * balance) / 10000);

477    require(totalBp == 10000, "PA1D: bps down't equal 10000");

551      return (_getDefaultReceiver(), (_getDefaultBp() * value) / 10000);

553      return (_getReceiver(tokenId), (_getBp(tokenId) * value) / 10000);

641      return (_getDefaultBp() * amount) / 10000;

643      return (_getBp(tokenId) * amount) / 10000;

Gas report

[G-01] Return directly the condition

Instead return true/false return the condition:

• return <CONDITION> ? true : false => return <CONDITION>
• if (<CONDITION>) { return true; } else { return false; } => return <CONDITION>

This save GAS and improve the code quality

File: /contracts/enforcer/HolographERC20.sol

288    if (
289      interfaces.supportsInterface(InterfaceType.ERC20, interfaceId) || erc165Contract.supportsInterface(interfaceId) // check global interfaces // check if source supports interface
290    ) {
291      return true;
292    } else {
293      return false;
294    }

755    return ((_eventConfig >> uint256(_eventName)) & uint256(1) == 1 ? true : false);

File: /contracts/enforcer/HolographERC721.sol

 298    if (
 299      interfaces.supportsInterface(InterfaceType.ERC721, interfaceId) || // check global interfaces
 301      interfaces.supportsInterface(InterfaceType.PA1D, interfaceId) || // check if royalties supports interface
 302      erc165Contract.supportsInterface(interfaceId) // check if source supports interface
 303    ) {
 304      return true;
 305    } else {
 306      return false;
 307    }

1003    return ((_eventConfig >> uint256(_eventName)) & uint256(1) == 1 ? true : false);

[G-02] Cache _getReceiver(tokenId) in a local var

File: /contracts/enforcer/PA1D.sol

/// @audit: instance 1
550    if (_getReceiver(tokenId) == address(0)) {
553      return (_getReceiver(tokenId), (_getBp(tokenId) * value) / 10000);

/// @audit: instance 2
571    if (_getReceiver(tokenId) == address(0)) {
574      receivers[0] = _getReceiver(tokenId);

/// @audit: instance 3
593    if (_getReceiver(tokenId) == address(0)) {
597      receivers[0] = _getReceiver(tokenId);

/// @audit: instance 4
607    if (_getReceiver(tokenId) == address(0)) {
611      receivers[0] = _getReceiver(tokenId);

[G-03] Unnecessary casts

File: /contracts/enforcer/HolographERC721.sol

/// @audit: remove payable
879    uint32 currentChain = HolographInterface(HolographerInterface(payable(address(this))).getHolograph())

/// @audit: remove payable
881    if (currentChain != HolographerInterface(payable(address(this))).getOriginChain()) {

884    return uint32(0);

[G-04] Unnecessary checks

File: /contracts/enforcer/HolographERC721.sol

/// @audit: checked in L817(inside _mint function)
404    require(!_exists(tokenId), "ERC721: token already exists");

/// @audit: It's a view function, all in the EVM it's public
520  function sourceGetChainPrepend() external view onlySource returns (uint256) {

/// @audit: checked in L818(inside _mint function)
513    require(!_burnedTokens[token], "ERC721: can't mint burned token");

[G-05] public functions to external functions

File: /contracts/enforcer/HolographERC721.sol

621  ) public payable {

643  function burned(uint256 tokenId) public view returns (bool) {

656  function exists(uint256 tokenId) public view returns (bool) {

935  function owner() public view override returns (address) {

File: /contracts/enforcer/PA1D.sol

471  function configurePayouts(address payable[] memory addresses, uint256[] memory bps) public onlyOwner {

488  function getPayoutInfo() public view returns (address payable[] memory addresses, uint256[] memory bps) {

497  function getEthPayout() public {

507  function getTokenPayout(address tokenAddress) public {

517  function getTokensPayout(address[] memory tokenAddresses) public {

[G-06] Duplicate code

The code of getFees and getRoyalties functions are exactly the same, move this code to an internal function

File: /contracts/enforcer/PA1D.sol

590  function getRoyalties(uint256 tokenId) public view returns (address payable[] memory, uint256[] memory) {

604  function getFees(uint256 tokenId) public view returns (address payable[] memory, uint256[] memory) {

[G-07] Duplicate contract

The code of ERC721H and ERC20H contracts are exactly the same, only change the error messages Use only one implementation