Holograph contest - __141345__'s results

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

Some tokens do not comply with the standard. The transfer() function may have no return value, such as USDT. In such cases, the payout functions will revert and users fund may be locked.

Proof of Concept

// contracts/enforcer/PA1D.sol
416:    require(erc20.transfer(addresses[i], sending), "PA1D: Couldn't transfer token");
439:    require(erc20.transfer(addresses[i], sending), "PA1D: Couldn't transfer token");

Tools Used

Manual analysis.

Recommended Mitigation Steps

Use openzeppelin wrapper for ERC20 transfer.

Lines of code

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

Vulnerability details

Impact

The random is predictable, hence some one could know in advance who will be the next operator.

Proof of Concept

jobHash, _jobNonce(), block.number, block.timestamp are all predictable.

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

Tools Used

Manual analysis.

Recommended Mitigation Steps

Use Chainlink VRF (Verifiable Random Function) as the source of randomness.

Lines of code

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

Vulnerability details

Impact

The sourceContract can transfer or burn any NFT despite the ownership. Hence the owners are not truly owning the token. If the sourceContract is compromised or a malicious user get the access, users NFT will be lost.

Proof of Concept

// contracts/enforcer/HolographERC721.sol
  function sourceTransfer(address to, uint256 tokenId) external onlySource {
    address wallet = _tokenOwner[tokenId];
    _transferFrom(wallet, to, tokenId);
  }

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

  modifier onlySource() {
    address sourceContract;
    assembly {
      sourceContract := sload(_sourceContractSlot)
    }
    require(msg.sender == sourceContract, "ERC721: source only call");
    _;
  }

Tools Used

Manual analysis.

Recommended Mitigation Steps

Disallow sourceTransfer() and sourceBurn() functionality.

Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

Instances number of this issue:

// contracts/enforcer/PA1D.sol
153:  event SecondarySaleFees(uint256 tokenId, address[] recipients, uint256[] bps);

Events not emitted for important state changes

When changing critical state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Instances number of this issue: 18

// contracts/HolographBridge.sol
  function setFactory(address factory) external onlyAdmin {
  function setHolograph(address holograph) external onlyAdmin {
  function setOperator(address operator) external onlyAdmin {
  function setRegistry(address registry) external onlyAdmin {

// contracts/HolographFactory.sol
  function setHolograph(address holograph) external onlyAdmin {
  function setRegistry(address registry) external onlyAdmin {

// contracts/HolographOperator.sol
  function setBridge(address bridge) external onlyAdmin {
  function setHolograph(address holograph) external onlyAdmin {
  function setInterfaces(address interfaces) external onlyAdmin {
  function setMessagingModule(address messagingModule) external onlyAdmin {
  function setRegistry(address registry) external onlyAdmin {
  function setUtilityToken(address utilityToken) external onlyAdmin {

// contracts/enforcer/PA1D.sol
  function _setDefaultReceiver(address receiver) private {
  function _setDefaultBp(uint256 bp) private {
  function _setReceiver(uint256 tokenId, address receiver) private {
  function _setBp(uint256 tokenId, uint256 bp) private {
  function _setPayoutBps(uint256[] memory bps) private {
  function _setTokenAddress(string memory tokenName, address tokenAddress) private {

// contracts/module/LayerZeroModule.sol
  function setBridge(address bridge) external onlyAdmin {
  function setInterfaces(address interfaces) external onlyAdmin {
  function setLZEndpoint(address lZEndpoint) external onlyAdmin {
  function setOperator(address operator) external onlyAdmin {
  function setBaseGas(uint256 baseGas) external onlyAdmin {
  function setGasPerByte(uint256 gasPerByte) external onlyAdmin {

Suggestion: Consider emitting events after sensitive changes take place, to facilitate tracking and notify off-chain clients following the contract’s activity.

Signature Malleability of EVM's ecrecover()

The _verifySigner() function calls the Solidity ecrecover() function directly to verify the given signatures. However, the ecrecover() EVM opcode allows malleable (non-unique) signatures and thus is susceptible to replay attacks.

Although a replay attack seems not possible for deployHolographableContract() since holographerAddress can only be deployed once per singer.

However ensuring the signatures are not malleable is considered a best practice. And it may become a vulnerability if used elsewhere.

Suggestion: Use the ecrecover function from OpenZeppelin's ECDSA library for signature verification. (Ensure using a version > 4.7.3 for there was a critical bug >= 4.1.0 < 4.7.3).

Missing Time locks

When critical parameters of systems need to be changed, it is required to broadcast the change via event emission and recommended to enforce the changes after a time-delay. This is to allow system users to be aware of such critical changes and give them an opportunity to exit or adjust their engagement with the system accordingly. None of the onlyOwner functions that change critical protocol addresses/parameters have a timelock for a time-delayed change to alert:

• users and give them a chance to engage/exit protocol if they are not agreeable to the changes
• team in case of compromised owner(s) and give them a chance to perform incident response.

Instances number of this issue: 18

// contracts/HolographBridge.sol
  function setFactory(address factory) external onlyAdmin {
  function setHolograph(address holograph) external onlyAdmin {
  function setOperator(address operator) external onlyAdmin {
  function setRegistry(address registry) external onlyAdmin {

// contracts/HolographFactory.sol
  function setHolograph(address holograph) external onlyAdmin {
  function setRegistry(address registry) external onlyAdmin {

// contracts/HolographOperator.sol
  function setBridge(address bridge) external onlyAdmin {
  function setHolograph(address holograph) external onlyAdmin {
  function setInterfaces(address interfaces) external onlyAdmin {
  function setMessagingModule(address messagingModule) external onlyAdmin {
  function setRegistry(address registry) external onlyAdmin {
  function setUtilityToken(address utilityToken) external onlyAdmin {

// contracts/enforcer/PA1D.sol
  function _setDefaultReceiver(address receiver) private {
  function _setDefaultBp(uint256 bp) private {
  function _setReceiver(uint256 tokenId, address receiver) private {
  function _setBp(uint256 tokenId, uint256 bp) private {
  function _setPayoutBps(uint256[] memory bps) private {
  function _setTokenAddress(string memory tokenName, address tokenAddress) private {

// contracts/module/LayerZeroModule.sol
  function setBridge(address bridge) external onlyAdmin {
  function setInterfaces(address interfaces) external onlyAdmin {
  function setLZEndpoint(address lZEndpoint) external onlyAdmin {
  function setOperator(address operator) external onlyAdmin {
  function setBaseGas(uint256 baseGas) external onlyAdmin {
  function setGasPerByte(uint256 gasPerByte) external onlyAdmin {

Suggestion: Users may be surprised when critical parameters are changed. Furthermore, it can erode users' trust since they can’t be sure the protocol rules won’t be changed later on. Compromised owner keys may be used to change protocol addresses/parameters to benefit attackers. Without a time-delay, authorized owners have no time for any planned incident response. (May be medium since owner can change critical parameters at anytime that can affect the users poorly).

Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

Instances number of this issue: 16

// contracts/HolographOperator.sol
193:  mapping(bytes32 => uint256) private _operatorJobs;
198:  mapping(bytes32 => bool) private _failedJobs;

218:  mapping(address => uint256) private _bondedOperators;
223:  mapping(address => uint256) private _operatorPodIndex;
228:  mapping(address => uint256) private _bondedAmounts;

// contracts/enforcer/HolographERC20.sol
156:  mapping(address => uint256) private _balances;
161:  mapping(address => mapping(address => uint256)) private _allowances;
186:  mapping(address => uint256) private _nonces;

// contracts/enforcer/HolographERC721.sol
170:  mapping(uint256 => uint256) private _ownedTokensIndex;
175:  mapping(uint256 => address) private _tokenOwner;
180:  mapping(uint256 => address) private _tokenApprovals;
201:  mapping(uint256 => uint256) private _allTokensIndex;
206:  mapping(uint256 => bool) private _burnedTokens;

185:  mapping(address => uint256) private _ownedTokensCount;
190:  mapping(address => uint256[]) private _ownedTokens;
196:  mapping(address => mapping(address => bool)) private _operatorApprovals;

Usage of uint/int smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed

Reference: Layout of State Variables in Storage

Instances number of this issue: 2

// contracts/enforcer/HolographERC721.sol
160:  uint16 private _bps;

// contracts/HolographOperator.sol
208:  uint32 private _operatorTempStorageCounter;

Splitting require() statements that use &&

require() statements with multiple conditions can be split.

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper.

The demo of the gas comparison can be seen here.

Instances number of this issue: 4

// contracts/HolographOperator.sol
857:    require(_bondedOperators[operator] == 0 && _bondedAmounts[operator] == 0, "HOLOGRAPH: operator is bonded");

// contracts/enforcer/Holographer.sol
166:    require(success && selector == InitializableInterface.init.selector, "initialization failed");

// contracts/enforcer/HolographERC721.sol
263:    require(success && selector == InitializableInterface.init.selector, "ERC721: coud not init PA1D");
464-470:
      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"
      );

Duplicated functions can be written into a library

// contracts/HolographBridge.sol
// contracts/HolographOperator.sol
  function _jobNonce() private returns (uint256 jobNonce) {
    assembly {
      jobNonce := add(sload(_jobNonceSlot), 0x0000000000000000000000000000000000000000000000000000000000000001)
      sstore(_jobNonceSlot, jobNonce)
    }
  }

Variables referred multiple times can be cached in local memory

Even for a memory struct variable, the member variable access still has overhead. It can be saved further by caching the final result.

Instances number of this issue: 7

bridgeInRequestPayload.offset and bridgeInRequestPayload.length:

// contracts/HolographOperator.sol
316:      gasLimit := calldataload(sub(add(bridgeInRequestPayload.offset, bridgeInRequestPayload.length), 0x40))
320:      gasPrice := calldataload(sub(add(bridgeInRequestPayload.offset, bridgeInRequestPayload.length), 0x20))

payload.length:

// contracts/HolographOperator.sol
451:      calldatacopy(0, payload.offset, sub(payload.length, 0x20))
461:        mload(sub(payload.length, 0x40)),
469:        sub(payload.length, 0x40),

bridgeInRequestPayload.length:

// contracts/HolographOperator.sol
551:      calldatacopy(0, bridgeInRequestPayload.offset, sub(bridgeInRequestPayload.length, 0x40))
556:      let result := call(gas(), sload(_bridgeSlot), callvalue(), 0, sub(bridgeInRequestPayload.length, 0x40), 0, 0)

Unnecessary variable allocation

No need to allocate extra variable in memory for the following:

_target:

// periphery/Voter.sol
970-975:
    address _target;
    if (HolographInterfacesInterface(_interfaces()).supportsInterface(InterfaceType.PA1D, msg.sig)) {
      _target = _royalties();
      assembly {
        calldatacopy(0, 0, calldatasize())
        let result := delegatecall(gas(), _target, 0, calldatasize(), 0, 0)

holographEnforcer:

// contracts/enforcer/Holographer.sol
229-232:
    address holographEnforcer = getHolographEnforcer();
    assembly {
      calldatacopy(0, 0, calldatasize())
      let result := delegatecall(gas(), holographEnforcer, 0, calldatasize(), 0, 0)

duplicate const declaration can be put into a library

Instances number of this issue: 23

  bytes32 constant _factorySlot = 0xa49f20855ba576e09d13c8041c8039fa655356ea27f6c40f1ec46a4301cd5b23;
  bytes32 constant _holographSlot = 0xb4107f746e9496e8452accc7de63d1c5e14c19f510932daa04077cd49e8bd77a;
  bytes32 constant _jobNonceSlot = 0x1cda64803f3b43503042e00863791e8d996666552d5855a78d53ee1dd4b3286d;
  bytes32 constant _operatorSlot = 0x7caba557ad34138fa3b7e43fb574e0e6cc10481c3073e0dffbc560db81b5c60f;
  bytes32 constant _registrySlot = 0xce8e75d5c5227ce29a4ee170160bb296e5dea6934b80a9bd723f7ef1e7c850e7;
  bytes32 constant _bridgeSlot = 0xeb87cbb21687feb327e3d58c6c16d552231d12c7a0e8115042a4165fac8a77f9;
  bytes32 constant _interfacesSlot = 0xbd3084b8c09da87ad159c247a60e209784196be2530cecbbd8f337fdd1848827;
  bytes32 constant _messagingModuleSlot = 0x54176250282e65985d205704ffce44a59efe61f7afd99e29fda50f55b48c061a;
  bytes32 constant _utilityTokenSlot = 0xbf76518d46db472b71aa7677a0908b8016f3dee568415ffa24055f9a670f9c37;
  bytes32 constant _holographerSlot = 0xe9fcff60011c1a99f7b7244d1f2d9da93d79ea8ef3654ce590d775575255b2bd;
  bytes32 constant _ownerSlot = 0xb56711ba6bd3ded7639fc335ee7524fe668a79d7558c85992e3f8494cf772777;
  bytes32 constant _originChainSlot = 0xd49ffd6af8249d6e6b5963d9d2b22c6db30ad594cb468453047a14e1c1bcde4d;
  bytes32 constant _contractTypeSlot = 0x0b671eb65810897366dd82c4cbb7d9dff8beda8484194956e81e89b8a361d9c7;
  bytes32 constant _sourceContractSlot = 0x27d542086d1e831d40b749e7f5509a626c3047a36d160781c40d5acc83e5b074;
  bytes32 constant _blockHeightSlot = 0x9172848b0f1df776dc924b58e7fa303087ae0409bbf611608529e7f747d55de3;
  bytes32 constant _defaultBpSlot = 0x3ab91e3c2ba71a57537d782545f8feb1d402b604f5e070fa6c3b911fc2f18f75;
  bytes32 constant _defaultReceiverSlot = 0xfd430e1c7265cc31dbd9a10ce657e68878a41cfe179c80cd68c5edf961516848;
  bytes32 constant _initializedPaidSlot = 0x33a44e907d5bf333e203bebc20bb8c91c00375213b80f466a908f3d50b337c6c;
  bytes32 constant _payoutAddressesSlot = 0x700a541bc37f227b0d36d34e7b77cc0108bde768297c6f80f448f380387371df;
  bytes32 constant _payoutBpsSlot = 0x7a62e8104cd2cc2ef6bd3a26bcb71428108fbe0e0ead6a5bfb8676781e2ed28d;
  bytes32 constant _lZEndpointSlot = 0x56825e447adf54cdde5f04815fcf9b1dd26ef9d5c053625147c18b7c13091686;
  bytes32 constant _baseGasSlot = 0x1eaa99919d5563fbfdd75d9d906ff8de8cf52beab1ed73875294c8a0c9e9d83a;
  bytes32 constant _gasPerByteSlot = 0x99d8b07d37c89d4c4f4fa0fd9b7396caeb5d1d4e58b41c61c71e3cf7d424a625;