I've competed in this contest between 18/10/22-25/10/22 and achieved first place. Holograph is an multi-chain NFT platform. Repo is here.
HIGH: 4
MED: 7
HIGH: MEV: Operator can bribe miner and steal honest operator's bond amount if gas price went high 🚩
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.
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.
HIGH: LayerZeroModule miscalculates gas, risking loss of assets
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:
Less severe - we waste excess gas, which is refunded to the lzReceive() caller (Layer Zero)
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.
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.
HIGH: Royalties cannot be collected for many ERC20 tokens (USDT, BNB and many more) due to use of transfer function.
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.
Recommended mitigation steps:
Use OpenZeppelin's SafeERC20 library to interact with ERC20 tokens.
HIGH: Honest operator can lose their bonded amount although gas price was unacceptable during their slot
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
Proof of Concept
User submits a transaction with gas price X on target chain
Gas price goes high (1.2X), primary operator cannot execute the job
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.
MED: isOwner / onlyOwner checks can be bypassed by attacker in ERC721/ERC20 implementations 🚩
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
Recommended mitigation steps:
Refactor the code to represent msg.sender information in a bug-free way.
MED: Attacker can force chaotic operator behavior
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:
bondUtilityToken(attacker, large_amount, high_pod_number)
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 uniformly, 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 occurrence, 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
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.
MED: Execution may be stuck in destination chain as operators estimate gas consumption
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.
Recommended Mitigation Steps
Gas needs to be calculated right after the call and correct for the memory storage of "result" variable:
MED: Insufficient validation in enforcer's OnERC20Received() callback may lead to loss of funds in implementations.
Explanation
HolographERC20's onERC20Received() is presumably a callback that is called when the Holographer receives tokens. Its implementation is:
if (_isEventRegistered(HolographERC20Event.beforeOnERC20Received)) {
require(SourceERC20().beforeOnERC20Received(account, sender, address(this), amount, data));
}
try ERC20(account).balanceOf(address(this)) returns (uint256 balance) {
require(balance >= amount, "ERC20: balance check failed");
} catch {
revert("ERC20: failed getting balance");
}
if (_isEventRegistered(HolographERC20Event.afterOnERC20Received)) {
require(SourceERC20().afterOnERC20Received(account, sender, address(this), amount, data));
}The issue is that the balance check is insufficient to verify that "amount" has been sent to the contract. It could already have this amount in the balance, or it can be called any number of times after a single transfer of the tokens.
This leads to very severe risks in implementations which receive beforeOnERC20Received / afterOnERC20Received calls. They will assume funds were sent and may well reward user for the transfer, although they have not actually sent the tokens.
Impact
ERC20 implementations that trust enforcer's checks in onERC20Received() to do fund related activity are subject to loss of funds.
Recommended Mitigation Steps
Implement some caching of current balance, which will be compared to new balance in onERC20Received.
MED: HolographERC20 breaks composability by forcing usage of draft proposal EIP-4524
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.
Recommended Mitigation Steps
Remove the EIP-4524 requirements altogether.
MED: Some royalty ETH will be stuck in the Holographer contract forever.
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.
Recommended mitigation steps:
Do not calculate gas costs for ETH transfers.
MED: Incorrect implementation of ERC721 may have bad consequences for receiver
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.
Recommended mitigation steps:
Pass the msg.sender parameter, as the ERC721 standard requires.
Comments