Holograph contest - Jeiwan's results

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographBridge.sol#L248-L249 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L593-L596

Vulnerability details

Impact

User can avoid paying gas fees by setting gasPrice to 1 wei and gasLimit to 0. Operators will not receive a gas compensation. Also, fallback operators won't be able to pick up such jobs.

Proof of Concept

1. Bridging out is a public function that can be called by anyone. The caller is able to set the gas price and the gas limit of the bridge-in call on the destination chain (HolographBridge.sol#L245-L251).
2. gasLimit and gasPrice are passed to the operator contract as is (HolographBridge.sol#L274-L282).
3. In the send function, the operator contract calculates hlgFee based on the gasLimit and gasPrice set by the user (HolographOperator.sol#L593-L596). Actual calculation happens in the getHlgFee function of LayerZeroModule (LayerZeroModule.sol#L178).
4. There's a check for gasPrice == 0, but no check for gasLimit == 0 (LayerZeroModule.sol#L191-L193).
5. Thus, we cannot set gasPrice to 0 (otherwise a real gas price will be used). So we set gasPrice to 1 wei and gasLimit to 0. getHlgFee will return 0 (LayerZeroModule.sol#L194).
6. When hlgFee is 0, we need to send 1 wei to pass this check – HolographOperator.sol#L595.
7. 0 fee will be collected from the user (HolographOperator.sol#L596). However, they will still pay 1 wei. Operator will receive 0 hlgFee (HolographOperator.sol#L622).
8. gasLimit and gasPrice are then encoded and passed to LayerZeroModule (HolographOperator.sol#L635-L647).
9. LayerZeroModule ignores them (LayerZeroModule.sol#L129-L130).
10. But they're still encoded in the crossChainPayload.
11. When a message is received on a destination chain, payload is passed to the operator contract as is (LayerZeroModule.sol#L122).
12. The operator contract creates a new job, no gas checks so far (HolographOperator.sol#L522).
13. When an operator executes the job, gas limit and price are extracted (HolographOperator.sol#L310-L321).
14. If gasPrice is 1 wei, fallback operator will never be able to pick up the job since tx.gasprice will always be higher (on most blockchains) (HolographOperator.sol#L354).
15. This check will always pass if gasLimit is 0 (HolographOperator.sol#L415).
16. hTokenValue will be 0 here, so the operator who's executing the job won't receive a gas compensation (HolographBridge.sol#L218). This value was set to 0 on step 7.

Even though the jobEstimator function can be used to estimate the gas consumption of a bridgeInRequest call, it still doesn't tell whether the gas compensation is higher that the cost of a bridge-in transaction.

Tools Used

Manual review

Recommended Mitigation Steps

Mitigation steps might include:

1. disallowing setting gas limit to 0;
2. ensuring hToken gas compensation is always greater than 0;
3. allowing operators to fail a job without the risk of being slashed when gas compensation is not enough;
4. forcing users to estimate the gas cost of the bridge-in transaction when calling bridgeOutRequest.

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

Impact

Royalties withdrawing of a Holographed ERC721 collection can be locked indefinitely due to a royalty token not returning a boolean during transfers.

Proof of Concept

The _payoutToken and _payoutTokens functions of the PA1D contract are used to withdraw royalties accrued in ERC20 tokens (PA1D.sol#L405-L443):

function _payoutToken(address tokenAddress) private {
  address payable[] memory addresses = _getPayoutAddresses();
  uint256[] memory bps = _getPayoutBps();
  uint256 length = addresses.length;
  ERC20 erc20 = ERC20(tokenAddress);
  uint256 balance = erc20.balanceOf(address(this));
  require(balance > 10000, "PA1D: Not enough tokens to transfer");
  uint256 sending;
  //uint256 sent;
  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;
  }
}

To make a payout, the transfer function is called and it's expected that it returns a true. Otherwise, the call wil fail with the "PA1D: Couldn't transfer token" error. However, not all ERC20 implementations return a boolean in the transfer function. If such token is used for royalties, _payoutToken and _payoutTokens will always revert and it won't be possible to withdraw royalties.

Tools Used

Manual review

Recommended Mitigation Steps

Consider improving the validation of the return value of the ERC20 transfer function. Use this library from OpenZeppelin as a reference.

Lines of code

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

Vulnerability details

Impact

A portion of collected royalties in ETH will be locked in the PA1D contract due to the subtraction of the ETH transfer stipend. There's no function that uses this amount to compensate gas spending, thus it'll get accumulated in the contract and won't be withdrawable.

Proof of Concept

The _payoutEth function in the PA1D contract subtracts (23300 * payoutAddresses.length) + payoutAddresses.length from the total withdrawable balance (PA1D.sol#L382-L399):

function _payoutEth() private {
  address payable[] memory addresses = _getPayoutAddresses();
  uint256[] memory bps = _getPayoutBps();
  uint256 length = addresses.length;
  // accommodating the 2300 gas stipend
  // adding 1x for each item in array to accomodate rounding errors
  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 remaining balance is then distributed to payout addresses proportionally to each address' share.

Thus, gasCost will never be withdrawn from the contract. Moreover, accruing the amount doesn't make sense for several reasons:

1. it cannot be withdrawn from the contract, thus it cannot be used to compensate gas spending on payouts;
2. gasCost is calculated wrongly: it counts only gas limit, without multiplying it by gas price;
3. the stipend amount is wrong (23300), the correct one is 2300;
4. hardcoding the value can cause problems in the future since the transfer stipend may be changed in a future hard fork of Ethereum.

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing the gas cost calculation and subtraction.

Lines of code

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

Vulnerability details

Impact

A source contract can burn and transfer NFTs of users without their permission.

Proof of Concept

Every Holographed ERC721 collection is paired with a source contract, which is the user created contract that's extended by the Holographed ERC721 contract (HolographFactory.sol#L234-L246). A source contract, however, has excessive privileges in the Holographed ERC721. Specifically, it can burn and transfer users' NFTs without their approval (HolographERC721.sol#L500, HolographERC721.sol#L577):

function sourceBurn(uint256 tokenId) external onlySource {
  address wallet = _tokenOwner[tokenId];
  _burn(wallet, tokenId);
}

function sourceTransfer(address to, uint256 tokenId) external onlySource {
  address wallet = _tokenOwner[tokenId];
  _transferFrom(wallet, to, tokenId);
}

While this might be desirable for extensibility and flexibility, this puts users at the risk of being robbed by the source contract owner or a hacker who hacked the source contract owner's key.

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing the sourceBurn and sourceTransfer functions of HolographERC721 and requiring user approval to transfer or burn their tokens (burn and safeTransferFrom can be called by a source contract instead of sourceBurn and sourceTransfer).

Lines of code

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

Vulnerability details

Impact

Non-operators are allowed to execute jobs, slash other operators, and receive the hToken reward.

Proof of Concept

In the executeJob function of HolographOperator, there's no check for whether a call was made by an operator (HolographOperator.sol#L301), which allows non-operators to execute jobs. Moreover, when nonRevertingBridgeCal is called in the function (HolographOperator.sol#L420), msg.sender is passed as the hToken receiver address:

HolographOperatorInterface(address(this)).nonRevertingBridgeCall{value: msg.value}(
  msg.sender,
  bridgeInRequestPayload
)

HolographOperator.sol#L455:

/**
  * @dev hToken recipient is injected right before making the call
  */
mstore(0x84, msgSender)

Which is then passed to the bridgeInRequest call of HolographBridge to receive hTokens (HolographBridge.sol#L195):

if (hTokenValue > 0) {
  /**
    * @dev mint the specific hToken amount for hToken recipient
    *      this value is equivalent to amount that is deposited on origin chain's hToken contract
    *      recipient can beam the asset to origin chain and unwrap for native gas token at any time
    */
  require(
    HolographERC20Interface(hToken).holographBridgeMint(hTokenRecipient, hTokenValue) ==
      HolographERC20Interface.holographBridgeMint.selector,
    "HOLOGRAPH: hToken mint failed"
  );
}

There's a test that was supposed to cover this behavior (14_holograph_operator_tests.ts#L821-L833):

it('Should fail non-operator address tries to execute job', async () => {
  operatorJobTokenId++;
  while (!(await createOperatorJob(l1, l2, operatorJobTokenId, true))) {
    operatorJobTokenId++;
  }
  let payloadHash: string = availableJobs[0] as string;
  let payload: string = availableJobs[1] as string;
  let operatorJob = await l1.operator.getJobDetails(payloadHash);
  let jobOperator = pickOperator(l1, operatorJob[2], true);
  await expect(l1.operator.connect(jobOperator).executeJob(payload)).to.be.revertedWith(
    'HOLOGRAPH: operator has time'
  );
});

However, it's a false positive since the revert error is different (HolographOperator.sol#L342-L350).

There are two scenarios how this can play out:

1. When the picked job operator is the zero address, anyone can execute the job right away. This whole piece is skipped, the only checks are: the hash check and the gas limit check.
2. When the picked job operator is an actual operator. In this case, the chosen operator has a chance to execute the job. However, if it's slow doing that, other operators can pick the job up, including non-operators. Moreover, a non-operator will slash the chosen operator (HolographOperator.sol#L374-L382)! But they won't be able to withdraw the tokens due to this check (they won't be able to bond to withdraw).

Tools Used

Manual review

Recommended Mitigation Steps

In the executeJob function, consider checking whether the caller is an operator.

[L-01] Bridge can mint Holographed ERC20 tokens

Targets

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

Impact

A bridge can mint an arbitrary amount of ERC20 tokens in a Holographed ERC20 contract.

Proof of Concept

HolographERC20.sol#L415:

/**
  * @dev Allows the bridge to mint tokens (used for hTokens only).
  */
function holographBridgeMint(address to, uint256 amount) external onlyBridge returns (bytes4) {
  _mint(to, amount);
  return HolographERC20Interface.holographBridgeMint.selector;
}

While the function comment says the function will be used only in the hToken, leaving thus function in a contract that will be used by users of Holograph creates unnecessary risks.

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing the holographBridgeMint function and leaving it only in the hToken contract.

[L-02] Fallback operators can be duplicated

Targets

• https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L526-L530
• https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L1185-L1193

Impact

When picking random fallback operators, there's a chance of getting duplicates, which means the real number of fallback operators will be less than expected (less than 5). It's also possible that all five fallback operators will be the same operator. In some rare situations (e.g. when the picked operator hasn't executed the job) this may lead to a stalled job because none of the fallback operators (it can be only one operator) is willing to execute the job.

Proof of Concept

When creating a new job, the list of fallback operators is filled and each operator is chosen pseudo-randomly (HolographOperator.sol#L522-L533):

_operatorJobs[jobHash] = uint256(
  ((pod + 1) << 248) |
    (uint256(_operatorTempStorageCounter) << 216) |
    (block.number << 176) |
    (_randomBlockHash(random, podSize, 1) << 160) |
    (_randomBlockHash(random, podSize, 2) << 144) |
    (_randomBlockHash(random, podSize, 3) << 128) |
    (_randomBlockHash(random, podSize, 4) << 112) |
    (_randomBlockHash(random, podSize, 5) << 96) |
    (block.timestamp << 16) |
    0
); // 80 next available bit position && so far 176 bits used with only 128 left

HolographOperator.sol#L1185-L1193:

function _randomBlockHash(
  uint256 random,
  uint256 podSize,
  uint256 n
) private view returns (uint256) {
  unchecked {
    return (random + uint256(blockhash(block.number - n))) % podSize;
  }
}

Since there's no check for whether a fallback operator is already in the list, duplicates are possible.

Tools Used

Manual review

Recommended Mitigation Steps

Consider checking whether a fallback operator is already in the list before adding it.

[L-03] Full minimal bond amount is slashed, instead of a percentage

Targets

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

Impact

A failed operator gets slashed for a full minimal bond amount, instead of only a percentage, as explained in Important flows.

Proof of Concept

HolographOperator.sol#L371-L385:

/**
  * @dev time to reward the current operator
  */
uint256 amount = _getBaseBondAmount(pod);
/**
  * @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;

Tools Used

Manual Review

Recommended Mitigation Steps

Consider either updating the documentation to reflect the real behavior or updating the code to reflect the documentation.