Holograph contest - 0x52's results

Lines of code

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

Vulnerability details

Impact

Operators maliciously slashed

Proof of Concept

A user can create a malicious token that when called by anyone other than themselves and the bridge use an extreme amount of gas. They create a bridge request with a gas limit higher than the block limit. If an operator tries to call execute they will always run out of gas. Meanwhile the malicious user can call the token for very little gas. The malicious user calls the request to slash the operator

Tools Used

Manual Review

Recommended Mitigation Steps

Ensure gas limit is not higher than destination block gas limit

Lines of code

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

Vulnerability details

Impact

Users tokens are still burned at source chain with no way to recover them

Proof of Concept

try
  HolographOperatorInterface(address(this)).nonRevertingBridgeCall{value: msg.value}(
    msg.sender,
    bridgeInRequestPayload
  )
{
  /// @dev do nothing
} catch {
  _failedJobs[hash] = true;
  emit FailedOperatorJob(hash);
}

When a bridging call fails, nothing is done on the destination chain. When sending the message from the source chain their tokens work burned. Since the destination call failed, no tokens were minted on the destination chain. This means their source tokens have been burned and they never received any tokens on the destination chain, leaving them with no tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

Create a function allowing users to create a message back to the source chain for failed transactions and a function that allows them to remint on source chain.

Lines of code

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

Vulnerability details

Impact

bidShares returned are incorrect leading to incorrect royalties

Proof of Concept

Zora Market

function isValidBidShares(BidShares memory bidShares)
    public
    pure
    override
    returns (bool)
{
    return
        bidShares.creator.value.add(bidShares.owner.value).add(
            bidShares.prevOwner.value
        ) == uint256(100).mul(Decimal.BASE);
}

Above you can see the Zora market lines that validate bidShares, which shows that Zora market bidShare.values should be percentages written out to 18 decimals. However PA1D#bidSharesForToken sets the bidShares.creator.value to the raw basis points set by the owner, which is many order of magnitudes different than expected.

Tools Used

Manual Review

Recommended Mitigation Steps

To return the proper value, basis points returned need to be adjusted. Convert from basis points to percentage by dividing by 10 ** 2 (100) then scale to 18 decimals. The final result it to multiple the basis point by 10 ** (18 - 2) or 10 ** 16:

function bidSharesForToken(uint256 tokenId) public view returns (ZoraBidShares memory bidShares) {
    // this information is outside of the scope of our
    bidShares.prevOwner.value = 0;
    bidShares.owner.value = 0;
    if (_getReceiver(tokenId) == address(0)) {
-       bidShares.creator.value = _getDefaultBp();
+       bidShares.creator.value = _getDefaultBp() * (10 ** 16);
    } else {
-       bidShares.creator.value = _getBp(tokenId);
+       bidShares.creator.value = _getBp(tokenId) * (10 ** 16);
    }
    return bidShares;
}

Lines of code

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

Vulnerability details

Impact

Unexpected reverts or portion of ETH permanently stuck in contract

Proof of Concept

uint256 gasCost = (23300 * length) + length;
uint256 balance = address(this).balance;
require(balance - gasCost > 10000, "PA1D: Not enough ETH to transfer");

When calculating gasCost, it is accidentally calculated using 23300 instead of 2300, which is the actual stipend for an ETH transfer. This can lead to unexpected reverts since gas costs is calculated incorrectly.

Tools Used

Manual Review

Recommended Mitigation Steps

Fix gas cost calculations by using the proper stipend for transfer:

-   uint256 gasCost = (23300 * length) + length;
+   uint256 gasCost = (2300 * length) + length;
    uint256 balance = address(this).balance;
    require(balance - gasCost > 10000, "PA1D: Not enough ETH to transfer");