Holograph contest - Trust's results

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/module/LayerZeroModule.sol#L431-L445

Vulnerability details

Description

Holograph gets it's cross chain messaging primitives through Layer Zero. To get pricing estimate, it uses the DstConfig price struct exposed in LZ's RelayerV2

The issue is that the important baseGas and gasPerByte configuration parameters, which are used to calculate a custom amount of gas for the destination LZ message, use the values that come from the source chain. This is in contrast to LZ which handles DstConfigs in a mapping keyed by chainID. The encoded gas amount is described here

Impact

The impact is that when those fields are different between chains, one of two things may happen:

1. Less severe - we waste excess gas, which is refunded to the lzReceive() caller (Layer Zero)
2. More severe - we underprice the delivery cost, causing lzReceive() to revert and the NFT stuck in limbo forever.

The code does not handle a failed lzReceive (differently to a failed executeJob). Therefore, no failure event is emitted and the NFT is screwed.

Tools Used

Manual audit

Recommended Mitigation Steps

Firstly,make sure to use the target gas costs. Secondly, re-engineer lzReceive to be fault-proof, i.e. save some gas to emit result event.

function send(
  uint256, /* gasLimit*/
  uint256, /* gasPrice*/
  uint32 toChain,
  address msgSender,
  uint256 msgValue,
  bytes calldata crossChainPayload
) external payable {
  require(msg.sender == address(_operator()), "HOLOGRAPH: operator only call");
  LayerZeroOverrides lZEndpoint;
  assembly {
    lZEndpoint := sload(_lZEndpointSlot)
  }
  // need to recalculate the gas amounts for LZ to deliver message
  lZEndpoint.send{value: msgValue}(
    uint16(_interfaces().getChainId(ChainIdType.HOLOGRAPH, uint256(toChain), ChainIdType.LAYERZERO)),
    abi.encodePacked(address(this), address(this)),
    crossChainPayload,
    payable(msgSender),
    address(this),
    abi.encodePacked(uint16(1), uint256(_baseGas() + (crossChainPayload.length * _gasPerByte())))
  );
}

Lines of code

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

Vulnerability details

Description

Operators in Holograph do their job by calling executeJob() with the bridged in bytes from source chain. If the primary job operator did not execute the job during his allocated block slot, he is punished by taking a single bond amount and transfer it to the operator doing it instead. The docs and code state that if there was a gas spike in the operator's slot, he shall not be punished. The way a gas spike is checked is with this code in executeJob:

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

However, there is still a way for operator to claim primary operator's bond amount although gas price is high. Attacker can submit a flashbots bundle including the executeJob() transaction, and one additional "bribe" transaction. The bribe transaction will transfer some incentive amount to coinbase address (miner), while the executeJob is submitted with a low gasprice. Miner will accept this bundle as it is overall rewarding enough for them, and attacker will receive the base bond amount from victim operator. This threat is not theoretical because every block we see MEV bots squeezing value from such opportunities.

info about coinbase transfer info about bundle selection

Impact

Dishonest operator can take honest operator's bond amount although gas price is above acceptable limits.

Tools Used

Manual audit, flashbot docs

Recommended Mitigation Steps

Do not use current tx.gasprice amount to infer gas price in a previous block. Probably best to use gas price oracle.

Lines of code

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

Vulnerability details

Description

Operators in Holograph do their job by calling executeJob() with the bridged in bytes from source chain. If the primary job operator did not execute the job during his allocated block slot, he is punished by taking a single bond amount and transfer it to the operator doing it instead. The docs and code state that if there was a gas spike in the operator's slot, he shall not be punished. The way a gas spike is checked is with this code in executeJob:

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

The issue is that tx.gasprice was possibly high during the primary operator's block, but a couple of blocks later went down to acceptable levels. But at this point another operator will call executeJob() and punish the primary operator.

Impact

Honest operator can lose their bonded amount although gas price was unacceptable during their slot

Tools Used

Manual audit

Proof of Concept

1. User submits a transaction with gas price X on target chain
2. Gas price goes high (1.2X), primary operator cannot execute the job
3. 4 blocks later, the gas price goes back down to 0.9X. Now the 3rd backup operator submits the transaction and claims primary operator's bonded amount.

Recommended Mitigation Steps

Do not use current tx.gasprice amount to infer gas price in a previous block. Probably best to use gas price oracle.

Lines of code

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

Vulnerability details

Operator can fail transactions causing user to lose their send asset, due to incorrect gas check calculation. Line 416 in Operator. If gasleft() = gasLimit + small amount, gas reaching nonRevertingBridgeCall will be 63/64 of casleft() - CALL opcode code, which is less than gasLimit, causing nonRevertingBridgeCall to revert. executeJob remains with 1/64 of sent gas, enough to store job as failed. From EVM.codes: From the Tangerine Whistle fork, gas is capped at all but one 64th (remaining_gas / 64) of the remaining gas of the current context.

Description

executeJob() function is called by Operator to complete the bridgeIn procedure. As a guarantee that valid transactions cannot be intentionally failed by operators, the bridging is done in a sandboxed environment.

Here is the meat of executeJob():

require(gasleft() > gasLimit, "HOLOGRAPH: not enough gas left");
/**
 * @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);
}

And below is nonRevertingBridgeCall:

function nonRevertingBridgeCall(address msgSender, bytes calldata payload) external payable {
  require(msg.sender == address(this), "HOLOGRAPH: operator only call");
  assembly {
    /**
     * @dev remove gas price from end
     */
    calldatacopy(0, payload.offset, sub(payload.length, 0x20))
    /**
     * @dev hToken recipient is injected right before making the call
     */
    mstore(0x84, msgSender)
    /**
     * @dev make non-reverting call
     */
    let result := call(
      /// @dev gas limit is retrieved from last 32 bytes of payload in-memory value
      mload(sub(payload.length, 0x40)),
      /// @dev destination is bridge contract
      sload(_bridgeSlot),
      /// @dev any value is passed along
      callvalue(),
      /// @dev data is retrieved from 0 index memory position
      0,
      /// @dev everything except for last 32 bytes (gas limit) is sent
      sub(payload.length, 0x40),
      0,
      0
    )
    if eq(result, 0) {
      revert(0, 0)
    }
    return(0, 0)
  }
}

The premise is that this require ensures there is enough gas for the bridge call to execute successfully, and if there isn't then the bringIn request is actually not legitimate. require(gasleft() > gasLimit, "HOLOGRAPH: not enough gas left");

This is wrong because the gas actually sent to nonRevertingBridgeCall is not gasleft(), but actually (gasleft() - CALL_COST ) * 63/64.

From evm.codes: "From the Tangerine Whistle fork, gas is capped at all but one 64th (remaining_gas / 64) of the remaining gas of the current context. If a call tries to send more, the gas is changed to match the maximum allowed."

Suppose operator crafts gas cost so that during the require statement, gasleft() = gasLimit + 10. This check will pass, and nonRevertingBridgeCall is called. In the assembly call operation, gasLimit is passed as the gas limit. Therefore, we have a situation where we can with a lower gasleft() than the gas limit sent to the call, which causes the call opcode to revert instantly. After the revert, executeJob() will report the failure and user's transaction does not carry through.

The effect is that any user asset sent through the Holograph bridge can be sniped by an operator. I am not familiar with how Holograph wishes to implement a rollback process (which is described in the docs), but from the existing code it seems like the NFT is lossed forever.

Impact

Any user asset sent through the bridge can be frozen in limbo by a malicious operator.

Tools Used

Manual audit

Recommended Mitigation Steps

For the sandbox model to work, the code needs to perform the gasleft() comparison as close to the actual call site as possible, and manually perform arithmatic to make sure the actual bridging operation is able to receive the gas limit.

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/abstract/ERC721H.sol#L185 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/abstract/ERC721H.sol#L121

Vulnerability details

Description

ERC20H and ERC721H are base contracts for NFTs / coins to inherit from. They supply the modifier onlyOwner and function isOwner which are used in the implementations for access control. However, there are several functions which when using these the answer may be corrupted to true by an attacker.

The issue comes from confusion between calls coming from HolographERC721's fallback function, and calls from actually implemented functions.

In the fallback function, the enforcer appends an additional 32 bytes of msg.sender :

assembly {
  calldatacopy(0, 0, calldatasize())
  mstore(calldatasize(), caller())
  let result := call(gas(), sload(_sourceContractSlot), callvalue(), 0, add(calldatasize(), 32), 0, 0)
  returndatacopy(0, 0, returndatasize())
  switch result
  case 0 {
    revert(0, returndatasize())
  }
  default {
    return(0, returndatasize())
  }
}

Indeed these are the bytes read as msgSender:

function msgSender() internal pure returns (address sender) {
  assembly {
    sender := calldataload(sub(calldatasize(), 0x20))
  }
}

and isOwner simply compares these to the stored owner:

function isOwner() external view returns (bool) {
  if (msg.sender == holographer()) {
    return msgSender() == _getOwner();
  } else {
    return msg.sender == _getOwner();
  }
}

However, the enforcer calls these functions directly in several locations, and in these cases it of course does not append a 32 byte msg.sender. For example, in safeTransferFrom:

function safeTransferFrom(
  address from,
  address to,
  uint256 tokenId,
  bytes memory data
) public payable {
  require(_isApproved(msg.sender, tokenId), "ERC721: not approved sender");
  if (_isEventRegistered(HolographERC721Event.beforeSafeTransfer)) {
    require(SourceERC721().beforeSafeTransfer(from, to, tokenId, data));
  }
  _transferFrom(from, to, tokenId);
  if (_isContract(to)) {
    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"
    );
  }
  if (_isEventRegistered(HolographERC721Event.afterSafeTransfer)) {
    require(SourceERC721().afterSafeTransfer(from, to, tokenId, data));
  }
}

Here, caller has arbitrary control of the data parameter, and can pass owner's address.When the implementation, SourceERC721(), gets called, beforeSafeTransfer / afterSafeTransfer will behave as if they are called by owner.

Therefore, depending on the actual implementation, derived contracts can lose funds by specifying owner-specific logic.

This pattern occurs with the following functions, which have an arbitrary data parameter:

• beforeSafeTransfer / after SafeTransfer
• beforeTransfer / afterTransfer
• beforeOnERC721Received / afterOnERC721Received
• beforeOnERC20Received / aferERC20Received

Impact

Owner-specific functionality can be initiated on NFT / ERC20 implementation contracts

Tools Used

Manual audit

Recommended Mitigation Steps

Refactor the code to represent msg.sender information in a bug-free way.

  function mint(
    address to,
    uint224 tokenId,
    string calldata URI
  ) external onlyHolographer onlyOwner {
    HolographERC721Interface H721 = HolographERC721Interface(holographer());
    if (tokenId == 0) {
      _currentTokenId += 1;
      while (H721.exists(uint256(_currentTokenId)) || H721.burned(uint256(_currentTokenId))) {
        _currentTokenId += 1;
      }
      tokenId = _currentTokenId;
    }
    H721.sourceMint(to, tokenId);
    uint256 id = H721.sourceGetChainPrepend() + uint256(tokenId);
    _tokenURIs[id] = URI;
  }

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L416 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L439

Vulnerability details

Description

ERC20 royalties are paid using _payoutTokens and _payoutToken functions in PA1D.sol. Unfortunately these functions use ERC20's transfer() instead of implementing safeTransfer:

    for (uint256 i = 0; i < length; i++) {
      sending = ((bps[i] * balance) / 10000);
      require(erc20.transfer(addresses[i], sending), "PA1D: Couldn't transfer token");
      // sent = sent + sending;
    }

The transfer signature includes a bool return value:

function transfer(address _to, uint256 _value) external returns (bool success);

Therefore, the calling code discovers there is no bool return parameter, it will revert. The impact is any non conforming ERC20 tokens will be forever stuck in the Holographer contract. There are hundreds of such tokens, such as USDT, BNB etc.

Because of the likelihood of non-conforming ERC20 tokens being a royalty token, this represents a high severity threat.

Impact

USDT, BNB and any other no-return-value coin will be stuck in the Holographer contract.

Tools Used

Manual audit

Recommended Mitigation Steps

Use OpenZeppelin's SafeERC20 library to interact with ERC20 tokens.

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/HolographERC721.sol#L467

Vulnerability details

Description

HolographERC721.sol is an enforcer contract that fully implements ERC721. In its safeTransferFromFunction there is the following code:

if (_isContract(to)) {
  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"
  );
}

If the target address is a contract, the enforcer requires the target's onERC721Received() to succeed. However, the call deviates from the standard:

interface ERC721TokenReceiver {
    /// @notice Handle the receipt of an NFT
    /// @dev The ERC721 smart contract calls this function on the recipient
    ///  after a `transfer`. This function MAY throw to revert and reject the
    ///  transfer. Return of other than the magic value MUST result in the
    ///  transaction being reverted.
    ///  Note: the contract address is always the message sender.
    /// @param _operator The address which called `safeTransferFrom` function
    /// @param _from The address which previously owned the token
    /// @param _tokenId The NFT identifier which is being transferred
    /// @param _data Additional data with no specified format
    /// @return `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`
    ///  unless throwing
    function onERC721Received(address _operator, address _from, uint256 _tokenId, bytes _data) external returns(bytes4);
}

The standard mandates that the first parameter will be the operator - the caller of safeTransferFrom. The enforcer passes instead the address(this) value, in other words the Holographer address. The impact is that any bookkeeping done in target contract, and allow / disallow decision of the transaction, is based on false information.

Impact

ERC721 transferFrom's "to" contract may fail to accept transfers, or record credit of transfers incorrectly.

Tools Used

Manual audit

Recommended Mitigation Steps

Pass the msg.sender parameter, as the ERC721 standard requires.

Lines of code

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

Vulnerability details

Description

Operators are organized into different pod tiers. Every time a new request arrives, it is scheduled to a random available pod. It is important to note that pods may be empty, in which case the pod array actually has a single zero element to help with all sorts of bugs. When a pod of a non existing tier is created, any intermediate tiers between the current highest tier to the new tier are filled with zero elements. This happens at bondUtilityToken():

if (_operatorPods.length < pod) {
  /**
   * @dev activate pod(s) up until the selected pod
   */
  for (uint256 i = _operatorPods.length; i <= pod; i++) {
    /**
     * @dev add zero address into pod to mitigate empty pod issues
     */
    _operatorPods.push([address(0)]);
  }
}

The issue is that any user can spam the contract with a large amount of empty operator pods. The attack would look like this:

1. bondUtilityToken(attacker, large_amount, high_pod_number)
2. unbondUtilityToken(attacker, attacker)

The above could be wrapped in a flashloan to get virtually any pod tier filled.

The consequence is that when the scheduler chooses pods uniformally, they will very likely choose an empty pod, with the zero address. Therefore, the chosen operator will be 0, which is referred to in the code as "open season". In this occurrance, any operator can perform the executeJob() call. This is of course really bad, because all but one operator continually waste gas for executions that will be reverted after the lucky first transaction goes through. This would be a practical example of a griefing attack on Holograph.

Impact

Any user can force chaotic "open season" operator behavior

Tools Used

Manual audit

Recommended Mitigation Steps

It is important to pay special attention to the scheduling algorithm, to make sure different pods are given execution time according to the desired heuristics.

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L382-L398

Vulnerability details

Description

PA1D.sol's _payoutEth function is responsible for distributing ETH holdings in the Holographer. It uses this code:

    uint256 gasCost = (23300 * length) + length;
    uint256 balance = address(this).balance;
    require(balance - gasCost > 10000, "PA1D: Not enough ETH to transfer");
    balance = balance - gasCost;
    uint256 sending;
    // uint256 sent;
    for (uint256 i = 0; i < length; i++) {
      sending = ((bps[i] * balance) / 10000);
      addresses[i].transfer(sending);
      // sent = sent + sending;
    }

The issue is that there is no reason to decrement balance by gasCost, since the gas for the entire payoutEth function will be paid for by the _payoutEth transaction. There is some confusion between ETH passed in calls to GAS passed in calls. ETH will be taken from address(this).balance. Gas is spent from current remaining gas.

The impact is that gasCost value is always leaked and will be stuck in the Holographer forever. Additionally, the minimum required balance for payout to execute is higher than it should be, by gasCost amount.

Impact

Some royalty ETH will be stuck in the Holographer contract forever.

Tools Used

Manual audit

Recommended Mitigation Steps

Do not calculate gas costs for ETH transfers.

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/HolographERC20.sol#L539

Vulnerability details

Description

HolographERC20 is the ERC20 enforcer for Holograph. In the safeTransferFrom operation, it calls _checkOnERC20Received:

if (_isEventRegistered(HolographERC20Event.beforeSafeTransfer)) {
  require(SourceERC20().beforeSafeTransfer(account, recipient, amount, data));
}
_transfer(account, recipient, amount);
require(_checkOnERC20Received(account, recipient, amount, data), "ERC20: non ERC20Receiver");
if (_isEventRegistered(HolographERC20Event.afterSafeTransfer)) {
  require(SourceERC20().afterSafeTransfer(account, recipient, amount, data));
}

The checkOnERC20Received function:

if (_isContract(recipient)) {
  try ERC165(recipient).supportsInterface(ERC165.supportsInterface.selector) returns (bool erc165support) {
    require(erc165support, "ERC20: no ERC165 support");
    // we have erc165 support
    if (ERC165(recipient).supportsInterface(0x534f5876)) {
      // we have eip-4524 support
      try ERC20Receiver(recipient).onERC20Received(address(this), account, amount, data) returns (bytes4 retv
        return retval == ERC20Receiver.onERC20Received.selector;
      } catch (bytes memory reason) {
        if (reason.length == 0) {
          revert("ERC20: non ERC20Receiver");
        } else {
          assembly {
            revert(add(32, reason), mload(reason))
          }
        }
      }
    } else {
      revert("ERC20: eip-4524 not supported");
    }
  } catch (bytes memory reason) {
    if (reason.length == 0) {
      revert("ERC20: no ERC165 support");
    } else {
      assembly {
        revert(add(32, reason), mload(reason))
      }
    }
  }
} else {
  return true;
}

In essence, if the target is a contract, the enforcer requires it to fully implement EIP-4524. The problem is that this EIP is just a draft proposal, which the project cannot assume to be supported by any receiver contract, and definitely not every receiver contract.

The specs warn us:

⚠️ This EIP is not recommended for general use or implementation as it is likely to change.

Therefore, it is a very dangerous requirement to add in an ERC20 enforcer, and must be left to the implementation to do if it so desires.

Impact

ERC20s enforced by HolographERC20 are completely uncomposable. They cannot be used for almost any DeFi application, making it basically useless.

Tools Used

Manual audit

Recommended Mitigation Steps

Remove the EIP-4524 requirements altogether.

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/HolographERC721.sol#L962

Vulnerability details

Description

HolographERC721.sol is an enforcer of the ERC721 standard. In its fallback function, it calls the actual implementation in order to handle additional logic.

If Holographer is called with no calldata and some msg.value, the call will reach the receive() function, which does not forward the call down to the implementation.

This can be a serious value leak issue, because the underlying implementation may have valid behavior for handling sending of value. For example, it can mint the next available tokenID and credit it to the user. Since this logic is never reached, the entire msg.value is just leaked.

Impact

Leak of value when interacting with an NFT using the receive() or fallback() callback. Note that if NFT implements fallback OR receive() function, execution will never reach either of them from the enforcer's receive() function.

Tools Used

Manual audit

Recommended Mitigation Steps

Funnel receive() empty calls down to the implementation.

Lines of code

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

Vulnerability details

Description

The Operator CLI uses jobEstimator() static function to calculate the required gas cost to perform executeJob(). The problem is that it overestimates used gas. The gas() opcode is called at the end of the assembly block, but needs to be calculated right after the call and correct for the memory storage of "result" variable:

assembly {
  calldatacopy(0, bridgeInRequestPayload.offset, sub(bridgeInRequestPayload.length, 0x40))
  /**
   * @dev bridgeInRequest doNotRevert is purposefully set to false so a rever would happen
   */
  mstore8(0xE3, 0x00)
  let result := call(gas(), sload(_bridgeSlot), callvalue(), 0, sub(bridgeInRequestPayload.length, 0x40), 0, 0)
  /**
   * @dev if for some reason the call does not revert, it is force reverted
   */
  if eq(result, 1) {
    returndatacopy(0, 0, returndatasize())
    revert(0, returndatasize())
  }
  /**
   * @dev remaining gas is set as the return value
   */
  mstore(0x00, gas())
  return(0x00, 0x20)
}

The impact of this overestimation of gas is that honest operators may reject a job to be executed, as they calculate higher gas consumption which means it will not be executed successfully. Therefore, executions may be stuck on the destination chain indefinitely.

Impact

Executions may be stuck in destination chain.

Tools Used

Manual audit

Recommended Mitigation Steps

Gas needs to be calculated right after the call and correct for the memory storage of "result" variable: