Protocol - Holograph

The Admin contract implements the function adminCall():

function adminCall(address target, bytes calldata data) external payable onlyAdmin {
  assembly {
    calldatacopy(0, data.offset, data.length)
    let result := call(gas(), target, callvalue(), 0, data.length, 0, 0)
    returndatacopy(0, 0, returndatasize())
    switch result
    case 0 {
      revert(0, returndatasize())
    }
    default {
      return(0, returndatasize())
    }
  }
}

This function allows any admin to perform a low-level call impersonating the actual contract that inherits from the Admin contract itself. Moreover, this function is the one that was abused by the privileged 0xc0ffee address that performed the Holograph exploit, last June:

"The exploit was due to unauthorized admin access of a proxy wallet by a disgruntled former contractor who minted approximately $14 million worth of new HLG & sold it on the open market, crashing the price".

During this exploit, the attacker, who was an admin, called the LayerZeroModuleProxy.adminCall() to call the crossChainMessage() function in the HolographOperator contract on behalf of the LayerZero module. This way, he managed to create jobs to mint HLG tokens.

The adminCall()makes the system vulnerable to abuse by any trusted admin.

Consider removing the adminCall() function from the Admin contract.

RISK ACCEPTED: The Holograph team accepted the risk of this finding. The Holograph team plans are to remove this function as the protocol grows older. This issue is known, but also by design, so the Holograph Protocol team can administer the protocol and upgrade as necessary. Additionally, this requires multiple parties to sign off on adminCall transactions via the Protocol Management Multisig on each network.

Eventually, the Holograph team plans to progressively decentralize and remove adminCall and hand over changes to the protocol to the community via governance.

The HolographFactory implements the function deployHolographableContract():

/**
 * @notice Deploy a holographable smart contract
 * @dev Using this function allows to deploy smart contracts that have the same address across all EVM chains
 * @param config contract deployement configurations
 * @param signature that was created by the wallet that created the original payload
 * @param signer address of wallet that created the payload
 */
function deployHolographableContract(
  DeploymentConfig memory config,
  Verification memory signature,
  address signer
) public {
  address registry;
  address holograph;
  assembly {
    holograph := sload(_holographSlot)
    registry := sload(_registrySlot)
  }
  /**
   * @dev the configuration is encoded and hashed along with signer address
   */
  bytes32 hash = keccak256(
    abi.encodePacked(
      config.contractType,
      config.chainType,
      config.salt,
      keccak256(config.byteCode),
      keccak256(config.initCode),
      signer
    )
  );
  /**
   * @dev the hash is validated against signature
   *      this is to guarantee that the original creator's configuration has not been altered
   */
  require(_verifySigner(signature.r, signature.s, signature.v, hash, signer), "HOLOGRAPH: invalid signature");
  /**
   * @dev check that this contract has not already been deployed on this chain
   */
  bytes memory holographerBytecode = type(Holographer).creationCode;
  address holographerAddress = address(
    uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), hash, keccak256(holographerBytecode)))))
  );
  require(!_isContract(holographerAddress), "HOLOGRAPH: already deployed");
  /**
   * @dev convert hash into uint256 which will be used as the salt for create2
   */
  uint256 saltInt = uint256(hash);
  address sourceContractAddress;
  bytes memory sourceByteCode = config.byteCode;
  assembly {
    /**
     * @dev deploy the user created smart contract first
     */
    sourceContractAddress := create2(0, add(sourceByteCode, 0x20), mload(sourceByteCode), saltInt)
  }
  assembly {
    /**
     * @dev deploy the Holographer contract
     */
    holographerAddress := create2(0, add(holographerBytecode, 0x20), mload(holographerBytecode), saltInt)
  }
  /**
   * @dev initialize the Holographer contract
   */
  require(
    InitializableInterface(holographerAddress).init(
      abi.encode(abi.encode(config.chainType, holograph, config.contractType, sourceContractAddress), config.initCode)
    ) == InitializableInterface.init.selector,
    "initialization failed"
  );
  /**
   * @dev update the Holograph Registry with deployed contract address
   */
  HolographRegistryInterface(registry).setHolographedHashAddress(hash, holographerAddress); //@audit overwritten if holograph is changed
  /**
   * @dev emit an event that on-chain indexers can easily read
   */
  emit BridgeableContractDeployed(holographerAddress, hash);
}

This function makes use of a signature. The signed hash is built as:

bytes32 hash = keccak256(
  abi.encodePacked(
    config.contractType,
    config.chainType,
    config.salt,
    keccak256(config.byteCode),
    keccak256(config.initCode),
    signer
  )
);

As the hash is built without any domain separator, this signature could be used across multiple chains. For example, signatures created by Bob in the Sepolia test network could be re-used also in Ethereum mainnet.

Consider including a domain separator in the HolographFactory.deployHolographableContract() function signed hash.

RISK ACCEPTED: The Holograph team accepted the risk of this finding.

The HolographOperator contract implements the function recoverJob():

/**
 * @notice Recover failed job
 * @dev If a job fails, it can be manually recovered
 * @param bridgeInRequestPayload the entire cross chain message payload
 */
function recoverJob(bytes calldata bridgeInRequestPayload) external payable onlyAdmin {
  bytes32 hash = keccak256(bridgeInRequestPayload);
  require(_failedJobs[hash], "HOLOGRAPH: invalid recovery job");
  (bool success, ) = _bridge().call{value: msg.value}(bridgeInRequestPayload);
  require(success, "HOLOGRAPH: recovery failed");
  delete (_failedJobs[hash]);
}

This function can only be executed by an admin in case of a failed job. However, the _failedJobs mapping is deleted after the _bridge().call() and consequently, it is entirely possible that if the bridgeInRequestPayload contains a call to the recoverJob() function, the job is re-entered and executed more than once. Note that this exploit requires also a malicious admin doing the call.

Consider updating the recoverJob() function as shown below, following the check-effects-interactions pattern:

/**
 * @notice Recover failed job
 * @dev If a job fails, it can be manually recovered
 * @param bridgeInRequestPayload the entire cross chain message payload
 */
function recoverJob(bytes calldata bridgeInRequestPayload) external payable onlyAdmin {
  bytes32 hash = keccak256(bridgeInRequestPayload);
  require(_failedJobs[hash], "HOLOGRAPH: invalid recovery job");
  delete (_failedJobs[hash]);
  (bool success, ) = _bridge().call{value: msg.value}(bridgeInRequestPayload);
  require(success, "HOLOGRAPH: recovery failed");
}

SOLVED: The Holograph team solved the issue by implementing the recommended solution.

The HolographOperator contract implements the function setUtilityToken():

/**
 * @notice Update the Holograph Utility Token address
 * @param utilityToken address of the Holograph Utility Token smart contract to use
 */
function setUtilityToken(address utilityToken) external onlyAdmin {
  assembly {
    sstore(_utilityTokenSlot, utilityToken)
  }
}

However, when a new utility token is set by an admin, the _bondedAmounts mapping is not reset to 0, breaking all the slashing infrastructure.

When a new utility token is set by an admin all the _bondedAmounts keys should be reset to 0. On the other hand, as the previous suggestion is nearly impossible to implement without very high gas costs, consinder simply removing the setUtilityToken() function.

PARTIALLY SOLVED: The Holograph team partially solved the issue as:

• The team is totally aware of the inconsistency caused if this function is ever called.

• The following warning was added as a comment at smart contract level: "This function should only be used in the event of a token migration which should never happen. Updating this will break all the slashing and bonding logic. To update the utility token in a safe way before calling this function, the bondedAmounts and operatorPods arrays should be reset to zero".

All the upgradeable contracts across the Holograph codebase implement initialize functions. However, as these contracts do not implement the disableInitializers() modifier their initialization could be front-run if they were not deployed and initialized atomically. Often, this would require to perform a new deployment.

Consider calling the _disableInitializers function in all the contracts' constructor:

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
    _disableInitializers();
}

RISK ACCEPTED: The Holograph team accepted the risk of this finding. The Holograph team states that Holograph Protocol contracts are always deployed and initialized atomically via the deployment scripts, so this isn’t a real concern, but they will consider the suggestion for future contracts that do not go through the existing deployment pipeline.

In the HolographOperator, multiple calls to transfer() or transferFrom()are executed using the IERC20 interface:

• _utilityToken().transfer((isBonded ? msg.sender : address(_holograph().getTreasury())), amount);

• _utilityToken().transfer(job.operator, leftovers);

• require(_utilityToken().transferFrom(msg.sender, address(this), amount), "HOLOGRAPH: token transfer failed");

• require(_utilityToken().transferFrom(msg.sender, address(this), amount), "HOLOGRAPH: token transfer failed");

• require(_utilityToken().transfer(recipient, amount), "HOLOGRAPH: token transfer failed");

In some tokens like USDT the transfer() and transferFrom() functions do not return a bool:

/**
* @dev transfer token for a specified address
* @param _to The address to transfer to.
* @param _value The amount to be transferred.
*/
function transfer(address _to, uint _value) public onlyPayloadSize(2 * 32) {
    uint fee = (_value.mul(basisPointsRate)).div(10000);
    if (fee > maximumFee) {
        fee = maximumFee;
    }
    uint sendAmount = _value.sub(fee);
    balances[msg.sender] = balances[msg.sender].sub(_value);
    balances[_to] = balances[_to].add(sendAmount);
    if (fee > 0) {
        balances[owner] = balances[owner].add(fee);
        Transfer(msg.sender, owner, fee);
    }
    Transfer(msg.sender, _to, sendAmount);
}

/**
* @dev Transfer tokens from one address to another
* @param _from address The address which you want to send tokens from
* @param _to address The address which you want to transfer to
* @param _value uint the amount of tokens to be transferred
*/
function transferFrom(address _from, address _to, uint _value) public onlyPayloadSize(3 * 32) {
    var _allowance = allowed[_from][msg.sender];

    // Check is not needed because sub(_allowance, _value) will already throw if this condition is not met
    // if (_value > _allowance) throw;

    uint fee = (_value.mul(basisPointsRate)).div(10000);
    if (fee > maximumFee) {
        fee = maximumFee;
    }
    if (_allowance < MAX_UINT) {
        allowed[_from][msg.sender] = _allowance.sub(_value);
    }
    uint sendAmount = _value.sub(fee);
    balances[_from] = balances[_from].sub(_value);
    balances[_to] = balances[_to].add(sendAmount);
    if (fee > 0) {
        balances[owner] = balances[owner].add(fee);
        Transfer(_from, owner, fee);
    }
    Transfer(_from, _to, sendAmount);
}

IERC20 interface expects a bool as a return of the transfer() and transferFrom() calls. In these situations, if the token used was, for example, USDT, the IERC20.transfer() or IERC20.transferFrom() calls would revert.

It is recommended to use the OpenZeppelin's safeTransfer() and safeTransferFrom() functions instead of transfer()& transferFrom().

SOLVED: The Holograph team solved the issue by implementing the recommended solution.

The HolographOperator contract transfers native assets paid by the operator using the Solidity low-level transfer():

try
  HolographOperatorInterface(address(this)).nonRevertingBridgeCall{value: msg.value}(
    msg.sender,
    bridgeInRequestPayload
  )
{
  /// @dev do nothing
} catch {
  /// @dev return any payed funds in case of revert
  payable(msg.sender).transfer(msg.value); //@audit use call instead of transfer
  _failedJobs[hash] = true;
  emit FailedOperatorJob(hash);
}

In Solidity, the call() function is often preferred over transfer() for sending Ether due to some gas limit considerations:

• transfer: Imposes a fixed gas limit of 2300 gas. This limit can be too restrictive, especially if the receiving contract is a multisig wallet that executes more complex logic in its receive() function. For example, native transfer()calls to Gnosis Safe multisigs will always revert with an out-of-gas error in Binance Smart Chain.

• call: Allows specifying a custom gas limit, providing more flexibility and ensuring that the receiving contract can perform necessary operations.

It should be noted that using call also requires explicit reentrancy protection mechanisms (e.g., using checks-effects-interactions pattern or the ReentrancyGuard contract from OpenZeppelin).

Consider using call() over transfer() to transfer native assets in order to ensure compatibility with any type of multisig wallet. As for the reentrancy risks, these are currently mitigated in the executeJobs() function as _failedJobs is updated after the actual transfer.

RISK ACCEPTED: The Holograph team accepted the risk of this finding.

The current ownership transfer process for all the contracts inheriting from Admin involves the current admin calling the setAdmin() function:

function setAdmin(address adminAddress) public onlyAdmin {
  assembly {
    sstore(_adminSlot, adminAddress)
  }
}

If the nominated EOA account is not a valid account, it is entirely possible that the owner may accidentally transfer ownership to an uncontrolled account, losing the access to all functions with the onlyAdmin modifier.

It is recommended to implement a two-step process where the owner nominates an account and the nominated account needs to call an acceptAdmin() function for the transfer of the ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

ACKNOWLEDGED: The Holograph team acknowledged this finding.

In the HolographGenesis contract, the recoverSigner() function calls the Solidity ecrecover function directly to verify the given signature. However, the ecrecover EVM opcode allows malleable(non-unique) signatures and thus is susceptible to replay attacks.

Although a replay attack is not possible here as it is prevented with the _approveDeployerNonce state variable, ensuring that signatures are not malleable is considered a best practice.

Consider using the [recover() function from OpenZeppelin’s ECDSA library](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L75-L93) for signature verification.

ACKNOWLEDGED: The Holograph team acknowledged this finding.

All the Holograph smart contracts are marked as unlicensed, as indicated by the SPDX license identifier at the top of the files:

// SPDX-License-Identifier: UNLICENSED

Using unlicensed contract can lead to legal uncertainties and potential conflicts regarding the usage, modification and distribution rights of the code. This may deter other developers from using or contributing to the project and could potentially lead to legal issues in the future.

It is recommended to choose and apply an appropriate open-source license to the smart contracts. Some popular options for blockchain and smart contract projects include:

1. MIT License: A permissive license that allows for reuse with minimal restrictions.

2. GNU General Public License (GPL): A copyleft license that ensures derivative works are also open-source.

3. Apache License 2.0: A permissive license that provides an express grant of patent rights from contributors to users.

ACKNOWLEDGED: The Holograph team acknowledged this finding.

Operators in Holograph perform tasks using the executeJob() function with bridged bytes from the source chain. If the primary operator fails to execute the job within the allocated block, a bond is taken and transferred to the operator doing the job. The presence of a gas spike is checked with tx.gasprice:

require(gasPrice >= tx.gasprice, "HOLOGRAPH: gas spike detected");

However, attackers can exploit this by submitting a flashbots bundle with executeJob() at a low gas price and an additional bribe to miners, enabling them to steal the bond from honest operators. This is not theoretical, as MEV bots exploit such opportunities regularly:

• See coinbase.transfer().

• See Bundle selection.

Therefore, a dishonest operator can seize an honest operator's bond even when gas prices are high.

Avoid using current tx.gasprice for previous block gas price inference. It is recommended to use a gas price oracle instead.

PENDING: The Holograph team will consider addressing this in a future release.

All the contracts in scope are using low-level calls with the 0.8.13 solidity compiler version, which can trigger an optimizer bug. See the following article.

The bug involves the Solidity compiler's optimizer making overly aggressive assumptions about certain storage writes and reads. Specifically, the optimizer may incorrectly assume that a storage slot being read from has not been written to earlier in the same function, leading to discrepancies between the actual storage content and the optimizer's expectations. For example, let's imagine a smart contract function that writes to a storage slot and later reads from it within the same function. Due to the optimizer's incorrect assumptions, the read operation might return the old value instead of the updated one.

Consider updating your contracts to a newer solidity version.

ACKNOWLEDGED: The Holograph team acknowledged this finding.

Using block.timestamp as a source of entropy to generate randomness in smart contracts is not recommended as it can be influenced by miners to a certain extent, making it a weak and predictable source of randomness. This can result in the manipulation of supposedly random outcomes. The block.timestamp is a value that miners can manipulate within a reasonable range (typically up to 15 seconds). This flexibility allows miners to adjust the timestamp in their favor, especially when there is an economic incentive to do so.

In the HolographOperator contract, the function crossChainMessage() uses block.timestamp as a source of entropy to generate a random number:

uint256 random = uint256(keccak256(abi.encodePacked(jobHash, _jobNonce(), block.number, block.timestamp)));

Consider removing the block.number and the block.timestamp from the source of entropy used to generate the random number in the HolographOperator.crossChainMessage() function.

PENDING: The Holograph team will consider addressing this in a future release.