Holograph contest - adriro's results

Lines of code

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

Vulnerability details

Impact

Bridged events that succeed in the origin chain may fail due to different reasons in the destination chain, leaving the jobs as processed but failed and irrecoverable. In the case of ERC20/ERC721 bridging this may lead to a burned token in the origin chain while nothing in the destination.

Bridged events are stored as jobs by the messaging layer and later completed by operators in the executeJob function of the HolographOperator contract. When a job is executed, after the required checks are passed, the bridged payload is passed to the nonRevertingBridgeCall function which does the bridge invocation to process the message. If the call to nonRevertingBridgeCall reverts, then the executeJob won't revert because the bridge call is wrapped in a try/catch construction. The job will be processed while the underlying call fails. Even though it's marked as failed using the _failedJobs mapping, there isn't a procedure to recover from these failures, the job can't be processed again.

The call to nonRevertingBridgeCall may fail and revert due to a revert anywhere in the chain that depends on this function. But it also can fail due to EIP-150 (see https://eips.ethereum.org/EIPS/eip-150) and the rule of 1/64, the function nonRevertingBridgeCall may run out of gas while the caller still have some gas left to continue the execution. An operator (accidentally or intentionally) can trigger an scenario in which the gas limit is accommodated in a way that makes the underlying call to the bridge to run out of gas while still saving a bit to complete and finalize the execution of the executeJob function.

Proof of Concept

1. The bridging process may fail because the bridgeIn function of the holographable contract reverts or the function that handles the event present in the source contract reverts.
2. An out of gas error could also be possible (or even carefully crafted intentionally by a bad actor) while still having gas available in the caller to complete the executeJob function and mark the job as failed. As described by the EIP-150, the caller retains at least 1/64 of the gas forwarded to the call. This makes it possible for the call in line 420 to revert due to lack of gas, while still continuing the execution of the executeJob function (and successfully completing the transaction).

Lines of code

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

Vulnerability details

Impact

The safeTransferFrom function present in the HolographERC721 contract calls the onERC721Received hook in the receiver with the wrong address in the first argument. The argument should match the operator (which is the one calling the transfer function) and instead the implementation is sending the address of the contract (address(this)). Integration contracts that hook on this callback will be receiving the wrong information.

Proof of Concept

Any contract that implements the onERC721Received expects the first argument to be the operator (the account that triggered the transfer) and not the address of the token contract.

contract SimpleERC721Receiver is ERC721TokenReceiver {
  function onERC721Received(
    address _operator,
    address _from,
    uint256 _tokenId,
    bytes calldata _data
  ) external returns (bytes4) {
    // _operator here should be the caller to the transfer function
    // not the address of the ERC721 contract!
    
    return ERC721TokenReceiver.onERC721Received.selector;
  }
}

Recommended Mitigation Steps

Change address(this) to msg.sender in the first argument in the onERC721Received call (line 467).

Lines of code

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

Vulnerability details

The pod and operator selection when a message is cross chained (function crossChainMessage of the HolographOperator contract) is chosen by calculating a pseudorandom value using the formula:

uint256 random = uint256(keccak256(abi.encodePacked(jobHash, _jobNonce(), block.number, block.timestamp)));

Since the job hash and nonce can be known in advance, the function can be executed in a specific block number such that random calculation favors a particular operator. Block timestamp is also malleable by miners.

Duplicate getters for unstructured storage access

Most of the contracts have an internal function to sload a storage variable (due to the usage of unstructured storage), and also have an external function which does exactly the same. For example in HolographBridge, there's getHolograph() and _holograph() which have the same code. The same happens with most of the unstructured storage variables in most of the contracts in scope. Consider using a single public function.

Duplicate calls in isOwner() function of PA1D contract

This function checks if the sender matches the owner and admin, and then calls each getter a second time by executing an external call.

Define constant for basis points in PA1D contract

There are several references to 10000 which is the basis point for the bps calculation. Consider defining a constant. Occurrences are in lines 395, 438, 477, 551, 553, 641, 643.

Change _validatePayoutRequestor to be a modifier in PA1D contract

The _validatePayoutRequestor() function present in the PA1D contract works as a guard for the payout function. Consider moving it to a modifier.

No need to define parameter receiver as payable in setRoyalties function of PA1D contract

The receiver parameter is marked as payable though it isn't needed.

getFees and getRoyalties have the same duplicated code in PA1D contract

The body of both functions is exactly the same. Consider extracting duplicated code to a private function.

The init function in the LayerZeroModule contract doesn't initialize lZEndpoint

Though there's an admin setter to set this value, consider initializing the lZEndpoint variable (_lZEndpointSlot slot) in the initializer so that it's fully set up at construction time. Note that this is a crucial part of the bridging process, and failure to correctly initialize this variable will make the whole process fail.

Unnecessary usage of low level assembly code in function lzReceive of LayerZeroModule contract

This function relies on assembly to simply do a couple of checks and revert with an error in case the conditions are not met. Consider using solidity, which will improve readability and will be less error prone.

Don't define receive and fallback functions for the default behavior

Several contracts define the receive and/or fallback functions with a single instruction to revert. This matches the default behavior taken by solidity if those functions aren't defined.

Contracts where this is present: LayerZeroModule, HolographBridge, HolographFactory, HolographOperator.

if (condition) { return true } else { return false } code construction should just return the condition

Constructions that test a condition and return true or false based on that condition should directly return the condition.

Occurrences:

• https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC721.sol#L298
• https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L289
• https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L755

Unnecessary functions marked as payable

There's no indication that these should receive a payment though they are marked as payable.

• approve(address to, uint256 tokenId) in HolographERC721 contract.
• safeTransferFrom(address from, address to, uint256 tokenId) in HolographERC721 contract.
• safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) in HolographERC721 contract.
• transfer(address to, uint256 tokenId) in HolographERC721 contract.
• transferFrom(address from, address to, uint256 tokenId) in HolographERC721 contract.
• transferFrom(address from, address to, uint256 tokenId, bytes memory data) in HolographERC721 contract.

Use solidity interfaceId to test for ERC165 interface support

In HolographERC721 contract line 465 and 466 the code uses the selectors of ERC165 and ERC721TokenReceiver to test for ERC165 interface support. This works because those interfaces contain a single function definition. Consider using instead type(ERC165).interfaceId and type(ERC721TokenReceiver).interfaceId.

In HolographERC20 contract line 644 use type(ERC165).interfaceId and in line 647 use type(ERC20Safer).interfaceId instead of the magic constant "0x534f5876".

Simplify _isContract implementation

Several contracts in the codebase implement a _isContract function to check if an address contains code through the extcodehash opcode using low level assembly. Consider replacing this with the simpler expression contractAddress.code.length > 0, which doesn't rely on assembly code.

Occurrences: HolographERC721, HolographERC20, HolographOperator, HolographFactory.

Naming convention for private functions

The SourceERC721() and SourceERC20() function present in the HolographERC721 and HolographERC20 contracts are private functions and don't follow the naming convention used mostly throughout the code where these type of functions are named starting with an underscore and a lowercase letter.

Define literal magic constants as named constants

• _royalties function in HolographERC721 contract, value 0x0000000000000000000000000000000000000000000000000000000050413144.
• permit function in HolographERC20 contract. Magic value 0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9 can be defined as a constant calculated using keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)").

No need to define low level literals as 32 bytes left padded with zeros

There are several places throughout the code base that define literals in low level code by left padding it with zeros to make those be 32 bytes long. This is unnecessary and damages readability. For example, HolographERC20 contract line 222 contains 0x0000000000000000000000000000000000000000000000000000000000000001 just to express a 1.

Incorrect EIP712 initialization in HolographERC20 contract

The initialization of the HolographERC20 contract calls the _eip712_init function with a domainSeperator parameter as the first argument. Aside from the spelling mistake, this argument should correspond to the name field of the EIP712 construct, the domain separator is the output of that function that is handled internally.

Unnecessary use of unchecked math in increaseAllowance in HolographERC20 contract

The code uses unchecked math to calculate the new allowance in line 424, just to check the overflow in line 427 (this line is also unnecessarily wrapped in an unchecked block since this is just a comparison). Safe math could be directly used here to simplify the construction.

uint256 newAllowance = currentAllowance + addedValue;

Nested if constructions can be defined using a single condition

An if construction nested in another if construction could have both guards combined to form a single if statement.

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

Check for same length array argument functions

Function that operate simultaneously on more than one array should sanity check that those arrays have the same length.

Occurrences:

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

Rename msgSender() function in ERC20H and ERC721H contracts

The function named msgSender() could be inadvertently confused with an utility function that returns msg.sender. For example, OpenZeppelin's Context contract defines a very similar function named _msgSender(), this is a highly popular library present in most solidity codebases.

Consider renaming this function to something like holographerMsgSender().

Improve low level variable override in jobEstimator function of the HolographOperator contract

The doNotRevert variable is set to 0 by executing a low level call to mstore8(0xE3, 0x00). This should be ok, since that byte corresponds to the last byte of the word that represents that boolean parameter. However, consider using mstore(0xC4, 0) instead since it better aligns with the intention of overriding the doNotRevert argument, which is the seventh argument in the payload considering the 4 byte selector (0x04 + 6 * 0x20 = C4).

Incorrect variable size when decoding job in getJobDetails of the HolographOperator contract

The variable startTimestamp should be defined as a uint80 instead of uint64 to align it with how it is initially packed (it occupies bits 16 to 96, 96-16=80).

Consider using EIP712 to sign structured data in deployHolographableContract of the HolographFactory contract

This function calculates a hash over the DeploymentConfig hash to verify the submitted signature. This is an ideal case to use the more user friendly EIP712 hashing and signing.

supportsInterface should return true for the EIP165 interface ID in ERC20H and ERC721H

Both contracts define the supportsInterface(bytes4) function and return just false for all cases. According to the EIP-165 specification, this function should return true when called with the value 0x01ffc9a7 (the interface id for the IERC165 interface).

Change the implementation to:

function supportsInterface(bytes4 interfaceId) external pure returns (bool) {
  return interfaceId == type(IERC165).interfaceId;
}

Double call to _getReceiver in royaltyInfo function of PA1D contract

The function _getReceiver which loads the receiver from storage is called twice in the body of royaltyInfo. Consider storing the first call in a local variable to prevent a second call.

Double call to _getReceiver in getRoyalties function of PA1D contract

The function _getReceiver which loads the receiver from storage is called twice in the body of getRoyalties. Consider storing the first call in a local variable to prevent a second call.

Double check of token existence in bridgeIn function of HolographERC721 contract

The code checks that the token doesn't exist in line 404 and then calls the _mint which executes the exact same check in line 817.

Unnecessary swap when removing last element from array in _removeTokenFromAllTokensEnumeration of HolographERC721 contract

The code removes a token from the _allTokens array by swapping it for the last element and calling pop. If the element to be removed is that last element then the swap is not needed.

The _transferFrom function in the HolographERC721 contract removes and adds the token again to the enumeration

The _transferFrom function will call _removeTokenFromOwnerEnumeration which will remove the token from the _allTokens array, just to add it back again in the _addTokenToOwnerEnumeration function. Consider refactoring the code to save gas in this scenario, where the token just needs to be removed from the sender and added to the recipient of the transfer, there's no need to modify the _allTokens array during a transfer.

Unchecked math gas savings in HolographERC20 contract

• In the _burn function, the update to the _totalSupply variable can be placed inside the unchecked block, since accountBalance <= _totalSupply and amount <= accountBalance (due to the check above). https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L633
• In the _mint function, the update to the _balances[to] variable can be placed inside an unchecked block, since balance <= totalSupply and the overflow check should be covered in line 685. https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L686
• In the _transfer function, the update to the _balances[recipient] variable can be placed inside the unchecked block, since the sum of all balances are limited by the totalSupply and an overflow here should be impossible. https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/enforcer/HolographERC20.sol#L702

Move the _blockTime variable to a constant in the HolographOperator contract

This storage variable is initialized with a literal value of 60 and doesn't have a setter to override its value. Consider moving it to a constant to avoid loading it from storage and save gas.

Store value of _operatorTempStorageCounter variable locally to avoid a re-read in function crossChainMessage of the HolographOperator contract

The _operatorTempStorageCounter variable is incremented in line 495, and then re-read from storage a second time in line 517 and a third time in line 524. Save the value in a local variable after the increment and use that value later

uint256 storageCounter = ++_operatorTempStorageCounter;

bridgeIn function in HolographFactory does an external call to a function in the same contract

The bridgeIn function in the HolographFactory contract calls the deployHolographableContract externally by doing HolographFactoryInterface(address(this)).deployHolographableContract(...). Consider changing deployHolographableContract to be public so it can be called internally to save gas.