Holograph contest - Chom's results

Lines of code

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

Vulnerability details

Impact

Gas price spikes cause the selected operator to be vulnerable to frontrunning and be slashed.

Proof of Concept

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

        /**
         * @dev select operator that failed to do the job, is slashed the pod base fee
         */
        _bondedAmounts[job.operator] -= amount;
        /**
         * @dev the slashed amount is sent to current operator
         */
        _bondedAmounts[msg.sender] += amount;

Since you have designed a mechanism to prevent other operators to slash the operator due to "the selected missed the time slot due to a gas spike". It can induce that operators won't perform their job if a gas price spike happens due to negative profit.

But your designed mechanism has a vulnerability. Other operators can submit their transaction to the mempool and queue it using gasPrice in bridgeInRequestPayload. It may get executed before the selected operator as the selected operator is waiting for the gas price to drop but doesn't submit any transaction yet. If it doesn't, these operators lose a little gas fee. But a slashed reward may be greater than the risk of losing a little gas fee.

require(timeDifference > 0, "HOLOGRAPH: operator has time");

Once 1 epoch has passed, selected operator is vulnerable to slashing and frontrunning.

Recommended Mitigation Steps

Modify your operator node software to queue transactions immediately with gasPrice in bridgeInRequestPayload if a gas price spike happened. Or allow gas fee loss tradeoff to prevent being slashed.

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L329 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L419-L429

Vulnerability details

Impact

Failed job can't be recovered. NFT may be lost.

Proof of Concept

function executeJob(bytes calldata bridgeInRequestPayload) external payable {
...
delete _operatorJobs[hash];
...
    try
      HolographOperatorInterface(address(this)).nonRevertingBridgeCall{value: msg.value}(
        msg.sender,
        bridgeInRequestPayload
      )
    {
      /// @dev do nothing
    } catch {
      _failedJobs[hash] = true;
      emit FailedOperatorJob(hash);
    }
}

First, it will delete _operatorJobs[hash]; to have it not replayable.

Next, assume nonRevertingBridgeCall failed. NFT won't be minted and the catch block is entered.

_failedJobs[hash] is set to true and event is emitted

Notice that _operatorJobs[hash] has been deleted, so this job is not replayable. This mean NFT is lost forever since we can't retry executeJob.

Recommended Mitigation Steps

Move delete _operatorJobs[hash]; to the end of function executeJob covered in if (!_failedJobs[hash])

...
if (!_failedJobs[hash]) delete _operatorJobs[hash];
...

But this implementation is not safe. The selected operator may get slashed. Additionally, you may need to check _failedJobs flag to allow retry for only the selected operator.

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L301-L439 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L849-L891

Vulnerability details

Impact

executeJob does not validate msg.sender against bonded operators of that pod on timeDifference >= 6. Unbonded operators can perform executeJob, slash selected operator, and receive the reward (if implemented) as well. This is unexpected since your document clearly indicates that "To become an operator, you must view the pods available to join, select a pod, and bond at least the minimum bond requirement". Unbonded operators shouldn't execute jobs and have fee rewards without bonding.

Proof of Concept

Joining Pods

To become an operator, you must view the pods available to join, select a pod, and bond at least the minimum bond requirement. ... At step 3, you are now able to call the bondUtilityToken function with the pod and amounts you want to use to enter the pod. Please note, there is a minimum bond requirement to join but no maximum.

  function bondUtilityToken(
    address operator,
    uint256 amount,
    uint256 pod
  ) external {
    ...
      _operatorPods[pod - 1].push(operator);
      _operatorPodIndex[operator] = _operatorPods[pod - 1].length - 1;
      _bondedOperators[operator] = pod;
      _bondedAmounts[operator] = amount;
      /**
       * @dev transfer tokens last, to prevent reentrancy attacks
       */
      require(_utilityToken().transferFrom(msg.sender, address(this), amount), "HOLOGRAPH: token transfer failed");
    }
  }

_bondedOperators[operator] will be set to bonded pod

Processing Jobs

You must join a pod to become an Operator

...

At step 5, the wallet sends a transaction to the executeJob method on the HolographOperator.sol contract. In here, further checks are done to validate the job and user's wallet. After this transaction is mined on the blockchain, the NFT will become finalized and available on the new blockchain.

function executeJob(bytes calldata bridgeInRequestPayload) external payable {
...
if (job.operator != msg.sender)
...
        if (timeDifference < 6) {
          uint256 podIndex = uint256(job.fallbackOperators[timeDifference - 1]);
          /**
           * @dev do a quick sanity check to make sure operator did not leave from index or is a zero address
           */
          if (podIndex > 0 && podIndex < _operatorPods[pod].length) {
            address fallbackOperator = _operatorPods[pod][podIndex];
            /**
             * @dev ensure that sender is currently valid backup operator
             */
            require(fallbackOperator == msg.sender, "HOLOGRAPH: invalid fallback");
          }
        }
...
    /**
     * @dev execute the job
     */
    try
      HolographOperatorInterface(address(this)).nonRevertingBridgeCall{value: msg.value}(
        msg.sender,
        bridgeInRequestPayload
      )
    {
      /// @dev do nothing
    } catch {
      _failedJobs[hash] = true;
      emit FailedOperatorJob(hash);
    }
    /**
     * @dev every executed job (even if failed) increments total message counter by one
     */
    ++_inboundMessageCounter;
    /**
     * @dev reward operator (with HLG) for executing the job
     * @dev this is out of scope and is purposefully omitted from code
     */
    ////  _bondedOperators[msg.sender] += reward;
}

I haven't seen anywhere checking msg.sender for operator that executed on timeDifference >= 6 is bonding to job.pod (_bondedOperators[msg.sender] == job.pod)

But the document clearly indicates that You must join a pod to become an Operator and an operator is one that should call executeJob

Tools Used

Manual review

Recommended Mitigation Steps

Check that msg.sender is bonded to job.pod by checking

require(_bondedOperators[msg.sender] == job.pod);

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/module/LayerZeroModule.sol#L81-L123 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/HolographOperator.sol#L385-L440 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/HolographOperator.sol#L201-L340

Vulnerability details

Impact

Compromised layerzero endpoint can steal all NFT. Hackers can craft a malicious message and operators can't prevent it.

Proof of Concept

  function lzReceive(
    uint16, /* _srcChainId*/
    bytes calldata _srcAddress,
    uint64, /* _nonce*/
    bytes calldata _payload
  ) external payable {
    ...

    // Validation is bypassed if layerzero endpoint is compromised.
    _operator().crossChainMessage(_payload);
  }

function crossChainMessage(bytes calldata bridgeInRequestPayload) external payable {
...
}

function executeJob(bytes calldata bridgeInRequestPayload) external payable {
...
}

If layerzero endpoint is compromised, hackers can craft a malicious message for example unlocking BAYC to the hacker. That message will bypass the validation in lzReceive and get passed to crossChainMessage. Then it will be assigned an operator. Since the operator is forced to perform the assigned job without any doubt, the operator will surely perform that job and BAYC will surely unlock to the hacker's wallet.

Recommended Mitigation Steps

Add a step to default operator node software to validate the validity of cross-chain transaction first and allows the operator to pause that transaction if a malicious transaction is found.