Holograph contest - Lambda's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. The contract supports the admin to set MessagingModule, Registry, etc respectively. Sometimes, one need to upgrade all modules to a new version at the SAME time to avoid mixed versions in one system, this is not supported in this implementation.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

Introduce a comprehensive function to include all the functionality of setters so that in one transaction, all settings can be done and support a full clean upgrade of all modules from version A to version B.

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/24bc4d8dfeb6e4328d2c6291d20553b1d3eff00b/src/HolographOperator.sol#L255

Vulnerability details

Impact & Proof Of Concept

The current gas price handling in HolographOperator has two problems: 1.) When a gas spike happened during the first blockTimes of a job (i.e., executing it was not profitable for the operator), but the gas price comes down afterwards, the chosen operator will still get slashed (for instance when the fallback operator was quicker to execute the job / frontrun the original operator after the gas price went down), although he did not have a chance to execute the job profitably, because the gas price was too high. In such a scenario, the operator loses funds unnecessarily. 2.) When gas prices increase persistently for a longer time (which happened in the past, see for instance https://etherscan.io/chart/gasprice), nobody else is able to execute the job and it is not profitable for the operator to do so. However, as long as the job is not executed, the operator cannot accept new jobs or unstake, meaning he is more or less forced to still execute the job at a loss, meaning he loses funds.

Recommended Mitigation Steps

Instead of simply checking if the current gas price is higher (and disallowing execution by others in such cases), consider a more sophisticated scheme. The historical prices (or rather the volatility) could be incorporated (with a gas price oracle) to prevent slashing / unprofitable executions in situations where 1.) volatility is very high (gas spike) 2.) the price increased persistently (i.e., a higher price with a low volatility)

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/24bc4d8dfeb6e4328d2c6291d20553b1d3eff00b/src/enforcer/PA1D.sol#L317

Vulnerability details

Impact

PA1D uses require(erc20.transfer(addresses[i], sending), "PA1D: Couldn't transfer token"); to transfer the token to the recipient. This will not work for tokens that do not return a boolean on success (USDT, BNB, OMG). While it is known as a low severity issue that unsafe ERC20 methods are used there, the severity in this specific case is higher: PA1D receives royalties, so it is likely that it will receive such a token (especially USDT) at some point. The transfer to the contract will succeed without any problems. However, nobody will be able to transfer the tokens out, meaning they will be stuck forever, leading to a loss of funds.

Proof Of Concept

Let's say an NFT is sold in USDT with 10% royalties. These royalties cannot be retrieved by the configured payout addresses, because the transfer will revert.

Recommended Mitigation Steps

Use OpenZeppelin's SafeERC20 library.

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/24bc4d8dfeb6e4328d2c6291d20553b1d3eff00b/src/HolographOperator.sol#L283

Vulnerability details

Impact

When enough time has passed (the exact amount depends on if address(0) is a backup operator or if a backup operator index was chosen that is now too large), a job that was assigned to a particular operator can be executed by everyone. This address will get the slashed amount, which is done by increasing _bondedAmounts:

_bondedAmounts[msg.sender] += amount;

However, the problem is that msg.sender may not be an operator currently. In such a scenario, this amount is lost forever. The operator cannot call bondUtilityToken (which requires _bondedAmounts[operator] == 0) or unbondUtilityToken (which requires _bondedOperators[operator] != 0).

Proof Of Concept

Bob sees that a job was not executed in a long time and to earn the slashing reward, he calls executeJob. However, this was the first interaction with the contract and he is not registered as an operator. While the call still succeeds, there is no way for Bob to retrieve the reward. Even worse, he now cannot register as an operator (by calling bondUtilityToken), as this call will also always revert. There is therefore no way for Bob to be an operator.

Recommended Mitigation Steps

Introduce a function to recover these rewards (or transfer them out directly).

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/24bc4d8dfeb6e4328d2c6291d20553b1d3eff00b/src/enforcer/HolographERC721.sol#L366

Vulnerability details

Impact

According to EIP-721, we have the following for safeTransferFrom:

///  (...) When transfer is complete, this function
///  checks if `_to` is a smart contract (code size > 0). If so, it calls
///  `onERC721Received` on `_to` and throws if the return value is not
///  `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`.

According to the specification, the function must therefore always call onERC721Received, not only when it has determined via ERC-165 that the contract provides this function. Note that in the EIP, the provided interface for ERC721TokenReceiver does not mention ERC-165. For the token itself, we have: interface ERC721 /* is ERC165 */ { However, for the receiver, the provided interface there is just: interface ERC721TokenReceiver { This leads to failed transfers when they should not fail, because many receivers will just implement the onERC721Received function (which is sufficient according to the EIP), and not supportsInterface for ERC-165 support.

Proof Of Concept

Let's say a receiver just implements the IERC721Receiver from OpenZeppelin: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/IERC721Receiver.sol Like the provided interface in the EIP itself, this interface does not derive from EIP-165. All of these receivers (which are most receivers in practice) will not be able to receive those tokens, because the require statement (that checks for ERC-165 support) reverts.

Recommended Mitigation Steps

Remove the ERC-165 check in the require statement (like OpenZeppelin does: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L436)

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/24bc4d8dfeb6e4328d2c6291d20553b1d3eff00b/src/enforcer/HolographERC721.sol#L272

Vulnerability details

Impact

According to EIP-721, we have for approve:

///  Throws unless `msg.sender` is the current NFT owner, or an authorized
///  operator of the current owner.

An operator in the context of EIP-721 is someone who was approved via setApprovalForAll:

/// @notice Enable or disable approval for a third party ("operator") to manage
///  all of `msg.sender`'s assets
/// @dev Emits the ApprovalForAll event. The contract MUST allow
///  multiple operators per owner.
/// @param _operator Address to add to the set of authorized operators
/// @param _approved True if the operator is approved, false to revoke approval
function setApprovalForAll(address _operator, bool _approved) external;

Besides operators, there are also approved addresses for a token (for which approve is used). However, approved addresses can only transfer the token, see for instance the safeTransferFrom description:

/// @dev Throws unless `msg.sender` is the current owner, an authorized
///  operator, or the approved address for this NFT.

HolographERC721 does not distinguish between authorized operators and approved addresses when it comes to the approve function. Because _isApproved(msg.sender, tokenId) is used there, an approved address can approve another address, which is a violation of the EIP (only authorized operators should be able to do so).

Proof Of Concept

Bob calls approve to approve Alice on token ID 42 (that is owned by Bob). One week later, Bob sees that a malicious address was approved for his token ID 42 (e.g., because Alice got phished) and stole his token. Bob wonders how this is possible, because Alice should not have the permission to approve other addresses. However, becaue HolographERC721 did not follow EIP-721, it was possible.

Recommended Mitigation Steps

Follow the EIP, i.e. do not allow approved addresses to approve other addresses.

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/24bc4d8dfeb6e4328d2c6291d20553b1d3eff00b/src/enforcer/PA1D.sol#L297

Vulnerability details

Impact

_payoutEth iterates over all recipient addresses and calls transfer on them. Because of that, one single recipient can cause the whole payout to fail (either by reverting in the fallback function or by using too much gas).

Note that the same problem theoretically also exists for _payoutToken, although to a lesser extent, because letting ERC20 transfers revert on purpose is harder as a recipient. However, when ERC777 tokens are used it is possible (with the hook) and for some ERC20 tokens it can also happen without the involvement of the user (e.g., the USDC blacklist). The USDC blacklist could potentially be even used by other persons that are not even a recipient to let all payments fail: When it is possible to get a recipient on the blacklist (e.g., by sending him ETH from Tornado Cash), all USDC payouts will fail.

Proof Of Concept

Let's say one recipient receives a very small share (e.g., 1%) of the payouts and is not happy with that. Therefore, he decides to blackmail the other recipients. He does so by creating a contract that reverts in the fallback function if it is configured to do so. The loss for the individual recipient is very small (1%), but the other 99% of the other recipients are also stuck as long as this contract exists.

Recommended Mitigation Steps

Accumulate the share for each recipient and let them retrieve the ETH, i.e. a pull pattern instead of the current push one (see also https://fravoll.github.io/solidity-patterns/pull_over_push.html for more details).